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

Set paid and completed dates #143

Merged
merged 2 commits into from
Aug 27, 2024
Merged

Conversation

mikkamp
Copy link
Contributor

@mikkamp mikkamp commented Aug 14, 2024

All Submissions:

Changes proposed in this Pull Request:

When we generate orders we are able to generate them over a range of dates. This works correctly when setting the order creation date. However other dates will default to the current date.

So if we generate a set of orders for the Analytics reports the date_paid will all be at the time of when the order was generated. Which isn't very useful for populating some test data for these reports.

This PR changes that behaviour and allows setting both the paid date as well as the order completed date. This is set based on the chosen status of the order. Paid date is set for both processing and completed. Completed date is set for only completed orders. The date is set by adding a random amount of 0 to 36 hours to the creation date.

How to test the changes in this Pull Request:

  1. Generate a set of orders in WP-CLI, for January wp wc generate orders 10 --date-start=2024-01-01 --date-end=2024-01-31
  2. Go to Analytics > Orders
  3. Set a custom date range to view all orders from January
  4. Confirm that we are seeing orders within the report (with a date of January and not the date we generate the orders)
    image

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?

Changelog entry

Set paid and completed dates based on order status.

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

@mikkamp mikkamp requested review from a team and naman03malhotra and removed request for a team August 14, 2024 12:51
Copy link
Contributor

@coreymckrill coreymckrill left a comment

Choose a reason for hiding this comment

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

👍 Tests well!

includes/Generator/Order.php Outdated Show resolved Hide resolved
@mikkamp
Copy link
Contributor Author

mikkamp commented Aug 20, 2024

@coreymckrill Thanks for the quick review. I went ahead and added that offset to the payment date as well.

Looking in the DB table for wc_order_stats I can now see the last 10 orders have a random offset for the payment date:
image

Would you mind taking one more quick look that all is well?

@mikkamp mikkamp requested a review from coreymckrill August 20, 2024 15:15
@coreymckrill
Copy link
Contributor

@mikkamp looks good! Another thing just occurred to me, though. In some cases, adding up to 36 hours to the paid/completed date could end up making those dates be in the future. That doesn't seem great to me, but I could be convinced otherwise.

If we want to prevent that, we could potentially do something like this:

min( strtotime( $date ) + ( wp_rand( 0, 36 ) * HOUR_IN_SECONDS ), time() )

@mikkamp
Copy link
Contributor Author

mikkamp commented Aug 26, 2024

Another thing just occurred to me, though. In some cases, adding up to 36 hours to the paid/completed date could end up making those dates be in the future. That doesn't seem great to me, but I could be convinced otherwise.

That is true, but considering that we are also able to generate orders in the future, with something like:
wp wc generate orders 10 --date-start=2024-10-01 --date-end=2024-10-30

I didn't think it would be such a big deal to have payment dates in the future. If they would really want to prevent that then they could always specify an older date range to prevent it from getting anything in the future.

Copy link
Contributor

@coreymckrill coreymckrill left a comment

Choose a reason for hiding this comment

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

Fair enough! Thanks for the PR

@coreymckrill coreymckrill merged commit 2702baf into trunk Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants