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: productName best localized choice #4746

Merged
merged 3 commits into from
Nov 24, 2023

Conversation

monsieurtanuki
Copy link
Contributor

What

  • About a week ago tests started to fail in openfoodfacts-dart (cf. Default productName and languages openfoodfacts-dart#816, fix: 816 - use localized versions of productName and ingredientsText openfoodfacts-dart#817)
  • It looks like from now on we cannot rely on productName to match the specified language, even if a localization is available. Looks like the product name is computed when the data is input (as product name or as localized product names), and then regardless of the language of the "get" queries later we always get the same product name.
  • A simple fix (that makes sense anyway) is to rely first on the localizations of the product name, and then on the single product name field as a fallback. It used to be coded the other way around, which started not to work about a week ago.

Screenshot

The product name changes between the 2 screenshots (it was not the case before this PR):

The product was downloaded in an app in English Then the app language was switched to French
Screenshot_1698486247 Screenshot_1698485167

Impacted files

  • nutrition_page_loaded.dart: minor refactoring
  • product_cards_helper.dart: now the product name is taken from the localized version first; minor refactoring

Impacted files:
* `nutrition_page_loaded.dart`: minor refactoring
* `product_cards_helper.dart`: now the product name is taken from the localized version first; minor refactoring
@codecov-commenter
Copy link

codecov-commenter commented Oct 28, 2023

Codecov Report

Merging #4746 (22b4bed) into develop (c28f8d2) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           develop   #4746      +/-   ##
==========================================
- Coverage     9.89%   9.89%   -0.01%     
==========================================
  Files          312     312              
  Lines        15809   15810       +1     
==========================================
  Hits          1565    1565              
- Misses       14244   14245       +1     
Files Coverage Δ
...h_app/lib/pages/product/nutrition_page_loaded.dart 0.00% <ø> (ø)
...s/smooth_app/lib/helpers/product_cards_helper.dart 0.00% <0.00%> (ø)

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@stephanegigandet
Copy link
Contributor

  • It looks like from now on we cannot rely on productName to match the specified language, even if a localization is available. Looks like the product name is computed when the data is input (as product name or as localized product names), and then regardless of the language of the "get" queries later we always get the same product name.

There hasn't been a change on the server side. product_name contains the name of the product in its main language (indicated by the lc field of the product), it does not changed based on the lc field of the request.

@monsieurtanuki
Copy link
Contributor Author

There hasn't been a change on the server side.

Strange, because there was no change on our code either and suddenly tests started to fail.

product_name contains the name of the product in its main language (indicated by the lc field of the product)

Aha, we were not aware of the existence of the lc product field!
Could we say that:

  • lc is the main language of the product
  • lang is the language used for the latest product update

@stephanegigandet Could you please confirm that, so that I can add field lc to the product fields in off-dart?

it does not changed based on the lc field of the request.

Then our tests were wrong from the very start. But still passed for some reason.

That said, your comment confirms that the current PR will be an improvement in Smoothie: instead of displaying always the "main" product_name (that always relates to the main product language called lc), we display the localized product name (with product_name as fallback).

@monsieurtanuki
Copy link
Contributor Author

@teolemon review ping

@stephanegigandet
Copy link
Contributor

stephanegigandet commented Nov 6, 2023

@monsieurtanuki When you read products, the "lc" field and the "lang" field you get back in the JSON have the same value: the main language of the product. When you write products, you have to use "lang" to specify the main language of the product, and "lc" is the language you are sending fields for (e.g. lc : fr, lang : de, product_name : nom en français)

@monsieurtanuki
Copy link
Contributor Author

@stephanegigandet Thank you for this clarification.
Then we shouldn't add lc as a product field in off-dart: when we read it it's the same value as lang anyway, and when we write we already have a lc parameter (not as a product field, but as a query parameter).

@monsieurtanuki
Copy link
Contributor Author

@teolemon Now that the possible confusion between lc and lang has been dealt with, we would be better off with the localized product name first, and then the main language product name as fallback. Like coded in this PR.

@monsieurtanuki
Copy link
Contributor Author

@teolemon Still not interested in using the best product name?

@teolemon
Copy link
Member

@g123k @M123-dev could one of you have a look codewise ?

product.productName ??
product.productNameInLanguages?[ProductQuery.getLanguage()] ??
/// Returns a trimmed version of the string, or null if null or empty.
String? _clearString(final String? string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This kind of feature may be useful elsewhere in the app.
I suggests to move-it to a helper-like class, or even better as an extension

Copy link
Member

@teolemon teolemon left a comment

Choose a reason for hiding this comment

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

Factorization would indeed be nice @monsieurtanuki , but approving anyway

@monsieurtanuki monsieurtanuki merged commit b60caf2 into openfoodfacts:develop Nov 24, 2023
6 checks passed
@monsieurtanuki
Copy link
Contributor Author

Thank you guys for your comments and reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants