Replace QuickSelect in DeviceHistory tool with HTMX#3440
Conversation
Test results 12 files 12 suites 12m 5s ⏱️ Results for commit 18190f6. ♻️ 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 @@
## master #3440 +/- ##
==========================================
- Coverage 60.96% 60.77% -0.20%
==========================================
Files 607 608 +1
Lines 44062 44053 -9
Branches 48 48
==========================================
- Hits 26863 26772 -91
- Misses 17187 17269 +82
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
9a0c844 to
9620b12
Compare
Replace quickselect in Device History error events with HTMX
9620b12 to
7aa7a1d
Compare
Looking at the original form, having Device Groups under "register error event" makes no sense. Error events cannot be logged for device groups, they must be logged for individual devices. I get the distinct feeling that device groups must have been added to the QuickSelect component at some point, and that whoever added it forgot to ensure it wasn't enabled on the "register error event" page. So I'd say your solution to this conundrum is a-ok. |
There was a problem hiding this comment.
Looks pretty good - but the button labels on the history search page are now misnomers: They say "Add this" or "Add that". But these buttons do not "add" anything, they execute a search based on what you've selected in the list (see the button text in the old UI).
I also realized we should follow up this PR with a fix for #1996 - and also that we should potentially follow up with something more complicated for netboxes, since the Netbox' model's relation to Device has change significantly since the device history tool was written...
0385fc8 to
435acb5
Compare
Oops! The search utility now takes a (required) button text pattern instead of a suffix, and modified the tests to reflect this. |
435acb5 to
18190f6
Compare
|
lunkwill42
left a comment
There was a problem hiding this comment.
Everything LGTM, except: The confirm/submit-button is missing from the "Register error event" form once I've selected a device to add an error comment to...
Belay that last comment - there are some inconsistencies in the UI I'm still trying to figure out...
lunkwill42
left a comment
There was a problem hiding this comment.
The whole of device history gets a bit confusing UX-wise, and it isn't really this PR's fault. We have a lot of followup work for usability here, methinks :-)
Scope and purpose
Implements #3434, replacing QuickSelect in DeviceHistory with an HTMX implementation, similar to for Maintenance Tasks.
The existing code has some discrepancy in the Register Error Event form, such as including device groups in the search, but not considering them in the form submission. This leaves only netboxes and modules as possible devices to register errors on. However, the
register_error_eventssupports both locations, rooms, netboxes and modules. As such, I was not able to understand the conflicting "business rules" by looking at the code.As a "solution" i opted to remove the device group select, and only make netboxes and modules searchable in the form. If the intended behavior is to support all component types in this form, I can implement this in a follow up PR.
A follow-up task it to remove QuickSelect from the codebase entirely, which was omitted to keep this PR small.
Screenshots
Before
Device History Search
Register Error Event
After
Device History search
Register Error Event
Contributor Checklist
Every pull request should have this checklist filled out, no matter how small it is.
More information about contributing to NAV can be found in the
Hacker's guide to NAV.
<major>.<minor>.x). For a new feature or other additions, it should be based onmaster.