[DEV-14570] Implement the spending_by_award filters in Pydantic#4634
[DEV-14570] Implement the spending_by_award filters in Pydantic#4634
Conversation
anjenkin
left a comment
anjenkin
left a comment
There was a problem hiding this comment.
I double checked and the FE isn't using 'program_activity' on the spending_by_award endpoint.
aguest-kc
added
in progress
zachflanders-frb
left a comment
zachflanders-frb
left a comment
There was a problem hiding this comment.
Approving with some comments to consider.
There was a problem hiding this comment.
I wonder if it makes sense to consolidate these into a single CodeObject model?
There was a problem hiding this comment.
I was originally going to combine them, but I kept them separate in case we wanted to do any custom validation for a specific code type in the future.
| @classmethod | ||
| def validate_zip(cls, v: str) -> str | None: | ||
| if v is not None and len(v) != 5: | ||
| raise ValueError("ZIP code must be exactly 5 digits") |
There was a problem hiding this comment.
Do we want to check if zip codes are digits as well as length? Could use and not v.isdigit()
| valid_def_codes = list(DisasterEmergencyFundCode.objects.values_list('code', flat=True)) | ||
|
|
||
| if isinstance(code, str): | ||
| code = code.upper() | ||
| if code not in valid_def_codes: |
There was a problem hiding this comment.
Instead of getting all the def codes and serializing them to a list it might be faster to do
DisasterEmergencyFundCode.filter(code=code).exists() and DisasterEmergencyFundCode.filter(code__in=code).exists()
There was a problem hiding this comment.
That would be more efficient. I did it this way to support the existing error message, which lists all of the possible DEF codes.
| naics_codes: NAICSCodeObject | None = None | ||
| place_of_performance_locations: list[StandardLocationObject] | None = None | ||
| place_of_performance_scope: Literal["domestic", "foreign"] | None = None | ||
| program_activities: list[ProgramActivityObject] | None = None | ||
| program_numbers: list[str] | None = None | ||
| psc_codes: PSCCodeObject | list[str] | None = None | ||
| recipient_locations: list[StandardLocationObject] | None = None | ||
| recipient_scope: Literal["domestic", "foreign"] | None = None | ||
| recipient_search_text: list[str] | None = None | ||
| recipient_type_names: list[str] | None = None | ||
| set_aside_type_codes: list[str] | None = None | ||
| tas_codes: TASCodeObject | None = None |
There was a problem hiding this comment.
If we consolidated the CodeObject models (see earlier) we could use that model for the naics codes, psc codes, and tas codes.
| filters: Filters | ||
| limit: int = 10 | ||
| order: Literal["asc", "desc"] = "desc" | ||
| page: int | None = None |
There was a problem hiding this comment.
Does page have to be >= 1? Might be worth a field validation. I feel like I've made the mistake of asking for page 0 before.
aguest-kc
dismissed stale reviews from zachflanders-frb and anjenkin
via
967917b
4 participants
Description:
Use Pydantic to validate the
spending_by_awardfilters instead of TinyShield.Technical Details:
Requirements for PR Merge:
Explain N/A in above checklist: