Summary

Implement review request action registration.

Review Request #7661 — Created Sept. 26, 2015 and submitted May 15, 2016, 3:10 a.m.

Information

Owner

adriano

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard, students

People

Description

This is an architectural change for Review Board 2.6 that affects
extensions. Currently, there are some default review request actions
(like "Close -> Submitted" or "Download Diff" or "Ship It!") that have
been hardcoded into the template. Extensions can provide custom actions
by passing dictionaries into a variety of ActionHooks.

This change encapsulates this logic into a BaseReviewRequestAction
class. Instead of passing ActionHook-style dictionaries into the hooks,
extensions can now pass in BaseReviewRequestAction instances. Hardcoded
actions have been ripped out from the template and retrofitted into a
cleaner action registration system. Other new features include:

- Up to two levels of action nesting via BaseReviewRequestMenuActions.
- A should_render() method that subclasses of BaseReviewRequestAction
can override in order to implement complex action rendering behaviour.
- A register_actions() method that can be used to add a custom action to
any of the default menu actions.
- An unregister_actions() method that can be used to remove any of the
default actions.
- A ReviewRequestActionHookModel for providing action callbacks in JS.

Testing Done

- Ran and passed the relevant reviewboard unit tests.
- Manually verified that all default actions display correctly in
  different scenarios.
    1. Test in all three types of pages ("Reviews", "File", "Diff").
    2. Test in normal as well as local sites.
    3. Test DownloadDiffAction with the slider (for interdiffs).
    4. Test as an authenticated user and as a guest.
    5. Test when the review request is pending review and is submitted.
- Added my own extension that provides its own actions in different ways
  and manually verified that each action displayed correctly in
  different scenarios.
    1. Test adding actions and menu actions via action-style dicts.
    2. Test adding actions and menu actions via action instances.
    3. Test adding menu actions that contain their own menu actions.
    4. Test that a menu action can override its children from rendering.
    5. Test unregistering a default action.
    6. Test unregistering a child action of a default menu action.
    7. Test registering a new child action on a default menu action.
- Added unit tests that cover the above functionality, which all pass.
- Added my own JS extension that provides its own callback and manually
  verified that the callback is correctly triggered when its corresponding
  action is clicked.
- Built the Sphinx docs and manually verified that my updates rendered
  correctly.

Issues

Description	From	Last Updated
These shouldnt be on the class. They should be module-levels. See my other comment about the pages/forms stuff.	brennie	Sept. 27, 2015, 11:48 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Sept. 27, 2015, 11:47 p.m.
Ideally, the should_render function should be a member funciton. This way, the people using this code can override the function …	brennie	Sept. 27, 2015, 11:48 p.m.
These should probably all be class attributes.	brennie	Sept. 27, 2015, 11:48 p.m.
Re: my earlier comment, this would be class EditReviewAction(ReviewRequestAction): """...""" def should_render(self, request): return request.user.is_authenticated()	brennie	Sept. 27, 2015, 11:48 p.m.
You should look at reviewboard/accounts/pages.py and reviewboard/accounts/forms/pages.py to see how we register account pages and forms. We're going to want …	brennie	Sept. 30, 2015, 10:22 a.m.
Style nit: blank line between these.	brennie	Sept. 27, 2015, 11:49 p.m.
BLank line between these.	brennie	Sept. 27, 2015, 11:49 p.m.
This file should start with from __future__ import unicode_literals	david	Sept. 30, 2015, 3:19 a.m.
Docstrings should be in the imperative mood ("Return" vs "Returns")	david	Sept. 30, 2015, 3:19 a.m.
Imperative mood ("Populates" -> "Populate")	david	Sept. 30, 2015, 3:19 a.m.
Returns -> Return	david	Sept. 30, 2015, 3:19 a.m.
Registers -> Register	david	Sept. 30, 2015, 3:19 a.m.
This file should start with the unicode_literals import.	david	Sept. 30, 2015, 3:19 a.m.
This will need to be able to change based on what revision of the diff a user is looking at.	david	Sept. 30, 2015, 3:19 a.m.
Place the comment above this and prefix it with a colon, e.g.: #: A mapping of action IDs to action …	brennie	Sept. 30, 2015, 8:32 p.m.
Likewise here. However, this doesn't need a #: comment.	brennie	Sept. 30, 2015, 8:32 p.m.
Can you give this a list of arguments? e.g. """.... Args: arg (arg_type): Brief description of arg. """ This comment …	brennie	Sept. 30, 2015, 8:33 p.m.
:py:exc:KeyError	brennie	Sept. 30, 2015, 8:33 p.m.
Blank line between these.	brennie	Sept. 30, 2015, 8:33 p.m.
We prefer %-expressions for formatting over ''.format.	brennie	Sept. 30, 2015, 8:33 p.m.
You should have a constant on the class, e.g. class DownloadDiffAction(BaseReviewRequestAction): # ... #: Brief comment descrbing what this does. …	brennie	Sept. 30, 2015, 8:32 p.m.
Blank line between statement and block.	brennie	Sept. 30, 2015, 8:32 p.m.
This needs a better variable name. Perhaps, content?	brennie	Sept. 30, 2015, 8:32 p.m.
:py:class:`BaseReviewRequestAction`	brennie	Oct. 11, 2015, 1:47 a.m.
This should use reST clode blocks. E.g., For example: .. code-block:: python class UsedOnceAction(BaseReviewRequestAction): action_id = 'once' label = 'This …	brennie	Oct. 11, 2015, 1:47 a.m.
Blank line between these.	brennie	Oct. 11, 2015, 1:47 a.m.
This should go above args, e.g.: """Return whether or not this action should be rendered. The default implementation is to …	brennie	Oct. 11, 2015, 1:47 a.m.
Blank line between these.	brennie	Oct. 11, 2015, 1:47 a.m.
This should go above Args	brennie	Oct. 11, 2015, 1:47 a.m.
This should just be KeyError. The :py:exc:Exception stuff is for inline in strings, not in things that the docs expect …	brennie	Oct. 11, 2015, 1:47 a.m.
Needs a docstring. Likewise for all members of this class.	brennie	Oct. 11, 2015, 1:47 a.m.
Needs a docstring. Also, blank line between this and action_id. Likewise for encapsulated classes.	brennie	Oct. 11, 2015, 1:47 a.m.
This should be above args.	brennie	Oct. 11, 2015, 1:47 a.m.
No blank line between the docstring and the first line of code for functions.	david	Oct. 13, 2015, 11:05 p.m.
No blank line between the docstring and the first line of code for functions.	david	Oct. 13, 2015, 11:05 p.m.
Because you're manipulating this in a method, it probably shouldn't be data on the class. Can you create an __init__ …	david	Oct. 13, 2015, 11:05 p.m.
No blank line between the docstring and the first line of code for functions.	david	Oct. 13, 2015, 11:05 p.m.
No blank line between the docstring and the first line of code for functions.	david	Oct. 13, 2015, 11:05 p.m.
No blank line between the docstring and the first line of code for functions.	david	Oct. 13, 2015, 11:05 p.m.
No blank line between the docstring and the first line of code for functions.	david	Oct. 13, 2015, 11:06 p.m.
This comment confuses me.	david	Oct. 13, 2015, 11:06 p.m.
Can we call this review_request? We don't abbreviate to "rev" this way.	david	Oct. 13, 2015, 11:06 p.m.
You should be able to just use .display_id here.	david	Oct. 13, 2015, 11:06 p.m.
No blank line between docstring and code in functions.	david	Oct. 13, 2015, 11:06 p.m.
These two lines could be combined (you only use actions once)	david	Oct. 13, 2015, 11:06 p.m.
Hmm. I'm not sure how I feel about passing in a list and having the render() method append to it. …	david	Oct. 13, 2015, 11:06 p.m.
Let's put the whole if/endif thing on the same line.	david	Oct. 13, 2015, 11:06 p.m.
Add another blank line after this.	david	Oct. 23, 2015, 10:29 p.m.
I'm not sure why all of these are class methods. Can you clarify for me?	david	Oct. 23, 2015, 10:29 p.m.
Can we just pull these out into top-level classes? There's no real reason to hide them inside their parent menus, …	david	Oct. 23, 2015, 10:29 p.m.
"children_actions" is kind of redundant. How about either "children" or "child_actions"?	david	Oct. 23, 2015, 10:29 p.m.
Same with these (pull them out into top-level classes).	david	Oct. 23, 2015, 10:29 p.m.
This is probably not a terribly reliable way to detect this. It might be a lot better to pass the …	david	Oct. 23, 2015, 10:29 p.m.
We should maybe also pass the request into this, and introspect the url/view name, instead of trying to reverse engineer …	david	Oct. 23, 2015, 10:29 p.m.
Can we wrap this somehow? Maybe split the line after the href="..."?	david	Oct. 23, 2015, 10:29 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Oct. 24, 2015, 9:32 a.m.
Yes, it should be. __all__ defines which symbols get exported from the file (if there's no __all__, everything is exported).	david	Oct. 30, 2015, 8:18 p.m.
_() doesn't work this way. The l10n scanner looks at the source code and extracts all string literals which are …	david	Oct. 30, 2015, 8:18 p.m.
Same here.	david	Oct. 30, 2015, 8:19 p.m.
I would wrap this differently: super(DiffViewerActionHook, self).__init__( extension, apply_to, actions, args, *kwargs)	david	Oct. 30, 2015, 8:19 p.m.
Add a blank line after this.	david	Oct. 30, 2015, 8:19 p.m.
Because we're intending to return a bool (and not a Repository), we should make this a truth test: return review_request.repository …	david	Oct. 30, 2015, 8:19 p.m.
Instead of using 'P' as a literal, you can compare this against ReviewRequest.PENDING_REVIEW.	david	Oct. 30, 2015, 8:19 p.m.
'_' imported but unused	reviewbot	Nov. 7, 2015, 11:05 p.m.
Mind fixing the spelling here while you're editing this file?	david	Nov. 7, 2015, 11:05 p.m.
It doesn't really matter, but I think it would be nicer to have this set after we register all the …	david	Nov. 14, 2015, 10:51 p.m.
Instead of just casting to bool, I think we should explicitly test review_request.repository is not None.	david	Nov. 7, 2015, 11:05 p.m.
This can be rewritten using logging.exception, which we're slowly transitioning to: logging.exception('Error rendering top-level action: %s', top_level_action.action_id)	david	Nov. 7, 2015, 11:05 p.m.
Same here.	david	Nov. 7, 2015, 11:05 p.m.
URL.	brennie	Nov. 18, 2015, 1:34 a.m.
JavaScript, ``'#'``	brennie	Nov. 18, 2015, 1:34 a.m.
``javascript:`` URL	brennie	Nov. 18, 2015, 1:34 a.m.
2.6 is feature frozen. Can you change this to 2.7?	brennie	Nov. 18, 2015, 1:34 a.m.
The right side can be reformatted to fit more on the first line.	brennie	Nov. 18, 2015, 1:34 a.m.
Again, 2.7.	brennie	Nov. 18, 2015, 1:34 a.m.
2.7	brennie	Nov. 18, 2015, 1:34 a.m.
The from should be lined up with code.	brennie	Nov. 18, 2015, 1:34 a.m.
Please format this as: from djblets.extensions.hooks import (AppliesToURLMixin, DataGridColumnsHook, ExtensionHook, ExtensionHookPoint, SignalHook, TemplateHook, URLHook)	brennie	Nov. 18, 2015, 1:34 a.m.
Format this as: super(BaseReviewRequestActionHook, self).__init__( extension, apply_to=apply_to, args, *kwargs)	brennie	Nov. 18, 2015, 1:34 a.m.
Needs Args and Returns, e.g. """Single line summary. Multi-line description. Args: actions (list): A brief description of what actions is. …	brennie	Nov. 18, 2015, 1:34 a.m.
Blank line between these.	brennie	Nov. 18, 2015, 1:34 a.m.
So registered_actions is a temporary that is only used in this function, but we're creating a iterator out of it …	brennie	Nov. 18, 2015, 1:35 a.m.
Needs Args and Returns.	brennie	Nov. 18, 2015, 1:35 a.m.
This should be a class member, or a global member. I'm not sure. But it definitely shouldn't be in this …	brennie	Nov. 18, 2015, 1:35 a.m.
Needs a better docstring to explain why this is necessary.	brennie	Nov. 18, 2015, 1:35 a.m.
Should be done in __init__.	brennie	Nov. 18, 2015, 1:35 a.m.
Please format as: super(ReviewRequestActionHook, self).__init__( extension, apply_to, actions, args, *kwargs)	brennie	Nov. 18, 2015, 1:35 a.m.
Likewise, I don't think this should be in the def.	brennie	Nov. 18, 2015, 1:35 a.m.
Format as: return TempMenuAction([ super(ReviewRequestDropdownActionHook, self)._to_action_instance( child_action_dict) for child_action_dict in action_dict['items'] ])	brennie	Nov. 18, 2015, 1:35 a.m.
Alphabetize these imports.	brennie	Nov. 18, 2015, 1:35 a.m.
Put this on the previous line.	brennie	Nov. 18, 2015, 1:35 a.m.
Put this on the previous line.	brennie	Nov. 18, 2015, 1:35 a.m.
Put this on the previous line.	brennie	Nov. 18, 2015, 1:35 a.m.
Put this on the previous line.	brennie	Nov. 18, 2015, 1:35 a.m.
Put this on the previous line.	brennie	Nov. 18, 2015, 1:35 a.m.
Put this on the previous line.	brennie	Nov. 18, 2015, 1:35 a.m.
Needs a docstring.	brennie	Nov. 18, 2015, 1:35 a.m.
Undo this.	brennie	Nov. 18, 2015, 1:36 a.m.
Undo this.	brennie	Nov. 18, 2015, 1:36 a.m.
Undo this.	brennie	Nov. 18, 2015, 1:36 a.m.
The text should line up with the n in note.	brennie	Nov. 18, 2015, 1:36 a.m.
Can you format this as: parent (BaseReviewRequestMenuAction): The parent action instance of this action instance (optional).	brennie	Nov. 18, 2015, 1:36 a.m.
Text should align with n in note.	brennie	Nov. 18, 2015, 1:36 a.m.
Put on the previous line.	brennie	Nov. 18, 2015, 1:36 a.m.
Should be a complete sentence.	brennie	Nov. 18, 2015, 1:36 a.m.
Docstring should mention that this is a generator.	brennie	Nov. 18, 2015, 1:36 a.m.
""" Yields: BaseReviewREquestActions: All top-level registered review request action instances. """	brennie	Nov. 18, 2015, 1:36 a.m.
Format as: """ KeyError: ... """	brennie	Nov. 18, 2015, 1:36 a.m.
Should be complete sentence.	brennie	Nov. 18, 2015, 1:36 a.m.
"Determines" over "Tracks"	brennie	Nov. 18, 2015, 1:37 a.m.
Put this on the previous line please.	brennie	Nov. 18, 2015, 1:37 a.m.
"otherwise" instead of "else".	brennie	Nov. 18, 2015, 1:37 a.m.
Needs Returns.	brennie	Nov. 18, 2015, 1:37 a.m.
I'd rather not use a table for this (and require the shorthand for the actions). A bullet point list is …	chipx86	Nov. 21, 2015, 1:23 p.m.
The description shouldn't be on the same line as the version number. Look around the docs codebase and you'll find …	chipx86	Nov. 21, 2015, 1:23 p.m.
Two blank lines between sections. Same below.	chipx86	Nov. 21, 2015, 1:23 p.m.
What's this for? Ideally, all named lists of URLs should be defined in exactly one place, reviewboard.urls, so depending on …	chipx86	Nov. 21, 2015, 1:23 p.m.
This should spend some time describing what actions are and roughly how they work, like the old docs did.	chipx86	Nov. 25, 2015, 5:17 a.m.
I'd prefer not to align like this. Instead: * ``id``: The ID of the action (optional). * ``label``: ... (Make …	chipx86	Nov. 25, 2015, 5:17 a.m.
Blank line between these.	chipx86	Nov. 21, 2015, 1:23 p.m.
This needs a docstring.	chipx86	Nov. 21, 2015, 1:24 p.m.
This definitely need more docs. I don't have a good sense of what this does.	chipx86	Nov. 25, 2015, 5:17 a.m.
This is assuming some things about the data passed in, and if the keys are missing, the caller's going to …	chipx86	Nov. 25, 2015, 5:18 a.m.
This looks as if the self.label... is a third parameter. I'd instead do: self.action_id = action_dict.get( 'id', ...)	chipx86	Nov. 25, 2015, 5:18 a.m.
This should also be fleshed out more.	chipx86	Nov. 25, 2015, 5:18 a.m.
Same stuff as above.	chipx86	Nov. 25, 2015, 5:18 a.m.
These are best referenced using :guilabel:.	chipx86	Nov. 21, 2015, 1:24 p.m.
Maybe list these, so a caller doesn't have to look through the class hierarchy and check parent classes.	chipx86	Nov. 25, 2015, 5:19 a.m.
Needs a docstring.	chipx86	Nov. 21, 2015, 1:25 p.m.
Instead of setting these, just pass them inline where needed.	chipx86	Nov. 21, 2015, 1:25 p.m.
This is kind of named awkwardly. From the name, I have no real sense of what it's doing, and the …	chipx86	Nov. 25, 2015, 5:19 a.m.
:py:class: for dict.	chipx86	Nov. 21, 2015, 1:25 p.m.
BaseReviewRequestAction	chipx86	Nov. 21, 2015, 1:25 p.m.
I notice we're reversing here, and then reversing again later. What's the reason for that?	chipx86	Nov. 25, 2015, 5:19 a.m.
What other values can this be? It might want to sanity-check that everything provided is an expected type of value, …	chipx86	Nov. 25, 2015, 5:19 a.m.
Blank line between these.	chipx86	Nov. 25, 2015, 5:19 a.m.
A function beginning with to_... generally means that the owning class would be converted to a thing. Let's go with …	chipx86	Nov. 21, 2015, 1:25 p.m.
Let's flesh this out a bit, mention that this means it'll only show up on the review request page containing …	chipx86	Nov. 25, 2015, 5:19 a.m.
Needs a docstring.	chipx86	Nov. 25, 2015, 5:19 a.m.
Same above. We should list the possible types.	chipx86	Nov. 25, 2015, 5:20 a.m.
Same as above with the formatting.	chipx86	Nov. 25, 2015, 5:20 a.m.
This should go in an Example section, like: """ ... Example: .. code-block:: python actions = [{ ... }]	chipx86	Nov. 25, 2015, 5:20 a.m.
Same naming thoughts as above. Also, needs docstrings.	chipx86	Nov. 25, 2015, 5:20 a.m.
Same comments as above.	chipx86	Nov. 25, 2015, 5:20 a.m.
This is one of the older unit test suites in the codebase. Let's go ahead and move all action unit …	chipx86	Nov. 25, 2015, 5:20 a.m.
I know the old unit test names didn't do this, but ideally, all new unit test names should reference the …	chipx86	Nov. 25, 2015, 5:20 a.m.
These are nice cleanups, but really should be broken out into its own change, to help keep this change slimmer. …	chipx86	Nov. 21, 2015, 1:27 p.m.
This should go in an "Example:" section, like: """ ... Example: .. code-block:: python: class .... """ We've been placing …	chipx86	Nov. 25, 2015, 5:20 a.m.
These should be documented inline where defined on the class. "Attributes:" is more useful when documenting things only set in …	chipx86	Nov. 25, 2015, 5:21 a.m.
Private attributes shouldn't be documented. Same below.	chipx86	Nov. 21, 2015, 1:27 p.m.
If a subclass is meant to override this, it should not be a private function.	chipx86	Nov. 21, 2015, 1:27 p.m.
Should be in a "Note:" section instead.	chipx86	Nov. 25, 2015, 5:21 a.m.
Should be indented one level.	chipx86	Nov. 25, 2015, 5:21 a.m.
Same here.	chipx86	Nov. 25, 2015, 5:21 a.m.
You can combine these to be optimistic: try: del _all_actions[...] except KeyError: raise KeyError(...)	chipx86	Nov. 25, 2015, 5:21 a.m.
Not worth repeating these. Same below.	chipx86	Nov. 25, 2015, 5:21 a.m.
Needs a docstring.	chipx86	Nov. 25, 2015, 5:21 a.m.
Blank line between these, generally, but this might be better as: try: parent = _all_actions[parent_id] except KeyError: raise KeyError(...)	chipx86	Nov. 25, 2015, 5:21 a.m.
Same as above.	chipx86	Nov. 25, 2015, 5:21 a.m.
Let's flesh this out just a bit, describing the resulting state. May also need a warning that this will impact …	chipx86	Nov. 25, 2015, 5:21 a.m.
Blank line before blocks.	chipx86	Nov. 25, 2015, 5:21 a.m.
Blank line between these. Though, you might want to just combine the statements.	chipx86	Nov. 25, 2015, 5:21 a.m.
Let's have the third condition on its own line.	chipx86	Nov. 25, 2015, 5:21 a.m.
So I have a concern here, and I want to follow up on how this works. We're registering instances, right? …	chipx86	Nov. 28, 2015, 9:37 p.m.
Blank line between these.	chipx86	Nov. 25, 2015, 5:21 a.m.
Here too. Though, you may want to just combine the statements.	chipx86	Nov. 25, 2015, 5:22 a.m.
Parens around the comparison.	chipx86	Nov. 25, 2015, 5:22 a.m.
Description on the next line.	chipx86	Nov. 24, 2015, 9:53 p.m.
This might need to provide an example.	chipx86	Nov. 25, 2015, 5:22 a.m.
This should be documented.	chipx86	Nov. 28, 2015, 9:37 p.m.
If we're changing the names, let's get rid of -link and replace it with -action.	chipx86	Nov. 21, 2015, 1:29 p.m.
No space after function.	chipx86	Nov. 25, 2015, 5:22 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Nov. 25, 2015, 5:23 a.m.
Col: 17 W503 line break before binary operator	reviewbot	Nov. 28, 2015, 9:37 p.m.
'register_actions' imported but unused	reviewbot	Nov. 28, 2015, 9:44 p.m.
'unregister_actions' imported but unused	reviewbot	Nov. 28, 2015, 9:44 p.m.
A form we're moving to, which reads and maintains better, is: ``id`` (optional): The ID of... ``label``: ...	chipx86	Feb. 12, 2016, 5 p.m.
Only two backticks are needed.	chipx86	Feb. 12, 2016, 5 p.m.
Any place we're referencing some key, we should use double backticks instead of making it bold.	chipx86	Feb. 12, 2016, 5 p.m.
Not sure that we should document registering without a hook in an extension, since that means more that the extension …	chipx86	Feb. 12, 2016, 5 p.m.
We should document that this works on all review request pages.	chipx86	Feb. 12, 2016, 5 p.m.
This is probably not something we want to explicitly document here. Instead, we should somehow save and restore the actions …	chipx86	Feb. 12, 2016, 5 p.m.
Like with the docs, let's do: ``id`` (optional): ... ``label``: ...	chipx86	Feb. 12, 2016, 5:01 p.m.
Can you add ", optional" to the end of this, inside the parens? Same with any other arguments you have …	chipx86	Feb. 12, 2016, 5:01 p.m.
Technically, this is a tuple. Same below.	chipx86	Feb. 12, 2016, 5:01 p.m.
The type should be "callable".	chipx86	Feb. 12, 2016, 5:01 p.m.
It's not generally good for classes to override functions like this. Let's define a proper method instead. Same below.	chipx86	Feb. 12, 2016, 5:01 p.m.
I'm actually not sure, what is #. ?	chipx86	Feb. 12, 2016, 5:01 p.m.
Should have ", optional".	chipx86	Feb. 12, 2016, 5:01 p.m.
"Raises" should go after "Returns". Same below.	chipx86	Feb. 12, 2016, 5:01 p.m.
We should use :py:class: here. Same below.	chipx86	Feb. 12, 2016, 5:01 p.m.
No indentation for the description here. Same below.	chipx86	Feb. 12, 2016, 5:01 p.m.
Maybe add a comment saying why we do this in reverse order.	chipx86	Feb. 12, 2016, 5:01 p.m.
Can return reversed(registered_actions) instead.	chipx86	Feb. 12, 2016, 5:01 p.m.
Should have :py:class on the class names in docstrings. Same elsewhere.	chipx86	Feb. 12, 2016, 5:02 p.m.
Should have ", optional"	chipx86	Feb. 12, 2016, 5:02 p.m.
Same as above with the formatting.	chipx86	Feb. 12, 2016, 5:02 p.m.
No period on unit test docstrings. Same below.	chipx86	Feb. 12, 2016, 5:02 p.m.
Let's specify the base class name.	chipx86	Feb. 12, 2016, 5:02 p.m.
Let's specify the class name.	chipx86	Feb. 12, 2016, 5:02 p.m.
I think "height" might actually be a bit too confusing. "Depth" worked better. "Height" usually means the dimensional height of …	chipx86	Feb. 12, 2016, 5:02 p.m.
Maybe "as HTML." ?	chipx86	Feb. 12, 2016, 5:02 p.m.
Should have ", optional". Then there's no need for "(optional)" at the end.	chipx86	Feb. 12, 2016, 5:02 p.m.
Should wrap this in a try:, in case there's a failure, and log in except:. Then we can pop in …	chipx86	Feb. 12, 2016, 5:02 p.m.
No need for the inner parens.	chipx86	Feb. 12, 2016, 5:02 p.m.
% should go on the next line. Not sure RuntimeError is right here. Is this something that should never happen, …	chipx86	Feb. 12, 2016, 5:03 p.m.
No need for the inner parens.	chipx86	Feb. 12, 2016, 5:03 p.m.
", optional"	chipx86	Feb. 12, 2016, 5:03 p.m.
"as HTML." Same with any others.	chipx86	Feb. 12, 2016, 5:03 p.m.
", optional"	chipx86	Feb. 12, 2016, 5:03 p.m.
This comment isn't about this line per se, but we have this nifty new Registry thing that Barret put together …	chipx86	Feb. 12, 2016, 5:03 p.m.
", optional"	chipx86	Feb. 12, 2016, 5:03 p.m.
No need for inner parens.	chipx86	Feb. 12, 2016, 5:03 p.m.
Blank line between these.	chipx86	Feb. 12, 2016, 5:03 p.m.
As mentioned earlier in the review, this is kind of dangerous for extensions, because they can stomp all over each …	chipx86	Feb. 12, 2016, 5:03 p.m.
We can now use @register.simple_tag(takes_context=True). @basictag is newly-deprecated. Same below.	chipx86	Feb. 12, 2016, 5:03 p.m.
Blank line between these. Same with ones below.	chipx86	Feb. 12, 2016, 5:03 p.m.
We should probably call this in tearDown().	chipx86	Feb. 12, 2016, 5:03 p.m.
We have a new way of handling the documentation of model attributes. See https://www.reviewboard.org/docs/codebase/dev/docs/writing-codebase-docs/#documenting-models	chipx86	Feb. 12, 2016, 5:03 p.m.
Instead of a list (since this is "only" one thing), maybe call this main_review_request_url_name?	chipx86	Feb. 12, 2016, 5:04 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/builtin_actions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/reviews/review_request_actions_primary.html



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/builtin_actions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/reviews/review_request_actions_primary.html

reviewboard/reviews/actions.py (Diff revision 1)

Show all issues

Col: 80
 E501 line too long (80 > 79 characters)

AD

adriano Sept. 26, 2015, 3 p.m.
```
So...the line limit is 79, not 80?
```

mike_conley Sept. 27, 2015, 8:57 a.m.

According to PEP8, that is correct: http://legacy.python.org/dev/peps/pep-0008/#maximum-line-length

AD

adriano Sept. 27, 2015, 9:55 p.m.
```
Fair enough, thanks.
```

reviewboard/reviews/actions.py (Diff revision 1)

From what I can tell, the only difference between primary and secondary actions was their positions in the list (where primary were always put after secondary, so they're closer to the right).

adriano Sept. 27, 2015, 9:55 p.m.

Thanks for the clarification. From what you said, it seems that I don't really need to worry about this. Extensions should probably be able to reorder actions as desired, instead of grouping them into primary or secondary.

reviewboard/reviews/actions.py (Diff revision 1)

So I see what you're doing here with these class methods, and it makes sense, but I don't think it's consistent with the way the rest of RB's extension framework works - specifically, the Fields API, which I believe this new Actions API is supposed to expose.

Here are some Fields defined in the MozReview extension, for example: https://hg.mozilla.org/hgcustom/version-control-tools/file/d1e664f1dc8d/pylib/mozreview/mozreview/fields.py

And here are the built-in fields being manipulated: https://hg.mozilla.org/hgcustom/version-control-tools/file/835dbd9444db/pylib/mozreview/mozreview/extension.py#l140

So I suspect what we want to do is expose some functions in actions.py that can be used to manipulate the current actions, and an equivalent to ReviewRequestFieldsHook (but for Actions) that makes it easy (and backwards compatible for ReviewRequestActionsHook... I'll just call it ReviewRequestActionsHook2 for now to differentiate, but that shouldn't be its name) to inject new Actions.

So the final result would be that, as an extension developer, I subclass something like a ReviewRequestBaseAction, and override something like a "should_render" method for saying whether or not the Action should apply under the current conditions.

And then I would instantiate the new ReviewRequestActionsHook2, passing in the subclass, along with the URL name that the hook should apply to.

Does that make sense?

adriano Sept. 27, 2015, 10:15 p.m.

Thanks, that makes a lot of sense. I originally had copied over the same structure used in the fields.py file, but as an experiment I wanted to see what would happen if I "brought everything one level down". Surprisingly, it sort of worked.
I understand that consistency is important (my experiment will be easy enough to revert), but I am a bit curious as to why the code is slightly more complicated than it perhaps needs to be. If we create an EditReviewAction class that subclasses some BaseReviewRequestAction (instead of instantiating an edit_review_action object from a ReviewRequestAction class), then does this mean that we could instantiate multiple objects from a single EditReviewAction class? To me, it seems that there is a one-to-one correspondence between an action class and an action instance (the lack of instance-level attributes confuses me).

reviewboard/reviews/actions.py (Diff revision 1)

Show all issues

These shouldnt be on the class. They should be module-levels. See my other comment about the pages/forms stuff.

reviewboard/reviews/actions.py (Diff revision 1)

Show all issues

Ideally, the should_render function should be a member funciton. This way, the people using this code can override the function (which by default should probably return True) to do complicated stuff with, e.g., the request.

reviewboard/reviews/actions.py (Diff revision 1)

Show all issues

These should probably all be class attributes.

reviewboard/reviews/builtin_actions.py (Diff revision 1)

Show all issues

Re: my earlier comment, this would be

class EditReviewAction(ReviewRequestAction):
    """..."""

    def should_render(self, request):
        return request.user.is_authenticated()

adriano Sept. 27, 2015, 10:56 p.m.

Thanks. As I mentioned in a reply to Mike, I originally had copied over the same structure used in the fields.py file, but as an experiment I wanted to see what would happen if I "brought everything one level down". Surprisingly, it sort of worked. At any rate, I'm going to go ahead and revert my experiment.
I am a little bit confused as to when we should ever put things at the instance level (instead of the module or class level). If we make an EditReviewAction class that subclasses some BaseReviewRequestAction (instead of instantiating an edit_review_action object from a ReviewRequestAction class), then does this mean that we could instantiate multiple objects from a single EditReviewAction class? To me, it seems that there is a one-to-one correspondence between an action class and an action instance (the lack of instance-level attributes confuses me).
In particular, should this should_render method actually be a class method (and can it access the full context, since this is needed by the Download Diff action)? Something like:

class EditReviewAction(BaseReviewRequestAction):
    """..."""

    @classmethod
    def should_render(cls, context):
        return context['request'].user.is_authenticated()

brennie Sept. 30, 2015, 10:18 a.m.

You may want to ping Christian about whether or not these should be @classmethods or instance methods.

reviewboard/reviews/builtin_actions.py (Diff revision 1)

Show all issues

You should look at reviewboard/accounts/pages.py and reviewboard/accounts/forms/pages.py to see how we register account pages and forms. We're going to want a similar system to that that. Feel free to ping me in slack if you have questions about it.

brennie Sept. 30, 2015, 10:18 a.m.

Please don't mark issues as fixed that haven't been; drop them and leave a reply instead.

brennie Sept. 30, 2015, 10:23 a.m.

Please disregard that last comment :)

I saw the same code and left that before I finished revieweing. Sorry!

reviewboard/reviews/templatetags/reviewtags.py (Diff revision 1)
Show all issues
```
Style nit: blank line between these.
```
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 1)
Show all issues
```
BLank line between these.
```

Change Summary:

Address the issues raised by Review Bot, Mike, and Barret.

Upgraded attributes from (instance, class) to (class, module) level.
Made the url attribute default to be '#'.
Renamed all "shipit-link" strings to "ship-it-link".

Description:

		Part 1: Update the simplest hardcoded actions in the "Reviews" tab.

~		Added the `ReviewRequestAction` class (see: `ReviewRequestFields`).
	~	Added the `BaseReviewRequestAction` class (see: `BaseReviewRequestField`).
		Added the `for_review_request_action` tag (see: `for_review_request_fieldset`).
		Updated the "Review", "Ship It!", and "Download Diff" actions.

		TODO:

		Update the dropdown hardcoded actions.
		Update the actions in the "Diff" tab.
		Make sure that the actions hooks are compatible.

Diff:

Revision 2 (+140 -7)

Show changes

	reviewboard/reviews/actions.py
	reviewboard/reviews/builtin_actions.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	reviewboard/templates/diffviewer/view_diff.html
	reviewboard/templates/reviews/review_detail.html
	reviewboard/templates/reviews/review_request_actions_primary.html

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/builtin_actions.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/builtin_actions.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

reviewboard/reviews/actions.py (Diff revision 2)

Show all issues

This file should start with from __future__ import unicode_literals

reviewboard/reviews/actions.py (Diff revision 2)

Show all issues

Docstrings should be in the imperative mood ("Return" vs "Returns")

adriano Sept. 29, 2015, 10:22 p.m.

Thanks, I didn't know that. Many of the docstrings in this actions.py file were taken directly from the fields.py file, which doesn't quite use the imperative mood. Should I update the docstrings in the fields.py file as well to be consistent?

Also, I added a new for_review_request_action method in templatetags/reviewtags.py whose docstring starts with "Loops through all review request actions." Like the fields.py file, the docstrings in templatetags/reviewtags.py also don't use the imperative mood. Should these docstrings be updated as well?

reviewboard/reviews/actions.py (Diff revision 2)
Show all issues
```
Imperative mood ("Populates" -> "Populate")
```
reviewboard/reviews/actions.py (Diff revision 2)
Show all issues
```
Returns -> Return
```
reviewboard/reviews/actions.py (Diff revision 2)
Show all issues
```
Registers -> Register
```
reviewboard/reviews/builtin_actions.py (Diff revision 2)
Show all issues
```
This file should start with the unicode_literals import.
```

reviewboard/reviews/builtin_actions.py (Diff revision 2)

Show all issues

This will need to be able to change based on what revision of the diff a user is looking at.

Change Summary:

Address the issues raised by David.

Imported unicode_literals for each new Python file.
Corrected the docstrings to use the imperative mood.
Shortened the names of some of the default actions.
Added the hidden class variable for DownloadDiffAction.
Modified view_diff.html so that simple actions now work for both tabs.

Description:

		Part 1: Update the simplest hardcoded actions in the "Reviews" tab.

		Added the `BaseReviewRequestAction` class (see: `BaseReviewRequestField`).
		Added the `for_review_request_action` tag (see: `for_review_request_fieldset`).
		Updated the "Review", "Ship It!", and "Download Diff" actions.

	+
	+
	+	Part 2: Update the simplest hardcoded actions in the "Diffs" tab.
	+
	+	Added the `hidden` class variable for `DownloadDiffAction`.
	+	Modified `view_diff.html` so that simple actions now work for both tabs.
	+
	+
	+
		TODO:

~		Update the dropdown hardcoded actions.
~		Update the actions in the "Diff" tab.
	~	Update the dropdown hardcoded actions in the Reviews tab.
	~	Update the dropdown hardcoded actions in the Diff tab.
		Make sure that the actions hooks are compatible.

Diff:

Revision 3 (+175 -10)

Show changes

	reviewboard/reviews/actions.py
	reviewboard/reviews/builtin_actions.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	reviewboard/templates/diffviewer/view_diff.html
	reviewboard/templates/reviews/review_detail.html
	1 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/builtin_actions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/builtin_actions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/reviews/builtin_actions.py (Diff revision 3)

Show all issues

You should have a constant on the class, e.g.

class DownloadDiffAction(BaseReviewRequestAction):
    # ...

    #: Brief comment descrbing what this does.
    URL_RE = re.compile(r'...')

    # ...

reviewboard/reviews/builtin_actions.py (Diff revision 3)
Show all issues
```
Blank line between statement and block.
```
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 3)
Show all issues
```
This needs a better variable name. Perhaps, content?
```

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

Show all issues

Place the comment above this and prefix it with a colon, e.g.:

#: A mapping of action IDs to action classes.
_action_classes = SortedDict()

That way, this member will end up in auto-generated documentation.

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

Show all issues

Likewise here. However, this doesn't need a #: comment.

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

Show all issues

Can you give this a list of arguments?

e.g.

"""....


Args:
    arg (arg_type):
        Brief description of arg.
"""

This comment applies to all functions that take arguments.
That way they will show up in autogenerated documentation.

reviewboard/reviews/actions.py (Diff revision 3)
Show all issues
```
:py:exc:KeyError
```
reviewboard/reviews/actions.py (Diff revision 3)
Show all issues
```
Blank line between these.
```

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

Show all issues

We prefer %-expressions for formatting over ''.format.

Change Summary:

Address the issues raised by Barret.

Switched to %-expressions over ''.format().
Improved the docstrings (following a Google-ish standard).
Added special #: type comments for auto-generated documentation.
Switched from registering actions classes to action instances.

Description:

~		Part 1: Update the simplest hardcoded actions in the "Reviews" tab.
	~	Part 1: Update the simplest hardcoded actions in the Reviews tab.

		Added the `BaseReviewRequestAction` class (see: `BaseReviewRequestField`).
		Added the `for_review_request_action` tag (see: `for_review_request_fieldset`).
		Updated the "Review", "Ship It!", and "Download Diff" actions.



~		Part 2: Update the simplest hardcoded actions in the "Diffs" tab.
	~	Part 2: Update the simplest hardcoded actions in the Diffs tab.

		Added the `hidden` class variable for `DownloadDiffAction`.
		Modified `view_diff.html` so that simple actions now work for both tabs.



		TODO:

~		Update the dropdown hardcoded actions in the Reviews tab.
~		Update the dropdown hardcoded actions in the Diff tab.
~		Make sure that the actions hooks are compatible.
	~	Update the dropdown hardcoded actions in both tabs.
	~	Make sure that the actions hooks are compatible.
	~	Start looking at the `RB.ReviewRequestEditorView` JavaScript class.

Diff:

Revision 4 (+230 -10)

Show changes

	reviewboard/reviews/actions.py
	reviewboard/reviews/builtin_actions.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	reviewboard/templates/diffviewer/view_diff.html
	reviewboard/templates/reviews/review_detail.html
	1 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/builtin_actions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/builtin_actions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Change Summary:

Update the default menu actions.

Changed from a block tag to a basic tag (review_request_actions).
Actions are now responsible for rendering via context.
Added a menu_action.html file that includes action.html.

Description:

		Part 1: Update the simplest hardcoded actions in the Reviews tab.

		Added the `BaseReviewRequestAction` class (see: `BaseReviewRequestField`).
		Added the `for_review_request_action` tag (see: `for_review_request_fieldset`).
		Updated the "Review", "Ship It!", and "Download Diff" actions.



~		Part 2: Update the simplest hardcoded actions in the Diffs tab.
	~	Part 2: Update the simplest hardcoded actions in the Diff tab.

		Added the `hidden` class variable for `DownloadDiffAction`.
		Modified `view_diff.html` so that simple actions now work for both tabs.



	+	Part 3: Update the default menu actions.
	+
	+	Changed from a block tag to a basic tag (`review_request_actions`).
	+	Actions are now responsible for rendering via `context`.
	+	Added a `menu_action.html` file that includes `action.html`.
	+
	+
	+
		TODO:

~		Update the dropdown hardcoded actions in both tabs.
	~	Make sure that extensions can unregister default actions.
		Make sure that the actions hooks are compatible.
		Start looking at the `RB.ReviewRequestEditorView` JavaScript class.

Testing Done:

		Ran and passed the relevant reviewboard unit tests.
~		Manually verified that the "Review", "Ship It!", and "Download Diff" actions display correctly.
	~	Manually verified that all default actions display correctly in different scenarios.

Diff:

Revision 5 (+355 -55)

Show changes

	reviewboard/reviews/actions.py
	reviewboard/reviews/builtin_actions.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	reviewboard/static/rb/js/views/reviewRequestEditorView.js
	reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
	7 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/builtin_actions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/templates/reviews/ui/base.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/templates/reviews/action.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/builtin_actions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/templates/reviews/ui/base.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/templates/reviews/action.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js

This is shaping up nicely :)
There are quite a few missing docstrings and I didn't flag all instane of this. In general, all classes should have docstrings. All methods (except things that are being overridden, unless they have a special use case) should be documented as well.
As far as formatting goes, classes should be formatted as follows (note the blank line between the docstring and the attribute):

class ClassName(object):
    """Single line summary.

    Multi-line description.
    """

    first_member = 1


Also, things like side effects and important notes should be in the multi-line description of the docstring, not under a separate section. The args/return sections are pulled out by sphinx into build docs, but those other sections won't be.

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

Show all issues

:py:class:`BaseReviewRequestAction`

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

Show all issues

This should use reST clode blocks.
E.g.,

For example:

.. code-block:: python
   class UsedOnceAction(BaseReviewRequestAction):
       action_id = 'once'
       label = 'This is used once.'

   class UsedMultipleAction(BaseReviewRequestAction):
       def __init__(self, action_id, label):
           self.action_id = 'repeat_' + action_id
           self.label = label


This really shouldn't be REPL output either

reviewboard/reviews/actions.py (Diff revision 5)
Show all issues
```
Blank line between these.
```

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

Show all issues

This should go above args, e.g.:

"""Return whether or not this action should be rendered.

The default implementation is to always render the action.

Args:
...

reviewboard/reviews/actions.py (Diff revision 5)
Show all issues
```
Blank line between these.
```
reviewboard/reviews/actions.py (Diff revision 5)
Show all issues
```
This should go above Args
```

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

Show all issues

This should just be KeyError.

The :py:exc:Exception stuff is for inline in strings, not in things that the docs expect to be types.

reviewboard/reviews/builtin_actions.py (Diff revision 5)
Show all issues
```
Needs a docstring. Likewise for all members of this class.
```

reviewboard/reviews/builtin_actions.py (Diff revision 5)

Show all issues

Needs a docstring. Also, blank line between this and action_id. Likewise for encapsulated classes.

reviewboard/reviews/builtin_actions.py (Diff revision 5)
Show all issues
```
This should be above args.
```

Change Summary:

Address the issues raised by Barret.

Improved various comments.
Added a :py:class: for classes in the multi-line part of a docstring.
Added a reST clode block for Python examples in the docstring.
Added a newline after each docstring.
Moved the Remark and Side Effect sections to the multi-line part.
Renamed builtin_actions to default_actions.
Removed :py:exc: from the Raises section.
Added docstrings to several simple classes.

Description:

		Part 1: Update the simplest hardcoded actions in the Reviews tab.

		Added the `BaseReviewRequestAction` class (see: `BaseReviewRequestField`).
		Added the `for_review_request_action` tag (see: `for_review_request_fieldset`).
		Updated the "Review", "Ship It!", and "Download Diff" actions.



		Part 2: Update the simplest hardcoded actions in the Diff tab.

		Added the `hidden` class variable for `DownloadDiffAction`.
		Modified `view_diff.html` so that simple actions now work for both tabs.



		Part 3: Update the default menu actions.

		Changed from a block tag to a basic tag (`review_request_actions`).
		Actions are now responsible for rendering via `context`.
		Added a `menu_action.html` file that includes `action.html`.



		TODO:

~		Make sure that extensions can unregister default actions.
~		Make sure that the actions hooks are compatible.
~		Start looking at the `RB.ReviewRequestEditorView` JavaScript class.
	~	Figure out how to register a menu action's children actions.
	~	Make sure that extensions can unregister default actions.
	~	Make sure that the actions hooks are compatible.
	+	Start looking at the `RB.ReviewRequestEditorView` JavaScript class.

Diff:

Revision 6 (+364 -55)

Show changes

	reviewboard/reviews/actions.py
	reviewboard/reviews/default_actions.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	reviewboard/static/rb/js/views/reviewRequestEditorView.js
	reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
	7 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/reviews/default_actions.py
    reviewboard/reviews/templatetags/reviewtags.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/templates/reviews/ui/base.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/templates/reviews/action.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/reviews/default_actions.py
    reviewboard/reviews/templatetags/reviewtags.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/templates/reviews/ui/base.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/templates/reviews/action.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js

Change Summary:

Improved the description for Part 3.

Description:

		Part 1: Update the simplest hardcoded actions in the Reviews tab.

		Added the `BaseReviewRequestAction` class (see: `BaseReviewRequestField`).
		Added the `for_review_request_action` tag (see: `for_review_request_fieldset`).
		Updated the "Review", "Ship It!", and "Download Diff" actions.



		Part 2: Update the simplest hardcoded actions in the Diff tab.

		Added the `hidden` class variable for `DownloadDiffAction`.
		Modified `view_diff.html` so that simple actions now work for both tabs.



		Part 3: Update the default menu actions.

~		Changed from a block tag to a basic tag (`review_request_actions`).
~		Actions are now responsible for rendering via `context`.
~		Added a `menu_action.html` file that includes `action.html`.
	~	Refactored how URLs are initialized by calling `local_site_reverse()`.
	~	Updated the "Close" and "Update" menu actions and their children actions.
	~	Changed from a block tag to a basic tag (`review_request_actions`).
	+	Made actions responsible for rendering via `context`.
	+	Added a `menu_action.html` file that includes a new `action.html` file.



		TODO:

		Figure out how to register a menu action's children actions.
		Make sure that extensions can unregister default actions.
		Make sure that the actions hooks are compatible.
		Start looking at the `RB.ReviewRequestEditorView` JavaScript class.

```
This looks like a great start!
```

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

Show all issues

No blank line between the docstring and the first line of code for functions.

adriano Oct. 13, 2015, 12:35 p.m.

I did have it like that originally, but Barret recommended putting a newline there (https://reviews.reviewboard.org/r/7661/#review22277).

david Oct. 13, 2015, 12:37 p.m.

So we have a blank line between the docstring and the first thing in a class, but not a function. The distinction here is that members/methods in a class always have a blank line separating them (and the docstring is a member), but lines of code in a function don't.

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

Show all issues

No blank line between the docstring and the first line of code for functions.

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

Show all issues

Because you're manipulating this in a method, it probably shouldn't be data on the class. Can you create an __init__ and assign this there?

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

Show all issues

No blank line between the docstring and the first line of code for functions.

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

Show all issues

No blank line between the docstring and the first line of code for functions.

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

Show all issues

No blank line between the docstring and the first line of code for functions.

reviewboard/reviews/default_actions.py (Diff revision 6)

Show all issues

No blank line between the docstring and the first line of code for functions.

reviewboard/reviews/default_actions.py (Diff revision 6)

Show all issues

This comment confuses me.

adriano Oct. 13, 2015, 8:52 p.m.

This comment is intended to explain why I am using context.get('has_diffs', True) instead of context['has_diffs']. A review request can be viewed from three different tabs: "Reviews", "File", and "Diff" (the third "File" tab only appears when viewing file attachments). In the "Reviews" and "File" tabs, there is a has_diffs key that can be looked up in the context dictionary. Its corresponding value in the dictionary is set to True iff the review request has at least one diff.

In the "Diff" tab on the other hand, the has_diffs key does not exist in the context dictionary. It makes sense not to include this redundant piece of information; after all, the only way we could have accessed the "Diff" tab in the first place is if the corresponding review request had a diff already.

Perhaps a better comment would be something like:

# If has_diffs does not exist, then we must be in the Diff tab.
# So the corresponding review request must already have a diff.

reviewboard/reviews/default_actions.py (Diff revision 6)

Show all issues

Can we call this review_request? We don't abbreviate to "rev" this way.

reviewboard/reviews/default_actions.py (Diff revision 6)
Show all issues
```
You should be able to just use .display_id here.
```
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 6)
Show all issues
```
No blank line between docstring and code in functions.
```

reviewboard/reviews/templatetags/reviewtags.py (Diff revision 6)

Show all issues

These two lines could be combined (you only use actions once)

reviewboard/reviews/templatetags/reviewtags.py (Diff revision 6)

Show all issues

Hmm. I'm not sure how I feel about passing in a list and having the render() method append to it. How about we let render() return a string, and then we build the result using a list comprehension?

return ''.join([
    action.render(context)
    for action in get_review_request_actions()
])


This way the data flow in render() is entirely one-way.

reviewboard/templates/reviews/action.html (Diff revision 6)
Show all issues
```
Let's put the whole if/endif thing on the same line.
```

Change Summary:

Address the issues raised by David.

Removed blanklines after docstrings for functions.
Changed renderable_actions to an instance attribute.
Improved various comments and variable names.
Replaced various list comprehensions with generator expressions.
Added Attributes sections for the base class docstrings.
Made better use of default arguments and calling super().
Implemented children action registration.

Description:

~		Part 1: Update the simplest hardcoded actions in the Reviews tab.
	~	Part 1: Update the simplest default actions.

		Added the `BaseReviewRequestAction` class (see: `BaseReviewRequestField`).
		Added the `for_review_request_action` tag (see: `for_review_request_fieldset`).
		Updated the "Review", "Ship It!", and "Download Diff" actions.
	+	Added the `hidden` class variable for `DownloadDiffAction`.
	+	Modified `view_diff.html` so that simple actions now work for all tabs.



~		Part 2: Update the simplest hardcoded actions in the Diff tab.
	~	Part 2: Update the default menu actions.
-
-		Added the `hidden` class variable for `DownloadDiffAction`.
-		Modified `view_diff.html` so that simple actions now work for both tabs.
-
-
-
-		Part 3: Update the default menu actions.

		Refactored how URLs are initialized by calling `local_site_reverse()`.
		Updated the "Close" and "Update" menu actions and their children actions.
		Changed from a block tag to a basic tag (`review_request_actions`).
~		Made actions responsible for rendering via `context`.
	~	Made actions responsible for rendering and registering.
		Added a `menu_action.html` file that includes a new `action.html` file.



		TODO:

~		Figure out how to register a menu action's children actions.
~		Make sure that extensions can unregister default actions.
~		Make sure that the actions hooks are compatible.
	~	Make sure that extensions can unregister default actions.
	~	Make sure that the actions hooks are compatible.
	~	Start looking at the `RB.ReviewRequestEditorView` JavaScript class.
-		Start looking at the `RB.ReviewRequestEditorView` JavaScript class.

Diff:

Revision 7 (+422 -55)

Show changes

	reviewboard/reviews/actions.py
	reviewboard/reviews/default_actions.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	reviewboard/static/rb/js/views/reviewRequestEditorView.js
	reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
	7 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/reviews/default_actions.py
    reviewboard/reviews/templatetags/reviewtags.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/templates/reviews/ui/base.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/templates/reviews/action.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/reviews/default_actions.py
    reviewboard/reviews/templatetags/reviewtags.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/templates/reviews/ui/base.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/templates/reviews/action.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js

Change Summary:

Allow extensions to provide their own actions (as dicts or instances).

Removed 3 action hook basic tags:
1. review_request_action_hooks
2. review_request_dropdown_action_hooks
3. diffviewer_action_hooks
Added a BaseReviewRequestActionHook for other hooks to inherit from.
Changed the _top_level_ids from a list to a deque.
- Registering an action involves appending to the left of a deque, so
we sometimes have to iterate through actions in reverse.

Description:

	+	Overview
	+
	+	This is an architectural change for Review Board 2.6 that affects extensions and requires both front-end (JavaScript) and back-end (Python) work.
	+	Currently, there are some default actions (like "Review" or "Ship It!") that can be performed on review requests. Extensions can provide custom actions by subclassing various `ActionHooks`.
	+	The goal of this project is to encapsulate this logic into a `ReviewRequestAction`. Instead of using hooks, extensions will be able to provide custom actions by registering them as `ReviewRequestAction` instances (or remove the default actions by unregistering them).
	+
	+
	+
		Part 1: Update the simplest default actions.

~		Added the `BaseReviewRequestAction` class (see: `BaseReviewRequestField`).
~		Added the `for_review_request_action` tag (see: `for_review_request_fieldset`).
	~	Added the `BaseReviewRequestAction` class (this was based on the `BaseReviewRequestField` class).
	~	Added the `for_review_request_action` block tag (this was based on the `for_review_request_fieldset` block tag).
		Updated the "Review", "Ship It!", and "Download Diff" actions.
		Added the `hidden` class variable for `DownloadDiffAction`.
~		Modified `view_diff.html` so that simple actions now work for all tabs.
	~	Modified `view_diff.html` so that simple actions work for all tabs.
	+	Switched from using a `SortedDict` to using a `dict` and a `list`.



		Part 2: Update the default menu actions.

		Refactored how URLs are initialized by calling `local_site_reverse()`.
~		Updated the "Close" and "Update" menu actions and their children actions.
	~	Updated the "Close" and "Update" menu actions (and their children).
		Changed from a block tag to a basic tag (`review_request_actions`).
		Made actions responsible for rendering and registering.
~		Added a `menu_action.html` file that includes a new `action.html` file.
	~	Added a `menu_action.html` file that includes a new `action.html` file.
	+
	+
	+
	+	Part 3: Allow extensions to provide their own actions.
	+
	+	Removed 3 action hook basic tags: 1. `review_request_action_hooks` 2. `review_request_dropdown_action_hooks` 3. `diffviewer_action_hooks`
	+	Added a `BaseReviewRequestActionHook` for other hooks to inherit from.
	+	Changed the `_top_level_ids` from a `list` to a `deque`.
	+	Registering an action involves appending to the left of a `deque`, so we sometimes have to iterate through actions in reverse.



		TODO:

~		Make sure that extensions can unregister default actions.
~		Make sure that the actions hooks are compatible.
	~	Fix 7 commented out unit tests (they fail either due to missing tags that were deleted, or because ID's must now be unique) and add my own.
	~	Make sure that extensions can unregister default actions.
		Start looking at the `RB.ReviewRequestEditorView` JavaScript class.

Testing Done:

		Ran and passed the relevant reviewboard unit tests.
~		Manually verified that all default actions display correctly in different scenarios.
	~	Manually verified that all default actions display correctly in different scenarios.
	+	Added my own extension that provides its own actions in different ways and manually verified that each action displayed correctly in different scenarios.

Diff:

Revision 8 (+598 -172)

Show changes

	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/extensions/templatetags/rb_extensions.py
	reviewboard/reviews/actions.py
	reviewboard/reviews/default_actions.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	11 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/extensions/tests.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/default_actions.py
    reviewboard/extensions/templatetags/rb_extensions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/templates/reviews/ui/base.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/extensions/action_dropdown.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/templates/reviews/action.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/extensions/tests.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/default_actions.py
    reviewboard/extensions/templatetags/rb_extensions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/templates/reviews/ui/base.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/extensions/action_dropdown.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/templates/reviews/action.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js

Change Summary:

Allow menu actions to have their own menu actions.

Removed the renderable_actions attribute.
Made sure that menu actions check if they themselves should render
before recursively rendering its children actions.
Added a new children_actions basic tag.

Description:

		Overview

		This is an architectural change for Review Board 2.6 that affects extensions and requires both front-end (JavaScript) and back-end (Python) work.
		Currently, there are some default actions (like "Review" or "Ship It!") that can be performed on review requests. Extensions can provide custom actions by subclassing various `ActionHooks`.
		The goal of this project is to encapsulate this logic into a `ReviewRequestAction`. Instead of using hooks, extensions will be able to provide custom actions by registering them as `ReviewRequestAction` instances (or remove the default actions by unregistering them).



		Part 1: Update the simplest default actions.

		Added the `BaseReviewRequestAction` class (this was based on the `BaseReviewRequestField` class).
		Added the `for_review_request_action` block tag (this was based on the `for_review_request_fieldset` block tag).
		Updated the "Review", "Ship It!", and "Download Diff" actions.
		Added the `hidden` class variable for `DownloadDiffAction`.
		Modified `view_diff.html` so that simple actions work for all tabs.
		Switched from using a `SortedDict` to using a `dict` and a `list`.



		Part 2: Update the default menu actions.

		Refactored how URLs are initialized by calling `local_site_reverse()`.
		Updated the "Close" and "Update" menu actions (and their children).
		Changed from a block tag to a basic tag (`review_request_actions`).
		Made actions responsible for rendering and registering.
		Added a `menu_action.html` file that includes a new `action.html` file.



		Part 3: Allow extensions to provide their own actions.

		Removed 3 action hook basic tags: 1. `review_request_action_hooks` 2. `review_request_dropdown_action_hooks` 3. `diffviewer_action_hooks`
		Added a `BaseReviewRequestActionHook` for other hooks to inherit from.
		Changed the `_top_level_ids` from a `list` to a `deque`.
~		Registering an action involves appending to the left of a `deque`, so we sometimes have to iterate through actions in reverse.
	~	Allowed menu actions to have their own children menu actions.
	+	Added a new `children_actions` basic tag.



		TODO:

		Fix 7 commented out unit tests (they fail either due to missing tags that were deleted, or because ID's must now be unique) and add my own.
		Make sure that extensions can unregister default actions.
		Start looking at the `RB.ReviewRequestEditorView` JavaScript class.

Diff:

Revision 9 (+605 -172)

Show changes

	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/extensions/templatetags/rb_extensions.py
	reviewboard/reviews/actions.py
	reviewboard/reviews/default_actions.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	11 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/extensions/tests.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/default_actions.py
    reviewboard/extensions/templatetags/rb_extensions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/templates/reviews/ui/base.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/extensions/action_dropdown.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/templates/reviews/action.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/extensions/tests.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/default_actions.py
    reviewboard/extensions/templatetags/rb_extensions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/templates/reviews/ui/base.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/extensions/action_dropdown.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/templates/reviews/action.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js

Change Summary:

Clean up comments and variable names and some redundant logic.

Renamed actions to children_actions.
Improved docstring explanation for registering actions.
Removed BaseReviewRequestMenuAction's redundant logic in render().

Description:

		Overview

		This is an architectural change for Review Board 2.6 that affects extensions and requires both front-end (JavaScript) and back-end (Python) work.
		Currently, there are some default actions (like "Review" or "Ship It!") that can be performed on review requests. Extensions can provide custom actions by subclassing various `ActionHooks`.
		The goal of this project is to encapsulate this logic into a `ReviewRequestAction`. Instead of using hooks, extensions will be able to provide custom actions by registering them as `ReviewRequestAction` instances (or remove the default actions by unregistering them).



		Part 1: Update the simplest default actions.

		Added the `BaseReviewRequestAction` class (this was based on the `BaseReviewRequestField` class).
		Added the `for_review_request_action` block tag (this was based on the `for_review_request_fieldset` block tag).
		Updated the "Review", "Ship It!", and "Download Diff" actions.
		Added the `hidden` class variable for `DownloadDiffAction`.
		Modified `view_diff.html` so that simple actions work for all tabs.
		Switched from using a `SortedDict` to using a `dict` and a `list`.



		Part 2: Update the default menu actions.

		Refactored how URLs are initialized by calling `local_site_reverse()`.
		Updated the "Close" and "Update" menu actions (and their children).
		Changed from a block tag to a basic tag (`review_request_actions`).
		Made actions responsible for rendering and registering.
		Added a `menu_action.html` file that includes a new `action.html` file.



		Part 3: Allow extensions to provide their own actions.

		Removed 3 action hook basic tags: 1. `review_request_action_hooks` 2. `review_request_dropdown_action_hooks` 3. `diffviewer_action_hooks`
		Added a `BaseReviewRequestActionHook` for other hooks to inherit from.
		Changed the `_top_level_ids` from a `list` to a `deque`.
		Allowed menu actions to have their own children menu actions.
		Added a new `children_actions` basic tag.



		TODO:

		Fix 7 commented out unit tests (they fail either due to missing tags that were deleted, or because ID's must now be unique) and add my own.
		Make sure that extensions can unregister default actions.
		Start looking at the `RB.ReviewRequestEditorView` JavaScript class.
	+
	+
	+
	+	(Low Priority) Questions for the Mentors:
	+
	+	Should `BaseReviewRequestActionHook` be included in `__all__`?
	+	Should IDs for `BaseReviewRequestActions` be optional, similar to action-style dicts?
	+	Should `BaseReviewRequestActions` also accept images as fields?
	+	Should we update the other `ActionHooks` (for the header) as well?
	+	Should we call them `DropdownActions` or `MenuActions`?
	+	How should nested menus be displayed?

Testing Done:

		Ran and passed the relevant reviewboard unit tests.
~		Manually verified that all default actions display correctly in different scenarios.
~		Added my own extension that provides its own actions in different ways and manually verified that each action displayed correctly in different scenarios.
	~	Manually verified that all default actions display correctly in different scenarios. Test in all three tabs ("Reviews", "File", "Diff"). Test in normal as well as local sites. Test `DownloadDiffAction` with the slider (for interdiffs). Test as an authenticated user and as a guest. Test when the review request is pending review and is submitted.
	~	Added my own extension that provides its own actions in different ways and manually verified that each action displayed correctly in different scenarios. Test adding actions and menu actions via action-style dicts. Test adding actions and menu actions via action instances. Test adding menu actions that contain their own menu actions. Test that a menu action can override its children from rendering.

Diff:

Revision 10 (+594 -172)

Show changes

	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/extensions/templatetags/rb_extensions.py
	reviewboard/reviews/actions.py
	reviewboard/reviews/default_actions.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	11 more

Added Files:

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/extensions/tests.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/default_actions.py
    reviewboard/extensions/templatetags/rb_extensions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/templates/reviews/ui/base.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/extensions/action_dropdown.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/templates/reviews/action.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/extensions/tests.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/default_actions.py
    reviewboard/extensions/templatetags/rb_extensions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/templates/reviews/ui/base.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/extensions/action_dropdown.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/templates/reviews/action.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js

reviewboard/extensions/hooks.py (Diff revision 10)
Show all issues
```
Add another blank line after this.
```

reviewboard/extensions/hooks.py (Diff revision 10)

Show all issues

I'm not sure why all of these are class methods. Can you clarify for me?

adriano Oct. 20, 2015, 3:10 p.m.

I was switching a lot between different types of methods actually. At first, they were static methods because they never used instance attributes like actions. Then I changed them to class methods because for example _register_and_get() calls _to_action_instance(), and subclasses might want to override these methods with their own implementations.
I suppose that we could extend this argument and say that extensions might want different instances of the same subclass to override these methods in different ways. For example:

class Foo(BaseReviewRequestActionHook):
    def __init__(self, extension, actions=[], is_awesome=True, *args, **kwargs):
        super(Foo, self).__init__(extension, actions, *args, **kwargs)
        self.is_awesome = is_awesome

    def action_should_render(self, context):
        if not self.is_awesome:
            return False
        return super(Foo, self).action_should_render(self, context)


So in the interest of being as flexible as possible, I guess I should just change them all to be simple instance methods. (Also, I forgot to put an underscore before action_should_render() - it should be a "protected" method as well.)

david Oct. 20, 2015, 3:12 p.m.

Instance methods sound good to me. In Python, static methods are used only occasionally and class methods are used almost never.

reviewboard/reviews/default_actions.py (Diff revision 10)

Show all issues

Can we just pull these out into top-level classes? There's no real reason to hide them inside their parent menus, and it makes it harder to read.

reviewboard/reviews/default_actions.py (Diff revision 10)

Show all issues

"children_actions" is kind of redundant. How about either "children" or "child_actions"?

reviewboard/reviews/default_actions.py (Diff revision 10)
Show all issues
```
Same with these (pull them out into top-level classes).
```

reviewboard/reviews/default_actions.py (Diff revision 10)

Show all issues

This is probably not a terribly reliable way to detect this. It might be a lot better to pass the review request into should_render.

adriano Oct. 23, 2015, 12:08 a.m.

I'm not sure what you mean by this. Basically, the should_render(self, context) method gets called from only one place: render(self, context, action_key, template_name). If I change the function prototype to something like should_render(self, context, review_request), then how would I get the review request in the first place? The only way I know how to retrieve the current review request is by grabbing it from the context, so I would naively do something like:
def render(self, context, action_key, template_name):
    content = ''

    if self.should_render(context, context['review_request']):
        context.push()
        context[action_key] = self
        content = render_to_string(template_name, context)
        context.pop()

    return content

Is there a special call that I can make to retrieve the current review request from somewhere other than the context?

Alternatively, perhaps you mean that it's fine to retrieve the review request from the context, but I shouldn't rely on using the has_diffs key. Instead, perhaps I should manually retrieve this from the review request by doing something like:
def should_render(self, context):
    review_request = context['review_request']
    request = context['request']
    draft = review_request.get_draft(request.user)
    diffsets = review_request.get_diffsets()

    if (draft and draft.diffset_id) or len(diffsets) > 0:
        self.label = _('Update Diff')
    else:
        self.label = _('Upload Diff')

    return context['upload_diff_form']


(I'm not terribly sure how I can check for an upload_diff_form without using the context.)

reviewboard/reviews/default_actions.py (Diff revision 10)

Show all issues

We should maybe also pass the request into this, and introspect the url/view name, instead of trying to reverse engineer it from the context contents.

reviewboard/templates/reviews/action.html (Diff revision 10)

Show all issues

Can we wrap this somehow? Maybe split the line after the href="..."?

Change Summary:

Fix the commented out unit tests and implement unregistration.

Both basic tags now have try/except blocks.
Both base actions now have an unregister method (good for unit tests).


Use AppliesToURLMixin and add one unit test.

BaseReviewRequestActionHook now also subclasses AppliesToURLMixin.
_action_should_render was removed.


Address the issues raised by David.

Replaced class methods with instance methods.
Modified some public methods to be protected instead.
Used resolver_match instead of relying on riskier context keys.
Renamed children actions -> child_actions.

Description:

		Overview

		This is an architectural change for Review Board 2.6 that affects extensions and requires both front-end (JavaScript) and back-end (Python) work.
		Currently, there are some default actions (like "Review" or "Ship It!") that can be performed on review requests. Extensions can provide custom actions by subclassing various `ActionHooks`.
~		The goal of this project is to encapsulate this logic into a `ReviewRequestAction`. Instead of using hooks, extensions will be able to provide custom actions by registering them as `ReviewRequestAction` instances (or remove the default actions by unregistering them).
	~	The goal of this project is to encapsulate this logic into a `ReviewRequestAction`. Instead of passing action-style dictionaries into the hooks, extensions will be able to provide custom actions by registering them as `ReviewRequestAction` instances. Furthermore, we will be able to remove any subset of the default actions by simply unregistering them.



		Part 1: Update the simplest default actions.

		Added the `BaseReviewRequestAction` class (this was based on the `BaseReviewRequestField` class).
		Added the `for_review_request_action` block tag (this was based on the `for_review_request_fieldset` block tag).
		Updated the "Review", "Ship It!", and "Download Diff" actions.
		Added the `hidden` class variable for `DownloadDiffAction`.
		Modified `view_diff.html` so that simple actions work for all tabs.
		Switched from using a `SortedDict` to using a `dict` and a `list`.



		Part 2: Update the default menu actions.

		Refactored how URLs are initialized by calling `local_site_reverse()`.
		Updated the "Close" and "Update" menu actions (and their children).
		Changed from a block tag to a basic tag (`review_request_actions`).
		Made actions responsible for rendering and registering.
		Added a `menu_action.html` file that includes a new `action.html` file.



		Part 3: Allow extensions to provide their own actions.

		Removed 3 action hook basic tags: 1. `review_request_action_hooks` 2. `review_request_dropdown_action_hooks` 3. `diffviewer_action_hooks`
		Added a `BaseReviewRequestActionHook` for other hooks to inherit from.
		Changed the `_top_level_ids` from a `list` to a `deque`.
		Allowed menu actions to have their own children menu actions.
~		Added a new `children_actions` basic tag.
	~	Added a new `child_actions` basic tag.
	+	Both basic tags now have try/except blocks.
	+	Both base actions now have an unregister method (good for unit tests).
	+	`BaseReviewRequestActionHook` now also subclasses `AppliesToURLMixin`.



		TODO:

~		Fix 7 commented out unit tests (they fail either due to missing tags that were deleted, or because ID's must now be unique) and add my own.
	~	Add more unit tests.
		Make sure that extensions can unregister default actions.
		Start looking at the `RB.ReviewRequestEditorView` JavaScript class.



		(Low Priority) Questions for the Mentors:

		Should `BaseReviewRequestActionHook` be included in `__all__`?
~		Should IDs for `BaseReviewRequestActions` be optional, similar to action-style dicts?
	~	How should nested menus be displayed?
-		Should `BaseReviewRequestActions` also accept images as fields?
-		Should we update the other `ActionHooks` (for the header) as well?
-		Should we call them `DropdownActions` or `MenuActions`?
-		How should nested menus be displayed?

Diff:

Revision 11 (+747 -147)

Show changes

	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/extensions/templatetags/rb_extensions.py
	reviewboard/reviews/actions.py
	reviewboard/reviews/default_actions.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	11 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/extensions/tests.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/default_actions.py
    reviewboard/extensions/templatetags/rb_extensions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/templates/reviews/ui/base.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/extensions/action_dropdown.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/templates/reviews/action.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/extensions/tests.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/default_actions.py
    reviewboard/extensions/templatetags/rb_extensions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/templates/reviews/ui/base.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/extensions/action_dropdown.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/templates/reviews/action.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js

reviewboard/reviews/templatetags/reviewtags.py (Diff revision 11)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```

Change Summary:

Fix whitespace and extend the base unit tests.

Diff:

Revision 12 (+785 -143)

Show changes

	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/extensions/templatetags/rb_extensions.py
	reviewboard/reviews/actions.py
	reviewboard/reviews/default_actions.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	11 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/extensions/tests.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/default_actions.py
    reviewboard/extensions/templatetags/rb_extensions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/templates/reviews/ui/base.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/extensions/action_dropdown.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/templates/reviews/action.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/extensions/tests.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/default_actions.py
    reviewboard/extensions/templatetags/rb_extensions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/templates/reviews/ui/base.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/extensions/action_dropdown.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/templates/reviews/action.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js

```
This is starting to look very solid!
```

reviewboard/extensions/hooks.py (Diff revision 12)

Show all issues

Yes, it should be. __all__ defines which symbols get exported from the file (if there's no __all__, everything is exported).

reviewboard/extensions/hooks.py (Diff revision 12)

Show all issues

_() doesn't work this way. The l10n scanner looks at the source code and extracts all string literals which are wrapped with gettext. If you call this on a variable, the scanner won't pull it out.

If actions want to localize, they should call ugettext_lazy (aka _()) on their labels themselves (which it looks like you're already doing).

reviewboard/extensions/hooks.py (Diff revision 12)
Show all issues
```
Same here.
```

reviewboard/extensions/hooks.py (Diff revision 12)

Show all issues

I would wrap this differently:

super(DiffViewerActionHook, self).__init__(
    extension, apply_to, actions, *args, **kwargs)

reviewboard/reviews/actions.py (Diff revision 12)
Show all issues
```
Add a blank line after this.
```

reviewboard/reviews/default_actions.py (Diff revision 12)

Show all issues

Because we're intending to return a bool (and not a Repository), we should make this a truth test:

return review_request.repository is not None

reviewboard/reviews/default_actions.py (Diff revision 12)

Show all issues

Instead of using 'P' as a literal, you can compare this against ReviewRequest.PENDING_REVIEW.

Change Summary:

Allow extensions to register actions with respect to a specified parent.

Added a public register_actions() method.
Added detailed comments as to how I might refactor the code.


Address the issues raised by David.

Stopped surrounding variables with _().
Wrapped _should_render's return statements with bool().
Replaced literals with imported constants (sort of like an enum).
Cleaned up some variable names and how default arguments are handled.
Cleaned up various whitespace and wrapping conventions.
Child actions can now be passed in via the constructor.


Allow extensions to unregister actions.

Added a public unregister_actions() method.
Replaced is_top_level with parent.
Added _parent so that extensions can unregister by ID.

Description:

		Overview

		This is an architectural change for Review Board 2.6 that affects extensions and requires both front-end (JavaScript) and back-end (Python) work.
		Currently, there are some default actions (like "Review" or "Ship It!") that can be performed on review requests. Extensions can provide custom actions by subclassing various `ActionHooks`.
		The goal of this project is to encapsulate this logic into a `ReviewRequestAction`. Instead of passing action-style dictionaries into the hooks, extensions will be able to provide custom actions by registering them as `ReviewRequestAction` instances. Furthermore, we will be able to remove any subset of the default actions by simply unregistering them.



		Part 1: Update the simplest default actions.

		Added the `BaseReviewRequestAction` class (this was based on the `BaseReviewRequestField` class).
		Added the `for_review_request_action` block tag (this was based on the `for_review_request_fieldset` block tag).
		Updated the "Review", "Ship It!", and "Download Diff" actions.
		Added the `hidden` class variable for `DownloadDiffAction`.
		Modified `view_diff.html` so that simple actions work for all tabs.
		Switched from using a `SortedDict` to using a `dict` and a `list`.



		Part 2: Update the default menu actions.

		Refactored how URLs are initialized by calling `local_site_reverse()`.
		Updated the "Close" and "Update" menu actions (and their children).
		Changed from a block tag to a basic tag (`review_request_actions`).
		Made actions responsible for rendering and registering.
		Added a `menu_action.html` file that includes a new `action.html` file.



		Part 3: Allow extensions to provide their own actions.

		Removed 3 action hook basic tags: 1. `review_request_action_hooks` 2. `review_request_dropdown_action_hooks` 3. `diffviewer_action_hooks`
		Added a `BaseReviewRequestActionHook` for other hooks to inherit from.
		Changed the `_top_level_ids` from a `list` to a `deque`.
		Allowed menu actions to have their own children menu actions.
		Added a new `child_actions` basic tag.
		Both basic tags now have try/except blocks.
		Both base actions now have an unregister method (good for unit tests).
		`BaseReviewRequestActionHook` now also subclasses `AppliesToURLMixin`.



	+	Part 4: Allow extensions to register/unregister actions by ID.
	+
	+	Added a public `unregister_actions()` method.
	+	Replaced `is_top_level` with `parent`.
	+	Added `_parent` so that extensions can unregister by ID.
	+	Added a public `register_actions()` method.
	+
	+
	+
		TODO:

		Add more unit tests.
~		Make sure that extensions can unregister default actions.
	~	Keep thinking about how to refactor the code (see the TODO comment).
		Start looking at the `RB.ReviewRequestEditorView` JavaScript class.



		(Low Priority) Questions for the Mentors:

~		Should `BaseReviewRequestActionHook` be included in `__all__`?
	~	How can I refactor my code (see the TODO comment)?
		How should nested menus be displayed?

Testing Done:

		Ran and passed the relevant reviewboard unit tests.
		Manually verified that all default actions display correctly in different scenarios. Test in all three tabs ("Reviews", "File", "Diff"). Test in normal as well as local sites. Test `DownloadDiffAction` with the slider (for interdiffs). Test as an authenticated user and as a guest. Test when the review request is pending review and is submitted.
~		Added my own extension that provides its own actions in different ways and manually verified that each action displayed correctly in different scenarios. Test adding actions and menu actions via action-style dicts. Test adding actions and menu actions via action instances. Test adding menu actions that contain their own menu actions. Test that a menu action can override its children from rendering.
	~	Added my own extension that provides its own actions in different ways and manually verified that each action displayed correctly in different scenarios. Test adding actions and menu actions via action-style dicts. Test adding actions and menu actions via action instances. Test adding menu actions that contain their own menu actions. Test that a menu action can override its children from rendering. Test unregistering a default action. Test unregistering a child action of a default menu action. Test registering a new child action on a default menu action.

Diff:

Revision 13 (+849 -143)

Show changes

	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/extensions/templatetags/rb_extensions.py
	reviewboard/reviews/actions.py
	reviewboard/reviews/default_actions.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	11 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/extensions/tests.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/default_actions.py
    reviewboard/extensions/templatetags/rb_extensions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/templates/reviews/ui/base.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/extensions/action_dropdown.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/templates/reviews/action.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/extensions/tests.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/default_actions.py
    reviewboard/extensions/templatetags/rb_extensions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/templates/reviews/ui/base.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/extensions/action_dropdown.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/templates/reviews/action.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js

reviewboard/extensions/hooks.py (Diff revision 13)
Show all issues
```
 '_' imported but unused
```

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

Show all issues

Mind fixing the spelling here while you're editing this file?

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

Show all issues

It doesn't really matter, but I think it would be nicer to have this set after we register all the default actions.

adriano Nov. 7, 2015, 8:43 a.m.

Hmm. I tried changing the order, but this results in infinite recursion since default_action.register() will call _populate_defaults() and enter the if statement forever. I suppose that we could instead use some kind of enum called _status that keeps track of three different states: UNPOPULATED, IS_POPULATING and POPULATED. Then we could have something like:
def _populate_defaults():
    """Populate the default action instances."""
    global _status

    if _status != POPULATED:
        from reviewboard.reviews.default_actions import default_actions

        _status = IS_POPULATING

        for default_action in reversed(default_actions):
            default_action.register()

        _status = POPULATED


Was this what you had in mind?

reviewboard/reviews/default_actions.py (Diff revision 13)

Show all issues

Instead of just casting to bool, I think we should explicitly test review_request.repository is not None.

reviewboard/reviews/templatetags/reviewtags.py (Diff revision 13)

Show all issues

This can be rewritten using logging.exception, which we're slowly transitioning to:

logging.exception('Error rendering top-level action: %s',
                  top_level_action.action_id)

reviewboard/reviews/templatetags/reviewtags.py (Diff revision 13)
Show all issues
```
Same here.
```

Change Summary:

Added a reset_actions() method (useful in shutdown() for extensions).

Replaced the default_actions variable with a get_default_actions()

  factory method so that _populate_defaults() can grab a fresh copy of

  the original default actions after a reset.


Refactored when child actions get registered.

Child actions now get registered as soon as they are passed in to the

  parent menu action's constructor. So when the parent gets registered,

  it no longer has to recursively register its children.


Added a unit test for modifying the default actions and cleaned up code.

Reverted a change from the previous commit, since it led to infinite

  recursion.
Fixed docstring omission.
Replaced 'P' with constant.

Description:

		Overview

		This is an architectural change for Review Board 2.6 that affects extensions and requires both front-end (JavaScript) and back-end (Python) work.
		Currently, there are some default actions (like "Review" or "Ship It!") that can be performed on review requests. Extensions can provide custom actions by subclassing various `ActionHooks`.
		The goal of this project is to encapsulate this logic into a `ReviewRequestAction`. Instead of passing action-style dictionaries into the hooks, extensions will be able to provide custom actions by registering them as `ReviewRequestAction` instances. Furthermore, we will be able to remove any subset of the default actions by simply unregistering them.



		Part 1: Update the simplest default actions.

		Added the `BaseReviewRequestAction` class (this was based on the `BaseReviewRequestField` class).
		Added the `for_review_request_action` block tag (this was based on the `for_review_request_fieldset` block tag).
		Updated the "Review", "Ship It!", and "Download Diff" actions.
		Added the `hidden` class variable for `DownloadDiffAction`.
		Modified `view_diff.html` so that simple actions work for all tabs.
		Switched from using a `SortedDict` to using a `dict` and a `list`.



		Part 2: Update the default menu actions.

		Refactored how URLs are initialized by calling `local_site_reverse()`.
		Updated the "Close" and "Update" menu actions (and their children).
		Changed from a block tag to a basic tag (`review_request_actions`).
		Made actions responsible for rendering and registering.
		Added a `menu_action.html` file that includes a new `action.html` file.



		Part 3: Allow extensions to provide their own actions.

		Removed 3 action hook basic tags: 1. `review_request_action_hooks` 2. `review_request_dropdown_action_hooks` 3. `diffviewer_action_hooks`
		Added a `BaseReviewRequestActionHook` for other hooks to inherit from.
		Changed the `_top_level_ids` from a `list` to a `deque`.
		Allowed menu actions to have their own children menu actions.
		Added a new `child_actions` basic tag.
		Both basic tags now have try/except blocks.
		Both base actions now have an unregister method (good for unit tests).
		`BaseReviewRequestActionHook` now also subclasses `AppliesToURLMixin`.



		Part 4: Allow extensions to register/unregister actions by ID.

		Added a public `unregister_actions()` method.
		Replaced `is_top_level` with `parent`.
		Added `_parent` so that extensions can unregister by ID.
		Added a public `register_actions()` method.
	+	Changed child action registration to happen as soon as they are passed in to the parent menu action's constructor.
	+	Added a `reset_actions()` method (useful in `shutdown()` for extensions) and changed `default_actions` to a factory method.



		TODO:

~		Add more unit tests.
~		Keep thinking about how to refactor the code (see the TODO comment).
	~	Start trying to implement my own JS extension that will make use of my own Djblets extension hook.
	~	Start updating the documentation for the affected hooks.
-		Start looking at the `RB.ReviewRequestEditorView` JavaScript class.



		(Low Priority) Questions for the Mentors:

~		How can I refactor my code (see the TODO comment)?
	~	How should nested menus be displayed?
-		How should nested menus be displayed?

Diff:

Revision 14 (+899 -146)

Show changes

	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/extensions/templatetags/rb_extensions.py
	reviewboard/reviews/actions.py
	reviewboard/reviews/default_actions.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	11 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/extensions/tests.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/default_actions.py
    reviewboard/extensions/templatetags/rb_extensions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/templates/reviews/ui/base.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/extensions/action_dropdown.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/templates/reviews/action.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/extensions/tests.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/default_actions.py
    reviewboard/extensions/templatetags/rb_extensions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/templates/reviews/ui/base.html
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/extensions/action_dropdown.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/templates/reviews/action.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js

Change Summary:

Update the documentation for action hooks.

Used "note" blocks in various docstrings and fixed various whitespace

  issues that were interfering with Sphinx.
Renamed action-hooks to action-hook.
Added reviewboard.reviews.actions and

reviewboard.reviews.default_actions to the toctree.
Introduced abbreviations to help make the tables nicer.
Added various "versionadded" and "seealso" and "autosummary" blocks.
Added a new section for modifying the default actions.
Extended the given example to illustrate the new functionality.


Add a ReviewRequestActionHook model for providing callbacks.

Summary:

-	[WIP] Review Request Action Registration.
+	Review Request Action Registration.

Description:

		Overview

		This is an architectural change for Review Board 2.6 that affects extensions and requires both front-end (JavaScript) and back-end (Python) work.
		Currently, there are some default actions (like "Review" or "Ship It!") that can be performed on review requests. Extensions can provide custom actions by subclassing various `ActionHooks`.
		The goal of this project is to encapsulate this logic into a `ReviewRequestAction`. Instead of passing action-style dictionaries into the hooks, extensions will be able to provide custom actions by registering them as `ReviewRequestAction` instances. Furthermore, we will be able to remove any subset of the default actions by simply unregistering them.



		Part 1: Update the simplest default actions.

		Added the `BaseReviewRequestAction` class (this was based on the `BaseReviewRequestField` class).
		Added the `for_review_request_action` block tag (this was based on the `for_review_request_fieldset` block tag).
		Updated the "Review", "Ship It!", and "Download Diff" actions.
		Added the `hidden` class variable for `DownloadDiffAction`.
		Modified `view_diff.html` so that simple actions work for all tabs.
		Switched from using a `SortedDict` to using a `dict` and a `list`.



		Part 2: Update the default menu actions.

		Refactored how URLs are initialized by calling `local_site_reverse()`.
~		Updated the "Close" and "Update" menu actions (and their children).
~		Changed from a block tag to a basic tag (`review_request_actions`).
~		Made actions responsible for rendering and registering.
~		Added a `menu_action.html` file that includes a new `action.html` file.
	~	Added the `BaseReviewRequestMenuAction` subclass.
	~	Updated the "Close" and "Update" menu actions (and their children).
	~	Changed from a block tag to a basic tag (`review_request_actions`).
	~	Made actions responsible for rendering and registering.
	+	Added a `menu_action.html` file that includes a new `action.html` file.



		Part 3: Allow extensions to provide their own actions.

		Removed 3 action hook basic tags: 1. `review_request_action_hooks` 2. `review_request_dropdown_action_hooks` 3. `diffviewer_action_hooks`
		Added a `BaseReviewRequestActionHook` for other hooks to inherit from.
		Changed the `_top_level_ids` from a `list` to a `deque`.
		Allowed menu actions to have their own children menu actions.
		Added a new `child_actions` basic tag.
		Both basic tags now have try/except blocks.
		Both base actions now have an unregister method (good for unit tests).
		`BaseReviewRequestActionHook` now also subclasses `AppliesToURLMixin`.



		Part 4: Allow extensions to register/unregister actions by ID.

		Added a public `unregister_actions()` method.
		Replaced `is_top_level` with `parent`.
		Added `_parent` so that extensions can unregister by ID.
		Added a public `register_actions()` method.
		Changed child action registration to happen as soon as they are passed in to the parent menu action's constructor.
		Added a `reset_actions()` method (useful in `shutdown()` for extensions) and changed `default_actions` to a factory method.
	+	Added a `ReviewRequestActionHook` model for providing callbacks.
	+	Updated the documentation for action hooks.



		TODO:

~		Start trying to implement my own JS extension that will make use of my own Djblets extension hook.
	~	Fix the issues that the mentors will inevitably find. =]
-		Start updating the documentation for the affected hooks.



		(Low Priority) Questions for the Mentors:

		How should nested menus be displayed?

Testing Done:

		Ran and passed the relevant reviewboard unit tests.
		Manually verified that all default actions display correctly in different scenarios. Test in all three tabs ("Reviews", "File", "Diff"). Test in normal as well as local sites. Test `DownloadDiffAction` with the slider (for interdiffs). Test as an authenticated user and as a guest. Test when the review request is pending review and is submitted.
		Added my own extension that provides its own actions in different ways and manually verified that each action displayed correctly in different scenarios. Test adding actions and menu actions via action-style dicts. Test adding actions and menu actions via action instances. Test adding menu actions that contain their own menu actions. Test that a menu action can override its children from rendering. Test unregistering a default action. Test unregistering a child action of a default menu action. Test registering a new child action on a default menu action.
	+	Added my own JS extension that provides its own callback and manually verified that the callback is correctly triggered when its corresponding action is clicked.
	+	Built the Sphinx docs and manually verified that my updates rendered correctly.

Diff:

Revision 15 (+1141 -252)

Show changes

	docs/manual/extending/coderef/index.rst
	docs/manual/extending/extensions/hooks/action-hook.rst
	docs/manual/extending/extensions/hooks/action-hooks.rst
	docs/manual/extending/extensions/hooks/index.rst
	reviewboard/staticbundles.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/extensions/templatetags/rb_extensions.py
	17 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/extensions/tests.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/default_actions.py
    reviewboard/staticbundles.py
    reviewboard/extensions/templatetags/rb_extensions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    docs/manual/extending/coderef/index.rst
    reviewboard/templates/reviews/ui/base.html
    reviewboard/static/rb/js/extensions/models/reviewRequestActionHookModel.js
    docs/manual/extending/extensions/hooks/index.rst
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/extensions/action_dropdown.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    docs/manual/extending/extensions/hooks/action-hook.rst
    reviewboard/templates/reviews/action.html
    docs/manual/extending/extensions/hooks/action-hooks.rst
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/extensions/tests.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/default_actions.py
    reviewboard/staticbundles.py
    reviewboard/extensions/templatetags/rb_extensions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    docs/manual/extending/coderef/index.rst
    reviewboard/templates/reviews/ui/base.html
    reviewboard/static/rb/js/extensions/models/reviewRequestActionHookModel.js
    docs/manual/extending/extensions/hooks/index.rst
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/extensions/action_dropdown.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    docs/manual/extending/extensions/hooks/action-hook.rst
    reviewboard/templates/reviews/action.html
    docs/manual/extending/extensions/hooks/action-hooks.rst
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

docs/manual/extending/extensions/hooks/action-hook.rst (Diff revision 15)
Show all issues
```
URL.
```
docs/manual/extending/extensions/hooks/action-hook.rst (Diff revision 15)
Show all issues
```
JavaScript,

``'#'``
```
docs/manual/extending/extensions/hooks/action-hook.rst (Diff revision 15)
Show all issues
```
``javascript:`` URL
```
docs/manual/extending/extensions/hooks/action-hook.rst (Diff revision 15)
Show all issues
```
2.6 is feature frozen. Can you change this to 2.7?
```
docs/manual/extending/extensions/hooks/action-hook.rst (Diff revision 15)
Show all issues
```
The right side can be reformatted to fit more on the first line.
```
docs/manual/extending/extensions/hooks/action-hook.rst (Diff revision 15)
Show all issues
```
Again, 2.7.
```
docs/manual/extending/extensions/hooks/action-hook.rst (Diff revision 15)
Show all issues
```
2.7
```
docs/manual/extending/extensions/hooks/action-hook.rst (Diff revision 15)
Show all issues
```
The from should be lined up with code.
```

reviewboard/extensions/hooks.py (Diff revision 15)

Show all issues

Please format this as:

from djblets.extensions.hooks import (AppliesToURLMixin, DataGridColumnsHook,
                                      ExtensionHook, ExtensionHookPoint,
                                      SignalHook, TemplateHook, URLHook)

reviewboard/extensions/hooks.py (Diff revision 15)

Show all issues

Format this as:

        super(BaseReviewRequestActionHook, self).__init__(
            extension, apply_to=apply_to, *args, **kwargs)

reviewboard/extensions/hooks.py (Diff revision 15)

Show all issues

Needs Args and Returns, e.g.

"""Single line summary.

Multi-line description.

Args:
    actions (list):
        A brief description of what actions is.

Returns:
    list:
    A brief description of what the return value is.
"""

reviewboard/extensions/hooks.py (Diff revision 15)
Show all issues
```
Blank line between these.
```

reviewboard/extensions/hooks.py (Diff revision 15)

Show all issues

So registered_actions is a temporary that is only used in this function, but we're creating a iterator out of it (via reversed()) and then turning it into a new list. This is rather inefficient.
Instead, can we do:

registered_actions.reverse()
return registered_actions


This way, registered_actions will be reversed in-place and so we won't waste time and memory creating a new list and freeing the old one.

reviewboard/extensions/hooks.py (Diff revision 15)
Show all issues
```
Needs Args and Returns.
```

reviewboard/extensions/hooks.py (Diff revision 15)

Show all issues

This should be a class member, or a global member. I'm not sure.
But it definitely shouldn't be in this method.

reviewboard/extensions/hooks.py (Diff revision 15)

Show all issues

Needs a better docstring to explain why this is necessary.

reviewboard/extensions/hooks.py (Diff revision 15)
Show all issues
```
Should be done in __init__.
```

reviewboard/extensions/hooks.py (Diff revision 15)

Show all issues

Please format as:


        super(ReviewRequestActionHook, self).__init__(
            extension, apply_to, actions, *args, **kwargs)

reviewboard/extensions/hooks.py (Diff revision 15)

Show all issues

Likewise, I don't think this should be in the def.

reviewboard/extensions/hooks.py (Diff revision 15)

Show all issues

Format as:

        return TempMenuAction([
            super(ReviewRequestDropdownActionHook, self)._to_action_instance(
                child_action_dict)
            for child_action_dict in action_dict['items']
        ])

reviewboard/extensions/tests.py (Diff revision 15)
Show all issues
```
Alphabetize these imports.
```
reviewboard/extensions/tests.py (Diff revision 15)
Show all issues
```
Put this on the previous line.
```
reviewboard/extensions/tests.py (Diff revision 15)
Show all issues
```
Put this on the previous line.
```
reviewboard/extensions/tests.py (Diff revision 15)
Show all issues
```
Put this on the previous line.
```
reviewboard/extensions/tests.py (Diff revision 15)
Show all issues
```
Put this on the previous line.
```
reviewboard/extensions/tests.py (Diff revision 15)
Show all issues
```
Put this on the previous line.
```
reviewboard/extensions/tests.py (Diff revision 15)
Show all issues
```
Put this on the previous line.
```
reviewboard/extensions/tests.py (Diff revision 15)
Show all issues
```
Needs a docstring.
```

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

Show all issues

Undo this.

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

Show all issues

Undo this.

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

Show all issues

Undo this.

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

Show all issues

The text should line up with the n in note.

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

Show all issues

Can you format this as:

parent (BaseReviewRequestMenuAction):
    The parent action instance of this action instance (optional).

reviewboard/reviews/actions.py (Diff revision 15)
Show all issues
```
Text should align with n in note.
```
reviewboard/reviews/actions.py (Diff revision 15)
Show all issues
```
Put on the previous line.
```
reviewboard/reviews/actions.py (Diff revision 15)
Show all issues
```
Should be a complete sentence.
```
reviewboard/reviews/actions.py (Diff revision 15)
Show all issues
```
Docstring should mention that this is a generator.
```

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

Show all issues

"""
Yields:
    BaseReviewREquestActions:
    All top-level registered review request action instances.
"""

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

Show all issues

Format as:

"""
KeyError:
    ...
"""

reviewboard/reviews/actions.py (Diff revision 15)
Show all issues
```
Should be complete sentence.
```
reviewboard/reviews/default_actions.py (Diff revision 15)
Show all issues
```
"Determines" over "Tracks"
```
reviewboard/reviews/default_actions.py (Diff revision 15)
Show all issues
```
Put this on the previous line please.
```
reviewboard/reviews/default_actions.py (Diff revision 15)
Show all issues
```
"otherwise" instead of "else".
```
reviewboard/reviews/default_actions.py (Diff revision 15)
Show all issues
```
Needs Returns.
```

Change Summary:

Address the issues raised by Barret and improve the ActionHook class.

Improved various wording/whitespace/bracket/docstring issues.
Changed Version 2.6 to Version 2.7.
Used ~.parent_dir. to help Sphinx find the correct hyperlinks.
Made ActionHook and other hooks more consistent with the docs/updates.
Extracted/improved the DictAction and DictMenuAction helper classes.
Made wording more consistent for ActionHook-style and

ReviewRequestDropdownActionHook-style dictionaries.

Description:

~		Overview
	~	This is an architectural change for Review Board 2.7 that affects
	+	extensions. Currently, there are some default review request actions
	+	(like "Close -> Submitted" or "Download Diff" or "Ship It!") that have
	+	been hardcoded into the template. Extensions can provide custom actions
	+	by passing dictionaries into a variety of ActionHooks.

~		This is an architectural change for Review Board 2.6 that affects extensions and requires both front-end (JavaScript) and back-end (Python) work.
~		Currently, there are some default actions (like "Review" or "Ship It!") that can be performed on review requests. Extensions can provide custom actions by subclassing various `ActionHooks`.
~		The goal of this project is to encapsulate this logic into a `ReviewRequestAction`. Instead of passing action-style dictionaries into the hooks, extensions will be able to provide custom actions by registering them as `ReviewRequestAction` instances. Furthermore, we will be able to remove any subset of the default actions by simply unregistering them.
	~	This change encapsulates this logic into a BaseReviewRequestAction
	~	class. Instead of passing ActionHook-style dictionaries into the hooks,
	~	extensions can now pass in BaseReviewRequestAction instances. Hardcoded
	+	actions have been ripped out from the template and retrofitted into a
	+	cleaner action registration system. Other new features include:

~
~
~		Part 1: Update the simplest default actions.
~
~		Added the `BaseReviewRequestAction` class (this was based on the `BaseReviewRequestField` class).
~		Added the `for_review_request_action` block tag (this was based on the `for_review_request_fieldset` block tag).
	~	Arbitrarily nested actions via BaseReviewRequestMenuActions.
	~	A _should_render() method that subclasses of BaseReviewRequestAction can override in order to implement complex action rendering behaviour.
	~	A register_actions() method that can be used to add a custom action to any of the default menu actions.
	~	An unregister_actions() method that can be used to remove any of the default actions.
	~	A reset_actions() method that extensions can call on shutdown.
	~	A ReviewRequestActionHookModel for providing action callbacks in JS.
-		Updated the "Review", "Ship It!", and "Download Diff" actions.
-		Added the `hidden` class variable for `DownloadDiffAction`.
-		Modified `view_diff.html` so that simple actions work for all tabs.
-		Switched from using a `SortedDict` to using a `dict` and a `list`.
-
-
-
-		Part 2: Update the default menu actions.
-
-		Refactored how URLs are initialized by calling `local_site_reverse()`.
-		Added the `BaseReviewRequestMenuAction` subclass.
-		Updated the "Close" and "Update" menu actions (and their children).
-		Changed from a block tag to a basic tag (`review_request_actions`).
-		Made actions responsible for rendering and registering.
-		Added a `menu_action.html` file that includes a new `action.html` file.
-
-
-
-		Part 3: Allow extensions to provide their own actions.
-
-		Removed 3 action hook basic tags: 1. `review_request_action_hooks` 2. `review_request_dropdown_action_hooks` 3. `diffviewer_action_hooks`
-		Added a `BaseReviewRequestActionHook` for other hooks to inherit from.
-		Changed the `_top_level_ids` from a `list` to a `deque`.
-		Allowed menu actions to have their own children menu actions.
-		Added a new `child_actions` basic tag.
-		Both basic tags now have try/except blocks.
-		Both base actions now have an unregister method (good for unit tests).
-		`BaseReviewRequestActionHook` now also subclasses `AppliesToURLMixin`.
-
-
-
-		Part 4: Allow extensions to register/unregister actions by ID.
-
-		Added a public `unregister_actions()` method.
-		Replaced `is_top_level` with `parent`.
-		Added `_parent` so that extensions can unregister by ID.
-		Added a public `register_actions()` method.
-		Changed child action registration to happen as soon as they are passed in to the parent menu action's constructor.
-		Added a `reset_actions()` method (useful in `shutdown()` for extensions) and changed `default_actions` to a factory method.
-		Added a `ReviewRequestActionHook` model for providing callbacks.
-		Updated the documentation for action hooks.
-
-
-
-		TODO:
-
-		Fix the issues that the mentors will inevitably find. =]
-
-
-
-		(Low Priority) Questions for the Mentors:
-
-		How should nested menus be displayed?

Testing Done:

		Ran and passed the relevant reviewboard unit tests.
~		Manually verified that all default actions display correctly in different scenarios. Test in all three tabs ("Reviews", "File", "Diff"). Test in normal as well as local sites. Test `DownloadDiffAction` with the slider (for interdiffs). Test as an authenticated user and as a guest. Test when the review request is pending review and is submitted.
	~	Manually verified that all default actions display correctly in different scenarios. Test in all three tabs ("Reviews", "File", "Diff"). Test in normal as well as local sites. Test DownloadDiffAction with the slider (for interdiffs). Test as an authenticated user and as a guest. Test when the review request is pending review and is submitted.
		Added my own extension that provides its own actions in different ways and manually verified that each action displayed correctly in different scenarios. Test adding actions and menu actions via action-style dicts. Test adding actions and menu actions via action instances. Test adding menu actions that contain their own menu actions. Test that a menu action can override its children from rendering. Test unregistering a default action. Test unregistering a child action of a default menu action. Test registering a new child action on a default menu action.
		Added my own JS extension that provides its own callback and manually verified that the callback is correctly triggered when its corresponding action is clicked.
		Built the Sphinx docs and manually verified that my updates rendered correctly.

Diff:

Revision 16 (+1258 -277)

Show changes

	docs/manual/extending/coderef/index.rst
	docs/manual/extending/extensions/hooks/action-hook.rst
	docs/manual/extending/extensions/hooks/action-hooks.rst
	docs/manual/extending/extensions/hooks/index.rst
	reviewboard/staticbundles.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/extensions/templatetags/rb_extensions.py
	17 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/extensions/tests.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/default_actions.py
    reviewboard/staticbundles.py
    reviewboard/extensions/templatetags/rb_extensions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    docs/manual/extending/coderef/index.rst
    reviewboard/templates/reviews/ui/base.html
    reviewboard/static/rb/js/extensions/models/reviewRequestActionHookModel.js
    docs/manual/extending/extensions/hooks/index.rst
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/extensions/action_dropdown.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    docs/manual/extending/extensions/hooks/action-hook.rst
    reviewboard/templates/reviews/action.html
    docs/manual/extending/extensions/hooks/action-hooks.rst
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/extensions/tests.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/default_actions.py
    reviewboard/staticbundles.py
    reviewboard/extensions/templatetags/rb_extensions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    docs/manual/extending/coderef/index.rst
    reviewboard/templates/reviews/ui/base.html
    reviewboard/static/rb/js/extensions/models/reviewRequestActionHookModel.js
    docs/manual/extending/extensions/hooks/index.rst
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/extensions/action_dropdown.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    docs/manual/extending/extensions/hooks/action-hook.rst
    reviewboard/templates/reviews/action.html
    docs/manual/extending/extensions/hooks/action-hooks.rst
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Change Summary:

Add a couple of missing "Returns" docstring sections.

Summary:

-	Review Request Action Registration.
+	Implement review request action registration.

Diff:

Revision 17 (+1264 -277)

Show changes

	docs/manual/extending/coderef/index.rst
	docs/manual/extending/extensions/hooks/action-hook.rst
	docs/manual/extending/extensions/hooks/action-hooks.rst
	docs/manual/extending/extensions/hooks/index.rst
	reviewboard/staticbundles.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/extensions/templatetags/rb_extensions.py
	17 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/extensions/tests.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/default_actions.py
    reviewboard/staticbundles.py
    reviewboard/extensions/templatetags/rb_extensions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    docs/manual/extending/coderef/index.rst
    reviewboard/templates/reviews/ui/base.html
    reviewboard/static/rb/js/extensions/models/reviewRequestActionHookModel.js
    docs/manual/extending/extensions/hooks/index.rst
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/extensions/action_dropdown.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    docs/manual/extending/extensions/hooks/action-hook.rst
    reviewboard/templates/reviews/action.html
    docs/manual/extending/extensions/hooks/action-hooks.rst
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/extensions/tests.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/default_actions.py
    reviewboard/staticbundles.py
    reviewboard/extensions/templatetags/rb_extensions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    docs/manual/extending/coderef/index.rst
    reviewboard/templates/reviews/ui/base.html
    reviewboard/static/rb/js/extensions/models/reviewRequestActionHookModel.js
    docs/manual/extending/extensions/hooks/index.rst
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/extensions/action_dropdown.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    docs/manual/extending/extensions/hooks/action-hook.rst
    reviewboard/templates/reviews/action.html
    docs/manual/extending/extensions/hooks/action-hooks.rst
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Change Summary:

Alphabetize the imports and clean up some of the tests for consistency.

To be consistent with the changed unit tests, t was renamed to
template and unnecessary calls to strip() were removed.

Diff:

Revision 18 (+1303 -315)

Show changes

	docs/manual/extending/coderef/index.rst
	docs/manual/extending/extensions/hooks/action-hook.rst
	docs/manual/extending/extensions/hooks/action-hooks.rst
	docs/manual/extending/extensions/hooks/index.rst
	reviewboard/staticbundles.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/extensions/templatetags/rb_extensions.py
	17 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/extensions/tests.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/default_actions.py
    reviewboard/staticbundles.py
    reviewboard/extensions/templatetags/rb_extensions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    docs/manual/extending/coderef/index.rst
    reviewboard/templates/reviews/ui/base.html
    reviewboard/static/rb/js/extensions/models/reviewRequestActionHookModel.js
    docs/manual/extending/extensions/hooks/index.rst
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/extensions/action_dropdown.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    docs/manual/extending/extensions/hooks/action-hook.rst
    reviewboard/templates/reviews/action.html
    docs/manual/extending/extensions/hooks/action-hooks.rst
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/extensions/tests.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/default_actions.py
    reviewboard/staticbundles.py
    reviewboard/extensions/templatetags/rb_extensions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    docs/manual/extending/coderef/index.rst
    reviewboard/templates/reviews/ui/base.html
    reviewboard/static/rb/js/extensions/models/reviewRequestActionHookModel.js
    docs/manual/extending/extensions/hooks/index.rst
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/extensions/action_dropdown.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    docs/manual/extending/extensions/hooks/action-hook.rst
    reviewboard/templates/reviews/action.html
    docs/manual/extending/extensions/hooks/action-hooks.rst
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

This is fantastic. Now, my review is large, because I'm in nit-picky mode, and want to get everything super-polished.

There are a couple of larger discussion points, which I cover below. Basically, the actions are setting state at runtime on themselves, but the actions are instances shared across every user rendering the page. This will lead to bad state at times, and so must be redesigned a little.

I also want to see more unit tests covering all the base action behavior, for registering/unregistering/iterating through actions (and handling all the success/error states), and unit tests for each default action covering the possible states. This will ensure things don't accidentally break down the road.

docs/manual/extending/extensions/hooks/action-hook.rst (Diff revision 18)

Show all issues

I'd rather not use a table for this (and require the shorthand for the actions). A bullet point list is sufficient.

Same below.

docs/manual/extending/extensions/hooks/action-hook.rst (Diff revision 18)

Show all issues

The description shouldn't be on the same line as the version number. Look around the docs codebase and you'll find some examples of how this should look.

docs/manual/extending/extensions/hooks/action-hook.rst (Diff revision 18)
Show all issues
```
Two blank lines between sections.
Same below.
```

reviewboard/extensions/hooks.py (Diff revision 18)

Show all issues

What's this for?

Ideally, all named lists of URLs should be defined in exactly one place, reviewboard.urls, so depending on what this is, maybe it should live there.

adriano Nov. 21, 2015, 10:59 a.m.

It's passed in to the apply_to attribute of ReviewRequestActionHook, so that it only renders on the review request page (and not the file attachment or diff viewer pages). I'll go ahead and move it there.

reviewboard/extensions/hooks.py (Diff revision 18)

Show all issues

This should spend some time describing what actions are and roughly how they work, like the old docs did.

reviewboard/extensions/hooks.py (Diff revision 18)

Show all issues

I'd prefer not to align like this. Instead:

* ``id``:
  The ID of the action (optional).


* ``label``:
  ...

(Make sure that renders correctly, though.. there are other docs that do something similar to this, so they can be used as a base.)

reviewboard/extensions/hooks.py (Diff revision 18)
Show all issues
```
Blank line between these.
```
reviewboard/extensions/hooks.py (Diff revision 18)
Show all issues
```
This needs a docstring.
```

reviewboard/extensions/hooks.py (Diff revision 18)

Show all issues

This definitely need more docs. I don't have a good sense of what this does.

reviewboard/extensions/hooks.py (Diff revision 18)

Show all issues

This is assuming some things about the data passed in, and if the keys are missing, the caller's going to get a less-than-nice exception. Ideally, this function should verify that action_dict has all the keys needed, and raise a KeyError with a friendly message if one is missing.

reviewboard/extensions/hooks.py (Diff revision 18)

Show all issues

This looks as if the self.label... is a third parameter. I'd instead do:

self.action_id = action_dict.get(
    'id',
    ...)

reviewboard/extensions/hooks.py (Diff revision 18)
Show all issues
```
This should also be fleshed out more.
```
reviewboard/extensions/hooks.py (Diff revision 18)
Show all issues
```
Same stuff as above.
```
reviewboard/extensions/hooks.py (Diff revision 18)
Show all issues
```
These are best referenced using :guilabel:.
```

reviewboard/extensions/hooks.py (Diff revision 18)

Show all issues

Maybe list these, so a caller doesn't have to look through the class hierarchy and check parent classes.

reviewboard/extensions/hooks.py (Diff revision 18)
Show all issues
```
Needs a docstring.
```

reviewboard/extensions/hooks.py (Diff revision 18)

Show all issues

Instead of setting these, just pass them inline where needed.

reviewboard/extensions/hooks.py (Diff revision 18)

Show all issues

This is kind of named awkwardly. From the name, I have no real sense of what it's doing, and the description doesn't really cover it any further.

Looking through the code, I believe it's taking the list passed in and creating a new list of normalized values. So, maybe _normalize_actions, with a description saying what it's taking in and what it's turning them into.

reviewboard/extensions/hooks.py (Diff revision 18)
Show all issues
```
:py:class: for dict.
```
reviewboard/extensions/hooks.py (Diff revision 18)
Show all issues
```
BaseReviewRequestAction
```

reviewboard/extensions/hooks.py (Diff revision 18)

Show all issues

I notice we're reversing here, and then reversing again later. What's the reason for that?

adriano Nov. 23, 2015, 3:52 a.m.

Since newly registered top-level actions are appended to the left of the other previously registered top-level actions, we must iterate through the actions in reverse. However, we don't want to mutate the original actions and we want to return the same (possibly normalized) actions in their original order. Hence, we reverse twice in this method.

I'll add the above explanation to the docstring. (Of course...it's possible that appending to the left of the _top_level_ids deque might be a bad design in the first place, and that we should perhaps be storing the top level action IDs in reverse instead. Do you have any thoughts?)

reviewboard/extensions/hooks.py (Diff revision 18)

Show all issues

What other values can this be? It might want to sanity-check that everything provided is an expected type of value, and raise an exception otherwise.

reviewboard/extensions/hooks.py (Diff revision 18)
Show all issues
```
Blank line between these.
```

reviewboard/extensions/hooks.py (Diff revision 18)

Show all issues

A function beginning with to_... generally means that the owning class would be converted to a thing. Let's go with _convert_action or something.

reviewboard/extensions/hooks.py (Diff revision 18)

Show all issues

Let's flesh this out a bit, mention that this means it'll only show up on the review request page containing reviews. I don't know that we want to mention "tabs" so much, as we're really dealing with pages, and who's to say whether tabs are the UI of choice for the product in all forms down the road.

Same below.

reviewboard/extensions/hooks.py (Diff revision 18)
Show all issues
```
Needs a docstring.
```
reviewboard/extensions/hooks.py (Diff revision 18)
Show all issues
```
Same above. We should list the possible types.
```
reviewboard/extensions/hooks.py (Diff revision 18)
Show all issues
```
Same as above with the formatting.
```

reviewboard/extensions/hooks.py (Diff revision 18)

Show all issues

This should go in an Example section, like:

"""

...

Example:
    .. code-block:: python

        actions = [{
            ...
        }]

reviewboard/extensions/hooks.py (Diff revision 18)
Show all issues
```
Same naming thoughts as above.
Also, needs docstrings.
```
reviewboard/extensions/hooks.py (Diff revision 18)
Show all issues
```
Same comments as above.
```

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

Show all issues

This is one of the older unit test suites in the codebase. Let's go ahead and move all action unit tests into a new class. This makes it easier for us to run the test suite only for action hook functionality.

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

Show all issues

I know the old unit test names didn't do this, but ideally, all new unit test names should reference the class being used for the test.
"Testing ReviewRequestActionHook with ..."
That sort of thing.

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

Show all issues

These are nice cleanups, but really should be broken out into its own change, to help keep this change slimmer.
Generally speaking, cleanup changes should be standalone. When running a git blame in the future, and seeing a change to this line, I'll wonder how it relates to the action hook work. If instead the change says it's standardizing names for something, then great!
That said, I don't know that this is cleanup that needs to happen (I'd rather see it done piece-meal as this mega-class is broken up into smaller test suite classes), so my preference would be to undo it.

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

Show all issues

This should go in an "Example:" section, like:

"""
...

Example:

    .. code-block:: python:

        class ....
"""


We've been placing these after things like attributes, as well.

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

Show all issues

These should be documented inline where defined on the class. "Attributes:" is more useful when documenting things only set in __init__ that are meant as public API.

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

Show all issues

Private attributes shouldn't be documented.

Same below.

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

Show all issues

If a subclass is meant to override this, it should not be a private function.

reviewboard/reviews/actions.py (Diff revision 18)
Show all issues
```
Should be in a "Note:" section instead.
```
reviewboard/reviews/actions.py (Diff revision 18)
Show all issues
```
Should be indented one level.
```
reviewboard/reviews/actions.py (Diff revision 18)
Show all issues
```
Same here.
```

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

Show all issues

You can combine these to be optimistic:

try:
    del _all_actions[...]
except KeyError:
    raise KeyError(...)

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

Show all issues

Not worth repeating these.

Same below.

reviewboard/reviews/actions.py (Diff revision 18)
Show all issues
```
Needs a docstring.
```

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

Show all issues

Blank line between these, generally, but this might be better as:

try:
    parent = _all_actions[parent_id]
except KeyError:
    raise KeyError(...)

reviewboard/reviews/actions.py (Diff revision 18)
Show all issues
```
Same as above.
```

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

Show all issues

Let's flesh this out just a bit, describing the resulting state.

May also need a warning that this will impact any extensions that have registered actions. (In fact, the extension hooks should probably become more bullet-proof, catching unregistration errors and logging them, instead of blowing up.)

reviewboard/reviews/default_actions.py (Diff revision 18)
Show all issues
```
Blank line before blocks.
```

reviewboard/reviews/default_actions.py (Diff revision 18)

Show all issues

Blank line between these.

Though, you might want to just combine the statements.

reviewboard/reviews/default_actions.py (Diff revision 18)
Show all issues
```
Let's have the third condition on its own line.
```

reviewboard/reviews/default_actions.py (Diff revision 18)

Show all issues

So I have a concern here, and I want to follow up on how this works.
We're registering instances, right? So if this is doing self.label = ... anywhere, it can pollute other renders (as the same codebase is going to be serving up multiple people). That's going to be a real problem.
Basically, if we're registering instances, rather than having an instance tied to that page load, then we cannot ever set anything on the class at runtime. We instead need functions that return the values needed.
In that case, we'd want get_label(), get_url(), etc. that take a context and return a value, much like should_render().

reviewboard/reviews/default_actions.py (Diff revision 18)
Show all issues
```
Blank line between these.
```

reviewboard/reviews/default_actions.py (Diff revision 18)

Show all issues

Here too.

Though, you may want to just combine the statements.

reviewboard/reviews/default_actions.py (Diff revision 18)
Show all issues
```
Parens around the comparison.
```

reviewboard/reviews/default_actions.py (Diff revision 18)

Show all issues

Description on the next line.

adriano Nov. 24, 2015, 10:12 p.m.

I talked to Barret about this on Slack here. Since this description fits on a single line, I think this is fine as it stands.

reviewboard/static/rb/js/extensions/models/reviewRequestActionHookModel.js (Diff revision 18)
Show all issues
```
This might need to provide an example.
```
reviewboard/static/rb/js/extensions/models/reviewRequestActionHookModel.js (Diff revision 18)
Show all issues
```
This should be documented.
```

reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 18)

Show all issues

If we're changing the names, let's get rid of -link and replace it with -action.

reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 18)
Show all issues
```
No space after function.
```

Change Summary:

Address some of the easier issues raised by Christian, like renaming.

Tables replaced with autosummary sections (abbreviations removed).
The versionadded sections are now aligned correctly.
Sections now have two newlines between them.
'-link' action_id suffixes have been renamed to '-action'.
ONLY_REVIEW_URL_NAMES has been moved to urls.py.
Various docstrings have been added/improved.
The :py:class: prefix was added to each dict.
The _should_render method is now public.
When using '(list of X)', the type X is now singular.
Added the use of :guilabel:.
Swapped the order of apply_to and actions in __init__.
Renamed _to_action_instance to _convert_action.
Unit tests now call reset_actions().
Reverted the extraneous code cleanup in tests.py.
Removed documentation of private variables.


TODO:

Address the remaining issues raised by Christian.
Redesign how actions set their state at runtime.
Add more unit tests.

Description:

		This is an architectural change for Review Board 2.7 that affects
		extensions. Currently, there are some default review request actions
		(like "Close -> Submitted" or "Download Diff" or "Ship It!") that have
		been hardcoded into the template. Extensions can provide custom actions
		by passing dictionaries into a variety of ActionHooks.

		This change encapsulates this logic into a BaseReviewRequestAction
		class. Instead of passing ActionHook-style dictionaries into the hooks,
		extensions can now pass in BaseReviewRequestAction instances. Hardcoded
		actions have been ripped out from the template and retrofitted into a
		cleaner action registration system. Other new features include:

		Arbitrarily nested actions via BaseReviewRequestMenuActions.
~		A _should_render() method that subclasses of BaseReviewRequestAction can override in order to implement complex action rendering behaviour.
	~	A should_render() method that subclasses of BaseReviewRequestAction can override in order to implement complex action rendering behaviour.
		A register_actions() method that can be used to add a custom action to any of the default menu actions.
		An unregister_actions() method that can be used to remove any of the default actions.
		A reset_actions() method that extensions can call on shutdown.
		A ReviewRequestActionHookModel for providing action callbacks in JS.

Diff:

Revision 19 (+1283 -290)

Show changes

	docs/manual/extending/coderef/index.rst
	docs/manual/extending/extensions/hooks/action-hook.rst
	docs/manual/extending/extensions/hooks/action-hooks.rst
	docs/manual/extending/extensions/hooks/index.rst
	reviewboard/staticbundles.py
	reviewboard/urls.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	18 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/extensions/tests.py
    reviewboard/urls.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/default_actions.py
    reviewboard/staticbundles.py
    reviewboard/extensions/templatetags/rb_extensions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    docs/manual/extending/coderef/index.rst
    reviewboard/templates/reviews/ui/base.html
    reviewboard/static/rb/js/extensions/models/reviewRequestActionHookModel.js
    docs/manual/extending/extensions/hooks/index.rst
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/extensions/action_dropdown.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    docs/manual/extending/extensions/hooks/action-hook.rst
    reviewboard/templates/reviews/action.html
    docs/manual/extending/extensions/hooks/action-hooks.rst
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/actions.py
    reviewboard/extensions/tests.py
    reviewboard/urls.py
    reviewboard/extensions/hooks.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/default_actions.py
    reviewboard/staticbundles.py
    reviewboard/extensions/templatetags/rb_extensions.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    docs/manual/extending/coderef/index.rst
    reviewboard/templates/reviews/ui/base.html
    reviewboard/static/rb/js/extensions/models/reviewRequestActionHookModel.js
    docs/manual/extending/extensions/hooks/index.rst
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/extensions/action_dropdown.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    docs/manual/extending/extensions/hooks/action-hook.rst
    reviewboard/templates/reviews/action.html
    docs/manual/extending/extensions/hooks/action-hooks.rst
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/templates/reviews/review_request_actions_secondary.html
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/templates/reviews/menu_action.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/extensions/tests.py (Diff revision 19)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Change Summary:

Address Christian's remaining easy issues.

Improved various docstrings, whitespace, and word choice.
Added ActionHook to the list of hooks in action-hook.rst.
Removed confusing "...is an instance of some subclass of..." references.
Renamed reset_actions -> clear_all_actions.
Removed all references to "tabs" (use "pages" instead).
Made DictAction and DictMenuAction private.
Renamed _register_and_get -> _register_actions and extracted out a

_normalize_action() method.
Exceptions are raised when invalid actions are provided.
"Example:" and "Note:" and "Warning:" sections are used.
Extracted the ActionHookTests into a separate class.
Made test docstrings more specific.
Class "Attributes:" section removed and added inline instead.
Optimistic try/except sections added.
Added an example of an extension that uses RB.ReviewRequestActionHook.


TODO:

Redesign how actions set their state at runtime.
Add more unit tests.

Diff:	Revisi

Implement review request action registration.

Information

Reviewers

Change Summary:

Description:

Part 1: Update the simplest hardcoded actions in the "Reviews" tab.

TODO:

Diff:

Change Summary:

Description:

Part 1: Update the simplest hardcoded actions in the "Reviews" tab.

Part 2: Update the simplest hardcoded actions in the "Diffs" tab.

TODO:

Diff:

Change Summary:

Description:

Part 1: Update the simplest hardcoded actions in the "Reviews" tab.

Part 1: Update the simplest hardcoded actions in the Reviews tab.

Part 2: Update the simplest hardcoded actions in the "Diffs" tab.

Part 2: Update the simplest hardcoded actions in the Diffs tab.

TODO:

Diff:

Change Summary:

Description:

Part 1: Update the simplest hardcoded actions in the Reviews tab.

Part 2: Update the simplest hardcoded actions in the Diffs tab.

Part 2: Update the simplest hardcoded actions in the Diff tab.

Part 3: Update the default menu actions.

TODO:

Testing Done:

Diff:

Change Summary:

Description:

Part 1: Update the simplest hardcoded actions in the Reviews tab.

Part 2: Update the simplest hardcoded actions in the Diff tab.

Part 3: Update the default menu actions.

TODO:

Diff:

Change Summary:

Description:

Part 1: Update the simplest hardcoded actions in the Reviews tab.

Part 2: Update the simplest hardcoded actions in the Diff tab.

Part 3: Update the default menu actions.

TODO:

Change Summary:

Description:

Part 1: Update the simplest hardcoded actions in the Reviews tab.

Part 1: Update the simplest default actions.

Part 2: Update the simplest hardcoded actions in the Diff tab.

Part 2: Update the default menu actions.

Part 3: Update the default menu actions.

TODO:

Diff:

Change Summary:

Description:

Overview

Part 1: Update the simplest default actions.

Part 2: Update the default menu actions.

Part 3: Allow extensions to provide their own actions.

TODO:

Testing Done:

Diff:

Change Summary:

Description:

Overview

Part 1: Update the simplest default actions.

Part 2: Update the default menu actions.

Part 3: Allow extensions to provide their own actions.

TODO:

Diff:

Change Summary:

Description:

Overview

Part 1: Update the simplest default actions.

Part 2: Update the default menu actions.

Part 3: Allow extensions to provide their own actions.

TODO:

(Low Priority) Questions for the Mentors:

Testing Done:

Diff:

Added a `reset_actions()` method (useful in `shutdown()` for extensions).