-
Notifications
You must be signed in to change notification settings - Fork 14
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
[Issue #3295] Create GET /users/:userId/saved-opportunities API schema and stub endpoint #3355
base: main
Are you sure you want to change the base?
[Issue #3295] Create GET /users/:userId/saved-opportunities API schema and stub endpoint #3355
Conversation
api/src/api/users/user_routes.py
Outdated
} | ||
) | ||
|
||
return {"message": "Success", "data": opportunities_data} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be returning a dict, return an ApiResponse object
api/src/api/users/user_routes.py
Outdated
opportunities_data = [] | ||
for saved in saved_opportunities: | ||
opp = saved.opportunity | ||
summary = opp.summary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This loop shouldn't be necessary, it should be fine to return the ORM objects and Marshmallow will handle iterating over/making them into JSON for you. If you're hitting an error about objects being detached from a session, then that's because SQLAlchemy lazy-loads all relationships (this loop effectively emits several SQL queries). Easiest way around that is to load in the query itself. It's why in an endpoint like GET opportunity
we do .options(selectinload("*"))
: https://github.com/HHS/simpler-grants-gov/blob/main/api/src/services/opportunities_v1/get_opportunity.py#L21
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep that makes sense. I thought we had to do more work here than needed. I did update the utility to return pure Opportunity
objects so it will fit in the shape of the response.
Co-authored-by: Michael Chouinard <[email protected]>
Co-authored-by: Michael Chouinard <[email protected]>
…s://github.com/HHS/simpler-grants-gov into mikehgrantsgov/3295-create-get-user-saved-opps
Co-authored-by: Michael Chouinard <[email protected]>
…s://github.com/HHS/simpler-grants-gov into mikehgrantsgov/3295-create-get-user-saved-opps
Summary
Fixes #3295
Time to review: 25 mins
Changes proposed
Create GET /users/:userId/saved-opportunities API schema
Context for reviewers
Overlaps with the implementation ticket, as it was easier to add the service and implementation vs. stub + implement in 2 different PRs
Additional information
See attached unit tests.