fix: #1352 - robotoff insight card shows permanently #1574

cli1005 · 2022-04-14T09:05:38Z

What

The Robotoff insight card ("🥇 card") persists on the product page which is supposed to be hided after having answering all the questions.

Root cause : Comment issues

Solutions :

Turn LoadingDialog to future builder
Create new hive box to cache vote history
More details : Comment soultions

Screenshot

simulator-screen-recording-iphone-13-2022-04-13-at-163702_08ZSUGyO.mp4

Fixes bug(s)

Robotoff refresh bug brings me into a loop #1352

cli1005 · 2022-04-14T09:12:13Z

@teolemon, could you please check why the workflow "Add bugs to the Release Smoothie GitHub Project" failed? thanks

teolemon · 2022-04-14T09:51:17Z

on it @cli1005

teolemon · 2022-04-14T17:23:40Z

fixed @cli1005

monsieurtanuki

Hi @cli1005!
Good things and bad things in your PR: please have a look at my comments and ask me if needed.

monsieurtanuki · 2022-04-20T14:25:02Z

packages/smooth_app/lib/data_models/product_query_model.dart

@@ -160,4 +162,51 @@ class ProductQueryModel with ChangeNotifier {
          .toList();
    }
  }
+
+  static Future<void> cacheInsightAnnotationVoted(


Something doesn't feel right here: the rest of the file is about a class that gets instantiated.
Here everything you add are static methods that don't seem to have any connection with this specific class.
I suggest that you create another class in another file. Possibly a class with a LocalDatabase parameter in the constructor.

monsieurtanuki · 2022-04-20T14:26:06Z

packages/smooth_app/lib/database/dao_robotoff_insight_map.dart

+
+  Future<bool> existsInsight(final String insightId) async {
+    final Map<String, List<String>> value = await getAll();
+    bool esixts = false;


monsieurtanuki · 2022-04-20T14:26:06Z

packages/smooth_app/lib/database/dao_robotoff_insight_map.dart

+
+  Future<bool> existsInsight(final String insightId) async {
+    final Map<String, List<String>> value = await getAll();
+    bool esixts = false;


monsieurtanuki · 2022-04-20T14:32:12Z

packages/smooth_app/lib/database/dao_robotoff_insight_map.dart

+    return false;
+  }
+
+  Future<bool> removeInsight(final String insightId) async {


removeStringValue would be more meaningful.

monsieurtanuki · 2022-04-20T14:32:12Z

packages/smooth_app/lib/database/dao_robotoff_insight_map.dart

+    return false;
+  }
+
+  Future<bool> removeInsight(final String insightId) async {


removeStringValue would be more meaningful.

monsieurtanuki · 2022-04-20T15:02:59Z

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

+      final ProductResult result = await OpenFoodAPIClient.getProduct(
+          configuration,
+          queryType: OpenFoodAPIConfiguration.globalQueryType);


Please keep it the way it was.

monsieurtanuki · 2022-04-20T15:04:58Z

packages/smooth_app/lib/generic_lib/loading_dialog.dart

@@ -68,7 +68,7 @@ class LoadingDialog<T> {
            (final T value) => _popDialog(context, value),
          );
          // TODO(monsieurtanuki): is that safe? If the future finishes before the "return" call?
-          return _getDialog(context, title);
+          return _getDialog(context, title, future);


Then you don't have to call the future just before, do you?

monsieurtanuki · 2022-04-20T15:04:58Z

packages/smooth_app/lib/generic_lib/loading_dialog.dart

@@ -68,7 +68,7 @@ class LoadingDialog<T> {
            (final T value) => _popDialog(context, value),
          );
          // TODO(monsieurtanuki): is that safe? If the future finishes before the "return" call?
-          return _getDialog(context, title);
+          return _getDialog(context, title, future);


Then you don't have to call the future just before, do you?

monsieurtanuki · 2022-04-20T15:07:02Z

packages/smooth_app/lib/generic_lib/loading_dialog.dart

+            return ListTile(
+              leading: const CircularProgressIndicator(),
+              title: Text(title),
+            );


In that case we're not here anymore. If you really need to return something, a Container will do. Not something that looks interesting but will only add confusion to the code reader.

monsieurtanuki · 2022-04-20T15:07:03Z

packages/smooth_app/lib/generic_lib/loading_dialog.dart

+            return ListTile(
+              leading: const CircularProgressIndicator(),
+              title: Text(title),
+            );


In that case we're not here anymore. If you really need to return something, a Container will do. Not something that looks interesting but will only add confusion to the code reader.

codecov-commenter · 2022-04-21T08:06:01Z

Codecov Report

Merging #1574 (9faf821) into develop (2ea0da3) will decrease coverage by 0.11%.
The diff coverage is 1.60%.

@@            Coverage Diff             @@
##           develop   #1574      +/-   ##
==========================================
- Coverage     8.86%   8.74%   -0.12%     
==========================================
  Files          161     164       +3     
  Lines         6623    6771     +148     
==========================================
+ Hits           587     592       +5     
- Misses        6036    6179     +143

Impacted Files	Coverage Δ
...rds/knowledge_panels/knowledge_panels_builder.dart	`2.17% <0.00%> (ø)`
...s/product_cards/smooth_product_card_not_found.dart	`0.00% <0.00%> (ø)`
...mooth_app/lib/data_models/product_query_model.dart	`1.38% <ø> (ø)`
...s/smooth_app/lib/data_models/user_preferences.dart	`34.42% <ø> (+2.11%)`	⬆️
...s/smooth_app/lib/database/dao_string_list_map.dart	`0.00% <0.00%> (ø)`
...ckages/smooth_app/lib/database/local_database.dart	`0.00% <0.00%> (ø)`
...s/smooth_app/lib/database/paged_product_query.dart	`0.00% <0.00%> (ø)`
...oth_app/lib/database/robotoff_questions_query.dart	`0.00% <0.00%> (ø)`
...ric_lib/buttons/smooth_large_button_with_icon.dart	`0.00% <0.00%> (ø)`
...ges/smooth_app/lib/generic_lib/loading_dialog.dart	`0.00% <0.00%> (ø)`
... and 13 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 75b677c...9faf821. Read the comment docs.

monsieurtanuki · 2022-04-21T08:07:53Z

@cli1005 Is that open or closed?

cli1005 · 2022-04-21T08:17:26Z

@cli1005 Is that open or closed?

closed for fixing pre-submit test issues, now it's ok

monsieurtanuki

Thank you @cli1005 for your quick fixes!
Please have a look at my comments: among them you'll see that you missed an await.
Fix that and push, and then we're done :)

monsieurtanuki · 2022-04-20T14:25:02Z

packages/smooth_app/lib/data_models/product_query_model.dart

@@ -160,4 +162,51 @@ class ProductQueryModel with ChangeNotifier {
          .toList();
    }
  }
+
+  static Future<void> cacheInsightAnnotationVoted(


Something doesn't feel right here: the rest of the file is about a class that gets instantiated.
Here everything you add are static methods that don't seem to have any connection with this specific class.
I suggest that you create another class in another file. Possibly a class with a LocalDatabase parameter in the constructor.

monsieurtanuki · 2022-04-21T08:06:51Z

packages/smooth_app/lib/data_models/product_query_model.dart

+import 'package:smooth_app/database/dao_string_list_map.dart';
+import 'package:smooth_app/database/local_database.dart';
 import 'package:smooth_app/database/product_query.dart';
+import 'package:smooth_app/database/robotoff_questions_query.dart';


Remove those additional imports: we don't need them anymore.

monsieurtanuki · 2022-04-21T08:16:50Z

packages/smooth_app/lib/database/dao_string_list_map.dart

+  static const String _hiveBoxName = 'robotoffMap';
+  static const String _key = 'votedHistory';


Of course that will have to be changed when we have another use for DaoStringListMap, but for the moment that's OK.

monsieurtanuki · 2022-04-21T08:19:08Z

packages/smooth_app/lib/database/robotoff_questions_query.dart

+  RobotoffQuestionsQuery(this._barcode);
+  final String _barcode;


Please don't do that again: for me it's an additional file to check, with no added value as your only change is making the barcode private.

monsieurtanuki · 2022-04-21T08:20:54Z

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

@@ -294,6 +295,9 @@ class _ProductListPageState extends State<ProductListPage> {
      }
      await DaoProduct(localDatabase).putAll(freshProducts);
      freshProducts.forEach(productList.refresh);
+      final RobotoffInsightHelper robotoffInsightHelper =
+          RobotoffInsightHelper(localDatabase);
+      robotoffInsightHelper.clearInsightAnnotationsSaved();


monsieurtanuki · 2022-04-21T08:22:37Z

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

-      final ProductResult result =
-          await OpenFoodAPIClient.getProduct(configuration);
+      final ProductResult result = await OpenFoodAPIClient.getProduct(
+        configuration,


Again, that's the only contribution on this file: I have to review this file for one comma that nobody complained about!
If you had to do something more relevant in this file and in the meanwhile correct here and there the presentation, why not, but without that please don't include that in a PR.

…ard-loop

…com/cli1005/smooth-app into bug/1352-robotoff-insight-card-loop

cli1005 added 6 commits April 8, 2022 14:52

fix: openfoodfacts#1395 - show newly added product in carousel

c47ebd2

Merge branch 'develop' into bug/1395-new-product-in-carousel

2d3f256

fix: openfoodfacts#1395 - refactoring unused import

238da17

fix: openfoodfacts#1395 - refactoring make variable private

52ac3c7

fix: openfoodfacts#1352 - robotoff card shows permanently

6b8acbc

Merge branch 'develop' into bug/1352-robotoff-insight-card-loop

f776ad2

cli1005 requested a review from a team as a code owner April 14, 2022 09:05

teolemon linked an issue Apr 14, 2022 that may be closed by this pull request

Robotoff refresh bug brings me into a loop #1352

Closed

teolemon added the 🤖 Robotoff label Apr 14, 2022

teolemon assigned cli1005 Apr 14, 2022

teolemon added this to the V1 milestone Apr 14, 2022

Merge branch 'develop' into bug/1352-robotoff-insight-card-loop

ab8a364

teolemon requested a review from monsieurtanuki April 17, 2022 17:52

monsieurtanuki reviewed Apr 20, 2022

View reviewed changes

cli1005 closed this Apr 21, 2022

fix: openfoodfacts#1352 - codereview refactoring

5f82115

cli1005 reopened this Apr 21, 2022

M123-dev requested a review from monsieurtanuki April 21, 2022 08:04

cli1005 closed this Apr 21, 2022

cli1005 reopened this Apr 21, 2022

cli1005 added 2 commits April 21, 2022 10:11

fix: openfoodfacts#1352 - check fix

4d77cec

fix: openfoodfacts#1352 - pretest check fix

ecea74f

monsieurtanuki approved these changes Apr 21, 2022

View reviewed changes

Merge branch 'openfoodfacts:develop' into bug/1352-robotoff-insight-c…

7e6224d

…ard-loop

cli1005 added 2 commits April 21, 2022 10:53

fix: openfoodfacts#1352 - codereview fix

7c62b2b

Merge branch 'bug/1352-robotoff-insight-card-loop' of https://github.…

9faf821

…com/cli1005/smooth-app into bug/1352-robotoff-insight-card-loop

cli1005 merged commit f671807 into openfoodfacts:develop Apr 21, 2022

github-actions bot mentioned this pull request Apr 21, 2022

chore(develop): release 1.0.0 #962

Closed

		static const String _hiveBoxName = 'robotoffMap';
		static const String _key = 'votedHistory';

		RobotoffQuestionsQuery(this._barcode);
		final String _barcode;

fix: #1352 - robotoff insight card shows permanently #1574

fix: #1352 - robotoff insight card shows permanently #1574

Conversation

cli1005 commented Apr 14, 2022

What

Screenshot

Fixes bug(s)

cli1005 commented Apr 14, 2022 • edited Loading

teolemon commented Apr 14, 2022

teolemon commented Apr 14, 2022

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

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

codecov-commenter commented Apr 21, 2022 • edited Loading

Codecov Report

monsieurtanuki commented Apr 21, 2022

cli1005 commented Apr 21, 2022

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

cli1005 commented Apr 14, 2022 •

edited

Loading

codecov-commenter commented Apr 21, 2022 •

edited

Loading