Port existing actions over to the new framework.

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

david
Review Board
release-6.x
reviewboard

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
Port existing actions over to the new framework.
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. 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. Should there be 2 spaces here?

  4. Should there be 2 spaces here?

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

    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)
     
     
     
     

    This could all fit on one line I think.

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

    Add -> None:

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

    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)
     
     

    Missing a space between "reviewrequest".

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

    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)
     
     

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

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

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

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

    Could sort this alphabetically.

  14. 
      
david
david
david
chipx86
  1. 
      
  2. Typo in the description: "conistency" -> "consistency"

  3. 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)
     
     
     
     
     
     

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

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

    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)
     
     

    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)
     
     

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

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

    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. What's the TODO?

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

  10. Alignment issue.

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

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

  12. This seems unrelated.

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

    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)
     
     
     

    Same comment about perms here.

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

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

    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)
     
     

    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)
     
     

    Same comment about perms.

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

    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)
     
     
     

    Same here about perms.

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

    Same comment about perms.

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

    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)
     
     

    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)
     
     
     
     
     

    Can you put this in alphabetical order?

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

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

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

    Can you put this in alphabetical order?

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

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

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

    Missing a trailing period.

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

    Missing a trailing period.

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

    Missing a trailing period.

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

    djblets is mixed in with django.

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

    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. 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)
     
     
     
     

    These should be assertIsNone.

    Same with many below.

  34. 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)
     
     
     
     
     
     
     
     

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

    Right now we just get an implicit undefined.

  36. While here, can we alphabetize these?

  37. Can we add Version Added to each of these?

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

  39. Extra blank line here.

  40. 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)
     
     
     
     

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

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

    Can we alphabetize these?

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

    actions should go after accounts, alphabetically.

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

  45. No space needed before style.

  46. 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)
     
     
     

    Leftover debugging information.

  3. We should keep to public before private.

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

  4. Can we document this as well?

    And same with other public attributes.

  5. 
      
david
Review request changed
Loading...