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.

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

Merged
merged 27 commits into from
Nov 22, 2024

Conversation

mikehgrantsgov
Copy link
Collaborator

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.

api/src/api/extracts_v1/extract_routes.py Outdated Show resolved Hide resolved
api/src/api/extracts_v1/extract_routes.py Outdated Show resolved Hide resolved
api/src/api/extracts_v1/extract_routes.py Outdated Show resolved Hide resolved
api/tests/src/api/extracts_v1/test_extracts_routes.py Outdated Show resolved Hide resolved
api/src/services/extracts_v1/get_extracts.py Outdated Show resolved Hide resolved
@mikehgrantsgov mikehgrantsgov marked this pull request as ready for review November 21, 2024 18:43
api/src/api/extracts_v1/extract_routes.py Outdated Show resolved Hide resolved
api/src/services/extracts_v1/get_extracts.py Outdated Show resolved Hide resolved
api/tests/src/api/extracts_v1/test_extract_schema.py Outdated Show resolved Hide resolved
Comment on lines 18 to 25
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,
)
Copy link
Collaborator

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?

Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator

@chouinar chouinar left a comment

Choose a reason for hiding this comment

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

LGTM!

@mikehgrantsgov mikehgrantsgov merged commit 5d4754f into main Nov 22, 2024
2 checks passed
@mikehgrantsgov mikehgrantsgov deleted the mikehgrantsgov/2793-create-extract-metadata-api branch November 22, 2024 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create the ExtractMetadata API
4 participants