Mage_Report: Migrated hardcoded cron expressions to configurable fields #1869
Conversation
github-actions
bot
added
Component: Reports
|
Nice one! |
midlan
added
hacktoberfest-accepted
work in progress
|
This is a very good idea. I totally agree to move the hardcoded values from XML files to Magento Configuration. The same modification should be done for Price Alerts and other jobs that only run when time is 00:00. At least for Price Alerts is funny to wait 10 - 12 hours till Magento sends the notifications. I posted in Discussion more thoughts about this issue. I fixed it by creating a custom module. |
|
The related PR is merged, you may now rebase and comment out the other changes |
There was a problem hiding this comment.
Can you add the new translations in CSV files?
Can you add a dot at the end of comments?
Can you add in comments the default value?
Edit: since, #2148, can you remove <frontend_type>text</frontend_type> for added configuration?
fballiano
force-pushed
the
mage-reports/cron-expr-configurable
branch
from
d815035
to
7a97c01
Compare
rebased |
|
I would revise the labels and comments for this PR. @luigifab also requested a few good changes. Once we agree the labels and comments we can add the translation phrases. |
|
I've removed the frontend_type and uncommented the commented part of the xml |
This point still has to be done |
|
I'm also waiting for the small change requested from "Cron settings" to "Cron Settings" but I have to insist because it is left to wait.. |
|
I did it for you (you could do it anyway since this PR is modificable) |
|
Indeed, I can create a commit with the change. Good tip. Before adding new translations, it would be good to discuss their wording. Since we are in a cron setting section I don't think there is a need for "Cron definition -" prefix. Bestsellers is just fine. Maybe the labels under the boxes need to be analyzed, maybe even removed. |
|
Can this be merged to 19.x? |
sreichel
force-pushed
the
mage-reports/cron-expr-configurable
branch
from
72fc905
to
6cf2b2e
Compare
|
I'm to stupid to rebase this. :( How to change target version for PRs? |
|
To v19? |
|
Yes. |
|
I guess you tries already editing the PR with the github web and broke everything right? |
|
Tried different things ... last error as i tried to update upstream repo .... It would take 2 min to edit 7 files manually ... lol. |
Your personal access token is lacking the |
|
Sven created another PR here #2595 that had all the changes, but he chose to close it. I allowed myself to make the necessary changes to move forward. Here you can see the image as it looks in the Backend.
In my opinion, it looks fine. |
|
The interface looks good now. See the screenshot above. The only issue I would like to discuss before approving is related to the "Most Viewed" field. Shouldn't we set up a default value for it? From what I can see, it takes the value from this PR #1829, but I think it's better to put a value in the box, similar to the others. |
|
@ADDISON74 I implemented this change also for the most_viewed report: 
;-) |
|
We can merge this PR, it works as expected. I changed all values to
cron_schedule table has a lot of records like aggregate_
By reloading the "Refresh Statistics" page on the "Update At" column the time is changing
|
fballiano
changed the title
|
Shall we mention for every field in comment the default value 0 0 * * *? I have to take a look in the Backend for similar fields. |
Description (*)
Replaced hardcoded cron expressions to system configurable with same default values. Please, see the image below:
Related Pull Requests
#1829
Contribution checklist (*)