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

Clear global_unique_id when restoring a product with an existing global_unique_id #50496

Merged
merged 2 commits into from
Aug 10, 2024
Merged

Clear global_unique_id when restoring a product with an existing global_unique_id #50496

merged 2 commits into from
Aug 10, 2024

Conversation

nathanss
Copy link
Contributor

@nathanss nathanss commented Aug 8, 2024

…unique id

Submission Review Guidelines:

Changes proposed in this Pull Request:

Clear global_unique_id when restoring a product with an existing global_unique_id

Closes #50442 .

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

(copied from original issue)

  1. Create a simple product with a GTIN, UPC, EAN or ISBN value of 012345678905. The GTIN, UPC, EAN or ISBN field can be found in the Inventory tab of the product editor.
  2. Trash that product.
  3. Create another simple product that also has the same GTIN, UPC, EAN or ISBN (012345678905).
  4. Restore the first product from trash.
  5. Verify that the field was cleared on the restored product

Changelog entry

  • Automatically create a changelog entry from the details below.
  • This Pull Request does not require a changelog entry. (Comment required below)
Changelog Entry Details

Significance

  • Patch
  • Minor
  • Major

Type

  • Fix - Fixes an existing bug
  • Add - Adds functionality
  • Update - Update existing functionality
  • Dev - Development related task
  • Tweak - A minor adjustment to the codebase
  • Performance - Address performance issues
  • Enhancement - Improvement to existing functionality

Message

Changelog Entry Comment

Comment

@nathanss nathanss self-assigned this Aug 8, 2024
@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Aug 8, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Aug 8, 2024

Hi @octaedro, @mattsherman, @louwie17, @mdperez86,

Apart from reviewing the code changes, please make sure to review the testing instructions and verify that relevant tests (E2E, Unit, Integration, etc.) have been added or updated as needed.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Aug 8, 2024 •
edited
Loading

Test using WordPress Playground

The changes in this pull request can be previewed and tested using a WordPress Playground instance.
WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Test this pull request with WordPress Playground.

Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit.

mattsherman
mattsherman suggested changes Aug 9, 2024
View reviewed changes
Copy link
Contributor

@mattsherman mattsherman left a comment

Choose a reason for hiding this comment

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

Tested and works as expected.

I think that the code should be made more robust by checking that the $product exists before calling methods on it.

plugins/woocommerce/includes/class-wc-post-data.php Outdated
*/
private static function clear_global_unique_id_if_necessary( $id ) {
$product = wc_get_product( $id );
if ( ! wc_product_has_global_unique_id( $id, $product->get_global_unique_id() ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be a check that $product exists before attempting to call methods on it.

@nathanss nathanss enabled auto-merge (squash) August 9, 2024 20:03
mattsherman
mattsherman approved these changes Aug 10, 2024
View reviewed changes
Copy link
Contributor

@mattsherman mattsherman 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 expected after re-testing with latest change. Nice job!

plugins/woocommerce/includes/class-wc-post-data.php
*/
private static function clear_global_unique_id_if_necessary( $id ) {
$product = wc_get_product( $id );
if ( $product && ! wc_product_has_global_unique_id( $id, $product->get_global_unique_id() ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for updating this.

@nathanss nathanss merged commit 9297409 into trunk Aug 10, 2024
26 checks passed
@nathanss nathanss deleted the update/clear-unique-id branch August 10, 2024 11:16
@github-actions github-actions bot added this to the 9.3.0 milestone Aug 10, 2024
@github-actions github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Aug 10, 2024
@nathanss nathanss mentioned this pull request Aug 13, 2024
Merged
12 tasks
@nigeljamesstevenson nigeljamesstevenson modified the milestones: 9.3.0, 9.2.0 Aug 13, 2024
github-actions bot pushed a commit that referenced this pull request Aug 13, 2024
@nathanss
Clear global_unique_id when restoring a product with an existing glob…
db33c80 
…al_unique_id (#50496)

* Clear global_unique_id when restoring a product that doesn't have an unique id

* Check if product exists before calling methods
This was referenced Aug 13, 2024
Merged
Merged
nigeljamesstevenson pushed a commit that referenced this pull request Aug 13, 2024
@github-actions @nathanss
Cherry pick 50496 into release/9.2 (#50645)
79d293a 
* Clear global_unique_id when restoring a product with an existing global_unique_id (#50496)

* Clear global_unique_id when restoring a product that doesn't have an unique id

* Check if product exists before calling methods

* Prep for cherry pick 50496

---------

Co-authored-by: Nathan Silveira <nsschneider1@gmail.com>
Co-authored-by: WooCommerce Bot <no-reply@woocommerce.com>
@rodelgc rodelgc added needs: external testing Indicates if the PR requires further testing conducted by testers external to the development team. status: analysis complete Indicates if a PR has been analysed by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattsherman mattsherman mattsherman approved these changes

@octaedro octaedro Awaiting requested review from octaedro

@louwie17 louwie17 Awaiting requested review from louwie17

@mdperez86 mdperez86 Awaiting requested review from mdperez86

Assignees

@nathanss nathanss

Labels
needs: external testing Indicates if the PR requires further testing conducted by testers external to the development team. plugin: woocommerce Issues related to the WooCommerce Core plugin. status: analysis complete Indicates if a PR has been analysed by Solaris
Projects
None yet
Milestone
9.2.0
Development

Successfully merging this pull request may close these issues.

Able to create two products with the same GTIN, UPC, EAN or ISBN value through trashing and restoring.
4 participants
@nathanss @mattsherman @rodelgc @nigeljamesstevenson