-
Notifications
You must be signed in to change notification settings - Fork 136
[WOOMOB-1446] Implement cellular data preference for WooPos local catalog sync #14725
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
[WOOMOB-1446] Implement cellular data preference for WooPos local catalog sync #14725
Conversation
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
…-downloadovercellular-preference-in # Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/settings/details/localcatalog/WooPosSettingsLocalCatalogViewModel.kt
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 PR implements cellular data preference functionality for WooPos local catalog synchronization, allowing users to restrict background sync operations to WiFi only to prevent unexpected data usage charges.
Key changes:
- Added a new preference setting to control cellular data usage for sync operations
- Updated WorkManager constraints to respect user preference (WiFi-only or any connection)
- Integrated the preference toggle into the local catalog settings UI
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| WooPosPreferencesRepository.kt | Adds storage and retrieval methods for the cellular data preference |
| WooPosSettingsLocalCatalogViewModel.kt | Integrates preference loading/saving with UI state and triggers constraint updates |
| WooPosLocalCatalogSyncScheduler.kt | Updates WorkManager constraints based on user preference and handles work rescheduling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...ain/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncScheduler.kt
Outdated
Show resolved
Hide resolved
...ain/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncScheduler.kt
Outdated
Show resolved
Hide resolved
...ain/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncScheduler.kt
Outdated
Show resolved
Hide resolved
70bb768 to
12bed58
Compare
…-downloadovercellular-preference-in
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #14725 +/- ##
============================================
- Coverage 38.43% 38.42% -0.02%
Complexity 9887 9887
============================================
Files 2105 2105
Lines 117088 117122 +34
Branches 15646 15646
============================================
Hits 45001 45001
- Misses 67914 67948 +34
Partials 4173 4173 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| workManager.enqueueUniquePeriodicWork( | ||
| WooPosLocalCatalogSyncWorker.WORK_NAME, | ||
| ExistingPeriodicWorkPolicy.UPDATE, |
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.
📓 I replaced Keep with Update and wrapped the block in coroutine.
| lastFullUpdate = formattedTimestamp // TBD local catalog: Replace with full sync timestamp | ||
| ) | ||
|
|
||
| val allowCellularDataUpdate = preferencesRepository.allowCellularDataUpdate.first() |
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.
Consider making the cellular preference fully reactive by continuously listening to the repository instead of reading it once at startup. This way the UI state always stays in sync with the stored
preference, and the Switch can pass its new value directly rather than the ViewModel toggling the current state.
Something like that. Wdyt?
Refactor_cellular_data_update_handling_to_pass_boolean_value_and_improve_state_management.patch
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 for the suggestion @kidinov! I'm wondering what is the benefit of observing, since the settings screen is the only place how this setting can be changed 🤔. The VM would be observing a repository that only the VM itself can change -> so the VM would update the setting and the VM would receive an update from the repo. I might be missing something, but I'm not sure what is the benefit.
Could you please elaborate on it a bit?
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.
- Unidirectional data flow: Data flows in one direction: UI → VM → Repo → VM → UI. This makes it easy to trace what's happening. No shortcuts where the VM updates itself directly.
- Less code: No need to flip !_state.value.allowCellularDataUpdate - the Switch already has the new value, just save it
- Can't get out of sync: Right now, if something goes wrong between updating the VM state and saving to repo, they could be different. With observing, that's impossible.
- Works if anything else changes it: currently there is assumption that it can be changed only from 1 place and in sync way, but if it changes asynchronously, it'll be out of sync with the view
Bottom line: We don't maintain two copies of the same data. The repo is the source of the truth
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 points, I've tested and applied the patch 🙇.
kidinov
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.
LGTM! I left one code improvement suggestion, please take a look
Fixes WOOMOB-1446
Description
Implements cellular data preference functionality for WooPos local catalog synchronization. When users disable "Download over cellular" in the local catalog settings, background sync operations will only run when connected to WiFi, preventing unexpected data usage charges.
The implementation uses WorkManager's built-in network constraints at the OS level for optimal efficiency - the system won't even start the worker when network conditions don't match the user's preferences.
Testing information
The tests that have been performed
The above