Refactor console_navbar for maintainability #2169
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 14 commits
December 21, 2023 14:01
This extends django.contrib.auth.decorators.permission_required: - It sets raise_exception=True, which we do for all console views (to be less confusing, I guess.) - It stores the permission name in an attribute, so that it can be introspected to determine what views the user might have permission for.
This view itself is somewhat of a placeholder and doesn't currently show any important information. But it also doesn't hurt to add a permission requirement so that we don't have to treat this view as a special case. The user.can_view_admin_console permission controls whether the "Admin Console" link appears in the nav bar, so it makes sense to restrict this view to people who have the user.can_view_admin_console permission.
This looks like perhaps some kind of leftover debugging thing. There is no "admin" template variable anywhere that I can see.
HTML IDs must be unique. Furthermore, to avoid confusion or possible
clashes, all of the IDs used in the navbar should begin with "nav_".
All of the #sectionComponents are renamed to #nav_section_components
(corresponding to #nav_section_dropdown), for consistency.
The "credentialing" section is renamed to "identity". The "usage"
section is renamed to "stats".
For consistency, ensure all of the page links have an ID. All of
these links are renamed to #nav_viewname (corresponding to
{% url 'viewname' %}.)
For those links that kludgily point to a URL with a mandatory
parameter, use #nav_viewname__arg.
For consistency, ensure that all of the page links have the same class.
None of these elements have tooltips.
Fix the style when the submenu is expanded on page load (note the arrow.) Remove the useless empty list element.
The new class NavMenu is used to construct the navigation menu links. Previously, console_navbar.html determined which links and submenus should be displayed by hardcoding the permissions, which needed to match the corresponding permissions for the view. This is now done automatically, by relying on the @console_permission_required decorator. Previously, in order to determine which link should be highlighted as "active" and which submenu should be visible by default, there was a separate "something_nav" parameter for every menu item, and the corresponding view needed to set the correct parameter by hand. This is now done automatically by comparing the request path to the link path. (This relies on the assumption that if page X is a logical sub-component of page Y, then URL Y is a prefix of URL X. That assumption might be wrong in some cases - in which case we should change the URLs to paths that make more sense, and perhaps add redirections if needed. Note in particular that every page in the console probably ought to have exactly one of the menu-item URLs as a prefix.)
|
Full of surprises @bemoody. Big improvement! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
console/templates/console/console_navbar.htmldefines the structure of the navigation sidebar/popup for the Admin Console.Currently this file contains a huge amount of boilerplate code, some of which is useless, some of which is buggy; it's full of logic that needs to be kept in sync with views.py, and that can't easily be tested by the test suite. The UI design is good, but the complicated implementation makes it needlessly difficult to add new features to the console.
With a bit of magic, this pull request removes the logic from the template and defines the menu structure in Python instead - replacing 300-ish lines of messy template code with 70-ish lines of Python (see the bottom of
console/navbar.py).This fixes some small bugs (styling of the stats menu, noscript accessibility) but shouldn't significantly change the appearance or functionality.
A few pages are now longer treated as "children" of their "parent" page, because their URLs don't match. I intend to fix this by changing the URLs, but I would like to do that in a separate pull request.