Skip to content

Comments

feat: add additional parameters to PriceUser model#1065

Merged
monsieurtanuki merged 3 commits intoopenfoodfacts:masterfrom
chetanr25:master
Apr 22, 2025
Merged

feat: add additional parameters to PriceUser model#1065
monsieurtanuki merged 3 commits intoopenfoodfacts:masterfrom
chetanr25:master

Conversation

@chetanr25
Copy link
Contributor

What

Updated PriceUser model to include additional fields from the OpenFoodFacts Prices API endpoint /api/v1/users/$username. These fields will be used to enhance the dashboard and other features in the Smooth app.

Reference - API - /api/v1/users/$username

Fixes bug(s)

  • This isn't part of any issue, but enhances the current PriceUser model so that we can use it in smooth-app easily.

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 @chetanr25!

Your code seems to work. But:

  • it's not a good idea to work directly on the master branch
  • it's not a good sign when there are no issue to base your PR on. Generally speaking I'd suggest to create an issue first, where the point, the priority and the strategy can be discussed
  • bad luck collision - yesterday with @AshutoshKhadse23's #1059 we added flexibility to some stats (PriceTotalStats) in order to avoid frequent PRs, and your PR is strongly related

Could you please rewrite your contribution with #1059 in mind, including:

  • getting rid of price_user.g.dart
  • removing @JsonSerializable and JsonObject from PriceUser

Something that would include that kind of code:

  final Map<String, dynamic> json;

  PriceUser(this.json);

  factory PriceUser.fromJson(Map<String, dynamic> json) {
    return PriceUser(json);
  }

  Map<String, dynamic> toJson() => json;

  int? getInt(String key) => json[key] as int? : null;

  String? getString(String key) => json[key] as String? : null;

@chetanr25
Copy link
Contributor Author

Thanks for the feedback. My bad, I forgot to switch to a new branch and didn’t realise until after I committed. Will definitely keep this in mind next time.

I will make the requested changes and commit to the PR.

@chetanr25
Copy link
Contributor Author

I have made the requested changes, kindly take a look when you get a chance.

I’ve also added one-line documentation for a few variables. Please let me know if you’d prefer the same for the remaining ones.

Apologies for not linking this PR to an issue earlier. I will make sure to follow the proper process moving forward. I made this PR because it was blocking openfoodfacts/smooth-app#6559

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 @chetanr25, and congratulations for your first PR here!

For the record if the PR is related to an issue in another project, it's a good idea to add a link to that other issue, in your description.

@github-project-automation github-project-automation bot moved this from In progress to Reviewer approved in Dart package - Support for new APIs and issues Apr 22, 2025
@monsieurtanuki monsieurtanuki merged commit 14174ef into openfoodfacts:master Apr 22, 2025
2 of 3 checks passed
@chetanr25
Copy link
Contributor Author

Thank you!
Understood, I will make sure to link Issue to PR as you have said.

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