-
Notifications
You must be signed in to change notification settings - Fork 134
[Shipping Labels] Cache carrier packages data #14541
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
[Shipping Labels] Cache carrier packages data #14541
Conversation
An auto-migration from version 59 to 60 has also been added.
This change introduces two new methods to `WooShippingDao`: - `insertShippingPackages`: Upserts a `WooShippingPackagesEntity`. - `getShippingPackages`: Retrieves a `WooShippingPackagesEntity` by `localSiteId`.
This commit refactors the `WooShippingLabelPackageMapper` to use entities from FluxC (`WooShippingPackagesEntity`, `CarrierPackageGroups`, `Package`, `StoreOptions`) instead of local DAO objects.
The `WooShippingDao` is now injected into the `WooShippingLabelPackageRepository` constructor. This change allows the repository to access shipping data directly from the local database.
The `fetchAllStorePackages` method in `WooShippingLabelPackageRepository` is updated to save the successfully fetched shipping packages to the local database using `wooShippingDao.insertShippingPackages`.
The method `fetchAllStorePackages` in `WooShippingLabelPackageRepository` and its corresponding tests have been renamed to `fetchShippingPackages` for clarity and consistency.
The local cache of shipping packages is now updated after a custom package is created, a predefined package is saved, or a saved package is deleted.
Generated by 🚫 Danger |
📲 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.
|
# Conflicts: # libs/fluxc-plugin/schemas/org.wordpress.android.fluxc.persistence.WCAndroidDatabase/60.json
# Conflicts: # libs/fluxc-plugin/schemas/org.wordpress.android.fluxc.persistence.WCAndroidDatabase/61.json # libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/persistence/WCAndroidDatabase.kt
98d5703
to
2cfd82d
Compare
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.
There’s actually no change in this file, but I couldn’t convince Git. 🤷🏻♂️
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.
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.
When I try to remove this line, I can’t commit and receive this error. No need to investigate it, I’ll leave it as is.
On branch issue/WOOMOB-887-cache-carrier-packages-data
Your branch is up to date with 'origin/issue/WOOMOB-887-cache-carrier-packages-data'.
nothing to commit, working tree clean
# Conflicts: # libs/fluxc-plugin/schemas/org.wordpress.android.fluxc.persistence.WCAndroidDatabase/63.json
Version |
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.
Nice work @irfano, this works well.
I have a different opinion about the approach you choose for caching, it's not a blocker, so I'm pre-approving, but just leaving my remark below for discussion:
I added the cache to Room since the date is not lightweight. Additionally, I didn't create multiple tables for storeOptions, savedPackages and carrierPackageGroups. Instead, I stored their raw JSON data in separate fields within a single row. I felt that adding multiple tables would make this feature unnecessarily complex for its size in the app. Because of this decision, I added markPackageSaved and markPackageRemoved functions in WooShippingLabelPackageRepository. Still, I believe this approach is simpler than having multiple tables or a highly complex table structure.
I think storing some items as json
in a single column is generally fine, especially when we are storing a single item. But for this case, I believe doing so for savedPackages
and carrierPackageGroups
could become problematic, as these are lists of items over which we don't control how much they can grow, which could theoretically lead to SQLiteBlobTooBigException
crashes (similar to the crashes we already have #6508).
Also, I don't see any advantage to using Room
in this case, since we are storing a single row
per store, which means we could have used DataStore
instead, and in my opinion, we could have avoided the chances of SQLiteBlobTooBigException
here.
As I said, I don't think this is a blocker, and given the chances of having big lists of packages are very small, I'm just sharing my opinion for discussion, and given the priority of next project, I'd say it's better to leave this as is.
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.
import org.wordpress.android.fluxc.persistence.entity.WooShippingPackagesEntity | ||
import javax.inject.Inject | ||
|
||
class FetchShippingPackages @Inject constructor( |
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.
np, I'm not sure this is needed, since we already use the usercase ObserveShippingPackages
for the accessing the packages, I think ObserveShippingPackages
can call the repository directly, and it would be easier to follow.
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.
ObserveShippingPackages
has a different logic: it uses cached data while also fetching new data. FetchShippingPackages
is used for retry functionality to force a fresh fetch, and it’s a very small use case.
If we switch to ObserveShippingPackages
, we’d need to add a forceFetch
parameter, adjust the logic, and probably rename the use case. I think keeping it as is would be simpler. But let me know if I’ve misunderstood your request.
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 think I wasn't clear here, I meant deleting this use case completely, and update ObserveShippingPackages
to use the repository directly (calling WooShippingLabelPackageRepository#fetchShippingPackages directly).
This use case doesn't have any additional logic; it just maps the response, which normally should be the repository role.
val savedPackages: List<Package>, | ||
val carrierPackageGroups: List<CarrierPackageGroups> | ||
) { | ||
data class StoreOptions( |
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.
np, we are already storing a copy of StoreOptions in AccountSettings
, so we could probably avoid caching this duplicate entity here, and instead access ObserveAccountSettings
directly when getting the packages, WDYT?
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.
This logic already existed. Now we’re caching it, and there is a side effect but it's minor since StoreOptions
data is small. I don’t want to increase the size of this PR, and I believe it’s a low priority. I’ve added a note for this task in WOOMOB-689
.
Good point, @hichamboushaba! Thanks for raising this. Let me share my thoughts after looking into this a bit further. The current data size is ~1.22 KB. Even it grows, it's highly unlikely to exceed 1 MB, which is typically when
In any case, I agree with leaving this as is for now. |
I think that the comment there means that there could be performance problems with DataStore if the size grows, as there is no hard limitation like what SQLite has. That's why I said that, given the current situation, using DataStore would still be a better choice. |
Closes WOOMOB-887
Description
This PR adds caching for shipment packages.
Its main purpose is to eliminate the waiting time when opening the Packages screen.
The video below starts from a “no cache” and API error state. In that case, we show an error. After the API succeeds, we update the cache and display the data. The logic in
ObserveShippingPackages
is the same as in ObserveAccountSettings.I added the cache to Room since the date is not lightweight. Additionally, I didn't create multiple tables for
storeOptions
,savedPackages
andcarrierPackageGroups
. Instead, I stored their raw JSON data in separate fields within a single row. I felt that adding multiple tables would make this feature unnecessarily complex for its size in the app. Because of this decision, I addedmarkPackageSaved
andmarkPackageRemoved
functions inWooShippingLabelPackageRepository
. Still, I believe this approach is simpler than having multiple tables or a highly complex table structure.Steps to reproduce
The tests that have been performed
See the steps above. I also debugged the database after each action.
Images/gif
Screen_recording_20250830_015437.webm
RELEASE-NOTES.txt
if necessary. Use the "[Internal]" label for non-user-facing changes.