feat: Added Dashboard to price section #6559
Conversation
github-project-automation
bot
moved this to 💬 To discuss and validate
in 🤳🥫 The Open Food Facts mobile app (Android & iOS)
|
I had made changes to the Flutter package in my local system to test if this works. I have added the kind parameter in my PR to extend Should I comment it out or remove it for now? |
|
Once the changes from openfoodfacts/openfoodfacts-dart#1065 are available in our Flutter package, we can delete this file packages/smooth_app/lib/data_models/users_profile_data.dart and use the PriceUser model instead. Untill then should the PR be a Draft PR or is it okay to keep it open? |
|
@chetanr25 can you rename "My Consumption" to "Receipts & GDPR requests", and "Other contributions" to "Price labels" ? |
|
error • The setter 'kind' isn't defined for the type 'GetPricesParameters' • packages/smooth_app/lib/pages/prices/prices_dashboard_widget.dart:73:11 • undefined_setter |
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
Files not reviewed (5)
- packages/smooth_app/lib/data_models/users_profile_data.dart: Language not supported
- packages/smooth_app/lib/pages/preferences/user_preferences_prices.dart: Language not supported
- packages/smooth_app/lib/pages/prices/prices_dashboard.dart: Language not supported
- packages/smooth_app/lib/pages/prices/prices_dashboard_widget.dart: Language not supported
- packages/smooth_app/lib/pages/prices/prices_user_profile.dart: Language not supported
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a user dashboard to the price section, displaying user statistics and recent contributions. The dashboard is inspired by the Open Food Facts prices dashboard and includes a toggle for filtering contributions by type (consumption/community). Overall, the implementation seems well-structured and addresses the feature request. However, there are a few areas that could be improved for better maintainability and clarity.
Summary of Findings
- TODO comments: There are a couple of TODO comments that indicate areas where the code is not yet fully implemented or needs further attention. It's important to address these before merging to ensure the feature is complete and functional.
- Error Handling: The error handling in
_fetchUserProfilefunction could be improved to provide more informative error messages to the user. - Code Duplication: The logic for determining contribution type ('CONSUMPTION' or 'COMMUNITY') is duplicated in
PricesDashboardWidget. Consider centralizing this logic to avoid redundancy.
Merge Readiness
The pull request introduces a valuable feature, but there are a few issues that should be addressed before merging. Specifically, the TODO comments should be resolved, error handling improved, and code duplication reduced. Addressing these points will improve the overall quality and maintainability of the code. I am unable to approve this pull request, and recommend that others review and approve this code before merging.
| ..kind = selectedIndex == 0 | ||
| ? 'CONSUMPTION' | ||
| : 'COMMUNITY', // TODO(chetanr25): This must be implemented in OpenFoodFacts flutter package |
There was a problem hiding this comment.
The logic for determining the kind parameter based on selectedIndex is duplicated in the _getUserPrices function and the customToggleButton widget. Consider creating a separate function or constant to encapsulate this logic and avoid redundancy.
| @@ -0,0 +1,81 @@ | |||
| // TODO(chetanr25): To be implemented in OpenFoodFacts flutter package in [https://github.com/openfoodfacts/smooth-app/tree/develop/packages/smooth_app/lib/data_models] as [UserProfile] JsonSerializable | |||
There was a problem hiding this comment.
This TODO indicates that this model should eventually be moved to the openfoodfacts-dart package. It would be good to create an issue in that repo to track this work and link it here.
| ); | ||
| } | ||
|
|
||
| // TODO(chetanr25): To be implemented in OpenFoodFacts flutter package |
There was a problem hiding this comment.
This TODO indicates that this function should eventually be moved to the openfoodfacts-dart package. It would be good to create an issue in that repo to track this work and link it here.
| // | ||
| } |
There was a problem hiding this comment.
The catch block is currently empty. At a minimum, log the error for debugging purposes. Ideally, display a user-friendly error message.
} catch (e) {
debugPrint('Error fetching user profile: $e'); // Log the error
// Optionally, display an error message to the user
}
| ..kind = selectedIndex == 0 | ||
| ? 'CONSUMPTION' | ||
| : 'COMMUNITY', // TODO(chetanr25): This must be implemented in OpenFoodFacts flutter package |
There was a problem hiding this comment.
This TODO indicates that the kind parameter implementation is still pending in the openfoodfacts-dart package. It's important to track the progress of that PR and update this code once it's merged.
| ..kind = selectedIndex == 0 | ||
| ? 'CONSUMPTION' | ||
| : 'COMMUNITY', // TODO(chetanr25): This must be implemented in OpenFoodFacts flutter package |
There was a problem hiding this comment.
Consider using an enum instead of hardcoded strings ('CONSUMPTION', 'COMMUNITY') for the kind parameter. This would improve type safety and readability.
..kind = selectedIndex == 0
? PriceKind.consumption
: PriceKind.community,
…tions and other contributions
This has been fixed. Since openfoodfacts/openfoodfacts-dart#1064 is merged and is available in openfoodfacts flutter package, we can now use the ContributionKind enum.
I have made the requested changes and committed them to the current PR. Initially, I had replicated the same titles (‘My Consumption’ and ‘Other Contributions’) from the dashboard web version. Since we have updated the titles, is there anything else I should update in the PR to stay consistent? |
@chetanr25 No it hasn't been fixed, as we haven't released a new version of openfoodfacts-dart yet. |
|
I had tested well and only then committing to this PR. The changes were made for testing purposes, to ensure that everything worked correctly on smooth-app. As a result, it appeared to work without any issues on my local system. However, I realize now that I forgot the fact that the actual package update had not yet been officially released. I mistakenly assumed that it had already been published and updated the PR. I completely understand that directly modifying installed packages is not a good practice. I had only done it temporarily for testing. After reinstalling the package, I realized the release hadn’t happened yet. Apologies for the confusion. |
Yeah, it's not obvious :/ By default :
Receipts can be added to "Community" if user opt-outs, using boolean |
Thank you @TTalex for your explanations! |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a Dashboard feature for the prices section by adding new UI components and a UserProfile model. Key changes include:
- A new PricesUserProfile widget to display user statistics.
- A PricesDashboardWidget that fetches and displays price/proof data with a toggle for contribution types.
- A PricesDashboard page and an updated user preferences list that link to the new dashboard.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/smooth_app/lib/pages/prices/prices_user_profile.dart | New widget to display user profile statistics. |
| packages/smooth_app/lib/pages/prices/prices_dashboard_widget.dart | Dashboard widget with contribution type toggling and price/proof fetching. |
| packages/smooth_app/lib/pages/prices/prices_dashboard.dart | Scaffold page assembling the dashboard and profile widgets. |
| packages/smooth_app/lib/pages/preferences/user_preferences_prices.dart | Added ListTile for navigating to the new dashboard. |
| packages/smooth_app/lib/data_models/users_profile_data.dart | Introduced UserProfile model for data deserialization. |
| spacing: MEDIUM_SPACE, | ||
| children: <Widget>[ | ||
| categorySwitch(), | ||
| const SizedBox(height: SMALL_SPACE), | ||
| priceProofButton(widget.userProfile!, appLocalizations), |
There was a problem hiding this comment.
The Column widget does not have a 'spacing' property. Replace it with SizedBox widgets or use a widget that supports spacing.
| spacing: MEDIUM_SPACE, | |
| children: <Widget>[ | |
| categorySwitch(), | |
| const SizedBox(height: SMALL_SPACE), | |
| priceProofButton(widget.userProfile!, appLocalizations), | |
| children: <Widget>[ | |
| categorySwitch(), | |
| const SizedBox(height: MEDIUM_SPACE), | |
| priceProofButton(widget.userProfile!, appLocalizations), | |
| const SizedBox(height: MEDIUM_SPACE), |
| const SizedBox(height: SMALL_SPACE), | ||
| priceProofButton(widget.userProfile!, appLocalizations), | ||
| FutureBuilder<MaybeError<GetPricesResult?>>( | ||
| future: _getUserPrices(), |
There was a problem hiding this comment.
The FutureBuilder is instantiating a new future every build call rather than using the stored 'pricesFuture'. Replace it with 'pricesFuture' to avoid unnecessary re-fetching.
| future: _getUserPrices(), | |
| future: pricesFuture, |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6559 +/- ##
==========================================
- Coverage 9.54% 5.66% -3.89%
==========================================
Files 325 515 +190
Lines 16411 30807 +14396
==========================================
+ Hits 1567 1745 +178
- Misses 14844 29062 +14218 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-actions
bot
added
the
📚 Documentation
…mporvary UserProfile model with PriceUser model which is created in openfoodfacts flutter package
…r in dart package
|
error • Target of URI doesn't exist: 'package:smooth_app/data_models/users_profile_data.dart' • packages/smooth_app/lib/pages/prices/prices_dashboard.dart:4:8 • uri_does_not_exist |
|
I was actually waiting for openfoodfacts/openfoodfacts-dart#1085 to be released in the openfoodfacts Flutter package so I could use it here. I'll remove this once the package is being published. |
|
@chetanr25 I've just released openfoodfacts 3.23.0. |
… to TitleElementType.PERCENTAGE in knowledge_pannel_builder.dart
|
I have updated the package version in For now, i have added a TODO for I haven't mentioned username in TODO |
@chetanr25 Oh you have to. I suggest @PrimaelQuemerais. |
|
In the previous commit 7075c4d , I resolved the scrolling functionality issue by refactoring the implementation. Previously, I was directly using After analyzing the codebase, I took inspiration from This solution resolves the scrolling issues, I'd particularly appreciate review on:
{Edited because the videos weren't visible] |
|
@chetanr25 I think you'd be better off using
This PR code could be very simple, with a very simple UI/UX. |
I tried to use
Just to clarify, do you suggest that we go back to the earlier approach (in before video)? I implemented this (similar to product_prices_list.dart but without InfiniteScrollList) after keeping maintainability and re-usability in mind. I am actually confused, how to go about this. |
@chetanr25 That's the point. Either you try to stick to the web UI, or you try to display and code things simply. |
5 participants
What
Created a user dashboard taking inspiration from https://prices.openfoodfacts.org/dashboard
Changes
Added Dashboard ListTile option under prices section packages/smooth_app/lib/pages/preferences/user_preferences_prices.dart
Created a UserProfile model which stores information returned by /api/v1/users/$username which is used in first section of dashboard. packages/smooth_app/lib/data_models/users_profile_data.dart
First section of the dashboard includes price_count, product_count, proof_count, location_count etc. + packages/smooth_app/lib/pages/prices/prices_user_profile.dart
Main dashboard is built in + packages/smooth_app/lib/pages/prices/prices_dashboard.dart
Prices and Proof button in dashboard currently onClicking will navigate to existing "My prices" and "My proofs" screen. (note: The list of prices or proofs isn't effected by the category selected "My Consumption" / "Other Contributions")
The toggle button "My Consumption" and "Other Contributions" when toggled will update price and proof contributions count based on the kind of contribution.
The recent contributions summary which is the last section of dashboard. We will display recent contributions of the user filtered by the kind of contribution ("Community" or "Consumption") but for this we need to add
kindparameter in GetPricesParameters() of OpenFoodFacts package in lib/src/prices/get_prices_parameters.dart. I have made a PR for this in openfoodfacts-dart repo feat: Added kind parameter to getPricesParameters and getProofsParameters openfoodfacts-dart#1064Screenshot
https://github.com/user-attachments/assets/6818d97b-3998-4f01-8b9e-bf024c96247f
Fixes bug(s)
Part of
I’ve tried to stick to the existing codebase conventions for this. If I’ve missed something or anything needs to be changed, please let me know, I will make the updates.