• 
      

    Refactor the actions system

    Review Request #9131 — Created Aug. 11, 2017 and discarded

    Information

    Review Board
    release-3.0.x

    Reviewers

    This is a large patch that refactors the action registry system
    implemented by a past student to use a proper registry. Much refactoring
    has been done:

    • Action base classes now live in reviewboard.actions. This package
      also includes header dropdown actions.
    • Review request-specific action classes now live in
      reviewboard.reviews.actions.
    • ALL action hooks support new-style action classes and old-style
      dicts. Previously, there was a discrepancy betweeen which hooks
      supported which type of action.
    • ActionHook is now deprecated in favour of ReviewRequestActionHook.
      The behaviour was the previously the same, except ActionHook supported
      old-style dicts instead of classes. Now that the behaviour of all
      hooks has been unified, we no longer need this distinction.
    • The {% child_actions %} templatetag has been moved from reviewtags
      to a new actions template tag library. It now takes the action it
      should list the child actions of.
    • The _DictAction and _DictMenuAction classes have been turned into
      the DictActionMixin.
    • There are now two action registries: one for review request actions
      and one for header actions. This allows us to keep them seperate.
    • We now store instances of dict actions instead of the dicts
      themselves. This is because you cannot store a dict in a set (as
      it is unhashable) and therefore cannot be put into a Registry.
    • Generic action templates now live in templates/actions.
    • Ran unit tests.
    • Built the docs and looked through them.

    Description From Last Updated

    W391 blank line at end of file

    reviewbotreviewbot

    W391 blank line at end of file

    reviewbotreviewbot

    F401 'reviewboard.actions.header.HeaderMenuAction' imported but unused

    reviewbotreviewbot

    F401 'reviewboard.actions.header.HeaderAction' imported but unused

    reviewbotreviewbot

    F841 local variable 'hook' is assigned to but never used

    reviewbotreviewbot

    F821 undefined name 'ReviewRequestAction'

    reviewbotreviewbot

    F821 undefined name 'ReviewRequestMenuAction'

    reviewbotreviewbot

    Should be changed to 4.0 now.

    chipx86chipx86

    Here, too.

    chipx86chipx86

    And here.

    chipx86chipx86

    = should be removed.

    chipx86chipx86

    Blank line between these.

    chipx86chipx86

    Capitalize the start of "subclasses."

    chipx86chipx86

    Only 1 blank line.

    chipx86chipx86

    Here, too.

    chipx86chipx86

    Here, too.

    chipx86chipx86

    Here, too.

    chipx86chipx86

    Missing docs.

    chipx86chipx86

    Alphabetical order.

    chipx86chipx86

    Missing docs.

    chipx86chipx86

    Missing docs.

    chipx86chipx86

    Missing a unicode_literals import.

    chipx86chipx86

    Missing docs.

    chipx86chipx86

    Missing the rest of the sentence. Also need one for image_height separately.

    chipx86chipx86

    Missing docs.

    chipx86chipx86

    Missing docs.

    chipx86chipx86

    Missing unicode_literals.

    chipx86chipx86

    Incomplete docstring.

    chipx86chipx86

    Missing period.

    chipx86chipx86

    Must be the full class path.

    chipx86chipx86

    Shouldn't use the "If ..." form. Instead, "The parent action is not registered."

    chipx86chipx86

    Same here.

    chipx86chipx86

    No trailing comma on the last parameter to a method.

    chipx86chipx86

    Missing docs.

    chipx86chipx86

    Leftover debugging.

    chipx86chipx86

    Missing docs.

    chipx86chipx86

    Must have the full class path.

    chipx86chipx86

    Just "The item was not foudn in the registry."

    chipx86chipx86

    Full class path.

    chipx86chipx86

    Full class path.

    chipx86chipx86

    Alignment issue.

    chipx86chipx86

    Same notes as above with the descriptions.

    chipx86chipx86

    Full class path.

    chipx86chipx86

    Description on the next line.

    chipx86chipx86

    We should have a module-level logger.

    chipx86chipx86

    Missing a file docstring.

    chipx86chipx86

    Missing a unicode_literals import.

    chipx86chipx86

    Needs a full class path.

    chipx86chipx86

    Description on the next line.

    chipx86chipx86

    Alphabetical order.

    chipx86chipx86

    Should be 4.0 now. And actually, this can now look like: """ ... Deprecated: 3.0: Blah blah """

    chipx86chipx86

    Missing docs.

    chipx86chipx86

    Should use str() instead of b'' for future Python compatibility.

    chipx86chipx86

    Needs the full class path.

    chipx86chipx86

    Same here.

    chipx86chipx86

    And here.

    chipx86chipx86

    And here.

    chipx86chipx86

    Double repr. We don't need the explicit call.

    chipx86chipx86

    No double underscore on the function. That will mangle the name. Needs docs.

    chipx86chipx86

    Needs a full class path.

    chipx86chipx86

    Missing docs.

    chipx86chipx86

    Should use str() instead of b''.

    chipx86chipx86

    Needs a full class path.

    chipx86chipx86

    Needs a full class path.

    chipx86chipx86

    Needs a full class path.

    chipx86chipx86

    These can be on the same line.

    chipx86chipx86

    No double underscore. Needs docs.

    chipx86chipx86

    Leftover debugging.

    chipx86chipx86

    Missing a test docstring.

    chipx86chipx86

    Missing a test docstring.

    chipx86chipx86

    Missing a test docstring.

    chipx86chipx86

    Missing a test docstring.

    chipx86chipx86

    Missing a test docstring.

    chipx86chipx86

    Missing a test docstring.

    chipx86chipx86

    Missing a test docstring.

    chipx86chipx86

    Missing a test docstring.

    chipx86chipx86

    Missing a test docstring.

    chipx86chipx86

    Swap these.

    chipx86chipx86

    Missing docs.

    chipx86chipx86

    Missing docs.

    chipx86chipx86

    Missing docs.

    chipx86chipx86

    Missing docs.

    chipx86chipx86

    Missing docs.

    chipx86chipx86

    Missing docs.

    chipx86chipx86

    Missing docs.

    chipx86chipx86

    Missing docs.

    chipx86chipx86

    Missing docs.

    chipx86chipx86

    No need for staticfiles or i18n.

    chipx86chipx86

    There should be a book for this.

    brenniebrennie

    E702 multiple statements on one line (semicolon)

    reviewbotreviewbot
    Checks run (1 failed, 1 succeeded)
    flake8 failed.
    JSHint passed.

    flake8

    brennie
    brennie
    Review request changed
    Change Summary:

    Rebase

    Commit:
    0ccb1bf8908fccba01c1cd83f801be0df14215da
    373472950257d0eee0be15cf79493d74a4137af3

    Checks run (1 failed, 1 succeeded)

    flake8 failed.
    JSHint passed.

    flake8

    brennie
    brennie
    brennie
    chipx86
    1. This is an old change at this point, and I'm in no way expecting you to finish this work. I just wanted to go through it and make suitable notes on what needs to be done before inclusion, so that we're at a good point to hand it off and continue work.

      Of course you're always more than welcome to finish the change, but that's not the priority right now :)

      1. This patch applied cleanly to e794a0945dcc1ed16d5bc2321c5eadc33a1374cc. It does not apply cleanly to 3.0.x

    2. Show all issues

      Should be changed to 4.0 now.

    3. Show all issues

      Here, too.

    4. Show all issues

      And here.

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

      = should be removed.

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

      Blank line between these.

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

      Capitalize the start of "subclasses."

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

      Only 1 blank line.

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

      Here, too.

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

      Here, too.

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

      Here, too.

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

      Missing docs.

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

      Alphabetical order.

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

      Missing docs.

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

      Missing docs.

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

      Missing a unicode_literals import.

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

      Missing docs.

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

      Missing the rest of the sentence.

      Also need one for image_height separately.

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

      Missing docs.

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

      Missing docs.

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

      Missing unicode_literals.

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

      Incomplete docstring.

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

      Missing period.

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

      Must be the full class path.

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

      Shouldn't use the "If ..." form. Instead, "The parent action is not registered."

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

      Same here.

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

      No trailing comma on the last parameter to a method.

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

      Missing docs.

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

      Leftover debugging.

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

      Missing docs.

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

      Must have the full class path.

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

      Just "The item was not foudn in the registry."

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

      Full class path.

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

      Full class path.

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

      Alignment issue.

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

      Same notes as above with the descriptions.

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

      Full class path.

    38. Show all issues

      Description on the next line.

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

      We should have a module-level logger.

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

      Missing a file docstring.

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

      Missing a unicode_literals import.

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

      Needs a full class path.

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

      Description on the next line.

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

      Alphabetical order.

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

      Should be 4.0 now.

      And actually, this can now look like:

      """
      ...
      
      Deprecated:
          3.0:
          Blah blah
      """
      
    46. reviewboard/extensions/hooks.py (Diff revision 5)
       
       
      Show all issues

      Missing docs.

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

      Should use str() instead of b'' for future Python compatibility.

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

      Needs the full class path.

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

      Same here.

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

      And here.

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

      And here.

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

      Double repr. We don't need the explicit call.

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

      No double underscore on the function. That will mangle the name.

      Needs docs.

      1. This was intentional.

      2. Or at least it was at some point. I recall there being multiple __from_dict_action methods but 🤷

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

      Needs a full class path.

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

      Missing docs.

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

      Should use str() instead of b''.

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

      Needs a full class path.

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

      Needs a full class path.

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

      Needs a full class path.

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

      These can be on the same line.

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

      No double underscore.

      Needs docs.

    62. Show all issues

      Leftover debugging.

    63. reviewboard/extensions/tests.py (Diff revision 5)
       
       
      Show all issues

      Missing a test docstring.

    64. reviewboard/extensions/tests.py (Diff revision 5)
       
       
      Show all issues

      Missing a test docstring.

    65. reviewboard/extensions/tests.py (Diff revision 5)
       
       
      Show all issues

      Missing a test docstring.

    66. reviewboard/extensions/tests.py (Diff revision 5)
       
       
      Show all issues

      Missing a test docstring.

    67. reviewboard/extensions/tests.py (Diff revision 5)
       
       
      Show all issues

      Missing a test docstring.

    68. reviewboard/extensions/tests.py (Diff revision 5)
       
       
      Show all issues

      Missing a test docstring.

    69. reviewboard/extensions/tests.py (Diff revision 5)
       
       
      Show all issues

      Missing a test docstring.

    70. reviewboard/extensions/tests.py (Diff revision 5)
       
       
      Show all issues

      Missing a test docstring.

    71. reviewboard/extensions/tests.py (Diff revision 5)
       
       
      Show all issues

      Missing a test docstring.

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

      Swap these.

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

      Missing docs.

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

      Missing docs.

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

      Missing docs.

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

      Missing docs.

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

      Missing docs.

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

      Missing docs.

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

      Missing docs.

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

      Missing docs.

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

      Missing docs.

    82. Show all issues

      No need for staticfiles or i18n.

    83. 
        
    brennie
    Review request changed
    Change Summary:

    This patch can now be applied to release-3.0.x to ease forward porting to 4.0.x.

    Commit:
    a10e69fd25ab2807d1b0b3d8f8e8dc7d151eade7
    cc0e50c126f85f58d819bb5f2a2f144a03b3f749

    Checks run (1 failed, 1 succeeded)

    flake8 failed.
    JSHint passed.

    flake8

    brennie
    1. 
        
    2. Show all issues

      There should be a book for this.

    3. 
        
    david
    Review request changed
    Status:
    Discarded