-
-
Notifications
You must be signed in to change notification settings - Fork 81
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
Officer Table Overhaul #352
Conversation
DMalone87
commented
Mar 5, 2024
•
edited
Loading
edited
- Adding tests for Officer API endpoints
- Creating association objects for:
- Accusation (Officer/Perpetrator)
- Employment (Officer/Agency)
- Added new officer endpoints
- Get all officers
- Get officer by ID
- Create Officer
- Adding tests for Officer API endpoints - Creating association objects for: - Accusation (Officer/Perpetrator) - Employment (Officer/Agency)
backend/tests/test_officers.py
Outdated
actual_incident_names = list( | ||
filter(None, map(incident_name, actual_incidents)) | ||
) | ||
assert set(actual_incident_names) == set(expected_incident_names) |
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.
nit: you can get more details in test failures by adding these to a class extending unittest.TestCase, then using unittest assertions (self.assertEqual()
, for example)
- Get officer by id - Get all officers - Create officer
Officer.first_name.ilike(f"%{first_name}%"), | ||
Officer.last_name.ilike(f"%{last_name}%") | ||
)) | ||
if len(names) == 0: |
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.
This would be an empty string. We would never accept this case, would we? I think this case and next should have their values incremented. On the len(names) == 1
case, we would experience an out of bounds exception when indexing names[1]
.
accusations: Optional[List[CreateAccusationSchema]] | ||
state_ids: Optional[List[CreateStateIDSchema]] | ||
|
||
|
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.
Why do we have two identical schemas here except for optionality (CreateOfficerSchema and OfficerSchema)?
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.
They aren't entirely identical. The create version (schema_create
) just excludes the ids. There's probably a cleaner way of doing it, but I haven't looked into it. Much of this code was inherited from an earlier cohort who have mostly moved on to other projects or left CfB.