[Woo POS][Coupons] Refactor WooPosProductsDataSource to Introduce WooPosBaseDataSource for Improved Maintainability
#13764
Conversation
…hed from the data source. All other specific data sources must extend from this class.
…sProductsDataSource
AnirudhBhat
added
type: task
📲 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.
|
…ogic # Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/WooPosItemsViewModel.kt
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #13764 +/- ##
=========================================
Coverage 38.16% 38.17%
- Complexity 9304 9312 +8
=========================================
Files 2084 2087 +3
Lines 115091 115109 +18
Branches 14661 14664 +3
=========================================
+ Hits 43927 43940 +13
- Misses 67164 67167 +3
- Partials 4000 4002 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
AnirudhBhat
changed the title
WooPosProductsDataSource to Introduce WooPosBaseDataSource for Improved Maintainability
|
Version |
…ogic # Conflicts: # WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/home/items/WooPosItemsViewModelTestSelectionViewState.kt
|
@coderabbitai review |
📝 WalkthroughWalkthroughThis pull request refactors the data fetching mechanism in the WooCommerce POS module. The legacy method Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ViewModel
participant DataSource as WooPosBaseDataSource
participant Cache
participant RemoteService
Caller->>ViewModel: Initiate data fetch (fetchData)
ViewModel->>DataSource: fetchData(fetchOptions)
DataSource->>Cache: fetchFromCache(fetchOptions)
Cache-->>DataSource: Return cached data
DataSource-->>ViewModel: Emit FetchResult.Cached(data)
DataSource->>RemoteService: fetchFromRemote(fetchOptions)
alt Remote fetch succeeds
RemoteService-->>DataSource: Return remote data (successful Result)
DataSource->>Cache: updateCache(fetchOptions, remote data)
DataSource-->>ViewModel: Emit FetchResult.Remote(Result.success(data))
else Remote fetch fails
RemoteService-->>DataSource: Return failure Result
DataSource-->>ViewModel: Emit FetchResult.Remote(Result.failure(exception))
end
Assessment against linked issues
Possibly related PRs
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: 2
🧹 Nitpick comments (17)
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/WooPosItemsViewModel.kt (1)
212-216: Proper handling of remote resultsThe handling of
FetchResult.Remotecorrectly checks for success and accesses the data usingresult.result.getOrThrow(), adapting to the new data structure. However, consider adding more specific error handling for network failures or other common issues.when { result.result.isSuccess -> { val products = result.result.getOrThrow() // Rest of the code } - else -> WooPosItemsViewState.Error() + else -> { + val exception = result.result.exceptionOrNull() + // Log the exception details for debugging + WooPosItemsViewState.Error( + errorMessage = exception?.message ?: "Unknown error occurred" + ) + } }WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/common/WooPosBaseDataSource.kt (5)
26-37: Consider implementing optional retry or specialized error handling.Right now, the code simply emits an error if remote fetching fails. Certain exceptions, such as transient network issues, might be recoverable with a retry policy. Introducing a retry mechanism or advanced error-handling pattern could enhance resilience.
16-21: Consider granular caching strategies.When
fetchOptions.forceRefreshis true, you permanently clear (overwrite with an empty list) the cached data before fetching. A more nuanced approach might allow partial or staged invalidation (e.g., removing stale entries only) for large caches, potentially improving performance under certain conditions.
23-25: Emit the cached data first.Emitting the cached data right away is a good pattern for quick UI updates. As a minor improvement, consider logging or tagging the emission points (cached vs. remote) to aid debugging or analytics.
26-31: Validate remote success path.Carefully handling the result of
remoteResult.getOrThrow()is good. Consider robust fallback or partial data usage if your business logic can handle partial retrieval in case of partial network failures.
32-37: Provide actionable error context.Emitting an exception as
"Unknown error"is helpful as a fallback, but returning more contextual data to the UI might improve error reporting or tracking. Adding a logging statement here could also aid debugging.WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/home/items/WooPosItemsViewModelTestSelectionViewState.kt (9)
106-115: Consistent mocking offetchDatawithargThat { !forceRefresh }.If many tests repeat the same mock logic, extracting it into a helper function could improve readability and reduce duplication.
29-29: Mock argument matchers with confidence.Using
argThatis a clear way to verifyfetchOptionsfields. Double-check that all relevant fields (beyondforceRefresh) are validated when necessary.
71-77: Confirm initial fetch logic.This mock sets up a default
Remotesuccess scenario. Consider adding a test forCachedemission in case the tests need to confirm the presence of a previously cached list as well.
106-110: Refine repeated mocks.Repeated code for mocking
productsDataSource.fetchDatacan increase maintenance overhead. If feasible, centralize these mocks in a helper function to reduce duplication.
164-168: Ensure error states remain consistent.Simulating a
Result.failure(Exception())scenario is valid. As a follow-up, confirm that the final UI displays the correct error message or fallback, especially if you introduce more explicit error modeling in the future.
267-285: Add coverage forLoadingstate transition.This test effectively checks the immediate
Loadingstate. Consider verifying that the subsequent emission (Cached or Remote) transitions fromLoadingappropriately in the same test or in a new complementary test.
306-312: Cover no-products scenario for pull-to-refresh.You’re testing the empty list scenario thoroughly. Also confirm if the UI requires a loading spinner or transitional state before finalizing as
Empty().
374-374: Instantiating the ViewModel for analytics.Creating the ViewModel triggers
PullToRefreshTriggered. Possibly test that no extraneous calls tofetchDataoccur on initialization if your design requires selective fetch triggers.
500-500: Dialog click handling adjacency.Ensuring the info icon triggers the correct event is good. If more actions get added, consider a dedicated test verifying all banner UI triggers remain consistent.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/products/WooPosProductsDataSource.kt (2)
61-76: Remote fetching logic is consistent with the base approach.You might consider converting certain typed exceptions or specific error codes into custom exceptions, allowing more granular error handling in the UI layer.
57-59: Confirming cache retrieval.
fetchFromCachesimply returnsproductCache. If your cached data can become invalid, consider a last-fetched timestamp or additional logic for stale checks.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/WooPosItemsViewModel.kt(2 hunks)WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/common/FetchDataSource.kt(1 hunks)WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/common/FetchOptions.kt(1 hunks)WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/common/FetchResult.kt(1 hunks)WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/common/WooPosBaseDataSource.kt(1 hunks)WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/products/WooPosProductsDataSource.kt(3 hunks)WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/splash/WooPosSplashViewModel.kt(2 hunks)WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/home/items/WooPosItemsViewModelTestSelectionViewState.kt(21 hunks)WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/home/items/WooPosProductsDataSourceTest.kt(14 hunks)WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/home/items/common/WooPosBaseDataSourceTest.kt(1 hunks)WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/splash/WooPosSplashViewModelTest.kt(4 hunks)
🔇 Additional comments (67)
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/common/FetchOptions.kt (1)
3-6: Clean data class implementation!The
FetchOptionsdata class is well-designed with appropriate default values for optional parameters. This class effectively centralizes the parameters needed for data fetching operations, making it easier to extend functionality in the future without breaking the API contract.WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/common/FetchDataSource.kt (1)
5-9: Well-designed generic interfaceThe
FetchDataSource<T>interface provides a clean contract for data sources with a generic type parameter that increases reusability. The use of Kotlin Flow for asynchronous operations is a modern approach that aligns with reactive programming principles.WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/common/FetchResult.kt (1)
3-6: Excellent use of sealed class for type safetyThe
FetchResult<T>sealed class creates a type-safe way to handle both cached and remote data results. This approach allows for exhaustivewhenstatements and eliminates the possibility of unhandled cases, improving code robustness and maintainability.WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/WooPosItemsViewModel.kt (4)
14-15: Good import additions for new functionalityThe imports for
FetchOptionsandFetchResultcorrectly integrate the new data fetching mechanism into this view model.
202-204: Successfully refactored to use the new data fetching mechanismThe code has been properly updated to use the new
fetchDatamethod withFetchOptions, which aligns with the refactoring goal of centralizing data source logic.
206-209: Proper handling of cached resultsThe handling of
FetchResult.Cachedcorrectly accesses the data usingresult.datainstead of the previousresult.products, adapting to the new data structure.
192-234:❓ Verification inconclusive
Verify pagination handling compatibility with new data structure
The refactored code seems to maintain pagination functionality, but make sure the
loadMore()method inproductsDataSourceis consistent with the newfetchDataapproach.
🏁 Script executed:
#!/bin/bash # Description: Check whether loadMore() in productsDataSource is consistent with fetchData approach # Search for loadMore method implementation in the productsDataSource class rg -A 10 "fun loadMore\(" --type kotlinLength of output: 16135
Pagination Handling Compatibility Check
I've reviewed the implementation of the
loadMore()method in the products data sources—especially the one inWooPosProductsDataSource.kt. It appears that this method callshandler.loadMore()and updates the product cache in a manner consistent with the newfetchDataapproach used inWooPosItemsViewModel.kt. That said, please verify the following:
- UI Synchronization: Ensure that the state update triggered by
loadMore()aligns with the UI pagination state (e.g., switching betweenPaginationState.LoadingandPaginationState.Noneas dictated by the active job in the view model).- Error Handling Consistency: Double-check that any error conditions or empty responses in
loadMore()are managed in a way that’s compatible with the error and empty state transitions in the newfetchDataflow.Overall, the current implementation seems aligned, but a manual verification of the above aspects across the related code paths is recommended.
WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/home/items/common/WooPosBaseDataSourceTest.kt (6)
14-19: Clean and clear implementation of the test data source.The anonymous object implementation of
WooPosBaseDataSourceprovides a good test subject with controlled responses from the abstract methods. This approach allows testing the base class functionality without unnecessary complexity.
21-32: Well-structured test with clear assertions.This test thoroughly validates that when both cached and remote data are available, the
fetchDatamethod correctly emits both types of data in the expected order. The assertions clearly verify the types and content of each result.
34-52: Good error handling test coverage.This test effectively verifies the behavior when remote data fetching fails but cached data is available. It ensures that the failure is properly propagated to the client while still providing the cached data.
54-74: Thorough validation of force refresh functionality.The test properly verifies that when
forceRefresh = true, the cache is cleared before fetching new data. The mock verification ensures that the cache update is called with the expected parameters.
76-95: Good validation of cache update mechanism.This test confirms that when remote data is successfully fetched, the cache is updated with the new data. The verification of the mock ensures that the right data is used to update the cache.
97-116: Comprehensive edge case handling.This test validates the behavior when both the cache is empty and the remote fetch fails, ensuring the data source correctly handles this worst-case scenario by emitting an empty cache result followed by the failure.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/splash/WooPosSplashViewModel.kt (3)
5-6: Good adoption of the new data fetching abstractions.The import of the new
FetchOptionsandFetchResultclasses aligns with the refactoring to introduce a common base data source.
28-30: Clean conversion to the new API.Replacing the previous method call with the new
fetchDatamethod using explicitFetchOptionsimproves code clarity and maintains the same functionality.
33-37: Simplified result handling.The switch to handling
FetchResulttypes makes the code more consistent and easier to understand, while maintaining the same behavior.WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/splash/WooPosSplashViewModelTest.kt (4)
3-4: Good alignment of test imports with implementation.Adding imports for
FetchOptionsandFetchResultensures consistency between tests and implementation code.
48-50: Test updated to match new API.Test mock correctly uses the new
fetchDatamethod withFetchOptionsand returns aFetchResult.Remotetype, maintaining test validity with the refactored implementation.
62-64: Consistent usage of new types across tests.The test correctly mocks the updated method signature and return types, ensuring the test remains valid after the refactoring.
76-78: Appropriate edge case test coverage maintained.The test for cached products is properly updated to use the new API while still testing the same behavior - ensuring the VM stays in loading state when only cached data is available.
WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/home/items/WooPosProductsDataSourceTest.kt (12)
5-6: Good import structure for the refactored code.Importing the new
FetchOptionsandFetchResultclasses is necessary for the updated API usage throughout the tests.
83-87: Consistent API usage across tests.The test has been properly updated to use the new
fetchDatamethod withFetchOptionsand handleFetchResulttypes, maintaining the same test coverage while adapting to the new structure.Also applies to: 93-96
110-116: Clean adaptation to new result structure.The test for cached products correctly handles the new
FetchResult.Cachedtype and accesses data through thedataproperty instead of the previousproductsproperty.
137-142: Good verification of both cached and remote results.The test properly extracts and verifies both the cached and remote results using the new typesafe result classes, improving code clarity.
178-183: Error handling test properly updated.The test for remote loading failures correctly handles the new result types while maintaining the same verification logic.
207-210: Consistent verification after loading more data.The test correctly verifies the cache state after loading more data, using the new API structure consistently.
237-240: Good verification of unchanged cache on failure.The test properly ensures that the cache remains unchanged when loading more data fails, correctly using the new result structure.
260-265: Edge case handling properly updated.The test for empty cache with remote failure correctly handles the new result structure while maintaining the same verification logic.
283-288: Comprehensive empty result testing.The test for empty product lists correctly verifies both cached and remote results using the new structure, ensuring edge cases are handled properly.
325-327: Product filtering logic correctly preserved.The test for filtering products by price correctly uses the new result structure while maintaining the same business logic verification.
365-367: Downloadable product filtering test properly updated.The test for filtering downloadable products correctly uses the new result structure while preserving the same verification logic.
404-406: Variable product handling test properly updated.The test for variable products with null prices correctly uses the new result structure while maintaining the same business logic verification.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/common/WooPosBaseDataSource.kt (3)
8-15: Establishing a unified base class for data fetching is a solid architectural choice.This approach encourages consistency across future data sources by centralizing shared logic such as caching and remote fetching in a single place.
23-25: Good call on always emitting cached data first.Emitting a cached result before proceeding with remote fetching ensures the UI or consumers have immediate data, even while awaiting the remote response.
8-14: Encapsulate shared data-fetching logic effectively.This abstract class neatly centralizes common data retrieval methods. By requiring subclasses to implement
fetchFromCache,fetchFromRemote, andupdateCache, you ensure consistent patterns and reduce duplication across data sources.WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/home/items/WooPosItemsViewModelTestSelectionViewState.kt (25)
12-13: Imports ofFetchOptionsandFetchResultalign with the new data-fetching structure.
29-29: MockitoargThatusage looks appropriate.
71-76: MockingfetchData(any())for initial test setup is clear.Consider adding a test to validate how the ViewModel handles cached data separately, if that path is critical. Otherwise, this is fine.
139-149: Ensuring an empty product list produces an empty state is well-tested.
164-174: Verifying error scenario coverage looks solid.
193-196: Great job verifying correctfetchDatacall with expectedFetchOptions.
259-262: Correct verification of non-force refresh call.
267-279: Testing the "view state is Loading" path ensures correctness of immediate state changes.
306-318: Confirming no-products returns an empty state upon refresh.This is a valuable user-facing scenario to test thoroughly.
335-344: Verifying parent notification on empty data scenario is well-structured.
346-346: No functional differences in creating the ViewModel.
365-365: Change is limited to a comment adjustment, no concerns.
375-375: No functional update beyond test readability.
383-383: Creating the ViewModel again for this test scenario is straightforward.
385-385: Event handling verifies correctness of closing the banner.
500-500: Repeated ViewModel instantiation is likely part of test arrangement, no issues.
516-516: Another standard ViewModel creation, no concerns.
536-545: Relevant test block for variable product fetching.Mocks appear correctly set up to confirm the flow from
fetchDatato the finalViewState.
12-13: Use imports consistently.Adding
FetchOptionsandFetchResultaligns with the new data-source architecture. Ensure that no unused imports remain if the old method references have been fully removed.
139-143: Validate empty-data scenario.This test ensures that an empty list triggers
WooPosItemsViewState.Empty(). Good job verifying the correct state is emitted. Also consider verifying theCachedvs.Remotestates if that is relevant to your logic.
193-196: Double-check pull-to-refresh arguments.Verifying
fetchDatawithproductId == null && !forceRefreshis consistent with standard refresh logic. If partial refreshing is ever supported, ensure these tests reflect it.
261-261: Loading without force refresh.Good job verifying the default
forceRefresh = falsescenario. No concerns here.
335-344: Parent notification scenario.This block ensures the parent event
ProductsStatusChanged.FullScreenis sent for empty data. Good alignment with requested feature behavior. No additional concerns.
365-365: Awaiting items after refresh.Awaiting the next emission ensures no concurrency issues. This is a solid pattern for verifying successive states. Good approach.
383-383: Confirm banner state test arrangement.Constructing the ViewModel here ensures the banner logic is tested with the updated architecture. This is consistent with the approach in the other tests.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/products/WooPosProductsDataSource.kt (7)
6-7: ImportingFetchOptionsandWooPosBaseDataSourceunderscores the new inheritance model.
21-21: ExtendingWooPosBaseDataSourceproperly alignsWooPosProductsDataSourcewith the new architecture.
57-59: Fetching from cache returns the currentproductCache.Ensure that usage of
productCacheoutside of concurrency locks won’t lead to race conditions, especially if updated from multiple threads. However, if read-only under normal usage, this is acceptable.
78-80: Updating the cache leverages an existing shared method.Maintaining a single entry point (
updateProductCache) avoids duplication and ensures consistent cache updates.
6-7: New import statements for base class.These imports (
FetchOptions,WooPosBaseDataSource) correctly align with the new refactored architecture. No issues here.
21-21: Extend fromWooPosBaseDataSource.Switching from a standalone class to inheriting the base data-source approach improves consistency and maintainability across data sources. Well done.
78-79: Synchronized cache update.Wrapping
updateProductCachein a mutex lock is good concurrency protection. No additional concerns here.
| override fun fetchData( | ||
| fetchOptions: FetchOptions | ||
| ): Flow<FetchResult<T>> = flow { | ||
| if (fetchOptions.forceRefresh) { | ||
| updateCache(fetchOptions, data = emptyList()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider mutex or other concurrency safeguards for force-refresh cache clearing.
When fetchOptions.forceRefresh is true, the existing cache is cleared without an apparent concurrency lock around updateCache. If multiple coroutines call fetchData simultaneously, there's a risk of overwriting each other's cache updates. To ensure thread safety, consider using a mutex or synchronized approach, similar to how other data sources might guard shared state.
| override suspend fun fetchFromRemote(fetchOptions: FetchOptions): Result<List<Product>> { | ||
| val result = handler.loadFromCacheAndFetch( | ||
| forceRefresh = true, | ||
| includeType = listOf(WCProductStore.IncludeType.Simple, WCProductStore.IncludeType.Variable), | ||
| searchType = ProductListHandler.SearchType.DEFAULT, | ||
| filters = mapOf( | ||
| WCProductStore.ProductFilterOption.STATUS to ProductStatus.PUBLISH.value, | ||
| WCProductStore.ProductFilterOption.DOWNLOADABLE to WCProductStore.DownloadableOptions.FALSE.toString(), | ||
| ) | ||
| ) | ||
| return if (result.isSuccess) { | ||
| Result.success(handler.productsFlow.first()) | ||
| } else { | ||
| Result.failure(result.exceptionOrNull() ?: Exception("Unknown error while fetching products")) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remote fetch strategy.
Using handler.loadFromCacheAndFetch(forceRefresh = true, ...) always forces a refresh from the server. If partial or non-forced fetch is required, passing fetchOptions.forceRefresh might be more flexible.
-val result = handler.loadFromCacheAndFetch(
- forceRefresh = true,
- ...
-)
+val result = handler.loadFromCacheAndFetch(
+ forceRefresh = fetchOptions.forceRefresh,
+ ...
+)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| override suspend fun fetchFromRemote(fetchOptions: FetchOptions): Result<List<Product>> { | |
| val result = handler.loadFromCacheAndFetch( | |
| forceRefresh = true, | |
| includeType = listOf(WCProductStore.IncludeType.Simple, WCProductStore.IncludeType.Variable), | |
| searchType = ProductListHandler.SearchType.DEFAULT, | |
| filters = mapOf( | |
| WCProductStore.ProductFilterOption.STATUS to ProductStatus.PUBLISH.value, | |
| WCProductStore.ProductFilterOption.DOWNLOADABLE to WCProductStore.DownloadableOptions.FALSE.toString(), | |
| ) | |
| ) | |
| return if (result.isSuccess) { | |
| Result.success(handler.productsFlow.first()) | |
| } else { | |
| Result.failure(result.exceptionOrNull() ?: Exception("Unknown error while fetching products")) | |
| } | |
| } | |
| override suspend fun fetchFromRemote(fetchOptions: FetchOptions): Result<List<Product>> { | |
| val result = handler.loadFromCacheAndFetch( | |
| forceRefresh = fetchOptions.forceRefresh, | |
| includeType = listOf(WCProductStore.IncludeType.Simple, WCProductStore.IncludeType.Variable), | |
| searchType = ProductListHandler.SearchType.DEFAULT, | |
| filters = mapOf( | |
| WCProductStore.ProductFilterOption.STATUS to ProductStatus.PUBLISH.value, | |
| WCProductStore.ProductFilterOption.DOWNLOADABLE to WCProductStore.DownloadableOptions.FALSE.toString(), | |
| ) | |
| ) | |
| return if (result.isSuccess) { | |
| Result.success(handler.productsFlow.first()) | |
| } else { | |
| Result.failure(result.exceptionOrNull() ?: Exception("Unknown error while fetching products")) | |
| } | |
| } |
|
|
||
| import kotlinx.coroutines.flow.Flow | ||
|
|
||
| interface FetchDataSource<T> { |
There was a problem hiding this comment.
🤔 I'm trying to figure out where the interface would be used. Did you have any specific use case in mind, like DI, testing, etc. If we provide base class, maybe interface is not needed?
Closes: #13746
Description
This PR refactors the
WooPosProductsDataSourceto introduce a newWooPosBaseDataSourceclass that consolidates common data-fetching logic. This change improves code maintainability and reduces duplication across data sources.Why This Refactor?
Previously, data sources like
WooPosProductsDataSourcecontained repetitive logic for:By introducing
WooPosBaseDataSource, this refactor:WooPosProductsDataSourcenow focuses only on product-specific details.What's Changed?
WooPosBaseDataSourceto manage common data-fetching logic.WooPosProductsDataSourceto extendWooPosBaseDataSource, removing redundant logic.Impact
Next Steps
WooPosVariationsDataSource.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: