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: simplified access to raw product images #839

Merged
merged 8 commits into from
Dec 24, 2023

Conversation

monsieurtanuki
Copy link
Contributor

What

  • This PR gives a direct access to "raw" (=uploaded) pictures at the same time as "main" pictures (e.g. 'front_en') (that are actually computed from raw pictures).
  • Those pictures will be in field images, that used to have only "main" pictures for historical reasons.
  • With this PR we have more flexibility: instead of having to load in a second step all the "raw" images, we directly get them from the IMAGES product field.
  • The target is to simplify in Smoothie the access to the product image gallery: the main pictures and all the other pictures, without needing an extra server access.

Part of

Impacted files

  • api_get_product_image_ids_test.dart: used new method and extended tests to all/main/raw images
  • api_get_product_test.dart: minor refactoring
  • api_json_to_from_test.dart: extended tests to all/main/raw images
  • image_helper.dart: minor refactoring
  • json_helper.dart: integrated "raw" images in addition to "main" images
  • open_food_api_client.dart: deprecated method getProductImageIds
  • product.dart: integrated "raw" images in addition to "main" images
  • product.g.dart: generated
  • product_helper.dart: minor refactoring
  • product_image.dart: now can include "raw" images

Impacted files:
* `api_get_product_image_ids_test.dart`: used new method and extended tests to all/main/raw images
* `api_get_product_test.dart`: minor refactoring
* `api_json_to_from_test.dart`: extended tests to all/main/raw images
* `image_helper.dart`: minor refactoring
* `json_helper.dart`: integrated "raw" images in addition to "main" images
* `open_food_api_client.dart`: deprecated method `getProductImageIds`
* `product.dart`: integrated "raw" images in addition to "main" images
* `product.g.dart`: generated
* `product_helper.dart`: minor refactoring
* `product_image.dart`: now can include "raw" images
@teolemon teolemon requested review from g123k and M123-dev December 15, 2023 12:15
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.

Thanks a lot @monsieurtanuki, I like the new implementation way more without the helper class.

I've clarified some comments as I was kinda confused by them without reading the doc comment for ProductImage. Feel free to have a look at them

lib/src/model/product.dart Outdated Show resolved Hide resolved
lib/src/model/product.dart Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Dec 24, 2023

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Comparison is base (0482f63) 75.93% compared to head (c8e6a12) 75.87%.

Files Patch % Lines
lib/src/utils/image_helper.dart 0.00% 10 Missing ⚠️
lib/src/model/product_image.dart 78.57% 6 Missing ⚠️
lib/src/utils/json_helper.dart 90.47% 4 Missing ⚠️
lib/src/open_food_api_client.dart 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #839      +/-   ##
==========================================
- Coverage   75.93%   75.87%   -0.07%     
==========================================
  Files         224      224              
  Lines        7817     7880      +63     
==========================================
+ Hits         5936     5979      +43     
- Misses       1881     1901      +20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@monsieurtanuki monsieurtanuki merged commit 3350251 into openfoodfacts:master Dec 24, 2023
3 checks passed
@monsieurtanuki
Copy link
Contributor Author

Thank you @M123-dev for your review!

I've changed the comments according to your suggestions.

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

Successfully merging this pull request may close these issues.

4 participants