Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mage_Report: Migrate hardcoded cron expressions to configurable field… #2595

Closed
wants to merge 6 commits into from

Conversation

sreichel
Copy link
Contributor

@sreichel sreichel commented Sep 13, 2022

Description (*)

Unable to rebase #1869 i opened a new PR. Thanks @Sekiphp.

Related Pull Requests

  1. Closes Mage_Report: Migrated hardcoded cron expressions to configurable fields #1869
  2. See [backport] Aggregate most viewed products report daily via a Cron job (#1829) #2610

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)

@github-actions github-actions bot added Component: Reports Relates to Mage_Reports Component: Sales Relates to Mage_Sales Component: SalesRule Relates to Mage_SalesRule Component: Tax Relates to Mage_Tax labels Sep 13, 2022
@sreichel sreichel marked this pull request as draft September 13, 2022 02:55
@addison74
Copy link
Contributor

You may close the other one. There is no rush to approve it. I have a few comments related to that PR.

@elidrissidev
Copy link
Member

Note that Most Viewed cron job is not available in v19, since #1829 was merged to 20.0.

@sreichel
Copy link
Contributor Author

Then we should port it to 1.9.

@addison74
Copy link
Contributor

You can do a cherry pick, there are only two commits.

@sreichel
Copy link
Contributor Author

You may close the other one. There is no rush to approve it. I have a few comments related to that PR.

I've read your comments ...

  • removed "Cron Definition" prefix, but also
  • reverted "Cron Settings" to lowercase

ToDo: translation csv

@sreichel
Copy link
Contributor Author

@fballiano why force-pushing ... ???

@fballiano
Copy link
Contributor

this does it

Schermata 2022-09-23 alle 20 55 54

@elidrissidev
Copy link
Member

FYI there's no need to update the branch unless it depends on a commit that was just merged, or if it has a conflict.

@sreichel
Copy link
Contributor Author

I'd prefer merge for already shared commits/PRs ...

@fballiano
Copy link
Contributor

You prefer merge but in the past i was told not to use merge cause it creates another commit so you know.. whatever.

@sreichel
Copy link
Contributor Author

@fballiano
Copy link
Contributor

Again, not my opinion and there is no official rule for that.
Anyway github checks for conflicts before so at least the first half of the article is not really applicable.
I try to follow what maintainers tell, and for maaaany things there are no rule, but then everybody complains anyway.

@sreichel
Copy link
Contributor Author

Again, not my opinion and there is no official rule for that.

This is why i just said i'd prefer .... whatever.

@fballiano
Copy link
Contributor

It’s confusing and frustrating to get told different things when you just want to follow and honor previously made work.
A lot of things should be decided by maintainers and told in a clear way to the others.
When you comment like “why did you do that” with three exclamation marks it’s like the other person did such a stupid mistake, and it’s simply not the case.

@sreichel
Copy link
Contributor Author

@fballiano big sorry. It was not meant rude. I justed wondered why "force-push" is used since some time. If someone suggested to use this ... okay.

@sreichel sreichel closed this by deleting the head repository Jan 8, 2023
@addison74
Copy link
Contributor

@sreichel - Why did you close this PR?

@sreichel
Copy link
Contributor Author

sreichel commented Jan 8, 2023

It was closed after deleting my fork.

@addison74
Copy link
Contributor

10 PRs closed instantly. Will we miss you again?

@sreichel
Copy link
Contributor Author

sreichel commented Jan 9, 2023

Dont know yet ... depends on @Flyingmana.

One thing is for sure this time, there is no going back.

@fballiano
Copy link
Contributor

@sreichel I think I can speak for some of us here and we love and appreciate what you did all this time, it is in immense work. I sincerely hope you may want to stay for all of us instead of leaving for one. I'll write you tomorrow morning.

@kiatng
Copy link
Contributor

kiatng commented Jan 9, 2023

@sreichel https://open.spotify.com/track/5WUgOOaTk00PnksDekcgcg?si=2f76306137f84076

@sreichel
Copy link
Contributor Author

sreichel commented Jan 20, 2023

5zm1j9

... not this time. Good luck guys.

(at Daniel ... last year i retiered for same reason. You claim to be to the most important, doing things that nobody sees - but not doing things that everybody sees. When i asked for the sponsorship-button you was the first and only one who wanted to get listed ... 🖕 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Reports Relates to Mage_Reports Component: Sales Relates to Mage_Sales Component: SalesRule Relates to Mage_SalesRule Component: Tax Relates to Mage_Tax
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants