-
Notifications
You must be signed in to change notification settings - Fork 11
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
Show department last updated dates #214
Conversation
What steps do you think we'll need to take in order to migrate this in production? |
When I try the following steps locally, I get an error 😰 I'm not sure what is going on, but I see a migration which drops the
|
In your local dev environment, you'll need to run |
Got it! I was able to run |
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.
So I've been thinking about this a bit more - I wonder if we might be able to show 3 dates: officer updated, assignments updated, and incidents updates.
Officers may be updated by volunteers (demographics and other info), so displaying that date might be useful. We may want to have that be an auto-update/TIMESTAMP NOW
column.
Assignments will be updated programmatically and in bulk, but those could also be updated by volunteers. Perhaps that should be auto-updated as well?
Incidents makes sense to have separately too since that's another dimension by which we could update info!
What do you think?
OpenOversight/app/models.py
Outdated
@@ -59,6 +60,24 @@ def toCustomDict(self): | |||
"unique_internal_identifier_label": self.unique_internal_identifier_label, | |||
} | |||
|
|||
def latest_incident_date(self) -> datetime.date: |
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.
Should we cache these at all? Would that make sense to do/affect anything?
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.
Yup, that makes a lot of sense for these values! Do you have any suggestions on what to use for caching? I know there's the built-in functools cache but we probably want to be able to set a TTL on these dates
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.
Added cachetools caching!
<p> | ||
|
||
<div> | ||
<i class="dept-{{department.id}}">Officers Updated: {{department.latest_officer_update()}}</i> |
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 displays as None
if there's no date, could we use "No data" in those cases instead?
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.
Yup, changed!
Great! (And just to re-iterate, we only need to run it once in prod to tell it to skip all the previously committed migrations)
So what I'm hearing is that we want to add an auto-updating |
Yes! Nailed it exactly 🚀 |
OpenOversight/app/models.py
Outdated
@@ -39,6 +42,27 @@ | |||
) | |||
|
|||
|
|||
date_updated_cache = TTLCache(maxsize=1024, ttl=60) |
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.
I forgot to update the ttl but does a 12 hour ttl make sense?
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.
Yes I think so!
date_updated = db.Column( | ||
db.DateTime, default=func.now(), onupdate=func.now(), index=True | ||
) |
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 might make bulk updates really slow since we'd be updating lots of values in this column
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.
I think even at scale that shouldn't be too impactful, the rows are getting updated anyway and in Postgres that means pulling out the whole row at once. I think we should be okay!
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.
Looks great! Can't wait to get this out!
Description of Changes
Fix #82.
Adds officer, incident, and assignment last updated date for each department to the browse page.
I just threw the updated dates under the department header but let me know if you can think of a better way to display the dates!
Notes for Deployment
Make sure alembic is run!
Screenshots (if appropriate)
SPD has last updated dates for officers, incidents, and assignments while APD has none
Tests and linting
I have rebased my changes on
main
just lint
passesjust test
passes