• 
      

    Convert the Admin UI sidebar to use the action framework.

    Review Request #14721 — Created Dec. 2, 2025 and submitted

    Information

    Review Board
    release-7.1.x

    Reviewers

    The admin sidebar used to be composed based off of an HTML file
    populated using state from a template tag, with template hooks to append
    custom HTML to the sections. While there was some degree of
    extensibility, it was a bit limited, requiring the consumer to generate
    just the right HTML.

    This change converts the sidebar to use actions instead. There's now an
    action group per group in the sidebar, and an action per child.
    Specialized sidebar action renderers take care of rendering with
    existing support for the template hook points.

    The Manage section has the most specializations. The group renderer
    takes care of looking up the counts and Add Item URLs for each child
    action, caching the results for quicker lookup. The item renderer then
    provides this to a template, based on the cached state (falling back to
    direct querying if not in the cache).

    With this, extensions can now simply register actions in the Admin UI
    sidebar, taking advantage of the built-in rendering and capabilities.

    Tested all the links in the admin UI.

    Tested fallback Manage item logic when the cache results come up empty.

    Summary ID
    Convert the Admin UI sidebar to use the action framework.
    The admin sidebar used to be composed based off of an HTML file populated using state from a template tag, with template hooks to append custom HTML to the sections. While there was some degree of extensibility, it was a bit limited, requiring the consumer to generate just the right HTML. This change converts the sidebar to use actions instead. There's now an action group per group in the sidebar, and an action per child. Specialized sidebar action renderers take care of rendering with existing support for the template hook points. The Manage section has the most specializations. The group renderer takes care of looking up the counts and Add Item URLs for each child action, caching the results for quicker lookup. The item renderer then provides this to a template, based on the cached state (falling back to direct querying if not in the cache). With this, extensions can now simply register actions in the Admin UI sidebar, taking advantage of the built-in rendering and capabilities.
    62600e33436f83cf6d95a0e6fb95ce44f4a28a0c

    Description From Last Updated

    Can we add tests for some of the new behavior? get_default_admin_actions() get_default_admin_attachment_points() Item counts/urls.

    david david

    Shouldn't this inherit from BaseAdminSidebarGroupAction

    david david

    Shouldn't this inherit from BaseAdminSidebarGroupAction

    david david

    This is no longer setting an expiration on the cache, which means it's not 5 minutes, it's 1 month.

    david david

    This ends up creating the queryset object when the class is defined, which seems like it might create confusing bugs …

    david david

    It looks like this method is unused now. Can it go away?

    david david

    This should be typed to return Iterator[ActionAttachmentPoint]

    david david

    Typo: actinos -> actions

    david david

    This is making an assumption that sidebar_hook_point is non-None, and it seems like that's the case for the existing groups …

    david david

    Shouldn't we type all of the class variables in this class and in BaseAdminSidebarManageItemAction with ClassVar?

    maubin maubin

    This and the rest of the private methods below need a version added.

    maubin maubin

    This is missing from the Args docs.

    maubin maubin

    This is missing from the Args docs.

    maubin maubin
    david
    1. 
        
    2. Show all issues

      Can we add tests for some of the new behavior?

      • get_default_admin_actions()
      • get_default_admin_attachment_points()
      • Item counts/urls.
      1. Forgot to add the tests files. Adding and updating for some other changes I've made. But I don't know that there's really anything to gain from tests for get_default_admin_*, since these are just returning instances and don't contain any real logic. What were you wanting there?

    3. reviewboard/admin/actions.py (Diff revision 1)
       
       
      Show all issues

      Shouldn't this inherit from BaseAdminSidebarGroupAction

    4. reviewboard/admin/actions.py (Diff revision 1)
       
       
      Show all issues

      Shouldn't this inherit from BaseAdminSidebarGroupAction

    5. reviewboard/admin/actions.py (Diff revision 1)
       
       
      Show all issues

      This ends up creating the queryset object when the class is defined, which seems like it might create confusing bugs (especially in the case of running tests). I feel like this would be much better done as a property that can be reimplemented in subclasses and return a new QuerySet when needed.

      1. I had forgotten the second step in this pattern (which Django also uses), which is to turn the class-defined instance into a local one (adding a .all() at the end does this). I glossed over this part while working on another problem.

    6. reviewboard/admin/actions.py (Diff revision 1)
       
       
      Show all issues

      This should be typed to return Iterator[ActionAttachmentPoint]

    7. 
        
    chipx86
    david
    1. 
        
    2. reviewboard/admin/actions.py (Diff revisions 1 - 2)
       
       
       
       
       
       
       
       
      Show all issues

      This is no longer setting an expiration on the cache, which means it's not 5 minutes, it's 1 month.

      1. Yeah, I removed that now that we have the proper cache invalidation. It's no longer depending on eventually-consistent behavior, which was really noticeable in some of my tests.

      2. Can we clarify that in the comment above?

    3. reviewboard/admin/actions.py (Diff revisions 1 - 2)
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      It looks like this method is unused now. Can it go away?

    4. Show all issues

      Typo: actinos -> actions

    5. Show all issues

      This is making an assumption that sidebar_hook_point is non-None, and it seems like that's the case for the existing groups here, but that attribute is typed as str | None and defaults to None, and template_hook_point doesn't do any validation there. Can we wrap this in an if?

    6. 
        
    chipx86
    chipx86
    chipx86
    david
    1. Ship It!
    2. 
        
    maubin
    1. 
        
    2. reviewboard/admin/actions.py (Diff revision 5)
       
       
      Show all issues

      Shouldn't we type all of the class variables in this class and in BaseAdminSidebarManageItemAction with ClassVar?

      1. Yeah, I suppose we can. Initially I was letting subclasses modify some of this at instantiation time, but I moved to methods when adding error handling.

    3. reviewboard/admin/actions.py (Diff revision 5)
       
       
      Show all issues

      This and the rest of the private methods below need a version added.

    4. reviewboard/admin/actions.py (Diff revision 5)
       
       
      Show all issues

      This is missing from the Args docs.

    5. reviewboard/admin/actions.py (Diff revision 5)
       
       
      Show all issues

      This is missing from the Args docs.

    6. 
        
    chipx86
    maubin
    1. Ship It!
    2. 
        
    david
    1. Ship It!
    2. 
        
    chipx86
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-7.1.x (9992cc3)