fix(spp_approval): stop offering New on the approval review lists

spp_approval/README.rst

@@ -157,6 +157,16 @@ Dependencies
 Changelog
 =========
 
+19.0.2.0.2
+~~~~~~~~~~
+
+- fix(spp_approval): stop offering **New** on the approval review lists.
+  A review is created by the approval flow when a record is submitted,
+  so a hand-made one would need a model name, a record id and a
+  definition typed into a blank form and would point at nothing. Both
+  **My Pending Approvals** and **Approval Reviews** are affected, along
+  with the New in a review's own breadcrumb (#1167)
+
 19.0.2.0.1
 ~~~~~~~~~~
 

spp_approval/__manifest__.py

@@ -2,7 +2,7 @@
 {
     "name": "OpenSPP Approval",
     "summary": "Standardized approval workflows with multi-tier sequencing and CEL rules",
-    "version": "19.0.2.0.1",
+    "version": "19.0.2.0.2",
     "license": "LGPL-3",
     "development_status": "Production/Stable",
     "author": "OpenSPP.org, OpenSPP Community",

spp_approval/readme/HISTORY.md

@@ -1,3 +1,7 @@
+### 19.0.2.0.2
+
+- fix(spp_approval): stop offering **New** on the approval review lists. A review is created by the approval flow when a record is submitted, so a hand-made one would need a model name, a record id and a definition typed into a blank form and would point at nothing. Both **My Pending Approvals** and **Approval Reviews** are affected, along with the New in a review's own breadcrumb (#1167)
+
 ### 19.0.2.0.1
 
 - Fix CEL Expressions tab crash: the ace editor fields used the invalid

spp_approval/static/description/index.html

@@ -536,6 +536,17 @@ <h2><a class="toc-backref" href="#toc-entry-1">Changelog</a></h2>
 </div>
 </div>
 <div class="section" id="section-1">
+<h1>19.0.2.0.2</h1>
+<ul class="simple">
+<li>fix(spp_approval): stop offering <strong>New</strong> on the approval review lists.
+A review is created by the approval flow when a record is submitted,
+so a hand-made one would need a model name, a record id and a
+definition typed into a blank form and would point at nothing. Both
+<strong>My Pending Approvals</strong> and <strong>Approval Reviews</strong> are affected, along
+with the New in a review’s own breadcrumb (#1167)</li>
+</ul>
+</div>
+<div class="section" id="section-2">
 <h1>19.0.2.0.1</h1>
 <ul class="simple">
 <li>Fix CEL Expressions tab crash: the ace editor fields used the invalid
@@ -546,7 +557,7 @@ <h1>19.0.2.0.1</h1>
 ternaries, which it highlights correctly.</li>
 </ul>
 </div>
-<div class="section" id="section-2">
+<div class="section" id="section-3">
 <h1>19.0.2.0.0</h1>
 <ul class="simple">
 <li>Initial migration to OpenSPP2</li>

spp_approval/tests/__init__.py

@@ -5,3 +5,4 @@
 from . import test_approval_security
 from . import test_cel_evaluator_security
 from . import test_cel_view
+from . import test_approval_review_no_create

spp_approval/tests/test_approval_review_no_create.py

@@ -0,0 +1,117 @@
+# Part of OpenSPP. See LICENSE file for full copyright and licensing details.
+"""OP#1167: a review is never created by hand, so no view offers a New button.
+
+The approval mixin creates a review when a record is submitted, and
+``create()`` builds the tier reviews from the definition. A review typed into a
+blank form would need a model name, a record id and a definition entered by
+hand, and would point at nothing — which is why the New button on **My Pending
+Approvals** was reported as not meant to work there.
+
+Both actions on ``spp.approval.review`` share these two views, so the attribute
+belongs on the views rather than on one action's context.
+"""
+
+from ast import literal_eval
+
+from lxml import etree
+
+from odoo.tests import TransactionCase, tagged
+
+VIEWS = [
+    ("spp_approval.approval_review_view_tree", "list"),
+    ("spp_approval.approval_review_view_form", "form"),
+]
+
+ACTIONS = [
+    "spp_approval.approval_review_my_pending_action",
+    "spp_approval.approval_review_action",
+]
+
+
+@tagged("post_install", "-at_install")
+class TestApprovalReviewNoCreate(TransactionCase):
+    def test_no_view_offers_a_new_button(self):
+        """`create` on the root node is what the New button reads.
+
+        For a plain list or form, ``activeActions.create`` comes straight from
+        this attribute (web/views/utils.js). An action context of
+        ``{'create': False}`` is not a separate mechanism — it rewrites this
+        same attribute (web/views/view.js) — so setting it on the view covers
+        every action that uses the view, including any added later.
+        """
+        for xml_id, view_type in VIEWS:
+            with self.subTest(view=xml_id):
+                view = self.env.ref(xml_id)
+                # The combined arch, not the record's own: spp_approval's
+                # multitier views inherit both of these, and it is the merged
+                # result the client renders.
+                combined = self.env["spp.approval.review"].get_view(view.id, view_type)
+                root = etree.fromstring(combined["arch"])
+
+                self.assertEqual(root.tag, view_type)
+                self.assertEqual(
+                    root.get("create"),
+                    "0",
+                    f"{xml_id} would still offer New once inheritance is applied",
+                )
+
+    def test_both_actions_use_those_views(self):
+        """Guards the reason the attribute lives on the views.
+
+        The report was about My Pending Approvals, but the sibling Approval
+        Reviews action lists the same model through the same views and had the
+        same button. If an action ever stops using them, this fails and the
+        create-suppression needs revisiting rather than silently lapsing.
+        """
+        for xml_id in ACTIONS:
+            with self.subTest(action=xml_id):
+                action = self.env.ref(xml_id)
+
+                self.assertEqual(action.res_model, "spp.approval.review")
+                self.assertFalse(
+                    action.view_id,
+                    f"{xml_id} pins a specific view; check it denies create too",
+                )
+                self.assertEqual(action.view_mode, "list,form")
+
+    def test_a_review_still_reaches_the_list(self):
+        """The views are read-only entry points, not disabled ones.
+
+        create="0" must not stop reviews created by the approval flow from
+        being listed and opened — that is the whole purpose of the view.
+
+        The definition is built here rather than searched for. Searching and
+        skipping made this leg of the safety net green-by-skip in exactly the
+        environment that gates merges: the CI database ships no approval
+        definition, so it only ever ran locally (#447 review).
+        """
+        definition = self.env["spp.approval.definition"].create(
+            {
+                "name": "OP#1167 listing check",
+                "model_id": self.env["ir.model"]._get_id("res.partner"),
+                "approval_type": "group",
+                "approval_group_id": self.env.ref("base.group_user").id,
+            }
+        )
+        review = self.env["spp.approval.review"].create(
+            {
+                "model": "res.partner",
+                "res_id": self.env.user.partner_id.id,
+                "definition_id": definition.id,
+            }
+        )
+
+        # Read it the way the action does, not with a bare search([]): the
+        # action's own domain is what decides whether the record reaches the
+        # list a user actually opens.
+        action = self.env.ref("spp_approval.approval_review_my_pending_action")
+        listed = self.env["spp.approval.review"].search(literal_eval(action.domain))
+        self.assertIn(review, listed, "a pending review no longer reaches My Pending Approvals")
+
+        # And it opens: every column the list arch renders must be readable on
+        # that record. create="0" suppresses New, nothing else.
+        list_view = self.env.ref("spp_approval.approval_review_view_tree")
+        arch = etree.fromstring(self.env["spp.approval.review"].get_view(list_view.id, "list")["arch"])
+        columns = [node.get("name") for node in arch.xpath("//field[@name]")]
+        self.assertTrue(columns, "the list arch renders no columns")
+        self.assertTrue(review.read(columns), "the review does not open through the list view")

spp_approval/views/approval_review_views.xml

@@ -5,7 +5,20 @@
         <field name="name">spp.approval.review.tree</field>
         <field name="model">spp.approval.review</field>
         <field name="arch" type="xml">
+            <!--
+                create="0": a review is never made by hand. The approval mixin
+                creates one when a record is submitted, and its create() builds
+                the tier reviews from the definition. Typing model, res_id and a
+                definition into a blank form only produces a review pointing at
+                nothing, which is why QA asked for the New button to go
+                (OP#1167). On a plain list this attribute is what the New button
+                reads — activeActions.create comes straight from the arch
+                (web/views/utils.js), and an action context {'create': False}
+                merely rewrites this same attribute (web/views/view.js), so
+                setting it here covers every action that uses the view.
+            -->
             <list
+                create="0"
                 decoration-warning="status == 'pending'"
                 decoration-success="status == 'approved'"
                 decoration-danger="status == 'rejected'"
@@ -34,7 +47,10 @@
         <field name="name">spp.approval.review.form</field>
         <field name="model">spp.approval.review</field>
         <field name="arch" type="xml">
-            <form>
+            <!-- create="0" for the same reason as the list: this also removes
+                 the New in the form's breadcrumb, which is the other way into
+                 a blank review (OP#1167). -->
+            <form create="0">
                 <header>
                     <field
                         name="status"