Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: #2561 - fixed value+unit management in nutrient page #2568

Merged
merged 1 commit into from
Jul 19, 2022

Conversation

monsieurtanuki
Copy link
Contributor

Impacted files:

  • nutrition_page_loaded.dart: little fix - now we consider the page edited also when at least one unit changed.
  • nutrition_container.dart: fix - now we populate units and we don't convert values when saving to the server; added initial units field for the "were units edited?" check; refactored

What

  • There were errors in the value+unit management in the nutrient page.
  • One good reason for those errors is that the server is not very user-friendly with weight data
    • when you read data (value + unit), the value is always in grams and the unit is the unit you should display (e.g. 0.12 and mg, which means 0.12 grams to be displayed in mg, therefore as "120 mg"). You have to convert the value from grams into the given unit.
    • when you write data, the server works different way - if you want to say "120 mg" you say 120 and mg (more intuitive)
  • This PR fixed conversion errors when the unit was not grams.
  • Beyond the errors fixes, there's something intrinsically twisted with sodium and salt - it looks like for the server the main value is "salt" and "sodium" is computed, and in the case of the issue product only sodium was indicated. Even if you change the sodium value, the previous salt value takes precedence and overwrite the sodium value.
    Should we automatically compute one from the other in the app?

Part of

Impacted files:
* `nutrition_page_loaded.dart`: little fix - now we consider the page edited also when at least one unit changed.
* `nutrition_container.dart`: fix - now we populate units and we don't convert values when saving to the server; added initial units field for the "were units edited?" check; refactored
@monsieurtanuki monsieurtanuki requested a review from a team as a code owner July 10, 2022 16:30
@codecov-commenter
Copy link

Codecov Report

Merging #2568 (0254804) into develop (2ea0da3) will decrease coverage by 1.30%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           develop   #2568      +/-   ##
==========================================
- Coverage     8.86%   7.56%   -1.31%     
==========================================
  Files          161     210      +49     
  Lines         6623   10090    +3467     
==========================================
+ Hits           587     763     +176     
- Misses        6036    9327    +3291     
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 60.00% <0.00%> (-22.98%) ⬇️
...p/lib/generic_lib/dialogs/smooth_alert_dialog.dart 15.29% <0.00%> (-18.92%) ⬇️
...mooth_app/lib/data_models/product_preferences.dart 24.65% <0.00%> (-6.78%) ⬇️
packages/smooth_app/lib/main.dart 14.16% <0.00%> (-3.73%) ⬇️
.../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%) ⬇️
...p/lib/pages/onboarding/consent_analytics_page.dart 0.00% <0.00%> (-1.57%) ⬇️
...smooth_app/lib/pages/product/new_product_page.dart 0.00% <0.00%> (-0.88%) ⬇️
... and 219 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 e4ea159...0254804. Read the comment docs.

void _setUnit(
final String nutrientId,
final Unit unit, {
required final bool init,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you document this field please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe in comments here:

  • it's a private method
  • the parameter has a decent name (init? "it must have something to do with the init phase")
  • the method is used only twice, first time at init time with init: true, and later with init: false

final String text = controller.value.text;
final double? value = getValue(key);
if (value == null) {
if (text != '') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isNotEmpty wouldn't be better here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it would have been the same.

Copy link
Member

@M123-dev M123-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @monsieurtanuki

@monsieurtanuki monsieurtanuki merged commit 750f429 into openfoodfacts:develop Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants