[Issue #3057] Create Agencies API #3065
Conversation
…//github.com/HHS/simpler-grants-gov into mikehgrantsgov/3057-add-agencies-listing-api
…//github.com/HHS/simpler-grants-gov into mikehgrantsgov/3057-add-agencies-listing-api
| has_system_to_system_certificate = fields.Boolean() | ||
| can_view_packages_in_grace_period = fields.Boolean() | ||
| is_image_workspace_enabled = fields.Boolean() | ||
| is_validation_workspace_enabled = fields.Boolean() | ||
|
|
||
| # Optional fields | ||
| ldap_group = fields.String(allow_none=True) |
There was a problem hiding this comment.
These seem more internal/private
There was a problem hiding this comment.
Agreed. @chouinar do we have conventions to have the requestor specify fields it needs? If not I can restrict the response for now to fields only needed by the FE.
There was a problem hiding this comment.
On the agency table there are several fields I marked as "unclear usage" that you can exclude. It is fine if the Marshmallow model has less fields than the DB, they'll just get discarded.
We can always add stuff later.
There was a problem hiding this comment.
I missed the other question you originally had, there isn't any way to change the fields in the response based on what the requestor asks for, at least not right now. Schemas are strictly defined, at best we could null out a bunch of fields for that
| agency_name = fields.String() | ||
| agency_code = fields.String() |
There was a problem hiding this comment.
I think these two are the only real pieces the existing FE use case needs. So not sure if we want to hyper fit that in some sort of AgencyList endpoint. Otherwise I think it's fine to return wider objects.
| has_system_to_system_certificate = fields.Boolean() | ||
| can_view_packages_in_grace_period = fields.Boolean() | ||
| is_image_workspace_enabled = fields.Boolean() | ||
| is_validation_workspace_enabled = fields.Boolean() | ||
|
|
||
| # Optional fields | ||
| ldap_group = fields.String(allow_none=True) |
There was a problem hiding this comment.
On the agency table there are several fields I marked as "unclear usage" that you can exclude. It is fine if the Marshmallow model has less fields than the DB, they'll just get discarded.
We can always add stuff later.
| label = fields.String(allow_none=True) | ||
|
|
||
| # Related agency info | ||
| top_level_agency_id = fields.Integer(allow_none=True) |
There was a problem hiding this comment.
We can exclude the ID, although we will need this API to return the top level agency info for the frontend to build a hierarchy.
There was a problem hiding this comment.
I removed this but sounds like it should stay, though?
There was a problem hiding this comment.
We don't need the ID since that is purely internal, but the frontend would want the top level agency itself - which might be a ticket itself as that requires nesting the schema in itself.
You should be able to do that https://marshmallow.readthedocs.io/en/stable/nesting.html#nesting-a-schema-within-itself
Something like:
class AgencySchema(Schema):
...
top_level_agency = fields.Nested(lambda: AgencySchema(exclude=("top_level_agency",)))
That'll nest it within itself, but only go one layer deep since you will have excluded nesting in the layer down.
There was a problem hiding this comment.
Thanks, I spent some time playing with this and added a test to cover this scenario.
…//github.com/HHS/simpler-grants-gov into mikehgrantsgov/3057-add-agencies-listing-api
mikehgrantsgov
deleted the
mikehgrantsgov/3057-add-agencies-listing-api
branch
Summary
Fixes #3057
Time to review: 30 mins
Changes proposed
Add agencies API supporting pagination and basic filtering (future requirements around shape TBD)
Context for reviewers
Open question: What should the schema return? Right now we are returning all Agency model fields. Maybe we should make this payload smaller depending on the frontend needs.
Additional information
See attached unit test