fix: Offline product knowledge panel issue #2693

AshAman999 · 2022-07-31T18:28:40Z

What

Don't update the already available products with KP when preloading data

Fixes bug(s)

Fixes: Partial download of a product for offline mode could remove KP #2667

Part of

https://github.com/orgs/openfoodfacts/projects/33

Co-authored-by: Abu Ghalib <abughalib@users.noreply.github.com>

…into offline-Product-Knowlege-Panel-Issue

packages/smooth_app/lib/query/products_preload_helper.dart

…into offline-Product-Knowlege-Panel-Issue

codecov-commenter · 2022-08-01T10:30:35Z

Codecov Report

Merging #2693 (35e074e) into develop (2ea0da3) will decrease coverage by 1.74%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           develop   #2693      +/-   ##
==========================================
- Coverage     8.86%   7.12%   -1.75%     
==========================================
  Files          161     218      +57     
  Lines         6623   10645    +4022     
==========================================
+ Hits           587     758     +171     
- Misses        6036    9887    +3851

Impacted Files	Coverage Δ
...kages/smooth_app/lib/widgets/attribute_button.dart	`0.00% <0.00%> (-92.00%)`	⬇️
...s/smooth_app/lib/data_models/user_preferences.dart	`8.73% <0.00%> (-23.57%)`	⬇️
packages/smooth_app/lib/themes/smooth_theme.dart	`62.26% <0.00%> (-20.72%)`	⬇️
...p/lib/generic_lib/dialogs/smooth_alert_dialog.dart	`15.11% <0.00%> (-19.10%)`	⬇️
...mooth_app/lib/data_models/product_preferences.dart	`21.68% <0.00%> (-9.75%)`	⬇️
packages/smooth_app/lib/main.dart	`14.65% <0.00%> (-3.24%)`	⬇️
.../smooth_app/lib/pages/onboarding/welcome_page.dart	`0.00% <0.00%> (-3.13%)`	⬇️
.../smooth_app/lib/pages/onboarding/scan_example.dart	`0.00% <0.00%> (-2.28%)`	⬇️
...ackages/smooth_app/lib/pages/scan/scan_header.dart	`2.50% <0.00%> (-2.27%)`	⬇️
...p/lib/pages/onboarding/consent_analytics_page.dart	`0.00% <0.00%> (-1.57%)`	⬇️
... and 237 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

monsieurtanuki

Hi @AshAman999!
Not 100% convinced: please have a look at my comments.

packages/smooth_app/lib/query/products_preload_helper.dart

monsieurtanuki · 2022-08-01T11:11:57Z

packages/smooth_app/lib/query/products_preload_helper.dart

+        await daoProduct.getAll(allProductCodes);
+    allProducts.removeWhere(
+        (String key, Product value) => value.knowledgePanels == null);
+    allProductCodes = allProducts.keys.toList();
    final List<ProductField> fields = ProductQuery.fields;
    fields.remove(ProductField.KNOWLEDGE_PANELS);


Actually that's not OK: you're also removing an item from the main fields.
You should copy all the fields you need instead, into your own list.

Had done a for each iteration but as a left suggestion I changed it

Might be something like this would work fine

final List<String> allProductCodes = await daoProduct.getAllKeys(); final Map<String, Product> allProducts = await daoProduct.getAll(allProductCodes); allProducts.forEach((String code, Product product) { if (product.knowledgePanels != null) { allProductCodes.remove(code); } });

You haven't implemented a fix, have you?

I guess I did the changes that you asked about not loading all the products

monsieurtanuki · 2022-08-01T11:13:45Z

packages/smooth_app/lib/query/products_preload_helper.dart

+    final Map<String, Product> allProducts =
+        await daoProduct.getAll(allProductCodes);


That means you're loading ALL the database.
I know it's only for dev mode, but that's not something I would recommend.
What about getting all the barcodes instead of barcodes AND products.
For the level of granularity we want that's good enough I think.

@monsieurtanuki
Just to clarify, this is not always supposed to be in dev mode, after further fine-tuning and doing this task in background, we are probably thinking of showing a menu in onboarding screens to let preload data, but not just 100o, rather in numbers of 10k.

And why I loaded the products as well is because there is no way where I can be sure that which barcodes will have knowledge panels or not. I tried thinking of alternate solutions even using daoproductlist, would be helpful for me to get an alternate idea to do the same without loading all the products but with just the barcode strings

I guess a SQL query for getting all the barcode with Products having knowledge panel could have probably helped here.
But since we already don't do much of magic with SQL, can't say what else we can do here

DaoProduct.getAllKeys()

Okay I get all the keys now I updated the database with top 1000 products
Now, if I do again try to get the top 1000 products, just for sense of updating it,
When I do DaoProduct.getAllKeys(), I will get the list of barcodes along with the top 1000 products, so now the list of barcodes will have more than 1000, and hence I can't update it. Ryt?
I hope now you can understand the problem...

Oops, I skipped that one:

Just to clarify, this is not always supposed to be in dev mode, after further fine-tuning and doing this task in background, we are probably thinking of showing a menu in onboarding screens to let preload data, but not just 100o, rather in numbers of 10k.

But now we're on dev mode, so let's focus on that. Background task later.
Regarding putting that in onboarding, that's a bold move considering the current popularity of the onboarding among the users.
Regarding 10K bulk load, I don't see how you can skip the "download first, then refresh locally if relevant, then the next 10K" for the whole server data.

@monsieurtanuki
About your query to not load all products to check if they have knowledge panels or not
What I propose is we add another column in the table with name "isHalfProduct", and we give it a default value of false
And when pushing a product to db, we can say that it is not fully downloaded by just passing an argument as true.

Would be best if you could suggest me if I should go ahead with this approach??

What I propose is we add another column in the table with name "isHalfProduct", and we give it a default value of false

I don't think it's a good idea to add computed columns to sqflite. In general that's a pain in the neck to add columns: not just adding them, but then initializing them, and keep the init script running forever, and then you add another column and you wind up with several scripts that run forever.
Especially in this case: there's not such thing as isHalfProduct. Unless that means "null knowledge panels" on August 5th, 2022. But what next?

My suggestion: you only have to code a single method that will solve all your problems. And luckily, that's easy to code, and to test as a background method:

2 parameters, DaoProduct and Product newProduct

you try to get from DaoProduct a Product with barcode = newProduct.barcode!

if there's indeed already one product, you check its knowledgePanels field. If it's not null, you return false (that's data that you don't want to overwrite)

else you upsert your newProduct and return true (that's data that either did not exist locally, or that didn't include kwowledge panels so your new version is better)

Et voilà.

Done that, plz re review

ping @monsieurtanuki

packages/smooth_app/lib/query/products_preload_helper.dart

Co-authored-by: monsieurtanuki <fabrice_fontaine@hotmail.com>

…into offline-Product-Knowlege-Panel-Issue

….com/AshAman999/smooth-app into offline-Product-Knowlege-Panel-Issue

…into offline-Product-Knowlege-Panel-Issue

monsieurtanuki

Hi @AshAman999!
My main concern is about ProductQuery.fields field that you alter.
Please make ProductQuery.fields a static const.

monsieurtanuki · 2022-08-10T13:06:06Z

packages/smooth_app/lib/query/products_preload_helper.dart

+        await daoProduct.getAll(allProductCodes);
+    allProducts.removeWhere(
+        (String key, Product value) => value.knowledgePanels == null);
+    allProductCodes = allProducts.keys.toList();
    final List<ProductField> fields = ProductQuery.fields;
    fields.remove(ProductField.KNOWLEDGE_PANELS);


You haven't implemented a fix, have you?

packages/smooth_app/lib/query/products_preload_helper.dart

AshAman999 · 2022-08-10T15:11:50Z

Hi @AshAman999! My main concern is about ProductQuery.fields field that you alter. Please make ProductQuery.fields a static const.

@monsieurtanuki
Tbh I tried , I am not understanding why it's a problem, I hope you could suggest me what exactly to here

…into offline-Product-Knowlege-Panel-Issue

monsieurtanuki · 2022-08-10T15:41:54Z

@monsieurtanuki Tbh I tried , I am not understanding why it's a problem, I hope you could suggest me what exactly to here

@AshAman999 I'm afraid you're right. ProductQuery.fields should probably be a static const. But it's not. Therefore your code is likely to work.

AshAman999 · 2022-08-10T15:52:26Z

@monsieurtanuki Tbh I tried , I am not understanding why it's a problem, I hope you could suggest me what exactly to here

@AshAman999 I'm afraid you're right. ProductQuery.fields should probably be a static const. But it's not. Therefore your code is likely to work.

I did most of the changes you suggested me here, If we could merge it,
I will probably add the translations later on and tweak this method as well

monsieurtanuki

Looks good to me @AshAman999

packages/smooth_app/lib/query/products_preload_helper.dart

teolemon · 2022-08-11T11:06:27Z

can I merge @AshAman999 @monsieurtanuki ?

AshAman999 · 2022-08-11T13:42:02Z

can I merge @AshAman999 @monsieurtanuki ?

Now it's good to go !!!
We can merge it

teolemon · 2022-08-11T14:04:36Z

🎉

AshAman999 and others added 4 commits July 30, 2022 02:42

Update products with knowledge panels seperately

715e0e5

Co-authored-by: Abu Ghalib <abughalib@users.noreply.github.com>

don't update products already with knowledge panel

2c811d4

minor fix

02a97e5

Merge branch 'develop' of https://github.com/openfoodfacts/smooth-app …

8371a2f

…into offline-Product-Knowlege-Panel-Issue

AshAman999 requested a review from a team as a code owner July 31, 2022 18:28

github-actions bot assigned AshAman999 Jul 31, 2022

VaiTon suggested changes Jul 31, 2022

View reviewed changes

packages/smooth_app/lib/query/products_preload_helper.dart Outdated Show resolved Hide resolved

packages/smooth_app/lib/query/products_preload_helper.dart Outdated Show resolved Hide resolved

AshAman999 added 3 commits August 1, 2022 12:30

use remove where instead of for each

38174ec

added exception handling

79241d0

replaced ! with ?

5924dc5

AshAman999 requested a review from VaiTon August 1, 2022 10:25

Merge branch 'develop' of https://github.com/openfoodfacts/smooth-app …

0781a47

…into offline-Product-Knowlege-Panel-Issue

VaiTon requested a review from monsieurtanuki August 1, 2022 11:00

VaiTon approved these changes Aug 1, 2022

View reviewed changes

monsieurtanuki reviewed Aug 1, 2022

View reviewed changes

AshAman999 and others added 6 commits August 1, 2022 17:28

Update packages/smooth_app/lib/query/products_preload_helper.dart

c12643f

Co-authored-by: monsieurtanuki <fabrice_fontaine@hotmail.com>

Merge branch 'develop' into offline-Product-Knowlege-Panel-Issue

8300936

Merge branch 'develop' of https://github.com/openfoodfacts/smooth-app …

e40f3f2

…into offline-Product-Knowlege-Panel-Issue

Merge branch 'offline-Product-Knowlege-Panel-Issue' of https://github…

a8a6714

….com/AshAman999/smooth-app into offline-Product-Knowlege-Panel-Issue

Merge branch 'develop' of https://github.com/openfoodfacts/smooth-app …

58565e1

…into offline-Product-Knowlege-Panel-Issue

custom check if product has kp

ff8eebd

AshAman999 requested a review from monsieurtanuki August 6, 2022 18:23

monsieurtanuki reviewed Aug 10, 2022

View reviewed changes

suggested changes

3c4128d

Merge branch 'develop' of https://github.com/openfoodfacts/smooth-app …

52808d4

…into offline-Product-Knowlege-Panel-Issue

monsieurtanuki approved these changes Aug 10, 2022

View reviewed changes

packages/smooth_app/lib/query/products_preload_helper.dart Outdated Show resolved Hide resolved

update style of writing docs

36dfe98

Merge branch 'develop' into offline-Product-Knowlege-Panel-Issue

35e074e

teolemon merged commit 4052a84 into openfoodfacts:develop Aug 11, 2022

github-actions bot mentioned this pull request Aug 11, 2022

chore(develop): release 3.7.0 #2735

Merged

github-actions bot mentioned this pull request Sep 6, 2022

chore(develop): release 3.14.0 #2951

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Offline product knowledge panel issue #2693

fix: Offline product knowledge panel issue #2693

AshAman999 commented Jul 31, 2022

codecov-commenter commented Aug 1, 2022 •

edited

Loading

monsieurtanuki left a comment

monsieurtanuki Aug 1, 2022

AshAman999 Aug 1, 2022

monsieurtanuki Aug 10, 2022

AshAman999 Aug 10, 2022

monsieurtanuki Aug 1, 2022

AshAman999 Aug 1, 2022

AshAman999 Aug 1, 2022

monsieurtanuki Aug 1, 2022

AshAman999 Aug 1, 2022

monsieurtanuki Aug 1, 2022

AshAman999 Aug 5, 2022

monsieurtanuki Aug 5, 2022

AshAman999 Aug 6, 2022

AshAman999 Aug 10, 2022

monsieurtanuki left a comment

monsieurtanuki Aug 10, 2022

AshAman999 commented Aug 10, 2022

monsieurtanuki commented Aug 10, 2022

AshAman999 commented Aug 10, 2022

monsieurtanuki left a comment

teolemon commented Aug 11, 2022 •

edited

Loading

AshAman999 commented Aug 11, 2022

teolemon commented Aug 11, 2022

		final Map<String, Product> allProducts =
		await daoProduct.getAll(allProductCodes);

fix: Offline product knowledge panel issue #2693

fix: Offline product knowledge panel issue #2693

Conversation

AshAman999 commented Jul 31, 2022

What

Fixes bug(s)

Part of

codecov-commenter commented Aug 1, 2022 • edited Loading

Codecov Report

monsieurtanuki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monsieurtanuki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AshAman999 commented Aug 10, 2022

monsieurtanuki commented Aug 10, 2022

AshAman999 commented Aug 10, 2022

monsieurtanuki left a comment

Choose a reason for hiding this comment

teolemon commented Aug 11, 2022 • edited Loading

AshAman999 commented Aug 11, 2022

teolemon commented Aug 11, 2022

codecov-commenter commented Aug 1, 2022 •

edited

Loading

teolemon commented Aug 11, 2022 •

edited

Loading