Add API endpoints for departments #1138
Conversation
michplunkett
requested review from
AetherUnbound,
abandoned-prototype and
sea-kelp
|
Things that might be needed, are documentation for these endpoints. I'm not sure what a good protocol is for that rn, though I'm welcome to suggestions. |
| resp_assignments = client.get( | ||
| url_for("v1.get_dept_assignments", department_id=department_id), | ||
| follow_redirects=True, | ||
| ) |
There was a problem hiding this comment.
I could probably get rid of follow_redirects, but I think it might be worth keeping in case we modify the endpoint at some point.
OpenOversight/app/models/database.py
Outdated
| "Location", backref=db.backref("incidents", cascade_backrefs=False) | ||
| "Location", | ||
| backref=db.backref("incidents", cascade_backrefs=False), | ||
| lazy="joined", |
There was a problem hiding this comment.
Addresses this periodic error: sqlalchemy.orm.exc.DetachedInstanceError: Parent instance <Incident at 0xffff731ba510> is not bound to a Session; lazy load operation of attribute 'address' cannot proceed (Background on this error at: h...
| officer = db.relationship("Officer", back_populates="descriptions") | ||
| id = db.Column(db.Integer, primary_key=True) | ||
| text_contents = db.Column(db.Text()) | ||
| officer_id = db.Column(db.Integer, db.ForeignKey("officers.id", ondelete="CASCADE")) | ||
| officer = db.relationship("Officer", back_populates="descriptions") |
There was a problem hiding this comment.
Ordering for ease of reading.
There was a problem hiding this comment.
Thank you, this is great work. I like the sharing of methods between csv download and API, that's great.
However, I do think that json output does not have the same limitations that csv has and we should make use of that. What I have in mind here is a response to /departments/<id>/officers that looks something like this:
[
{
"birth_year": 1967,
"department_id": 3,
"employment_date": "1993-04-12",
"first_name": "MICHAEL",
"gender": "M",
"id": 32892,
"last_name": "TYMOCH",
"middle_initial": "K",
"race": "WHITE",
"suffix": null,
"unique_internal_identifier": null,
"assignments": [
{
"id": 15001,
"job_id": 20,
"officer_id": 32892,
"resign_date": null,
"star_no": "",
"start_date": null,
"unit_id": null
},
{
"id": 88142,
"job_id": 20,
"officer_id": 32892,
"resign_date": null,
"star_no": "P3662",
"start_date": null,
"unit_id": 15
},
{
"id": 88143,
"job_id": 20,
"officer_id": 32892,
"resign_date": null,
"star_no": "P2107",
"start_date": null,
"unit_id": 14
}
],
"salaries": [
{
"id": 15001,
"is_fiscal_year": true,
"officer_id": 32892,
"overtime_pay": null,
"salary": 63342,
"year": 2020
},
{
"id": 15002,
"is_fiscal_year": true,
"officer_id": 32892,
"overtime_pay": null,
"salary": 60042,
"year": 2020
},
{
"id": 15003,
"is_fiscal_year": true,
"officer_id": 32892,
"overtime_pay": null,
"salary": 50141,
"year": 2020
}
]
},
{
"birth_year": 1995,
"department_id": 3,
"employment_date": "2018-06-18",
"first_name": "BRITTANY",
"gender": "F",
"id": 32763,
"last_name": "SANDS",
"middle_initial": "E",
"race": "WHITE",
"suffix": null,
"unique_internal_identifier": null,
"assignments": [],
"salaries": []
},
{
"birth_year": 1958,
"department_id": 3,
"employment_date": "2014-08-25",
"first_name": "ANDREW",
"gender": "M",
"id": 32607,
"last_name": "NEWMARK",
"middle_initial": "D",
"race": "WHITE",
"suffix": null,
"unique_internal_identifier": null,
"assignments": [],
"salaries": []
}
]
Basically all department data combined and reducing the number of endpoints to /officers and /incidents. I think that makes working with the data much easier, as otherwise one would have to call multiple different endpoints and then merge all the datasets.
It might make sense to omit null value entries, not sure.
I am also not sure how computationally expensive creating this output would be, so it might be useful (or necessary) to also include pagination to the endpoint output to alleviate that issue.
Similarly eventually we also want to support filters, either of these points could be a follow up.
In terms of documentation, I suggested swagger in the original issue. I haven't used it in a while so there might be better solutions out there by now
|
I think the Swagger point could be addressed now, but I'd like to address the nesting part in another PR. It's already quite large and the nesting part is a non-trivial addition, imo. |
| @@ -620,6 +591,211 @@ class Image(BaseModel, TrackUpdates): | |||
| ) | |||
|
|
|||
|
|
|||
| class Incident(BaseModel, TrackUpdates): | |||
There was a problem hiding this comment.
The classes were reordered so that I could accurately label return values for the Department class.
| __tablename__ = "users" | ||
|
|
There was a problem hiding this comment.
This spacing is consistent with every other BaseModel class.
|
Tbh, I think it'd be worth adding the Swagger documentation in another PR and then we can deploy this once that's done. I'd rather make things more succinct than otherwise. |
michplunkett
changed the title
michplunkett
changed the title
Fixes issue
#385
Description of Changes
Add the baseline features for a data-focused API.
Tests and Linting
developbranch.pytestpasses on my local development environment.pre-commitpasses on my local development environment.