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: 831 - Prices getLocation, getPriceProduct, getStatus methods #855

Merged
merged 9 commits into from
Feb 13, 2024

Conversation

monsieurtanuki
Copy link
Contributor

What

  • Implementation and tests of new methods for the Prices API: getLocation, getPriceProduct and getStatus.
  • Those are the last READ methods that needed to be implemented.
  • In a next PR, the WRITE methods will be implemented, which somehow includes the authentication.

Part of

Files

New files:

  • location.dart: Location object in the Prices API.
  • location.g.dart: generated
  • price_product.dart: Product object in the Prices API.
  • price_product.g.dart: generated

Impacted files:

  • api_prices_test.dart: added tests for getLocation, getPriceProduct and getStatus
  • open_prices_api_client.dart: new methods getLocation, getPriceProduct and getStatus
  • openfoodfacts.dart: exported the 2 new files
  • price.dart: minor refactoring
  • price.g.dart: generated

New files:
* `location.dart`: Location object in the Prices API.
* `location.g.dart`: generated
* `price_product.dart`: Product object in the Prices API.
* `price_product.g.dart`: generated

Impacted files:
* `api_prices_test.dart`: added tests for `getLocation`, `getPriceProduct` and `getStatus`
* `open_prices_api_client.dart`: new methods `getLocation`, `getPriceProduct` and `getStatus`
* `openfoodfacts.dart`: exported the 2 new files
* `price.dart`: minor refactoring
* `price.g.dart`: generated
@codecov-commenter
Copy link

codecov-commenter commented Dec 15, 2023

Codecov Report

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

Comparison is base (e2d3f80) 73.00% compared to head (6719844) 72.97%.

Files Patch % Lines
lib/src/prices/location.g.dart 50.00% 13 Missing ⚠️
lib/src/prices/price_product.g.dart 47.36% 10 Missing ⚠️
lib/src/prices/price.g.dart 50.00% 4 Missing ⚠️
lib/src/prices/location.dart 60.00% 2 Missing ⚠️
lib/src/prices/price_product.dart 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #855      +/-   ##
==========================================
- Coverage   73.00%   72.97%   -0.03%     
==========================================
  Files         224      228       +4     
  Lines        7895     7990      +95     
==========================================
+ Hits         5764     5831      +67     
- Misses       2131     2159      +28     

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

@teolemon teolemon requested a review from g123k December 15, 2023 12:08
Copy link
Contributor

@g123k g123k left a comment

Choose a reason for hiding this comment

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

Some minor comments


/// Date when the product was bought.
@JsonKey(fromJson: JsonHelper.stringTimestampToDate)
late DateTime created;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you renamed latitude/longitude, I would rename it to createDate


/// Date when the product was bought.
@JsonKey(fromJson: JsonHelper.nullableStringTimestampToDate)
DateTime? updated;
Copy link
Contributor

@g123k g123k Dec 18, 2023

Choose a reason for hiding this comment

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

Since you renamed latitude/longitude, I would rename it to updatedDate

String? imageURL;

@JsonKey(fromJson: JsonHelper.stringTimestampToDate)
late DateTime created;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same suggestion as before

@@ -40,27 +40,27 @@ class Price extends JsonObject {
/// If the price is about a barcode-less product, it must be the price per
/// kilogram or per liter.
@JsonKey()
num? price;
late num price;
Copy link
Contributor

Choose a reason for hiding this comment

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

By curiosity, why don't you include JsonKeys in this file?

@raphodn
Copy link
Member

raphodn commented Dec 29, 2023

Thanks @monsieurtanuki !

Related to your PR, we have 2 upcoming PRs that add new endpoints :

@monsieurtanuki
Copy link
Contributor Author

For the record we need this PR to go a step further for Prices in Smoothie.

@M123-dev
Copy link
Member

M123-dev commented Feb 4, 2024

Some unanswered suggestions from Edouard, other than that I think we should be fine

@monsieurtanuki
Copy link
Contributor Author

Some unanswered suggestions from Edouard, other than that I think we should be fine

I have read Edouard's comments.
Basically they say we could have coded things slightly differently with no added value.
Fair enough.
I guess we can also code things slightly my way, then.

@teolemon
Copy link
Member

teolemon commented Feb 7, 2024

Tests are failing @monsieurtanuki (and we're in the Open Prices call right now ;-) https://meet.google.com/oin-hiqp-tmd )

@monsieurtanuki
Copy link
Contributor Author

Tests are failing @monsieurtanuki (and we're in the Open Prices call right now ;-) https://meet.google.com/oin-hiqp-tmd )

Robotoff tests are failing, indeed.
Screenshot_20240207_150731_Chrome

@monsieurtanuki
Copy link
Contributor Author

@teolemon btw here we don't the tests to pass to merge (especially unrelated platform-dependent tests).
And this morning I detected another unrelated server bug that prevents our tests to pass anyway:
openfoodfacts/openfoodfacts-server#9770

@monsieurtanuki
Copy link
Contributor Author

@teolemon For the record currently 4 tests are failing:

@monsieurtanuki
Copy link
Contributor Author

Folksonomy bug fixed by #881

@teolemon teolemon merged commit e18080b into openfoodfacts:master Feb 13, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants