-
Notifications
You must be signed in to change notification settings - Fork 263
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
App Defined Navbar #485
App Defined Navbar #485
Conversation
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.
Good start!
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.
Unless there is a specific reason for it, can you please rename nautobot.core.nautobot_app
to nautobot.core.apps
in keeping with the same naming pattern w/ the rest of the internal apps, and also Django convention for configuring applications?
…Added isinstance and issubclass checking. Added new choiceset for button colors.
… item list to item dict.
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.
Can you add a test case or two to make sure that the various parts of the nav menu template are rendered as expected (menus, sections, items, buttons)?
This PR should probably be retargeted to |
…d if statement. Added new templatetag to return True if user has at least one permission.
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 think this still needs:
- Retarget PR to develop-1.1 branch
- Add release-note description of this new feature
- Add tests for rendering of
nav_menu.html
(possibly built atop the integration test / Selenium infrastructure, as that provides easier introspection of rendered HTML than the base UT infra readily provides?) - Add documentation (probably in
nautobot/docs/development/application-registry.md
) about how to make use of this feature as a developer.
…ded error if failed to login with test suite.
try: | ||
self.selenium.find_element_by_xpath(tab_xpath) | ||
raise Exception(f"Tab element {tab_name} should not exist.") | ||
except NoSuchElementException: | ||
pass |
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.
try: | |
self.selenium.find_element_by_xpath(tab_xpath) | |
raise Exception(f"Tab element {tab_name} should not exist.") | |
except NoSuchElementException: | |
pass | |
with self.assertRaises(NoSuchElementException): | |
self.selenium.find_element_by_xpath(tab_xpath) |
…_render_restricted_ui. Fixed navigation wrong link.
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.
Looking damn good! Just some nitpicks as I tend to do.
try: | ||
self.user = User.objects.create_user(username="testuser") | ||
except IntegrityError: | ||
self.user = User.objects.get(username="testuser") |
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.
Use get_or_create
here please.
try: | |
self.user = User.objects.create_user(username="testuser") | |
except IntegrityError: | |
self.user = User.objects.get(username="testuser") | |
self.user, _ = User.objects.get_or_create(username="testuser") |
nautobot/core/apps/__init__.py
Outdated
class NavMenuMixin: # replaces PermissionsMixin | ||
"""Base class for navigation classes.""" | ||
|
||
@property | ||
def initial_dict(self): # to be implemented by each subclass | ||
return {} | ||
|
||
@property | ||
def fixed_fields(self): # to be implemented by subclass | ||
return () |
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.
Can you please make this inherit from abc.Abc
and make the properties in to abc.abstractproperty
instead? This will help to enforce the interface. Also since this is a base and not a mixin, I feel like we should instead have it named as such?
self.user.save() | ||
|
||
# Retrieve home page | ||
self.selenium.get(f"{self.live_server_url}") |
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.
No need to f-string this if it's just a single variable.
self.selenium.get(f"{self.live_server_url}") | |
self.selenium.get(self.live_server_url) |
self.selenium.get(f"{self.live_server_url}") | ||
self.selenium.wait_for_html("body") |
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.
These two lines are repeated in several tests. Perhaps we should factor this into a helper-method on the SeleniumTestCase
? Like load_page(url)
or something?
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 reviewed the various navigation.py
files and found a number of issues - I'm concerned that there may be others I've missed though. What have you done to manually verify the correctness of the menus - perhaps compare them side-by-side with the Nautobot demo site's menus?
…permissions. added more choices to button choicesets.
@@ -2086,4 +2086,4 @@ urllib3 = [ | |||
zipp = [ | |||
{file = "zipp-3.4.1-py3-none-any.whl", hash = "sha256:51cb66cc54621609dd593d1787f286ee42a5c0adbb4b29abea5a63edc3e03098"}, | |||
{file = "zipp-3.4.1.tar.gz", hash = "sha256:3607921face881ba3e026887d8150cca609d517579abe052ac81fc5aeffdbd76"}, | |||
] | |||
] |
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.
Revert?
…ing in doc string.
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.
Fixes: #12
New app config class allows each app to define its Navbar properties through a navigation file.
The config below shows how the navbar could be configured.
Weights per property defines the position of the item in the menu. This will allow plugins to add to or define new top level items.
Every tab and group have weights which are multiples of 100 allowing space between each items for plugins to use.