- 
                Notifications
    
You must be signed in to change notification settings  - Fork 45
 
Refactor dashboard navlets to use HTMX #3634
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
base: master
Are you sure you want to change the base?
Conversation
          Test results   11 files     11 suites   12m 21s ⏱️ For more details on these failures, see this check. Results for commit 143c621. ♻️ This comment has been updated with latest results.  | 
    
          Codecov Report❌ Patch coverage is  Additional details and impacted files@@            Coverage Diff             @@
##           master    #3634      +/-   ##
==========================================
+ Coverage   62.81%   62.82%   +0.01%     
==========================================
  Files         611      611              
  Lines       45125    45162      +37     
  Branches       43       43              
==========================================
+ Hits        28344    28374      +30     
- Misses      16771    16778       +7     
  Partials       10       10              ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
  | 
    
0303f9b    to
    577b2c8      
    Compare
  
    577b2c8    to
    79f0b28      
    Compare
  
    79f0b28    to
    c9d6286      
    Compare
  
    c9d6286    to
    20865d9      
    Compare
  
    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.
Add one commit that fixes the get's and post's of all the TemplateView desecendants to also have *args (or *_) and **kwargs (or **__) even if they don use them. Devs used to Django expect it.
| raise NotImplementedError | ||
| 
               | 
          ||
| def get_template_names(self): | ||
| def get_template_names(self, override_mode=None): | 
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.
Put in the class docstring what override_mode is used for.
        
          
                python/nav/web/navlets/__init__.py
              
                Outdated
          
        
      | return context | ||
| 
               | 
          ||
| def post(self, _request, **kwargs): | ||
| def post(self, request, **kwargs): | 
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.
Change signature to def post(self, request, *_, **kwargs):, due to Liskov substitution principle: as this is now it's a TemplateView that breaks the rules of a TemplateView.
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.
Whoa, many old sins in this code, Liskov-wise...
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.
Done
| @property | ||
| def mode(self): | ||
| """Fetch the mode of this request""" | ||
| return self.request.GET.get('mode', NAVLET_MODE_VIEW) | 
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.
Changing viewclass.as_view() to spit out two different classes depending on mode might be better, but is out of scope. Django-style would be to have the mode as part of the url (`path("something/str:mode", ..)  and two different views but that's even more out of scope.
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 agree. The Navlets code is quite strange, and really we should separate GET and POST at least. I opted not to dig too much into the Navlets in this round, and leave that for later.
        
          
                python/nav/web/navlets/graph.py
              
                Outdated
          
        
      | url = self.preferences.get('url') | ||
| timestamp = int(datetime.now().timestamp()) | ||
| context['graph_url'] = f'{url}&bust={timestamp}' | 
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.
Cache-busting? Add a comment about it.
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.
The bust parameter was defined here.
nav/python/nav/web/static/js/src/plugins/navlet_controller.js
Lines 129 to 139 in 16d6419
| if (this.navlet.image_reload) { | |
| this.refresh = setInterval(function () { | |
| // Find image each time because of async loading | |
| var image = that.node.find('img[data-image-reload], [data-image-reload] img'); | |
| if (image.length) { | |
| // Add bust parameter to url to prevent caching | |
| var uri = new URI(image.get(0)).setSearch('bust', Math.random()); | |
| image.attr('src', uri.href()); | |
| } | |
| }, preferences.refresh_interval); | |
| } else if (this.navlet.ajax_reload) { | 
I created a new staticmethod on the Navlet class, add_bust_param_to_url
        
          
                python/nav/web/navlets/vlangraph.py
              
                Outdated
          
        
      | timestamp = int(datetime.now().timestamp()) | ||
| context['graph_url'] = f'{url}&bust={timestamp}' | 
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.
Twice now.. should we have a cache-buster function?
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 created a method on the Navlet class
| hx-post="{% url 'add-user-navlet' dashboard_id %}" | ||
| hx-swap="beforeend" | ||
| > | 
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.
What happens if you keep action and post in addition to what's here now?
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.
The native form submit event is overruled by HTMX, so adding them would make no difference. Action is only used for html form submission, or by certain javascript submit handlers across the codebase.
06a86d6    to
    4418f4c      
    Compare
  
    4418f4c    to
    143c621      
    Compare
  
    
          
 | 
    




Scope and purpose
Resolves #3635.
This PR refactors the dashboard navlets logic from a complicated JS flow, to use HTMX where possible, and simplifies necessary javascript plugin interoperability.
See issue #3635 for a full description of changes.
This pull request
navlet_controllerandnavlets_controllerwithnavlets_htmx_controllerHow to test
Loading navlets
Navlet Load
Adding and Removing Navlets
Dashboard Layout
Re-ordering Navlets
Updating columns
Compact Dashboard Mode
Editing Navlets
Periodic Refresh of Navlets
Refresh with standard interval
This can be tested with all Navlets that have a
refresh_intervaldefined, and don't haveajax_reloadorimage_reloadset toTrue. I have selected RoomStatus as an example as the refresh interval is short.Refresh with
ajax_reload=TrueAlert,PDUandUPSnavlet.renderrequests.Refresh for VlanGraphNavlet and GraphWidget
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.