feat: Nutrition facts: check values before sending them #4446 #6562
Conversation
github-project-automation
bot
moved this to 💬 To discuss and validate
in 🤳🥫 The Open Food Facts mobile app (Android & iOS)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6562 +/- ##
==========================================
- Coverage 9.54% 5.72% -3.83%
==========================================
Files 325 511 +186
Lines 16411 30462 +14051
==========================================
+ Hits 1567 1744 +177
- Misses 14844 28718 +13874 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hi @LorenzoMascia!
Please have a look at my comments.
Remember also that I'm expecting a screenshot in #6555.
| switch (nutrient) { | ||
| case Nutrient.addedSugars: | ||
| return _validateAddedSugars(quantity, unit, servingSize); |
There was a problem hiding this comment.
Not very elegant.
An extension on Nutrient would be much better.
There was a problem hiding this comment.
I refactored the code using a Map. Please, let me know if in this way it's better.
| } | ||
| } | ||
|
|
||
| bool _validateAddedSugars(double q, Unit u, double s) => |
There was a problem hiding this comment.
We can afford to use words instead of letters for variables.
A computer wouldn't make the difference. A reviewer and a developer would.
There was a problem hiding this comment.
Thank you, I appreciate the suggestion.
| } | ||
|
|
||
| bool _validateAddedSugars(double q, Unit u, double s) => | ||
| _validateMax(_normalize(q, u, Unit.G, s), 50.0); |
There was a problem hiding this comment.
I have no idea where you picked the min/max values.
There was a problem hiding this comment.
I obtained the values as the maximum possible values in foods. Some must simply be less than the weight, such as carbohydrate or sugar content, but in other cases, like zinc or calcium, it's not possible to simply assume a content less to the weight. The check prevents also potential errors in the use of measurement units, like 50g instead of 50mg
| } | ||
| // Energy conversions | ||
| if (from == Unit.KCAL && to == Unit.KJ) { | ||
| return 4.184; |
There was a problem hiding this comment.
Please use NutrimentsHelper.fromKCalToKJ and NutrimentsHelper.fromKJtoKCal.
There was a problem hiding this comment.
Not sure on how to use them, This method does not converts values but returns conversion factors.
| return 1.0; | ||
| } | ||
|
|
||
| double? _parseDouble(String? input) { |
There was a problem hiding this comment.
I'm sure there are other places in the code where we parse a double. Please don't reinvent the wheel.
There was a problem hiding this comment.
I changed the name of this method because it does not simply parse a double but it also reads the unit specified in the input string
| String servingSize = '100.0'; | ||
|
|
||
| if (_nutritionContainer.perSize == PerSize.serving) { | ||
| servingSize = _nutritionContainer.servingSize; | ||
| } |
There was a problem hiding this comment.
Not sure what you want to do here, but a cleaner syntax would be:
| String servingSize = '100.0'; | |
| if (_nutritionContainer.perSize == PerSize.serving) { | |
| servingSize = _nutritionContainer.servingSize; | |
| } | |
| final String servingSize; | |
| if (_nutritionContainer.perSize == PerSize.serving) { | |
| servingSize = _nutritionContainer.servingSize; | |
| } else { | |
| servingSize = '100.0'; | |
| } |
There was a problem hiding this comment.
The scope is to obtain the serving size weight. There are 2 options: per serving and per 100 g/ml
In the first case the weight is specified in the serving size field (and the user can specify the unit).
| final TextEditingControllerWithHistory controller = | ||
| _controllers[nutrient]!; | ||
| final String quantity = controller.text.trim(); | ||
| final Unit u = _nutritionContainer.getUnit(nutrient); |
There was a problem hiding this comment.
PLEASE USE WORDS!
| final Unit u = _nutritionContainer.getUnit(nutrient); | |
| final Unit unit = _nutritionContainer.getUnit(nutrient); |
| return 1.0; | ||
| } | ||
| // Mass conversions | ||
| if (from == Unit.G && to == Unit.MILLI_G) { |
There was a problem hiding this comment.
I'm sure there are conversion methods somewhere else in the app.
Please use them.
There was a problem hiding this comment.
I did the search looking for all occurrences of the Unit enum. I didn't find a similar method.
…#4446 openfoodfacts#6562 Fixes after first review Changes: modified: packages/smooth_app/lib/pages/product/nutrition_page/nutrition_validator.dart
github-project-automation
bot
moved this to To triage
in 🤖 Artificial Intelligence @ Open Food Facts
github-project-automation
bot
added this to 🤖 Artificial Intelligence @ Open Food Facts
LorenzoMascia
force-pushed
the
feat/check-values-before-seding#4446
branch
3 times, most recently
from
26aa31b to
22c6dd4
Compare
|
Hi @LorenzoMascia! |
|
/gemini review |
There was a problem hiding this comment.
Pull Request Overview
This PR adds client-side validation of nutrition facts to ensure values normalized per 100 g/ml don’t exceed typical maxima, highlights invalid inputs, and blocks saving invalid data.
- Introduces
NutritionValidatorfor parsing, normalizing, and checking nutrient values. - Extends
TextEditingControllerWithHistorywith a validity flag and updates editor styling to visually flag invalid entries. - Integrates validation logic in
NutritionPageand replaces custom question dialogs with standardNavigator.pushflows.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/smooth_app/lib/pages/text_field_helper.dart | Added _valid flag, getter/setter to track input validity. |
| packages/smooth_app/lib/pages/product/nutrition_page | Imported and integrated NutritionValidator to block invalid saves. |
| packages/smooth_app/lib/pages/product/nutrition_validator.dart | New class normalizing inputs and enforcing nutrient maxima. |
| packages/smooth_app/lib/pages/product/nutrition_page/widgets/nutrition_facts_editor.dart | Updated text field style to show invalid state and reset on change. |
| packages/smooth_app/lib/pages/product/product_questions_widget.dart & user_preferences_contribute.dart | Switched custom dialogs to Navigator.push. |
| packages/smooth_app/lib/pages/hunger_games/*.dart | Renamed dialog classes to pages, refactored UI and state. |
Comments suppressed due to low confidence (5)
packages/smooth_app/lib/pages/text_field_helper.dart:13
- [nitpick] The property
_validand its accessorvaliddon’t follow Dart boolean naming conventions. Consider renaming toisValidfor clarity.
bool _valid = true;
packages/smooth_app/lib/pages/product/nutrition_page/nutrition_validator.dart:4
- This map is mutable at runtime. Consider declaring it as a
static constmap to prevent accidental modifications.
final Map<Nutrient, double> nutrientMaxContentPer100g = <Nutrient, double>{
packages/smooth_app/lib/pages/product/nutrition_page/nutrition_validator.dart:4
- [nitpick] The map contains many hardcoded numeric limits. Extract these magic numbers into named constants or an external config for readability.
final Map<Nutrient, double> nutrientMaxContentPer100g = <Nutrient, double>{
packages/smooth_app/lib/pages/product/nutrition_page/nutrition_page.dart:262
- [nitpick] The variable
erroris ambiguous. Rename it to something likehasValidationErrorto convey its purpose more clearly.
bool error = false;
packages/smooth_app/lib/pages/product/nutrition_page/nutrition_validator.dart:1
- There are no unit tests for
NutritionValidator. Add tests to cover parsing edge cases, unit conversions, and validation logic.
import 'package:openfoodfacts/openfoodfacts.dart';
|
|
||
| input = input.trim().toLowerCase(); | ||
|
|
||
| final RegExp regex = RegExp(r'^([0-9]+\.?[0-9]*)\s*([a-zA-Z%]+)?$'); |
There was a problem hiding this comment.
The regex doesn’t match values starting with a decimal point (e.g. ".5"). Update it to allow optional leading digits or a leading dot.
| final RegExp regex = RegExp(r'^([0-9]+\.?[0-9]*)\s*([a-zA-Z%]+)?$'); | |
| final RegExp regex = RegExp(r'^([0-9]*\.?[0-9]+)\s*([a-zA-Z%]+)?$'); |
Copilot uses AI. Check for mistakes.
| input = input.trim().toLowerCase(); | ||
|
|
||
| final RegExp regex = RegExp(r'^([0-9]+\.?[0-9]*)\s*([a-zA-Z%]+)?$'); | ||
| final RegExpMatch? match = regex.firstMatch(input); | ||
|
|
||
| if (match == null) { | ||
| throw FormatException('Invalid input format: $input'); | ||
| } | ||
|
|
||
| final double value = double.parse(match.group(1)!); | ||
|
|
||
| // Default to grams if unit not specified | ||
| final String unitStr = match.group(2) ?? 'g'; | ||
|
|
||
| // Convert unit string to Unit enum | ||
| final Unit unit = UnitHelper.stringToUnit(unitStr) ?? Unit.G; | ||
|
|
||
| // Use _unitConversionFactor to convert from source unit to grams | ||
| return value * _unitConversionFactor(unit, Unit.G); |
There was a problem hiding this comment.
Parsing errors bubble up as exceptions and can crash validation. Wrap parsing in try/catch and return false to mark invalid input instead of throwing.
| input = input.trim().toLowerCase(); | |
| final RegExp regex = RegExp(r'^([0-9]+\.?[0-9]*)\s*([a-zA-Z%]+)?$'); | |
| final RegExpMatch? match = regex.firstMatch(input); | |
| if (match == null) { | |
| throw FormatException('Invalid input format: $input'); | |
| } | |
| final double value = double.parse(match.group(1)!); | |
| // Default to grams if unit not specified | |
| final String unitStr = match.group(2) ?? 'g'; | |
| // Convert unit string to Unit enum | |
| final Unit unit = UnitHelper.stringToUnit(unitStr) ?? Unit.G; | |
| // Use _unitConversionFactor to convert from source unit to grams | |
| return value * _unitConversionFactor(unit, Unit.G); | |
| try { | |
| input = input.trim().toLowerCase(); | |
| final RegExp regex = RegExp(r'^([0-9]+\.?[0-9]*)\s*([a-zA-Z%]+)?$'); | |
| final RegExpMatch? match = regex.firstMatch(input); | |
| if (match == null) { | |
| return null; // Invalid format | |
| } | |
| final double value = double.parse(match.group(1)!); | |
| // Default to grams if unit not specified | |
| final String unitStr = match.group(2) ?? 'g'; | |
| // Convert unit string to Unit enum | |
| final Unit unit = UnitHelper.stringToUnit(unitStr) ?? Unit.G; | |
| // Use _unitConversionFactor to convert from source unit to grams | |
| return value * _unitConversionFactor(unit, Unit.G); | |
| } catch (e) { | |
| return null; // Handle any parsing errors | |
| } |
Copilot uses AI. Check for mistakes.
| @@ -249,6 +250,42 @@ class _NutritionPageLoadedState extends State<NutritionPageLoaded> | |||
| } | |||
| } | |||
|
|
|||
| final NutritionValidator validator = NutritionValidator(); | |||
There was a problem hiding this comment.
[nitpick] A new NutritionValidator is created on each save. Consider reusing a single instance to reduce allocations.
Copilot uses AI. Check for mistakes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces nutrition facts validation and refactors the Hunger Games question UI. The validation logic is a great step towards improving data quality. The Hunger Games UI changes seem to modernize its presentation.
Summary of Findings
- Unhandled Exception Risk in Validator:
NutritionValidator._parseValueWithUnitcan throw aFormatExceptionif input strings are malformed. This is not caught innutrition_page.dartwhenvalidator.validateis called, potentially leading to a crash during the save process. This is marked as a critical issue. - Hunger Games Question Count: The refactored
QuestionsPageuses a local integer_questionsAnsweredfor the pop result, butCongratsWidgetstill has aChangeNotifierProviderthat is never updated. This will likely result inCongratsWidgetalways showing 0 answered questions. This is marked as a high severity issue. - Validator Robustness:
_parseValueWithUnitcould be made internally safer by handling potentialFormatExceptionfromdouble.parseand returningnull. Also, the fallback unit conversion factor of 1.0 in_unitConversionFactormight hide issues with unsupported unit pairs. These are marked as medium severity.
Merge Readiness
The pull request introduces important validation features and refactors parts of the UI. However, due to the critical issue in NutritionValidator's error handling and the high-severity bug in the Hunger Games question counting, I recommend that these changes not be merged until these issues are addressed. Once these are resolved, the PR will be in much better shape. I am not authorized to approve pull requests, so please ensure further review and approval by authorized team members after addressing the feedback.
| for (final Nutrient nutrient in _controllers.keys) { | ||
| final TextEditingControllerWithHistory controller = | ||
| _controllers[nutrient]!; | ||
| final String quantity = controller.text.trim(); | ||
| final Unit unit = _nutritionContainer.getUnit(nutrient); | ||
|
|
||
| if (!validator.validate(nutrient, quantity, unit, servingSize)) { | ||
| error = true; | ||
| controller.valid = false; | ||
| } else { | ||
| controller.valid = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
The validator.validate call might throw an unhandled exception if _parseValueWithUnit encounters a malformed numeric string that double.parse cannot handle. This could lead to a crash or prevent saving. Consider making _parseValueWithUnit more robust by catching FormatExceptions and returning null instead. This would allow the existing if ((quantity == null) || (servingSize == null)) { return true; } logic in validate to handle malformed inputs gracefully.
try {
if (!validator.validate(nutrient, quantity, unit, servingSize)) {
error = true;
controller.valid = false;
} else {
controller.valid = true;
}
} catch (e) {
// Handle the exception, possibly setting error = true and logging the error
error = true;
controller.valid = false; // Or some other appropriate error handling
print('Validation error: $e');
}
| title: const Text('Hunger Games'), | ||
| leading: IconButton( | ||
| icon: const Icon(Icons.close), | ||
| onPressed: () => Navigator.of(context).pop(_questionsAnswered), |
There was a problem hiding this comment.
The Navigator.of(context).pop(_questionsAnswered) call is using the local _questionsAnswered variable, which is good. However, the ChangeNotifierProvider<_QuestionsAnsweredNotifier> is still present but its value is never updated. Is this notifier still needed, or can it be removed to avoid confusion?
Navigator.of(context).pop(_questionsAnswered);
| if (match == null) { | ||
| throw FormatException('Invalid input format: $input'); | ||
| } | ||
|
|
||
| final double value = double.parse(match.group(1)!); | ||
|
|
||
| // Default to grams if unit not specified | ||
| final String unitStr = match.group(2) ?? 'g'; | ||
|
|
||
| // Convert unit string to Unit enum | ||
| final Unit unit = UnitHelper.stringToUnit(unitStr) ?? Unit.G; | ||
|
|
||
| // Use _unitConversionFactor to convert from source unit to grams | ||
| return value * _unitConversionFactor(unit, Unit.G); | ||
| } |
There was a problem hiding this comment.
To make _parseValueWithUnit itself safer, you could wrap the parsing logic in a try-catch block here and return null if a FormatException occurs during double.parse(match.group(1)!).
if (match == null) {
return null;
}
try {
final double value = double.parse(match.group(1)!);
final String unitStr = match.group(2) ?? 'g';
final Unit unit = UnitHelper.stringToUnit(unitStr) ?? Unit.G;
return value * _unitConversionFactor(unit, Unit.G);
} catch (e) {
return null;
}
| return 1.0; | ||
| } | ||
|
|
||
| return 1.0; |
There was a problem hiding this comment.
The fallback return 1.0; for unit conversions could potentially mask errors if an unexpected pair of units is encountered. Would it be safer to throw an ArgumentError or log a warning if a conversion is attempted for an undefined pair, rather than silently defaulting to a 1.0 factor?
assert(false, 'Unexpected unit conversion');
return 1.0;
…ad of overlay Committer: Lorenzo Mascia <[email protected]> Changes : modified: packages/smooth_app/lib/pages/hunger_games/question_card.dart modified: packages/smooth_app/lib/pages/hunger_games/question_page.dart modified: packages/smooth_app/lib/pages/preferences/user_preferences_contribute.dart modified: packages/smooth_app/lib/pages/product/product_questions_widget.dart
Committer: Lorenzo Mascia <[email protected]> Changes: modified: packages/smooth_app/lib/pages/hunger_games/question_card.dart modified: packages/smooth_app/lib/pages/hunger_games/question_image_thumbnail.dart modified: packages/smooth_app/lib/pages/hunger_games/question_page.dart modified: packages/smooth_app/lib/pages/preferences/user_preferences_contribute.dart modified: packages/smooth_app/lib/pages/product/product_questions_widget.dart
Committer: Lorenzo Mascia <[email protected]> Changes: modified: packages/smooth_app/lib/pages/hunger_games/question_image_thumbnail.dart modified: packages/smooth_app/lib/pages/hunger_games/question_page.dart modified: packages/smooth_app/lib/pages/preferences/user_preferences_contribute.dart modified: packages/smooth_app/lib/pages/product/product_questions_widget.dart
Co-authored-by: monsieurtanuki <[email protected]>
Committer: Lorenzo Mascia <[email protected]> Changes: modified: lib/pages/hunger_games/question_image_thumbnail.dart modified: lib/pages/hunger_games/question_page.dart
…4446 Changes: modified: packages/smooth_app/lib/pages/product/nutrition_page/nutrition_page.dart new file: packages/smooth_app/lib/pages/product/nutrition_page/nutrition_validator.dart modified: packages/smooth_app/lib/pages/product/nutrition_page/widgets/nutrition_facts_editor.dart modified: packages/smooth_app/lib/pages/text_field_helper.dart
Changes: modified: packages/smooth_app/lib/pages/product/nutrition_page/nutrition_page.dart modified: packages/smooth_app/lib/pages/product/nutrition_page/nutrition_validator.dart
modified: packages/smooth_app/lib/pages/product/nutrition_page/nutrition_page.dart modified: packages/smooth_app/lib/pages/product/nutrition_page/nutrition_validator.dart
LorenzoMascia
force-pushed
the
feat/check-values-before-seding#4446
branch
from
328d5ac to
8a15f1c
Compare
|
I’ve rebased the branch onto the latest develop to resolve all conflicts, and I've applied gemini's suggestions. |
|
Thanks for the update, @LorenzoMascia! It's great to hear that you've rebased the branch and incorporated the suggestions. To ensure everything is looking good after the changes and rebase, I recommend running another review. You can do this by adding a new issue comment with Let me know if you have any other questions! |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature to validate nutrition facts data before saving, which is crucial for data quality. The approach of creating a dedicated NutritionValidator class is a good step towards separating concerns. However, the implementation of the validation logic, particularly the parsing, unit conversion, and normalization, contains critical issues that need to be addressed. Additionally, the lack of unit tests for this core logic is a significant concern.
Summary of Findings
- Critical Validation Logic Errors: The parsing of values with units, the unit conversion logic, and the normalization calculation in
NutritionValidatorare fundamentally flawed. This leads to incorrect validation results. - Serving Size Unit Handling: The code does not correctly handle serving size units (g vs ml) based on product type or
perSizewhen parsing and normalizing, potentially leading to incorrect calculations for liquid products. - Lack of Unit Tests for Validator: There are no unit tests for the critical parsing, unit conversion, and normalization logic in the
NutritionValidatorclass, making it difficult to verify correctness and prevent regressions. - Broad Exception Handling: A generic
catch (e)is used in_mayExitPage, which can hide specific error types and make debugging difficult. - Hardcoded Max Nutrient Values: The maximum nutrient content values are hardcoded in
NutritionValidator, which might make future updates or configuration changes more difficult. - Minor UI Logic Issue: The
onChangedcallback in_NutrientValueCellhas a minor logic issue regarding resetting thevalidflag.
Merge Readiness
This pull request introduces critical correctness issues in the core validation logic and lacks necessary unit tests to ensure the reliability of this feature. Therefore, I recommend that these issues be addressed before merging. I am unable to approve this pull request; please have other reviewers review and approve this code before merging.
| final String servingSize; | ||
| if (_nutritionContainer.perSize == PerSize.serving) { | ||
| servingSize = _nutritionContainer.servingSize; | ||
| } else { | ||
| servingSize = '100.0'; | ||
| } |
There was a problem hiding this comment.
The serving size is determined here. If the perSize is not serving, it defaults to '100.0'. This assumes the base unit is grams. However, for liquid products, the base unit should be milliliters. The validation logic needs to correctly handle both grams and milliliters as base units for normalization, depending on the product type or perSize.
| for (final Nutrient nutrient in _controllers.keys) { | ||
| final TextEditingControllerWithHistory controller = | ||
| _controllers[nutrient]!; | ||
| final String quantity = controller.text.trim(); | ||
| final Unit unit = _nutritionContainer.getUnit(nutrient); | ||
|
|
||
| try { | ||
| if (!validator.validate(nutrient, quantity, unit, servingSize)) { | ||
| error = true; | ||
| controller.valid = false; | ||
| } else { | ||
| controller.valid = true; | ||
| } | ||
| } catch (e) { | ||
| error = true; | ||
| controller.valid = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
This loop iterates through the controllers and calls the validator.validate method. As noted in comments on nutrition_validator.dart, the parsing, unit conversion, and normalization logic within the validator are currently flawed. This means the validation performed here will not be accurate.
| bool validate(Nutrient nutrient, String? qty, Unit unit, String? servSize) { | ||
| final double? quantity = _parseValueWithUnit(qty); | ||
| final double? servingSize = _parseValueWithUnit(servSize); | ||
|
|
||
| if ((quantity == null) || (servingSize == null)) { | ||
| return true; | ||
| } | ||
|
|
||
| final double limit = nutrientMaxContentPer100g[nutrient] ?? 100; | ||
| return _normalize(quantity, unit, nutrient.typicalUnit, servingSize) <= | ||
| limit; |
There was a problem hiding this comment.
This validate method relies on _parseValueWithUnit and _normalize. As detailed in comments on those methods, the logic for parsing values with units and normalizing them per 100g/100ml is currently incorrect, leading to inaccurate validation results.
| double _normalize( | ||
| double quantity, Unit inputUnit, Unit typicalUnit, double servingSize) { | ||
| final double factor = _unitConversionFactor(inputUnit, typicalUnit); | ||
| final double normalized = (quantity * factor) * (100.0 / servingSize); | ||
| return normalized; |
There was a problem hiding this comment.
The normalization logic here is incorrect due to the flawed input from _parseValueWithUnit and potential unit mismatches. The quantity and servingSize parameters are already potentially incorrectly converted to grams by _parseValueWithUnit. The calculation (quantity * factor) * (100.0 / servingSize) does not correctly normalize the quantity per 100g/100ml while accounting for the correct units of the quantity, serving size, and the target limit.
| double? _parseValueWithUnit(String? input) { | ||
| if (input == null || input.isEmpty) { | ||
| return null; | ||
| } | ||
|
|
||
| try { | ||
| input = input.trim().toLowerCase(); | ||
| final RegExp regex = RegExp(r'^([0-9]+\.?[0-9]*)\s*([a-zA-Z%]+)?$'); | ||
| final RegExpMatch? match = regex.firstMatch(input); | ||
| if (match == null) { | ||
| return null; // Invalid format | ||
| } | ||
| final double value = double.parse(match.group(1)!); | ||
| // Default to grams if unit not specified | ||
| final String unitStr = match.group(2) ?? 'g'; | ||
| // Convert unit string to Unit enum | ||
| final Unit unit = UnitHelper.stringToUnit(unitStr) ?? Unit.G; | ||
| // Use _unitConversionFactor to convert from source unit to grams | ||
| return value * _unitConversionFactor(unit, Unit.G); | ||
| } catch (e) { | ||
| return null; // Handle any parsing errors | ||
| } | ||
| } |
There was a problem hiding this comment.
This parsing method has several issues:
- It attempts to convert the parsed value to grams (
value * _unitConversionFactor(unit, Unit.G)). This is incorrect; the raw value and its unit should be returned, and conversion should happen later based on the nutrient's typical unit and the serving size unit. - It assumes 'g' as the default unit if none is specified. This is incorrect for serving size when the product is liquid (should be ml).
- It uses
UnitHelper.stringToUnit(unitStr) ?? Unit.Gwhich means any unrecognized unit string will default to grams, which is likely wrong.
| Nutrient.elaidicAcid: 5.0, | ||
| Nutrient.gammaLinolenicAcid: 5.0, | ||
| Nutrient.gondoicAcid: 5.0, | ||
| Nutrient.lignocericAcid: 5.0, | ||
| Nutrient.meadAcid: 5.0, | ||
| Nutrient.melissicAcid: 5.0, | ||
| Nutrient.montanicAcid: 5.0, | ||
| Nutrient.nervonicAcid: 5.0, | ||
| Nutrient.transFat: 100.0, | ||
| }; | ||
|
|
||
| bool validate(Nutrient nutrient, String? qty, Unit unit, String? servSize) { | ||
| final double? quantity = _parseValueWithUnit(qty); | ||
| final double? servingSize = _parseValueWithUnit(servSize); | ||
|
|
||
| if ((quantity == null) || (servingSize == null)) { | ||
| return true; | ||
| } | ||
|
|
||
| final double limit = nutrientMaxContentPer100g[nutrient] ?? 100; | ||
| return _normalize(quantity, unit, nutrient.typicalUnit, servingSize) <= | ||
| limit; | ||
| } | ||
|
|
||
| double _normalize( | ||
| double quantity, Unit inputUnit, Unit typicalUnit, double servingSize) { | ||
| final double factor = _unitConversionFactor(inputUnit, typicalUnit); | ||
| final double normalized = (quantity * factor) * (100.0 / servingSize); | ||
| return normalized; | ||
| } | ||
|
|
||
| double _unitConversionFactor(Unit from, Unit to) { | ||
| if (from == to) { | ||
| return 1.0; | ||
| } | ||
| // Mass conversions | ||
| if (from == Unit.G && to == Unit.MILLI_G) { | ||
| return 1000.0; | ||
| } | ||
| if (from == Unit.MILLI_G && to == Unit.G) { | ||
| return 0.001; | ||
| } | ||
| if (from == Unit.MICRO_G && to == Unit.G) { | ||
| return 0.000001; | ||
| } | ||
| if (from == Unit.G && to == Unit.MICRO_G) { | ||
| return 1000000.0; | ||
| } | ||
| if (from == Unit.MILLI_G && to == Unit.MICRO_G) { | ||
| return 1000.0; | ||
| } | ||
| if (from == Unit.MICRO_G && to == Unit.MILLI_G) { | ||
| return 0.001; | ||
| } | ||
| // Energy conversions | ||
| if (from == Unit.KCAL && to == Unit.KJ) { | ||
| return 4.184; | ||
| } | ||
| if (from == Unit.KJ && to == Unit.KCAL) { | ||
| return 0.239006; | ||
| } | ||
| // Volume conversions | ||
| if (from == Unit.L && to == Unit.MILLI_L) { | ||
| return 1000.0; | ||
| } | ||
| if (from == Unit.MILLI_L && to == Unit.L) { | ||
| return 0.001; | ||
| } | ||
| // Percentage is dimensionless - no conversion needed | ||
| if (from == Unit.PERCENT || to == Unit.PERCENT) { | ||
| return 1.0; | ||
| } | ||
|
|
||
| return 1.0; | ||
| } | ||
|
|
||
| double? _parseValueWithUnit(String? input) { | ||
| if (input == null || input.isEmpty) { | ||
| return null; | ||
| } | ||
|
|
||
| try { | ||
| input = input.trim().toLowerCase(); | ||
| final RegExp regex = RegExp(r'^([0-9]+\.?[0-9]*)\s*([a-zA-Z%]+)?$'); | ||
| final RegExpMatch? match = regex.firstMatch(input); | ||
| if (match == null) { | ||
| return null; // Invalid format | ||
| } | ||
| final double value = double.parse(match.group(1)!); | ||
| // Default to grams if unit not specified | ||
| final String unitStr = match.group(2) ?? 'g'; | ||
| // Convert unit string to Unit enum | ||
| final Unit unit = UnitHelper.stringToUnit(unitStr) ?? Unit.G; | ||
| // Use _unitConversionFactor to convert from source unit to grams | ||
| return value * _unitConversionFactor(unit, Unit.G); | ||
| } catch (e) { | ||
| return null; // Handle any parsing errors | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The NutritionValidator class implements core logic for parsing, unit conversion, and validation. There are no unit tests provided for this class. Given the complexity and critical nature of this logic, comprehensive unit tests are essential to ensure correctness, cover edge cases, and prevent regressions.
| } catch (e) { | ||
| error = true; | ||
| controller.valid = false; | ||
| } |
There was a problem hiding this comment.
Catching a generic catch (e) is generally discouraged as it can hide unexpected errors. It would be better to catch more specific exceptions if possible, or at least log the error (e) for debugging purposes.
| final Map<Nutrient, double> nutrientMaxContentPer100g = <Nutrient, double>{ | ||
| Nutrient.salt: 100.0, | ||
| Nutrient.sodium: 40.0, | ||
| Nutrient.fiber: 100.0, | ||
| Nutrient.sugars: 100.0, | ||
| Nutrient.addedSugars: 100.0, | ||
| Nutrient.fat: 100.0, | ||
| Nutrient.saturatedFat: 100.0, | ||
| Nutrient.proteins: 100.0, | ||
| Nutrient.energyKCal: 900.0, | ||
| Nutrient.energyKJ: 3700.0, | ||
| Nutrient.carbohydrates: 100.0, | ||
| Nutrient.caffeine: 80.0, | ||
| Nutrient.calcium: 5000.0, | ||
| Nutrient.iron: 100.0, | ||
| Nutrient.vitaminC: 6000.0, | ||
| Nutrient.magnesium: 2000.0, | ||
| Nutrient.phosphorus: 5000.0, | ||
| Nutrient.potassium: 5000.0, | ||
| Nutrient.zinc: 1000.0, | ||
| Nutrient.copper: 1000.0, | ||
| Nutrient.selenium: 1000.0, | ||
| Nutrient.vitaminA: 3000.0, | ||
| Nutrient.vitaminE: 1000.0, | ||
| Nutrient.vitaminD: 100.0, | ||
| Nutrient.vitaminB1: 5.0, | ||
| Nutrient.vitaminB2: 5.0, | ||
| Nutrient.vitaminPP: 50.0, | ||
| Nutrient.vitaminB6: 5.0, | ||
| Nutrient.vitaminB12: 500.0, | ||
| Nutrient.vitaminB9: 1000.0, | ||
| Nutrient.vitaminK: 1000.0, | ||
| Nutrient.cholesterol: 500.0, | ||
| Nutrient.butyricAcid: 5.0, | ||
| Nutrient.caproicAcid: 5.0, | ||
| Nutrient.caprylicAcid: 5.0, | ||
| Nutrient.capricAcid: 5.0, | ||
| Nutrient.lauricAcid: 5.0, | ||
| Nutrient.myristicAcid: 10.0, | ||
| Nutrient.palmiticAcid: 20.0, | ||
| Nutrient.stearicAcid: 20.0, | ||
| Nutrient.oleicAcid: 80.0, | ||
| Nutrient.linoleicAcid: 70.0, | ||
| Nutrient.docosahexaenoicAcid: 5.0, | ||
| Nutrient.eicosapentaenoicAcid: 5.0, | ||
| Nutrient.erucicAcid: 5.0, | ||
| Nutrient.monounsaturatedFat: 80.0, | ||
| Nutrient.polyunsaturatedFat: 70.0, | ||
| Nutrient.alcohol: 100.0, | ||
| Nutrient.pantothenicAcid: 20.0, | ||
| Nutrient.biotin: 500.0, | ||
| Nutrient.chloride: 1000.0, | ||
| Nutrient.chromium: 500.0, | ||
| Nutrient.fluoride: 10.0, | ||
| Nutrient.iodine: 1000.0, | ||
| Nutrient.manganese: 20.0, | ||
| Nutrient.molybdenum: 1000.0, | ||
| Nutrient.omega3: 30.0, | ||
| Nutrient.omega6: 70.0, | ||
| Nutrient.omega9: 80.0, | ||
| Nutrient.betaCarotene: 50.0, | ||
| Nutrient.bicarbonate: 500.0, | ||
| Nutrient.sugarAlcohol: 100.0, | ||
| Nutrient.alphaLinolenicAcid: 10.0, | ||
| Nutrient.arachidicAcid: 5.0, | ||
| Nutrient.arachidonicAcid: 5.0, | ||
| Nutrient.behenicAcid: 5.0, | ||
| Nutrient.ceroticAcid: 5.0, | ||
| Nutrient.dihomoGammaLinolenicAcid: 5.0, | ||
| Nutrient.elaidicAcid: 5.0, | ||
| Nutrient.gammaLinolenicAcid: 5.0, | ||
| Nutrient.gondoicAcid: 5.0, | ||
| Nutrient.lignocericAcid: 5.0, | ||
| Nutrient.meadAcid: 5.0, | ||
| Nutrient.melissicAcid: 5.0, | ||
| Nutrient.montanicAcid: 5.0, | ||
| Nutrient.nervonicAcid: 5.0, | ||
| Nutrient.transFat: 100.0, | ||
| }; |
There was a problem hiding this comment.
The maximum nutrient content values are hardcoded in this map. While this works, consider if these values might need to be configurable or potentially sourced from an external API or configuration file in the future for easier updates and maintenance.
| double _unitConversionFactor(Unit from, Unit to) { | ||
| if (from == to) { | ||
| return 1.0; | ||
| } | ||
| // Mass conversions | ||
| if (from == Unit.G && to == Unit.MILLI_G) { | ||
| return 1000.0; | ||
| } | ||
| if (from == Unit.MILLI_G && to == Unit.G) { | ||
| return 0.001; | ||
| } | ||
| if (from == Unit.MICRO_G && to == Unit.G) { | ||
| return 0.000001; | ||
| } | ||
| if (from == Unit.G && to == Unit.MICRO_G) { | ||
| return 1000000.0; | ||
| } | ||
| if (from == Unit.MILLI_G && to == Unit.MICRO_G) { | ||
| return 1000.0; | ||
| } | ||
| if (from == Unit.MICRO_G && to == Unit.MILLI_G) { | ||
| return 0.001; | ||
| } | ||
| // Energy conversions | ||
| if (from == Unit.KCAL && to == Unit.KJ) { | ||
| return 4.184; | ||
| } | ||
| if (from == Unit.KJ && to == Unit.KCAL) { | ||
| return 0.239006; | ||
| } | ||
| // Volume conversions | ||
| if (from == Unit.L && to == Unit.MILLI_L) { | ||
| return 1000.0; | ||
| } | ||
| if (from == Unit.MILLI_L && to == Unit.L) { | ||
| return 0.001; | ||
| } | ||
| // Percentage is dimensionless - no conversion needed | ||
| if (from == Unit.PERCENT || to == Unit.PERCENT) { | ||
| return 1.0; | ||
| } | ||
|
|
||
| return 1.0; | ||
| } |
There was a problem hiding this comment.
The _unitConversionFactor method provides conversions only between a limited set of units (g, mg, mcg, kcal, kj, l, ml, %). It does not include conversions for other units like IU (International Units) which are common for vitamins. A more comprehensive unit conversion system might be needed.
| onChanged: (String? value) { | ||
| if (!controller.valid) { | ||
| controller.valid = true; | ||
| } |
There was a problem hiding this comment.
The onChanged callback only sets controller.valid = true if !controller.valid is true. It might be simpler and more robust to always set controller.valid = true when the text changes, as the user is now editing the value, and the previous validation status is no longer relevant to the current input.
onChanged: (String? value) {
controller.valid = true;
},
Changes:
modified: packages/smooth_app/lib/pages/product/nutrition_page/nutrition_page.dart
new file: packages/smooth_app/lib/pages/product/nutrition_page/nutrition_validator.dart
modified: packages/smooth_app/lib/pages/product/nutrition_page/widgets/nutrition_facts_editor.dart
modified: packages/smooth_app/lib/pages/text_field_helper.dart
What
Created a class to perform nutrient validation for each product. The NutritionValidator class normalizes the input values based on the weight of the Serving Size, converting them into values relative to 100g or 100ml. The normalized values are then compared to the maximum values typically found in food products. The values are verified and displayed in red when the save button is pressed.
Screenshot
Fixes bug(s)
Enhancement: Nutrition facts: check values before sending them #4446
Part of