Conversation
|
Test results 6 files 1 182 suites 2m 22s ⏱️ Results for commit 9e13baa. ♻️ 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 #1820 +/- ##
=======================================
Coverage 87.96% 87.96%
=======================================
Files 139 139
Lines 6848 6849 +1
=======================================
+ Hits 6024 6025 +1
Misses 824 824 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
hmpf
added
data model
hmpf
force-pushed
the
start-of-heartbeat
branch
3 times, most recently
from
3775cb1 to
9cdf079
Compare
|
Different PRs:
|
hmpf
added
the
nonews
aleksfl
left a comment
aleksfl
left a comment
There was a problem hiding this comment.
Looks good, not much to comment.
| help_text="Base url to combine with an incident's relative url to point to more info in the source system.", | ||
| blank=True, | ||
| ) | ||
| last_seen = models.DateTimeField(null=True, blank=True) |
There was a problem hiding this comment.
Should this field also have help text, e.g. "Timestamp of last seen heartbeat" ?
There was a problem hiding this comment.
At this point it is not strictly a heartbeat =)
There was a problem hiding this comment.
There will never under any circumstances be a form field for this field, so in that context, a help text is never needed.
| text_input_form_fields = ("name",) | ||
| raw_id_fields = ("user",) | ||
| readonly_fields = ("last_seen",) | ||
| date_hierarchy = "last_seen" |
There was a problem hiding this comment.
How will this behave when most values are initially null?
Edit: Meant to only mark line 63
There was a problem hiding this comment.
You can test it yourself in the admin - nothing is different until it is set for at least one source and then you can navigate into the different dates and if you click on "All dates" then the ones without a date are shown
There was a problem hiding this comment.
The easiest way to test is to remove line 62 at first and set last_seen for some source
There was a problem hiding this comment.
Well, having it in readonly_fields breaks things so spectacularly that I've had to remove it.
Having it in date_hierarchy works fine. If last_seen is set on at least one source, the date hierarchy is generated and shown. If not, the hierarchy is hidden.
hmpf
force-pushed
the
start-of-heartbeat
branch
from
ad62597 to
83ca9fb
Compare
hmpf
force-pushed
the
start-of-heartbeat
branch
from
83ca9fb to
1fa9963
Compare
Test naming conventionAll new test names follow the convention. 👍 |
hmpf
force-pushed
the
start-of-heartbeat
branch
2 times, most recently
from
93b526f to
3542fc3
Compare
hmpf
force-pushed
the
start-of-heartbeat
branch
from
3542fc3 to
9e13baa
Compare
|
4 participants
Scope and purpose
For #1160
This only adds the minimum alteration possible to the SourceSystem model. Baby steps!
This pull request
NOTE!
The last_seen field should be read only in the admin. BUT: if we do that, then suddenly tests fail because the admin stops honoring the
exclude = ["user"]in the AddSourceSystemForm (which is used by the admin). WAAT? Visiting the admin shows that the "user" field is shown in the add-form if "last_seen" is read only, and not shown if "last_seen" is editable.This might be a bug in the Django admin itself, and needs at least its own issue.
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.