AA4: Flawed implementation of render logic for new menu
Context
Alliance Auth has a flexible sidebar menu. Apps can add custom entries to the sidebar by defining hooks. Hooks give apps a lot of control about how their menu entries look and which users can see them (e.g. based on permissions).
AA4 aims to add four features to the sidebar:
- Ability to add custom menu items (i.e. linking to an external web page)
- Ability to modify the order of all menu entries
- Ability to group menu entries in folders, which can be collapsed/expanded
- Adopt styling to Bootstrap 5
Current implementation
Conceptually this means that there are now 3 types of "menu items":
- App entries: generated by hooks from Django apps (same as before)
- Link entries: Added by users (new)
- Folder: Grouping together several app or link entries (new)
To persists changes all menu items are now stored in the DB through the new model ModelItem. Items for app entries are synced with the menu item hooks of all currently installed apps, while link entries and folders can be added through the admin site.
For creating the HTML for the sidebar, the menu entries are rendered partly from the MenuItem objects (for links and folders) and partly by executing the render method of the menu item hooks. In addition there is logic in the main template that allows rendering the the folders with their child items. However, all types of menu items are rendered by the same template.
What the problem is
The issue is that the same template is used to render different objects:
- MenuItem model objects (for link entries mostly)
- MenuItemHook objects (for app entries)
- A dictionary generated from MenuItem objects (for folders mostly)
This couples these three object types and the template tight together. i.e. all three object types need to have the same properties as expected by the template. A change to any one of them needs to be replicated to all others or the whole thing can break. However, these objects have no visible dependencies in the code (e.g. class inheritance).
Why this is bad
This approach is bad, because it creates a trap for anyone who wants to change this later. One has to know about these dependencies in order to not break it. On top of that, templates often break silently, which will make such bugs even harder to find.
How to proceed
There are at least 3 different approaches for dealing with this issue:
Reset
Remove this feature from AA4 for now. This feature is not critical for the new release and can be added later, once a better implementation has been found.
Mitigate
Try to "fix" the current implementation by refactoring and/or adding special tests that verifies the needed dependencies between all object types. Also add documentation.
Ignore
Ignore the risks and release this feature in it's current state. NOT RECOMMENDED.
I am currently working on "mitigate".