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

[emjay0921]

Why is this change needed?

My Pending Approvals showed a New button, which cannot do anything useful (OP#1167). A review is created by the approval flow when a record is submitted — create() builds the tier reviews from the definition — so making one by hand means typing a model name, a record id and a definition into a blank form, and the result points at nothing. Mark's note on the ticket puts it plainly: the view exists to show the status of change requests for the logged-in approver.

One correction to the ticket's framing, since the title says "across any user": only approval managers ever saw the button. Approvers hold create=0 on spp.approval.review already, so nothing changes for them. The fix is the same either way, but do not expect a visible difference when testing as an approver.

How was the change implemented?

create="0" on both root nodes — the list and the form — in spp_approval/views/approval_review_views.xml.

That covers more than the reported view: Approval Reviews lists the same records through the same views and had the same button, and the form's own breadcrumb New goes too. It also cannot be bypassed by a future action that forgets a context key.

The attribute is the authoritative gate here, unlike the x2many case in OP#1171: for a plain list or form activeActions.create comes straight from the arch (web/views/utils.js), and an action context of {'create': False} is not a separate mechanism — the framework rewrites that same attribute (web/views/view.js). So there is no link-vs-create trap in this one, and an arch-level test genuinely proves the behaviour.

New unit tests

spp_approval/tests/test_approval_review_no_create.py:

• no view offers New — asserted against the combined arch via get_view, not the view record's own, because approval_review_views_multitier.xml inherits both views and the merged result is what the client renders;
• both actions still resolve to those views — the attribute only protects them while they do, so if an action starts pinning its own view_id this fails rather than the suppression silently lapsing;
• a review created by the flow still lists and opens — the views must be read-only entry points, not dead ones.

Unit tests executed by the author

Full spp_approval suite after merging 19.0 in: 132 tests, 0 failed, 0 errors.

How to test manually

1. As an Approval Manager or Administrator — the accounts that saw the button — open Approvals → My Pending Approvals: no New, everything else unchanged.
2. Approvals → Approval Reviews: no New there either; open a review and check the breadcrumb offers none.
3. Submit a record for approval and confirm its review still appears in both lists and can still be approved or rejected. This is the check that matters — the fix must remove the button without touching how the flow creates reviews.

Related links

• OP#1167 — https://projects.acn.fr/work_packages/1167

Reviewer notes

• spp_approval bumped to 19.0.2.0.2 with its changelog entry, per the in-PR convention. Load-bearing here: the change is view definitions only, so without the upgrade the button stays exactly where QA found it.
• Not addressed, and out of scope: approval managers retain unlink on reviews, so an approval trail can still be deleted. Flagging it so it is not mistaken for part of this fix.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.80%. Comparing base (35021ce) to head (5badd53).

Additional details and impacted files

@@            Coverage Diff             @@
##             19.0     #447      +/-   ##
==========================================
- Coverage   76.59%   74.80%   -1.80%     
==========================================
  Files         629      558      -71     
  Lines       42351    37725    -4626     
==========================================
- Hits        32439    28219    -4220     
+ Misses       9912     9506     -406     

Flag	Coverage Δ
spp_api_v2	?
spp_api_v2_change_request	66.53% <ø> (ø)
spp_api_v2_cycles	71.03% <ø> (ø)
spp_api_v2_entitlements	70.23% <ø> (ø)
spp_api_v2_programs	92.22% <ø> (ø)
spp_api_v2_simulation	71.19% <ø> (ø)
spp_approval	50.85% <ø> (?)
spp_attendance	?
spp_base_common	91.07% <ø> (ø)
spp_case_entitlements	100.00% <ø> (ø)
spp_case_programs	100.00% <ø> (ø)
spp_cel_event	85.34% <ø> (?)
spp_cel_load_testing	98.11% <ø> (ø)
spp_change_request_v2	77.83% <ø> (ø)
spp_cr_type_assign_program	92.07% <ø> (ø)
spp_dci_compliance	93.01% <ø> (ø)
spp_farmer_registry	?
spp_import_match	?
spp_pii_encryption	?
spp_programs	65.53% <ø> (ø)
spp_registry	87.79% <ø> (ø)
spp_security	69.56% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.
see 124 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[gonzalesedwin1123]

Thorough review against OP#1167. The fix itself is correct, well-placed, and I verified every claim in the PR description against the code. One must-fix on the tests, then this is good to go.

Verified — the fix

• create="0" on the root <list> and <form> archs is the right mechanism on Odoo 19: activeActions.create is read straight from the arch for plain list/form views, and an action context {'create': False} rewrites the same attribute rather than being a separate gate. The arch-level attribute therefore covers My Pending Approvals, Approval Reviews, the form breadcrumb, and any future action that reuses these views.
• The multitier inherits (approval_review_views_multitier.xml) xpath into //sheet and //field[@name='status'] only — they never touch the root attributes, so the merged arch keeps create="0". Test 1 asserting on the combined get_view arch (not the view record's own) is exactly the right pin for this.
• ACL claim confirmed: ir.model.access.csv gives group_approval_approver perm_create=0 on spp.approval.review and only group_approval_manager perm_create=1 — so indeed only managers ever saw the button, and the ticket's "across any user" framing correction is accurate.
• Both actions (approval_review_action, approval_review_my_pending_action) are the only entry points: no other menus, no dynamically-built act_window in Python references this model.
• I also checked the OP#1171-style x2many trap the description mentions: approval_review_ids is embedded in 6 other modules' forms (spp_change_request_v2, spp_drims, spp_event_data, spp_programs ×3) — every one is readonly="1", so there is no "Add a line" exposure left anywhere.
• Version chain: 19.0 is at 19.0.2.0.1 and none of the 3 commits the branch is behind (#435, #432, #444) touch spp_approval, so 19.0.2.0.2 is the correct next number and the branch merges cleanly. HISTORY.md / README.rst / index.html are consistent.

Must fix — the third test never runs in CI

test_a_review_still_reaches_the_list starts with search([], limit=1) on spp.approval.definition and skips when nothing is found — and in the CI database nothing is found. From the test (spp_approval) job log on this PR:

02:44:19 ... skipped TestApprovalReviewNoCreate.test_a_review_still_reaches_the_list : no approval definition available in this database

So the "views stayed usable" leg of the safety net is green-by-skip in exactly the environment that gates merges; it only ever ran on your local DB. Please create the definition inside the test instead of searching for one — then the test runs everywhere:

definition = self.env["spp.approval.definition"].create(
    {"name": "Test Definition", "model": "spp.approval.definition"}
)

(adjust required fields to the model). While you're in there, consider making the assertion earn its docstring: self.assertIn(review, search([])) proves ORM basics, not that the record "lists and opens". Reading the list through the action would guard what the test claims to guard, e.g. resolve approval_review_action, run search(action_domain) and assert the review is in it, and/or get_view + a web_search_read-shaped read of the list fields. Not a blocker if you keep it simple, but the CI-skip itself is.

Non-blocking observations (out of scope, noting so they aren't lost)

• Your own note stands: managers retain unlink on reviews, so an approval trail is still deletable.
• While verifying the ACLs I noticed rules.xml attaches the reviewer-scoping rule (domain [('reviewer_id','=',user.id)], named "Approver Access") to group_approval_officer, while group_approval_approver only implies group_approval_viewer. Two smells: (a) a plain approver is in no ruled group, so no record rule scopes their read=1/write=1 ACL at all; (b) reviewer_id is only set at approve/reject time, so pending reviews would be invisible to anyone the rule does scope. Pre-existing, nothing to do with this PR — flagging for a follow-up ticket.
• Related: approval_review_my_pending_action filters on status = 'pending' only; the "my" in the name relies entirely on those record rules, so managers see everyone's pending reviews there. Also pre-existing.

[emjay0921]

Fixed in 5badd53b.

Must fix — the test now runs in CI

You were right about the mechanism and about why it mattered: the search-and-skip meant the only leg of the safety net that proves the views still work never executed in the environment that gates merges. The definition is created in the test instead:

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,
    }
)

approval_group_id is needed on top of your sketch — approval_type defaults to group, and _check_approval_config refuses that combination without a group.

Verified on a fresh database (same shape as CI, not my local one): all three tests start, none skip.

0 failed, 0 error(s) of 132 tests
Starting TestApprovalReviewNoCreate.test_a_review_still_reaches_the_list ...
Starting TestApprovalReviewNoCreate.test_both_actions_use_those_views ...
Starting TestApprovalReviewNoCreate.test_no_view_offers_a_new_button ...

Took the optional half too

The assertion no longer proves ORM basics. It reads the review the way the action does — resolving approval_review_my_pending_action and searching its own domain — then reads every column the list arch renders, so "lists and opens" is what is actually guarded:

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, ...)
...
columns = [node.get("name") for node in arch.xpath("//field[@name]")]
self.assertTrue(review.read(columns), ...)

One thing your review turned up indirectly

Checking for remaining skips, the same pathology exists elsewhere in this module — pre-existing and unrelated to this PR:

skipped setUpClass (spp_approval.tests.test_approval_mixin.TestApprovalMixin)
  : Test model x_test.approval.model not registered - skipping mixin tests

That is the entire mixin suite silently not running in CI. Out of scope here; flagging it since it is the same class of problem and worth more than a footnote.

Your rules.xml / my_pending_action observations are recorded alongside the other follow-ups.

Ready for re-review.

[gonzalesedwin1123]

Approved — the single must-fix from the previous review is resolved, and verified end-to-end:

• 5badd53b replaces the search-and-skip with an in-test spp.approval.definition create (name + model_id are the only required fields; res.partner satisfies the mail-thread domain), so the test no longer depends on the database shipping a definition.
• Confirmed in today's CI run (job 97672791306): TestApprovalReviewNoCreate.test_a_review_still_reaches_the_list starts and runs — no skip line — and the suite finishes 0 failed / 0 errors of 132 tests. The only skip in the module is the pre-existing, unrelated TestApprovalMixin model-registration skip.
• The strengthened assertion is a genuine improvement over what I asked for: reading through approval_review_my_pending_action's own domain ([('status', '=', 'pending')], safe under literal_eval, and the created review defaults to pending) plus every column of the combined list arch proves the record reaches the list a user actually opens, not just that search([]) works.
• Version chain re-verified after the 19.0 merge-in: base is at 19.0.2.0.1, this PR takes 19.0.2.0.2 with its HISTORY entry.

The three out-of-scope observations from the first review (manager unlink on reviews; the reviewer_id-scoping rule attached to officer-only while approvers go unscoped and pending reviews have no reviewer_id; the action domain relying entirely on those rules for "my") stand as follow-up candidates and do not block this fix.