Replace oscar dashboard navigation with menu registry #3771
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #3771 +/- ##
==========================================
- Coverage 88.28% 88.25% -0.03%
==========================================
Files 293 306 +13
Lines 16046 16264 +218
==========================================
+ Hits 14166 14354 +188
- Misses 1880 1910 +30
|
| (which by default is the lowercase equivalent to the parent menu's label | ||
| with spaces replaced by ``_`` e.g. ``Parent menu`` label will by default | ||
| generate ``parent_menu`` as it's ID. An alternative ID can be specified | ||
| using the ``identifier`` kwarg) as follows: |
There was a problem hiding this comment.
The is no example below of the identifier kwarg?
| label=_("Admin site"), | ||
| icon="fas fa-list", | ||
| url_name="admin:index", | ||
| access_fn=lambda user, url_name, url_args, url_kwargs: user.is_staff, |
There was a problem hiding this comment.
I think it would be useful to specify access_fn as a separate function, fully explaining the prototype of the function, eg:
def staff_only_access_fn(user, url_name, url_args, url_kwargs):
"access_fn is passed the user, url_name, urls_args and url_kwargs"
return user.is_staff
This shows it can be defined once and reused.
| """ | ||
| for name, module in _get_app_modules(): | ||
| if module_has_submodule(module, submodule_name): | ||
| yield name, import_module('%s.%s' % (name, submodule_name)) |
There was a problem hiding this comment.
I don't think we need to copy any of this from wagtail.
I think you should write it like this:
def discover_app_modules(module_name):
"Searches all registered app for submodules with the name ``module_name``"
for app in apps.get_app_configs():
if module_has_submodule(app.module, submodule_name):
yield app.name, import_module('%s.%s' % (app.name, submodule_name))
| return self.position < other.position | ||
|
|
||
| def __eq__(self, other): | ||
| return self.identifier == other.identifier |
There was a problem hiding this comment.
I don't think this is a very intuitive definition of equality. For menus I think the intuition is that 2 menus are equal if they are equal and all of the children are equal. I think you should write a methode identifier_equal and use that instead.
| self.label = label | ||
| self.url_name = url_name | ||
| self.icon = icon | ||
| self.identifier = identifier or label.lower().replace(" ", "_") |
There was a problem hiding this comment.
I don't think making the identifier translated is a good idea, it just makes no
sense, because it is not guaranteed that such values are different across
locales. It seems to have no purpose. If you insist on making the identifier optional,
I think you should use the "english" translation of the string by default, just like
permission names in the django auth database. Since this is not an actual
rocksolid method (it fails if the string is translated in english), I suggest
that in oscar itself the identifier is always specified.
That way can remove code like
_parent_id = _("offers")
and just write "offers"
What is the reason you don't use slugify to generate the identifier? If the
idea is to generate a valid python identifier, the code used is too simple.
If possible, use slugify on the english translation of the label.
Something like this:
if identifier is not None:
self.identifier = identifier
else:
with override("en"):
self.identifier = slugify(label)
|
|
||
|
|
||
| class Menu: | ||
| def __init__(self, label, url_name=None, icon=None, identifier=None, position=None, **use_as_provided): |
There was a problem hiding this comment.
It is more pythonic to name **use_as_provided **kwargs 😉
| 1. preserving the order of addition. | ||
| 2. merging `menu` to existing menu (with same identity). | ||
| """ | ||
| for index, child in enumerate(self._children.copy()): |
| def decorator(decorated_function): | ||
| register_dashboard_menu(decorated_function, parent_id) | ||
| return decorated_function | ||
|
|
There was a problem hiding this comment.
What is the purpose of the above code?
There was a problem hiding this comment.
The code aims to support conditional menu registration (using annotations) e.g.
@register_dashboard_menu()
def register_exchange_rate_menu():
if len(get_supported_currencies_setting().keys()) > 1:
return Menu(label="Exchange rates", icon="fas fa-money-bill-alt", url_name="dashboard:exchange-rate-list")| # Placeholder instance will be used temporarily if child (menu) was registered ahead of | ||
| # its parent. It will be replaced (merged) into the actual parent once it's registered. | ||
| _dashboard_navigation_menu.setdefault(parent_id, Menu.placeholder(parent_id)).add_child( | ||
| menu).auto_position_if_applicable(len(_dashboard_navigation_menu)) |
There was a problem hiding this comment.
Can you add comments to the above code and explain why there are so many different types of object accepted as input to create a menu?
| else: | ||
| raise ValueError(f"Parent menu with the ID '{parent_id}' does not exist") | ||
| else: | ||
| del _dashboard_navigation_menu[parent_id] |
There was a problem hiding this comment.
Please put the code that deals with removing deregistered menus in a separate function.
…SCAR_DASHBOARD_NAVIGATION-with-menu-registry
…SCAR_DASHBOARD_NAVIGATION-with-menu-registry
…gistry' of github.com:ajharry69/django-oscar into feature/replace-OSCAR_DASHBOARD_NAVIGATION-with-menu-registry
|
Please let me know if this is ready for review again |
@specialunderwear it is - please review. |
…AR_DASHBOARD_NAVIGATION-with-menu-registry
This PR aims to provide an alternative approach to the dashboard navigation menu registration. It replaces the
OSCAR_DASHBOARD_NAVIGATIONsetting with a hopefully more flexible registry pattern. See the updated doc on how the approach works.