From b45a134aa46b57a8c81c6f292602facd7abada15 Mon Sep 17 00:00:00 2001
From: gsinghpal <gur_preet.singh@outlook.com>
Date: Fri, 24 Apr 2026 21:36:58 -0400
Subject: [PATCH] refactor(jobs): address code review feedback on fp.job

- Sequence: add noupdate="1" to fp_job_sequences.xml. Without it,
  every module update resets number_next to 1, corrupting the live
  job-number stream. Matches fp_sequence_data.xml convention.
- action_cancel now raises UserError on an already-cancelled job
  instead of silently rewriting state. Audit-grade traceability
  expects explicit failures.
- Added TODO marker for action_hold / action_resume /
  action_revert_to_confirmed so future authors don't bypass the
  state-machine guards.
- Tests: added cannot_cancel_done (covers the dead-code UserError
  branch) and cannot_cancel_already_cancelled.

Manifest version bumped 19.0.8.2.0 -> 19.0.8.2.1.

Part of: native job model migration (spec 2026-04-25)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 fusion_plating/fusion_plating/__manifest__.py  |  2 +-
 .../fusion_plating/data/fp_job_sequences.xml   |  5 ++++-
 fusion_plating/fusion_plating/models/fp_job.py | 14 +++++++++++++-
 .../tests/test_fp_job_state_machine.py         | 18 ++++++++++++++++++
 4 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/fusion_plating/fusion_plating/__manifest__.py b/fusion_plating/fusion_plating/__manifest__.py
index 73014a4e..b9e282ab 100644
--- a/fusion_plating/fusion_plating/__manifest__.py
+++ b/fusion_plating/fusion_plating/__manifest__.py
@@ -5,7 +5,7 @@
 
 {
     'name': 'Fusion Plating',
-    'version': '19.0.8.2.0',
+    'version': '19.0.8.2.1',
     'category': 'Manufacturing/Plating',
     'summary': 'Core plating / metal finishing ERP: facilities, processes, tanks, baths, jobs, operators.',
     'description': """
diff --git a/fusion_plating/fusion_plating/data/fp_job_sequences.xml b/fusion_plating/fusion_plating/data/fp_job_sequences.xml
index 775838f0..ae13baaf 100644
--- a/fusion_plating/fusion_plating/data/fp_job_sequences.xml
+++ b/fusion_plating/fusion_plating/data/fp_job_sequences.xml
@@ -1,5 +1,8 @@
 <?xml version="1.0" encoding="UTF-8"?>
-<odoo>
+<!-- noupdate="1" is REQUIRED — without it, every -u fusion_plating
+     resets number_next back to 1, which corrupts the live sequence
+     on every module update. Matches the convention in fp_sequence_data.xml. -->
+<odoo noupdate="1">
     <!-- Sequence for fp.job. Format: WH/JOB/00001 onwards.
          Migrated mrp.production records keep their WH/MO/... names. -->
     <record id="seq_fp_job" model="ir.sequence">
diff --git a/fusion_plating/fusion_plating/models/fp_job.py b/fusion_plating/fusion_plating/models/fp_job.py
index aab0e553..d1305640 100644
--- a/fusion_plating/fusion_plating/models/fp_job.py
+++ b/fusion_plating/fusion_plating/models/fp_job.py
@@ -89,6 +89,14 @@ class FpJob(models.Model):
                 vals['name'] = self.env['ir.sequence'].next_by_code('fp.job') or _('New')
         return super().create(vals_list)
 
+    # ------------------------------------------------------------------
+    # State machine — actions
+    # ------------------------------------------------------------------
+    # TODO(fp.job state-machine completeness): action_hold, action_resume,
+    # action_revert_to_confirmed (rework path) — to be added when shopfloor
+    # / rework workflows are wired up. For now, draft → confirmed and the
+    # cancel paths are the only enforced transitions; everything else is
+    # an explicit `state` write by privileged code.
     def action_confirm(self):
         for job in self:
             if job.state != 'draft':
@@ -102,7 +110,11 @@ class FpJob(models.Model):
         for job in self:
             if job.state == 'done':
                 raise UserError(_(
-                    "Job %s is done - cannot cancel."
+                    "Job %s is done — cannot cancel."
+                ) % job.name)
+            if job.state == 'cancelled':
+                raise UserError(_(
+                    "Job %s is already cancelled."
                 ) % job.name)
             job.state = 'cancelled'
         return True
diff --git a/fusion_plating/fusion_plating/tests/test_fp_job_state_machine.py b/fusion_plating/fusion_plating/tests/test_fp_job_state_machine.py
index 4f1242d1..47a9429e 100644
--- a/fusion_plating/fusion_plating/tests/test_fp_job_state_machine.py
+++ b/fusion_plating/fusion_plating/tests/test_fp_job_state_machine.py
@@ -44,3 +44,21 @@ class TestFpJobStateMachine(TransactionCase):
         job.action_cancel()
         with self.assertRaises(UserError):
             job.action_confirm()
+
+    def test_cannot_cancel_done(self):
+        # Done jobs cannot be cancelled — covers the UserError branch in
+        # action_cancel.
+        job = self._make_job()
+        job.action_confirm()
+        # Force the state to 'done' for the test (no public action yet —
+        # done is set by step-completion logic landing in Task 1.5+).
+        job.state = 'done'
+        with self.assertRaises(UserError):
+            job.action_cancel()
+
+    def test_cannot_cancel_already_cancelled(self):
+        # Idempotent re-cancel is now an explicit error.
+        job = self._make_job()
+        job.action_cancel()
+        with self.assertRaises(UserError):
+            job.action_cancel()