Use a DurationField for heartbeats#1962
Conversation
Test results 8 files 1 736 suites 3m 14s ⏱️ Results for commit 2526843. ♻️ This comment has been updated with latest results. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1962 +/- ##
==========================================
+ Coverage 88.56% 88.58% +0.02%
==========================================
Files 148 148
Lines 7152 7168 +16
==========================================
+ Hits 6334 6350 +16
Misses 818 818 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
78794b0 to
88c2efa
Compare
lunkwill42
left a comment
There was a problem hiding this comment.
- Should we have an upper bound? If so, what? It is currently set to one day.
Yes. The whole feature becomes moot if heartbeats are configured to be days apart. 1 days seems fine for now.
- Should the built-in help-text be changed?
The message seems pretty confusing in the admin interface, but I guess that isn't the primary place for most users to edit after the source list page was added.
- Should the source form help text be changed? If yes, as part of this PR?
I guess it's formatting is ok for the form as is.
- Should there be a placeholder in the source form? (In admin is deffo a follow-up issue ;) )
Huh?
My only gripe atm. is that "last seen" for a source is editable in Django admin. Is that intentional?
A "placeholder" would mean that there is a fake value in the form field to illustrate what the value looks like. This is more work in the admin than in a form we control ourselves.
Out of scope. Know problem with the source admin, adding more readonly fields leads to "500 Server Error". |
Ah, that kind of placeholder (backend engineer laugh) That seems reasonable to ask for (even though the input format seems quite flexible, according to the help text) |
This means easier database queries but trickier frontend display.
9819f90 to
2526843
Compare
Too flexible, I could have made the table in the frontend much bigger.. |
|



Scope and purpose
Turns out the math on the database-side gets trickier if we store heartbeat frequency in seconds as an integer. See #1954, compare 78794b0 (IntegerField) with 043f69d (DurationField).
The downside of using a DurationField lies in the frontend forms needing polish, both on display and validation. For that reason, there's an additional commit that shows the frequency in the source pages. This could be split out into its own PR as it needs some polish, especially in the form.
This pull request
Note that this PR replaces a migration. Before checking out the branch, return to the migration before this one with
python manage.py migrate argus_incident 0002_s. Then check out this branch and run migrations as usual.Doing it as an additional migration would necessitate adding a column, doing a data migration, dropping the old column, renaming the new column to the old name.
Open questions
Conclusion: yes
Good as is, admin is admin.
Conclusion: keep as current.
Conclusion: yes to frontend, laters for admin
If it seems to take too long to agree on looks for the frontend this PR will be split into two, since other features are waiting for the field to exist.
Screenshots
Admin
Source list
Source-form page
Contributor Checklist
Every pull request should have this checklist filled out, no matter how small it is.
More information about contributing to Argus can be found in the
Development docs.