From 899929f438e007a46faf1b59925091b0e138509d Mon Sep 17 00:00:00 2001 From: emjay0921 Date: Wed, 19 Aug 2026 10:34:55 +0800 Subject: [PATCH 1/4] fix(spp_approval): stop offering New on the approval review views MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit My Pending Approvals showed a New button, which cannot do anything useful: a review is created by the approval flow when a record is submitted, and its create() builds the tier reviews from the definition. Making one by hand would mean typing a model name, a record id and a definition into a blank form, and the result would point at nothing (OP#1167). Both views carry create="0" rather than one action carrying context={'create': False}. For a plain list or form, activeActions.create comes straight from the arch attribute, and a falsy create in an action's context is not a separate mechanism — the framework rewrites that same attribute. Setting it on the views covers My Pending Approvals, the sibling Approval Reviews list that had the same button, the form's own breadcrumb, and any action added later. Scope of the report: only approval managers ever saw the button. Approvers hold create=0 on spp.approval.review already, so nothing changes for them. The test reads the combined arch through get_view rather than the view record's own, because the multitier views inherit both of these and the merged result is what the client renders. A second test pins the assumption the fix rests on — that both actions still resolve to these views — and a third checks the views stayed usable for the reviews the flow does create. --- spp_approval/tests/__init__.py | 1 + .../tests/test_approval_review_no_create.py | 93 +++++++++++++++++++ spp_approval/views/approval_review_views.xml | 18 +++- 3 files changed, 111 insertions(+), 1 deletion(-) create mode 100644 spp_approval/tests/test_approval_review_no_create.py diff --git a/spp_approval/tests/__init__.py b/spp_approval/tests/__init__.py index b932f1122..08a66b35f 100644 --- a/spp_approval/tests/__init__.py +++ b/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 diff --git a/spp_approval/tests/test_approval_review_no_create.py b/spp_approval/tests/test_approval_review_no_create.py new file mode 100644 index 000000000..88c20daac --- /dev/null +++ b/spp_approval/tests/test_approval_review_no_create.py @@ -0,0 +1,93 @@ +# 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 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. + """ + definition = self.env["spp.approval.definition"].search([], limit=1) + if not definition: + self.skipTest("no approval definition available in this database") + + review = self.env["spp.approval.review"].create( + { + "model": "spp.approval.definition", + "res_id": definition.id, + "definition_id": definition.id, + } + ) + + self.assertIn(review, self.env["spp.approval.review"].search([])) diff --git a/spp_approval/views/approval_review_views.xml b/spp_approval/views/approval_review_views.xml index ea34c0490..d69a65cd6 100644 --- a/spp_approval/views/approval_review_views.xml +++ b/spp_approval/views/approval_review_views.xml @@ -5,7 +5,20 @@ spp.approval.review.tree spp.approval.review + spp.approval.review.form spp.approval.review -
+ +
Date: Mon, 24 Aug 2026 10:38:08 +0800 Subject: [PATCH 2/4] fix(spp_approval): bump version for the approval-review New button fix 19.0.2.0.2 with its changelog entry, per the in-PR convention. The change is view definitions only, so the bump is what makes an existing database pick it up: without an upgrade the New button stays exactly where QA found it. --- spp_approval/__manifest__.py | 2 +- spp_approval/readme/HISTORY.md | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/spp_approval/__manifest__.py b/spp_approval/__manifest__.py index 61fe3ffad..9fd559da9 100644 --- a/spp_approval/__manifest__.py +++ b/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", diff --git a/spp_approval/readme/HISTORY.md b/spp_approval/readme/HISTORY.md index 803caefff..fdec2a3c7 100644 --- a/spp_approval/readme/HISTORY.md +++ b/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 From 5b038c2e0b8706c23d11db080e37af394098def6 Mon Sep 17 00:00:00 2001 From: emjay0921 Date: Mon, 24 Aug 2026 10:38:53 +0800 Subject: [PATCH 3/4] docs(spp_approval): regenerate README for the 19.0.2.0.2 entry --- spp_approval/README.rst | 10 ++++++++++ spp_approval/static/description/index.html | 13 ++++++++++++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/spp_approval/README.rst b/spp_approval/README.rst index 2c0d29ab2..0bbe37d19 100644 --- a/spp_approval/README.rst +++ b/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 ~~~~~~~~~~ diff --git a/spp_approval/static/description/index.html b/spp_approval/static/description/index.html index 7ee87cb3a..1cb0ed684 100644 --- a/spp_approval/static/description/index.html +++ b/spp_approval/static/description/index.html @@ -536,6 +536,17 @@

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

  • Fix CEL Expressions tab crash: the ace editor fields used the invalid @@ -546,7 +557,7 @@

    19.0.2.0.1

    ternaries, which it highlights correctly.
-
+

19.0.2.0.0

  • Initial migration to OpenSPP2
    • From 5badd53b328992c7f8bfaca4b6aab04479c72762 Mon Sep 17 00:00:00 2001 From: emjay0921 Date: Tue, 25 Aug 2026 11:17:02 +0800 Subject: [PATCH 4/4] test(spp_approval): make the review-listing test run in CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit test_a_review_still_reaches_the_list searched for an approval definition and skipped when it found none. The CI database ships none, so the "views stayed usable" half of the safety net was green-by-skip in exactly the environment that gates merges — it only ever ran against a local database that happened to have one. The definition is created in the test instead. The assertion now earns its docstring too: it reads the review through the action's own domain and the list arch's columns, rather than asserting a bare search([]) contains it, which proved ORM basics rather than that the record lists and opens. --- .../tests/test_approval_review_no_create.py | 38 +++++++++++++++---- 1 file changed, 31 insertions(+), 7 deletions(-) diff --git a/spp_approval/tests/test_approval_review_no_create.py b/spp_approval/tests/test_approval_review_no_create.py index 88c20daac..70c3941e7 100644 --- a/spp_approval/tests/test_approval_review_no_create.py +++ b/spp_approval/tests/test_approval_review_no_create.py @@ -11,6 +11,8 @@ 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 @@ -77,17 +79,39 @@ def test_a_review_still_reaches_the_list(self): create="0" must not stop reviews created by the approval flow from being listed and opened — that is the whole purpose of the view. - """ - definition = self.env["spp.approval.definition"].search([], limit=1) - if not definition: - self.skipTest("no approval definition available in this database") + 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": "spp.approval.definition", - "res_id": definition.id, + "model": "res.partner", + "res_id": self.env.user.partner_id.id, "definition_id": definition.id, } ) - self.assertIn(review, self.env["spp.approval.review"].search([])) + # 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")