Load portadmin search results with htmx#3545
Conversation
Test results 27 files 27 suites 45m 22s ⏱️ Results for commit 4b984ba. ♻️ This comment has been updated with latest results. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3545 +/- ##
==========================================
+ Coverage 62.21% 62.47% +0.26%
==========================================
Files 611 611
Lines 44939 44997 +58
Branches 43 43
==========================================
+ Hits 27960 28114 +154
+ Misses 16969 16873 -96
Partials 10 10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ef41f86 to
650040c
Compare
johannaengland
left a comment
There was a problem hiding this comment.
I actually think this PR should have been multiple - one to use htmx for loading, one to refactor populate_infodict and one to add test coverage
Because I can see this PR being open quite long with reviews and changes going back and forth - especially since reviewing this took me quite a while
Maybe split it up still?
0be5d20 to
373129c
Compare
4ad10e6 to
05044b0
Compare
lunkwill42
left a comment
There was a problem hiding this comment.
I still haven't reviewed the code, I've just tested by hand, and this doesn't work very well for me. Did I do something wrong?
The "loading the port data using HTMX"-part seems to work fine, but the actual configuration functionality no longer works for me. If I try to change any configuration item, the modified rows are no longer highlighted as such for me, and so I am not allowed to click any of the save-buttons.
Leaving this as my initial review while I look at the actual code...
Yes, I am observing the same behavior, so something must've broken since the last time I manually reviewed this (which was when I first reviewed this PR) |
lunkwill42
left a comment
There was a problem hiding this comment.
Apart from the apparent non-workingness reported in my first review iteration, I have these comments:
-
3e829c0 commit log is "Add test coverage for port admin data loading views" - it seem to do no such thing?
-
I'm glad you're refactoring the ugly infodict-population code ❤️. I never liked that when I had to do maintenance on this code. But your changes inspire me to potentially add an issue about further refactoring opportunities here. I feel like maybe the infodict itself should be a separate class containing the data and also the methods to populate the data.
-
Many of the test names sound a bit opaque to me. Again, my primary measure of a good test name is one that gives me a good indication of what kind of assertion actually went wrong when I see the test flagged as failed in a report. I did suggest some name changes here, but there are several tests where I don't like the current name, but am also not sure what the name should be ...
|
I might also add that I find the title of the PR jarring (but I'm certain the Portadmin code is even more confusing to a newcomer than it is to me): The use of the term "search" in Portadmin is strange. I don't view the table of configurable port attributes as a "search result" at all, that just sounds strange. The only actual "search" function I find PortAdmin to have is the form that allows you to search for devices/ports to configure by name/description (which will actually send you to a search result page if there is more than a single match). I suspect most users don't even use this search box, but instead clicks through to the PortAdmin tool from the "configure ports" button in ipdevinfo - at which point this really isn't a search at all, it is just "show the form for this specific thing". |
I see what you mean. The existing views are I agree the wording isn’t great though, and a bit confusing given that there already is a search that lists interfaces and netboxes. You might have a better suggestion, but something like |
|
|
Can confirm that this works for me now, looking a bit more into it now :) |
lunkwill42
left a comment
There was a problem hiding this comment.
I like this a lot (though I agree with @johannaengland that it would have been easier to review if it had been broken down into smaller pieces, for example by having a separate refactoring PR before adding the actual HTMX bits).
The code looks fine to me, but there are usability issues when migrating to an HTMX-based solution:
Error handling
There doesn't seem to be any error handling if the back-end fails to load interface data in the HTMX request. I can easily make the view crash with a 500 error due to a ConnectRefusedError from the NAPALM library (simulated by changing the IP address of a switch in SeedDB to something that doesn't exist).
The error handling was non-existent for this in the original code as well (smh), but at least the user got feedback that something went wrong, in the form of a 500 page. In this PR, the "loading live data" keeps spinning indefinitely after the background request fails.
Strange table column setup when background request does fail gracefully
In cases where the background request does not crash, but fails more gracefully, the interface reverts to the "read-only" view of the IP Device. However, the column setup for VLANs looks weird - and I'm not sure if this is something new, or an old problem:
Loading indicator
It may be my personal opinion, but I don't like having multiple copies of the "loading live data" indicator spinning, one for each table row. It looks like something is trying to update each individual row, which is not the case, since we're waiting for a full table replacement.
What I envision in my head is a sort of "modal, slightly transparent, loading indicator dialog" on top of the table, which not only indicates that there isn't really anything to interact with on this page until the load is complete, but it would be located centrally in the viewport, rather than forcing my eyes to the right side of the screen to see what's going on globally.
2bc3f89 to
48923f7
Compare
|
@lunkwill42 I've added two commits which addresses your concerns. Error handlingI have added error handling for the 
Strange table column setup when background request does fail gracefullyThe issue is that the existing code does some weird conditional column spanning based on whether certain conditions are true. As these are all false while loading or when an error occurs, it causes layout shift. Since the old implementation did not have an initial loading view, this was not an issue before. I have adjusted the column layouts to be as consistent as possible, with the same view for loading and error states. See image above. Loading indicatorI have adjusted the loading indicator by moving it to the table caption, and adding loading styles to the portadmin table with opacity and 
|
48923f7 to
b793593
Compare
johannaengland
left a comment
There was a problem hiding this comment.
Looks good to me now, but I would wait for @lunkwill42's review
lunkwill42
left a comment
There was a problem hiding this comment.
Looks and works much better now!
I still do find the loading indicator a bit anonymous on a wide screen, but let's drop this on the users and see what the feedback is :)
(and, a small typo, which I trust you can fix without the need for a full review 😄 )
b793593 to
4b984ba
Compare
|
Scope and purpose
Resolves #3544.
This PR changes the Port Admin search views to show an initial loading page while fetching live data from the backend. This leads to a significant improvement in UX, as we can reassure the user that nothing is wrong with their internet connection or browser, but that connecting to the physical equipment just takes time.
As such, the
search_by_*views have been split into two different views per resource type, i.e.search_by_sysnameandload_portadmin_data('portadmin-sysname-data')search_by_ipandload_portadmin_data('portadmin-ip-data')search_by_interfaceidandload_portadmin_data('portadmin-interface-data')The
load_portadmin_dataview is reused for search bysysname,ipandinterfaceid. The internal logic handlesinterfaceidseparately fromipandsysname(which have the same behaviour).I am unsure about the text for the loading indicator, if anyone has a better suggestion please suggest something else :)
Screenshots
Before
After clicking on an interface, the page just loads for a long time
After
On initial load of a search
After live data has been loaded
This pull request
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.