-
Notifications
You must be signed in to change notification settings - Fork 13
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 #1364] Update API schema with modified DB schema fields #1448
Conversation
…4-update-api-schema
…4-update-api-schema
…4-update-api-schema
) | ||
factories.CurrentOpportunitySummaryFactory.create( | ||
opportunity=all_null_opportunity, opportunity_summary=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.
👍
"description": "Details regarding what modification was last made", | ||
"example": None, | ||
} | ||
) |
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.
Should these be removed if they're not added back anywhere?
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.
I removed them from the model to avoid any confusion as the summary object has similar values. While I don't know what we want to do with these long-term, the current system returns the summary values, so I removed these in the top-level opportunity for now.
@@ -27,6 +27,8 @@ def search_opportunities( | |||
# TODO - when we want to sort by non-opportunity table fields we'll need to change this | |||
.order_by(sort_fn(getattr(Opportunity, search_params.pagination.order_by))) | |||
.where(Opportunity.is_draft.is_(False)) # Only ever return non-drafts | |||
# Filter anything without a current opportunity summary | |||
.where(Opportunity.current_opportunity_summary != None) # noqa: E711 |
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 makes it so we only pull opportunity records that have a summary when we make the request?
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.
Yes. Opportunities have to have a current opportunity summary (and thus an opportunity status which is that table) in order to be searchable. This is deliberate to avoid showing opportunities before their summaries, which are really the public notices of funding, are actually live/public.
Note this doesn't mean the opportunity is entirely hidden, if you navigate to the page of an opportunity directly, you would be able to find it.
Summary
Fixes #1364
Time to review: 10 mins
Changes proposed
Updated the API schema to match the updated database schema
Fixed various tests, updated a few factory utilities
Context for reviewers
Nothing too elaborate here, did add a filter to only include records if the current_opportunity_summary is not null (eg. it has a current summary).
Additional information
For testing locally, you can seed your DB with
make db-seed-local
.Here is what a response looks like (page size = 3)