fix: #2146 refresh product edition #2201

cli1005 · 2022-06-07T09:57:02Z

What

Refresh the edit page after every edit (Basic info, Nutritions, photos)

Screenshot

simulator-screen-recording-iphone-13-2022-06-07-at-114305_VMS9o1rF.mp4

Fixes bug(s)

Fixes: Refresh the edit page after every edit #2146

monsieurtanuki

Hi @cli1005!
I'm a bit embarrassed because we're currently implementing similar refresh algorithms on similar pages and similar files.
I've just merged code that will sound familiar to you in #2195, especially in edit_product_page.dart. I suggest that you refresh your code. I also have a pending PR named #2202.
Beyond that, I think that the trick is - when updating a product on the server - to return the server product (not just a bool), that we reuse without having to query the database.
And a difference we should maybe address now is: when do we save the product? In my latest merge about "stores", I do it when I leave the specific "edit stores" page. I think that you save when you leave the main "edit product" page. My method is safest, your method is smoothest for the end-user.

cli1005 · 2022-06-07T14:56:14Z

Thanks for your information, I will check and update my branch
I did not change the existing procedure, all the saving are done in "edit X" page by clicking on confirm button as it did before. I just want to make sure all those modifications have been put into cache so that I could get it back while next clicking. An exception for image editing, since I have to re-fetch the product from server in order to get the good URLs of last uploaded images

monsieurtanuki

Hi @cli1005!

Not a big fan actually:

either you changed the product, and you can get the product directly from saveAndRefresh
or you did not change the product, and then you don't need to refresh it as it did not change at all - unless you're very afraid someone else changed it in the meanwhile

The idea behind that being:

if a page (that is called from the edit product page) changed the product
that page must pop the edited product (including server and local database, everything done by saveAndRefresh) or null is nothing happened.

I think it's more robust if the called pages do everything (UI, server, local database, return product if changed).
Of course there's also the option of not saving anything until you quit the main edit page (in order to have frequent "save dialog"s), but that would require more attention about the product state.

cli1005 · 2022-06-08T13:52:12Z

I think it's more robust if the called pages do everything (UI, server, local database, return product if changed).

Thanks for your suggestions :)
The called pages of editing Basic infos, Ingredients, Nutritions do everything except returning product if changed. If you think it will be better to return changed product, I could do the necessary.

But editing image is an exception, since we pass just the barcode instead of an object product, and the page need newly added urls to initialize the carousel, the only way to get all newly added urls is to refetch the product, since as I know, the API of uploading image does not return the new url)

monsieurtanuki · 2022-06-08T15:04:00Z

Thanks for your suggestions :) The called pages of editing Basic infos, Ingredients, Nutritions do everything except returning product if changed. If you think it will be better to return changed product, I could do the necessary.

I do suggest that you return Product, at least for consistency. And btw Nutrients already do use saveAndRefresh.

But editing image is an exception, since we pass just the barcode instead of an object product, and the page need newly added urls to initialize the carousel, the only way to get all newly added urls is to refetch the product, since as I know, the API of uploading image does not return the new url)

Yes and maybe no: I coded one method in off-dart that changes the image and returns the new url. There may be a possibility to retrieve the new image url with the methods you're using (maybe not coded yet in off-dart, but possible, still). That said, you're probably right, in that specific case (only) it would be cleaner to refresh the product from the server.

I'm currently working on #2204 (probable PR today), which has an obvious impact on class ProductRefresher.
If that's ok with you, for the moment don't touch ProductRefresher and just spread the use of saveAndRefresh as I suggested above. That's right, for the moment that means doing nothing for pictures. I'm taking into consideration your use case. Anyway we need to deconstruct the whole class because it's split in several bits that we may need one by one or all together:

is the user logged in?
save to the server with waiting dialog
refresh the database from the server
display dialogs like error or success

cli1005 · 2022-06-09T09:08:23Z

I'm currently working on #2204 (probable PR today), which has an obvious impact on class ProductRefresher.
If that's ok with you, for the moment don't touch ProductRefresher and just spread the use of saveAndRefresh as I suggested above. T

👌 , no problem

monsieurtanuki · 2022-06-09T11:54:07Z

@cli1005 Now #2224 is merged. Basically it adds a "check if logged in" step in saveAndRefresh.
You should refresh your code in order to include that merged code.
And you're right, in some cases (not product update but picture update) you probably need a mere refresh: please add this method in ProductRefresher, reusing the maximum of what is already coded.

codecov-commenter · 2022-06-09T16:42:11Z

Codecov Report

Merging #2201 (116bf66) into develop (2ea0da3) will decrease coverage by 1.12%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           develop   #2201      +/-   ##
==========================================
- Coverage     8.86%   7.73%   -1.13%     
==========================================
  Files          161     181      +20     
  Lines         6623    9096    +2473     
==========================================
+ Hits           587     704     +117     
- Misses        6036    8392    +2356

Impacted Files	Coverage Δ
...kages/smooth_app/lib/widgets/attribute_button.dart	`0.00% <0.00%> (-92.00%)`	⬇️
packages/smooth_app/lib/themes/smooth_theme.dart	`60.00% <0.00%> (-22.98%)`	⬇️
...s/smooth_app/lib/data_models/user_preferences.dart	`9.37% <0.00%> (-22.94%)`	⬇️
...p/lib/generic_lib/dialogs/smooth_alert_dialog.dart	`19.23% <0.00%> (-14.98%)`	⬇️
...mooth_app/lib/data_models/product_preferences.dart	`26.47% <0.00%> (-4.96%)`	⬇️
.../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%)`	⬇️
packages/smooth_app/lib/main.dart	`15.74% <0.00%> (-2.16%)`	⬇️
...s/smooth_app/lib/pages/onboarding/next_button.dart	`2.22% <0.00%> (-1.78%)`	⬇️
...p/lib/pages/onboarding/consent_analytics_page.dart	`0.00% <0.00%> (-1.57%)`	⬇️
... and 169 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 588443d...116bf66. Read the comment docs.

monsieurtanuki

Hi @cli1005! Thank you for your changes!

Still, we do things differently and we need to harmonize (your way, my way or a better way).

When I update a product, for instance its categories, I only update its categories, not the whole product (I use the barcode as a key and the categories as a value). From what I read in your code I think that you call the server with the whole product, where you changed what needed to be changed but with tons of unrelated and possibly staled data.

And after I update the product, I get a fresh one from the server. And it looks like you update your dart Product object, possibly call the server that may fail but you don't really care as the fields are already changed in your object.

In short, I'm more cautious because I update the minimum of things (I use a proxy object) and always get a server-refreshed version of the product. In your code, I'm afraid in the end your product may be in a incoherent state.

But maybe I haven't interpreted your code correctly.

monsieurtanuki · 2022-06-09T16:59:14Z

packages/smooth_app/lib/pages/product/add_basic_details_page.dart

-    if (status == null || status.error != null) {
+    if (savedAndRefreshed != null) {
+      return true;
+    } else {
      _errorMessageAlert(appLocalizations.basic_details_add_error);


There's already an error message in saveAndRefresh. If you don't like the current error message that's normal, we have the make it a parameter in saveAndRefresh.

monsieurtanuki · 2022-06-09T17:00:21Z

packages/smooth_app/lib/pages/product/add_basic_details_page.dart

@@ -168,39 +166,23 @@ class _AddBasicDetailsPageState extends State<AddBasicDetailsPage> {
        ),
      );

-  Future<bool> _saveData(LocalDatabase localDatabase) async {
+  Future<bool> _saveData(
+      LocalDatabase localDatabase, Product inputProduct) async {


First thing first, please add a ',' after inputProduct.

Then, I would not recommend using the inputProduct, because you're changing it. You should start from scratch with a new Product().

Thanks for your suggestion, I removed the parameter inputProduct

monsieurtanuki · 2022-06-09T17:00:36Z

packages/smooth_app/lib/pages/product/add_basic_details_page.dart

@@ -168,39 +166,23 @@ class _AddBasicDetailsPageState extends State<AddBasicDetailsPage> {
        ),
      );

-  Future<bool> _saveData(LocalDatabase localDatabase) async {
+  Future<bool> _saveData(


You should probably return a Product?.

You should probably return a Product?.

✅

monsieurtanuki · 2022-06-09T17:03:41Z

packages/smooth_app/lib/pages/product/common/product_refresher.dart

  }

-  Future<bool> _fetchAndRefresh(
+  Future<_MetaProductRefresher> _fetchAndRefresh(


_saveAndRefresh should call this method instead of having to copy its code there.

fetchAndRefresh calls this method instead

monsieurtanuki · 2022-06-09T17:06:09Z

packages/smooth_app/lib/pages/product/edit_product_page.dart

@@ -112,7 +110,21 @@ class _EditProductPageState extends State<EditProductPage> {
                );
                if (refreshed ?? false) {


I know you didn't write it, but I'm sure refreshed == true is easier to read than refreshed ?? false.

monsieurtanuki · 2022-06-09T17:07:16Z

packages/smooth_app/lib/pages/product/edit_product_page.dart

@@ -112,7 +110,21 @@ class _EditProductPageState extends State<EditProductPage> {
                );
                if (refreshed ?? false) {
                  _changes++;
-                  await _refreshProduct();
+                  //Refetch product if needed for new urls


Please be more generous in your comment, and explain why you do it there and not in the other places where you code _changes++;

I added some more details

monsieurtanuki

Hi @cli1005!
Thank you for your changes.
I cannot say that I agree with 100% of what I read, but what I'm reflecting about ("a standard way of updating a product") goes beyond this specific PR, so let's go with it!

cli1005 added 2 commits June 7, 2022 10:47

fix: openfoodfacts#2146 refresh product edition

f43720d

fix: openfoodfacts#2146 refresh product edition photo editing

853ad17

cli1005 requested a review from a team as a code owner June 7, 2022 09:57

github-actions bot assigned cli1005 Jun 7, 2022

fix: openfoodfacts#2146 refactoring

42e3db6

cli1005 closed this Jun 7, 2022

monsieurtanuki reviewed Jun 7, 2022

View reviewed changes

cli1005 added 2 commits June 7, 2022 14:58

fix: fix presubmit test

59986c0

fix: fix presubmit

4865c66

Merge branch 'develop' into bug/2146-refresh-product-edition

1af30a0

cli1005 reopened this Jun 7, 2022

teolemon added ✏️ Contribution ✏️ Editing Many products are incomplete and don't have Nutri-Score, Eco-Score…so editing is important for users labels Jun 8, 2022

teolemon added this to the V1 milestone Jun 8, 2022

monsieurtanuki reviewed Jun 8, 2022

View reviewed changes

cli1005 added 3 commits June 9, 2022 15:38

Merge branch 'develop' into bug/2146-refresh-product-edition

2714bcd

fix: openfoodfacts#2146 refresh product edition v2

e718072

fix: openfoodfacts#2146 fix presubmit check

c2b3471

monsieurtanuki reviewed Jun 9, 2022

View reviewed changes

fix: openfoodfacts#2146 refactoring v3

116bf66

monsieurtanuki approved these changes Jun 10, 2022

View reviewed changes

monsieurtanuki merged commit 316e9df into openfoodfacts:develop Jun 10, 2022

monsieurtanuki mentioned this pull request Jun 10, 2022

Link the on-page add category button now that the dedicated section is working #2244

Closed

1 task

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: #2146 refresh product edition #2201

fix: #2146 refresh product edition #2201

cli1005 commented Jun 7, 2022

monsieurtanuki left a comment

cli1005 commented Jun 7, 2022 •

edited

Loading

monsieurtanuki left a comment

cli1005 commented Jun 8, 2022

monsieurtanuki commented Jun 8, 2022

cli1005 commented Jun 9, 2022

monsieurtanuki commented Jun 9, 2022

codecov-commenter commented Jun 9, 2022 •

edited

Loading

monsieurtanuki left a comment

monsieurtanuki Jun 9, 2022

monsieurtanuki Jun 9, 2022

cli1005 Jun 10, 2022

monsieurtanuki Jun 9, 2022

cli1005 Jun 10, 2022

monsieurtanuki Jun 9, 2022

cli1005 Jun 10, 2022

monsieurtanuki Jun 9, 2022

monsieurtanuki Jun 9, 2022

cli1005 Jun 10, 2022

monsieurtanuki left a comment

		@@ -112,7 +110,21 @@ class _EditProductPageState extends State<EditProductPage> {
		);
		if (refreshed ?? false) {

fix: #2146 refresh product edition #2201

fix: #2146 refresh product edition #2201

Conversation

cli1005 commented Jun 7, 2022

What

Screenshot

Fixes bug(s)

monsieurtanuki left a comment

Choose a reason for hiding this comment

cli1005 commented Jun 7, 2022 • edited Loading

monsieurtanuki left a comment

Choose a reason for hiding this comment

cli1005 commented Jun 8, 2022

monsieurtanuki commented Jun 8, 2022

cli1005 commented Jun 9, 2022

monsieurtanuki commented Jun 9, 2022

codecov-commenter commented Jun 9, 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

monsieurtanuki left a comment

Choose a reason for hiding this comment

cli1005 commented Jun 7, 2022 •

edited

Loading

codecov-commenter commented Jun 9, 2022 •

edited

Loading