feat(change_request): redesign Create Group & Add Member CRs + review-page fixes (#876 #871)#242
Open
emjay0921 wants to merge 27 commits into
Open
feat(change_request): redesign Create Group & Add Member CRs + review-page fixes (#876 #871)#242emjay0921 wants to merge 27 commits into
emjay0921 wants to merge 27 commits into
Conversation
Replace the flat head-of-household fields on the Create Group detail with member-list sub-tables and per-type configuration: - Detail: drop head_individual_id / create_new_head / head_* fields; add phone_line_ids, bank_line_ids, id_doc_line_ids, member_existing_ids, member_new_ids sub-tables, plus area_id and latitude/longitude. - CR type: add allow_empty_members and requires_head booleans so each group-creating type ships its own rules; apply strategy validates both. - Wizard: new spp.cr.detail.create_group.member.wizard handles add/edit for existing + new members in one transient model. - Strategy: rewrite apply() into _validate / _create_group / _attach_* helpers; preview() reports per-sub-table counts and head label. - View: notebook 'Members' page with per-mode alert blocks and primary buttons; member lists are wizard-only (create=0 edit=0). - Type registration: create_group sets is_requires_registrant=False — the CR creates the registrant, so the picker is hidden in the create wizard. create_wizard.py renders a tailored "creates a new ..." hint. - MIS demo generator: split head_name into given/family and emit a member_new_ids row with the 'head' role. - Security: ACLs for the 5 new sub-models + wizard. - Tests: rewrite test_create_group_strategy.py around the new schema (empty / existing-head / new-head / mixed / name-required); update two E2E call sites in test_e2e_workflows.py.
Replace the flat 7-field Add Member detail with the full spec layout from
OP#871, reusing the per-line sub-models introduced by OP#876.
- Detail: add middle_name; demographics block (is_approximate_birthdate,
age compute, birth_place, occupation_id, civil_status_id, income);
area_id + address + email; latitude/longitude; one-to-many phone /
bank / id_doc lines; membership_type_id (role); related mirror
type_requires_head; computed roles_available and existing_membership_ids
(read-only context table). member_name is now a stored compute of
"FAMILY, GIVEN MIDDLE".
- Shared sub-models: make detail_id nullable on
spp.cr.detail.create_group.{phone,bank,id_doc}, add a parallel
add_member_detail_id FK, and an exactly-one-parent constraint so the
same row shape can attach to either detail.
- Strategy: rewrite into _validate / _create_individual /
_attach_phones|banks|id_docs / _demote_existing_head_if_needed /
_create_membership. When the new member is added with the 'head' role,
any existing head on the group is demoted automatically. Names are
required; if the CR type sets requires_head, role assignment is too.
- View: full rewrite matching spec sections (Name, Demographics, Contact,
Phone Numbers, Location, Financial Information, Identity Document,
Members) with the review-stage banner "The following individual is to
be added:" and a warning banner when requires_head is set.
- MIS demo generator: write membership_type_id instead of the removed
relationship_id; drop the now-computed member_name from the payload.
- Tests: rewrite test_add_member_strategy.py against the new schema
(basic create, name compute including middle name, age compute,
validation paths, head demotion, sub-record attach); fix the four
add_member call sites in test_e2e_workflows.py.
Stacks on top of feat/876-create-group-cr-redesign.
- Expose Allow Empty Groups / Requires Head of Household on the Change Request Type configuration form - Collect the group address as a single field matching the registry (was split into Address Line 1/2, City, State, Postal Code, Country) - Enforce one Head of Household at the member-row level so the rule also holds when rows are added through the member wizard - Capture the full registry profile (names, demographics, contact) for a new individual in the Add New Individual modal - Show "The following group is to be added" above the review table
- Collect the member address as a single field matching the registry (was split into Address Line 1/2, City, State, Postal Code, Country) - Make the existing-members table truly read-only — rows can no longer be opened and edited from the change request form - Show "The following individual is to be added" above the review table - Map approximate-birthdate to the registry individual on apply Requires Head of Household is now configurable on the Change Request Type form (merged from #876).
The Add New Individual modal now collects multiple phone numbers in an editable list (matching the group's phone list) instead of a single field. On apply, the numbers are folded (primary first) into the new individual's single header phone field. The create-group phone row model gained a member_new parent so the rows attach to the new member.
…mber-cr-redesign # Conflicts: # spp_change_request_v2/details/create_group.py
…r (#876) Editing a new in-group individual that already had phone numbers raised "Exactly one parent must be set on a phone-number row". The wizard rebuilt the list with a clear (5) command, which only nulls the phone row's inverse FK instead of deleting it, momentarily orphaning the rows. Use delete (2) commands so the old rows are removed cleanly.
…i-parenting (#876) The exactly-one-parent constraint also rejected zero-parent rows, which Odoo can produce transiently while rewriting a one2many — surfacing a confusing "Exactly one parent must be set" error to users editing a new member's phone list. Only reject a row genuinely linked to more than one record.
…mber-cr-redesign # Conflicts: # spp_change_request_v2/details/create_group.py
…#876) A new in-group individual's phone numbers are all folded into the partner's single phone field, so there is no header-vs-other distinction — the Primary toggle was meaningless. Remove it from the modal and join the numbers in entry order.
…hone rows (#876) Root cause of the phone-row 'parent' errors: the Add New Individual wizard is opened with a default_detail_id context (for the member row), and the phone sub-model also has a detail_id field, so that default was applied to the new phone rows — giving them two parents (detail_id + member_new_id). Force detail_id=False when building the phone rows. Adds a regression test that opens the wizard with the leaking context.
…hone rows (#876) Root cause of the phone-row 'parent' errors: the Add New Individual wizard is opened with a default_detail_id context (for the member row), and the phone sub-model also has a detail_id field, so that default was applied to the new phone rows — giving them two parents (detail_id + member_new_id). Force detail_id=False when building the phone rows. Adds a regression test that opens the wizard with the leaking context.
…ply (#876) A new individual's captured phone numbers were only folded into the partner's single header phone field, not created as spp.phone.number records — so they didn't appear in the registry's Phone Numbers list. Attach the phone records on apply, the same way the group's phones are attached.
…ember (#871) When the target group already has a head, the Add Member form now shows an info banner naming the current head and a 'Replace current Head of Household' toggle. Replacement is no longer silent/automatic: giving the new member the Head role while a head exists is blocked unless the toggle is enabled; when enabled, applying demotes the current head and makes the new member the head.
… is Head (#871) The banner and Replace toggle now appear only when the picked Role is Head AND the group already has a head. The toggle is purely a confirmation gate: the picked Role determines the member's role, so a stale toggle with a non-head role no longer forces head or demotes the current head.
…ice (#871) Drop the 'Replace current Head of Household' boolean. When the new member is given the Head role and the group already has a head, the form now shows a warning that applying will make this member the head and remove the role from the current head; the replacement then happens automatically on apply (the CR still passes through approval first).
…ew (#876) The Create Group CR review page (review_comparison_html) showed only counts for the one2many fields and omitted several group fields. Surface the actual data via a generic render contract on preview(): - _tables: one2many fields render as their own tables - Phone Numbers, Bank Accounts, ID Documents, and Existing Members (Name / Role). - _sections: new members render as a labelled detail block each, showing the full captured individual (role, DOB, gender, civil status, occupation, birth place, income, area, address, email) plus that member's own phone numbers. - Add the previously-missing scalar fields: email, area, location. - Drop the phone/bank/id_doc/member count keys. _generate_review_comparison_html() renders _tables and _sections after the action summary; _render_data_tables() / _render_data_sections() are generic so other CR strategies can surface the same way.
… (#871) The Add Member CR review page showed only counts for phones/bank/ID docs and omitted the added individual's details. Surface the full data via the generic _tables contract (shared with Create Group): - All individual fields are shown even when empty (name, role, date of birth, gender, civil status, occupation, birth place, income, area, address, email, location). - Phone Numbers / Bank Accounts / ID Documents render as their own tables. - Drop the phone/bank/id_doc count keys. The head-of-household replacement feature was reverted separately and deferred to #1006.
![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 redesigns the 'Add Member' and 'Create New Group' Change Request workflows to capture a comprehensive set of profile fields and sub-records (phones, banks, ID documents) using a new wizard. Feedback focuses on resolving a bug where zero values are incorrectly treated as empty in previews, using timezone-aware Odoo methods for age calculations, and batching database inserts for better performance when attaching sub-records.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+1214
to
+1215
| for label, value in field_rows: | ||
| display = html_escape(value) if value else '<span class="text-muted">—</span>' |
There was a problem hiding this comment.
The current logic treats 0 or 0.0 as falsy, which will display a — placeholder instead of the actual value (e.g., for zero income or age). Additionally, passing non-string values directly to html_escape can raise a TypeError. Converting the value to a string and explicitly checking against None, False, and empty strings is safer and more robust.
Suggested change
| for label, value in field_rows: | |
| display = html_escape(value) if value else '<span class="text-muted">—</span>' | |
| for label, value in field_rows: | |
| display = html_escape(str(value)) if value not in (None, False, '') else '<span class="text-muted">—</span>' |
|
|
||
| @api.depends("birthdate") | ||
| def _compute_age(self): | ||
| today = date.today() |
There was a problem hiding this comment.
Using date.today() uses the server's local timezone, which can lead to incorrect age calculations for users in different timezones. In Odoo, it is best practice to use fields.Date.context_today(self) to get the current date in the user's timezone.
Suggested change
| today = date.today() | |
| today = fields.Date.context_today(self) |
|
|
||
| @api.depends("birthdate") | ||
| def _compute_age(self): | ||
| today = date.today() |
There was a problem hiding this comment.
Using date.today() uses the server's local timezone, which can lead to incorrect age calculations for users in different timezones. In Odoo, it is best practice to use fields.Date.context_today(self) to get the current date in the user's timezone.
Suggested change
| today = date.today() | |
| today = fields.Date.context_today(self) |
Comment on lines
+189
to
+224
| def _attach_phones(self, detail, individual): | ||
| SppPhone = self.env["spp.phone.number"] | ||
| for line in detail.phone_line_ids: | ||
| SppPhone.create( | ||
| { | ||
| "partner_id": individual.id, | ||
| "phone_no": line.phone_no, | ||
| "country_id": line.country_id.id if line.country_id else False, | ||
| "date_collected": fields.Date.today(), | ||
| } | ||
| ) | ||
|
|
||
| def _attach_banks(self, detail, individual): | ||
| Bank = self.env["res.partner.bank"] | ||
| for line in detail.bank_line_ids: | ||
| vals = { | ||
| "partner_id": individual.id, | ||
| "acc_number": line.acc_number, | ||
| } | ||
| if line.acc_holder_name: | ||
| vals["acc_holder_name"] = line.acc_holder_name | ||
| if line.bank_id: | ||
| vals["bank_id"] = line.bank_id.id | ||
| Bank.create(vals) | ||
|
|
||
| def _attach_id_docs(self, detail, individual): | ||
| RegId = self.env["spp.registry.id"] | ||
| for line in detail.id_doc_line_ids: | ||
| RegId.create( | ||
| { | ||
| "partner_id": individual.id, | ||
| "id_type_id": line.id_type_id.id, | ||
| "value": line.value, | ||
| "expiry_date": line.expiry_date, | ||
| } | ||
| ) |
There was a problem hiding this comment.
Creating records inside a loop (SppPhone.create, Bank.create, RegId.create) is a performance bottleneck in Odoo because it triggers multiple database inserts. Accumulating the values in a list and calling create once with the list of dictionaries is much more efficient.
def _attach_phones(self, detail, individual):
SppPhone = self.env["spp.phone.number"]
phone_vals = [
{
"partner_id": individual.id,
"phone_no": line.phone_no,
"country_id": line.country_id.id if line.country_id else False,
"date_collected": fields.Date.today(),
}
for line in detail.phone_line_ids
]
if phone_vals:
SppPhone.create(phone_vals)
def _attach_banks(self, detail, individual):
Bank = self.env["res.partner.bank"]
bank_vals = []
for line in detail.bank_line_ids:
vals = {
"partner_id": individual.id,
"acc_number": line.acc_number,
}
if line.acc_holder_name:
vals["acc_holder_name"] = line.acc_holder_name
if line.bank_id:
vals["bank_id"] = line.bank_id.id
bank_vals.append(vals)
if bank_vals:
Bank.create(bank_vals)
def _attach_id_docs(self, detail, individual):
RegId = self.env["spp.registry.id"]
doc_vals = [
{
"partner_id": individual.id,
"id_type_id": line.id_type_id.id,
"value": line.value,
"expiry_date": line.expiry_date,
}
for line in detail.id_doc_line_ids
]
if doc_vals:
RegId.create(doc_vals)
Comment on lines
+233
to
+270
| def _attach_phones(self, phone_lines, partner): | ||
| """Create spp.phone.number records (the registry's Phone Numbers list) | ||
| on ``partner`` from the given phone rows.""" | ||
| SppPhone = self.env["spp.phone.number"] | ||
| for line in phone_lines: | ||
| SppPhone.create( | ||
| { | ||
| "partner_id": partner.id, | ||
| "phone_no": line.phone_no, | ||
| "country_id": line.country_id.id if line.country_id else False, | ||
| "date_collected": fields.Date.today(), | ||
| } | ||
| ) | ||
|
|
||
| def _attach_banks(self, detail, group): | ||
| Bank = self.env["res.partner.bank"] | ||
| for line in detail.bank_line_ids: | ||
| vals = { | ||
| "partner_id": group.id, | ||
| "acc_number": line.acc_number, | ||
| } | ||
| if line.acc_holder_name: | ||
| vals["acc_holder_name"] = line.acc_holder_name | ||
| if line.bank_id: | ||
| vals["bank_id"] = line.bank_id.id | ||
| Bank.create(vals) | ||
|
|
||
| def _attach_id_docs(self, detail, group): | ||
| RegId = self.env["spp.registry.id"] | ||
| for line in detail.id_doc_line_ids: | ||
| RegId.create( | ||
| { | ||
| "partner_id": group.id, | ||
| "id_type_id": line.id_type_id.id, | ||
| "value": line.value, | ||
| "expiry_date": line.expiry_date, | ||
| } | ||
| ) |
There was a problem hiding this comment.
Creating records inside a loop (SppPhone.create, Bank.create, RegId.create) is a performance bottleneck in Odoo because it triggers multiple database inserts. Accumulating the values in a list and calling create once with the list of dictionaries is much more efficient.
def _attach_phones(self, phone_lines, partner):
"""Create spp.phone.number records (the registry's Phone Numbers list)
on partner from the given phone rows."""
SppPhone = self.env["spp.phone.number"]
phone_vals = [
{
"partner_id": partner.id,
"phone_no": line.phone_no,
"country_id": line.country_id.id if line.country_id else False,
"date_collected": fields.Date.today(),
}
for line in phone_lines
]
if phone_vals:
SppPhone.create(phone_vals)
def _attach_banks(self, detail, group):
Bank = self.env["res.partner.bank"]
bank_vals = []
for line in detail.bank_line_ids:
vals = {
"partner_id": group.id,
"acc_number": line.acc_number,
}
if line.acc_holder_name:
vals["acc_holder_name"] = line.acc_holder_name
if line.bank_id:
vals["bank_id"] = line.bank_id.id
bank_vals.append(vals)
if bank_vals:
Bank.create(bank_vals)
def _attach_id_docs(self, detail, group):
RegId = self.env["spp.registry.id"]
doc_vals = [
{
"partner_id": group.id,
"id_type_id": line.id_type_id.id,
"value": line.value,
"expiry_date": line.expiry_date,
}
for line in detail.id_doc_line_ids
]
if doc_vals:
RegId.create(doc_vals)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 19.0 #242 +/- ##
==========================================
+ Coverage 71.50% 71.53% +0.02%
==========================================
Files 1001 1004 +3
Lines 58626 59883 +1257
==========================================
+ Hits 41921 42836 +915
- Misses 16705 17047 +342
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…ady has one (#871) A group can have at most one Head of Household, so the Head role is now removed from the Role options when the target group already has an active head. Implemented as a computed allowed_role_ids driving the Role field domain (UI restriction only; head-of-household configurability stays with #1006).
Replace the single new-head dropdown with an editable members-with-roles table: one role line is seeded per active group member, and the member assigned the Head role becomes the new head (a constraint blocks two heads). Make the current head non-clickable, drop the unused Effective Date field, and remove the Voluntary Transfer reason. Add reason-driven required documents: a per-reason document config on the CR type (shown only for types whose detail exposes a reason) drives the request's required documents via _get_effective_required_document_ids(). The review preview now renders the household, reason, remarks and a members table (Name / Current Role / New Role).
Per the updated #871 spec, the Add Member CR first page now searches for an existing individual registrant instead of building a new individual from scratch. - detail model: replace the create-new field set (names, demographics, phone/ bank/ID lines) with an individual_id picker scoped by a computed domain to existing individuals not already in the group; keep the Role picker and the 'no Head when the group already has one' restriction. - strategy: apply() creates a membership for the selected individual; preview() reads the individual's fields and shows 'The following individual is to be added to the group'. - view: first page is a member search + role. Also update the MIS demo generator so its Add Member and Change Head of Household change requests match the redesigned detail contracts (register the individual then set individual_id; rebuild the change-of-head role lines).
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?
QA review of
spp_cr_types_advancedreturned findings on the Create Group (#876) and Add Member (#871) change-request flows. The main issue: the review page showed only counts for relational data (phones, bank accounts, ID documents, members) and omitted several defined fields (email, area, location, member demographics).This branch consolidates both tickets (they share
spp_change_request_v2, sofeat/871was merged intofeat/876to stop cross-merging).How was the change implemented?
A generic render contract on each strategy's
preview(), rendered by_generate_review_comparison_html():_tables— one2many fields render as their own tables (Phone Numbers, Bank Accounts, ID Documents, Existing Members)._sections— entities that are full records (new group members) render as a labelled detail block each, with all fields + that member's own phones._render_data_tables()/_render_data_sections()are generic and reused by both Create Group and Add Member.New unit tests
test_create_group_strategy.py:test_preview_one2many_as_tables_and_scalars,test_preview_members_table_and_sections,test_review_comparison_html_renders_tables(+ updatedtest_preview).test_add_member_strategy.py:test_preview_shows_fields_and_tables.Unit tests executed by the author
spp_change_request_v2full suite: 0 failed, 0 error of 324 tests. Pre-commit clean.How to test manually
Create a Create Group and an Add Member CR with phones/bank/ID docs and members, advance to the Review stage → Proposed Changes, and confirm the data shows as tables / detail blocks (not counts), all fields appear, and the Add Member head-replacement toggle is gone. (See the QA guides on #876 / #871.)
Related links
#876, #871 (parent: Review [spp_cr_types_advanced]); head-of-household handling deferred to #1006