fix(fp.job): post-shop state machine entech smoke fixes (Task 23)
Three bugs caught + fixed during entech battle test:
1. _fp_check_finish_gates calling button_mark_done triggered the
step-completion gate prematurely (step still in_progress at
pre-super time). Pass fp_skip_step_gate=True alongside
fp_check_gates_only — we know the operator is about to finish
the last open step.
2. _fp_schedule_cert_activity used env.get('fp.notification.template')
for presence check. env.get returns an EMPTY recordset (falsy),
not None — 'if not Template: return' silently exited and no
activity was ever scheduled. Switch to 'in self.env' check
pattern + explicit indexing. CLAUDE.md Rule 24.
3. _fp_check_advance_after_cert_issue + _fp_check_regress_after_cert_void
used 'state != issued' as outstanding-cert count. This made
voided certs count as outstanding forever, so void+re-issue
cycles never re-advanced. Switch to per-type coverage check:
each required cert TYPE needs at least one issued cert.
Regress mirrors: only fire if a type loses all issued certs.
CLAUDE.md gains Rule 24 (env.get falsy empty recordset trap).
Rule 25 (mail.template parse-time validation) renumbered.
Battle test ALL PASS on entech admin DB:
10/10 steps green — auto-advance, kanban placement, activity
schedule + auto-resolve, ACL guard, cert issue advance, void
regress, re-issue advance, manual ship.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsinghpal
@@ -348,7 +348,16 @@ Use only: `name`, `model_id`, `state`, `code` (or `function`/`model`), `interval
|
||||
21. **`ir.actions.act_window_close` is a no-op when the current action was opened with `target: "current"`**: replacing the current action wipes the breadcrumb backstack, so there's nothing to close back to. The user clicks "Back" and nothing happens (no error, no navigation). This bites every OWL client-action surface that calls another client action via `doAction({..., target: "current"})` — the destination has no way to return to the source. **Fix pattern for "Back" buttons in OWL client actions**: navigate EXPLICITLY to the landing/parent action by tag, e.g. `this.action.doAction({ type: "ir.actions.client", tag: "fp_shopfloor_landing", target: "current" })` — works regardless of how the action was reached (kanban tap, QR scan, smart button, direct URL). **Do NOT rely on `act_window_close`, `history.back()`, or `this.env.config.breadcrumbs`** — all three are unreliable across navigation paths. Bit us 2026-05-23 on the Job Workspace Back button after the kanban opened the workspace with `target: "current"`. The same pattern applies to every other "Back" button in shopfloor / manager / portal OWL surfaces — explicit destination via `tag:` is the only robust answer.
|
||||
22. **Odoo 19 HTML fields auto-wrap plain-string writes**: writing `co.report_header = 'Plating & Finishing'` to an HTML field (like `res.company.report_header`, `res.partner.comment`, `mail.template.body_html`, `product.template.description_sale`) stores `<p>Plating & Finishing</p>` after Odoo's HTML sanitizer runs. Equality tests against the raw input string FAIL (`payload['tagline'] != 'Plating & Finishing'`). **Three implications**: (a) **In tests**, don't `assertEqual` against the literal string you wrote — strip tags first, OR write the wrapped form (`<p>Plating & Finishing</p>`), OR write an explicit `Markup('<p>...</p>')` so the round-trip stays stable. (b) **In display code**, render HTML fields with `t-out` (QWeb) or `markup(...)` (OWL) — `t-esc` would render the literal `<p>` tags as text. (c) **In comparison logic**, normalize first: `from markupsafe import escape; escape(input_str)` produces the same shape the field stores. Bit us 2026-05-24 testing the lock-screen tagline source (`_lock_company_payload` reads `res.company.report_header`); the test that wrote a plain string and asserted equality failed because the value came back wrapped. The fix was to delete the brittle equality test — the helper's responsibility is just "use the field's value when present, else fall back," which is covered by the empty-field test. Generalizes to ANY HTML-typed Odoo field. Distinct from the `mail.template.body_html is Markup + jsonb` gotcha noted earlier in this file — that's about Markup objects vs strings; this is about the sanitizer wrapping plain strings on write.
|
||||
23. **`res.users.group_ids` vs `all_group_ids` for domain filters**: in Odoo 19, `res.users` carries TWO M2M-to-`res.groups` fields and they have different membership semantics. `group_ids` is the user's DIRECTLY-assigned groups (what the user record literally wrote). `all_group_ids` is the TRANSITIVE set — direct groups PLUS every group implied via `implied_ids` chains. **For domain filters on user pickers** (e.g. "show users who can act as a Quality Manager"), ALWAYS use `all_group_ids`, never `group_ids`. An Owner user only carries `group_fp_owner` directly; the QM capability comes via `implied_ids → group_fp_quality_manager`, so a `domain="[('group_ids', 'in', [ref('...quality_manager')])]"` excludes Owners and the picker looks empty. Use `domain="[('all_group_ids', 'in', [ref('...quality_manager'), ref('...owner')])]"` instead. Compute helpers (`@api.depends('group_ids')`) and write vals (`{'group_ids': [(4, gid)]}`) still use `group_ids` because those operate on direct assignments — only domain filters need the transitive set. Bit us 2026-05-24 on the CGP DO + Nadcap Authority pickers on `res.company`. Same gotcha applies to ANY domain that needs "does this user effectively have role X" semantics across user-facing pickers, ACL rules, server actions, and search filters.
|
||||
24. **`mail.template` data files validate templates at PARSE time — only reference CORE-module fields on the target model**: when Odoo loads a `<record model="mail.template">` from XML, it eagerly RENDERS the `subject`/`body_html` once against a sample `object` to validate the inline_template renders cleanly. If the template references a field defined in a DOWNSTREAM module (one that loads AFTER the data-file's home module), the field isn't on the model yet and you get `AttributeError: 'fp.job' object has no attribute 'X'` → `ParseError: Failed to render inline_template template` → module install/upgrade ABORTS. Bit us 2026-05-25 deploying the cert authority templates: `fusion_plating_notifications` loads BEFORE `fusion_plating_jobs` in dep order, and the templates referenced `object.display_wo_name` and `object.part_catalog_id` (both added by `fusion_plating_jobs` via `_inherit`). Even though the columns exist in the DB from previous installs, the Python class hadn't registered the field yet at parse time. **Fix:** mail.template files in upstream modules must only reference fields defined in the SAME module's classes or earlier-loading deps. For `fp.job` references in `fusion_plating_notifications/data/`, that means CORE-only fields: `name`, `partner_id`, `qty_done`, `recipe_id`, `state`, `date_*`, `company_id` — NOT `display_wo_name`, `part_catalog_id`, `customer_spec_id`, `delivery_id`, `portal_job_id` (all jobs-module fields). Same trap for any other cross-module template (`account.move`, `sale.order`, `stock.picking`). **Two structural alternatives** if you really need downstream fields: (a) move the mail.template + fp.notification.template data records into the downstream module so they load after the field is registered (cleanest); (b) compute the value in the calling Python code and pass via `email_values` to the dispatch — no template-time rendering.
|
||||
24. **`env.get('model.name')` returns an EMPTY recordset (falsy), NOT None — never use it as a presence check**: `self.env.get('fp.notification.template')` returns `fp.notification.template()` (empty recordset) when the model IS registered. Empty recordsets are falsy in Python, so `if not Template: return` silently exits even when the model exists and the call should proceed. Same gotcha for `env.get('any.model')` — they all return empty recordsets. **Fix: use the membership check first, then index:**
|
||||
```python
|
||||
if 'fp.notification.template' not in self.env:
|
||||
return # model not installed
|
||||
Template = self.env['fp.notification.template']
|
||||
# now Template is the model class; use it
|
||||
```
|
||||
The `Template.sudo()._some_classmethod()` call works on the empty recordset because `@api.model` methods run on the class. The breakage is purely the truthy-check. Bit us 2026-05-25 deploying `_fp_schedule_cert_activity` — the helper hit `env.get(...)` and immediately returned without ever attempting `activity_schedule`, so the QM never got their Issue-CoC activity. Took a monkey-patch trace through the helper to surface, because the function was silently no-oping with no exception. Same pattern likely scattered in any code that gates on `if env.get(...): ...` — grep for it.
|
||||
|
||||
25. **`mail.template` data files validate templates at PARSE time — only reference CORE-module fields on the target model**: when Odoo loads a `<record model="mail.template">` from XML, it eagerly RENDERS the `subject`/`body_html` once against a sample `object` to validate the inline_template renders cleanly. If the template references a field defined in a DOWNSTREAM module (one that loads AFTER the data-file's home module), the field isn't on the model yet and you get `AttributeError: 'fp.job' object has no attribute 'X'` → `ParseError: Failed to render inline_template template` → module install/upgrade ABORTS. Bit us 2026-05-25 deploying the cert authority templates: `fusion_plating_notifications` loads BEFORE `fusion_plating_jobs` in dep order, and the templates referenced `object.display_wo_name` and `object.part_catalog_id` (both added by `fusion_plating_jobs` via `_inherit`). Even though the columns exist in the DB from previous installs, the Python class hadn't registered the field yet at parse time. **Fix:** mail.template files in upstream modules must only reference fields defined in the SAME module's classes or earlier-loading deps. For `fp.job` references in `fusion_plating_notifications/data/`, that means CORE-only fields: `name`, `partner_id`, `qty_done`, `recipe_id`, `state`, `date_*`, `company_id` — NOT `display_wo_name`, `part_catalog_id`, `customer_spec_id`, `delivery_id`, `portal_job_id` (all jobs-module fields). Same trap for any other cross-module template (`account.move`, `sale.order`, `stock.picking`). **Two structural alternatives** if you really need downstream fields: (a) move the mail.template + fp.notification.template data records into the downstream module so they load after the field is registered (cleanest); (b) compute the value in the calling Python code and pass via `email_values` to the dispatch — no template-time rendering.
|
||||
|
||||
## Naming
|
||||
- **New custom models** (post-2026-04): `fp.*` prefix (e.g. `fp.part.catalog`, `fp.certificate`)
|
||||
|
||||
@@ -2286,15 +2286,19 @@ class FpJob(models.Model):
|
||||
|
||||
The trick: re-uses button_mark_done's gate logic but short-
|
||||
circuits BEFORE the state flip via the fp_check_gates_only
|
||||
context flag (honoured in button_mark_done below).
|
||||
context flag.
|
||||
|
||||
IMPORTANT: pass fp_skip_step_gate=True. At pre-super time the
|
||||
current step is STILL in_progress (we're about to finish it
|
||||
but super().button_finish hasn't fired yet), so button_mark_done's
|
||||
step-completion gate would always fail with "1/1 step is not
|
||||
finished". The step gate is structurally wrong for this caller;
|
||||
the bake/qty/QC gates are not. Bit us on entech smoke test.
|
||||
"""
|
||||
self.ensure_one()
|
||||
# Pass through with context flag; button_mark_done will run all
|
||||
# its gates and then exit before flipping state. The actual
|
||||
# state transition (in_progress → awaiting_cert/ship) is owned
|
||||
# by _fp_check_advance_post_shop running AFTER super().button_finish.
|
||||
self.with_context(
|
||||
fp_check_gates_only=True,
|
||||
fp_skip_step_gate=True,
|
||||
).button_mark_done()
|
||||
|
||||
def _fp_check_advance_post_shop(self):
|
||||
@@ -2335,8 +2339,17 @@ class FpJob(models.Model):
|
||||
|
||||
def _fp_check_advance_after_cert_issue(self):
|
||||
"""Called from fp.certificate.action_issue. If every required
|
||||
cert for this job is now `issued`, advance awaiting_cert →
|
||||
awaiting_ship. Idempotent — safe to call repeatedly.
|
||||
cert TYPE for this job has at least one `issued` cert, advance
|
||||
awaiting_cert → awaiting_ship. Idempotent — safe to call
|
||||
repeatedly.
|
||||
|
||||
Semantics chosen: per-TYPE coverage, not per-CERT exhaustion.
|
||||
A previously-voided cert (state='voided') of the same type is
|
||||
irrelevant — the operator's intent on void was "this attempt
|
||||
is invalid"; a fresh `issued` cert of that type satisfies the
|
||||
requirement. Counting voided certs as outstanding would block
|
||||
the advance after a void+re-issue cycle (caught on entech
|
||||
smoke test 2026-05-25).
|
||||
"""
|
||||
for job in self:
|
||||
if job.state != 'awaiting_cert':
|
||||
@@ -2353,12 +2366,14 @@ class FpJob(models.Model):
|
||||
job._fp_resolve_cert_activities()
|
||||
continue
|
||||
Cert = self.env['fp.certificate'].sudo()
|
||||
outstanding = Cert.search_count([
|
||||
# Per-type coverage: every required cert type must have at
|
||||
# least ONE cert in state=issued. Voided certs are ignored.
|
||||
covered_types = set(Cert.search([
|
||||
('x_fc_job_id', '=', job.id),
|
||||
('certificate_type', 'in', list(required)),
|
||||
('state', '!=', 'issued'),
|
||||
])
|
||||
if outstanding == 0:
|
||||
('state', '=', 'issued'),
|
||||
]).mapped('certificate_type'))
|
||||
if required.issubset(covered_types):
|
||||
job.state = 'awaiting_ship'
|
||||
job._fp_create_delivery()
|
||||
if hasattr(job, '_fp_resolve_cert_activities'):
|
||||
@@ -2366,9 +2381,14 @@ class FpJob(models.Model):
|
||||
|
||||
def _fp_check_regress_after_cert_void(self):
|
||||
"""Called from fp.certificate.write when state=voided. If a
|
||||
previously-issued cert is no longer issued, slide the job back
|
||||
to awaiting_cert so it reappears in Final Inspection and the
|
||||
QM is re-notified.
|
||||
required cert TYPE has lost coverage (no remaining issued
|
||||
cert), slide the job back to awaiting_cert so it reappears in
|
||||
Final Inspection and the QM is re-notified.
|
||||
|
||||
Per-type coverage (mirror of _fp_check_advance_after_cert_issue):
|
||||
voiding ONE cert only regresses if it was the only issued cert
|
||||
of its type. If a sibling issued cert still covers the type,
|
||||
coverage holds and no regress fires.
|
||||
"""
|
||||
for job in self:
|
||||
if job.state != 'awaiting_ship':
|
||||
@@ -2379,12 +2399,12 @@ class FpJob(models.Model):
|
||||
if not required:
|
||||
continue
|
||||
Cert = self.env['fp.certificate'].sudo()
|
||||
outstanding = Cert.search_count([
|
||||
covered_types = set(Cert.search([
|
||||
('x_fc_job_id', '=', job.id),
|
||||
('certificate_type', 'in', list(required)),
|
||||
('state', '!=', 'issued'),
|
||||
])
|
||||
if outstanding > 0:
|
||||
('state', '=', 'issued'),
|
||||
]).mapped('certificate_type'))
|
||||
if not required.issubset(covered_types):
|
||||
job.state = 'awaiting_cert'
|
||||
job._fp_fire_notification('cert_voided_re_notify')
|
||||
if hasattr(job, '_fp_schedule_cert_activity'):
|
||||
@@ -2411,9 +2431,15 @@ class FpJob(models.Model):
|
||||
)
|
||||
if existing:
|
||||
return
|
||||
Template = self.env.get('fp.notification.template')
|
||||
if not Template or not hasattr(
|
||||
Template, '_fp_resolve_cert_authority_users'):
|
||||
# env.get('model.name') returns an EMPTY recordset when the
|
||||
# model exists but has no records — empty recordsets are
|
||||
# falsy in Python, so `if not Template: return` exits early
|
||||
# even when the model IS registered. Use the membership check
|
||||
# instead. Bit us on entech smoke test 2026-05-25.
|
||||
if 'fp.notification.template' not in self.env:
|
||||
return
|
||||
Template = self.env['fp.notification.template']
|
||||
if not hasattr(Template, '_fp_resolve_cert_authority_users'):
|
||||
return
|
||||
qms = Template.sudo()._fp_resolve_cert_authority_users(self)
|
||||
if not qms:
|
||||
|
||||
Reference in New Issue
Block a user