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: added "app_name" parameter for prices methods uploadProof and createPrice #938

Merged

Conversation

monsieurtanuki
Copy link
Contributor

What

  • Now we pass an app_name parameter to prices methods uploadProof and createPrice, for tracking purposes.
  • Meanwhile, some refactoring, including trying to make all tests pass.
  • And some new prices methods somehow needed by Smoothie.

Impacted files

  • api_get_taxonomy_origins_server_test.dart: minor refactoring
  • api_get_user_products_test.dart: set max page size to new server max page size (100)
  • api_search_products_test.dart: set default page size to new server default page size (50)
  • api_suggestion_manager_test.dart: now targeting PROD; minor refactoring
  • currency.dart: new fromName method
  • open_prices_api_client.dart: added app_name parameter for uploadProof and createPrice; refactored around new method getUri
  • page_size.dart: added comments
  • price.dart: added comments
  • proof.dart: new getFileUrl method; added comments
  • proof_type.dart: new fromOffTag method

…eatePrice

Impacted files:
* `api_get_taxonomy_origins_server_test.dart`: minor refactoring
* `api_get_user_products_test.dart`: set max page size to new server max page size (100)
* `api_search_products_test.dart`: set default page size to new server default page size (50)
* `api_suggestion_manager_test.dart`: now targeting PROD; minor refactoring
* `currency.dart`: new `fromName` method
* `open_prices_api_client.dart`: added `app_name` parameter for `uploadProof` and `createPrice`; refactored around new method `getUri`
* `page_size.dart`: added comments
* `price.dart`: added comments
* `proof.dart`: new `getFileUrl` method; added comments
* `proof_type.dart`: new `fromOffTag` method
@monsieurtanuki
Copy link
Contributor Author

cc @raphodn @raphael0202

@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2024

Codecov Report

Attention: Patch coverage is 83.72093% with 7 lines in your changes missing coverage. Please review.

Project coverage is 75.20%. Comparing base (820d145) to head (641d195).
Report is 34 commits behind head on master.

Files Patch % Lines
lib/src/prices/currency.dart 0.00% 3 Missing ⚠️
lib/src/prices/proof_type.dart 0.00% 2 Missing ⚠️
test/api_search_products_test.dart 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #938      +/-   ##
==========================================
- Coverage   76.34%   75.20%   -1.15%     
==========================================
  Files         239      247       +8     
  Lines        8494     8881     +387     
==========================================
+ Hits         6485     6679     +194     
- Misses       2009     2202     +193     

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

@raphodn
Copy link
Member

raphodn commented Jun 6, 2024

Fyi in the prices web app I opted to pass the app_name in the URL of every request (so not only POST, but also GET).

I guess in Sentry if there are any errors, the User Agent is sent and available in any case. But i realised that some browsers do not allow to edit the UA (for instance Chrome).

@monsieurtanuki
Copy link
Contributor Author

Fyi in the prices web app I opted to pass the app_name in the URL of every request (so not only POST, but also GET).

DELETE and PATCH too?

@raphodn
Copy link
Member

raphodn commented Jun 6, 2024

Yes every call to the API 👌

And you can send extra info like the app_version and any other relevant data as extra url params if you want.

I need to document at least the app_name in the API documentation

Impacted files:
* `api_prices_test.dart`: test for new method `getFileUrl`
* `open_prices_api_client.dart`: similar URI user agent management as off-dart
* `proof.dart`: minor fix
@monsieurtanuki
Copy link
Contributor Author

@raphodn I've just pushed a change where all prices URLs will include "user agent" fields (when populated):

  • app_name
  • app_version (optional)
  • app_uuid (optional)
  • app_platform (optional)
  • comment (optional)

With the exception of the proof file URL where we don't add those parameters, e.g. https://prices.openfoodfacts.net/img/0002/yRw3zcph08.jpg

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.

Looks good, thanks @monsieurtanuki

lib/src/prices/price.dart Outdated Show resolved Hide resolved
lib/src/prices/proof.dart Outdated Show resolved Hide resolved
@monsieurtanuki monsieurtanuki merged commit e4bd1ed into openfoodfacts:master Jun 7, 2024
3 checks passed
@monsieurtanuki
Copy link
Contributor Author

Thank you @M123-dev for your reviews!

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

Successfully merging this pull request may close these issues.

5 participants