fix(change_request): show real data (not counts) on Create Group review (#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.

Author: emjay0921

spp_change_request_v2/models/change_request.py

@@ -1154,13 +1154,74 @@ def _generate_review_comparison_html(self):
 
         action = changes.pop("_action", None)
         header = changes.pop("_header", None)
+        tables = changes.pop("_tables", None)
+        sections = changes.pop("_sections", None)
 
         # Determine if this is a field-mapping type (has old/new dicts)
         has_comparison = any(isinstance(v, dict) and "old" in v and "new" in v for v in changes.values())
 
         if has_comparison:
-            return self._render_comparison_table(changes, header=header)
-        return self._render_action_summary(action, changes, header=header)
+            html = self._render_comparison_table(changes, header=header)
+        else:
+            html = self._render_action_summary(action, changes, header=header)
+        if tables:
+            html += self._render_data_tables(tables)
+        if sections:
+            html += self._render_data_sections(sections)
+        return html
+
+    def _render_data_tables(self, tables):
+        """Render preview() ``_tables`` entries as separate HTML tables.
+
+        Each entry is ``{"title", "columns", "rows"}`` where ``rows`` is a list
+        of cell-string lists. Used to show one2many data (phones, bank accounts,
+        ID documents, ...) on the review page instead of a bare count (OP#876).
+        """
+        out = []
+        for table in tables:
+            columns = table.get("columns") or []
+            rows = table.get("rows") or []
+            out.append(f'<h6 class="mt-3 mb-1">{html_escape(table.get("title") or "")}</h6>')
+            if not rows:
+                out.append('<div class="text-muted small mb-0"><i class="fa fa-info-circle me-2"></i>None.</div>')
+                continue
+            out.append('<table class="table table-sm table-bordered mb-0" style="width:100%">')
+            out.append(
+                "<thead><tr>"
+                + "".join(f'<th class="bg-light">{html_escape(c)}</th>' for c in columns)
+                + "</tr></thead>"
+            )
+            out.append("<tbody>")
+            for row in rows:
+                out.append(
+                    "<tr>" + "".join(f"<td>{html_escape('' if c is None else str(c))}</td>" for c in row) + "</tr>"
+                )
+            out.append("</tbody></table>")
+        return "".join(out)
+
+    def _render_data_sections(self, sections):
+        """Render preview() ``_sections`` entries — one labelled detail block per
+        entity (e.g. each new group member): its fields as a key/value table plus
+        any nested ``tables`` (e.g. that member's phone numbers) (OP#876).
+        """
+        out = []
+        for section in sections:
+            out.append(f'<h6 class="mt-3 mb-1">{html_escape(section.get("title") or "")}</h6>')
+            field_rows = section.get("fields") or []
+            if field_rows:
+                out.append('<table class="table table-sm table-bordered mb-0" style="width:100%">')
+                out.append("<tbody>")
+                for label, value in field_rows:
+                    display = html_escape(value) if value else '<span class="text-muted">—</span>'
+                    out.append(
+                        f'<tr><td class="bg-light" style="width:30%"><strong>{html_escape(label)}</strong></td>'
+                        f"<td>{display}</td></tr>"
+                    )
+                out.append("</tbody></table>")
+            nested = section.get("tables")
+            if nested:
+                out.append(self._render_data_tables(nested))
+        return "".join(out)
 
     def _render_comparison_table(self, changes, header=None):
         """Render a three-column comparison table for field-mapping CR types."""

spp_change_request_v2/strategies/create_group.py

@@ -66,18 +66,104 @@ def preview(self, change_request):
         elif new_heads:
             head_label = new_heads[0].full_name
 
+        location = None
+        if detail.latitude or detail.longitude:
+            location = f"{detail.latitude}, {detail.longitude}"
+
+        # One2many lines are shown as separate tables on the review page (OP#876).
+        # preview() supplies them via the generic "_tables" contract:
+        # each entry is {title, columns, rows} with rows as lists of cell strings.
+        tables = []
+        phone_rows = [
+            [p.phone_no or "", p.country_id.display_name or "", _("Yes") if p.is_primary else ""]
+            for p in detail.phone_line_ids
+        ]
+        if phone_rows:
+            tables.append(
+                {"title": _("Phone Numbers"), "columns": [_("Number"), _("Country"), _("Primary")], "rows": phone_rows}
+            )
+        bank_rows = [
+            [b.acc_number or "", b.acc_holder_name or "", b.bank_id.display_name or ""] for b in detail.bank_line_ids
+        ]
+        if bank_rows:
+            tables.append(
+                {
+                    "title": _("Bank Accounts"),
+                    "columns": [_("Account Number"), _("Account Holder"), _("Bank")],
+                    "rows": bank_rows,
+                }
+            )
+        id_doc_rows = [
+            [d.id_type_id.display_name or "", d.value or "", str(d.expiry_date) if d.expiry_date else ""]
+            for d in detail.id_doc_line_ids
+        ]
+        if id_doc_rows:
+            tables.append(
+                {
+                    "title": _("ID Documents"),
+                    "columns": [_("Type"), _("Number"), _("Expiry Date")],
+                    "rows": id_doc_rows,
+                }
+            )
+
+        # Existing members: a simple Name + Role table.
+        existing_rows = [
+            [m.individual_id.name or "", m.membership_type_id.display or ""] for m in detail.member_existing_ids
+        ]
+        if existing_rows:
+            tables.append({"title": _("Existing Members"), "columns": [_("Name"), _("Role")], "rows": existing_rows})
+
+        # New members: one labelled detail block each (full individual record plus
+        # that member's own phone numbers), via the generic "_sections" contract.
+        sections = []
+        for m in detail.member_new_ids:
+            title = _("New member: %s") % (m.full_name or "")
+            if m.membership_type_id:
+                title = f"{title} ({m.membership_type_id.display})"
+            member_phone_rows = [
+                [p.phone_no or "", p.country_id.display_name or "", _("Yes") if p.is_primary else ""]
+                for p in m.phone_line_ids
+            ]
+            member_tables = []
+            if member_phone_rows:
+                member_tables.append(
+                    {
+                        "title": _("Phone Numbers"),
+                        "columns": [_("Number"), _("Country"), _("Primary")],
+                        "rows": member_phone_rows,
+                    }
+                )
+            sections.append(
+                {
+                    "title": title,
+                    "fields": [
+                        [_("Role"), m.membership_type_id.display or ""],
+                        [_("Date of Birth"), str(m.birthdate) if m.birthdate else ""],
+                        [_("Gender"), m.gender_id.display_name or ""],
+                        [_("Civil Status"), m.civil_status_id.display_name or ""],
+                        [_("Occupation"), m.occupation_id.display_name or ""],
+                        [_("Birth Place"), m.birth_place or ""],
+                        [_("Income"), str(m.income) if m.income else ""],
+                        [_("Area"), m.area_id.display_name or ""],
+                        [_("Address"), m.address or ""],
+                        [_("Email"), m.email or ""],
+                    ],
+                    "tables": member_tables,
+                }
+            )
+
         return {
             "_action": "create_group",
             "_header": _("The following group is to be added:"),
             "group_name": detail.group_name,
             "group_type": detail.group_type_id.display if detail.group_type_id else None,
+            "area": detail.area_id.display_name if detail.area_id else None,
             "address": detail.address,
-            "phone_count": len(detail.phone_line_ids),
-            "bank_count": len(detail.bank_line_ids),
-            "id_doc_count": len(detail.id_doc_line_ids),
-            "existing_member_count": len(detail.member_existing_ids),
-            "new_member_count": len(detail.member_new_ids),
+            "email": detail.email,
+            "location": location,
             "head_of_household": head_label,
+            "_tables": tables,
+            "_sections": sections,
         }
 
     # ──────────────────────────────────────────────────────────────────────

spp_change_request_v2/tests/test_create_group_strategy.py

@@ -327,11 +327,133 @@ def test_preview(self):
         preview = cr.action_preview_changes()
         self.assertEqual(preview["_action"], "create_group")
         self.assertEqual(preview["group_name"], "Preview Group")
-        self.assertEqual(preview["new_member_count"], 1)
-        self.assertEqual(preview["bank_count"], 1)
+        # The new member is now a "_sections" detail block, not a count (OP#876).
+        self.assertNotIn("new_member_count", preview)
+        self.assertEqual(len(preview["_sections"]), 1)
+        # The bank line is now surfaced as a "_tables" entry, not a count (OP#876).
+        self.assertNotIn("bank_count", preview)
+        bank_tables = [t for t in preview["_tables"] if t["title"] == "Bank Accounts"]
+        self.assertEqual(len(bank_tables), 1)
+        self.assertEqual(len(bank_tables[0]["rows"]), 1)
+        self.assertIn("12-34-56", bank_tables[0]["rows"][0])
         if self.head_kind:
             self.assertEqual(preview["head_of_household"], "PERSON, Head")
 
+    def test_preview_one2many_as_tables_and_scalars(self):
+        """OP#876: phones / banks / ID docs are surfaced as `_tables` (actual
+        data, not counts), and the previously-missing scalar fields are added."""
+        id_type = self.env["spp.vocabulary.code"].search(
+            [("vocabulary_id.namespace_uri", "=", "urn:openspp:vocab:id-type")], limit=1
+        )
+        cr = self._make_cr(
+            group_name="Tables Group",
+            address="12 Rizal St",
+            email="group@example.com",
+            phone_line_ids=[
+                (0, 0, {"phone_no": "+63911111111", "is_primary": True}),
+                (0, 0, {"phone_no": "+63922222222"}),
+            ],
+            bank_line_ids=[(0, 0, {"acc_number": "ACC-1", "acc_holder_name": "Jane"})],
+            id_doc_line_ids=([(0, 0, {"id_type_id": id_type.id, "value": "ID-9"})] if id_type else []),
+        )
+        preview = cr.action_preview_changes()
+
+        # Previously-missing scalar fields are now surfaced.
+        self.assertEqual(preview["email"], "group@example.com")
+        self.assertEqual(preview["address"], "12 Rizal St")
+        self.assertIn("area", preview)
+
+        # Counts are replaced by the generic `_tables` contract.
+        for removed in ("phone_count", "bank_count", "id_doc_count"):
+            self.assertNotIn(removed, preview)
+
+        titles = [t["title"] for t in preview["_tables"]]
+        self.assertIn("Phone Numbers", titles)
+        self.assertIn("Bank Accounts", titles)
+        phones = next(t for t in preview["_tables"] if t["title"] == "Phone Numbers")
+        self.assertEqual(phones["columns"], ["Number", "Country", "Primary"])
+        self.assertEqual(len(phones["rows"]), 2)
+        self.assertIn("+63911111111", phones["rows"][0])
+        if id_type:
+            self.assertIn("ID Documents", titles)
+
+    def test_preview_members_table_and_sections(self):
+        """OP#876: existing members render as a Name/Role table; new members
+        render as per-member detail sections (with their own phones)."""
+        existing = self.partner_model.create({"name": "Existing Member A", "is_registrant": True, "is_group": False})
+        cr = self._make_cr(
+            group_name="Members Group",
+            member_existing_ids=[
+                (
+                    0,
+                    0,
+                    {
+                        "individual_id": existing.id,
+                        "membership_type_id": self.member_kind.id if self.member_kind else False,
+                    },
+                )
+            ],
+            member_new_ids=[
+                (
+                    0,
+                    0,
+                    {
+                        "given_name": "Nina",
+                        "family_name": "Cruz",
+                        "birthdate": "2000-05-05",
+                        "membership_type_id": self.head_kind.id if self.head_kind else False,
+                        "phone_line_ids": [(0, 0, {"phone_no": "+63999999999", "is_primary": True})],
+                    },
+                )
+            ],
+        )
+        preview = cr.action_preview_changes()
+
+        # Counts are gone.
+        self.assertNotIn("existing_member_count", preview)
+        self.assertNotIn("new_member_count", preview)
+
+        # Existing members -> a Name/Role table.
+        existing_tbl = [t for t in preview["_tables"] if t["title"] == "Existing Members"]
+        self.assertEqual(len(existing_tbl), 1)
+        self.assertIn("Existing Member A", existing_tbl[0]["rows"][0])
+
+        # New members -> a per-member detail section with fields + own phones.
+        self.assertEqual(len(preview["_sections"]), 1)
+        sec = preview["_sections"][0]
+        self.assertIn("Nina", sec["title"])
+        labels = [f[0] for f in sec["fields"]]
+        self.assertIn("Date of Birth", labels)
+        self.assertIn("Gender", labels)
+        self.assertTrue(any(t["title"] == "Phone Numbers" for t in sec["tables"]))
+        self.assertIn("+63999999999", sec["tables"][0]["rows"][0])
+
+        # The rendered review HTML surfaces the member data.
+        html = cr._generate_review_comparison_html()
+        self.assertIn("Existing Members", html)
+        self.assertIn("Existing Member A", html)
+        self.assertIn("Nina", html)
+        self.assertIn("+63999999999", html)
+
+    def test_review_comparison_html_renders_tables(self):
+        """The review page HTML shows the actual phone / bank rows as tables,
+        not a bare count (OP#876)."""
+        cr = self._make_cr(
+            group_name="HTML Group",
+            email="g@example.com",
+            phone_line_ids=[(0, 0, {"phone_no": "+63900000000", "is_primary": True})],
+            bank_line_ids=[(0, 0, {"acc_number": "BANK-777"})],
+        )
+        html = cr._generate_review_comparison_html()
+        self.assertIn("Phone Numbers", html)
+        self.assertIn("+63900000000", html)
+        self.assertIn("Bank Accounts", html)
+        self.assertIn("BANK-777", html)
+        self.assertIn("g@example.com", html)
+        # Raw count keys must not leak into the review output.
+        self.assertNotIn("phone_count", html)
+        self.assertNotIn("bank_count", html)
+
     # ──────────────────────────────────────────────────────────────────
     # Wizard flow (OP#876 round 2): Add Member wizard, both modes
     # ──────────────────────────────────────────────────────────────────