feat(spp_programs): program config UX polish, payment zero-state, cancelable manager wizard (#941 #952 #953)#198
Draft
emjay0921 wants to merge 10 commits into
Draft
feat(spp_programs): program config UX polish, payment zero-state, cancelable manager wizard (#941 #952 #953)#198emjay0921 wants to merge 10 commits into
emjay0921 wants to merge 10 commits into
Conversation
…tem 2)
Add a Send email notifications toggle on spp.program (default OFF),
exposed under the Notifications section of the Configuration tab.
When the toggle is off, the approval workflow does not emit outgoing
email for that program's records:
- _notify_thread_by_email on spp.entitlement / spp.entitlement.inkind /
spp.cycle short-circuits the mail-framework email dispatch, which
catches tracking emails on approval_state transitions and the
approval-result message_notify call.
- _create_approval_activity on the same three models skips scheduling
the approver mail.activity at submit time (the main email source —
a large cycle would flood every approver's inbox).
Chatter entries and in-app notifications still fire, preserving the
audit trail.
The toggle is disabled (readonly) whenever no ir.mail_server record is
configured; an info line underneath points admins to the outgoing-mail
server settings. Gating resolves through
_should_send_email_notifications() which checks both the flag and
mail-server presence.
Rework the 5 manager banners on the Program Configuration tab so the
layout is clearer whether one or many managers are configured, and
the details actually describe the configuration.
Smart display names on concrete managers
Each concrete manager falls back to a method-specific label when its
name is still the "Default" auto-name: "CEL Eligibility Criteria",
"Cash Entitlement", "In-kind Entitlement", "Basic Cash", "Default
Cycle Schedule", "CEL Compliance Criteria", "Default Payment",
"Default Program Manager". User renames are preserved.
Dialog title
manager_mixin.open_manager_form() accepts a title override and the
per-banner action_configure_* methods pass "Who Qualifies?",
"What Do They Receive?", "Program Schedule", "Compliance Criteria",
"Payment Processing", etc. so the Edit modal shows the banner label
instead of the default "Odoo" breadcrumb.
Layout helpers on spp.program
Per banner, three computed fields (<banner>_manager_count,
_display, _detail). When a banner has exactly one manager, the
header row shows Method + the manager's display name and a full-
width detail block below renders method-specific content instead of
the truncated summary:
- CEL managers → the full CEL expression, or an explicit "no CEL
defined, every <target> will match" warning when empty.
- Entitlement with items → one line per item with the CEL
expression, multiplier, or flat amount (plus condition).
- Basic Cash entitlement → amount per cycle, amount per person
(with cap), transfer-fee breakdown.
When count > 1 the list widget is shown as before. When count == 1
the list is hidden.
Compliance zero-state
Compliance is optional, so count==0 now shows an informational block
body + an "Add" button in the banner header that creates the default
compliance manager and opens its form in a popup. The generic list
widget only shows when count >= 2.
Recurrence label
Cycle summary previously produced "Every 1 monthly" which confused
QA. A new _format_recurrence helper turns it into "Monthly", "Every
2 months", "Weekly", etc.
View labels
"Details" renamed to banner-specific strings: "Eligibility Method"
(eligibility) and "Entitlement Type" (entitlement). Schedule,
Compliance, and Payment already had per-banner labels.
Add an exit_reason Char field on spp.program.membership and a small
wizard that captures the reason at the moment the Exit button is
clicked. action_exit now opens the wizard instead of writing the
exited state immediately; the wizard owns the atomic write of state,
exit_date, and exit_reason so every exit event persists a reason for
audit.
Surface the new field on:
- Program Membership list (Programs > Program Memberships)
- Program Membership form (main group next to exit_date)
- Other Programs nested list on the membership form
- Participation > Program Enrollments list on the registrant form
(both individual and group variants) — exit_date also added here
as an optional column
ACL granted to officer (create/read/write), manager (full), and admin.
Existing three lifecycle tests updated to drive the wizard end-to-end.
Cycle-level exit fields don't exist on spp.cycle.membership so no
cycle view changes; flagging for a follow-up if that capability is
needed.
… (#941 r2) Round-1 fixed display_name via a compute, but the stored `name` was still literally "Default" — leaking through to the Edit dialog's Name field, the program config card's Eligibility/Entitlement cell, and the manager picker dropdown. Round 2 sets the actual `name` field at creation so the value is consistent everywhere. Concrete managers (eligibility, entitlement default/cash/inkind, cycle, compliance, payment, program, deduplication) now override default_get to seed `name` with their method-specific label. The earlier _compute_display_name overrides are removed. Every call site that explicitly passed name="Default" was updated to drop it so the model's default_get takes over: programs.py create_default_managers, create_program_wizard (3 places), mis_demo_generator (the demo's manager-bootstrap path), and action_add_compliance_manager. The MANAGER_TYPE_INFO picker labels for cycle / program / payment / deduplication are renamed from "Default" to their method-specific counterparts so the Add-a-line dropdown is also meaningful. A post-migration script (spp_programs/migrations/19.0.2.0.11/post-migration.py) backfills existing rows where name = 'Default' to the method-specific label, across all 9 concrete-manager tables. Module version bumped 19.0.2.0.10 -> 19.0.2.0.11 so the migration runs on upgrade.
Mirror the Compliance pattern on the Payment Processing banner: show an informational block + `+ Add` button in the header when no payment manager is configured, instead of an empty list with a generic "Add a line" entry. Clicking Add pre-creates a default payment manager (wrapper + concrete + default batch tag) and opens it for editing.
The compliance and payment banners' `+ Add` zero-state buttons used to pre-create both the wrapper and the concrete manager records before the dialog opened, then open that pre-created record in edit mode. If the user dismissed the dialog with `X`, the new manager stayed attached to the program even though the user never confirmed they wanted it. The trade-off was deliberate when the feature shipped (#941 / #952) — we needed the parent form to refresh after Save — but it violated the standard "click X = nothing changed" expectation. This round switches both Add actions to open the concrete model in *create mode* (no `res_id`) with the necessary defaults seeded via `default_*` context. Persistence now runs only when Save fires: - `spp.manager.source.mixin.create` (already shared by every concrete manager) was extended with a tiny auto-wrapper hook gated by two context keys passed by the action — `_spp_wrapper_model` and optional `_spp_program_m2m_field`. After the concrete record is created, the mixin creates the wrapper and (for Payment, whose program-side field is a Many2many) writes it onto the program. - `DefaultFilePaymentManager.create` auto-creates the default batch tag when `create_batch=True` and `batch_tag_ids` is empty, so the dialog Save doesn't fail the constraint and we don't have to pre-create the tag in the action method (which would orphan it on cancel). - Both `create` overrides switch to `@api.model_create_multi` to match Odoo 19's preferred batch-create semantics. Cancel path: dialog `X` → no `create()` ever runs → no records persist → banner stays in zero-state. Save path: concrete created → wrapper auto-created and linked → dialog closes → parent reloads → banner shows configured. spp_programs suite remains green: 0 failed, 0 errors of 636.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new wizard for capturing beneficiary exit reasons, implements a toggle for program-level email notifications, and refactors manager naming to use method-specific labels instead of a generic "Default" string. UI enhancements were also added to improve the display of manager configurations. Feedback identifies critical issues where suppressing approval activities entirely could break in-app workflows and lead to tracebacks. Additionally, there are recommendations to optimize database writes in loops, ensure all strings are translatability-ready, and handle potential duplicate record creation in multi-create methods.
Comment on lines
+621
to
+631
| def _create_approval_activity(self, definition, review): | ||
| """Gate the approver-email path on the parent program's toggle. | ||
|
|
||
| spp.approval.mixin schedules a mail.activity for each approver on | ||
| submit; the activity dispatch sends email through the assignee's | ||
| notification preferences. Skip the scheduling entirely when the | ||
| program has email notifications turned off.""" | ||
| self.ensure_one() | ||
| if self.program_id and not self.program_id._should_send_email_notifications(): | ||
| return | ||
| return super()._create_approval_activity(definition, review) |
There was a problem hiding this comment.
Overriding _create_approval_activity to return None when email notifications are disabled suppresses the creation of the Odoo Activity entirely. This prevents approvers from seeing the task in their Odoo systray or on the record's chatter, which might break the in-app workflow. If the intent is only to suppress the email notification, overriding _notify_thread_by_email (as done above) is sufficient, as it is called during activity creation to send the mail. Furthermore, returning None instead of a recordset might cause tracebacks in callers expecting an activity object.
Comment on lines
+403
to
+414
| def _create_approval_activity(self, definition, review): | ||
| """Gate the approver-email path on the parent program's toggle. | ||
|
|
||
| spp.approval.mixin schedules a mail.activity for each approver on | ||
| submit; the activity dispatch sends email through the assignee's | ||
| notification preferences. Skip the scheduling entirely when the | ||
| program has email notifications turned off.""" | ||
| self.ensure_one() | ||
| program = self.cycle_id.program_id | ||
| if program and not program._should_send_email_notifications(): | ||
| return | ||
| return super()._create_approval_activity(definition, review) |
There was a problem hiding this comment.
Overriding _create_approval_activity to return None when email notifications are disabled suppresses the creation of the Odoo Activity entirely. This prevents approvers from seeing the task in their Odoo systray or on the record's chatter, which might break the in-app workflow. If the intent is only to suppress the email notification, overriding _notify_thread_by_email (as done above) is sufficient, as it is called during activity creation to send the mail. Furthermore, returning None instead of a recordset might cause tracebacks in callers expecting an activity object.
Comment on lines
+921
to
+927
| def _create_approval_activity(self, definition, review): | ||
| """Gate the approver-email path on the parent program's toggle.""" | ||
| self.ensure_one() | ||
| program = self.cycle_id.program_id | ||
| if program and not program._should_send_email_notifications(): | ||
| return | ||
| return super()._create_approval_activity(definition, review) |
There was a problem hiding this comment.
Overriding _create_approval_activity to return None when email notifications are disabled suppresses the creation of the Odoo Activity entirely. This prevents approvers from seeing the task in their Odoo systray or on the record's chatter, which might break the in-app workflow. If the intent is only to suppress the email notification, overriding _notify_thread_by_email (as done above) is sufficient, as it is called during activity creation to send the mail. Furthermore, returning None instead of a recordset might cause tracebacks in callers expecting an activity object.
| if not program_id: | ||
| continue | ||
| program = self.env["spp.program"].browse(program_id) | ||
| tag_name = f"Default {program.name}" |
There was a problem hiding this comment.
The tag name string is partially hardcoded in English. It should be fully translatable to support multi-language environments.
Suggested change
| tag_name = f"Default {program.name}" | |
| tag_name = _("Default %s") % program.name |
Comment on lines
+109
to
+136
| for vals in vals_list: | ||
| if not vals.get("create_batch", True) or vals.get("batch_tag_ids"): | ||
| continue | ||
| program_id = vals.get("program_id") or self.env.context.get("default_program_id") | ||
| if not program_id: | ||
| continue | ||
| program = self.env["spp.program"].browse(program_id) | ||
| tag_name = f"Default {program.name}" | ||
| BatchTag = self.env["spp.payment.batch.tag"].sudo() # nosemgrep: odoo-sudo-without-context | ||
| tag = BatchTag.search( | ||
| [ | ||
| ("name", "=", tag_name), | ||
| ("order", "=", 1), | ||
| ("max_batch_size", "=", 500), | ||
| ], | ||
| limit=1, | ||
| ) | ||
| if not tag: | ||
| tag = BatchTag.create( | ||
| { | ||
| "name": tag_name, | ||
| "order": 1, | ||
| "domain": [], | ||
| "max_batch_size": 500, | ||
| } | ||
| ) | ||
| vals["batch_tag_ids"] = [(4, tag.id)] | ||
| return super().create(vals_list) |
There was a problem hiding this comment.
In api.model_create_multi, if vals_list contains multiple records for the same program, the search for the default batch tag might not find a tag created in a previous iteration of the same loop because the database hasn't been flushed yet. This could lead to duplicate tag creation or unique constraint violations. Consider using a local cache (dictionary) to track tags created during the execution of this method.
Comment on lines
+27
to
+41
| for record in records: | ||
| wrapper = self.env[wrapper_model].create( | ||
| { | ||
| "program_id": program_id, | ||
| "manager_ref_id": f"{record._name},{record.id}", | ||
| } | ||
| ) | ||
| # Compliance is One2many on `spp.program.compliance_manager_ids` | ||
| # and auto-resolves via the wrapper's `program_id` inverse. | ||
| # Payment is Many2many — its program-side field needs an | ||
| # explicit write, otherwise the program never picks up the | ||
| # new wrapper. | ||
| m2m_field = self.env.context.get("_spp_program_m2m_field") | ||
| if m2m_field and "spp.program" in self.env: | ||
| self.env["spp.program"].browse(program_id).write({m2m_field: [(4, wrapper.id)]}) |
There was a problem hiding this comment.
Performing a write operation on the program record inside the loop is inefficient as it triggers multiple database updates and recomputations. It is better to collect all wrapper records and perform a single write after the loop. Additionally, Odoo's Reference fields can accept a recordset directly instead of a manually constructed string.
Suggested change
| for record in records: | |
| wrapper = self.env[wrapper_model].create( | |
| { | |
| "program_id": program_id, | |
| "manager_ref_id": f"{record._name},{record.id}", | |
| } | |
| ) | |
| # Compliance is One2many on `spp.program.compliance_manager_ids` | |
| # and auto-resolves via the wrapper's `program_id` inverse. | |
| # Payment is Many2many — its program-side field needs an | |
| # explicit write, otherwise the program never picks up the | |
| # new wrapper. | |
| m2m_field = self.env.context.get("_spp_program_m2m_field") | |
| if m2m_field and "spp.program" in self.env: | |
| self.env["spp.program"].browse(program_id).write({m2m_field: [(4, wrapper.id)]}) | |
| if wrapper_model and program_id and wrapper_model in self.env: | |
| wrappers = self.env[wrapper_model] | |
| for record in records: | |
| wrappers |= self.env[wrapper_model].create( | |
| { | |
| "program_id": program_id, | |
| "manager_ref_id": record, | |
| } | |
| ) | |
| m2m_field = self.env.context.get("_spp_program_m2m_field") | |
| if m2m_field and "spp.program" in self.env: | |
| self.env["spp.program"].browse(program_id).write({m2m_field: [(4, w.id) for w in wrappers]}) |
…ey Receive?" overview (#941 r2)
The entitlement summary on the program overview rendered raw amounts like
"Amount per beneficiary: 1.0 per cycle" — no currency symbol, no decimal
precision. Two code paths fixed:
- _manager_detail_entitlement_items: read item.currency_id, prefix the
symbol, and format the amount with currency.decimal_places. This is the
path hit by default cash-transfer programs with entitlement items.
- _manager_detail_basic_cash: amount fields were already prefixed with the
currency symbol but rendered the raw Float ("1.0"); now formatted with
currency.decimal_places ("1.00").
Adds TestManagerSummaryFormatting with focused assertions covering both
paths: currency symbol present, two-decimal precision, no stray "1.0"
rendering.
…lement summary (#941 r3) QA round-3 follow-up: amounts on the 'What Do They Receive?' overview were rendered with currency symbol + 2-decimal precision but without thousands grouping (e.g. "$ 1000000.00" instead of "$ 1,000,000.00"). Switch both render paths in program_manager_ui.py to odoo.tools.misc. format_amount, which gives locale-aware grouping plus correct currency symbol position out of the box: - _manager_detail_entitlement_items - _manager_detail_basic_cash Adds test_items_summary_includes_thousands_separator covering the 1,000,000 case. Local run: 0 failed of 3 tests.
…itlement summary (#941 r3 followup) QA flagged that the previous round-3 fix (which used odoo.tools.misc. format_amount) rendered the currency symbol on the RIGHT for currency records configured with position='after'. The program overview should always show the symbol on the LEFT for consistency. Replace format_amount with a small static helper _format_money that: - groups thousands with Python's :, formatter, - pads to currency.decimal_places (default 2), - always prefixes the symbol on the left, regardless of currency.position. Adds test_items_summary_symbol_appears_left_of_amount which explicitly sets currency.position='after' before asserting the rendered summary still puts the symbol left of the amount.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why is this change needed?
Three related polish/UX OPs on the program-configuration screen, kept on a single branch because the commits touch overlapping files (banners, manager wizards, source mixin):
+ Add/ cancel UX on Compliance + Payment banners persists the manager record even when the user cancels mid-config; now correctly cancelable.How was the change implemented?
All work is in
spp_programs, primarily on the program config banners and the_create_*_manager_uiwizard flow. Commit-by-commit:7cd387206496692cdfa7b2b34fe075bb731052244a846f12f8c0a2e8#953 was originally on a separate
fix/953-cancelable-manager-addbranch; fast-forwarded into this branch since it was strictly downstream of #941 — see OP#953 activity for the consolidation note.New unit tests
None added on this branch; manual UI verification covered by QA on the individual OPs.
Unit tests executed by the author
pre-commit runon changed files passed locally in the CI-matched container.How to test manually
QA round-by-round on the individual OP tickets:
Related links