[WOOMOB-1334] Execute Product and Variation sync in single transaction #14857
Conversation
Generated by 🚫 Danger |
malinajirka
added
the
status: do not merge
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
…both-products-and-variations-within-v2-step3
samiuelson
left a comment
samiuelson
left a comment
There was a problem hiding this comment.
LGTM! I left a couple of nonblocking comments.
| } | ||
|
|
||
| return result | ||
| val syncDuration = System.currentTimeMillis() - startTime |
There was a problem hiding this comment.
💡 n.p.: I'm wondering if it would be better to inject the current time via DateTimeProvider and replace all 3 System::currentTimeMillis calls with DateTimeProvider::now already. Currently, it's not a big deal since we are not even asserting the syncDurationMs value in tests, but it can avoid making tests non-deterministic by accident in the future.
| private val logger: WooPosLogWrapper, | ||
| ) { | ||
| suspend fun execute( | ||
| private suspend fun executeDatabaseTransaction( |
There was a problem hiding this comment.
💡 n.p.: The executeDatabaseTransaction name is slightly misleading, because it suggests it's some general-use DB transaction executor. Could it be something like updateDatabaseWithFetchedItems?
malinajirka
removed
the
status: do not merge
malinajirka
deleted the
issue/woomob-1334-woo-poslocal-catalog-run-both-products-and-variations-within-v2-step3
branch
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #14857 +/- ##
============================================
- Coverage 38.26% 38.26% -0.01%
- Complexity 10077 10085 +8
============================================
Files 2131 2131
Lines 120744 120701 -43
Branches 16547 16537 -10
============================================
- Hits 46207 46189 -18
+ Misses 69840 69816 -24
+ Partials 4697 4696 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
#WOOMOB-1334
Do not merge label - ensure the target branch is trunk before merging.
Description
This is the final PR in the chain of PRs that aim to ensure that both product and variation sync is done within a single transaction.
This PR is quite big - but the changes are so related that I couldnt' find a way how to split it further. Essentially we are removing WooPosSyncProductAction and WooPosSyncVariationAction and all the logic is done within WooPosSyncAction (since we removed the classes, we also need to remove all their tests and introduce new tests for WooPosSyncAction => that's the main reason for the large PR).
Considering this feature is still behind a FF and this PR doesn't touch any code outside of Local Catalog, I believe the change is safe to make.
Test Steps
Verify both initial local catalog sync as well as incremental sync work.
Images/gif
N/A
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.