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.

Sign up for GitHub

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

Jump to bottom

feat(wc): duplicate orders admin notice #3555

Merged
merged 10 commits into from
Dec 4, 2024
Merged

feat(wc): duplicate orders admin notice #3555

merged 10 commits into from
Dec 4, 2024

Conversation

adekbadek
Copy link
Member

@adekbadek adekbadek commented Nov 18, 2024 •
edited by leogermani
Loading

All Submissions:

Changes proposed in this Pull Request:

Adds an admin notice when duplicate orders are detected:

image

How to test the changes in this Pull Request:

  1. Donate two times with the same amount, within 10 minutes
  2. Do that again, but change the dates of these two to more than a month ago
  3. Trigger the CRON job: wp cron event run newspack_wc_check_order_duplicates
  4. Visit the admin panel, observe the admin notice about potentially duplicated orders – from the last month (the first two created)
  5. Dismiss one of the cases, observe the page reloads and the list item is no longer displayed
  6. Run wp newspack detect-order-duplicates --cutoff_time='1 year' --save and refresh the admin page
  7. Observe three items are present on the list: two new ones from more than a month ago

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 ran tests with your changes locally?

@adekbadek adekbadek marked this pull request as ready for review November 19, 2024 14:52
@adekbadek adekbadek requested a review from a team as a code owner November 19, 2024 14:52
@adekbadek adekbadek added the [Status] Needs Review The issue or pull request needs to be reviewed label Nov 19, 2024
leogermani
leogermani reviewed Nov 20, 2024
View reviewed changes
Copy link
Contributor

@leogermani leogermani left a comment

Choose a reason for hiding this comment

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

Even though the SQL queries are pretty efficient, they are huge, hard to read and hard to maintain.

And also hard to test, because I need two different sites to test it.

Doing this with simple code, looping though recent orders would be less efficient, but much more readable, maintainable and testable... And considering that this check runs daily, we don't need to look at all the orders in the history, just the most recent ones.

@adekbadek
Copy link
Member Author

Yeah, that makes sense. I've updated with the following:

  1. Instead of SQL query, iterate order objects retrieved with wc_get_orders (in batches of 100, so the site does not fall over)
  2. Added a wp newspack detect-order-duplicates CLI command to enable filling in historical data (older than a month). This will have to be ran on live sites after the PR is deployed.

Update the PR description accordingly.

@adekbadek adekbadek requested a review from leogermani November 28, 2024 11:57
leogermani
leogermani reviewed Nov 28, 2024
View reviewed changes
Copy link
Contributor

@leogermani leogermani left a comment

Choose a reason for hiding this comment

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

Works as described! Thanks for the changes! I left a few minor comments.

I just noticed one thing. I first ran the CLI command without the --save param. It detected the duplicated but didn't save. However, it ended up displaying the admin notice, but with no orders listed. Just the summary. When you clicked, it was an empty list.

Another question: Since we're running this daily, and cutting off based on completion date, why default to check the last 30 days? Looks like we will be doing a lot of unnecessary checks every day. Isn't it enough to check just the last one or two days?

includes/reader-revenue/woocommerce/class-woocommerce-duplicate-orders.php Outdated
}

/**
* CLI handler to upsert the order duplicates with a specified timeframe.
Copy link
Contributor

Choose a reason for hiding this comment

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

is it detect or upsert?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a clearer description of what the command does

  • search for duplicates and optionally store this info to be displayed in the admin panel

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in 520f49c

includes/reader-revenue/woocommerce/class-woocommerce-duplicate-orders.php
*
* ## EXAMPLES
*
* wp newspack detect-order-duplicates --cutoff_time='2 months' --save
Copy link
Contributor

Choose a reason for hiding this comment

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

is it detect or upsert?

Copy link
Member Author

Choose a reason for hiding this comment

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

Without --save, it's only detection. With it, it's upsertion.

@adekbadek
Copy link
Member Author

However, it ended up displaying the admin notice, but with no orders listed.

Yeah, I failed to account for an empty state, thanks for catching that! Fixed in 520f49c

Isn't it enough to check just the last one or two days?

That's right, changed to a day in 520f49c

@adekbadek adekbadek requested a review from leogermani November 29, 2024 08:04
@github-actions github-actions bot added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Nov 29, 2024
@leogermani leogermani merged commit cb764e3 into trunk Dec 4, 2024
9 checks passed
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 4, 2024

Hey @adekbadek, good job getting this PR merged! 🎉

Now, the needs-changelog label has been added to it.

Please check if this PR needs to be included in the "Upcoming Changes" and "Release Notes" doc. If it doesn't, simply remove the label.

If it does, please add an entry to our shared document, with screenshots and testing instructions if applicable, then remove the label.

Thank you! ❤️

@leogermani leogermani mentioned this pull request Dec 6, 2024
Merged
6 tasks
chickenn00dle pushed a commit that referenced this pull request Dec 12, 2024
@adekbadek @chickenn00dle
feat(wc): duplicate orders admin notice (#3555)
b6b7c85 
* feat(wc): duplicate orders admin notice

* feat: handle HPOS

* feat: handle order series dismissal

* feat: better UX with a details element

* feat: iterate orders instead of using a direct DB query

* chore: unify wording

* fix: handle the order meta

* feat: CLI tool

* feat: process orders in batches

* feat: tweak
matticbot pushed a commit that referenced this pull request Dec 12, 2024
@semantic-release-bot
chore(release): 5.10.0-alpha.1 [skip ci]
222aee9 
# [5.10.0-alpha.1](v5.9.1...v5.10.0-alpha.1) (2024-12-12)

### Bug Fixes

* duplicate orders save on cron ([#3604](#3604)) ([ec69167](ec69167))
* **ras-acc:** re-add recaptcha to the WooCommerce checkout ([#3605](#3605)) ([f42a75b](f42a75b))
* **ras:** do not require Woo plugins if using NRH ([#3614](#3614)) ([363a834](363a834))
* **wcs:** remove subscriptions expiration feature flag ([#3618](#3618)) ([7c175d9](7c175d9))
* **wcs:** update subscription expiration feature ([#3613](#3613)) ([ebf6e6d](ebf6e6d))
* **wcs:** update subscriptions expiration cli behavior ([#3617](#3617)) ([07e768c](07e768c))

### Features

* **subscriptions:** add cancellation reason metadata ([#3568](#3568)) ([de83e02](de83e02))
* **wc:** duplicate orders admin notice ([#3555](#3555)) ([cb764e3](cb764e3))
* **wcs:** add expired subscription cli tool ([#3593](#3593)) ([5d39398](5d39398))
* **webhooks:** filter request priority ([#3587](#3587)) ([1928a6a](1928a6a))
* **woocommerce-subscriptions:** add url redirect for wc subscription renewals ([#3525](#3525)) ([5b14aeb](5b14aeb))

### Reverts

* Revert "feat: command to initialize cron job to slowly backfill CAP term data (#3425)" (#3620) ([c9a9d45](c9a9d45)), closes [#3425](#3425) [#3620](#3620)
@matticbot
Copy link
Contributor

🎉 This PR is included in version 5.10.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leogermani leogermani leogermani approved these changes

Assignees
No one assigned
Labels
released on @alpha [Status] Approved The pull request has been reviewed and is ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adekbadek @matticbot @leogermani