Merge pull request #130 from OpenSPP/fix/batch-approval-wizard-delete-lines

fix(spp_change_request_v2): fix batch approval wizard line deletion

Author: emjay0921

spp_change_request_v2/__manifest__.py

@@ -1,6 +1,6 @@
 {
     "name": "OpenSPP Change Request V2",
-    "version": "19.0.2.0.1",
+    "version": "19.0.2.0.2",
     "sequence": 50,
     "category": "OpenSPP",
     "summary": "Configuration-driven change request system with UX improvements, conflict detection and duplicate prevention",

spp_change_request_v2/tests/test_ux_wizards.py

@@ -225,9 +225,108 @@ def test_wizard_onchange_clears_mismatched_registrant(self):
         self.assertFalse(wizard.registrant_id)
 
 
+@tagged("post_install", "-at_install", "cr_ux")
+class TestBatchApprovalWizardBase(TestChangeRequestBase):
+    """Tests for batch approval wizard that don't require pending CRs."""
+
+    def _create_wizard(self, cr_ids, action_type="approve", **kwargs):
+        """Helper to create a batch wizard via create_from_selection."""
+        result = (
+            self.env["spp.cr.batch.approval.wizard"].with_context(active_ids=cr_ids).create_from_selection(action_type)
+        )
+        wizard = self.env["spp.cr.batch.approval.wizard"].browse(result["res_id"])
+        if kwargs:
+            wizard.write(kwargs)
+        return wizard
+
+    def test_create_from_selection_no_active_ids(self):
+        """Test create_from_selection raises error with no active_ids."""
+        with self.assertRaises(UserError):
+            self.env["spp.cr.batch.approval.wizard"].create_from_selection("approve")
+
+    def test_create_from_selection_returns_action(self):
+        """Test create_from_selection returns a valid window action."""
+        cr_type = self.env["spp.change.request.type"].search([], limit=1)
+        if not cr_type:
+            self.skipTest("No CR type available")
+        draft_cr = self.env["spp.change.request"].create(
+            {"request_type_id": cr_type.id, "registrant_id": self.group.id}
+        )
+        result = (
+            self.env["spp.cr.batch.approval.wizard"]
+            .with_context(active_ids=[draft_cr.id])
+            .create_from_selection("approve")
+        )
+        self.assertEqual(result["type"], "ir.actions.act_window")
+        self.assertEqual(result["res_model"], "spp.cr.batch.approval.wizard")
+        self.assertEqual(result["target"], "new")
+        self.assertTrue(result["res_id"])
+
+    def test_create_from_selection_action_types(self):
+        """Test create_from_selection sets action_type correctly."""
+        cr_type = self.env["spp.change.request.type"].search([], limit=1)
+        if not cr_type:
+            self.skipTest("No CR type available")
+        draft_cr = self.env["spp.change.request"].create(
+            {"request_type_id": cr_type.id, "registrant_id": self.group.id}
+        )
+        for action_type in ("approve", "reject", "revision"):
+            result = (
+                self.env["spp.cr.batch.approval.wizard"]
+                .with_context(active_ids=[draft_cr.id])
+                .create_from_selection(action_type)
+            )
+            wizard = self.env["spp.cr.batch.approval.wizard"].browse(result["res_id"])
+            self.assertEqual(wizard.action_type, action_type)
+
+    def test_error_message_not_pending(self):
+        """Test error message for CR not in pending state."""
+        cr_type = self.env["spp.change.request.type"].search([], limit=1)
+        if not cr_type:
+            self.skipTest("No CR type available")
+        draft_cr = self.env["spp.change.request"].create(
+            {"request_type_id": cr_type.id, "registrant_id": self.group.id}
+        )
+        wizard = self._create_wizard([draft_cr.id])
+        line = wizard.line_ids[0]
+        self.assertFalse(line.can_process)
+        self.assertIn("Not pending approval", line.error_message)
+
+    def test_batch_approve_requires_valid_crs(self):
+        """Test batch approve fails if no valid CRs."""
+        cr_type = self.env["spp.change.request.type"].search([], limit=1)
+        if not cr_type:
+            self.skipTest("No CR type available")
+        draft_cr = self.env["spp.change.request"].create(
+            {"request_type_id": cr_type.id, "registrant_id": self.group.id}
+        )
+        wizard = self._create_wizard([draft_cr.id])
+        self.assertEqual(wizard.valid_count, 0)
+        with self.assertRaises(UserError):
+            wizard.action_confirm()
+
+    def test_batch_wizard_line_removal(self):
+        """Test that lines can be removed from a saved wizard."""
+        cr_type = self.env["spp.change.request.type"].search([], limit=1)
+        if not cr_type:
+            self.skipTest("No CR type available")
+        crs = self.env["spp.change.request"]
+        for _i in range(3):
+            crs |= self.env["spp.change.request"].create(
+                {"request_type_id": cr_type.id, "registrant_id": self.group.id}
+            )
+        wizard = self._create_wizard(crs.ids)
+        self.assertEqual(len(wizard.line_ids), 3)
+
+        # Remove one line
+        wizard.line_ids[0].unlink()
+        wizard.invalidate_recordset()
+        self.assertEqual(len(wizard.line_ids), 2)
+
+
 @tagged("post_install", "-at_install", "cr_ux")
 class TestBatchApprovalWizard(TestChangeRequestBase):
-    """Tests for the batch approval wizard."""
+    """Tests for batch approval wizard that require pending CRs."""
 
     @classmethod
     def setUpClass(cls):
@@ -237,7 +336,6 @@ def setUpClass(cls):
 
     def setUp(self):
         super().setUp()
-        # Create multiple pending CRs for batch testing
         cr_type = self.env["spp.change.request.type"].search([("approval_definition_id", "!=", False)], limit=1)
 
         if not cr_type:
@@ -254,56 +352,57 @@ def setUp(self):
             cr.action_submit_for_approval()
             self.pending_crs |= cr
 
+    def _create_wizard(self, cr_ids, action_type="approve", **kwargs):
+        """Helper to create a batch wizard via create_from_selection."""
+        result = (
+            self.env["spp.cr.batch.approval.wizard"].with_context(active_ids=cr_ids).create_from_selection(action_type)
+        )
+        wizard = self.env["spp.cr.batch.approval.wizard"].browse(result["res_id"])
+        if kwargs:
+            wizard.write(kwargs)
+        return wizard
+
     def test_batch_wizard_initialization(self):
         """Test batch wizard initializes with selected CRs."""
-        wizard = self.env["spp.cr.batch.approval.wizard"].with_context(active_ids=self.pending_crs.ids).create({})
+        wizard = self._create_wizard(self.pending_crs.ids)
 
         self.assertEqual(wizard.total_count, 3)
         self.assertEqual(len(wizard.line_ids), 3)
 
     def test_batch_wizard_counts(self):
         """Test batch wizard computes valid/invalid counts correctly."""
-        wizard = self.env["spp.cr.batch.approval.wizard"].with_context(active_ids=self.pending_crs.ids).create({})
+        wizard = self._create_wizard(self.pending_crs.ids)
 
         # All should be valid if user can approve
         self.assertEqual(wizard.total_count, wizard.valid_count + wizard.invalid_count)
 
-    def test_batch_approve_requires_valid_crs(self):
-        """Test batch approve fails if no valid CRs."""
-        # Create wizard with draft CR (cannot be approved)
-        cr_type = self.env["spp.change.request.type"].search([], limit=1)
-        draft_cr = self.env["spp.change.request"].create(
-            {
-                "request_type_id": cr_type.id,
-                "registrant_id": self.group.id,
-            }
-        )
-
-        wizard = self.env["spp.cr.batch.approval.wizard"].with_context(active_ids=[draft_cr.id]).create({})
-
-        # Should have 0 valid CRs
-        self.assertEqual(wizard.valid_count, 0)
+    def test_batch_reject_requires_comment(self):
+        """Test batch reject requires a reason."""
+        wizard = self._create_wizard(self.pending_crs.ids, action_type="reject", comment="")
 
+        # Should fail without comment
         with self.assertRaises(UserError):
             wizard.action_confirm()
 
-    def test_batch_reject_requires_comment(self):
-        """Test batch reject requires a reason."""
-        wizard = (
-            self.env["spp.cr.batch.approval.wizard"]
-            .with_context(active_ids=self.pending_crs.ids)
-            .create(
-                {
-                    "action_type": "reject",
-                    "comment": "",
-                }
-            )
-        )
+    def test_batch_revision_requires_comment(self):
+        """Test batch revision requires notes."""
+        wizard = self._create_wizard(self.pending_crs.ids, action_type="revision", comment="")
 
-        # Should fail without comment
         with self.assertRaises(UserError):
             wizard.action_confirm()
 
+    def test_error_message_mixed_crs(self):
+        """Test error messages with mix of pending and draft CRs."""
+        cr_type = self.env["spp.change.request.type"].search([], limit=1)
+        draft_cr = self.env["spp.change.request"].create(
+            {"request_type_id": cr_type.id, "registrant_id": self.group.id}
+        )
+        wizard = self._create_wizard(self.pending_crs.ids + [draft_cr.id])
+        self.assertTrue(wizard.total_count > 0)
+        invalid_lines = wizard.line_ids.filtered(lambda ln: not ln.can_process)
+        for line in invalid_lines:
+            self.assertTrue(line.error_message)
+
 
 @tagged("post_install", "-at_install", "cr_ux")
 class TestConflictComparisonWizard(TestChangeRequestBase):

spp_change_request_v2/views/batch_approval_wizard_views.xml

@@ -95,7 +95,7 @@
                     <!-- Selected requests -->
                     <notebook>
                         <page string="Selected Requests" name="requests">
-                            <field name="line_ids" nolabel="1" readonly="1">
+                            <field name="line_ids" nolabel="1">
                                 <list
                                     decoration-muted="not can_process"
                                     decoration-danger="not can_process"
@@ -115,12 +115,6 @@
                                         invisible="not can_process"
                                         title="Ready to process"
                                     />
-                                    <button
-                                        name="action_remove_line"
-                                        type="object"
-                                        icon="fa-trash"
-                                        title="Remove from batch"
-                                    />
                                 </list>
                             </field>
                         </page>
@@ -211,39 +205,40 @@
     </record>
 
     <!-- ══════════════════════════════════════════════════════════════════════════
-         BATCH APPROVAL - SERVER ACTION (for list view context menu)
+         BATCH APPROVAL - SERVER ACTIONS (for list view context menu)
+         Creates the wizard record before opening it, so One2many line
+         operations (delete, buttons) work without "Please save first" errors.
          ══════════════════════════════════════════════════════════════════════════ -->
-    <!-- Note: Security is enforced via ir.model.access.csv - only validators can access wizard -->
-    <record id="action_batch_approve" model="ir.actions.act_window">
+    <record id="server_action_batch_approve" model="ir.actions.server">
         <field name="name">Batch Approve</field>
-        <field name="res_model">spp.cr.batch.approval.wizard</field>
-        <field name="view_mode">form</field>
-        <field name="view_id" ref="view_batch_approval_wizard_form" />
-        <field name="target">new</field>
+        <field name="model_id" ref="model_spp_change_request" />
         <field name="binding_model_id" ref="model_spp_change_request" />
         <field name="binding_view_types">list</field>
-        <field name="context">{'default_action_type': 'approve'}</field>
+        <field name="state">code</field>
+        <field name="code">
+action = env["spp.cr.batch.approval.wizard"].with_context(active_ids=records.ids).create_from_selection("approve")
+        </field>
     </record>
 
-    <record id="action_batch_reject" model="ir.actions.act_window">
+    <record id="server_action_batch_reject" model="ir.actions.server">
         <field name="name">Batch Decline</field>
-        <field name="res_model">spp.cr.batch.approval.wizard</field>
-        <field name="view_mode">form</field>
-        <field name="view_id" ref="view_batch_approval_wizard_form" />
-        <field name="target">new</field>
+        <field name="model_id" ref="model_spp_change_request" />
         <field name="binding_model_id" ref="model_spp_change_request" />
         <field name="binding_view_types">list</field>
-        <field name="context">{'default_action_type': 'reject'}</field>
+        <field name="state">code</field>
+        <field name="code">
+action = env["spp.cr.batch.approval.wizard"].with_context(active_ids=records.ids).create_from_selection("reject")
+        </field>
     </record>
 
-    <record id="action_batch_request_changes" model="ir.actions.act_window">
+    <record id="server_action_batch_request_changes" model="ir.actions.server">
         <field name="name">Batch Request Changes</field>
-        <field name="res_model">spp.cr.batch.approval.wizard</field>
-        <field name="view_mode">form</field>
-        <field name="view_id" ref="view_batch_approval_wizard_form" />
-        <field name="target">new</field>
+        <field name="model_id" ref="model_spp_change_request" />
         <field name="binding_model_id" ref="model_spp_change_request" />
         <field name="binding_view_types">list</field>
-        <field name="context">{'default_action_type': 'revision'}</field>
+        <field name="state">code</field>
+        <field name="code">
+action = env["spp.cr.batch.approval.wizard"].with_context(active_ids=records.ids).create_from_selection("revision")
+        </field>
     </record>
 </odoo>

spp_change_request_v2/wizards/batch_approval_wizard.py

@@ -79,44 +79,60 @@ class SPPCRBatchApprovalWizard(models.TransientModel):
     )
 
     # ══════════════════════════════════════════════════════════════════════════
-    # DEFAULT
+    # CREATION
     # ══════════════════════════════════════════════════════════════════════════
 
     @api.model
-    def default_get(self, fields_list):
-        """Initialize wizard with selected change requests."""
-        res = super().default_get(fields_list)
+    def create_from_selection(self, action_type="approve"):
+        """Create and populate wizard from selected change requests.
 
+        Called by server actions to create a saved wizard record before
+        opening the form, so that One2many operations (delete lines, etc.)
+        work without 'Please save your changes first' errors.
+        """
         active_ids = self.env.context.get("active_ids", [])
-        if active_ids and "line_ids" in fields_list:
-            crs = self.env["spp.change.request"].browse(active_ids)
-            # Prefetch all needed fields in a single query to avoid N+1
-            crs.read(["approval_state", "can_approve", "display_state"])
-            lines = []
-            for cr in crs:
-                can_process = cr.approval_state == "pending" and cr.can_approve
-                lines.append(
-                    Command.create(
-                        {
-                            "change_request_id": cr.id,
-                            "can_process": can_process,
-                            "error_message": "" if can_process else self._get_error_message(cr),
-                        }
-                    )
+        if not active_ids:
+            raise UserError(_("No change requests selected."))
+
+        crs = self.env["spp.change.request"].browse(active_ids)
+        crs.read(["approval_state", "can_approve", "display_state"])
+
+        lines = []
+        for change_request in crs:
+            can_process = change_request.approval_state == "pending" and change_request.can_approve
+            lines.append(
+                Command.create(
+                    {
+                        "change_request_id": change_request.id,
+                        "can_process": can_process,
+                        "error_message": "" if can_process else self._get_error_message(change_request),
+                    }
                 )
-            res["line_ids"] = lines
+            )
 
-        return res
+        wizard = self.create(
+            {
+                "action_type": action_type,
+                "line_ids": lines,
+            }
+        )
 
-    def _get_error_message(self, cr):
-        """Get error message explaining why CR cannot be processed.
+        return {
+            "type": "ir.actions.act_window",
+            "name": _("Batch Approval"),
+            "res_model": "spp.cr.batch.approval.wizard",
+            "res_id": wizard.id,
+            "view_mode": "form",
+            "view_id": self.env.ref("spp_change_request_v2.view_batch_approval_wizard_form").id,
+            "target": "new",
+        }
 
-        Note: Plain strings are used here instead of _() to avoid translation
-        context issues during default_get() in test scenarios.
-        """
-        if cr.approval_state != "pending":
-            return f"Not pending approval (state: {cr.display_state})"
-        if not cr.can_approve:
+    @staticmethod
+    def _get_error_message(change_request):
+        """Get error message explaining why CR cannot be processed."""
+        if change_request.approval_state != "pending":
+            return f"Not pending approval (state: {change_request.display_state})"
+        if not change_request.can_approve:
             return "You are not authorized to approve this request"
         return ""
 
@@ -311,11 +327,6 @@ class SPPCRBatchApprovalLine(models.TransientModel):
     )
     result_message = fields.Char()
 
-    def action_remove_line(self):
-        """Remove this line from the batch wizard."""
-        self.ensure_one()
-        self.unlink()
-
     @api.depends("change_request_id")
     def _compute_document_count(self):
         # Batch prefetch document_ids to avoid N+1 queries