feat: 4941 - refresh products when switching app language #5016
Conversation
New file: * `background_task_language_refresh.dart`: Background task about downloading products to translate. Impacted files: * `background_task_manager.dart`: minor refactoring * `dao_product.dart`: new method `getTopProductsToTranslate` * `local_database.dart`: minor refactoring * `offline_tasks_page.dart`: minor refactoring * `operation_type.dart`: new operation type `languageRefresh` * `product_refresher.dart`: minor refactoring * `up_to_date_mixin.dart`: minor refactoring * `up_to_date_product_provider.dart`: minor refactoring * `user_preferences_language_selector.dart`: now calling new task `BackgroundTaskLanguageRefresh` when changing language
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5016 +/- ##
==========================================
- Coverage 9.54% 9.51% -0.04%
==========================================
Files 325 327 +2
Lines 16411 16495 +84
==========================================
+ Hits 1567 1569 +2
- Misses 14844 14926 +82 ☔ View full report in Codecov by Sentry. |
| Future<void> preExecute(final LocalDatabase localDatabase) async {} | ||
|
|
||
| @override | ||
| bool get hasImmediateNextTask => true; |
There was a problem hiding this comment.
It's not always the case. I'm again doing a mobile review so I can't double-check the implications but I guess it's not causing any problems, is it?
There was a problem hiding this comment.
bool get hasImmediateNextTask => true;@M123-dev When we page our background tasks, task A will deal with only say 10 products and at the end will say "I know there are more products to deal with and I create task B". In that case we need the background manager to immediately restart, to deal with task B.
In case we have no more products to deal with, no additional task will be created and the manager will see an empty task queue, and do nothing.
As opposed to standard one-shot tasks (e.g. product detail change), that are not somehow chained to other tasks to be run immediately.
There was a problem hiding this comment.
I'm again doing a mobile review so I can't double-check the implications
Many thanks @M123-dev, that's not the easiest PR to review on mobile ;)
| // ...and remove barcodes we actually found on the server. | ||
| for (final Product product in searchResult.products!) { | ||
| newExcludeBarcodes.remove(product.barcode); |
There was a problem hiding this comment.
Say we try to refresh a product that was removed in the time before the refresh, then we'll have a lopped until the Cannot refresh language exception
Let's remove the ones we queried, not the ones we received
There was a problem hiding this comment.
Let's remove the ones we queried, not the ones we received
First we add all the barcodes we expected, and then remove all the barcodes we received. In the end we exclude the barcodes we expected but haven't received.
Here we're dealing with an exclusion barcode list: we need to tell the next task "I have already tried this barcode but received nothing".
// ...add the new barcodes...
newExcludeBarcodes.addAll(barcodes);
// ...and remove barcodes we actually found on the server.
for (final Product product in searchResult.products!) {
newExcludeBarcodes.remove(product.barcode);There was a problem hiding this comment.
@M123-dev Just to clarify a bit:
- we run a query in the local database - "which products are not yet in the expected language?"
- we maintain a list of "excluded" barcodes - barcodes we already tried to download but were not returned by the server
- the products returned by the server won't get listed again by the (1) query, as they are now flagged with the expected language
- and then we go back to (1) query in the next background task, looking for products not in the expected language and not queried yet.
There was a problem hiding this comment.
Oh okay, yeah makes more sense reading it now
|
Thank you very much @M123-dev for your review! |
|
@teolemon Had the opportunity to test the automatic product refresh when the user changes the app language? |
|
Yes, it seems to work well. 👍
|
We don't have a default country, do we? We have countries where the product was erm... produced and where the product is sold. Which one would that be?
That could be somehow already solved by #5025 (and incidentally later by #4771), cf. |
What
Screenshots
Even the pictures are different!
For the record, those are the barcodes I used that have different names/images in Italian/French:
Fixes bug(s)
Part of
Files
New file:
background_task_language_refresh.dart: Background task about downloading products to translate.Impacted files:
background_task_manager.dart: minor refactoringdao_product.dart: new methodgetTopProductsToTranslatelocal_database.dart: minor refactoringoffline_tasks_page.dart: minor refactoringoperation_type.dart: new operation typelanguageRefreshproduct_refresher.dart: minor refactoringup_to_date_mixin.dart: minor refactoringup_to_date_product_provider.dart: minor refactoringuser_preferences_language_selector.dart: now calling new taskBackgroundTaskLanguageRefreshwhen changing languageSpecial thanks to @LuanRoger for the "last accessed timestamp" idea!