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

Refactor & fix undesired exceptions in AS Jobs #2666

Open
wants to merge 1 commit into
base: add/support-core-gtin-field
Choose a base branch
from

Conversation

puntope
Copy link
Contributor

@puntope puntope commented Nov 8, 2024

Changes proposed in this Pull Request:

Closes part of #2617

  • This PR adds some try catch preventing AS to fail when saving the GTIN
  • Additionally this PR refactors some of the common methods to make the code cleaner

Screenshots:

Screenshot 2024-11-08 at 19 56 48

Detailed test instructions:

  1. Run the GTIN migration with the next GTINs
  2. GTINs with dashes but 8 numbers
  3. GTINs with other chars like dots . or letters
  4. GTIN that also has the GTIN saved in Product Inventory for that product
  5. Duplicated GTIN
  6. GTINS in variations
  7. See the AS doesn't fail and you can see some logs also.

Additional details:

Changelog entry

Fix - Prevent fatal in GTIN MIgration AS Job

@puntope puntope requested a review from a team November 8, 2024 19:09
@puntope puntope self-assigned this Nov 8, 2024
@github-actions github-actions bot added type: bug The issue is a confirmed bug. changelog: fix Took care of something that wasn't working. labels Nov 8, 2024
@puntope puntope marked this pull request as ready for review November 8, 2024 19:11
Copy link

codecov bot commented Nov 8, 2024

Codecov Report

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

Project coverage is 64.9%. Comparing base (8b7c586) to head (734a77a).
Report is 14 commits behind head on add/support-core-gtin-field.

Files with missing lines Patch % Lines
src/Jobs/MigrateGTIN.php 0.0% 16 Missing ⚠️
src/HelperTraits/GTINMigrationUtilities.php 0.0% 12 Missing ⚠️
src/Utility/WPCLIMigrationGTIN.php 0.0% 10 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@                       Coverage Diff                       @@
##             add/support-core-gtin-field   #2666     +/-   ##
===============================================================
+ Coverage                           64.7%   64.9%   +0.2%     
- Complexity                          4638    4667     +29     
===============================================================
  Files                                798     478    -320     
  Lines                              24536   19517   -5019     
  Branches                            1242       0   -1242     
===============================================================
- Hits                               15875   12674   -3201     
+ Misses                              8488    6843   -1645     
+ Partials                             173       0    -173     
Flag Coverage Δ
js-unit-tests ?
php-unit-tests 64.9% <0.0%> (-0.4%) ⬇️

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

Files with missing lines Coverage Δ
src/Utility/WPCLIMigrationGTIN.php 0.0% <0.0%> (ø)
src/HelperTraits/GTINMigrationUtilities.php 29.6% <0.0%> (-23.7%) ⬇️
src/Jobs/MigrateGTIN.php 0.0% <0.0%> (ø)

... and 322 files with indirect coverage changes

Copy link
Contributor

@mikkamp mikkamp 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 the changes, they are looking good. I just left a few minor nitpicks but the migration is working. I can confirm that I see log messages, including an error logged for a duplicate GTIN:

image

* @param string $gtin
* @return string
*/
protected function error_gtin_invalid( \WC_Product $product, string $gtin ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal, and we don't seem to be consistent. But in most cases in the code we add use WC_Product to be able to remove the \. But I'm not attached to either method so both are fine.


/**
* Add some info in the debug console.
* Add --debug to see this kind of logs in WP CLI
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a small nitpick about the wording (this should be plural):

Suggested change
* Add --debug to see this kind of logs in WP CLI
* Add --debug to see these logs in WP CLI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: fix Took care of something that wasn't working. type: bug The issue is a confirmed bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants