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.

Sign up for GitHub

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

Jump to bottom

Ratings intergration #1371

Merged
merged 8 commits into from
Sep 15, 2023
Merged

Ratings intergration #1371

merged 8 commits into from
Sep 15, 2023

Conversation

matthew-hagemann
Copy link
Collaborator

@matthew-hagemann matthew-hagemann commented Sep 13, 2023 •
edited
Loading

Creating as a draft until I finish updating the tests, but probably best to start getting eyes on this now

  • Created new provider ratingsModelProvider for getting rating of snap in the details_page.
  • Created new service ratingsService that wraps the ratings client that can be found here

@matthew-hagemann matthew-hagemann marked this pull request as ready for review September 14, 2023 14:11
d-loose
d-loose reviewed Sep 14, 2023
View reviewed changes
test/detail_page_test.dart Outdated Show resolved Hide resolved
d-loose
d-loose reviewed Sep 14, 2023
View reviewed changes
test/snap_card_test.dart Outdated Show resolved Hide resolved
d-loose
d-loose requested changes Sep 14, 2023
View reviewed changes
Copy link
Member

@d-loose d-loose left a comment

Choose a reason for hiding this comment

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

Could we also get that lib/ratings.dart we talked about and import '/ratings.dart' instead?

Hope the comments aren't too blunt - great work overall, thanks a lot!

@matthew-hagemann
feat: get ratings from new ratings service
9e8a50f
@matthew-hagemann
test: fix all failing tests
c2c67e3
@matthew-hagemann
test: ratings tests on details_page
5b276f1
@matthew-hagemann
refactor: removing ratingsListModelProvider
d0bdac5 
Removed the ratingsListModelProvider, opting to use ratingsModelProvider instead and nesting the calls deeper in the widget tree
@matthew-hagemann matthew-hagemann force-pushed the ratings-intergration branch 2 times, most recently from baeb57e to dd14cbc Compare September 15, 2023 05:33
@matthew-hagemann
Copy link
Collaborator Author

matthew-hagemann commented Sep 15, 2023 •
edited
Loading

Not quite sure what to make of CI failures:

  • Analyze wants me to swap to relative imports in lib/src, but I believe we want to avoid doing that? Is there something we want to update in analysis_options.yaml?
  • Mocks are freshly updated, not sure why that failed?

d-loose
d-loose approved these changes Sep 15, 2023
View reviewed changes
Copy link
Member

@d-loose d-loose left a comment

Choose a reason for hiding this comment

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

Cleaned up imports and regenerated mocks. Also removed the loading spinner from the card, we can check back later with Ana to see if it's necessary, but I think it doesn't look great :)

LGTM 👍

@d-loose d-loose merged commit c9ba0c0 into ubuntu:dev Sep 15, 2023
6 checks passed
tim-hm pushed a commit that referenced this pull request Sep 17, 2023
@matthew-hagemann @d-loose
feat: integrate ratings service (#1371)
56d8668 
* feat: get ratings from new ratings service
* test: fix all failing tests
* test: ratings tests on details_page
* refactor: removing ratingsListModelProvider

Removed the ratingsListModelProvider, opting to use ratingsModelProvider
instead and nesting the calls deeper in the widget tree.

* style: formatting and small code tweaks
* fix: relative imports
* feat: remove loading spinner from snap card
* chore: update mocks

---------

Co-authored-by: Dennis Loose <[email protected]>
@matthew-hagemann matthew-hagemann deleted the ratings-intergration branch September 19, 2023 12:36
tim-hm pushed a commit that referenced this pull request Sep 19, 2023
@matthew-hagemann @d-loose @tim-hm
feat: integrate ratings service (#1371)
c1a5f0b 
* feat: get ratings from new ratings service
* test: fix all failing tests
* test: ratings tests on details_page
* refactor: removing ratingsListModelProvider

Removed the ratingsListModelProvider, opting to use ratingsModelProvider
instead and nesting the calls deeper in the widget tree.

* style: formatting and small code tweaks
* fix: relative imports
* feat: remove loading spinner from snap card
* chore: update mocks

---------

Co-authored-by: Dennis Loose <[email protected]>
ashuntu pushed a commit to ashuntu/app-center that referenced this pull request Feb 28, 2024
@matthew-hagemann @d-loose
feat: integrate ratings service (ubuntu#1371)
220f568 
* feat: get ratings from new ratings service
* test: fix all failing tests
* test: ratings tests on details_page
* refactor: removing ratingsListModelProvider

Removed the ratingsListModelProvider, opting to use ratingsModelProvider
instead and nesting the calls deeper in the widget tree.

* style: formatting and small code tweaks
* fix: relative imports
* feat: remove loading spinner from snap card
* chore: update mocks

---------

Co-authored-by: Dennis Loose <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d-loose d-loose d-loose approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@matthew-hagemann @d-loose