Implement review request action registration.
Review Request #7661 — Created Sept. 26, 2015 and submitted
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.
- 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.
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 | |
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot | |
Ideally, the should_render function should be a member funciton. This way, the people using this code can override the function … |
brennie | |
These should probably all be class attributes. |
brennie | |
Re: my earlier comment, this would be class EditReviewAction(ReviewRequestAction): """...""" def should_render(self, request): return request.user.is_authenticated() |
brennie | |
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 | |
Style nit: blank line between these. |
brennie | |
BLank line between these. |
brennie | |
This file should start with from __future__ import unicode_literals |
david | |
Docstrings should be in the imperative mood ("Return" vs "Returns") |
david | |
Imperative mood ("Populates" -> "Populate") |
david | |
Returns -> Return |
david | |
Registers -> Register |
david | |
This file should start with the unicode_literals import. |
david | |
This will need to be able to change based on what revision of the diff a user is looking at. |
david | |
Place the comment above this and prefix it with a colon, e.g.: #: A mapping of action IDs to action … |
brennie | |
Likewise here. However, this doesn't need a #: comment. |
brennie | |
Can you give this a list of arguments? e.g. """.... Args: arg (arg_type): Brief description of arg. """ This comment … |
brennie | |
:py:exc:KeyError |
brennie | |
Blank line between these. |
brennie | |
We prefer %-expressions for formatting over ''.format. |
brennie | |
You should have a constant on the class, e.g. class DownloadDiffAction(BaseReviewRequestAction): # ... #: Brief comment descrbing what this does. … |
brennie | |
Blank line between statement and block. |
brennie | |
This needs a better variable name. Perhaps, content? |
brennie | |
:py:class:`BaseReviewRequestAction` |
brennie | |
This should use reST clode blocks. E.g., For example: .. code-block:: python class UsedOnceAction(BaseReviewRequestAction): action_id = 'once' label = 'This … |
brennie | |
Blank line between these. |
brennie | |
This should go above args, e.g.: """Return whether or not this action should be rendered. The default implementation is to … |
brennie | |
Blank line between these. |
brennie | |
This should go above Args |
brennie | |
This should just be KeyError. The :py:exc:Exception stuff is for inline in strings, not in things that the docs expect … |
brennie | |
Needs a docstring. Likewise for all members of this class. |
brennie | |
Needs a docstring. Also, blank line between this and action_id. Likewise for encapsulated classes. |
brennie | |
This should be above args. |
brennie | |
No blank line between the docstring and the first line of code for functions. |
david | |
No blank line between the docstring and the first line of code for functions. |
david | |
Because you're manipulating this in a method, it probably shouldn't be data on the class. Can you create an __init__ … |
david | |
No blank line between the docstring and the first line of code for functions. |
david | |
No blank line between the docstring and the first line of code for functions. |
david | |
No blank line between the docstring and the first line of code for functions. |
david | |
No blank line between the docstring and the first line of code for functions. |
david | |
This comment confuses me. |
david | |
Can we call this review_request? We don't abbreviate to "rev" this way. |
david | |
You should be able to just use .display_id here. |
david | |
No blank line between docstring and code in functions. |
david | |
These two lines could be combined (you only use actions once) |
david | |
Hmm. I'm not sure how I feel about passing in a list and having the render() method append to it. … |
david | |
Let's put the whole if/endif thing on the same line. |
david | |
Add another blank line after this. |
david | |
I'm not sure why all of these are class methods. Can you clarify for me? |
david | |
Can we just pull these out into top-level classes? There's no real reason to hide them inside their parent menus, … |
david | |
"children_actions" is kind of redundant. How about either "children" or "child_actions"? |
david | |
Same with these (pull them out into top-level classes). |
david | |
This is probably not a terribly reliable way to detect this. It might be a lot better to pass the … |
david | |
We should maybe also pass the request into this, and introspect the url/view name, instead of trying to reverse engineer … |
david | |
Can we wrap this somehow? Maybe split the line after the href="..."? |
david | |
Col: 1 E302 expected 2 blank lines, found 1 |
reviewbot | |
Yes, it should be. __all__ defines which symbols get exported from the file (if there's no __all__, everything is exported). |
david | |
_() doesn't work this way. The l10n scanner looks at the source code and extracts all string literals which are … |
david | |
Same here. |
david | |
I would wrap this differently: super(DiffViewerActionHook, self).__init__( extension, apply_to, actions, *args, **kwargs) |
david | |
Add a blank line after this. |
david | |
Because we're intending to return a bool (and not a Repository), we should make this a truth test: return review_request.repository … |
david | |
Instead of using 'P' as a literal, you can compare this against ReviewRequest.PENDING_REVIEW. |
david | |
'_' imported but unused |
reviewbot | |
Mind fixing the spelling here while you're editing this file? |
david | |
It doesn't really matter, but I think it would be nicer to have this set after we register all the … |
david | |
Instead of just casting to bool, I think we should explicitly test review_request.repository is not None. |
david | |
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 | |
Same here. |
david | |
URL. |
brennie | |
JavaScript, ``'#'`` |
brennie | |
``javascript:`` URL |
brennie | |
2.6 is feature frozen. Can you change this to 2.7? |
brennie | |
The right side can be reformatted to fit more on the first line. |
brennie | |
Again, 2.7. |
brennie | |
2.7 |
brennie | |
The from should be lined up with code. |
brennie | |
Please format this as: from djblets.extensions.hooks import (AppliesToURLMixin, DataGridColumnsHook, ExtensionHook, ExtensionHookPoint, SignalHook, TemplateHook, URLHook) |
brennie | |
Format this as: super(BaseReviewRequestActionHook, self).__init__( extension, apply_to=apply_to, *args, **kwargs) |
brennie | |
Needs Args and Returns, e.g. """Single line summary. Multi-line description. Args: actions (list): A brief description of what actions is. … |
brennie | |
Blank line between these. |
brennie | |
So registered_actions is a temporary that is only used in this function, but we're creating a iterator out of it … |
brennie | |
Needs Args and Returns. |
brennie | |
This should be a class member, or a global member. I'm not sure. But it definitely shouldn't be in this … |
brennie | |
Needs a better docstring to explain why this is necessary. |
brennie | |
Should be done in __init__. |
brennie | |
Please format as: super(ReviewRequestActionHook, self).__init__( extension, apply_to, actions, *args, **kwargs) |
brennie | |
Likewise, I don't think this should be in the def. |
brennie | |
Format as: return TempMenuAction([ super(ReviewRequestDropdownActionHook, self)._to_action_instance( child_action_dict) for child_action_dict in action_dict['items'] ]) |
brennie | |
Alphabetize these imports. |
brennie | |
Put this on the previous line. |
brennie | |
Put this on the previous line. |
brennie | |
Put this on the previous line. |
brennie | |
Put this on the previous line. |
brennie | |
Put this on the previous line. |
brennie | |
Put this on the previous line. |
brennie | |
Needs a docstring. |
brennie | |
Undo this. |
brennie | |
Undo this. |
brennie | |
Undo this. |
brennie | |
The text should line up with the n in note. |
brennie | |
Can you format this as: parent (BaseReviewRequestMenuAction): The parent action instance of this action instance (optional). |
brennie | |
Text should align with n in note. |
brennie | |
Put on the previous line. |
brennie | |
Should be a complete sentence. |
brennie | |
Docstring should mention that this is a generator. |
brennie | |
""" Yields: BaseReviewREquestActions: All top-level registered review request action instances. """ |
brennie | |
Format as: """ KeyError: ... """ |
brennie | |
Should be complete sentence. |
brennie | |
"Determines" over "Tracks" |
brennie | |
Put this on the previous line please. |
brennie | |
"otherwise" instead of "else". |
brennie | |
Needs Returns. |
brennie | |
I'd rather not use a table for this (and require the shorthand for the actions). A bullet point list is … |
chipx86 | |
The description shouldn't be on the same line as the version number. Look around the docs codebase and you'll find … |
chipx86 | |
Two blank lines between sections. Same below. |
chipx86 | |
What's this for? Ideally, all named lists of URLs should be defined in exactly one place, reviewboard.urls, so depending on … |
chipx86 | |
This should spend some time describing what actions are and roughly how they work, like the old docs did. |
chipx86 | |
I'd prefer not to align like this. Instead: * ``id``: The ID of the action (optional). * ``label``: ... (Make … |
chipx86 | |
Blank line between these. |
chipx86 | |
This needs a docstring. |
chipx86 | |
This definitely need more docs. I don't have a good sense of what this does. |
chipx86 | |
This is assuming some things about the data passed in, and if the keys are missing, the caller's going to … |
chipx86 | |
This looks as if the self.label... is a third parameter. I'd instead do: self.action_id = action_dict.get( 'id', ...) |
chipx86 | |
This should also be fleshed out more. |
chipx86 | |
Same stuff as above. |
chipx86 | |
These are best referenced using :guilabel:. |
chipx86 | |
Maybe list these, so a caller doesn't have to look through the class hierarchy and check parent classes. |
chipx86 | |
Needs a docstring. |
chipx86 | |
Instead of setting these, just pass them inline where needed. |
chipx86 | |
This is kind of named awkwardly. From the name, I have no real sense of what it's doing, and the … |
chipx86 | |
:py:class: for dict. |
chipx86 | |
BaseReviewRequestAction |
chipx86 | |
I notice we're reversing here, and then reversing again later. What's the reason for that? |
chipx86 | |
What other values can this be? It might want to sanity-check that everything provided is an expected type of value, … |
chipx86 | |
Blank line between these. |
chipx86 | |
A function beginning with to_... generally means that the owning class would be converted to a thing. Let's go with … |
chipx86 | |
Let's flesh this out a bit, mention that this means it'll only show up on the review request page containing … |
chipx86 | |
Needs a docstring. |
chipx86 | |
Same above. We should list the possible types. |
chipx86 | |
Same as above with the formatting. |
chipx86 | |
This should go in an Example section, like: """ ... Example: .. code-block:: python actions = [{ ... }] |
chipx86 | |
Same naming thoughts as above. Also, needs docstrings. |
chipx86 | |
Same comments as above. |
chipx86 | |
This is one of the older unit test suites in the codebase. Let's go ahead and move all action unit … |
chipx86 | |
I know the old unit test names didn't do this, but ideally, all new unit test names should reference the … |
chipx86 | |
These are nice cleanups, but really should be broken out into its own change, to help keep this change slimmer. … |
chipx86 | |
This should go in an "Example:" section, like: """ ... Example: .. code-block:: python: class .... """ We've been placing … |
chipx86 | |
These should be documented inline where defined on the class. "Attributes:" is more useful when documenting things only set in … |
chipx86 | |
Private attributes shouldn't be documented. Same below. |
chipx86 | |
If a subclass is meant to override this, it should not be a private function. |
chipx86 | |
Should be in a "Note:" section instead. |
chipx86 | |
Should be indented one level. |
chipx86 | |
Same here. |
chipx86 | |
You can combine these to be optimistic: try: del _all_actions[...] except KeyError: raise KeyError(...) |
chipx86 | |
Not worth repeating these. Same below. |
chipx86 | |
Needs a docstring. |
chipx86 | |
Blank line between these, generally, but this might be better as: try: parent = _all_actions[parent_id] except KeyError: raise KeyError(...) |
chipx86 | |
Same as above. |
chipx86 | |
Let's flesh this out just a bit, describing the resulting state. May also need a warning that this will impact … |
chipx86 | |
Blank line before blocks. |
chipx86 | |
Blank line between these. Though, you might want to just combine the statements. |
chipx86 | |
Let's have the third condition on its own line. |
chipx86 | |
So I have a concern here, and I want to follow up on how this works. We're registering instances, right? … |
chipx86 | |
Blank line between these. |
chipx86 | |
Here too. Though, you may want to just combine the statements. |
chipx86 | |
Parens around the comparison. |
chipx86 | |
Description on the next line. |
chipx86 | |
This might need to provide an example. |
chipx86 | |
This should be documented. |
chipx86 | |
If we're changing the names, let's get rid of -link and replace it with -action. |
chipx86 | |
No space after function. |
chipx86 | |
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot | |
Col: 17 W503 line break before binary operator |
reviewbot | |
'register_actions' imported but unused |
reviewbot | |
'unregister_actions' imported but unused |
reviewbot | |
A form we're moving to, which reads and maintains better, is: ``id`` (optional): The ID of... ``label``: ... |
chipx86 | |
Only two backticks are needed. |
chipx86 | |
Any place we're referencing some key, we should use double backticks instead of making it bold. |
chipx86 | |
Not sure that we should document registering without a hook in an extension, since that means more that the extension … |
chipx86 | |
We should document that this works on all review request pages. |
chipx86 | |
This is probably not something we want to explicitly document here. Instead, we should somehow save and restore the actions … |
chipx86 | |
Like with the docs, let's do: ``id`` (optional): ... ``label``: ... |
chipx86 | |
Can you add ", optional" to the end of this, inside the parens? Same with any other arguments you have … |
chipx86 | |
Technically, this is a tuple. Same below. |
chipx86 | |
The type should be "callable". |
chipx86 | |
It's not generally good for classes to override functions like this. Let's define a proper method instead. Same below. |
chipx86 | |
I'm actually not sure, what is #. ? |
chipx86 | |
Should have ", optional". |
chipx86 | |
"Raises" should go after "Returns". Same below. |
chipx86 | |
We should use :py:class: here. Same below. |
chipx86 | |
No indentation for the description here. Same below. |
chipx86 | |
Maybe add a comment saying why we do this in reverse order. |
chipx86 | |
Can return reversed(registered_actions) instead. |
chipx86 | |
Should have :py:class on the class names in docstrings. Same elsewhere. |
chipx86 | |
Should have ", optional" |
chipx86 | |
Same as above with the formatting. |
chipx86 | |
No period on unit test docstrings. Same below. |
chipx86 | |
Let's specify the base class name. |
chipx86 | |
Let's specify the class name. |
chipx86 | |
I think "height" might actually be a bit too confusing. "Depth" worked better. "Height" usually means the dimensional height of … |
chipx86 | |
Maybe "as HTML." ? |
chipx86 | |
Should have ", optional". Then there's no need for "(optional)" at the end. |
chipx86 | |
Should wrap this in a try:, in case there's a failure, and log in except:. Then we can pop in … |
chipx86 | |
No need for the inner parens. |
chipx86 | |
% should go on the next line. Not sure RuntimeError is right here. Is this something that should never happen, … |
chipx86 | |
No need for the inner parens. |
chipx86 | |
", optional" |
chipx86 | |
"as HTML." Same with any others. |
chipx86 | |
", optional" |
chipx86 | |
This comment isn't about this line per se, but we have this nifty new Registry thing that Barret put together … |
chipx86 | |
", optional" |
chipx86 | |
No need for inner parens. |
chipx86 | |
Blank line between these. |
chipx86 | |
As mentioned earlier in the review, this is kind of dangerous for extensions, because they can stomp all over each … |
chipx86 | |
We can now use @register.simple_tag(takes_context=True). @basictag is newly-deprecated. Same below. |
chipx86 | |
Blank line between these. Same with ones below. |
chipx86 | |
We should probably call this in tearDown(). |
chipx86 | |
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 | |
Instead of a list (since this is "only" one thing), maybe call this main_review_request_url_name? |
chipx86 |
-
-
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).
-
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?
-
-
These shouldnt be on the class. They should be module-levels. See my other comment about the pages/forms stuff.
-
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 returnTrue
) to do complicated stuff with, e.g., therequest
. -
-
Re: my earlier comment, this would be
class EditReviewAction(ReviewRequestAction): """...""" def should_render(self, request): return request.user.is_authenticated()
-
You should look at
reviewboard/accounts/pages.py
andreviewboard/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. -
-
- 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.
- Added the
- Diff:
-
Revision 2 (+140 -7)
-
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
- 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 forDownloadDiffAction
. - Modified
view_diff.html
so that simple actions now work for both tabs.
- Imported
- 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 forDownloadDiffAction
.
+ - 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.
- Added the
- Diff:
-
Revision 3 (+175 -10)
-
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
-
-
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.
-
-
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. -
-
-
- 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.
- Switched to
- 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 forDownloadDiffAction
.
- 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.
- Added the
- Diff:
-
Revision 4 (+230 -10)
-
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 includesaction.html
.
- Changed from a block tag to a basic tag (
- 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 forDownloadDiffAction
.
- 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 includesaction.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.
- Added the
- 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)
-
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.
-
-
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
-
-
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: ...
-
-
-
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. -
-
-
- 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
todefault_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 forDownloadDiffAction
.
- 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 includesaction.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.
- Added the
- Diff:
-
Revision 6 (+364 -55)
-
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 forDownloadDiffAction
.
- 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 includesaction.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 newaction.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.
- Added the
-
This looks like a great start!
-
-
-
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? -
-
-
-
-
-
-
-
-
-
Hmm. I'm not sure how I feel about passing in a list and having the
render()
method append to it. How about we letrender()
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. -
- 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 forDownloadDiffAction
.
+ - 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 forDownloadDiffAction
.
- - 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 newaction.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.
- Added the
- Diff:
-
Revision 7 (+422 -55)
-
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 alist
to adeque
.
- Registering an action involves appending to the left of adeque
, so
we sometimes have to iterate through actions in reverse.
- Removed 3 action hook basic tags:
- 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 variousActionHooks
.
+ - 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 asReviewRequestAction
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 forDownloadDiffAction
.
~ - 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 adict
and alist
.
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 newaction.html
file.
~ - Added a
menu_action.html
file that includes a newaction.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 alist
to adeque
.
+ - 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.
- This is an architectural change for Review Board 2.6 that affects
- 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)
-
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.
- Removed the
- 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 variousActionHooks
.
- 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 asReviewRequestAction
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 forDownloadDiffAction
.
- Modified
view_diff.html
so that simple actions work for all tabs.
- Switched from using a
SortedDict
to using adict
and alist
.
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 newaction.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 alist
to adeque
.
~ - 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.
- This is an architectural change for Review Board 2.6 that affects
- Diff:
-
Revision 9 (+605 -172)
-
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
tochildren_actions
. - Improved docstring explanation for registering actions.
- Removed
BaseReviewRequestMenuAction
's redundant logic inrender()
.
- Renamed
- 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 variousActionHooks
.
- 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 asReviewRequestAction
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 forDownloadDiffAction
.
- Modified
view_diff.html
so that simple actions work for all tabs.
- Switched from using a
SortedDict
to using adict
and alist
.
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 newaction.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 alist
to adeque
.
- 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
orMenuActions
?
+ - How should nested menus be displayed?
- This is an architectural change for Review Board 2.6 that affects
- 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)
- 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
-
-
-
-
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.
-
-
-
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
. -
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.
-
- 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 subclassesAppliesToURLMixin
._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 variousActionHooks
.
~ - 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 asReviewRequestAction
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 asReviewRequestAction
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 forDownloadDiffAction
.
- Modified
view_diff.html
so that simple actions work for all tabs.
- Switched from using a
SortedDict
to using adict
and alist
.
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 newaction.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 alist
to adeque
.
- 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 subclassesAppliesToURLMixin
.
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
orMenuActions
?
- - How should nested menus be displayed?
- This is an architectural change for Review Board 2.6 that affects
- Diff:
-
Revision 11 (+747 -147)
-
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:
-
Fix whitespace and extend the base unit tests.
- Diff:
-
Revision 12 (+785 -143)
-
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!
-
Yes, it should be.
__all__
defines which symbols get exported from the file (if there's no__all__
, everything is exported). -
_()
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). -
-
I would wrap this differently:
super(DiffViewerActionHook, self).__init__( extension, apply_to, actions, *args, **kwargs)
-
-
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
-
- 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 withbool()
. - 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
withparent
. - Added
_parent
so that extensions can unregister by ID.
- Added a public
- 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 variousActionHooks
.
- 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 asReviewRequestAction
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 forDownloadDiffAction
.
- Modified
view_diff.html
so that simple actions work for all tabs.
- Switched from using a
SortedDict
to using adict
and alist
.
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 newaction.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 alist
to adeque
.
- 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 subclassesAppliesToURLMixin
.
+ Part 4: Allow extensions to register/unregister actions by ID.
+ + - Added a public
unregister_actions()
method.
+ - Replaced
is_top_level
withparent
.
+ - 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?
- This is an architectural change for Review Board 2.6 that affects
- 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)
-
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
-
-
-
-
It doesn't really matter, but I think it would be nicer to have this set after we register all the default actions.
-
Instead of just casting to bool, I think we should explicitly test
review_request.repository is not None
. -
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)
-
- Change Summary:
-
Added a
reset_actions()
method (useful inshutdown()
for extensions).- Replaced the
default_actions
variable with aget_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.
- Replaced the
- 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 variousActionHooks
.
- 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 asReviewRequestAction
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 forDownloadDiffAction
.
- Modified
view_diff.html
so that simple actions work for all tabs.
- Switched from using a
SortedDict
to using adict
and alist
.
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 newaction.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 alist
to adeque
.
- 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 subclassesAppliesToURLMixin
.
Part 4: Allow extensions to register/unregister actions by ID.
- Added a public
unregister_actions()
method.
- Replaced
is_top_level
withparent
.
- 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 inshutdown()
for
extensions) and changeddefault_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?
- This is an architectural change for Review Board 2.6 that affects
- Diff:
-
Revision 14 (+899 -146)
-
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
toaction-hook
. - Added
reviewboard.reviews.actions
and
reviewboard.reviews.default_actions
to thetoctree
. - 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.
- Used "note" blocks in various docstrings and fixed various whitespace
- 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 variousActionHooks
.
- 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 asReviewRequestAction
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 forDownloadDiffAction
.
- Modified
view_diff.html
so that simple actions work for all tabs.
- Switched from using a
SortedDict
to using adict
and alist
.
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 newaction.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 newaction.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 alist
to adeque
.
- 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 subclassesAppliesToURLMixin
.
Part 4: Allow extensions to register/unregister actions by ID.
- Added a public
unregister_actions()
method.
- Replaced
is_top_level
withparent
.
- 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 inshutdown()
for
extensions) and changeddefault_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?
- This is an architectural change for Review Board 2.6 that affects
- 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)
-
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
-
-
-
-
-
-
-
-
-
-
Please format this as:
from djblets.extensions.hooks import (AppliesToURLMixin, DataGridColumnsHook, ExtensionHook, ExtensionHookPoint, SignalHook, TemplateHook, URLHook)
-
Format this as:
super(BaseReviewRequestActionHook, self).__init__( extension, apply_to=apply_to, *args, **kwargs)
-
Needs
Args
andReturns
, 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. """
-
-
So
registered_actions
is a temporary that is only used in this function, but we're creating a iterator out of it (viareversed()
) 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. -
-
This should be a class member, or a global member. I'm not sure.
But it definitely shouldn't be in this method.
-
-
-
Please format as:
super(ReviewRequestActionHook, self).__init__( extension, apply_to, actions, *args, **kwargs)
-
-
Format as:
return TempMenuAction([ super(ReviewRequestDropdownActionHook, self)._to_action_instance( child_action_dict) for child_action_dict in action_dict['items'] ])
-
-
-
-
-
-
-
-
-
-
-
-
-
Can you format this as:
parent (BaseReviewRequestMenuAction): The parent action instance of this action instance (optional).
-
-
-
-
-
-
-
-
-
-
-
- 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
andDictMenuAction
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 variousActionHooks
.
~ - 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 asReviewRequestAction
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 forDownloadDiffAction
.
- - Modified
view_diff.html
so that simple actions work for all tabs.
- - Switched from using a
SortedDict
to using adict
and alist
.
- - - - 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 newaction.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 alist
to adeque
.
- - 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 subclassesAppliesToURLMixin
.
- - - - Part 4: Allow extensions to register/unregister actions by ID.
- - - Added a public
unregister_actions()
method.
- - Replaced
is_top_level
withparent
.
- - 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 inshutdown()
for
extensions) and changeddefault_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?
- This is an architectural change for Review Board 2.6 that affects
- 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)
-
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)
-
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 tostrip()
were removed. - Diff:
-
Revision 18 (+1303 -315)
-
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.
-
I'd rather not use a table for this (and require the shorthand for the actions). A bullet point list is sufficient.
Same below.
-
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.
-
-
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. -
This should spend some time describing what actions are and roughly how they work, like the old docs did.
-
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.)
-
-
-
-
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 aKeyError
with a friendly message if one is missing. -
This looks as if the
self.label...
is a third parameter. I'd instead do:self.action_id = action_dict.get( 'id', ...)
-
-
-
-
Maybe list these, so a caller doesn't have to look through the class hierarchy and check parent classes.
-
-
-
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. -
-
-
-
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.
-
-
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. -
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.
-
-
-
-
This should go in an Example section, like:
""" ... Example: .. code-block:: python actions = [{ ... }]
-
-
-
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.
-
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.
-
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.
-
This should go in an "Example:" section, like:
""" ... Example: .. code-block:: python: class .... """
We've been placing these after things like attributes, as well.
-
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. -
-
-
-
-
-
You can combine these to be optimistic:
try: del _all_actions[...] except KeyError: raise KeyError(...)
-
-
-
Blank line between these, generally, but this might be better as:
try: parent = _all_actions[parent_id] except KeyError: raise KeyError(...)
-
-
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.)
-
-
-
-
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 likeshould_render()
. -
-
-
-
-
-
-
-
- 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 tourls.py
.- Various docstrings have been added/improved.
- The
:py:class:
prefix was added to eachdict
. - 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
andactions
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)
-
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
-
- Change Summary:
-
Address Christian's remaining easy issues.
- Improved various docstrings, whitespace, and word choice.
- Added
ActionHook
to the list of hooks inaction-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
andDictMenuAction
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:
-
Revision 20 (+1582 -328)
-
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
-
- Change Summary:
-
Add the remaining
ActionTests
,DefaultActionTests
, andActionHookTests
.- Added unit tests that cover registering/unregistering actions.
- Added unit tests that cover success/error states.
- Added unit tests that cover
get_url
(inview-diff
, etc).
Add default action unit tests that cover
should_render()
.- Rearranged the order of default actions to make more sense.
- Added the
ActionTests
andDefaultActionTests
classes.
Redesign actions so that their state is only set once on initialization.
- 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 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 clear_all_actions() method that extensions can call on shutdown.
- 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.- 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 types of pages ("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.
~ - 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.
- Diff:
-
Revision 21 (+2260 -328)
-
Tool: Pyflakes Processed Files: reviewboard/reviews/actions.py reviewboard/extensions/tests.py reviewboard/reviews/tests.py reviewboard/extensions/hooks.py reviewboard/reviews/templatetags/reviewtags.py reviewboard/reviews/default_actions.py reviewboard/urls.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/reviews/tests.py reviewboard/extensions/hooks.py reviewboard/reviews/templatetags/reviewtags.py reviewboard/reviews/default_actions.py reviewboard/urls.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
<