Port existing actions over to the new framework.

Review Request #12757 — Created Dec. 22, 2022 and submitted

Information

Review Board
release-6.x

Reviewers

The new actions framework has a lot more flexibility and consistency
than the old one. This change ports over our existing review request
actions and extension hooks to use it. In addition, several things which
worked like actions (like the star and archive/mute menu) but were
implemented outside of the framework have been rewritten to be actions
instead.

This also reimplements the client-side styling and logic for menus and
the mobile menu, using the new CSS component style and building on
RB.MenuView for the drop-downs. This gives us more consistent style
rules as well as support for keyboard and screen-reader accessibility.

  • Tested review request actions in several scenarios, including when
    logged out and in mobile mode.
  • Ran unit tests.
Summary ID
Port existing actions over to the new framework.
The new actions framework has a lot more flexibility and consistency than the old one. This change ports over our existing review request actions and extension hooks to use it. In addition, several things which worked like actions (like the star and archive/mute menu) but were implemented outside of the framework have been rewritten to be actions instead. This also reimplements the client-side styling and logic for menus and the mobile menu, using the new CSS component style and building on RB.MenuView for the drop-downs. This gives us more consistent style rules as well as support for keyboard and screen-reader accessibility. Testing Done: - Tested review request actions in several scenarios, including when logged out and in mobile mode. - Ran unit tests.
716400770d1d99a6bf79937527aa84c28ef5204b
Description From Last Updated

Typo in the description: "conistency" -> "consistency"

chipx86chipx86

undefined name 'parent' Column: 52 Error code: F821

reviewbotreviewbot

Expected '===' and instead saw '=='. Column: 46 Error code: W116

reviewbotreviewbot

Is it possible for people to make their own attachment points? From the docs it's unclear whether this is possible …

maubinmaubin

Should there be 2 spaces here?

maubinmaubin

Should there be 2 spaces here?

maubinmaubin

This could use some updating to mention how we can have non-UI actions and actions for the unified banner, maybe …

maubinmaubin

This could all fit on one line I think.

maubinmaubin

Add -> None:

maubinmaubin

Update the description, action arg type and Raises section of this method to include how the given action can be …

maubinmaubin

Missing a space between "reviewrequest".

maubinmaubin

I wonder if it would be worthwhile to make some mixins or decorators for some of the repeated access control …

maubinmaubin

Should we wrap reviewboard.actions.actions_registry in some Sphinx directive? Would :py:obj: work?

maubinmaubin

Should we wrap reviewboard.actions.actions_registry in some Sphinx directive? Would :py:obj: work?

maubinmaubin

Could sort this alphabetically.

maubinmaubin

Maybe worth making all of these links/refs to the appropriate attributes/methods?

chipx86chipx86

Super small nit, but I feel like this may read better with the IDs grouped together

chipx86chipx86

Can we document this, with a Version Added?

chipx86chipx86

While not a problem, we shouldn't need to do this, as initialize is intentionally empty by default.

chipx86chipx86

Can we add a type/docs for this on the class body?

chipx86chipx86

Changing the IDs here (and below) might break existing extensions.

chipx86chipx86

What's the TODO?

chipx86chipx86

Alignment issue.

chipx86chipx86

This can probably use self.create_http_request(...)

chipx86chipx86

This seems unrelated.

chipx86chipx86

Everything in here but the perms and seems to return a bool result. Maybe we just want bool(...) for perms?

chipx86chipx86

Same comment about perms here. Also, can we add a blank line before return statements?

chipx86chipx86

Leftover debugging information.

chipx86chipx86

This isn't something we can reverse?

chipx86chipx86

This doesn't have to be this change, but we should track it: We could improve this through a ReviewRequest.has_diffsets() method …

chipx86chipx86

Same comment about perms.

chipx86chipx86

Super tiny nit for all these similar blocks. We have 3 lines that pull from context, and one line that …

chipx86chipx86

Same here about perms.

chipx86chipx86

Same comment about perms.

chipx86chipx86

For these, it'd be nice to group related attributes. action_id and parent_id should probably go together. attachment and apply_to feel …

chipx86chipx86

Since extensions will most likely be using some subclass of this, we should ideally use that name. Not sure what …

chipx86chipx86

Can you put this in alphabetical order?

chipx86chipx86

Can this be self.child_actions = child_actions or []?

chipx86chipx86

Can you put this in alphabetical order?

chipx86chipx86

Can we state in the docs here and below that we're removing this in 7?

chipx86chipx86

Missing a trailing period.

chipx86chipx86

Missing a trailing period.

chipx86chipx86

Missing a trailing period.

chipx86chipx86

djblets is mixed in with django.

chipx86chipx86

Older code, but I think this can be: self.assertIs( self.action.should_render(context=request_context), getattr(self, 'read_only_always_show', False)) Though, maybe less obvious? I don't know.

chipx86chipx86

Too much JS! deprecation_message :) But with my other comments about fixing warnings to be per-class, this will need to …

chipx86chipx86

These should be assertIsNone. Same with many below.

chipx86chipx86

This is hard to read. Can we go with keyword arguments here, like create_http_request does? (And same elsewhere.) I almost …

chipx86chipx86

Can we explicitly return a value if we don't return in the loop? Right now we just get an implicit …

chipx86chipx86

While here, can we alphabetize these?

chipx86chipx86

Can we add Version Added to each of these?

chipx86chipx86

Can you put parens around the comparison? Helps with readability, given the =.

chipx86chipx86

We should keep to public before private. Also, it'd be nice to have docs for the public attributes.

chipx86chipx86

Extra blank line here.

chipx86chipx86

Can we break this down into the multi-line format? Any time we have typing in params. Same with others.

chipx86chipx86

Can we document this as well? And same with other public attributes.

chipx86chipx86

I don't think we need export {} anymore.

chipx86chipx86

Can we alphabetize these?

chipx86chipx86

actions should go after accounts, alphabetically.

chipx86chipx86

Missing quotes around the {{action.get_dom_element_id}}.

chipx86chipx86

No space needed before style.

chipx86chipx86

Missing quotes around {{action.get_dom_element_id}}.

chipx86chipx86
Checks run (2 failed)
flake8 failed.
JSHint failed.

flake8

JSHint

david
david
maubin
  1. 
      
  2. Show all issues

    Is it possible for people to make their own attachment points? From the docs it's unclear whether this is possible and how to do this, maybe we could elaborate on this.

  3. Show all issues

    Should there be 2 spaces here?

  4. Show all issues

    Should there be 2 spaces here?

  5. reviewboard/extensions/hooks.py (Diff revision 3)
     
     
     
    Show all issues

    This could use some updating to mention how we can have non-UI actions and actions for the unified banner, maybe you could link to the AttachmentPoint class too.

    1. This gets updated in a later change.

  6. reviewboard/extensions/hooks.py (Diff revision 3)
     
     
     
     
    Show all issues

    This could all fit on one line I think.

  7. reviewboard/extensions/hooks.py (Diff revision 3)
     
     
    Show all issues

    Add -> None:

  8. reviewboard/extensions/hooks.py (Diff revision 3)
     
     
    Show all issues

    Update the description, action arg type and Raises section of this method to include how the given action can be a BaseReviewRequestMenuAction.

    1. This gets updated in a later change.

  9. reviewboard/reviews/actions.py (Diff revision 3)
     
     
    Show all issues

    Missing a space between "reviewrequest".

  10. reviewboard/reviews/actions.py (Diff revision 3)
     
     
     
     
     
     
    Show all issues

    I wonder if it would be worthwhile to make some mixins or decorators for some of the repeated access control logic we have in the should_render methods for these actions.

    1. If they were all identical I'd say yes for sure. For now I think that's maybe more complexity than is worthwhile.

  11. reviewboard/reviews/actions.py (Diff revision 3)
     
     
    Show all issues

    Should we wrap reviewboard.actions.actions_registry in some Sphinx directive? Would :py:obj: work?

  12. reviewboard/reviews/actions.py (Diff revision 3)
     
     
    Show all issues

    Should we wrap reviewboard.actions.actions_registry in some Sphinx directive? Would :py:obj: work?

  13. reviewboard/reviews/errors.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Could sort this alphabetically.

  14. 
      
david
david
david
chipx86
  1. 
      
  2. Show all issues

    Typo in the description: "conistency" -> "consistency"

  3. Show all issues

    Maybe worth making all of these links/refs to the appropriate attributes/methods?

    1. I feel like people can click through to the class reference if they really want to.

  4. docs/manual/extending/extensions/hooks/action-hooks.rst (Diff revision 6)
     
     
     
     
     
     
    Show all issues

    Super small nit, but I feel like this may read better with the IDs grouped together

  5. reviewboard/extensions/hooks.py (Diff revision 6)
     
     
    Show all issues

    Can we document this, with a Version Added?

    1. I'm actually rearranging the code so all the extension changes will be in a single commit, and this doesn't exist in the later code.

  6. reviewboard/extensions/hooks.py (Diff revision 6)
     
     
    Show all issues

    While not a problem, we shouldn't need to do this, as initialize is intentionally empty by default.

  7. reviewboard/extensions/hooks.py (Diff revision 6)
     
     
    Show all issues

    Can we add a type/docs for this on the class body?

  8. reviewboard/extensions/hooks.py (Diff revision 6)
     
     
    Show all issues

    Changing the IDs here (and below) might break existing extensions.

    1. I'm going to change the internal ID here but I'll add an override of get_dom_element_id in order to keep the DOM-side the same.

  9. Show all issues

    What's the TODO?

    1. Oh, figuring out what I should put as the type hint.

  10. Show all issues

    Alignment issue.

  11. reviewboard/extensions/tests/test_action_hooks.py (Diff revision 6)
     
     
     
     
     
     
    Show all issues

    This can probably use self.create_http_request(...)

  12. Show all issues

    This seems unrelated.

  13. reviewboard/reviews/actions.py (Diff revision 6)
     
     
    Show all issues

    Everything in here but the perms and seems to return a bool result. Maybe we just want bool(...) for perms?

  14. reviewboard/reviews/actions.py (Diff revision 6)
     
     
     
    Show all issues

    Same comment about perms here.

    Also, can we add a blank line before return statements?

  15. reviewboard/reviews/actions.py (Diff revision 6)
     
     
    Show all issues

    This isn't something we can reverse?

    1. We're explicitly appending 'raw/' to whatever the current URL is.

  16. reviewboard/reviews/actions.py (Diff revision 6)
     
     
    Show all issues

    This doesn't have to be this change, but we should track it:

    We could improve this through a ReviewRequest.has_diffsets() method that checks:

    1. Is self._diffsets set (via get_diffsets()). If so, use that.
    2. Is diffset_history loaded from the database? If so, use that diffsets relation.
    3. Else, do DiffSet.objects.filter(history=self.diffset_history_id).exists()
  17. reviewboard/reviews/actions.py (Diff revision 6)
     
     
    Show all issues

    Same comment about perms.

  18. reviewboard/reviews/actions.py (Diff revision 6)
     
     
     
     
     
    Show all issues

    Super tiny nit for all these similar blocks. We have 3 lines that pull from context, and one line that doesn't, smack in the middle. It'd be nice to separate that one out.

  19. reviewboard/reviews/actions.py (Diff revision 6)
     
     
     
    Show all issues

    Same here about perms.

  20. reviewboard/reviews/actions.py (Diff revision 6)
     
     
     
    Show all issues

    Same comment about perms.

  21. reviewboard/reviews/actions.py (Diff revision 6)
     
     
     
     
     
    Show all issues

    For these, it'd be nice to group related attributes. action_id and parent_id should probably go together. attachment and apply_to feel related.

  22. reviewboard/reviews/actions.py (Diff revision 6)
     
     
    Show all issues

    Since extensions will most likely be using some subclass of this, we should ideally use that name.

    Not sure what the best approach is to get that name, since type(self).__name__ will return the subclass.

    But we probably should list that anyway in the error message, so there's a warning per-usage, instead of only one per-process (since warnings with the same text don't repeat).

    1. Extensions actually don't use this directly. The old extension hooks only allowed dict-based usage.

  23. reviewboard/reviews/actions.py (Diff revision 6)
     
     
     
     
     
    Show all issues

    Can you put this in alphabetical order?

  24. reviewboard/reviews/actions.py (Diff revision 6)
     
     
     
     
     
    Show all issues

    Can this be self.child_actions = child_actions or []?

  25. reviewboard/reviews/actions.py (Diff revision 6)
     
     
     
     
     
     
    Show all issues

    Can you put this in alphabetical order?

  26. reviewboard/reviews/actions.py (Diff revision 6)
     
     
     
    Show all issues

    Can we state in the docs here and below that we're removing this in 7?

  27. reviewboard/reviews/actions.py (Diff revision 6)
     
     
    Show all issues

    Missing a trailing period.

  28. reviewboard/reviews/actions.py (Diff revision 6)
     
     
    Show all issues

    Missing a trailing period.

  29. reviewboard/reviews/actions.py (Diff revision 6)
     
     
    Show all issues

    Missing a trailing period.

  30. reviewboard/reviews/tests/test_actions.py (Diff revision 6)
     
     
     
     
    Show all issues

    djblets is mixed in with django.

  31. reviewboard/reviews/tests/test_actions.py (Diff revision 6)
     
     
     
     
     
     
     
    Show all issues

    Older code, but I think this can be:

    self.assertIs(
        self.action.should_render(context=request_context),
        getattr(self, 'read_only_always_show', False))
    

    Though, maybe less obvious? I don't know.

    1. Yeah, I think I'd prefer this as-written.

  32. Show all issues

    Too much JS! deprecation_message :)

    But with my other comments about fixing warnings to be per-class, this will need to go away and move into the tests.

  33. reviewboard/reviews/tests/test_actions.py (Diff revision 6)
     
     
     
     
    Show all issues

    These should be assertIsNone.

    Same with many below.

  34. Show all issues

    This is hard to read. Can we go with keyword arguments here, like create_http_request does? (And same elsewhere.)

    I almost wonder if we should ditch self.request and just have tests provide what they want in create_http_request, so it's all self-contained. But probably not worth it for this change.

    1. I'll switch over to create_http_request for all of these.

  35. reviewboard/static/rb/js/pages/views/basePageView.es6.js (Diff revision 6)
     
     
     
     
     
     
     
     
    Show all issues

    Can we explicitly return a value if we don't return in the loop?

    Right now we just get an implicit undefined.

  36. Show all issues

    While here, can we alphabetize these?

  37. Show all issues

    Can we add Version Added to each of these?

  38. Show all issues

    Can you put parens around the comparison? Helps with readability, given the =.

  39. Show all issues

    Extra blank line here.

  40. Show all issues

    Can we break this down into the multi-line format? Any time we have typing in params.

    Same with others.

  41. reviewboard/static/rb/js/reviews/index.ts (Diff revision 6)
     
     
     
     
    Show all issues

    I don't think we need export {} anymore.

  42. reviewboard/static/rb/js/ui/views/menuView.es6.js (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Can we alphabetize these?

  43. reviewboard/templates/base.html (Diff revision 6)
     
     
    Show all issues

    actions should go after accounts, alphabetically.

  44. Show all issues

    Missing quotes around the {{action.get_dom_element_id}}.

  45. Show all issues

    No space needed before style.

  46. Show all issues

    Missing quotes around {{action.get_dom_element_id}}.

  47. 
      
david
chipx86
  1. The change looks great.

    I have a couple comments about documentation. I want to try to get our public docs a bit more fleshed out so it's easier to later track deprecations and usage and typing as we go forward.

  2. reviewboard/reviews/actions.py (Diff revisions 6 - 7)
     
     
     
    Show all issues

    Leftover debugging information.

  3. Show all issues

    We should keep to public before private.

    Also, it'd be nice to have docs for the public attributes.

  4. Show all issues

    Can we document this as well?

    And same with other public attributes.

  5. 
      
david
chipx86
  1. Ship It!
  2. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-6.x (819883b)