-
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 #2793] Create Extract Metadata API #2963
[Issue #2793] Create Extract Metadata API #2963
Conversation
…ps://github.com/HHS/simpler-grants-gov into mikehgrantsgov/2793-create-extract-metadata-api
def test_extract_metadata_get_default_dates( | ||
client, api_auth_token, enable_factory_create, db_session | ||
): | ||
"""Test that default date range (last 7 days) is applied when no dates provided""" | ||
ExtractMetadataFactory.create_batch( | ||
2, | ||
extract_type=ExtractType.OPPORTUNITIES_JSON, | ||
) |
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.
Could you add a record that is older/newer than the last 7 days?
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.
Could you add at least one test that verifies the file can be downloaded / the pre-signing works.
Can probably just steal some of what https://github.com/HHS/simpler-grants-gov/blob/main/api/tests/src/api/opportunities_v1/test_opportunity_route_get.py#L111 does which is testing the presigned logic for GET opportunities.
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.
Sure, I just added this. Used some adaptation from that example you sent, thanks.
Co-authored-by: Michael Chouinard <[email protected]>
Co-authored-by: Michael Chouinard <[email protected]>
Co-authored-by: Michael Chouinard <[email protected]>
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.
LGTM!
Summary
Fixes #2793
Time to review: 30 mins
Changes proposed
Add a new Blueprint / API for the extract metadata API.
Add tests for all business rules and data retrieval
Context for reviewers
Population of the table will be addressed in a follow-on ticket.
Additional information
Open questions:
Should this be called V1 to follow the opportunity API convention?
extract_type
is a filter in the POST param vs an explicit path param in the URL. Is that OK?See unit tests for additional context.