-
Notifications
You must be signed in to change notification settings - Fork 136
[Woo POS][Local Catalog] Refine sync status checker states #14894
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
Conversation
Refactors the local catalog full sync logic by replacing the `Overdue` state with a more descriptive `NonBlockingRequired` state. The new `NonBlockingRequired` state includes a boolean flag, `isOverdue`, to indicate when the sync has passed the 7-day threshold, making states more granular.
Introduced a `DateTimeProvider` mock to ensure consistent and predictable time-based calculations within the tests. This change replaces `System.currentTimeMillis()` with the mock's `now()` method, improving the reliability and determinism of the test suite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request refactors the sync requirement logic to introduce a more granular distinction between "overdue" and "required but not yet overdue" sync states. It replaces the previous Overdue state with a new NonBlockingRequired state that includes an isOverdue boolean flag, allowing the system to differentiate between syncs that are simply due (between 2-7 days) versus those that are overdue (>7 days).
Key changes:
- Introduced a
DateTimeProviderdependency and fixed timestamp in tests to eliminate flaky time-dependent test behavior - Replaced
WooPosFullSyncRequirement.OverduewithNonBlockingRequired(lastSyncTimestamp, isOverdue)to support intermediate sync states - Added a new
FULL_SYNC_NOT_REQUIRED_THRESHOLD(2 days) to complement the existingFULL_SYNC_OVERDUE_THRESHOLD(7 days)
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| WooPosFullSyncStatusChecker.kt | Refactored sync requirement logic to use NonBlockingRequired with isOverdue flag; injected DateTimeProvider for testability; added threshold for "not required" state |
| WooPosFullSyncStatusCheckerTest.kt | Replaced System.currentTimeMillis() with mocked DateTimeProvider and constant NOW for deterministic testing; updated assertions for new NonBlockingRequired state |
| WooPosProductsDataSourceTest.kt | Updated test to use NonBlockingRequired instead of Overdue |
| WooPosProductsDataSource.kt | Updated pattern matching to use NonBlockingRequired instead of Overdue |
| WooPosLocalCatalogSyncWorker.kt | Updated pattern matching to use NonBlockingRequired instead of Overdue |
| WooPosItemsViewModel.kt | Added conditional logic to show banner only when isOverdue is true within NonBlockingRequired state |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📲 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.
|
malinajirka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @samiuelson ! The approach looks solid, I've left a note about the threshold.
| companion object { | ||
| private val FULL_SYNC_OVERDUE_THRESHOLD = 7.days | ||
| private val FULL_SYNC_OVERDUE_THRESHOLD = 7.days.inWholeMilliseconds | ||
| private val FULL_SYNC_NOT_REQUIRED_THRESHOLD = 2.days.inWholeMilliseconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 Two notes here
- According to the test plan, we should run periodic full sync every 24 hours. So we should either update the test plan and sync with iOS folks or change this to one-ish day.
- If we set it exactly to 1 day, it won't work either -> the worker might be run some time before the exact 24 hours, that's up to to the system to determine when to run the worker => I'd set it to 23 hours.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I changed FULL_SYNC_NOT_REQUIRED_THRESHOLD to 23 hours: 309cbbc
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #14894 +/- ##
============================================
- Coverage 38.27% 38.27% -0.01%
+ Complexity 10103 10102 -1
============================================
Files 2137 2137
Lines 121017 121024 +7
Branches 16584 16587 +3
============================================
Hits 46318 46318
- Misses 69985 69991 +6
- Partials 4714 4715 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
💡 Don't merge until parent branch is
trunkDescription
This pull request refactors the sync requirement logic to introduce a more granular distinction between "overdue" and "required but not yet overdue" sync states. It replaces the previous
Overduestate with a newNonBlockingRequiredstate that includes anisOverdueboolean flag, allowing the system to differentiate between syncs that are simply due (between 2-7 days) versus those that are overdue (>7 days).Key changes:
WooPosFullSyncRequirement.OverduewithNonBlockingRequired(lastSyncTimestamp, isOverdue)to support intermediate sync statesFULL_SYNC_NOT_REQUIRED_THRESHOLD(2 days) to complement the existingFULL_SYNC_OVERDUE_THRESHOLD(7 days)Test Steps
N/A
Images/gif
N/A
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.