diff --git a/docs/manual/extending/extensions/hooks/review-request-approval-hook.rst b/docs/manual/extending/extensions/hooks/review-request-approval-hook.rst index 1688e21d3e9252b5ede4232a85ae043bfd14943f..478b6ca40cfd275f74389d6c54ec8d716bdb9e92 100644 --- a/docs/manual/extending/extensions/hooks/review-request-approval-hook.rst +++ b/docs/manual/extending/extensions/hooks/review-request-approval-hook.rst @@ -4,35 +4,128 @@ ReviewRequestApprovalHook ========================= -In Review Board 2.0, review requests have a concept of "approval." This is a -flag exposed in the API on :ref:`webapi2.0-review-request-resource` that -indicates if the change on the review request has met the necessary -requirements to be committed to the codebase. Pre-commit hooks on the -repository can use this to allow or prevent check-ins. +Review Board exposes an *approval* state for review requests through the +API. This state indicates whether a change has met whatever criteria an +organization requires before it can be committed or merged. -.. note:: +Review Board itself **does not enforce** approval. Approval states are +instead made available to tooling such as: - Note that this is purely for integration with extensions and consumers of - the API. Review Board does not use approval to enforce any actions itself.) +* Pre-commit, pre-push, or pre-receive hooks (for example, `RBTools's + repository hooks`_). -By default, the flag is set if there's at least one Ship It! and no open -issues, but custom logic can be provided by an extension. +* CI or merge gate systems -:py:class:`reviewboard.extensions.hooks.ReviewRequestApprovalHook` allows -an extension to make a decision on whether a review request is approved. +* :ref:`Extensions `, bots, + :ref:`integrations `, or in-house tools -To use it, simply subclass and provide a custom :py:meth:`is_approved` -function. This takes the review request, the previously calculated approved -state, and the previously calculated approval failure string. (Both the -previously calculated values may come from another -:py:class:`ReviewRequestApprovalHook` or the initial approval checks.) +The results of approval are available in two places: -Based on that information and its calculations, it can return the new -approval state and optional failure reason, which will be reflected in the -API. +1. The :ref:`review request API ` (as + ``approved`` and ``approval_failure`` fields). -Most often, a hook will want to return ``False`` if the previous approved -value is ``False``, and pass along the previous failure reason as well. +2. The :py:class:`~reviewboard.reviews.models.ReviewRequest` model accessible + by extensions (as :py:attr:`~reviewboard.reviews.models.ReviewRequest. + approved` and :py:attr:`~reviewboard.reviews.models.ReviewRequest. + approval_failure` properties). + +By default, a review request is considered approved if it has at least one +:ref:`Ship It! ` and no :ref:`open issues `. +Extensions can override or extend this logic using +:py:class:`~reviewboard.extensions.hooks.ReviewRequestApprovalHook`. + + +.. _RBTools's repository hooks: + https://github.com/reviewboard/rbtools/tree/master/contrib/tools + + +Overview +======== + +:py:class:`~reviewboard.extensions.hooks.ReviewRequestApprovalHook` +participates in a *chain* of approval checks. Each hook: + +* Receives the review request +* Receives the previously-computed approval state +* Receives the previous failure reason (if any) +* Returns a new approval state and optional failure reason + +Multiple approval hooks may be registered. Hooks are evaluated in +registration order. + + +Execution Model +=============== + +Each approval hook must implement an ``is_approved`` method with the +following signature. This method will be part of a chain of calls used to +determine approval. + +.. code-block:: python + + def is_approved( + self, + review_request: ReviewRequest, + prev_approved: bool, + prev_failure: str | None, + ) -> bool | tuple[bool, str | None]: + ... + +It's called with the following arguments: + +* ``review_request``: The review request being evaluated +* ``prev_approved``: The approval result computed so far +* ``prev_failure``: The failure message associated with the most recent + ``prev_approved=False`` result + +A hook may do any of the following: + +* Preserve the existing approval state +* Add additional requirements for approval +* Override previous results (though this must be done with great care) + +The result may be one of the following: + +1. A boolean result (``True`` to approve, ``False`` to reject while + preserving any existing failure reason, if present). + +2. A tuple in the form of ``(approved, failure_reason)``. + + This form is preferred over simply returning ``False``. + +.. important:: + + If a hook returns ``False``, later hooks will still be called. The value + returned by each hook becomes the input to the next hook. + + Hooks must explicitly preserve failure state if that is the desired + behavior. + + Most hooks should treat ``prev_approved=False`` as a hard stop. This allows + multiple hooks to cooperatively build approval policy, but also means hooks + must be written to coexist with others. + + +Failure Messages +================ + +Failure messages are exposed through the API and may be surfaced by +external tools or extensions. + +* Only one failure message is retained at a time. +* Hooks should always propagate ``prev_failure`` when preserving failures. +* Messages should be short, actionable, and user-facing. + + +Best Practices +============== + +* Treat ``prev_approved=False`` as authoritative unless you have a strong + reason not to. + +* Keep approval logic fast and side-effect-free. + +* Do not assume your hook is the only one installed. Example @@ -54,21 +147,89 @@ Example self, review_request: ReviewRequest, prev_approved: bool, - prev_failure: str, - ) -> bool | tuple[bool, str]: - # Require at least 2 Ship It!'s from everyone but Bob. Bob needs - # at least 3. + prev_failure: str | None, + ) -> bool | tuple[bool, str | None]: + # Always preserve prior failures. if not prev_approved: return prev_approved, prev_failure - elif (review_request.submitter.username == 'bob' and - review_request.shipit_count < 3): + + # Require stricter approval rules for Bob. He requires at least + # 3 Ship It!'s. + if (review_request.submitter.username == 'bob' and + review_request.shipit_count < 3): return False, 'Bob, you need at least 3 "Ship It!s."' - elif review_request.shipit_count < 2: + + # Default requirements are at least 2 Ship It!'s. + if review_request.shipit_count < 2: return False, 'You need at least 2 "Ship It!s."' - else: - return True + + # This has met all of this hook's requirements. + return True class SampleExtension(Extension): def initialize(self) -> None: SampleApprovalHook(self) + + +.. tip:: + + The Python type hints shown above are shown here for demonstrative + purposes only. Your own hooks can leave them out. For example: + + .. code-block:: python + + def is_approved(self, review_request, prev_approved, prev_failure): + ... + + +Common Patterns +=============== + +Pass-through with additional checks (recommended) +------------------------------------------------- + +Most hooks should preserve previous failures and only add new requirements: + +.. code-block:: python + + def is_approved( + self, + review_request: ReviewRequest, + prev_approved: bool, + prev_failure: str | None, + ) -> bool | tuple[bool, str | None]: + # Preserve any previous failures. + if not prev_approved: + return prev_approved, prev_failure + + # An example new requirement. + if not review_request.testing_done: + return False, 'Testing must be completed.' + + # This has met all of this hook's requirements. + return True + + +Override previous failures (use with caution) +--------------------------------------------- + +Hooks that ignore ``prev_approved`` override all prior approval logic. This +is rarely appropriate and can lead to unexpected behavior when multiple +extensions are installed. + +.. code-block:: python + + def is_approved( + self, + review_request: ReviewRequest, + prev_approved: bool, + prev_failure: str | None, + ) -> bool | tuple[bool, str | None]: + # If the review request's author is a special user, ignore any + # previous failures and approve this change. + if review_request.submitter.username == 'special-user': + return True + + # Return the result from any previous hook, if any. + return prev_approved, prev_failure diff --git a/docs/manual/users/reviews/reviews.rst b/docs/manual/users/reviews/reviews.rst index 43de5e8f23d8d4f23d6db8aa146eace9594a0bb1..ac0a303b892466573ed2f29c732449f69bc81000 100644 --- a/docs/manual/users/reviews/reviews.rst +++ b/docs/manual/users/reviews/reviews.rst @@ -30,6 +30,8 @@ be edited until published using the :ref:`review banner Let's take a look at the parts of a review: +.. _ship-it: + 1. Ship It! =========== diff --git a/reviewboard/extensions/hooks/review_request_approval.py b/reviewboard/extensions/hooks/review_request_approval.py index 36370db98e717d9ffabff7c3438d6a09742e169b..49a3d2d4d215ed101814f50b053b20b2609b2b50 100644 --- a/reviewboard/extensions/hooks/review_request_approval.py +++ b/reviewboard/extensions/hooks/review_request_approval.py @@ -2,8 +2,13 @@ from __future__ import annotations +from typing import TYPE_CHECKING + from djblets.extensions.hooks import ExtensionHook, ExtensionHookPoint +if TYPE_CHECKING: + from reviewboard.reviews.models import ReviewRequest + class ReviewRequestApprovalHook(ExtensionHook, metaclass=ExtensionHookPoint): """A hook for determining if a review request is approved. @@ -11,9 +16,19 @@ class ReviewRequestApprovalHook(ExtensionHook, metaclass=ExtensionHookPoint): Extensions can use this to hook into the process for determining review request approval, which may impact any scripts integrating with Review Board to, for example, allow committing to a repository. + + .. seealso:: + + * :ref:`ReviewRequestApprovalHook Developer Guide + ` """ - def is_approved(self, review_request, prev_approved, prev_failure): + def is_approved( + self, + review_request: ReviewRequest, + prev_approved: bool, + prev_failure: str | None, + ) -> bool | tuple[bool, str | None]: """Determine if the review request is approved. This function is provided with the review request and the previously @@ -39,7 +54,7 @@ class ReviewRequestApprovalHook(ExtensionHook, metaclass=ExtensionHookPoint): The previously-calculated approval result, either from another hook or by Review Board. - prev_failure (unicode): + prev_failure (str): The previously-calculated approval failure message, either from another hook or by Review Board. diff --git a/reviewboard/reviews/models/review_request.py b/reviewboard/reviews/models/review_request.py index 6dc86e6d6afaf3e7f404a086ab3f50b5ff336046..f1e46d6928e1a03820b025cc6a2d222d3729d655 100644 --- a/reviewboard/reviews/models/review_request.py +++ b/reviewboard/reviews/models/review_request.py @@ -605,11 +605,12 @@ class ReviewRequest(BaseReviewRequestDetails): at least one Ship It!, and doesn't have any open issues. Extensions may customize approval by providing their own - ReviewRequestApprovalHook. + :py:class:`~reviewboard.extensions.hooks.ReviewRequestApprovalHook`. - Returns: - bool: - Whether the review request is approved. + .. seealso:: + + * :ref:`ReviewRequestApprovalHook Developer Guide + ` """ if not hasattr(self, '_approved'): self._calculate_approval() @@ -617,17 +618,19 @@ class ReviewRequest(BaseReviewRequestDetails): return self._approved @property - def approval_failure(self) -> str: + def approval_failure(self) -> str | None: """An error indicating why a review request isn't approved. If ``approved`` is ``False``, this will provide the text describing why it wasn't approved. Extensions may customize approval by providing their own - ReviewRequestApprovalHook. + :py:class:`~reviewboard.extensions.hooks.ReviewRequestApprovalHook`. - Type: - str + .. seealso:: + + * :ref:`ReviewRequestApprovalHook Developer Guide + ` """ if not hasattr(self, '_approval_failure'): self._calculate_approval()