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

feat: Add localized fields for conservation conditions and customer service in Product object #1020

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

AffanShaikhsurab
Copy link

What

  • Added 'conservation_conditions_languages' and 'customer_service_languages' fields to the Product object for better multilingual support.
  • Updated the logic to handle localization using language-based keys, similar to product name fields.

Screenshot

Fixes bug(s)

Part of

…vice in Product object

- Added 'conservation_conditions_languages' and 'customer_service_languages' fields to the Product object for better multilingual support.
- Fixed issue with handling French-specific fields by using language-based keys.
@AffanShaikhsurab AffanShaikhsurab requested a review from a team as a code owner January 22, 2025 07:44
@AffanShaikhsurab AffanShaikhsurab changed the title Add localized fields for conservation conditions and customer service in Product object feat: Add localized fields for conservation conditions and customer service in Product object Jan 22, 2025
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Hi @AffanShaikhsurab!
I didn't get that: even product.g.dart wasn't generated? Without it we cannot do anything.
Besides, reviewing your code, I saw that we don't need 3 product fields for each of those minor labels: the "all languages" is more than enough.

lib/src/utils/product_fields.dart Outdated Show resolved Hide resolved
lib/src/utils/product_fields.dart Outdated Show resolved Hide resolved
lib/src/utils/product_fields.dart Outdated Show resolved Hide resolved
lib/src/utils/product_fields.dart Outdated Show resolved Hide resolved
lib/src/utils/product_fields.dart Outdated Show resolved Hide resolved
lib/src/model/product.dart Outdated Show resolved Hide resolved
lib/src/model/product.dart Outdated Show resolved Hide resolved
lib/src/model/product.dart Outdated Show resolved Hide resolved
lib/src/model/product.dart Outdated Show resolved Hide resolved
@AffanShaikhsurab
Copy link
Author

AffanShaikhsurab commented Jan 22, 2025

soory for that . i have removed all the unwanted fields . But still when i try to rebuild its still casusing the same error :
am i doing something wrong ?

PS C:\Users\affan\Affan Projects\open source\open\openfoodfacts-dart> dart run build_runner build
[INFO] Generating build script completed, took 207ms
[WARNING] Invalidated precompiled build script due to missing asset graph.
[INFO] Precompiling build script... completed, took 803ms
[INFO] Building new asset graph completed, took 578ms
[INFO] Found 59 declared outputs which already exist on disk. This is likely because the.dart_tool/build folder was deleted, or you are submitting generated files to your source repository.
Delete these files?
1 - Delete
2 - Cancel build
3 - List conflicts

@AffanShaikhsurab
Copy link
Author

AffanShaikhsurab commented Jan 23, 2025

i have created the product.g.dart using the dart run build_runner build but it still giving me error while testing ! should i use another command to generate the .g.dart files ?

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Hi @AffanShaikhsurab!
Looks good to me, but the generated code is not compatible. My bad. cf. #1024.

@AffanShaikhsurab
Copy link
Author

Should I need to change anything

@monsieurtanuki
Copy link
Contributor

Should I need to change anything

Now you need to "Update your project", which will include #1024, and generate again the files - especially product.g.dart.

@AffanShaikhsurab
Copy link
Author

Should I need to change anything

Now you need to "Update your project", which will include #1024, and generate again the files - especially product.g.dart.

thanks for solving the error

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Hi @AffanShaikhsurab!
Looks good, but the code must be tested.
For instance in api_get_product_test.dart.

@AffanShaikhsurab
Copy link
Author

can you please check if the tests are okay or they need any changes ?

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Thank you @AffanShaikhsurab for your latest changes!
Please have a look at my comments, so that I can approve your PR.

test/api_get_product_test.dart Show resolved Hide resolved
expect(result.status, ProductResultV3.statusSuccess);
expect(result.product, isNotNull);

// Check conservation conditions in languages
Copy link
Contributor

Choose a reason for hiding this comment

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

Please get rid of useless comments.

test/api_get_product_test.dart Outdated Show resolved Hide resolved
test/api_get_product_test.dart Outdated Show resolved Hide resolved
test/api_get_product_test.dart Outdated Show resolved Hide resolved
test/api_get_product_test.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

@AffanShaikhsurab Please systematically have your code formatted.
Screenshot_20250124_180217_Chrome
Besides, my suggestion regarding languages was to use JAPANESE and expect all languages. You use a language called ALL_LANGUAGES, I couldn't find it.

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

@AffanShaikhsurab As expected:
Screenshot_20250124_180802_Chrome
You're supposed to run at least your own tests.

@AffanShaikhsurab
Copy link
Author

@AffanShaikhsurab Please systematically have your code formatted. Screenshot_20250124_180217_Chrome Besides, my suggestion regarding languages was to use JAPANESE and expect all languages. You use a language called ALL_LANGUAGES, I couldn't find it.

Really sorry about that! I must have missed it somehow. I'll make sure to pay closer attention next time. Thanks for pointing it out!

@AffanShaikhsurab
Copy link
Author

@AffanShaikhsurab As expected: Screenshot_20250124_180802_Chrome You're supposed to run at least your own tests.

I've updated the test case to use the Japanese language and allow for other languages as well. Could you please check if it's correct? Apologies for the inconvenience, as I'm still getting familiar with the repository. I'll make sure this doesn't happen again in the future. Thanks for your patience!

@monsieurtanuki
Copy link
Contributor

@AffanShaikhsurab What about running locally your test at least once, just to be sure:
image

@AffanShaikhsurab
Copy link
Author

@AffanShaikhsurab What about running locally your test at least once, just to be sure: image

I just wanted to clarify that these tests were failing for me even before. When the repository is forked and the tests are run, they still fail. So, it doesn’t seem to be an issue caused by me, as the failures appear to have been there from the start. Could you kindly check how we might be able to fix this? Thank you so much!

@monsieurtanuki
Copy link
Contributor

I just wanted to clarify that these tests were failing for me even before.

@AffanShaikhsurab Tests are often failing globally, that's a fact. Sometimes it's because server data changed, sometimes because the server is not reachable. We're not careful enough about them, and that can give the wrong impression that we don't care about tests, that they always fail, and that may be discouraging. I'll give you that.

But I'm talking more specifically about your new test, that fails not because of the server, but apparently because of the code.

So, please run just your test, and make sure it passes.

@AffanShaikhsurab
Copy link
Author

I just wanted to clarify that these tests were failing for me even before.

@AffanShaikhsurab Tests are often failing globally, that's a fact. Sometimes it's because server data changed, sometimes because the server is not reachable. We're not careful enough about them, and that can give the wrong impression that we don't care about tests, that they always fail, and that may be discouraging. I'll give you that.

But I'm talking more specifically about your new test, that fails not because of the server, but apparently because of the code.

So, please run just your test, and make sure it passes.

Thank you for pointing that out! I’ve taken the time to test my changes specifically, and my test appears to be passing successfully. Please let me know if there’s anything else I should review or address.

image

@AffanShaikhsurab
Copy link
Author

AffanShaikhsurab commented Jan 25, 2025

The only tests that are failing at the moment are these two, and they were already failing prior to this.

image

@monsieurtanuki
Copy link
Contributor

@AffanShaikhsurab I'm afraid there's a misunderstanding:

  • the test did not pass in github, as you can see in https://github.com/openfoodfacts/openfoodfacts-dart/actions/runs/12954923834/job/36160031718?pr=1020
  • this is probably because in your code there's nothing about your new fields in method setLanguageString in product.dart
  • as explained earlier, the test would be to ask for a product in Japanese and check that non Japanese labels are available (e.g. in French), and currently you don't test that there are non Japanese labels, so you should add those tests

…l tests

- Updated setLanguageString in product.dart to include new fields.
- Added tests to verify availability of non-primary language labels.
@AffanShaikhsurab
Copy link
Author

@AffanShaikhsurab I'm afraid there's a misunderstanding:

  • the test did not pass in github, as you can see in https://github.com/openfoodfacts/openfoodfacts-dart/actions/runs/12954923834/job/36160031718?pr=1020
  • this is probably because in your code there's nothing about your new fields in method setLanguageString in product.dart
  • as explained earlier, the test would be to ask for a product in Japanese and check that non Japanese labels are available (e.g. in French), and currently you don't test that there are non Japanese labels, so you should add those tests

I’m sorry for the misunderstanding earlier; you’re absolutely right. I had missed that point.

I’ve now added the French language test to verify that non-Japanese labels (e.g., in French) are available. Additionally, I’ve updated the setLanguageString method in product.dart to include support for the new fields.

Could you please take a look and let me know if there’s anything I’m still missing?

Also, I’d appreciate any advice on how I can communicate better and avoid such misunderstandings in the future. Thank you for pointing this out and for your guidance!

@monsieurtanuki
Copy link
Contributor

image
@AffanShaikhsurab To be honest, I'm a bit puzzled.
It's hard to believe that you did run the tests as the code syntax is not even correct.
I assume that to save time you sometimes skip basic steps like checking that the code actually runs: on the editor you'd see that there's an error. I'm not even talking about correct results.
As a reviewer, I spend time to review your code and write comments, in a positive way as much as possible. I'm not even getting paid for that: that's my leisure time you're using.
Have a nice day.

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

Successfully merging this pull request may close these issues.

2 participants