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: 4575 - country is now always populated #4591

Merged
merged 3 commits into from
Aug 26, 2023

Conversation

monsieurtanuki
Copy link
Contributor

What

  • For historical reasons the country was nullable, but in the rest of the app we considered it as not null.
  • Now we always have a value for it. Worst case scenario: it will be defaulted at init time to Switzerland. With of course the possibility to change the value.
  • En passant, I fixed a bug about UK (we use 'uk' when everybody else uses 'gb', which could provoke an "I don't know this country" error at init time)

Fixes bug(s)

@monsieurtanuki monsieurtanuki requested a review from a team as a code owner August 23, 2023 13:57
@github-actions github-actions bot added 🥫 Product page Product addition The easier it is to add a product and get Nutri-Score, Eco-Score, the happier the users. 🤗 Onboarding We need to onboard users on how the app works, but also on content like Nutri-Score, Eco-Score… labels Aug 23, 2023
final UserPreferences userPreferences,
) async {
const OpenFoodFactsCountry defaultCountry =
OpenFoodFactsCountry.SWITZERLAND; // why not?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this one will be the subject to discussions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need a country.
The normal behavior is to use the device's country.
If everything falls apart, only then will we use the default country. And the user sees in the onboarding that it can be changed to what is more relevant.

I have no problem changing the default country (I would even say "fallback country"). Just pick a "more relevant" country.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that was exactly, the idea behind my comment.

What's the best value to use @teolemon and @stephanegigandet?

Copy link
Member

Choose a reason for hiding this comment

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

In which case could it be null ? In that case, we should have a special screen appearing, asking to fix the issue ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be null only in once case: when the app is started the very first time, we need to pre-select the country of the user. For that we use some platform data (PlatformDispatcher.instance.locale.countryCode). If this platform data is not available or is a code that does not match "our" countries, the country will be null.
Only in that case will we use the default country (so far, Switzerland).
And in the onboarding, the user will see the pre-selected country and will have the obvious possibility to change it.

Copy link
Member

Choose a reason for hiding this comment

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

I'm proposing France, since we have contributors monitoring the influx of new products that could be a consequence of that

await userPreferences.setUserCountryCode(isoCode);
}
}

// TODO(monsieurtanuki): move to off-dart
Copy link
Member

Choose a reason for hiding this comment

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

Good catch

Copy link
Member

Choose a reason for hiding this comment

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

Why did already get answers in Ukrainian ?

Copy link
Member

Choose a reason for hiding this comment

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

answers in Ukrainian, but regarding the UK for Eco-Score adjustments ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://fr.wikipedia.org/wiki/ISO_3166-1:

  • Ukraine is 'ua'
  • United Kingdom is 'gb'

For some reason, United Kingdom is 'uk' in openfoodfacts.

final UserPreferences userPreferences,
) async {
const OpenFoodFactsCountry defaultCountry =
OpenFoodFactsCountry.SWITZERLAND; // why not?
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
OpenFoodFactsCountry.SWITZERLAND; // why not?
OpenFoodFactsCountry.FRANCE; // not ideal, but we have many contributors monitoring France

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@teolemon OK for France. I haven't used your code suggestion because I wanted to avoid to reformat manually (your comment being too long).
I didn't work out, I have to reformat anyway...
We'll be ok in 5 minutes.

@codecov-commenter
Copy link

Codecov Report

Merging #4591 (a28d322) into develop (8193cf1) will decrease coverage by 0.07%.
Report is 9 commits behind head on develop.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop    #4591      +/-   ##
===========================================
- Coverage    10.32%   10.26%   -0.07%     
===========================================
  Files          296      297       +1     
  Lines        15410    15502      +92     
===========================================
  Hits          1591     1591              
- Misses       13819    13911      +92     
Files Changed Coverage Δ
...mooth_app/lib/background/background_task_crop.dart 0.00% <0.00%> (ø)
...th_app/lib/background/background_task_details.dart 0.00% <0.00%> (ø)
.../background/background_task_download_products.dart 0.00% <0.00%> (ø)
...p/lib/background/background_task_full_refresh.dart 0.00% <0.00%> (ø)
...p/lib/background/background_task_hunger_games.dart 0.00% <0.00%> (ø)
...ooth_app/lib/background/background_task_image.dart 0.00% <0.00%> (ø)
...th_app/lib/background/background_task_offline.dart 0.00% <0.00%> (ø)
.../lib/background/background_task_refresh_later.dart 0.00% <0.00%> (ø)
...p/lib/background/background_task_top_barcodes.dart 0.00% <0.00%> (ø)
...h_app/lib/background/background_task_unselect.dart 0.00% <0.00%> (ø)
... and 6 more

... and 27 files with indirect coverage changes

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

@monsieurtanuki
Copy link
Contributor Author

Thank you @teolemon for your review!

@monsieurtanuki monsieurtanuki merged commit d804222 into openfoodfacts:develop Aug 26, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤗 Onboarding We need to onboard users on how the app works, but also on content like Nutri-Score, Eco-Score… Product addition The easier it is to add a product and get Nutri-Score, Eco-Score, the happier the users. 🥫 Product page
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Weird issues
4 participants