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

Add default date range to Delivery Report #4260

Conversation

jasonk357
Copy link
Contributor

What? Why?

Closes #4184

This adds a default date range, the most recent month, to the parameters sent to the OrderCycleManagementReport.

I think putting the work here in a background job would make more sense but given the context maybe that isn't necessary?

What should we test?

Delivery report, payment methods report, with and without dates.

Release notes
Assigns a default date range from one month ago to today for the delivery report and payment methods report.

Changelog Category: Fixed

Woo first OFN PR!

Copy link
Contributor

@luisramos0 luisramos0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome Jason, welcome to OFN!!!

Copy link
Contributor

@sauloperez sauloperez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome @jasonk357 ! Welcome onboard 🎉

I think putting the work here in a background job would make more sense but given the context maybe that isn't necessary?

That's very interesting. I recently got a similar suggestion from someone else. What if we unblocked the app server by performing it async and send an email to the user when done?

@filipefurtad0 filipefurtad0 self-assigned this Sep 17, 2019
@sigmundpetersen sigmundpetersen added the pr-staged-fr staging.coopcircuits.fr label Sep 17, 2019
@filipefurtad0
Copy link
Contributor

Hi @jasonk357, welcome :-)

The default date and time appears now, for both report types - thank you.

The "now" button on the calendar provides the date/time for the local system in use and not the actual hub, which might lead to some discrepancies when these are not on the same timezone. I am wondering whether the "now" button should provide the timestamp of the hub instead?

You can find details on
https://docs.google.com/document/d/1Fq7Dtc_ESzHTGnewJhfqlRx4M6r4ISMRb2mCHCE7fNM/edit#

@filipefurtad0
Copy link
Contributor

Hi @sigmundpetersen 🙂 I see you've staged this on FR previously. I've tested it and it seems ok, should I move it to Ready to Go for further review, or are you still checking something?

@sigmundpetersen
Copy link
Contributor

Hey @filipefurtad0 I just added the label after you staged it 😉 Please move to ready if you are happy with the test results 👍

@filipefurtad0 filipefurtad0 removed the pr-staged-fr staging.coopcircuits.fr label Sep 19, 2019
@sauloperez sauloperez merged commit baa4783 into openfoodfoundation:master Sep 19, 2019
@sauloperez
Copy link
Contributor

Congratulations @jasonk357 and thanks for this contribution! 🎉

@sauloperez
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a default date range for Delivery Report
6 participants