Feature-gate the new class-based actions system and shutdown hooks correctly

Review Request #9384 — Created Nov. 17, 2017 and submitted

brennie
Review Board
release-3.0.x
035a6a8...
reviewboard

Since https://reviews.reviewboard.org/r/9131/ did not make it into the
release window for RB 3.0, we cannot address the issues in the original
implementation of class-based actions. So we are gating them behind a
feature so they can be marked as experimental (and not to be used in
production). That way we will be able to fix up the issues in the
implementation for a future release.

This patch also addresses the issue of the new action hooks not
unregistering their child actions.

Documentation on action hooks has been reverted to how it was in
Review Board 2.5.

  • Unit tests all pass.
  • Wrote a test extension which (1) registered a hook that uses
    class-based actions and (2) registered a hook using dict-based
    actions. The former were not available (and was logged as such) and
    the latter were.
Description From Last Updated

E501 line too long (80 > 79 characters)

reviewbotreviewbot

I don't think we need the conditional here. In the only case where uses_experimental_actions is True, self.actions is set to …

daviddavid

I don't think "Class-Based" needs to be capitalized here.

daviddavid

This can remain action-hooks, since that anchor is valid for the "new" (as of this change) docs.

chipx86chipx86

This can also now be simplified into a single level: if (not class_based_actions_feature.is_enabled() and not all(isinstance(action, dict) for action in …

daviddavid

Let's also state that the API is expected to change in coming patch releases.

chipx86chipx86

Blank line between these.

chipx86chipx86

This should be toward the end of the class. If we also use self.__class__.__name__, we don't need to override this …

chipx86chipx86

E211 whitespace before '('

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

flake8

brennie
david
  1. 
      
  2. reviewboard/extensions/hooks.py (Diff revision 2)
     
     

    I don't think we need the conditional here. In the only case where uses_experimental_actions is True, self.actions is set to [] (and so calling _register_actions will be a no-op).

  3. reviewboard/reviews/features.py (Diff revision 2)
     
     

    I don't think "Class-Based" needs to be capitalized here.

  4. 
      
brennie
brennie
david
  1. 
      
  2. reviewboard/extensions/hooks.py (Diff revision 4)
     
     
     
     
     
     
     
     

    This can also now be simplified into a single level:

    if (not class_based_actions_feature.is_enabled() and
        not all(isinstance(action, dict) for action in actions)):
        logging.error(...)
        actions = []
    
  3. 
      
chipx86
  1. 
      
  2. docs/manual/extending/index.rst (Diff revision 4)
     
     

    This can remain action-hooks, since that anchor is valid for the "new" (as of this change) docs.

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

    Let's also state that the API is expected to change in coming patch releases.

  4. reviewboard/extensions/tests.py (Diff revision 4)
     
     
     

    Blank line between these.

  5. reviewboard/reviews/actions.py (Diff revision 4)
     
     
     

    This should be toward the end of the class.

    If we also use self.__class__.__name__, we don't need to override this for subclasses.

    1. Oops this was just for me while I was debugging what actions were getting double-released.

  6. 
      
brennie
Review request changed

Change Summary:

Addressed David & Christian's issues

Commit:

-d05366149910046d6e8edf62b0d0deae6d30c98e
+c7a91ce55c503451543de824e0b2d2f1a9bbf549

Diff:

Revision 5 (+235 -277)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
chipx86
  1. Ship It!
  2. 
      
david
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (7eaa381)
Loading...