diff --git a/reviewboard/actions/registry.py b/reviewboard/actions/registry.py index 28ec422f0a21c6662dde40ae7dc12d24d4dceb22..d0c10992e1060e3a027fdc6e9490f49199dff1eb 100644 --- a/reviewboard/actions/registry.py +++ b/reviewboard/actions/registry.py @@ -48,9 +48,43 @@ class ActionsRegistry(OrderedRegistry): reviewboard.actions.base.BaseAction: The built-in actions. """ + from reviewboard.reviews.actions import (AddGeneralCommentAction, + ArchiveAction, + ArchiveMenuAction, + CloseMenuAction, + CloseCompletedAction, + CloseDiscardedAction, + DeleteAction, + DownloadDiffAction, + LegacyEditReviewAction, + LegacyShipItAction, + MuteAction, + StarAction, + UpdateMenuAction, + UploadDiffAction, + UploadFileAction) + # The order here is important, and will reflect the order that items # appear in the UI. builtin_actions: List[BaseAction] = [ + # Review request actions (left side) + StarAction(), + ArchiveMenuAction(), + ArchiveAction(), + MuteAction(), + + # Review request actions (right side) + CloseMenuAction(), + CloseCompletedAction(), + CloseDiscardedAction(), + DeleteAction(), + UpdateMenuAction(), + UploadDiffAction(), + UploadFileAction(), + DownloadDiffAction(), + LegacyEditReviewAction(), + LegacyShipItAction(), + AddGeneralCommentAction(), ] for action in builtin_actions: diff --git a/reviewboard/reviews/actions.py b/reviewboard/reviews/actions.py index 75ea54ccd4fa3ef97f9e9c077aab01ab812f4bf8..d64715f1902b65ecde2482a42a2b0426720fee3d 100644 --- a/reviewboard/reviews/actions.py +++ b/reviewboard/reviews/actions.py @@ -1,105 +1,224 @@ -from collections import deque +"""Actions for the reviews app.""" -from django.template.loader import render_to_string +from __future__ import annotations -from reviewboard.reviews.errors import DepthLimitExceededError +from typing import Iterable, List, Optional, TYPE_CHECKING, Union +from django.http import HttpRequest +from django.template import Context +from django.utils.translation import gettext_lazy as _ -#: The maximum depth limit of any action instance. -MAX_DEPTH_LIMIT = 2 +from reviewboard.actions import (AttachmentPoint, + BaseAction, + BaseMenuAction, + actions_registry) +from reviewboard.admin.read_only import is_site_read_only_for +from reviewboard.deprecation import RemovedInReviewBoard70Warning +from reviewboard.reviews.features import (general_comments_feature, + unified_banner_feature) +from reviewboard.reviews.models import ReviewRequest +from reviewboard.site.urlresolvers import local_site_reverse +from reviewboard.urls import reviewable_url_names, review_request_url_names -#: The mapping of all action IDs to their corresponding action instances. -_all_actions = {} +if TYPE_CHECKING: + # This is available only in django-stubs. + from django.utils.functional import _StrOrPromise -#: All top-level action IDs (in their left-to-right order of appearance). -_top_level_ids = deque() -#: Determines if the default action instances have been populated yet. -_populated = False +all_review_request_url_names = reviewable_url_names + review_request_url_names -class BaseReviewRequestAction(object): - """A base class for an action that can be applied to a review request. +class AddGeneralCommentAction(BaseAction): + """The action for adding a general comment. - Creating an action requires subclassing :py:class:`BaseReviewRequestAction` - and overriding any fields/methods as desired. Different instances of the - same subclass can also override the class fields with their own instance - fields. + Version Added: + 6.0 + """ - Example: - .. code-block:: python + action_id = 'add-general-comment' + label = _('Add General Comment') + apply_to = all_review_request_url_names - class UsedOnceAction(BaseReviewRequestAction): - action_id = 'once' - label = 'This is used once.' + def should_render( + self, + *, + context: Context, + ) -> bool: + """Return whether this action should render. - class UsedMultipleAction(BaseReviewRequestAction): - def __init__(self, action_id, label): - super(UsedMultipleAction, self).__init__() + This differs from :py:attr:`hidden` in that hidden actions still render + but are hidden by CSS, whereas if this returns ``False`` the action + will not be included in the DOM at all. - self.action_id = 'repeat-' + action_id - self.label = 'This is used multiple times,' + Args: + context (django.template.Context): + The current rendering context. - Note: - Since the same action will be rendered for multiple different users in - a multithreaded environment, the action state should not be modified - after initialization. If we want different action attributes at - runtime, then we can override one of the getter methods (such as - :py:meth:`get_label`), which by default will simply return the original - attribute from initialization. - """ + Returns: + bool: + ``True`` if the action should render. + """ + request = context['request'] + user = request.user - #: The ID of this action. Must be unique across all types of actions and - #: menu actions, at any depth. - action_id = None + return (super().should_render(context=context) and + user.is_authenticated and + not is_site_read_only_for(user) and + general_comments_feature.is_enabled(request=request)) - #: The label that displays this action to the user. - label = None - #: The URL to invoke if this action is clicked. - url = '#' +class CloseMenuAction(BaseMenuAction): + """A menu for closing the review request. - #: Determines if this action should be initially hidden to the user. - hidden = False + Version Added: + 6.0 + """ - def __init__(self): - """Initialize this action. + action_id = 'close-menu' + label = _('Close') + apply_to = all_review_request_url_names - By default, actions are top-level and have no children. + def should_render( + self, + *, + context: Context, + ) -> bool: + """Return whether this action should render. + + This differs from :py:attr:`hidden` in that hidden actions still render + but are hidden by CSS, whereas if this returns ``False`` the action + will not be included in the DOM at all. + + Args: + context (django.template.Context): + The current rendering context. + + Returns: + bool: + ``True`` if the action should render. """ - self._parent = None - self._max_depth = 0 + request = context['request'] + review_request = context.get('review_request') + perms = context.get('perms') + user = request.user + + return (super().should_render(context=context) and + review_request is not None and + review_request.status == ReviewRequest.PENDING_REVIEW and + not is_site_read_only_for(user) and + (request.user.pk == review_request.submitter_id or + (bool(perms) and + perms['reviews']['can_change_status'] and + review_request.public))) + + +class CloseCompletedAction(BaseAction): + """The action to close a review request as completed. + + Version Added: + 6.0 + """ - def copy_to_dict(self, context): - """Copy this action instance to a dictionary. + action_id = 'close-completed' + parent_id = CloseMenuAction.action_id + label = _('Completed') + apply_to = all_review_request_url_names + + def should_render( + self, + *, + context: Context, + ) -> bool: + """Return whether this action should render. + + This differs from :py:attr:`hidden` in that hidden actions still render + but are hidden by CSS, whereas if this returns ``False`` the action + will not be included in the DOM at all. Args: context (django.template.Context): - The collection of key-value pairs from the template. + The current rendering context. Returns: - dict: The corresponding dictionary. + bool: + ``True`` if the action should render. """ - return { - 'action_id': self.action_id, - 'label': self.get_label(context), - 'url': self.get_url(context), - 'hidden': self.get_hidden(context), - } + review_request = context.get('review_request') + + return (super().should_render(context=context) and + review_request is not None and + review_request.public and + not is_site_read_only_for(context['request'].user)) + + +class CloseDiscardedAction(BaseAction): + """The action to close a review request as discarded. + + Version Added: + 6.0 + """ + + action_id = 'close-discarded' + parent_id = CloseMenuAction.action_id + label = _('Discarded') + apply_to = all_review_request_url_names + + +class DeleteAction(BaseAction): + """The action to permanently delete a review request. + + Version Added: + 6.0 + """ + + action_id = 'delete-review-request' + parent_id = CloseMenuAction.action_id + label = _('Delete Permanently') + apply_to = all_review_request_url_names + + def should_render( + self, + *, + context: Context, + ) -> bool: + """Return whether this action should render. - def get_label(self, context): - """Return this action's label. + This differs from :py:attr:`hidden` in that hidden actions still render + but are hidden by CSS, whereas if this returns ``False`` the action + will not be included in the DOM at all. Args: context (django.template.Context): - The collection of key-value pairs from the template. + The current rendering context. Returns: - unicode: The label that displays this action to the user. + bool: + ``True`` if the action should render. """ - return self.label + perms = context.get('perms') + + return (super().should_render(context=context) and + bool(perms) and + perms['reviews']['delete_reviewrequest'] and + not is_site_read_only_for(context['request'].user)) + + +class DownloadDiffAction(BaseAction): + """The action to download a diff. + + Version Added: + 6.0 + """ + + action_id = 'download-diff' + label = _('Download Diff') + apply_to = all_review_request_url_names - def get_url(self, context): + def get_url( + self, + *, + context: Context, + ) -> str: """Return this action's URL. Args: @@ -107,39 +226,494 @@ class BaseReviewRequestAction(object): The collection of key-value pairs from the template. Returns: - unicode: The URL to invoke if this action is clicked. + str: + The URL to invoke if this action is clicked. """ - return self.url + request = context['request'] + + # We want to use a relative URL in the diff viewer as we will not be + # re-rendering the page when switching between revisions. + from reviewboard.urls import diffviewer_url_names + match = request.resolver_match + + if match.url_name in diffviewer_url_names: + return 'raw/' + + return local_site_reverse( + 'raw-diff', + request, + kwargs={ + 'review_request_id': context['review_request'].display_id, + }) + + def get_visible( + self, + *, + context: Context, + ) -> bool: + """Return whether the action should start visible or not. - def get_hidden(self, context): - """Return whether this action should be initially hidden to the user. + Args: + context (django.template.Context): + The current rendering context. + + Returns: + bool: + ``True`` if the action should start visible. ``False``, otherwise. + """ + from reviewboard.urls import diffviewer_url_names + match = context['request'].resolver_match + + if match.url_name in diffviewer_url_names: + return match.url_name != 'view-interdiff' + + return super().get_visible(context=context) + + def should_render( + self, + *, + context: Context, + ) -> bool: + """Return whether this action should render. + + This differs from :py:attr:`hidden` in that hidden actions still render + but are hidden by CSS, whereas if this returns ``False`` the action + will not be included in the DOM at all. Args: context (django.template.Context): - The collection of key-value pairs from the template. + The current rendering context. Returns: - bool: Whether this action should be initially hidden to the user. + bool: + ``True`` if the action should render. """ - return self.hidden + from reviewboard.urls import diffviewer_url_names + match = context['request'].resolver_match + + # If we're on a diff viewer page, then this should be initially + # rendered, but might be hidden. + if match.url_name in diffviewer_url_names: + return True + + review_request = context.get('review_request') - def should_render(self, context): - """Return whether or not this action should render. + return (super().should_render(context=context) and + review_request is not None and + review_request.repository_id is not None and + review_request.diffset_history.diffsets.exists()) - The default implementation is to always render the action everywhere. + +class LegacyEditReviewAction(BaseAction): + """The old-style "Edit Review" action. + + This exists within the review request actions area, and will be supplanted + by the new action in the Review menu in the unified banner. + + Version Added: + 6.0 + """ + + action_id = 'edit-review' + label = _('Review') + apply_to = all_review_request_url_names + + def should_render( + self, + *, + context: Context, + ) -> bool: + """Return whether this action should render. + + This differs from :py:attr:`hidden` in that hidden actions still render + but are hidden by CSS, whereas if this returns ``False`` the action + will not be included in the DOM at all. + + Args: + context (django.template.Context): + The current rendering context. + + Returns: + bool: + ``True`` if the action should render. + """ + request = context['request'] + user = request.user + + return (super().should_render(context=context) and + user.is_authenticated and + not is_site_read_only_for(user) and + not unified_banner_feature.is_enabled(request=request)) + + +class LegacyShipItAction(BaseAction): + """The old-style "Ship It" action. + + This exists within the review request actions area, and will be supplanted + by the new action in the Review menu in the unified banner. + + Version Added: + 6.0 + """ + + action_id = 'ship-it' + label = _('Ship It!') + apply_to = all_review_request_url_names + + def should_render( + self, + *, + context: Context, + ) -> bool: + """Return whether this action should render. + + This differs from :py:attr:`hidden` in that hidden actions still render + but are hidden by CSS, whereas if this returns ``False`` the action + will not be included in the DOM at all. Args: context (django.template.Context): - The collection of key-value pairs available in the template - just before this action is to be rendered. + The current rendering context. Returns: - bool: Determines if this action should render. + bool: + ``True`` if the action should render. """ - return True + request = context['request'] + user = request.user + + return (super().should_render(context=context) and + user.is_authenticated and + not is_site_read_only_for(user) and + not unified_banner_feature.is_enabled(request=request)) + + +class UpdateMenuAction(BaseMenuAction): + """A menu for updating the review request. + + Version Added: + 6.0 + """ + + action_id = 'update-menu' + label = _('Update') + apply_to = all_review_request_url_names + + def should_render( + self, + *, + context: Context, + ) -> bool: + """Return whether this action should render. + + This differs from :py:attr:`hidden` in that hidden actions still render + but are hidden by CSS, whereas if this returns ``False`` the action + will not be included in the DOM at all. + + Args: + context (django.template.Context): + The current rendering context. + + Returns: + bool: + ``True`` if the action should render. + """ + request = context['request'] + review_request = context.get('review_request') + perms = context.get('perms') + user = request.user + + return (super().should_render(context=context) and + review_request is not None and + review_request.status == ReviewRequest.PENDING_REVIEW and + not is_site_read_only_for(user) and + (user.pk == review_request.submitter_id or + (bool(perms) and perms['reviews']['can_edit_reviewrequest']))) + + +class UploadDiffAction(BaseAction): + """The action to update or upload a diff. + + Version Added: + 6.0 + """ + + action_id = 'upload-diff' + parent_id = UpdateMenuAction.action_id + apply_to = all_review_request_url_names + + def get_label( + self, + *, + context: Context, + ) -> _StrOrPromise: + """Return the label for the action. + + Args: + context (django.template.Context): + The current rendering context. + + Returns: + str: + The label to use for the action. + """ + review_request = context['review_request'] + draft = review_request.get_draft(context['request'].user) + + if (draft and draft.diffset) or review_request.get_diffsets(): + return _('Update Diff') + else: + return _('Upload Diff') + + def should_render( + self, + *, + context: Context, + ) -> bool: + """Return whether this action should render. + + This differs from :py:attr:`hidden` in that hidden actions still render + but are hidden by CSS, whereas if this returns ``False`` the action + will not be included in the DOM at all. + + Args: + context (django.template.Context): + The current rendering context. + + Returns: + bool: + ``True`` if the action should render. + """ + request = context['request'] + review_request = context.get('review_request') + perms = context.get('perms') + user = request.user + + return (super().should_render(context=context) and + review_request is not None and + review_request.repository_id is not None and + not is_site_read_only_for(user) and + (user.pk == review_request.submitter_id or + (bool(perms) and + perms['reviews']['can_edit_reviewrequest']))) + + +class UploadFileAction(BaseAction): + """The action to upload a new file attachment. + + Version Added: + 6.0 + """ + + action_id = 'upload-file' + parent_id = UpdateMenuAction.action_id + label = _('Add File') + apply_to = all_review_request_url_names + + def should_render( + self, + *, + context: Context, + ) -> bool: + """Return whether this action should render. + + This differs from :py:attr:`hidden` in that hidden actions still render + but are hidden by CSS, whereas if this returns ``False`` the action + will not be included in the DOM at all. + + Args: + context (django.template.Context): + The current rendering context. + + Returns: + bool: + ``True`` if the action should render. + """ + request = context['request'] + review_request = context.get('review_request') + perms = context.get('perms') + user = request.user + + return (super().should_render(context=context) and + review_request is not None and + review_request.status == ReviewRequest.PENDING_REVIEW and + not is_site_read_only_for(user) and + (user.pk == review_request.submitter_id or + (bool(perms) and + perms['reviews']['can_edit_reviewrequest']))) + + +class StarAction(BaseAction): + """The action to star a review request. + + Version Added: + 6.0 + """ + + action_id = 'star-review-request' + attachment = AttachmentPoint.REVIEW_REQUEST_LEFT + label = '' + template_name = 'reviews/star_action.html' + apply_to = all_review_request_url_names + + def should_render( + self, + *, + context: Context, + ) -> bool: + """Return whether this action should render. + + This differs from :py:attr:`hidden` in that hidden actions still render + but are hidden by CSS, whereas if this returns ``False`` the action + will not be included in the DOM at all. + + Args: + context (django.template.Context): + The current rendering context. + + Returns: + bool: + ``True`` if the action should render. + """ + request = context['request'] + review_request = context.get('review_request') + user = request.user + + return (user.is_authenticated and + review_request is not None and + review_request.public and + not is_site_read_only_for(user) and + super().should_render(context=context)) + + +class ArchiveMenuAction(BaseMenuAction): + """A menu for managing the visibility state of the review request. + + Version Added: + 6.0 + """ + + action_id = 'archive-menu' + attachment = AttachmentPoint.REVIEW_REQUEST_LEFT + label = '' + template_name = 'reviews/archive_menu_action.html' + js_view_class = 'RB.ArchiveMenuActionView' + apply_to = all_review_request_url_names + + def should_render( + self, + *, + context: Context, + ) -> bool: + """Return whether this action should render. + + This differs from :py:attr:`hidden` in that hidden actions still render + but are hidden by CSS, whereas if this returns ``False`` the action + will not be included in the DOM at all. + + Args: + context (django.template.Context): + The current rendering context. + + Returns: + bool: + ``True`` if the action should render. + """ + request = context['request'] + review_request = context.get('review_request') + user = request.user + + return (user.is_authenticated and + review_request is not None and + review_request.public and + not is_site_read_only_for(user) and + super().should_render(context=context)) + + +class ArchiveAction(BaseAction): + """An action for archiving the review request. + + Version Added: + 6.0 + """ + + action_id = 'archive' + parent_id = ArchiveMenuAction.action_id + attachment = AttachmentPoint.REVIEW_REQUEST_LEFT + apply_to = all_review_request_url_names + label = '' + template_name = 'reviews/archive_action.html' + js_view_class = 'RB.ArchiveActionView' + + +class MuteAction(BaseAction): + """An action for muting the review request. + + Version Added: + 6.0 + """ + + action_id = 'mute' + parent_id = ArchiveMenuAction.action_id + attachment = AttachmentPoint.REVIEW_REQUEST_LEFT + apply_to = all_review_request_url_names + label = '' + template_name = 'reviews/archive_action.html' + js_view_class = 'RB.MuteActionView' + + +class BaseReviewRequestAction(BaseAction): + """A base class for an action that can be applied to a review request. + + Creating an action requires subclassing :py:class:`BaseReviewRequestAction` + and overriding any fields/methods as desired. Different instances of the + same subclass can also override the class fields with their own instance + fields. + + Example: + .. code-block:: python + + class UsedOnceAction(BaseReviewRequestAction): + action_id = 'once' + label = 'This is used once.' + + class UsedMultipleAction(BaseReviewRequestAction): + def __init__(self, action_id, label): + super().__init__() + + self.action_id = 'repeat-' + action_id + self.label = 'This is used multiple times,' + + Note: + Since the same action will be rendered for multiple different users in + a multithreaded environment, the action state should not be modified + after initialization. If we want different action attributes at + runtime, then we can override one of the getter methods (such as + :py:meth:`get_label`), which by default will simply return the original + attribute from initialization. + + Deprecated: + 6.0: + New code should be written using + :py:class:`reviewboard.actions.base.BaseAction`. This class will be + removed in 7.0. + """ + + def __init__(self) -> None: + """Initialize this action. + + By default, actions are top-level and have no children. + """ + RemovedInReviewBoard70Warning.warn( + 'BaseReviewRequestAction is deprecated and will be removed in ' + 'Review Board 7.0. Please update your code to use ' + 'reviewboard.actions.base.BaseAction') + + super().__init__() + self._parent = None @property - def max_depth(self): + def max_depth(self) -> int: """Lazily compute the max depth of any action contained by this action. Top-level actions have a depth of zero, and child actions have a depth @@ -151,49 +725,62 @@ class BaseReviewRequestAction(object): of a UI element. Returns: - int: The max depth of any action contained by this action. + int: + The max depth of any action contained by this action. """ - return self._max_depth + return 0 - def reset_max_depth(self): + def reset_max_depth(self) -> None: """Reset the max_depth of this action and all its ancestors to zero.""" - self._max_depth = 0 - - if self._parent: - self._parent.reset_max_depth() + # The max depth is now calculated on the fly, so this is a no-op. + pass - def render(self, context, action_key='action', - template_name='reviews/action.html'): - """Render this action instance and return the content as HTML. + def get_extra_context( + self, + *, + request: HttpRequest, + context: Context, + ) -> dict: + """Return extra template context for the action. Args: - context (django.template.Context): - The collection of key-value pairs that is passed to the - template in order to render this action. + request (django.http.HttpRequest): + The HTTP request from the client. - action_key (unicode, optional): - The key to be used for this action in the context map. - - template_name (unicode, optional): - The name of the template to be used for rendering this action. + context (django.template.Context): + The current rendering context. Returns: - unicode: The action rendered in HTML. + dict: + Extra context to use when rendering the action's template. """ - content = '' + data = super().get_extra_context(request=request, context=context) - if self.should_render(context): - context.push() + if self.template_name != 'actions/action.html': + data['action'] = self.copy_to_dict(context) - try: - context[action_key] = self.copy_to_dict(context) - content = render_to_string(template_name, context.flatten()) - finally: - context.pop() + return data - return content + def copy_to_dict( + self, + context: Context, + ) -> dict: + """Copy this action instance to a dictionary. - def register(self, parent=None): + This is a legacy implementation left to maintain compatibility with + custom templates. + """ + return { + 'action_id': self.action_id, + 'hidden': not self.get_visible(context=context), + 'label': self.get_label(context=context), + 'url': self.get_url(context=context), + } + + def register( + self, + parent: Optional[BaseReviewRequestMenuAction] = None, + ) -> None: """Register this review request action instance. Note: @@ -214,24 +801,12 @@ class BaseReviewRequestAction(object): DepthLimitExceededError: The maximum depth limit is exceeded. """ - _populate_defaults() - - if self.action_id in _all_actions: - raise KeyError('%s already corresponds to a registered review ' - 'request action' % self.action_id) - - if self.max_depth > MAX_DEPTH_LIMIT: - raise DepthLimitExceededError(self.action_id, MAX_DEPTH_LIMIT) - if parent: - parent.child_actions.append(self) - self._parent = parent - else: - _top_level_ids.appendleft(self.action_id) + self.parent_id = parent.action_id - _all_actions[self.action_id] = self + actions_registry.register(self) - def unregister(self): + def unregister(self) -> None: """Unregister this review request action instance. Note: @@ -240,32 +815,26 @@ class BaseReviewRequestAction(object): consider first making a clone of the list of children. Raises: - KeyError: An unregistration is attempted before it's registered. + KeyError: + An unregistration is attempted before it's registered. """ - _populate_defaults() - - try: - del _all_actions[self.action_id] - except KeyError: - raise KeyError('%s does not correspond to a registered review ' - 'request action' % self.action_id) - - if self._parent: - self._parent.child_actions.remove(self) - else: - _top_level_ids.remove(self.action_id) - - self.reset_max_depth() + actions_registry.unregister(self) -class BaseReviewRequestMenuAction(BaseReviewRequestAction): +class BaseReviewRequestMenuAction(BaseMenuAction): """A base class for an action with a dropdown menu. - Note: - A menu action's child actions must always be pre-registered. + Deprecated: + 6.0: + New code should be written using + :py:class:`reviewboard.actions.base.BaseMenuAction`. This class will be + removed in 7.0. """ - def __init__(self, child_actions=None): + def __init__( + self, + child_actions: Optional[List[BaseReviewRequestAction]] = None, + ) -> None: """Initialize this menu action. Args: @@ -280,68 +849,108 @@ class BaseReviewRequestMenuAction(BaseReviewRequestAction): DepthLimitExceededError: The maximum depth limit is exceeded. """ - super(BaseReviewRequestMenuAction, self).__init__() + super().__init__() - self.child_actions = [] - child_actions = child_actions or [] + self._children = child_actions or [] - for child_action in child_actions: - child_action.register(self) - - def copy_to_dict(self, context): + def copy_to_dict( + self, + context: Context, + ) -> dict: """Copy this menu action instance to a dictionary. + This is a legacy implementation left to maintain compatibility with + custom templates. + Args: context (django.template.Context): The collection of key-value pairs from the template. Returns: - dict: The corresponding dictionary. + dict: + The corresponding dictionary. """ - dict_copy = { + return { + 'action_id': self.action_id, 'child_actions': self.child_actions, + 'hidden': not self.get_visible(context=context), + 'label': self.get_label(context=context), + 'url': self.get_url(context=context), } - dict_copy.update(super(BaseReviewRequestMenuAction, self).copy_to_dict( - context)) - return dict_copy + def get_extra_context( + self, + *, + request: HttpRequest, + context: Context, + ) -> dict: + """Return extra template context for the action. + + Args: + request (django.http.HttpRequest): + The HTTP request from the client. + + context (django.template.Context): + The current rendering context. + + Returns: + dict: + Extra context to use when rendering the action's template. + """ + data = super().get_extra_context(request=request, context=context) + + if self.template_name != 'actions/menu_action.html': + data['menu_action'] = self.copy_to_dict(context) + + return data @property - def max_depth(self): + def max_depth(self) -> int: """Lazily compute the max depth of any action contained by this action. Returns: int: The max depth of any action contained by this action. """ - if self.child_actions and self._max_depth == 0: - self._max_depth = 1 + max(child_action.max_depth - for child_action in self.child_actions) + if self.child_actions: + return max(child_action.max_depth + for child_action in self.child_actions) + else: + return self.depth - return self._max_depth + def register( + self, + parent: Optional[BaseReviewRequestMenuAction] = None, + ) -> None: + """Register this review request action instance. - def render(self, context, action_key='menu_action', - template_name='reviews/menu_action.html'): - """Render this menu action instance and return the content as HTML. + Note: + Newly registered top-level actions are appended to the left of the + other previously registered top-level actions. So if we intend to + register a collection of top-level actions in a certain order, then + we likely want to iterate through the actions in reverse. Args: - context (django.template.Context): - The collection of key-value pairs that is passed to the - template in order to render this menu action. - - action_key (unicode, optional): - The key to be used for this menu action in the context map. + parent (BaseReviewRequestMenuAction, optional): + The parent action instance of this action instance. - template_name (unicode, optional): - The name of the template to be used for rendering this menu - action. + Raises: + KeyError: + A second registration is attempted (action IDs must be unique + across all types of actions and menu actions, at any depth). - Returns: - unicode: The action rendered in HTML. + DepthLimitExceededError: + The maximum depth limit is exceeded. """ - return super(BaseReviewRequestMenuAction, self).render( - context, action_key, template_name) + if parent: + self.parent_id = parent.action_id + + actions_registry.register(self) - def unregister(self): + for child in self._children: + child.parent_id = self.action_id + child.register() + + def unregister(self) -> None: """Unregister this review request action instance. This menu action recursively unregisters its child action instances. @@ -349,44 +958,26 @@ class BaseReviewRequestMenuAction(BaseReviewRequestAction): Raises: KeyError: An unregistration is attempted before it's registered. """ - super(BaseReviewRequestMenuAction, self).unregister() - - # Unregistration will mutate self.child_actions, so we make a copy. - for child_action in list(self.child_actions): - child_action.unregister() + for child in self._children: + child.unregister() + actions_registry.unregister(self) -# TODO: Convert all this to use djblets.registries. -def _populate_defaults(): - """Populate the default action instances.""" - global _populated - if not _populated: - _populated = True - - from reviewboard.reviews.default_actions import get_default_actions - - for default_action in reversed(get_default_actions()): - default_action.register() - - -def get_top_level_actions(): - """Return a generator of all top-level registered action instances. - - Yields: - BaseReviewRequestAction: - All top-level registered review request action instances. - """ - _populate_defaults() - - return (_all_actions[action_id] for action_id in _top_level_ids) - - -def register_actions(actions, parent_id=None): +def register_actions( + actions: List[Union[BaseReviewRequestAction, BaseReviewRequestMenuAction]], + parent_id: Optional[str] = None, +) -> None: """Register the given actions as children of the corresponding parent. If no parent_id is given, then the actions are assumed to be top-level. + Deprecated: + 6.0: + Users should switch to + :py:const:`reviewboard.actions.actions_registry`. This method will be + removed in Review Board 7. + Args: actions (iterable of BaseReviewRequestAction): The collection of action instances to be registered. @@ -404,59 +995,68 @@ def register_actions(actions, parent_id=None): DepthLimitExceededError: The maximum depth limit is exceeded. """ - _populate_defaults() + RemovedInReviewBoard70Warning.warn( + 'register_actions has been deprecated and will be removed in ' + 'Review Board 7.0. Please update your code to use ' + 'reviewboard.actions.actions_registry.') - if parent_id is None: - parent = None + if parent_id: + parent = actions_registry.get('action_id', parent_id) else: - try: - parent = _all_actions[parent_id] - except KeyError: - raise KeyError('%s does not correspond to a registered review ' - 'request action' % parent_id) + parent = None - for action in reversed(actions): + for action in actions: action.register(parent) - if parent: - parent.reset_max_depth() - -def unregister_actions(action_ids): +def unregister_actions( + action_ids: Iterable[str], +) -> None: """Unregister each of the actions corresponding to the given IDs. + Deprecated: + 6.0: + Users should switch to + :py:const:`reviewboard.actions.actions_registry`. This method will be + removed in Review Board 7. + Args: action_ids (iterable of unicode): The collection of action IDs corresponding to the actions to be removed. Raises: - KeyError: An unregistration is attempted before it's registered. + KeyError: + An unregistration is attempted before it's registered. """ - _populate_defaults() + RemovedInReviewBoard70Warning.warn( + 'unregister_actions has been deprecated and will be removed in ' + 'Review Board 7.0. Please update your code to use ' + 'reviewboard.actions.actions_registry.') for action_id in action_ids: - try: - action = _all_actions[action_id] - except KeyError: - raise KeyError('%s does not correspond to a registered review ' - 'request action' % action_id) - + action = actions_registry.get('action_id', action_id) action.unregister() -def clear_all_actions(): +def clear_all_actions() -> None: """Clear all registered actions. This method is really only intended to be used by unit tests. We might be able to remove this hack once we convert to djblets.registries. + Deprecated: + 6.0: + Users should switch to + :py:const:`reviewboard.actions.actions_registry`. This method will be + removed in Review Board 7. + Warning: This will clear **all** actions, even if they were registered in separate extensions. """ - global _populated - - _all_actions.clear() - _top_level_ids.clear() - _populated = False + RemovedInReviewBoard70Warning.warn( + 'clear_all_actions has been deprecated and will be removed in ' + 'Review Board 7.0. Please update your code to use ' + 'reviewboard.actions.actions_registry.') + actions_registry.reset() diff --git a/reviewboard/reviews/default_actions.py b/reviewboard/reviews/default_actions.py index c482bffd0e81378659e9cdeebc71ec4380b2b2aa..35795539c2a6cc2edbab939d709deae8d2f98690 100644 --- a/reviewboard/reviews/default_actions.py +++ b/reviewboard/reviews/default_actions.py @@ -1,322 +1,36 @@ -from django.utils.translation import gettext_lazy as _ - -from reviewboard.admin.read_only import is_site_read_only_for -from reviewboard.reviews.actions import (BaseReviewRequestAction, - BaseReviewRequestMenuAction) -from reviewboard.reviews.features import general_comments_feature -from reviewboard.reviews.models import ReviewRequest -from reviewboard.site.urlresolvers import local_site_reverse -from reviewboard.urls import diffviewer_url_names - - -class CloseMenuAction(BaseReviewRequestMenuAction): - """A menu action for closing the corresponding review request.""" - - action_id = 'close-review-request-action' - label = _('Close') - - def should_render(self, context): - """Return whether the action should render. - - Args: - context (dict): - The current render context. - - Returns: - bool: - Whether the action should render. - """ - review_request = context['review_request'] - user = context['request'].user - - return (review_request.status == ReviewRequest.PENDING_REVIEW and - not is_site_read_only_for(user) and - (context['request'].user.pk == review_request.submitter_id or - (context['perms']['reviews']['can_change_status'] and - review_request.public))) - - -class SubmitAction(BaseReviewRequestAction): - """An action for submitting the review request.""" - - action_id = 'submit-review-request-action' - label = _('Submitted') - - def should_render(self, context): - """Return whether the action should render. - - Args: - context (dict): - The current render context. - - Returns: - bool: - Whether the action should render. - """ - return (context['review_request'].public and - not is_site_read_only_for(context['request'].user)) - - -class DiscardAction(BaseReviewRequestAction): - """An action for discarding the review request.""" - - action_id = 'discard-review-request-action' - label = _('Discarded') - - -class DeleteAction(BaseReviewRequestAction): - """An action for permanently deleting the review request.""" - - action_id = 'delete-review-request-action' - label = _('Delete Permanently') - - def should_render(self, context): - """Return whether the action should render. - - Args: - context (dict): - The current render context. - - Returns: - bool: - Whether the action should render. - """ - return (context['perms']['reviews']['delete_reviewrequest'] and - not is_site_read_only_for(context['request'].user)) - - -class UpdateMenuAction(BaseReviewRequestMenuAction): - """A menu action for updating the corresponding review request.""" - - action_id = 'update-review-request-action' - label = _('Update') - - def should_render(self, context): - """Return whether the action should render. - - Args: - context (dict): - The current render context. - - Returns: - bool: - Whether the action should render. - """ - review_request = context['review_request'] - user = context['request'].user - - return (review_request.status == ReviewRequest.PENDING_REVIEW and - not is_site_read_only_for(user) and - (user.pk == review_request.submitter_id or - context['perms']['reviews']['can_edit_reviewrequest'])) - - -class UploadDiffAction(BaseReviewRequestAction): - """An action for updating/uploading a diff for the review request.""" - - action_id = 'upload-diff-action' - - def get_label(self, context): - """Return this action's label. - - The label will change depending on whether or not the corresponding - review request already has a diff. - - Args: - context (django.template.Context): - The collection of key-value pairs from the template. - - Returns: - unicode: The label that displays this action to the user. - """ - review_request = context['review_request'] - draft = review_request.get_draft(context['request'].user) - - if (draft and draft.diffset) or review_request.get_diffsets(): - return _('Update Diff') - - return _('Upload Diff') - - def should_render(self, context): - """Return whether or not this action should render. - - If the corresponding review request has a repository, then an upload - diff form exists, so we should render this UploadDiffAction. - - Args: - context (django.template.Context): - The collection of key-value pairs available in the template - just before this action is to be rendered. - - Returns: - bool: Determines if this action should render. - """ - return (context['review_request'].repository_id is not None and - not is_site_read_only_for(context['request'].user)) - - -class UploadFileAction(BaseReviewRequestAction): - """An action for uploading a file for the review request.""" - - action_id = 'upload-file-action' - label = _('Add File') - - -class DownloadDiffAction(BaseReviewRequestAction): - """An action for downloading a diff from the review request.""" - - action_id = 'download-diff-action' - label = _('Download Diff') - - def get_url(self, context): - """Return this action's URL. - - Args: - context (django.template.Context): - The collection of key-value pairs from the template. - - Returns: - unicode: The URL to invoke if this action is clicked. - """ - match = context['request'].resolver_match - - # We want to use a relative URL in the diff viewer as we will not be - # re-rendering the page when switching between revisions. - if match.url_name in diffviewer_url_names: - return 'raw/' - - return local_site_reverse('raw-diff', context['request'], kwargs={ - 'review_request_id': context['review_request'].display_id, - }) - - def get_hidden(self, context): - """Return whether this action should be initially hidden to the user. - - Args: - context (django.template.Context): - The collection of key-value pairs from the template. - - Returns: - bool: Whether this action should be initially hidden to the user. - """ - match = context['request'].resolver_match - - if match.url_name in diffviewer_url_names: - return match.url_name == 'view-interdiff' - - return super(DownloadDiffAction, self).get_hidden(context) - - def should_render(self, context): - """Return whether or not this action should render. - - Args: - context (django.template.Context): - The collection of key-value pairs available in the template - just before this action is to be rendered. - - Returns: - bool: Determines if this action should render. - """ - review_request = context['review_request'] - request = context['request'] - match = request.resolver_match - - # If we're on a diff viewer page, then this DownloadDiffAction should - # initially be rendered, but possibly hidden. - if match.url_name in diffviewer_url_names: - return True - - return review_request.repository_id is not None - - -class EditReviewAction(BaseReviewRequestAction): - """An action for editing a review intended for the review request.""" - - action_id = 'review-action' - label = _('Review') - - def should_render(self, context): - """Return whether the action should render. - - Args: - context (dict): - The current render context. - - Returns: - bool: - Whether the action should render. - """ - user = context['request'].user - - return (user.is_authenticated and - not is_site_read_only_for(user)) - - -class AddGeneralCommentAction(BaseReviewRequestAction): - """An action for adding a new general comment to a review.""" - - action_id = 'general-comment-action' - label = _('Add General Comment') - - def should_render(self, context): - """Return whether the action should render. - - Args: - context (dict): - The current render context. - - Returns: - bool: - Whether the action should render. - """ - request = context['request'] - user = request.user - - return (user.is_authenticated and - not is_site_read_only_for(user) and - general_comments_feature.is_enabled(request=request)) - - -class ShipItAction(BaseReviewRequestAction): - """An action for quickly approving the review request without comments.""" - - action_id = 'ship-it-action' - label = _('Ship It!') - - def should_render(self, context): - """Return whether the action should render. - - Args: - context (dict): - The current render context. - - Returns: - bool: - Whether the action should render. - """ - user = context['request'].user - return (user.is_authenticated and - not is_site_read_only_for(user)) - - -def get_default_actions(): - """Return a copy of all the default actions. - - Returns: - list of BaseReviewRequestAction: A copy of all the default actions. - """ - return [ - CloseMenuAction([ - SubmitAction(), - DiscardAction(), - DeleteAction(), - ]), - UpdateMenuAction([ - UploadDiffAction(), - UploadFileAction(), - ]), - DownloadDiffAction(), - EditReviewAction(), - AddGeneralCommentAction(), - ShipItAction(), - ] +"""Default actions for the reviews app. + +These have moved to :py:mod:`reviewboard.reviews.actions`. The imports here +are for legacy compatibility, and will be removed in Review Board 7.0. +""" + +from reviewboard.reviews.actions import ( + AddGeneralCommentAction, + CloseMenuAction, + CloseCompletedAction as SubmitAction, + CloseDiscardedAction as DiscardAction, + DeleteAction, + DownloadDiffAction, + LegacyEditReviewAction as EditReviewAction, + LegacyShipItAction as ShipItAction, + UpdateMenuAction, + UploadDiffAction, + UploadFileAction) + + +__all__ = [ + 'AddGeneralCommentAction', + 'CloseMenuAction', + 'DeleteAction', + 'DiscardAction', + 'DownloadDiffAction', + 'EditReviewAction', + 'ShipItAction', + 'SubmitAction', + 'UpdateMenuAction', + 'UploadDiffAction', + 'UploadFileAction', +] + + +__autodoc_excludes__ = __all__ diff --git a/reviewboard/reviews/errors.py b/reviewboard/reviews/errors.py index 5d4cc6da928b2dcb5e50a9cfc8602dc894901ec6..6aed270574813607c5539729ea3194c70f0ef0f4 100644 --- a/reviewboard/reviews/errors.py +++ b/reviewboard/reviews/errors.py @@ -1,3 +1,8 @@ +"""Error definitions for the reviews app.""" + +from reviewboard.actions.errors import DepthLimitExceededError + + class OwnershipError(ValueError): """An error that occurs when a user does not own a review request.""" pass @@ -50,29 +55,21 @@ class NotModifiedError(PublishError): 'The draft has no modifications.') -class DepthLimitExceededError(ValueError): - """An error that occurs when the maximum depth limit is exceeded. - - Review request actions cannot be arbitrarily nested. For example, if the - depth limit is 2, then this error would be triggered if an extension tried - to add a menu action as follows: - - .. code-block:: python - - BaseReviewRequestActionHook(self, actions=[ - DepthZeroMenuAction([ - DepthOneFirstItemAction(), - DepthOneMenuAction([ - DepthTwoMenuAction([ # This depth is acceptable. - DepthThreeTooDeepAction(), # This action is too deep. - ]), - ]), - DepthOneLastItemAction(), - ]), - ]) - """ - - def __init__(self, action_id, depth_limit): - super(DepthLimitExceededError, self).__init__( - '%s exceeds the maximum depth limit of %d' - % (action_id, depth_limit)) +__all__ = ( + 'CloseError', + 'NotModifiedError', + 'OwnershipError', + 'PermissionError', + 'PublishError', + 'ReopenError', + 'RevokeShipItError', + + # This is left as a forwarding import. When + # reviewboard.reviews.actions.BaseReviewRequestAction is removed in Review + # Board 7.0, this can go away. + 'DepthLimitExceededError', +) + +__autodoc_excludes__ = ( + 'DepthLimitExceededError', +) diff --git a/reviewboard/reviews/templatetags/reviewtags.py b/reviewboard/reviews/templatetags/reviewtags.py index 191532461521838561b3fbc9a7b301cb725510ce..ea87a5ff56f36ff34d5ff6ca9bc44450c92c7ffa 100644 --- a/reviewboard/reviews/templatetags/reviewtags.py +++ b/reviewboard/reviews/templatetags/reviewtags.py @@ -18,7 +18,6 @@ from reviewboard.accounts.models import Profile, Trophy from reviewboard.accounts.trophies import UnknownTrophy from reviewboard.admin.read_only import is_site_read_only_for from reviewboard.diffviewer.diffutils import get_displayed_diff_line_ranges -from reviewboard.reviews.actions import get_top_level_actions from reviewboard.reviews.builtin_fields import FileAttachmentsField from reviewboard.reviews.fields import (get_review_request_field, get_review_request_fieldset, @@ -355,52 +354,6 @@ def reviewer_list(review_request): for user in review_request.target_people.all()]) -@register.simple_tag(takes_context=True) -def review_request_actions(context): - """Render all registered review request actions. - - Args: - context (django.template.Context): - The collection of key-value pairs available in the template. - - Returns: - unicode: The HTML content to be rendered. - """ - content = [] - - for top_level_action in get_top_level_actions(): - try: - content.append(top_level_action.render(context)) - except Exception: - logger.exception('Error rendering top-level action %s', - top_level_action.action_id) - - return mark_safe(''.join(content)) - - -@register.simple_tag(takes_context=True) -def child_actions(context): - """Render all registered child actions. - - Args: - context (django.template.Context): - The collection of key-value pairs available in the template. - - Returns: - unicode: The HTML content to be rendered. - """ - content = [] - - for child_action in context['menu_action']['child_actions']: - try: - content.append(child_action.render(context)) - except Exception: - logger.exception('Error rendering child action %s', - child_action.action_id) - - return mark_safe(''.join(content)) - - @register.tag @blocktag(end_prefix='end_') def for_review_request_field(context, nodelist, review_request_details, diff --git a/reviewboard/reviews/tests/test_actions.py b/reviewboard/reviews/tests/test_actions.py index df2b70b2776e094730b428e476b8b48b016ab5a5..a98687e78fd781cf2dcd06b5c18c00b753a7e81a 100644 --- a/reviewboard/reviews/tests/test_actions.py +++ b/reviewboard/reviews/tests/test_actions.py @@ -1,17 +1,17 @@ +from typing import Optional, TYPE_CHECKING + from django.contrib.auth.models import AnonymousUser, User from django.template import Context from django.test.client import RequestFactory +from django.urls.resolvers import ResolverMatch +from djblets.features.testing import override_feature_check from djblets.siteconfig.models import SiteConfiguration from djblets.testing.decorators import add_fixtures -from mock import Mock +from reviewboard.actions import actions_registry +from reviewboard.deprecation import RemovedInReviewBoard70Warning from reviewboard.reviews.actions import (BaseReviewRequestAction, - BaseReviewRequestMenuAction, - MAX_DEPTH_LIMIT, - clear_all_actions, - get_top_level_actions, - register_actions, - unregister_actions) + BaseReviewRequestMenuAction) from reviewboard.reviews.default_actions import (AddGeneralCommentAction, CloseMenuAction, DeleteAction, @@ -22,10 +22,17 @@ from reviewboard.reviews.default_actions import (AddGeneralCommentAction, UpdateMenuAction, UploadDiffAction) from reviewboard.reviews.errors import DepthLimitExceededError +from reviewboard.reviews.features import unified_banner_feature from reviewboard.reviews.models import ReviewRequest from reviewboard.testing import TestCase +if TYPE_CHECKING: + MixinParent = TestCase +else: + MixinParent = object + + class FooAction(BaseReviewRequestAction): action_id = 'foo-action' label = 'Foo Action' @@ -33,9 +40,8 @@ class FooAction(BaseReviewRequestAction): class BarAction(BaseReviewRequestMenuAction): def __init__(self, action_id, child_actions=None): - super(BarAction, self).__init__(child_actions) - self.action_id = 'bar-' + action_id + super().__init__(child_actions) class TopLevelMenuAction(BaseReviewRequestMenuAction): @@ -51,38 +57,13 @@ class PoorlyCodedAction(BaseReviewRequestAction): class ActionsTestCase(TestCase): """Test case for unit tests dealing with actions.""" - def tearDown(self): - super(ActionsTestCase, self).tearDown() - - # This prevents registered/unregistered/modified actions from leaking - # between different unit tests. - clear_all_actions() + def tearDown(self) -> None: + """Tear down the test case.""" + super().tearDown() + actions_registry.reset() - def make_nested_actions(self, depth): - """Return a nested list of actions to register. - This returns a list of actions, each entry nested within the prior - entry, of the given length. The resulting list is intended to be - registered. - - Args: - depth (int): - The nested depth for the actions. - - Returns: - list of reviewboard.reviews.actions.BaseReviewRequestAction: - The list of actions. - """ - actions = [None] * depth - actions[0] = BarAction('0') - - for i in range(1, depth): - actions[i] = BarAction(str(i), [actions[i - 1]]) - - return actions - - -class ReadOnlyActionTestsMixin(object): +class ReadOnlyActionTestsMixin(MixinParent): """Mixin for Review Board actions-related unit tests with read-only mode. This mixin is used to add read-only mode tests to action test cases.Using @@ -91,14 +72,14 @@ class ReadOnlyActionTestsMixin(object): visible can also be tested by setting ``read_only_always_show``. """ - def setUp(self): + def setUp(self) -> None: """Set up the test case.""" super(ReadOnlyActionTestsMixin, self).setUp() self.request = RequestFactory().request() self.siteconfig = SiteConfiguration.objects.get_current() - def shortDescription(self): + def shortDescription(self) -> str: """Return an updated description for a particular test. If the test has an ``action`` attribute set and contains ```` @@ -107,7 +88,7 @@ class ReadOnlyActionTestsMixin(object): :py:class:`~reviewboard.reviews.actions.BaseReviewRequestAction`. Returns: - unicode: + str: The description of the test. """ desc = super(ReadOnlyActionTestsMixin, self).shortDescription() @@ -117,7 +98,7 @@ class ReadOnlyActionTestsMixin(object): return desc - def _create_request_context(self, *args, **kwargs): + def _create_request_context(self, *args, **kwargs) -> Context: """Create and return objects for use in the request context. Args: @@ -128,12 +109,12 @@ class ReadOnlyActionTestsMixin(object): Keyword arguments for use in subclasses. Returns: - dict: + django.template.Context: Additional context to use when testing read-only actions. """ - return {} + return Context() - def test_should_render_with_user_in_read_only(self): + def test_should_render_with_user_in_read_only(self) -> None: """Testing .should_render with authenticated user in read-only mode """ @@ -149,11 +130,13 @@ class ReadOnlyActionTestsMixin(object): with self.siteconfig_settings(settings): if getattr(self, 'read_only_always_show', False): - self.assertTrue(self.action.should_render(request_context)) + self.assertTrue( + self.action.should_render(context=request_context)) else: - self.assertFalse(self.action.should_render(request_context)) + self.assertFalse( + self.action.should_render(context=request_context)) - def test_should_render_with_superuser_in_read_only(self): + def test_should_render_with_superuser_in_read_only(self) -> None: """Testing .should_render with superuser in read-only mode""" self.request.user = User.objects.get(username='admin') @@ -166,221 +149,126 @@ class ReadOnlyActionTestsMixin(object): } with self.siteconfig_settings(settings): - self.assertTrue(self.action.should_render(request_context)) - - -class ActionRegistryTests(ActionsTestCase): - """Unit tests for the review request actions registry.""" - - def test_register_actions_with_invalid_parent_id(self): - """Testing register_actions with an invalid parent ID""" - foo_action = FooAction() - - message = ( - 'bad-id does not correspond to a registered review request action' - ) - - foo_action.register() + self.assertTrue( + self.action.should_render(context=request_context)) - with self.assertRaisesMessage(KeyError, message): - register_actions([foo_action], 'bad-id') - def test_register_actions_with_already_registered_action(self): - """Testing register_actions with an already registered action""" - foo_action = FooAction() +class ActionRegistrationTests(ActionsTestCase): + """Unit tests for legacy action registration. - message = ( - '%s already corresponds to a registered review request action' - % foo_action.action_id - ) - - foo_action.register() - - with self.assertRaisesMessage(KeyError, message): - register_actions([foo_action]) - - def test_register_actions_with_max_depth(self): - """Testing register_actions with max_depth""" - actions = self.make_nested_actions(MAX_DEPTH_LIMIT) - extra_action = BarAction('extra') - foo_action = FooAction() - - for d, action in enumerate(actions): - self.assertEqual(action.max_depth, d) - - register_actions([extra_action], actions[0].action_id) - actions = [extra_action] + actions - - for d, action in enumerate(actions): - self.assertEqual(action.max_depth, d) - - register_actions([foo_action]) - self.assertEqual(foo_action.max_depth, 0) - - def test_register_actions_with_too_deep(self): - """Testing register_actions with exceeding max depth""" - actions = self.make_nested_actions(MAX_DEPTH_LIMIT + 1) - invalid_action = BarAction(str(len(actions)), [actions[-1]]) - - error_message = ( - '%s exceeds the maximum depth limit of %d' - % (invalid_action.action_id, MAX_DEPTH_LIMIT) - ) - - with self.assertRaisesMessage(DepthLimitExceededError, error_message): - register_actions([invalid_action]) - - def test_unregister_actions(self): - """Testing unregister_actions""" - - orig_action_ids = { - action.action_id - for action in get_top_level_actions() - } - self.assertIn('update-review-request-action', orig_action_ids) - self.assertIn('review-action', orig_action_ids) + Deprecated: + 6.0: + This can go away once we remove the legacy actions. + """ - unregister_actions(['update-review-request-action', 'review-action']) + deprecation_message = ( + 'BaseReviewRequestAction is deprecated and will be removed in ' + 'Review Board 7.0. Please update your code to use ' + 'reviewboard.actions.base.BaseAction') - new_action_ids = { - action.action_id - for action in get_top_level_actions() - } - self.assertEqual(len(orig_action_ids), len(new_action_ids) + 2) - self.assertNotIn('update-review-request-action', new_action_ids) - self.assertNotIn('review-action', new_action_ids) - - def test_unregister_actions_with_child_action(self): - """Testing unregister_actions with child action""" - menu_action = TopLevelMenuAction([ - FooAction() - ]) - - self.assertEqual(len(menu_action.child_actions), 1) - unregister_actions([FooAction.action_id]) - self.assertEqual(len(menu_action.child_actions), 0) - - def test_unregister_actions_with_unregistered_action(self): - """Testing unregister_actions with unregistered action""" - foo_action = FooAction() - error_message = ( - '%s does not correspond to a registered review request action' - % foo_action.action_id - ) - - with self.assertRaisesMessage(KeyError, error_message): - unregister_actions([foo_action.action_id]) - - def test_unregister_actions_with_max_depth(self): - """Testing unregister_actions with max_depth""" - actions = self.make_nested_actions(MAX_DEPTH_LIMIT + 1) - - unregister_actions([actions[0].action_id]) - extra_action = BarAction(str(len(actions)), [actions[-1]]) - extra_action.register() - self.assertEqual(extra_action.max_depth, MAX_DEPTH_LIMIT) - - -class BaseReviewRequestActionTests(ActionsTestCase): - """Unit tests for BaseReviewRequestAction.""" - - def test_register_then_unregister(self): - """Testing BaseReviewRequestAction.register then unregister for - actions - """ - foo_action = FooAction() - foo_action.register() + def test_action_register_methods(self) -> None: + """Testing BaseReviewRequestAction.register and unregister""" + with self.assertWarns(RemovedInReviewBoard70Warning, + self.deprecation_message): + foo_action = FooAction() + foo_action.register() - self.assertIn(foo_action.action_id, ( - action.action_id - for action in get_top_level_actions() - )) + self.assertEqual(actions_registry.get('action_id', 'foo-action'), + foo_action) foo_action.unregister() - self.assertNotIn(foo_action.action_id, ( - action.action_id - for action in get_top_level_actions() - )) + self.assertIsNone(actions_registry.get('action_id', 'foo-action')) - def test_register_with_already_registered(self): - """Testing BaseReviewRequestAction.register with already registered - action + def test_action_register_methods_with_parent(self) -> None: + """Testing BaseReviewRequestAction.register and unregister with + parent """ - foo_action = FooAction() - error_message = ( - '%s already corresponds to a registered review request action' - % foo_action.action_id - ) - - foo_action.register() + with self.assertWarns(RemovedInReviewBoard70Warning, + self.deprecation_message): + bar_action = BarAction('action-1') + foo_action = FooAction() - with self.assertRaisesMessage(KeyError, error_message): - foo_action.register() + bar_action.register() + foo_action.register(bar_action) - def test_register_with_too_deep(self): - """Testing BaseReviewRequestAction.register with exceeding max depth""" - actions = self.make_nested_actions(MAX_DEPTH_LIMIT + 1) - invalid_action = BarAction(str(len(actions)), [actions[-1]]) - error_message = ( - '%s exceeds the maximum depth limit of %d' - % (invalid_action.action_id, MAX_DEPTH_LIMIT) - ) - - with self.assertRaisesMessage(DepthLimitExceededError, error_message): - invalid_action.register() - - def test_unregister_with_unregistered_action(self): - """Testing BaseReviewRequestAction.unregister with unregistered - action - """ - foo_action = FooAction() + self.assertEqual(actions_registry.get('action_id', 'foo-action'), + foo_action) + self.assertEqual(foo_action.parent_action, bar_action) + self.assertEqual(bar_action.child_actions, [foo_action]) - message = ( - '%s does not correspond to a registered review request action' - % foo_action.action_id - ) + foo_action.unregister() - with self.assertRaisesMessage(KeyError, message): - foo_action.unregister() + self.assertIsNone(actions_registry.get('action_id', 'foo-action')) + self.assertIsNone(foo_action.parent_action) + self.assertEqual(bar_action.child_actions, []) - def test_unregister_with_max_depth(self): - """Testing BaseReviewRequestAction.unregister with max_depth""" - actions = self.make_nested_actions(MAX_DEPTH_LIMIT + 1) - actions[0].unregister() - extra_action = BarAction(str(len(actions)), [actions[-1]]) + def test_menuaction_register_methods(self) -> None: + """Testing BaseReviewRequestMenuAction.register and unregister""" + with self.assertWarns(RemovedInReviewBoard70Warning, + self.deprecation_message): + foo_action = FooAction() + bar_action = BarAction('action-1', [foo_action]) - extra_action.register() - self.assertEqual(extra_action.max_depth, MAX_DEPTH_LIMIT) + bar_action.register() - def test_init_already_registered_in_menu(self): - """Testing BaseReviewRequestAction.__init__ for already registered - action when nested in a menu action - """ - foo_action = FooAction() - error_message = ('%s already corresponds to a registered review ' - 'request action') % foo_action.action_id + self.assertEqual(actions_registry.get('action_id', 'foo-action'), + foo_action) + self.assertEqual(actions_registry.get('action_id', 'bar-action-1'), + bar_action) + self.assertEqual(foo_action.parent_action, bar_action) + self.assertEqual(bar_action.child_actions, [foo_action]) - foo_action.register() + bar_action.unregister() - with self.assertRaisesMessage(KeyError, error_message): - TopLevelMenuAction([ - foo_action, - ]) + self.assertIsNone(actions_registry.get('action_id', 'foo-action')) + self.assertIsNone(actions_registry.get('action_id', 'bar-action-1')) + self.assertIsNone(foo_action.parent_action) + self.assertEqual(bar_action.child_actions, []) - def test_render_pops_context_even_after_error(self): - """Testing BaseReviewRequestAction.render pops the context after an - error + def test_menuaction_register_methods_with_parent(self) -> None: + """Testing BaseReviewRequestMenuAction.register and unregister with + parent """ - context = Context({'comment': 'this is a comment'}) - old_dict_count = len(context.dicts) - poorly_coded_action = PoorlyCodedAction() - - with self.assertRaises(Exception): - poorly_coded_action.render(context) - - new_dict_count = len(context.dicts) - self.assertEqual(old_dict_count, new_dict_count) + with self.assertWarns(RemovedInReviewBoard70Warning, + self.deprecation_message): + foo_action = FooAction() + bar_action = BarAction('action-1', [foo_action]) + toplevel_action = TopLevelMenuAction() + + toplevel_action.register() + + bar_action.register(toplevel_action) + + self.assertEqual(actions_registry.get('action_id', 'foo-action'), + foo_action) + self.assertEqual(actions_registry.get('action_id', 'bar-action-1'), + bar_action) + self.assertEqual(toplevel_action.child_actions, [bar_action]) + self.assertEqual(bar_action.parent_action, toplevel_action) + self.assertEqual(foo_action.parent_action, bar_action) + self.assertEqual(bar_action.child_actions, [foo_action]) + + bar_action.unregister() + + self.assertIsNone(actions_registry.get('action_id', 'foo-action')) + self.assertIsNone(actions_registry.get('action_id', 'bar-action-1')) + self.assertIsNone(foo_action.parent_action) + self.assertEqual(bar_action.child_actions, []) + self.assertEqual(toplevel_action.child_actions, []) + self.assertIsNone(bar_action.parent_action) + + def test_register_max_depth_exceeded(self) -> None: + """Testing BaseReviewRequestAction.register with max depth exceeded""" + with self.assertWarns(RemovedInReviewBoard70Warning, + self.deprecation_message): + foo_action = FooAction() + bar_action1 = BarAction('action-1', [foo_action]) + bar_action2 = BarAction('action-2', [bar_action1]) + bar_action3 = BarAction('action-3', [bar_action2]) + + with self.assertRaises(DepthLimitExceededError): + bar_action3.register() class AddGeneralCommentActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): @@ -389,10 +277,22 @@ class AddGeneralCommentActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): action = AddGeneralCommentAction() fixtures = ['test_users'] - def _create_request_context(self, *args, **kwargs): + def _create_request_context( + self, + user: Optional[User] = None, + url_name: str = 'review-request-detail', + *args, + **kwargs, + ) -> Context: """Create and return objects for use in the request context. Args: + user (django.contrib.auth.models.User, optional): + The user to run the request. + + url_name (str, optional): + The URL name to fake on the resolver. + *args (tuple): Positional arguments (unused). @@ -400,28 +300,32 @@ class AddGeneralCommentActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): Keyword arguments (unused). Returns: - dict: + django.template.Context: Additional context to use when testing read-only actions. """ - return { - 'request': self.request, - } + self.request.resolver_match = ResolverMatch( + lambda: None, tuple(), {}, url_name=url_name) + + return Context({ + 'request': self.create_http_request(user=user, url_name=url_name), + }) - def test_should_render_with_authenticated(self): + def test_should_render_with_authenticated(self) -> None: """Testing AddGeneralCommentAction.should_render with authenticated user """ self.request.user = User.objects.get(username='doc') self.assertTrue( - self.action.should_render(self._create_request_context())) + self.action.should_render( + context=self._create_request_context( + User.objects.get(username='doc')))) - def test_should_render_with_anonymous(self): + def test_should_render_with_anonymous(self) -> None: """Testing AddGeneralCommentAction.should_render with authenticated user """ - self.request.user = AnonymousUser() self.assertFalse( - self.action.should_render(self._create_request_context())) + self.action.should_render(context=self._create_request_context())) class CloseMenuActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): @@ -430,19 +334,27 @@ class CloseMenuActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): action = CloseMenuAction() fixtures = ['test_users'] - def _create_request_context(self, can_change_status=True, public=True, - status=ReviewRequest.PENDING_REVIEW, - user=None): + def _create_request_context( + self, + url_name: str = 'review-request-detail', + can_change_status: bool = True, + public: bool = True, + status: str = ReviewRequest.PENDING_REVIEW, + user: Optional[User] = None, + ) -> Context: """Create and return objects for use in the request context. Args: + url_name (str, optional): + The URL name to fake on the resolver. + can_change_status (bool, optional): Whether the ``can_change_status`` permission should be set. public (bool, optional): Whether the review request should be public. - status (unicode, optional): + status (str, optional): The status for the review request. user (django.contrib.auth.models.User, optional): @@ -455,75 +367,79 @@ class CloseMenuActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): Additional keyword arguments (unused). Returns: - dict: + django.template.Context: Additional context to use when testing read-only actions. """ review_request = self.create_review_request( public=public, status=status) - self.request.user = user or review_request.submitter + request = self.create_http_request( + user=user or review_request.submitter, + url_name=url_name) - return { + return Context({ 'review_request': review_request, - 'request': self.request, + 'request': request, 'perms': { 'reviews': { 'can_change_status': can_change_status, }, }, - } + }) - def test_should_render_for_owner(self): + def test_should_render_for_owner(self) -> None: """Testing CloseMenuAction.should_render for owner of review request""" self.assertTrue(self.action.should_render( - self._create_request_context())) + context=self._create_request_context())) - def test_should_render_for_owner_unpublished(self): + def test_should_render_for_owner_unpublished(self) -> None: """Testing CloseMenuAction.should_render for owner of review unpublished review request """ self.assertTrue(self.action.should_render( - self._create_request_context(public=False))) + context=self._create_request_context( + public=False))) - def test_should_render_for_user(self): + def test_should_render_for_user(self) -> None: """Testing CloseMenuAction.should_render for normal user""" self.assertFalse(self.action.should_render( - self._create_request_context( + context=self._create_request_context( can_change_status=False, - user=User.objects.create_user(username='test-user', - email='user@example.com')))) + user=self.create_user()))) - def test_should_render_user_with_can_change_status(self): + def test_should_render_user_with_can_change_status(self) -> None: """Testing CloseMenuAction.should_render for user with can_change_status permission """ self.assertTrue(self.action.should_render( - self._create_request_context( + context=self._create_request_context( can_change_status=True, - user=User.objects.create_user(username='test-user', - email='user@example.com')))) + user=self.create_user()))) - def test_should_render_user_with_can_change_status_and_unpublished(self): + def test_should_render_user_with_can_change_status_and_unpublished( + self, + ) -> None: """Testing CloseMenuAction.should_render for user with can_change_status permission and unpublished review request """ self.assertFalse(self.action.should_render( - self._create_request_context( + context=self._create_request_context( can_change_status=True, public=False, - user=User.objects.create_user(username='test-user', - email='user@example.com')))) + user=self.create_user()))) - def test_should_render_with_discarded(self): + def test_should_render_with_discarded(self) -> None: """Testing CloseMenuAction.should_render with discarded review request """ self.assertFalse(self.action.should_render( - self._create_request_context(status=ReviewRequest.DISCARDED))) + context=self._create_request_context( + status=ReviewRequest.DISCARDED))) - def test_should_render_with_submitted(self): + def test_should_render_with_submitted(self) -> None: """Testing CloseMenuAction.should_render with submitted review request """ self.assertFalse(self.action.should_render( - self._create_request_context(status=ReviewRequest.SUBMITTED))) + context=self._create_request_context( + status=ReviewRequest.SUBMITTED))) class DeleteActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): @@ -532,11 +448,23 @@ class DeleteActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): fixtures = ['test_users'] action = DeleteAction() - def _create_request_context(self, delete_reviewrequest=True, - *args, **kwargs): + def _create_request_context( + self, + user: Optional[User] = None, + url_name: str = 'review-request-detail', + delete_reviewrequest: bool = True, + *args, + **kwargs, + ) -> Context: """Create and return objects for use in the request context. Args: + user (django.contrib.auth.models.User, optional): + The user to run the request. + + url_name (str, optional): + The URL name to fake on the resolver. + delete_reviewrequest (bool, optional): Whether the resulting context should include the ``delete_reviewrequest`` permission. @@ -548,31 +476,32 @@ class DeleteActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): Keyword arguments (unused). Returns: - dict: + django.template.Context: Additional context to use when testing read-only actions. """ - return { - 'request': self.request, + return Context({ + 'request': self.create_http_request(user=user, url_name=url_name), 'perms': { 'reviews': { 'delete_reviewrequest': delete_reviewrequest, }, }, - } + }) - def test_should_render_with_published(self): + def test_should_render_with_published(self) -> None: """Testing DeleteAction.should_render with standard user""" - self.request.user = User() self.assertFalse(self.action.should_render( - self._create_request_context(delete_reviewrequest=False))) + context=self._create_request_context( + user=self.create_user(), + delete_reviewrequest=False))) - def test_should_render_with_permission(self): + def test_should_render_with_permission(self) -> None: """Testing SubmitAction.should_render with delete_reviewrequest permission """ - self.request.user = User() self.assertTrue(self.action.should_render( - self._create_request_context())) + context=self._create_request_context( + user=self.create_user()))) class DownloadDiffActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): @@ -582,9 +511,14 @@ class DownloadDiffActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): fixtures = ['test_users'] read_only_always_show = True - def _create_request_context(self, review_request=None, - url_name='view-diff', - *args, **kwargs): + def _create_request_context( + self, + review_request: Optional[ReviewRequest] = None, + url_name: str = 'view-diff', + with_local_site: bool = False, + *args, + **kwargs, + ) -> Context: """Create and return objects for use in the request context. Args: @@ -593,9 +527,12 @@ class DownloadDiffActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): The review request to use. If not specified, one will be created. - url_name (unicode, optional): + url_name (str, optional): The URL name to fake on the resolver. + with_local_site (bool, optional): + Whether to use a local site. + *args (tuple): Positional arguments (unused). @@ -603,183 +540,149 @@ class DownloadDiffActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): Keyword arguments (unused). Returns: - dict: + django.template.Context: Additional context to use when testing read-only actions. """ - self.request.resolver_match = Mock() - self.request.resolver_match.url_name = url_name - if not review_request: - review_request = self.create_review_request() - - return { - 'request': self.request, + review_request = self.create_review_request( + with_local_site=with_local_site) + + return Context({ + 'request': self.create_http_request( + user=self.create_user(), + with_local_site=with_local_site, + url_name=url_name), 'review_request': review_request, - } + }) - def test_get_url_on_diff_viewer(self): + def test_get_url_on_diff_viewer(self) -> None: """Testing DownloadDiffAction.get_url on diff viewer page""" - self.request.resolver_match = Mock() - self.request.resolver_match.url_name = 'view-diff' - - self.assertEqual(self.action.get_url({'request': self.request}), - 'raw/') + self.assertEqual( + self.action.get_url(context=self._create_request_context()), + 'raw/') - def test_get_url_on_interdiff(self): + def test_get_url_on_interdiff(self) -> None: """Testing DownloadDiffAction.get_url on diff viewer interdiff page""" - self.request.resolver_match = Mock() - self.request.resolver_match.url_name = 'view-interdiff' - - self.assertEqual(self.action.get_url({'request': self.request}), - 'raw/') + self.assertEqual( + self.action.get_url(context=self._create_request_context( + url_name='view-interdiff')), + 'raw/') - def test_get_url_on_diff_viewer_revision(self): + def test_get_url_on_diff_viewer_revision(self) -> None: """Testing DownloadDiffAction.get_url on diff viewer revision page""" - self.request.resolver_match = Mock() - self.request.resolver_match.url_name = 'view-diff-revision' - - self.assertEqual(self.action.get_url({'request': self.request}), - 'raw/') + self.assertEqual( + self.action.get_url(context=self._create_request_context( + url_name='view-diff-revision')), + 'raw/') - def test_get_url_on_review_request(self): + def test_get_url_on_review_request(self) -> None: """Testing DownloadDiffAction.get_url on review request page""" - self.request.resolver_match = Mock() - self.request.resolver_match.url_name = 'review-request-detail' - review_request = self.create_review_request() self.assertEqual( - self.action.get_url({ - 'request': self.request, - 'review_request': review_request, - }), + self.action.get_url(context=self._create_request_context( + review_request=review_request, + url_name='review-request-detail')), '/r/%s/diff/raw/' % review_request.display_id) @add_fixtures(['test_site']) - def test_get_url_on_review_request_with_local_site(self): + def test_get_url_on_review_request_with_local_site(self) -> None: """Testing DownloadDiffAction.get_url on review request page with LocalSite """ - self.request.resolver_match = Mock() - self.request.resolver_match.url_name = 'review-request-detail' - self.request._local_site_name = self.local_site_name - review_request = self.create_review_request(id=123, with_local_site=True) self.assertEqual( - self.action.get_url({ - 'request': self.request, - 'review_request': review_request, - }), + self.action.get_url(context=self._create_request_context( + review_request=review_request, + url_name='review-request-detail', + with_local_site=True)), '/s/%s/r/%s/diff/raw/' % (self.local_site_name, review_request.display_id)) - def test_get_hidden_on_diff_viewer(self): - """Testing DownloadDiffAction.get_hidden on diff viewer page""" - self.request.resolver_match = Mock() - self.request.resolver_match.url_name = 'view-diff' - - self.assertFalse(self.action.get_hidden({'request': self.request})) + def test_get_hidden_on_diff_viewer(self) -> None: + """Testing DownloadDiffAction.get_visible on diff viewer page""" + self.assertTrue(self.action.get_visible( + context=self._create_request_context(url_name='view-diff'))) - def test_get_hidden_on_interdiff(self): - """Testing DownloadDiffAction.get_hidden on diff viewer interdiff page + def test_get_hidden_on_interdiff(self) -> None: + """Testing DownloadDiffAction.get_visible on diff viewer interdiff page """ - self.request.resolver_match = Mock() - self.request.resolver_match.url_name = 'view-interdiff' + self.assertFalse(self.action.get_visible( + context=self._create_request_context(url_name='view-interdiff'))) - self.assertTrue(self.action.get_hidden({'request': self.request})) - - def test_get_hidden_on_diff_viewer_revision(self): - """Testing DownloadDiffAction.get_hdiden on diff viewer revision page + def test_get_hidden_on_diff_viewer_revision(self) -> None: + """Testing DownloadDiffAction.get_visible on diff viewer revision page """ - self.request.resolver_match = Mock() - self.request.resolver_match.url_name = 'view-diff-revision' - - self.assertFalse(self.action.get_hidden({'request': self.request})) + self.assertTrue(self.action.get_visible( + context=self._create_request_context( + url_name='view-diff-revision'))) - def test_get_hidden_on_review_request(self): - """Testing DownloadDiffAction.get_hdiden on diff viewer revision page + def test_get_hidden_on_review_request(self) -> None: + """Testing DownloadDiffAction.get_visible on diff viewer revision page """ - self.request.resolver_match = Mock() - self.request.resolver_match.url_name = 'review-request-detail' - - review_request = self.create_review_request() - - self.assertFalse(self.action.get_hidden({ - 'request': self.request, - 'review_request': review_request, - })) + self.assertTrue(self.action.get_visible( + context=self._create_request_context( + url_name='review-request-detail'))) - def test_should_render_on_diff_viewer(self): + def test_should_render_on_diff_viewer(self) -> None: """Testing DownloadDiffAction.should_render on diff viewer page""" - self.request.resolver_match = Mock() - self.request.resolver_match.url_name = 'view-diff' - - review_request = self.create_review_request() - - self.assertTrue(self.action.should_render({ - 'request': self.request, - 'review_request': review_request, - })) + self.assertTrue(self.action.should_render( + context=self._create_request_context( + url_name='view-diff'))) - def test_should_render_on_interdiff(self): + def test_should_render_on_interdiff(self) -> None: """Testing DownloadDiffAction.should_render on diff viewer interdiff page """ - self.request.resolver_match = Mock() - self.request.resolver_match.url_name = 'view-interdiff' - - review_request = self.create_review_request() - - self.assertTrue(self.action.should_render({ - 'request': self.request, - 'review_request': review_request, - })) + self.assertTrue(self.action.should_render( + context=self._create_request_context( + url_name='view-diff-revision'))) - def test_should_render_on_diff_viewer_revision(self): + def test_should_render_on_diff_viewer_revision(self) -> None: """Testing DownloadDiffAction.should_render on diff viewer revision page """ - self.request.resolver_match = Mock() - self.request.resolver_match.url_name = 'view-diff-revision' + self.assertTrue(self.action.should_render( + context=self._create_request_context( + url_name='view-diff-revision'))) - review_request = self.create_review_request() + @add_fixtures(['test_scmtools']) + def test_should_render_on_review_request_with_repository(self) -> None: + """Testing DownloadDiffAction.should_render on review request page + with repository but no diff + """ + review_request = self.create_review_request(create_repository=True) - self.assertTrue(self.action.should_render({ - 'request': self.request, - 'review_request': review_request, - })) + self.assertFalse(self.action.should_render( + context=self._create_request_context( + review_request=review_request, + url_name='review-request-detail'))) @add_fixtures(['test_scmtools']) - def test_should_render_on_review_request_with_repository(self): + def test_should_render_on_review_request_with_repository_and_diff( + self, + ) -> None: """Testing DownloadDiffAction.should_render on review request page - with repository + with repository and diff history """ - self.request.resolver_match = Mock() - self.request.resolver_match.url_name = 'review-request-detail' - review_request = self.create_review_request(create_repository=True) + self.create_diffset(review_request) - self.assertTrue(self.action.should_render({ - 'request': self.request, - 'review_request': review_request, - })) + self.assertTrue(self.action.should_render( + context=self._create_request_context( + review_request=review_request, + url_name='review-request-detail'))) @add_fixtures(['test_scmtools']) - def test_should_render_on_review_request_without_repository(self): + def test_should_render_on_review_request_without_repository(self) -> None: """Testing DownloadDiffAction.should_render on review request page without repository """ - self.request.resolver_match = Mock() - self.request.resolver_match.url_name = 'review-request-detail' - - review_request = self.create_review_request() - - self.assertFalse(self.action.should_render({ - 'request': self.request, - 'review_request': review_request, - })) + self.assertFalse(self.action.should_render( + context=self._create_request_context( + url_name='review-request-detail'))) class EditReviewActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): @@ -788,10 +691,22 @@ class EditReviewActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): action = EditReviewAction() fixtures = ['test_users'] - def _create_request_context(self, *args, **kwargs): + def _create_request_context( + self, + url_name: str = 'review-request-detail', + user: Optional[User] = None, + *args, + **kwargs, + ) -> Context: """Create and return objects for use in the request context. Args: + url_name (str, optional): + The URL name to fake on the resolver. + + user (django.contrib.auth.models.User, optional): + The user to set on the request. + *args (tuple): Positional arguments (unused). @@ -799,24 +714,40 @@ class EditReviewActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): Keyword arguments (unused). Returns: - dict: + django.template.Context: Additional context to use when testing read-only actions. """ - return { - 'request': self.request, - } + return Context({ + 'request': self.create_http_request(url_name=url_name, + user=user), + }) - def test_should_render_with_authenticated(self): + def test_should_render_with_authenticated(self) -> None: """Testing EditReviewAction.should_render with authenticated user""" - self.request.user = User.objects.get(username='doc') - self.assertTrue(self.action.should_render( - self._create_request_context())) + with override_feature_check(unified_banner_feature.feature_id, False): + self.assertTrue(self.action.should_render( + context=self._create_request_context( + user=User.objects.get(username='doc')))) - def test_should_render_with_anonymous(self): + def test_should_render_with_anonymous(self) -> None: """Testing EditReviewAction.should_render with authenticated user""" - self.request.user = AnonymousUser() - self.assertFalse(self.action.should_render( - self._create_request_context())) + with override_feature_check(unified_banner_feature.feature_id, False): + self.assertFalse(self.action.should_render( + context=self._create_request_context())) + + def test_should_render_with_user_in_read_only(self) -> None: + """Testing EditReviewAction.should_render with authenticated user in + read-only mode + """ + with override_feature_check(unified_banner_feature.feature_id, False): + super().test_should_render_with_user_in_read_only() + + def test_should_render_with_superuser_in_read_only(self) -> None: + """Testing EditReviewAction.should_render with superuser in read-only + mode + """ + with override_feature_check(unified_banner_feature.feature_id, False): + super().test_should_render_with_superuser_in_read_only() class ShipItActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): @@ -825,10 +756,22 @@ class ShipItActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): action = ShipItAction() fixtures = ['test_users'] - def _create_request_context(self, *args, **kwargs): + def _create_request_context( + self, + url_name: str = 'review-request-detail', + user: Optional[User] = None, + *args, + **kwargs, + ) -> Context: """Create and return objects for use in the request context. Args: + url_name (str, optional): + The URL name to fake on the resolver. + + user (django.contrib.auth.models.User, optional): + The user to set on the request. + *args (tuple): Positional arguments (unused). @@ -836,24 +779,40 @@ class ShipItActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): Keyword arguments (unused). Returns: - dict: + django.template.Context: Additional context to use when testing read-only actions. """ - return { - 'request': self.request, - } + return Context({ + 'request': self.create_http_request(url_name=url_name, + user=user), + }) - def test_should_render_with_authenticated(self): + def test_should_render_with_authenticated(self) -> None: """Testing ShipItAction.should_render with authenticated user""" - self.request.user = User.objects.get(username='doc') - self.assertTrue(self.action.should_render( - self._create_request_context())) + with override_feature_check(unified_banner_feature.feature_id, False): + self.assertTrue(self.action.should_render( + context=self._create_request_context( + user=User.objects.get(username='doc')))) - def test_should_render_with_anonymous(self): + def test_should_render_with_anonymous(self) -> None: """Testing ShipItAction.should_render with authenticated user""" - self.request.user = AnonymousUser() - self.assertFalse(self.action.should_render( - self._create_request_context())) + with override_feature_check(unified_banner_feature.feature_id, False): + self.assertFalse(self.action.should_render( + context=self._create_request_context())) + + def test_should_render_with_user_in_read_only(self) -> None: + """Testing ShipItAction.should_render with authenticated user in + read-only mode + """ + with override_feature_check(unified_banner_feature.feature_id, False): + super().test_should_render_with_user_in_read_only() + + def test_should_render_with_superuser_in_read_only(self) -> None: + """Testing ShipItAction.should_render with superuser in read-only + mode + """ + with override_feature_check(unified_banner_feature.feature_id, False): + super().test_should_render_with_superuser_in_read_only() class SubmitActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): @@ -862,10 +821,20 @@ class SubmitActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): action = SubmitAction() fixtures = ['test_users'] - def _create_request_context(self, public=True, user=None, *args, **kwargs): + def _create_request_context( + self, + url_name: str = 'review-request-detail', + public: bool = True, + user: Optional[User] = None, + *args, + **kwargs, + ) -> Context: """Create and return objects for use in the request context. Args: + url_name (str, optional): + The URL name to fake on the resolver. + public (bool, optional): Whether the review request should be public. @@ -879,27 +848,28 @@ class SubmitActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): Keyword arguments (unused). Returns: - dict: + django.template.Context: Additional context to use when testing read-only actions. """ review_request = self.create_review_request(public=public) - self.request.user = user or review_request.submitter - return { - 'request': self.request, + return Context({ + 'request': self.create_http_request( + user=user or review_request.submitter, + url_name=url_name), 'review_request': review_request, - } + }) - def test_should_render_with_published(self): + def test_should_render_with_published(self) -> None: """Testing SubmitAction.should_render with published review request""" self.assertTrue(self.action.should_render( - self._create_request_context(public=True))) + context=self._create_request_context(public=True))) - def test_should_render_with_unpublished(self): + def test_should_render_with_unpublished(self) -> None: """Testing SubmitAction.should_render with unpublished review request """ self.assertFalse(self.action.should_render( - self._create_request_context(public=False))) + context=self._create_request_context(public=False))) class UpdateMenuActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): @@ -908,19 +878,26 @@ class UpdateMenuActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): action = UpdateMenuAction() fixtures = ['test_users'] - def _create_request_context(self, - public=True, - status=ReviewRequest.PENDING_REVIEW, - user=None, - can_edit_reviewrequest=True, - *args, **kwargs): + def _create_request_context( + self, + url_name: str = 'review-request-detail', + public: bool = True, + status: str = ReviewRequest.PENDING_REVIEW, + user: Optional[User] = None, + can_edit_reviewrequest: bool = True, + *args, + **kwargs, + ) -> Context: """Create and return objects for use in the request context. Args: + url_name (str, optional): + The URL name to fake on the resolver. + public (bool, optional): Whether the review request should be public. - status (unicode, optional): + status (str, optional): Review request status. user (django.contrib.auth.models.User, optional): @@ -937,59 +914,58 @@ class UpdateMenuActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): Keyword arguments (unused). Returns: - dict: + django.template.Context: Additional context to use when testing read-only actions. """ review_request = self.create_review_request(public=public, status=status) - self.request.user = user or review_request.submitter - return { + return Context({ 'review_request': review_request, - 'request': self.request, + 'request': self.create_http_request( + user=user or review_request.submitter, + url_name=url_name), 'perms': { 'reviews': { 'can_edit_reviewrequest': can_edit_reviewrequest, }, }, - } + }) - def test_should_render_for_owner(self): + def test_should_render_for_owner(self) -> None: """Testing UpdateMenuAction.should_render for owner of review request """ self.assertTrue(self.action.should_render( - self._create_request_context(can_edit_reviewrequest=False))) + context=self._create_request_context( + can_edit_reviewrequest=False))) - def test_should_render_for_user(self): + def test_should_render_for_user(self) -> None: """Testing UpdateMenuAction.should_render for normal user""" self.assertFalse(self.action.should_render( - self._create_request_context( - user=User.objects.create_user(username='test-user', - email='user@example.com'), + context=self._create_request_context( + user=self.create_user(), can_edit_reviewrequest=False))) - def test_should_render_user_with_can_edit_reviewrequest(self): + def test_should_render_user_with_can_edit_reviewrequest(self) -> None: """Testing UpdateMenuAction.should_render for user with can_edit_reviewrequest permission """ self.assertTrue(self.action.should_render( - self._create_request_context( - user=User.objects.create_user(username='test-user', - email='user@example.com')))) + context=self._create_request_context(user=self.create_user()))) - def test_should_render_with_discarded(self): + def test_should_render_with_discarded(self) -> None: """Testing UpdateMenuAction.should_render with discarded review request """ self.assertFalse(self.action.should_render( - self._create_request_context( + context=self._create_request_context( status=ReviewRequest.DISCARDED, can_edit_reviewrequest=False))) - def test_should_render_with_submitted(self): + def test_should_render_with_submitted(self) -> None: """Testing UpdateMenuAction.should_render with submitted review request """ self.assertFalse(self.action.should_render( - self._create_request_context( + context=self._create_request_context( status=ReviewRequest.SUBMITTED, can_edit_reviewrequest=False))) @@ -1000,11 +976,21 @@ class UploadDiffActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): action = UploadDiffAction() fixtures = ['test_users', 'test_scmtools'] - def _create_request_context(self, create_repository=True, user=None, - *args, **kwargs): + def _create_request_context( + self, + url_name: str = 'review-request-detail', + can_edit_reviewrequest: bool = True, + create_repository: bool = True, + user: Optional[User] = None, + *args, + **kwargs, + ) -> Context: """Create and return objects for use in the request context. Args: + url_name (str, optional): + The URL name to fake on the resolver. + create_repository (bool, optional): Whether to create a repository for the review request. @@ -1018,31 +1004,37 @@ class UploadDiffActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): Keyword arguments (unused). Returns: - dict: + django.template.Context: Additional context to use when testing read-only actions. """ review_request = self.create_review_request( create_repository=create_repository) - self.request.user = user or review_request.submitter - return { + return Context({ 'review_request': review_request, - 'request': self.request, - } + 'request': self.create_http_request( + user=user or review_request.submitter, + url_name=url_name), + 'perms': { + 'reviews': { + 'can_edit_reviewrequest': can_edit_reviewrequest, + }, + }, + }) - def test_get_label_with_no_diffs(self): + def test_get_label_with_no_diffs(self) -> None: """Testing UploadDiffAction.get_label with no diffs""" review_request = self.create_review_request() self.request.user = review_request.submitter self.assertEqual( - self.action.get_label({ + self.action.get_label(context=Context({ 'review_request': review_request, 'request': self.request, - }), + })), 'Upload Diff') - def test_get_label_with_diffs(self): + def test_get_label_with_diffs(self) -> None: """Testing UploadDiffAction.get_label with diffs""" review_request = self.create_review_request(create_repository=True) self.create_diffset(review_request) @@ -1050,18 +1042,18 @@ class UploadDiffActionTests(ReadOnlyActionTestsMixin, ActionsTestCase): self.request.user = review_request.submitter self.assertEqual( - self.action.get_label({ + self.action.get_label(context=Context({ 'review_request': review_request, 'request': self.request, - }), + })), 'Update Diff') - def test_should_render_with_repository(self): + def test_should_render_with_repository(self) -> None: """Testing UploadDiffAction.should_render with repository""" self.assertTrue(self.action.should_render( - self._create_request_context())) + context=self._create_request_context())) - def test_should_render_without_repository(self): + def test_should_render_without_repository(self) -> None: """Testing UploadDiffAction.should_render without repository""" self.assertFalse(self.action.should_render( - self._create_request_context(create_repository=False))) + context=self._create_request_context(create_repository=False))) diff --git a/reviewboard/static/rb/css/pages/review-request.less b/reviewboard/static/rb/css/pages/review-request.less index 7ebb996005ecb892b0b6550921db682158c26364..12fca4b37471315b1ed9cdbeb825e1ca1d5e07a0 100644 --- a/reviewboard/static/rb/css/pages/review-request.less +++ b/reviewboard/static/rb/css/pages/review-request.less @@ -310,37 +310,32 @@ * Review request actions ****************************************************************************/ -.action-menu() { - background: @review-request-action-menu-bg; - border: 1px @review-request-action-menu-border-color solid; - border-radius: 0 0 @box-border-radius @box-border-radius; - box-shadow: @box-shadow; - list-style: none; - margin: 0; - - li { - background: @review-request-action-bg; - border: 0; - float: none; - margin: 0; - padding: 0; - - .on-mobile-medium-screen-720({ - /* Give some extra room for tapping. */ - padding: @review-request-action-mobile-padding; - }); - - &:last-child { - border-radius: 0 0 @box-border-radius @box-border-radius; - } - - &:hover { - background-color: @review-request-action-menu-item-hover-bg; - } - } -} - -.review-request-actions-container { +/** + * Review request actions. + * + * Actions are grouped into two sections, which are left- and right-aligned. + * The left-aligned group includes the star and archive actions, and is always + * visible. The right-aligned group contains all of the editing actions, and + * will be hidden behind a disclosure when on mobile devices. + * + * Structure: + * + */ +#review-request .rb-c-actions { background: @review-request-action-bg; border-color: @review-request-action-border-color; border-radius: @box-inner-border-radius @box-inner-border-radius 0 0; @@ -358,136 +353,146 @@ .review-ui-box.has-review-ui-box-content & { border-radius: @box-inner-border-radius @box-inner-border-radius 0 0; } -} -.review-request-actions-left { - float: left; -} + /** + * A group of review request actions. + * + * Modifiers: + * -is-left: + * The menu should be floated to the left. + * + * -is-right: + * The menu should be floated to the right. + */ + &__content { + box-sizing: border-box; + list-style: none; + margin: 0; + padding: 0; + white-space: nowrap; -.review-request-actions-right-container, -.review-request-actions-right { - float: right; -} + &.-is-left { + float: left; + } -.review-request-actions { - box-sizing: border-box; - list-style: none; - margin: 0; - padding: 0; - white-space: nowrap; -} + &.-is-right { + float: right; + } + } -.review-request-action { - float: left; + /** + * A review request action. + * + * Modifiers: + * -is-icon: + * The action only uses an icon without any additional text. + * + * Structure: + * + */ + &__action { + display: inline-block; - &:hover { - background-color: @review-request-action-hover-bg; - } + &:hover { + background: @review-request-action-hover-bg; + } + + &:active { + background: @review-request-action-active-bg; + } - &:active { - background-color: @review-request-action-active-bg; + a { + color: black; + cursor: pointer; + display: block; + margin: 0; + line-height: @review-request-action-line-height; + text-decoration: none; + padding: @review-request-action-padding-vert + @review-request-action-padding-horiz-text; + } + + &.-is-icon > a { + line-height: 0; + padding: @review-request-action-padding-vert + @review-request-action-padding-horiz-icon; + } + + &.rb-o-mobile-menu-label { + display: none; + } } - a { - color: black; - cursor: pointer; - display: block; + .rb-c-menu { + background: @review-request-action-bg; + border: 1px @review-request-action-menu-border-color solid; + border-radius: 0 0 @box-border-radius @box-border-radius; + box-shadow: @box-shadow; margin: 0; - line-height: @review-request-action-line-height; - text-decoration: none; - padding: @review-request-action-padding-vert - @review-request-action-padding-horiz-text; - - .on-mobile-medium-screen-720({ - /* Give some extra room for tapping. */ - padding: @review-request-action-mobile-padding - @review-request-action-padding-horiz-text; - }); } - .menu { - .action-menu(); + .rb-c-menu__item { + background: @review-request-action-bg; + border: 0; float: none; - } -} + margin: 0; + padding: 0; -/* - * Disable some confusing cursors and interaction when the site is in - * read-only mode. - */ -body.read-only { - .review-request-action-archive, - .review-request-action-star { - &:active, &:hover { - background-color: inherit; + background-color: @review-request-action-menu-item-hover-bg; } - & > a { - cursor: default; + &:last-child { + border-radius: 0 0 @box-border-radius @box-border-radius; } } } -.review-request-action-icon { - display: inline-block; - - > a:first-child { - line-height: 0; - padding: @review-request-action-padding-vert - @review-request-action-padding-horiz-icon; - - .on-mobile-medium-screen-720({ - padding: @review-request-action-mobile-padding; - }); - } -} - -/* - * The main text-based actions turn into a drop-down menu on mobile. - * - * The following rules do several things: - * - On desktop, we don't want to have any kind of hover/active highlighting - * on the review-request-actions-right-container (which is itself an "action" - * due to it's menu-ness). - * - Hide the mobile actions menu "..." icon on desktop, show on mobile. - * - Turn .review-request-actions-right into a drop-down menu, but a special - * one that lists its items horizontalli (with wrapping), rather than a - * vertical layout. - */ -.review-request-actions-right-container > .review-request-action:hover { - background-color: inherit; -} - -a.mobile-actions-menu-label { - display: none; -} - .on-mobile-medium-screen-720({ - a.mobile-actions-menu-label { - display: inline-block; - } + #review-request .rb-c-actions { + &__action { + &.rb-o-mobile-menu-label { + display: inline-block; + float: none; + } - .review-request-actions-right-container > .review-request-action:hover { - background-color: @review-request-action-menu-item-hover-bg; - } + a, &.-is-icon a { + /* Give some extra room for tapping. */ + padding: @review-request-action-mobile-padding; + } + } - .review-request-actions-right { - .popup-menu(); - .action-menu(); + .rb-o-mobile-menu { + .rb-c-menu(); - background: @review-request-action-bg; - left: 0; - white-space: normal; - width: 100%; + background: @review-request-action-bg; + border-color: @review-request-action-border-color; + box-sizing: border-box; + left: 0; + width: 100%; - > li { - display: inline-block; - } + &.-is-visible { + opacity: 1; + visibility: visible; + } - .has-menu:hover & { - opacity: 1; - visibility: visible; + .rb-c-actions__action { + display: block; + text-align: left; + + /* This is for submenus. Just display them inline. */ + .rb-c-menu { + border: 0; + border-radius: 0; + box-shadow: none; + opacity: 1; + padding-left: 1em; + position: inherit; + visibility: visible; + } + } } } }); diff --git a/reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js b/reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js index 98fe8f9463fd6a30210f6c499521307a4e76aca0..9eec6cb3eacefb5eeb5d8cec4040d7dc20a8cf64 100644 --- a/reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js +++ b/reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js @@ -121,10 +121,10 @@ const UpdatesBubbleView = Backbone.View.extend({ */ RB.ReviewablePageView = RB.PageView.extend({ events: _.defaults({ - 'click #review-action': '_onEditReviewClicked', - 'click #ship-it-action': '_onShipItClicked', - 'click #general-comment-action': '_onAddCommentClicked', - 'click .has-menu .has-menu': '_onMenuClicked', + 'click #action-add-general-comment': '_onAddCommentClicked', + 'click #action-edit-review': '_onEditReviewClicked', + 'click #action-ship-it': '_onShipItClicked', + 'click .rb-o-mobile-menu-label': '_onMenuClicked', }, RB.PageView.prototype.events), /** @@ -239,6 +239,28 @@ RB.ReviewablePageView = RB.PageView.extend({ _super(this).remove.call(this); }, + /** + * Return the review request editor view. + * + * Returns: + * RB.ReviewRequestEditorView: + * The review request editor view. + */ + getReviewRequestEditorView() { + return this.reviewRequestEditorView; + }, + + /** + * Return the review request editor model. + * + * Returns: + * RB.ReviewRequestEditor: + * The review request editor model. + */ + getReviewRequestEditorModel() { + return this.model.reviewRequestEditor; + }, + /** * Catch the review updated event and send the user a visual update. * @@ -412,6 +434,20 @@ RB.ReviewablePageView = RB.PageView.extend({ _onMenuClicked(e) { e.preventDefault(); e.stopPropagation(); + + const $menuButton = $(e.currentTarget).find('a'); + + const expanded = $menuButton.attr('aria-expanded'); + const target = $menuButton.attr('aria-controls'); + const $target = this.$(`#${target}`); + + if (expanded === 'false') { + $menuButton.attr('aria-expanded', 'true'); + $target.addClass('-is-visible'); + } else { + $menuButton.attr('aria-expanded', 'false'); + $target.removeClass('-is-visible'); + } }, }); diff --git a/reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.es6.js b/reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.es6.js index 378cced68c80b00e2290db8cbd25983326e00773..04979ccc4a53a97a745bca5cd362e8f84d1e08c5 100644 --- a/reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.es6.js +++ b/reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.es6.js @@ -1,8 +1,8 @@ suite('rb/pages/views/ReviewablePageView', function() { const pageTemplate = dedent`
- Edit Review - Ship It + Edit Review + Ship It `; let $editReview; @@ -17,8 +17,8 @@ suite('rb/pages/views/ReviewablePageView', function() { RB.DnDUploader.instance = null; - $editReview = $container.find('#review-action'); - $shipIt = $container.find('#ship-it-action'); + $editReview = $container.find('#action-edit-review'); + $shipIt = $container.find('#action-ship-it'); page = new RB.ReviewablePage({ checkForUpdates: false, diff --git a/reviewboard/static/rb/js/reviews/actions/views/reviewRequestActions.ts b/reviewboard/static/rb/js/reviews/actions/views/reviewRequestActions.ts new file mode 100644 index 0000000000000000000000000000000000000000..7a3d49c73b84f4ef54f4c00fc2585fc477f64dcc --- /dev/null +++ b/reviewboard/static/rb/js/reviews/actions/views/reviewRequestActions.ts @@ -0,0 +1,312 @@ +import { spina } from '@beanbag/spina'; + +import { Actions } from 'reviewboard/common/actions'; + + +/** + * The view to manage the archive menu. + * + * Version Added: + * 6.0 + */ +@spina +export class ArchiveMenuActionView extends Actions.MenuActionView { + events = { + 'click': this.onClick, + 'focusout': this.onFocusOut, + 'keydown': this.onKeyDown, + 'keyup': this.onKeyUp, + 'mouseenter': this.openMenu, + 'mouseleave': this.closeMenu, + 'touchend': this.onTouchEnd, + }; + + /********************** + * Instance variables * + **********************/ + #activationKeyDown = false; + #reviewRequest: RB.ReviewRequest; + + /** + * Initialize the view. + */ + initialize() { + super.initialize(); + + const page = RB.PageManager.getPage(); + const reviewRequestEditor = page.getReviewRequestEditorModel(); + this.#reviewRequest = reviewRequestEditor.get('reviewRequest'); + + this.listenTo(this.#reviewRequest, 'change:visibility', this.render); + } + + /** + * Render the view. + */ + onRender() { + super.onRender(); + + const visibility = this.#reviewRequest.get('visibility'); + const visible = (visibility === RB.ReviewRequest.VISIBILITY_VISIBLE); + + this.$('.rb-icon') + .toggleClass('rb-icon-archive-on', !visible) + .toggleClass('rb-icon-archive-off', visible) + .attr('title', + visible + ? _`Unarchive review request` + : _`Archive review request`); + } + + /** + * Handle a click event. + * + * Args: + * e (MouseEvent): + * The event object. + */ + protected async onClick(e: MouseEvent) { + if (!this.#activationKeyDown) { + e.preventDefault(); + e.stopPropagation(); + + const visibility = this.#reviewRequest.get('visibility'); + const visible = ( + visibility === RB.ReviewRequest.VISIBILITY_VISIBLE); + const collection = ( + visibility === RB.ReviewRequest.VISIBILITY_MUTED + ? RB.UserSession.instance.mutedReviewRequests + : RB.UserSession.instance.archivedReviewRequests) + + if (visible) { + await collection.addImmediately(this.#reviewRequest); + } else { + await collection.removeImmediately(this.#reviewRequest); + } + + this.#reviewRequest.set('visibility', + visible + ? RB.ReviewRequest.VISIBILITY_ARCHIVED + : RB.ReviewRequest.VISIBILITY_VISIBLE); + } + } + + /** + * Handle a keydown event. + * + * We use this to track whether the activation keys are being pressed + * (Enter or Space) so that we can avoid triggering the default click + * behavior, which is a shortcut to the archive functionality. + * + * Args: + * e (KeyboardEvent): + * The event object. + */ + protected onKeyDown(e: KeyboardEvent) { + if (e.key === 'Enter' || e.key === 'Space') { + this.#activationKeyDown = true; + } + + super.onKeyDown(e); + } + + /** + * Handle a keyup event. + */ + protected onKeyUp() { + this.#activationKeyDown = false; + } + + /** + * Handle a touchend event. + * + * Args: + * e (TouchEvent): + * The event object. + */ + protected onTouchEnd(e: TouchEvent) { + /* + * With mouse clicks, we allow users to click on the menu header itself + * as a shortcut for just choosing archive, but with touch events we + * can't do that because then the user would never have access to the + * menu. + * + * If we allow this event to run the default handler, it would also + * give us a 'click' event after. + */ + e.preventDefault(); + + if (this.menu.isOpen) { + this.closeMenu(); + } else { + this.openMenu(); + } + } +} + + +/** + * Base class for archive views. + * + * Version Added: + * 6.0 + */ +@spina +abstract class BaseVisibilityActionView extends Actions.ActionView { + events = { + 'click': this.#toggle, + }; + + /********************** + * Instance variables * + **********************/ + + /** The collection to use for making changes to the visibility. */ + collection: RB.BaseResource; + + /** The visibility type controlled by this action. */ + visibilityType = RB.ReviewRequest.VISIBILITY_ARCHIVED; + + #reviewRequest: RB.ReviewRequest; + + /** + * Initialize the view. + */ + initialize() { + super.initialize(); + + const page = RB.PageManager.getPage(); + const reviewRequestEditor = page.getReviewRequestEditorModel(); + this.#reviewRequest = reviewRequestEditor.get('reviewRequest'); + + this.listenTo(this.#reviewRequest, 'change:visibility', this.render); + } + + /** + * Render the view. + * + * Returns: + * BaseVisibilityActionView: + * This object, for chaining. + */ + onRender() { + this.$('span').text( + this.getLabel(this.#reviewRequest.get('visibility'))); + } + + /** + * Return the label to use for the menu item. + * + * Args: + * visibility (number): + * The visibility state of the review request. + * + * Returns: + * string: + * The label to show based on the current visibility state. + */ + abstract getLabel( + visibility: number, + ): string; + + /** + * Toggle the archive state of the review request. + * + * Args: + * e (Event): + * The event that triggered the action. + */ + async #toggle(e: Event) { + e.preventDefault(); + e.stopPropagation(); + + const visibility = this.#reviewRequest.get('visibility'); + const visible = (visibility !== this.visibilityType); + + if (visible) { + await this.collection.addImmediately(this.#reviewRequest); + } else { + await this.collection.removeImmediately(this.#reviewRequest); + } + + this.#reviewRequest.set('visibility', + visible + ? this.visibilityType + : RB.ReviewRequest.VISIBILITY_VISIBLE); + } +} + + +/** + * Archive action view. + * + * Version Added: + * 6.0 + */ +@spina +export class ArchiveActionView extends BaseVisibilityActionView { + /********************** + * Instance variables * + **********************/ + + /** The collection to use for making changes to the visibility. */ + collection = RB.UserSession.instance.archivedReviewRequests; + + /** + * Return the label to use for the menu item. + * + * Args: + * visibility (number): + * The visibility state of the review request. + * + * Returns: + * string: + * The text to use for the label. + */ + getLabel( + visibility: number, + ): string { + return visibility === this.visibilityType + ? _`Unarchive` + : _`Archive`; + } +} + + +/** + * Mute action view. + * + * Version Added: + * 6.0 + */ +@spina +export class MuteActionView extends BaseVisibilityActionView { + /********************** + * Instance variables * + **********************/ + + /** The collection to use for making changes to the visibility. */ + collection = RB.UserSession.instance.mutedReviewRequests; + + /** The visibility type controlled by this action. */ + visibilityType = RB.ReviewRequest.VISIBILITY_MUTED; + + /** + * Return the label to use for the menu item. + * + * Args: + * visibility (number): + * The visibility state of the review request. + * + * Returns: + * string: + * The text to use for the label. + */ + getLabel( + visibility: number, + ): string { + return visibility === this.visibilityType + ? _`Unmute` + : _`Mute`; + } +} diff --git a/reviewboard/static/rb/js/reviews/index.ts b/reviewboard/static/rb/js/reviews/index.ts index cb0ff5c3b541f646105198ee23ac0fc3d805023e..826f3609d5d95864ab294839d4009db44d039311 100644 --- a/reviewboard/static/rb/js/reviews/index.ts +++ b/reviewboard/static/rb/js/reviews/index.ts @@ -1 +1 @@ -export {}; +export * from './actions/views/reviewRequestActions'; diff --git a/reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js b/reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js index d1a9ba31dd6bbede5c3dca81fc7096389599cc40..314ce20f1cee938a85c24b94d0272932007cb80b 100644 --- a/reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js +++ b/reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js @@ -421,26 +421,6 @@ RB.ReviewRequestEditorView = Backbone.View.extend({ }, ], - events: { - 'click #archive-review-request-link': '_onArchiveClicked', - 'click #unarchive-review-request-link': '_onUnarchiveClicked', - 'click #mute-review-request-link': '_onMuteClicked', - 'click #unmute-review-request-link': '_onUnmuteClicked', - 'click #toggle-unarchived': '_onUnarchiveClicked', - 'click #toggle-archived': '_onArchiveClicked', - }, - - _archiveActionsTemplate: _.template(dedent` - <% if (visibility === RB.ReviewRequest.VISIBILITY_VISIBLE) { %> -
  • <%- archiveText %>
    • -
  • <%- muteText %>
    • - <% } else if (visibility === RB.ReviewRequest.VISIBILITY_ARCHIVED) { %> -
  • <%- unarchiveText %>
    • - <% } else if (visibility === RB.ReviewRequest.VISIBILITY_MUTED) { %> -
  • <%- unmuteText %>
    • - <% } %> - `), - /** * Initialize the view. */ @@ -448,8 +428,7 @@ RB.ReviewRequestEditorView = Backbone.View.extend({ _.bindAll(this, '_checkResizeLayout', '_scheduleResizeLayout', '_onCloseDiscardedClicked', '_onCloseSubmittedClicked', '_onDeleteReviewRequestClicked', '_onUpdateDiffClicked', - '_onArchiveClicked', '_onUnarchiveClicked', - '_onMuteClicked', '_onUnmuteClicked', '_onUploadFileClicked'); + '_onUploadFileClicked'); this._fieldViews = {}; this._fileAttachmentThumbnailViews = []; @@ -527,10 +506,6 @@ RB.ReviewRequestEditorView = Backbone.View.extend({ this._$main = $('#review-request-main'); this._$extra = $('#review-request-extra'); - this.listenTo(reviewRequest, 'change:visibility', - this._updateArchiveVisibility); - this._updateArchiveVisibility(); - /* * We need to show any banners before we render the fields, since the * banners can add their own fields. @@ -739,11 +714,11 @@ RB.ReviewRequestEditorView = Backbone.View.extend({ * Set up all review request actions and listens for events. */ _setupActions() { - const $closeDiscarded = this.$('#discard-review-request-action'); - const $closeSubmitted = this.$('#submit-review-request-action'); - const $deletePermanently = this.$('#delete-review-request-action'); - const $updateDiff = this.$('#upload-diff-action'); - const $uploadFile = this.$('#upload-file-action'); + const $closeDiscarded = this.$('#action-close-discarded'); + const $closeSubmitted = this.$('#action-close-completed'); + const $deletePermanently = this.$('#action-delete-review-request'); + const $updateDiff = this.$('#action-upload-diff'); + const $uploadFile = this.$('#action-upload-file'); /* * We don't want the click event filtering from these down to the @@ -1138,132 +1113,6 @@ RB.ReviewRequestEditorView = Backbone.View.extend({ uploadDialog.show(); }, - /** - * Handle a click on "Archive -> Archive". - * - * Returns: - * boolean: - * False, always. - */ - _onArchiveClicked() { - return this._updateArchiveState( - RB.UserSession.instance.archivedReviewRequests, - true, - RB.ReviewRequest.VISIBILITY_ARCHIVED); - }, - - /** - * Handle a click on "Archive -> Unarchive". - * - * Returns: - * boolean: - * False, always. - */ - _onUnarchiveClicked() { - return this._updateArchiveState( - RB.UserSession.instance.archivedReviewRequests, - false, - RB.ReviewRequest.VISIBILITY_VISIBLE); - }, - - /** - * Handle a click on "Archive -> Mute". - * - * Returns: - * boolean: - * False, always. - */ - _onMuteClicked() { - return this._updateArchiveState( - RB.UserSession.instance.mutedReviewRequests, - true, - RB.ReviewRequest.VISIBILITY_MUTED); - }, - - /** - * Handle a click on "Archive -> Unmute". - * - * Returns: - * boolean: - * False, always. - */ - _onUnmuteClicked() { - return this._updateArchiveState( - RB.UserSession.instance.mutedReviewRequests, - false, - RB.ReviewRequest.VISIBILITY_VISIBLE); - }, - - /** - * Update archive/mute state. - * - * Args: - * collection (Backbone.Collection): - * The collection representing the user's archived or muted review - * requests. - * - * add (boolean): - * True if the review request should be added to the collection - * (archived or muted), false if it shold be removed (unarchived or - * unmuted). - * - * newState (number): - * The new state for the review request's ``visibility`` attribute. - * - * Returns: - * boolean: - * False, always. - */ - async _updateArchiveState(collection, add, newState) { - const reviewRequest = this.model.get('reviewRequest'); - - if (add) { - await collection.addImmediately(reviewRequest); - } else { - await collection.removeImmediately(reviewRequest); - } - - reviewRequest.set('visibility', newState); - - return false; - }, - - /** - * Update the visibility of the archive/mute menu items. - */ - _updateArchiveVisibility() { - const visibility = this.model.get('reviewRequest').get('visibility'); - - this.$('#hide-review-request-menu').html(this._archiveActionsTemplate({ - visibility: visibility, - archiveText: gettext('Archive'), - muteText: gettext('Mute'), - unarchiveText: gettext('Unarchive'), - unmuteText: gettext('Unmute'), - })); - - const visible = (visibility === RB.ReviewRequest.VISIBILITY_VISIBLE); - - const iconClass = (visible - ? 'rb-icon-archive-off' - : 'rb-icon-archive-on'); - - const iconTitle = (visible - ? gettext('Archive review request') - : gettext('Unarchive review request')); - - const iconId = (visible - ? 'toggle-archived' - : 'toggle-unarchived'); - - this.$('#hide-review-request-link') - .html(``); - - if (RB.UserSession.instance.get('readOnly')) { - this.$('#hide-review-request-menu').hide(); - } - }, - /** * Refresh the page. */ diff --git a/reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.es6.js b/reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.es6.js index 6ef7fd882ccd1f7dc02d02a3df76e4d27f1b9290..632b3067d7d71f591a0101aefdad8be9c9215a69 100644 --- a/reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.es6.js +++ b/reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.es6.js @@ -217,66 +217,6 @@ suite('rb/views/ReviewRequestEditorView', function() { }); describe('Actions bar', function() { - describe('Close', function() { - beforeEach(function() { - view.render(); - }); - - it('Delete Permanently', function(done) { - let $buttons = $(); - - spyOn(reviewRequest, 'destroy').and.resolveTo(); - spyOn($.fn, 'modalBox').and.callFake(options => { - options.buttons.forEach($btn => { - $buttons = $buttons.add($btn); - }); - - /* Simulate the modalBox API for what we need. */ - return { - modalBox: cmd => { - expect(cmd).toBe('buttons'); - return $buttons; - } - }; - }); - - $('#delete-review-request-action').click(); - expect($.fn.modalBox).toHaveBeenCalled(); - - /* This gets called at the end of the operation. */ - spyOn(RB, 'navigateTo').and.callFake(() => { - expect(reviewRequest.destroy).toHaveBeenCalled(); - done(); - }); - - $buttons.filter('input[value="Delete"]').click(); - }); - - it('Discarded', function() { - spyOn(reviewRequest, 'close').and.callFake(options => { - expect(options.type).toBe(RB.ReviewRequest.CLOSE_DISCARDED); - return Promise.resolve(); - }); - - spyOn(window, 'confirm').and.returnValue(true); - - $('#discard-review-request-action').click(); - - expect(reviewRequest.close).toHaveBeenCalled(); - }); - - it('Submitted', function() { - spyOn(reviewRequest, 'close').and.callFake(options => { - expect(options.type).toBe(RB.ReviewRequest.CLOSE_SUBMITTED); - return Promise.resolve(); - }); - - $('#submit-review-request-action').click(); - - expect(reviewRequest.close).toHaveBeenCalled(); - }); - }); - it('ReviewRequestActionHooks', function() { var MyExtension, extension, diff --git a/reviewboard/templates/reviews/action.html b/reviewboard/templates/reviews/action.html deleted file mode 100644 index 7aeeb65987ffd30bdd3a885d4972287429a7672a..0000000000000000000000000000000000000000 --- a/reviewboard/templates/reviews/action.html +++ /dev/null @@ -1,3 +0,0 @@ -
  • - -
    • diff --git a/reviewboard/templates/reviews/archive_action.html b/reviewboard/templates/reviews/archive_action.html new file mode 100644 index 0000000000000000000000000000000000000000..bba2db5e34bcf72321240a396666b9239f055d7f --- /dev/null +++ b/reviewboard/templates/reviews/archive_action.html @@ -0,0 +1,3 @@ + diff --git a/reviewboard/templates/reviews/archive_menu_action.html b/reviewboard/templates/reviews/archive_menu_action.html new file mode 100644 index 0000000000000000000000000000000000000000..9203bc80f1ded42561b73afc809d6167f679a4ad --- /dev/null +++ b/reviewboard/templates/reviews/archive_menu_action.html @@ -0,0 +1,11 @@ +{% load actions %} + diff --git a/reviewboard/templates/reviews/menu_action.html b/reviewboard/templates/reviews/menu_action.html deleted file mode 100644 index 77c653703b3a7cd90fab84dfeaab3c036fd40965..0000000000000000000000000000000000000000 --- a/reviewboard/templates/reviews/menu_action.html +++ /dev/null @@ -1,9 +0,0 @@ -{% load reviewtags %} -
  • - {{menu_action.label}} - -
      -{% child_actions %} -
    -
    • diff --git a/reviewboard/templates/reviews/review_request_header.html b/reviewboard/templates/reviews/review_request_header.html index 7328669f8a4e2560e0cb66202bd627f829cb5164..3f1b1599527cd9bb7f98919772a66a334e659600 100644 --- a/reviewboard/templates/reviews/review_request_header.html +++ b/reviewboard/templates/reviews/review_request_header.html @@ -1,36 +1,33 @@ -{% load reviewtags %} +{% load actions %}
    -{% for tab in tabs %} +{% for tab in tabs %} -{% endfor %} +{% endfor %} -
    -
      -{% if request.user.is_authenticated and review_request.status == 'P' %} -
    • - {% star review_request %} +
    -
      -
    • - -
        -{% review_request_actions %} -
      -
      • -
    +
    +{% actions_html "review-request" %} +
    +
    diff --git a/reviewboard/templates/reviews/star_action.html b/reviewboard/templates/reviews/star_action.html new file mode 100644 index 0000000000000000000000000000000000000000..d1db1fa491b7cd44ac26f28743ed5ae7049e74a1 --- /dev/null +++ b/reviewboard/templates/reviews/star_action.html @@ -0,0 +1,4 @@ +{% load reviewtags %} +