Review process

source/FurtherDetails/who.rst

@@ -80,8 +80,8 @@ Sci/Tech Reviewer
 -----------------
 
 A Sci/Tech reviewer is assigned for every ticket and comprises the first stage
-of review that considers the change as a whole. Further details are linked
-from the :ref:`Working Practices page<scitech>`. In some cases, the reviewer
+of review that considers the change as a whole. Further details are found in
+the :ref:`Scitech Reviewers Guide<scitech_review>`. In some cases, the reviewer
 can delegate parts of the work to another person.
 
 Reviews should be turned around on a reasonable timescale and follow the
@@ -97,7 +97,7 @@ Code Reviewer
 -------------
 
 The Code Reviewer performs the 2nd stage of review for every ticket. Further
-details are described :ref:`Working Practices page<codereview>`.
+details are described in the :ref:`Code Reviewers Guide<code_review>`.
 
 Reviews should be turned around on a reasonable timescale and follow the Code
 Review guidance.

source/Reviewers/codereview.rst

@@ -3,6 +3,13 @@
 Code and System Review
 ======================
 
+.. tip::
+
+  GitHub documentation on the review process and interface:
+  `Reviewing Proposed Changes in a Pull Request <https://docs.github.com/en/pull
+  -requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/
+  reviewing-proposed-changes-in-a-pull-request>`_
+
 Purpose of the review
 ---------------------
 The purpose of the code/system review is to analyse a change for its impact and
@@ -16,25 +23,26 @@ and should question anything that is poorly coded.
 Reviewer responsibilities and checkpoints
 -----------------------------------------
 
-The Code/System review template exists to help you think through all the areas
-of concern. A completed :ref:`review template <template>` should be appended
-to the ticket once you are finished.
+The pull request template that populates the pull request description box
+contains a Code Review section with questions to help you think through all the
+areas of concern. This Code Review section should be completed once you are
+finished.
 
 Work through the code review template considering each question in turn. These
 will include areas such as:
 
-.. dropdown:: Is the ticket and testing complete?
+.. dropdown:: Is the pull request and testing complete?
 
-    * A Ticket Summary should be attached and filled in. This includes:
+    * The Pull Request Template should be filled in. This includes:
         * Proof of :ref:`testing <testing>` completed. All tests should pass
           with the exception of any known changes in answers.
         * Approvals from the code owners for every file changed.
         * If the change affects answers then approval from the owners of the
           affected configurations.
         * If the change modifies OMP code sections then approval from the
           optimisation team.
-    * It should be possible to understand the purpose of the ticket from the
-      details provided.
+    * It should be possible to understand the purpose of the pull request from
+      the details provided.
 
     .. tip::
 
@@ -45,7 +53,7 @@ will include areas such as:
 
     .. tip::
 
-        If the ticket has been completed by a non-Met Office developer it is
+        If the pull request has been completed by a non-Met Office developer it is
         useful to run tests ourselves early in the review process as different
         compilers may behave differently. It may also be necessary to run
         tests for systems that the partner has not had access to.
@@ -63,34 +71,34 @@ will include areas such as:
         * had the reconfiguration been altered? If so the UM testing must
           include the `recon` group
         * is there another rose-stem group that covers this area of code?
-          See :ref:`um_testing` for common examples
 
-.. dropdown:: Is this a :ref:`multi-repository<multirepo>` ticket?
+
+.. dropdown:: Is this a :ref:`multi-repository<multirepo>` pull request?
 
     Each of the repositories covered by these WPs have overlapping use of
     code.
 
-    The Ticket Summary/Code Review templates in each repository contain the
+    The pull request templates in each repository contain the
     details of when testing against other repositories are required. These
     highlight where the code is likely to interact. *e.g. if code in the
     shared/science folder in JULES is modified then both the UM and LFRIc Apps
     test suites will need to be run with that change.*
 
     If this testing doesn't pass then either
 
-        a) the change in ticket will need modifying so that the parent
+        a) the change in pull request will need modifying so that the parent
         repository's test suite passes
 
-        b) this change requires a linked ticket in that repository so that all
-        tests can pass.
+        b) this change requires a linked pull request in that repository so that
+        all tests can pass.
 
     .. tip::
 
-        All linked tickets are reviewed as a group. Each ticket in the group
-        should contain links to all the others and the correct keywords
-        applied to make it easier to keep track of them all.
+        All linked pull requests are reviewed as a group. Each pull request in
+        the group should contain links to all the others and the correct
+        keywords applied to make it easier to keep track of them all.
 
-        Care is needed when :ref:`committing these tickets
+        Care is needed when :ref:`committing these pull requests
         <committinglinkedtickets>`.
 
 .. dropdown:: Is the code up to scratch?
@@ -112,16 +120,19 @@ will include areas such as:
 Final decision points and actions
 ---------------------------------
 
-The ticket will likely iterate between the reviewer and the developer during
-the review process while retaining it's code review status. However, the
-reviewer has the option to "reject and assign" back to the code author should
-the documentation or code not meet the required standards and major
-alterations/improvements are required. In this case the change will need a
-further SciTech review before it can be returned to the code reviewer.
+The pull request will likely iterate between the reviewer and the developer
+during the review process while retaining it's code review status. The Code
+Reviewer can `Resolve Conversations` when they feel each query has been
+satisfactorily answered.
+
+In the case where a reviewer believes a PR requires substantial changes to be
+made in order to reach sufficient quality for commit, the PR may be closed
+without merging. This will be done in consultation with the developer and other
+Code Owners and Reviewers.
 
-Once you are happy that the change is appropriate and correct, complete the
-approval section of the Code/System review template and change the ticket
-status to **approved**.
+Once you are happy that the change is appropriate and correct, and the code
+review parts of the pull request template have been completed. Submit a
+review that `approves` the change.
 
-From here follow the :ref:`How To Commit<howtocommit>` guide through to ticket
+From here follow the :ref:`How To Commit<howtocommit>` guide through to pull request
 closure.

source/Reviewers/scitechreview.rst

@@ -3,6 +3,13 @@
 Science and Technical Review
 ============================
 
+.. tip::
+
+  GitHub documentation on the review process and interface:
+  `Reviewing Proposed Changes in a Pull Request <https://docs.github.com/en/pull
+  -requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/
+  reviewing-proposed-changes-in-a-pull-request>`_
+
 Purpose of the review
 ---------------------
 
@@ -12,9 +19,9 @@ performs, is standards compliant and well documented.
 Reviewer responsibilities and checkpoints
 -----------------------------------------
 
-The Sci/Tech review template exists to help you think through all the following
-areas. A completed :ref:`review template <template>` should be appended to the
-ticket once you are finished.
+The pull request template that populates the pull request description box
+contains a SciTech Review section with questions to help you think through all
+the areas of concern. This SciTech review section should be completed once you are finished.
 
 The Science / Technical reviewer should
 
@@ -33,11 +40,11 @@ The Science / Technical reviewer should
 * Ensure that any related :ref:`external documentation <docs>` is updated as
   necessary.
 
-* Check that the Trac ticket has been completed fully and accurately with
+* Check that the pull request has been completed fully and accurately with
   sufficient detail for others to understand the impact of the change.
 
 * Ensure that testing has been carried out satisfactorily (and recorded on the
-  Trac ticket), and that there is no impact for configurations outside the
+  pull request), and that there is no impact for configurations outside the
   required scope of the changeset.
 
 Final decision points and actions
@@ -46,13 +53,16 @@ Final decision points and actions
 The science/technical reviewer must demand that non-compliance is corrected
 before a change is passed onto the next level of review.
 
-The ticket will likely iterate between the reviewer and the developer during
-the review process while retaining it's sci/tech review status. However, the
-reviewer has the option to "reject and assign" back to the code author should
-the documentation or code not meet the required standards and major
-alterations/improvements are required.
+The pull request will likely iterate between the reviewer and the developer during
+the review process while retaining it's sci/tech review status.
+
+Once you are happy that the change is appropriate and correct, and the code
+review parts of the pull request template have been completed then change
+the review state to
+Code Review.
+
+.. image:: ../WorkingPractices/images/gh_screenshots/project_scitech_light.png
+    :class: only-light border
 
-Once you are happy that the change is appropriate and correct, complete the
-approval section of the Sci/Tech review template and re-assign the ticket to
-the system/code reviewer. If need be ask the code author for the agreed
-system/code reviewer's name.
+.. image:: ../WorkingPractices/images/gh_screenshots/project_scitech_dark.png
+    :class: only-dark border.

source/WorkingPractices/approvals.rst

@@ -3,9 +3,9 @@
 Approval Process
 ================
 
-Every ticket will need to get approval from a group of people. These approvals
-are marked on the :ref:`Ticket Summary wiki pages <template>` by those signing
-off the approval.
+Every pull request will need to get approval from a group of people. These
+approvals are marked in the pull request description by those signing off the
+approval.
 
 .. tip::
 
@@ -17,21 +17,16 @@ off the approval.
 
 .. tip::
 
-    From vn13.2 the :ref:`trac.log <traclog>` summary of your testing will also
+    The :ref:`trac.log <traclog>` summary of your testing will also
     list all the Code and Config Owner approvals required in a handy table to
     make it easy to see what is required.
 
-.. note::
-
-    LFRic Apps does not yet have a formal Code Owner approval process. A list
-    of code owners is available and they should still be consulted as part of
-    your development.
-
 .. important::
 
     New UM Ancils must be submitted to the MIAO team for approval. Please
     follow their process for `Requesting New UM Ancils
-    <https://code.metoffice.gov.uk/trac/ancil/wiki/ANTS/ProjectManagement/updating_UMDIR>`__.
+    <https://code.metoffice.gov.uk/
+    trac/ancil/wiki/ANTS/ProjectManagement/updating_UMDIR>`__.
 
 Code Owners
 -----------