Conversation
|
@chouinar see below for the 3 curl commands to hit each of the endpoints. ListOpportunities: Get Opportunity: SearchOpportunities: |
widal001
left a comment
There was a problem hiding this comment.
This PR makes a ton of progress setting up the custom fields! So nice work. I left a quite a few comments but not all of them have to be addressed in this PR.
Things that should be addressed before merging:
- Aligning these custom fields with the ones in the custom field catalog. If there are issues we run into validation-wise, we can always update the catalog, but they should be in sync.
- Expanding the tests to handle the additional test cases I described -- this might require some updates to
transformation.pyto handle malformed custom fields data. In particular we want to be really certain how missing or malformed data will be handled so we're not returning 500 errors to API consumers if just one record has a bad value.
The other notes about the tests or the Pyright aren't urgent but can you create some tickets to account for that work?
| custom_fields["attachments"] = CustomField( | ||
| name="attachments", | ||
| fieldType=CustomFieldType.ARRAY, | ||
| value=attachment_values, | ||
| description="Attachments such as NOFOs and supplemental documents for the opportunity", | ||
| ) |
There was a problem hiding this comment.
There was a problem hiding this comment.
| return None | ||
|
|
||
|
|
||
| def populate_custom_fields(opp_data: dict) -> dict[str, CustomField] | None: |
There was a problem hiding this comment.
This doesn't need to be changed in this PR, but we should discuss the pros and cons of creating dedicated schemas for these custom fields, instead of just using the generic CustomField (similar to what we're doing for the Marshmallow schemas). Maybe this is a future KT topic with Bryan.
While the current pattern formats the data, we're not actually doing any validation of the data going into these custom fields at the transformation layer (via pydantic), which makes it harder to catch and handle malformed data before we try to serialize it with the Marshmallow models.
The plugin framework we're working on now will make it easier to define and auto-generate those pydantic schemas.
| self.legacy_opportunity_id = 67890 | ||
| self.opportunity_number = "2024-010" | ||
| self.category = "Mandatory" | ||
| self.agency_code = "A2345" | ||
| self.agency_name = "Testing Agency" | ||
| self.top_level_agency_name = "Testing top level agency" | ||
| self.opportunity_assistance_listings = [ |
There was a problem hiding this comment.
Instead of defining this over and over again in each mock test, can we bubble this up to a fixture or even just a separate utility function that returns default values that we can override as needed?
Also I notice that we're setting the same attributes in each test but the values are changing slightly, but there doesn't seem to be a reason for having them be different across tests.
Seeing the same values set over and over again makes it hard to track what's actually be tested.
| type( | ||
| "MockAttachment", | ||
| (), |
There was a problem hiding this comment.
Where is this "MockAttachment" defined and what purpose is it solving?
I only see the phrase MockAttachment in this file. Same with the other Mock<CustomField> examples
There was a problem hiding this comment.
It's mocking the opportunity_attachments custom fields property.
| def test_populate_custom_fields(self): | ||
| """Test that populate_custom_fields correctly maps all fields from opp_data.""" |
There was a problem hiding this comment.
Can you move this out into its own TestPopulateCustomFields class and test a few additional cases, each as a separate method:
- This base transformation test case, where everything is populated as expected.
- A test case where some or all of these custom fields are missing values -- we want to check that
Nonevalues are handled correctly. - A test case where some of these values have the incorrect type -- we want to check how we handle malformed data types
For test cases 2 and 3 we might need to also load the Marshmallow schemas you created to ensure that malformed data is caught before it gets to those values. Because otherwise we'll likely pass on some internal 500 errors to the API consumers -- which we don't want.
On a related we should have a similar set of rout-level test cases that tests how the new marshmallow models handle invalid data (e.g. does it raise a 500 error or a different error type like 422 or something else)



Summary
Fixes #8495 and fixes #8490
Changes proposed
This PR adds CustomFields to the
common grantsOpenAPI endpoints via the updates to the Marshmallow schema. It also supports populating the custom fields from the SGG datasource into the expected field. This is done in thepopulate_custom_fieldsfunction.Context for reviewers
Part of this PR is that this establishes the pattern for custom fields in the common grants endpoints going forward. There is a new file added
common_grants_custom_fields.pythat will serve as the container for all custom fields going forward.These custom fields can then be imported and registered to any base object that is returned in an endpoint by adding it to the CustomFields Marshmallow class. Below is an example of the class registration.
As well as registering the custom fields in the schema it also adds the population of the fields from the SGG base data. This occurs in
populate_custom_fields. There are also changes intransform_opportunity_to_cgwhich is called from the GET endpoint before the populate call is made. These changes are to pre-populate some of the dictionary fields to ensure the data is pulled out of the initial request so that it can be added to the eventual response.Validation steps
jeff-8495-Ad…s-to-openapicd /apimake openapi-spec-common-grantsto generate the new openAPI yaml filemake init make db-seed-local && make populate-search-opportunities make run-logsto stand up the API endpoints.SIMPLER-GRANTS-PROTOCOLrepo and navigate tolib/python-sdk/examples6.portsin the 3 custom fields examples to be 8080lib/python-sdkRun `poetry run python examples/list_opportunities.py and validate the custom fieldspoetry run python examples/get_opportunity.pyrepeat validationpoetry run python examples/search_opportunities.pyrepeat validationUpdated list_opportunities.py
Updated get_opportunity.py
Updated search_opportunities.py