[GTIN] Migration notice #2656
[GTIN] Migration notice #2656
Conversation
github-actions
bot
added
type: enhancement
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## add/gtin-migration-api-controller #2656 +/- ##
=====================================================================
- Coverage 65.3% 64.7% -0.6%
- Complexity 4628 4638 +10
=====================================================================
Files 476 798 +322
Lines 19379 24536 +5157
Branches 0 1242 +1242
=====================================================================
+ Hits 12661 15875 +3214
- Misses 6718 8488 +1770
- Partials 0 173 +173
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Thanks for the changes, they are working as expected. I just left a comment about checking if GTIN is supported in core. Also as we discussed we might want to give some more feedback on when the task is completed.
The code looks good to me, but we might just want to get a quick lookover by someone a bit more familiar with react to confirm the handling of the modals is done correctly.
|
Hi @mikkamp I added several tweaks. Now we do show 2 banners depending on the status of the migration. I also refactored some code to prevent duplication. So for that, I created a Utility class to use as a trait. Can you take another look? ps. Notice I bumped min version to hide the GTIN to 2.8.7 |
There was a problem hiding this comment.
Thanks for the changes, although it seems that it's fully relying on a status that is set on the page (meaning I have to reload the page to transition the banner status).
For the banner to completely clear it's not such a problem to have it stay until the page is refreshed. But for the transition between banners it looks kind of odd. When I start the migration I'm shown the new banner:
But if I switch to any other tab like Product Feed, or even go to another react page in WooCommerce it reverts back to the first banner (until I hard refresh the page):
Can we get that status updated dynamically so it keeps the status current?
js/src/components/gtin-migration-banner/gtin-migration-banner.js
Outdated
Show resolved
Hide resolved
|
Hey @mikkamp Thanks for the new review. I did some changes fixing the issue with the banner not changing between tabs. I did also rename the value of the constants. This is ready for another round. Thanks |
There was a problem hiding this comment.
Thanks for the changes, the banners are working correctly now since the status is fetched and kept up to date. I just left some minor comments.
One thing that I think should still be addressed is handling duplicate GTIN numbers. The action scheduler doesn't handle that too well, because since they are done in batches, if one product fails the whole batch fails. It tries to redo the tasks until it reaches the failure threshold.
Anyways I'll go ahead and approve this PR if you'd like to handle the fatal in a follow up PR.
| @@ -89,6 +95,22 @@ protected function start_migration_callback(): callable { | |||
| }; | |||
| } | |||
|
|
|||
| /** | |||
| * Callback function for scheduling GTIN migration job. | |||
There was a problem hiding this comment.
Seems like a copy paste error, but this is not the right description. Also, would it make more sense to call the function get_migration_status_callback?
Changes proposed in this Pull Request:
Closes part of #2616 as part of pcTzPl-2p2-p2.
Related to #2655
Screenshots:
Screen.Recording.2024-10-29.at.20.44.24.mov
Detailed test instructions:
localStorage.setItem( 'debug', 'wc-admin:*' )wcadmin_gla_gtin_migration_banner_clickwcadmin_gla_modal_openwcadmin_gla_modal_closedwcadmin_gla_gtin_migration_banner_migration_startwcadmin_gla_gtin_migration_banner_migration_scheduledgla/jobs/migrate_gtin/create_batchis scheduled. Dont run itwcadmin_gla_gtin_migration_banner_migration_failedAdditional details:
Changelog entry