[Woo POS][Coupons] Refactor WooPosVariationsDataSource to Use BaseDataSource for Improved Maintainability #13766
Conversation
…ssue/13763-refactor-variations-data-source
AnirudhBhat
added
type: task
AnirudhBhat
changed the title
📲 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.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## issue/13746-refactor-data-source-business-logic #13766 +/- ##
=====================================================================================
- Coverage 38.17% 38.17% -0.01%
- Complexity 9312 9316 +4
=====================================================================================
Files 2087 2087
Lines 115109 115109
Branches 14664 14658 -6
=====================================================================================
- Hits 43940 43939 -1
- Misses 67167 67169 +2
+ Partials 4002 4001 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Version |
…ssue/13763-refactor-variations-data-source
|
@coderabbitai review |
📝 WalkthroughWalkthroughThe pull request refactors the variations data fetching logic across production and test code. In the data source, the class now extends a common base and renames methods (e.g., updating Changes
Sequence Diagram(s)sequenceDiagram
participant VM as VariationsViewModel
participant DS as VariationsDataSource
participant Cache as LocalCache
participant Remote as RemoteSource
VM->>DS: fetchData(FetchOptions)
alt Valid productId provided
DS->>Cache: fetchFromCache(FetchOptions)
DS->>Remote: fetchFromRemote(FetchOptions)
DS-->>VM: List<ProductVariation>
else Missing productId
DS-->>VM: throw IllegalArgumentException
end
Assessment against linked issues
Suggested labels
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/home/variations/WooPosVariationsViewModelTest.kt (1)
64-64: Consider verifying productId argument
When usingany()inwhenever(), we lose explicit verification of product ID. For safer testing, consider usingeq(productId)to ensure correct arguments.WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/home/variations/WooPosVariationsDataSourceTest.kt (1)
80-83: Multiple sequential fetch calls
Consider storing the repeated fetch calls in local variables or a setup function to reduce duplication. Otherwise, the test intent is clear.WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/variations/WooPosVariationsDataSource.kt (1)
21-21: Refactor to base data source
Inheriting fromWooPosBaseDataSourceclarifies caching and remote fetching logic. The repeated checks for nullproductIdwith thrownIllegalArgumentExceptionis acceptable. Consider consolidating argument checks into a single helper to reduce repetition.Also applies to: 62-94
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/variations/WooPosVariationsDataSource.kt(3 hunks)WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/variations/WooPosVariationsViewModel.kt(2 hunks)WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/home/variations/WooPosVariationsDataSourceTest.kt(15 hunks)WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/home/variations/WooPosVariationsViewModelTest.kt(13 hunks)
🔇 Additional comments (35)
WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/home/variations/WooPosVariationsViewModelTest.kt (11)
12-13: New imports aligned with updated fetch approach
These imports forFetchOptionsandFetchResultare consistent with the new data fetching pattern. No issues found.
87-87: Same suggestion as line 64
110-110: Same suggestion as line 64
130-130: Same suggestion as line 64
146-146: Same suggestion as line 64
167-167: Same suggestion as line 64
176-176: Explicitly verifying fetch options
Great thatFetchOptions(productId = 123L, forceRefresh = true)is explicitly used. This ensures correct coverage in your verification.
181-181: Same suggestion as line 64
201-202: Same suggestion as line 64
228-229: Same suggestion as line 64
361-369: Good validation test for missing productId
This new test effectively ensures thatfetchDatathrows anIllegalArgumentExceptionwhen productId is null, improving error handling coverage.WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/variations/WooPosVariationsViewModel.kt (2)
16-18: New imports for common fetch logic
These imports indicate usage of the newFetchOptionsandFetchResultpatterns, aligning with the base data source approach. Looks good.
82-122: Refined data fetching logic
ReplacingfetchFirstPagewithfetchDataclarifies how local and remote results are handled. The usage ofCachedandRemotestates is well-structured. No issues found.WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/home/variations/WooPosVariationsDataSourceTest.kt (20)
4-8: Refined imports
AddingProductStatus,FetchOptions,FetchResult, andany()fromorg.mockito.kotlinaligns with the new data fetching logic. No concerns.Also applies to: 20-20
86-86: Same suggestion as lines 80-83
90-90: Same suggestion as lines 80-83
126-126: Same suggestion as lines 80-83
129-129: Same suggestion as lines 80-83
152-152: Same suggestion as lines 80-83
167-167: Same suggestion as lines 80-83
191-191: Same suggestion as lines 80-83
193-193: Same suggestion as lines 80-83
200-200: Same suggestion as lines 80-83
201-201: Same suggestion as lines 80-83
202-202: Same suggestion as lines 80-83
228-228: Same suggestion as lines 80-83
229-229: Same suggestion as lines 80-83
237-237: Same suggestion as lines 80-83
238-238: Same suggestion as lines 80-83
260-260: Same suggestion as lines 80-83
267-267: Same suggestion as lines 80-83
268-268: Same suggestion as lines 80-83
361-369: Test coverage for null productId
Verifying that an appropriate exception is thrown helps ensure robust parameter validation. Great addition.WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/variations/WooPosVariationsDataSource.kt (2)
4-7: New import statements
These imports align with the new usage ofProductStatus,FetchOptions, andWooPosBaseDataSource. Implementation looks straightforward.
26-26: Renaming ensures clarity
RenamingupdateCachetoupdateVariationCacheclarifies the function's purpose and reduces confusion.
|
Version |
|
Closing after an internal discussion, more info on #13764. |
Closes: #137
Description
This PR refactors the
WooPosVariationsDataSourceto extend the newly introducedWooPosBaseDataSource, consolidating common data-fetching logic to reduce duplication and improve maintainability.Why This Refactor?
Previously, data sources like
WooPosVariationsDataSourcecontained repetitive logic for:By introducing
WooPosBaseDataSource, this refactor:WooPosVariationsDataSourcenow focuses only on variation-specific details.What's Changed?
WooPosBaseDataSourceto manage common data-fetching logic.WooPosVariationsDataSourceto extendWooPosBaseDataSource, removing redundant logic.Impact
Next Steps
WooPosCouponsDataSourcewhile implementing Coupons feature in POS.Testing information
Ensure that there is not change in Items screen behaviour. This refactoring should not have any visible impact on end users.
The tests that have been performed
All the scenarios listed above.
Images/gif
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: