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

[GTIN] Add GTIN migration JOB #2645

Merged
merged 5 commits into from
Oct 28, 2024
Merged

Conversation

puntope
Copy link
Contributor

@puntope puntope commented Oct 18, 2024

Changes proposed in this Pull Request:

Closes part of #2616 as part of pcTzPl-2p2-p2.

This PR creates the logic for the Job responsible for the migration GTIN in batches for all the products.

Screenshots:

Screenshot 2024-10-17 at 18 29 40

Detailed test instructions:

  1. Set a simple product and add a GTIN in Google for WooCommerce tab
  2. Set a simple product and add a different GTIN in Google for WooCommerce tab and in the Inventory tab
  3. Set a Variable product and set GTIN on one of the variations in Google for WooCommerce only and in both (inventory and Google for WooCommerce) in other.
  4. Go to Connection Test page and click on "Start GTIN Migration"
  5. See the job gla/jobs/migrate_gtin/create_batch scheduled in WooCommerce - Status - Scheduled actions
  6. Run the job and see the gla/jobs/migrate_gtin/process_items created with the product ids.
  7. Run it
  8. Go back to the products created in steps 1,2,3 and verify that:
  9. For the product in step 1- the GTIN was loaded in the inventory GTIN
  10. For the product in step 2 - The GTIN is still the GTIN you set initially and is not override by the GTIN in google for WooCommerce GTIN
  11. Check the same in the variations (GTIN set in core should not be overrided in the migration).

Additional details:

In follow-up PRs I will add:

  • the banners for starting the migration
  • tests
  • WP CLI tool

Changelog entry

Add - Add GTIN Migration Job

@puntope puntope self-assigned this Oct 18, 2024
@github-actions github-actions bot added type: enhancement The issue is a request for an enhancement. changelog: add A new feature, function, or functionality was added. labels Oct 18, 2024
@puntope puntope changed the title Add/gtin migration Add GTIN migration JOB Oct 18, 2024
@puntope puntope requested a review from a team October 18, 2024 01:37
@tomalec tomalec self-requested a review October 18, 2024 22:10
Copy link
Member

@tomalec tomalec left a comment

Choose a reason for hiding this comment

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

I run the tests locally and for simple products it goes smooth.


I noticed a different behavior for simple products when the GTIN is already set to the same value or when it's set to a different one. Is that expected? if there are differences, it keeps the core GTIN but adds the G4W as SKU, while for the case where they're the same, it does not set SKU.
image

Plus, for variable ones, it didn't migrate the GTINs.
The process job was executed.
image
image

* @param int[] $items A single batch of WooCommerce product IDs from the get_batch() method.
*/
protected function process_items( array $items ) {
// todo: prevent execution if migration was completed?
Copy link
Member

Choose a reason for hiding this comment

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

❓ Is this something you are planning to add in the follow-up PRs, or was it missed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This is a todo and will be done in a further PR when setting the Banner for the merchant to run the job

@puntope
Copy link
Contributor Author

puntope commented Oct 19, 2024

I noticed a different behavior for simple products when the GTIN is already set to the same value or when it's set to a different one. Is that expected? if there are differences, it keeps the core GTIN but adds the G4W as SKU, while for the case where they're the same, it does not set SKU.

Hey @tomalec that's odd I was not able to reproduce this. Can you verify again or help me to reproduce this issue?

The expected behaviour is:

  • If GTIN is set in the inventory tab then migration does nothing.
  • If GTIN is not set in the inventory tab then migration loads the GTIN set in Google for WooCommerce tab (if it is set)

@puntope
Copy link
Contributor Author

puntope commented Oct 19, 2024

Plus, for variable ones, it didn't migrate the GTINs. The process job was executed.

The variables are required to process the Variations. I just added a tweak here because I notice I was processing only available variations (those with price set) Now I do get all the variations.

Notice the GTIN in the variable inventory is useless because we don't support GTIN in Variables. So you need to check each of the variations (they have their own GTIN fields in core part and G4W tab) .

@puntope puntope requested a review from tomalec October 19, 2024 00:31
Copy link

codecov bot commented Oct 19, 2024

Codecov Report

Attention: Patch coverage is 0% with 38 lines in your changes missing coverage. Please review.

Project coverage is 65.4%. Comparing base (a2c3654) to head (02e6224).
Report is 10 commits behind head on add/support-core-gtin-field.

Files with missing lines Patch % Lines
src/Jobs/MigrateGTIN.php 0.0% 27 Missing ⚠️
src/ConnectionTest.php 0.0% 6 Missing ⚠️
...ternal/DependencyManagement/JobServiceProvider.php 0.0% 5 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@                       Coverage Diff                       @@
##             add/support-core-gtin-field   #2645     +/-   ##
===============================================================
- Coverage                           65.5%   65.4%   -0.1%     
- Complexity                          4608    4622     +14     
===============================================================
  Files                                474     475      +1     
  Lines                              19299   19337     +38     
===============================================================
  Hits                               12638   12638             
- Misses                              6661    6699     +38     
Flag Coverage Δ
php-unit-tests 65.4% <0.0%> (-0.1%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ternal/DependencyManagement/JobServiceProvider.php 0.0% <0.0%> (ø)
src/ConnectionTest.php 0.0% <0.0%> (ø)
src/Jobs/MigrateGTIN.php 0.0% <0.0%> (ø)

@puntope
Copy link
Contributor Author

puntope commented Oct 19, 2024

Hey @tomalec I addressed all your comments and questions. Can you follow up with another review?

@puntope puntope changed the title Add GTIN migration JOB [GTIN] Add GTIN migration JOB Oct 19, 2024
Copy link
Member

@tomalec tomalec left a comment

Choose a reason for hiding this comment

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

Thanks for changes and clarifications, now it works well for me :)

@puntope puntope merged commit 1b060aa into add/support-core-gtin-field Oct 28, 2024
12 checks passed
@puntope puntope deleted the add/gtin-migration branch October 28, 2024 16:28
@ianlin ianlin mentioned this pull request Nov 26, 2024
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: add A new feature, function, or functionality was added. type: enhancement The issue is a request for an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants