admin/Odoo-Modules
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(configurator/bridge_mrp): address all bugs from code review

Browse Source 
Two critical, one important, four polish fixes found by the
pr-review-toolkit code-reviewer.

C1 (CRITICAL) Start-at-node filter dropped later siblings
  fusion_plating_bridge_mrp/models/mrp_production.py:448
  The allowed_ids set was {descendants} ∪ {ancestors}, which wrongly
  excluded nodes that should run AFTER the start node — including
  later siblings of the start node and all operations in subsequent
  sub-processes. Rewrote the upward walk to ALSO include each
  ancestor's later-sequence siblings and their descendants. Smoke on
  ENP-ALUM-BASIC: full=9 WOs, partial from mid-tree 'De-Masking'=5
  WOs (previously was 1).

C2 (CRITICAL) Duplicate MO on re-confirm of pre-PR SOs
  fusion_plating_bridge_mrp/models/sale_order.py:96
  Legacy untagged MOs (created before this PR had line-linkage m2m)
  were not recognized by the untagged idempotency check, so
  re-confirming an already-processed SO would create one additional
  MO per untagged plating line. Fix: pre-scan for a single legacy
  untagged MO and adopt it by linking ALL untagged plating lines
  onto it. Those lines are then treated as covered and no per-line
  MOs are created on top. Smoke: S00066 before=1 MO, after
  re-run=1 MO.

I5 (IMPORTANT) push_to_defaults wrote to pre-bump revision
  fusion_plating_configurator/wizard/fp_direct_order_wizard.py:236
  When create_new_revision=True, _get_or_bump_revision() returned a
  new part record that got written to the SO line, but the
  post-confirm push_to_defaults loop re-read line.part_catalog_id
  (still the OLD rev) and wrote defaults there, defeating the whole
  point of "save as default". Fix: cache resolved parts in a dict
  keyed by wizard-line ID during the build loop, and use that cache
  in the push_to_defaults pass.

I3/I4/I6 (PERF) Computes lacked @api.depends and did per-record
  search_count / search queries
  fusion_plating_configurator/models/sale_order.py
  _compute_nav_counts, _compute_workorder_count, _compute_wo_completion
  now:
  - declare @api.depends
  - batch via read_group across the whole self recordset
  - rebuild {origin: counts} dicts and assign per record

M7 (MEDIUM) No savepoint around per-group MO creation
  fusion_plating_bridge_mrp/models/sale_order.py:_fp_auto_create_mo
  A mid-loop exception left group 1's MO persisted and aborted
  groups 2..N. Wrapped each group's create in SAVEPOINT/RELEASE/
  ROLLBACK TO SAVEPOINT so one bad group no longer corrupts state.

M8 (MEDIUM) Email 'opened' status false-positived on internal CC
  fusion_plating_configurator/models/sale_order.py:_compute_email_status
  Switched from 'any notification is_read' to 'customer partner has
  a read email notification on this SO'.

M9 (LOW) start_at_node_id domain silently empty when coating unset
  fusion_plating_configurator/wizard/fp_direct_order_line.py:94
  Changed `('parent_id', 'child_of', ...)` to
  `('id', 'child_of', ..., or 0)` and clarified the help text.

Regression smoke passed all checks on odoo-entech.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsinghpal
2026-04-19 23:35:03 -04:00
parent 9d8db0f9b1
commit b15bf2293e
5 changed files with 277 additions and 142 deletions
Download Patch File Download Diff File Expand all files Collapse all files

43
fusion_plating/fusion_plating_bridge_mrp/models/mrp_production.py
View File

@@ -440,25 +440,36 @@ class MrpProduction(models.Model):
for override in production.x_fc_override_ids: for override in production.x_fc_override_ids:
override_map[override.node_id.id] = override.included override_map[override.node_id.id] = override.included
# Start-at-node: if set, build the set of node IDs that are # Start-at-node: if set, the allowed set is the union of:
# "at or descended from" the start node OR on its ancestor # 1. start_node and all its descendants (we run these)
# path (so we keep the containing recipe / sub-processes # 2. each ancestor of start_node (to preserve the container
# visible but skip sibling branches that come before the # hierarchy the recipe walker uses to reach start_node)
# start point). # 3. at each ancestor level, any LATER-sequence sibling and
# all of its descendants (these come after start_node
# in the flow and must still run)
# Earlier siblings at each level are implicitly skipped.
start_node = production.x_fc_start_at_node_id start_node = production.x_fc_start_at_node_id
allowed_ids = None # None = include everything allowed_ids = None # None = include everything
if start_node: if start_node:
# Descendants (inclusive) Node = self.env['fusion.plating.process.node']
descendants = self.env['fusion.plating.process.node'].search([ # 1. Descendants of start_node (inclusive)
('id', 'child_of', start_node.id), descendants = Node.search([('id', 'child_of', start_node.id)])
]) allowed_ids = set(descendants.ids)
# Ancestors (excluding self — already in descendants) # 2+3. Walk up; at each level add the parent and the
ancestors = self.env['fusion.plating.process.node'] # later-sibling subtrees.
cur = start_node.parent_id cur = start_node
while cur: while cur.parent_id:
ancestors |= cur parent = cur.parent_id
cur = cur.parent_id allowed_ids.add(parent.id)
allowed_ids = set(descendants.ids) | set(ancestors.ids) later_sibs = parent.child_ids.filtered(
lambda n: n.sequence > cur.sequence
)
for sib in later_sibs:
sib_descendants = Node.search([
('id', 'child_of', sib.id),
])
allowed_ids |= set(sib_descendants.ids)
cur = parent
# Bind the source SO once per production so walk_node closure # Bind the source SO once per production so walk_node closure
# can read coating config / spec without an extra search per WO. # can read coating config / spec without an extra search per WO.

201
fusion_plating/fusion_plating_bridge_mrp/models/sale_order.py
View File

@@ -110,9 +110,15 @@ class SaleOrder(models.Model):
""" """
self.ensure_one() self.ensure_one()
Production = self.env['mrp.production'] Production = self.env['mrp.production']
existing_tags = set(Production.search([ existing_mos = Production.search([('origin', '=', self.name)])
('origin', '=', self.name), existing_tags = set(existing_mos.mapped('x_fc_wo_group_tag'))
]).mapped('x_fc_wo_group_tag')) # Legacy MOs = untagged MOs created before this PR that never
# had x_fc_sale_order_line_ids populated. We adopt them 1-for-1
# onto the first N untagged groups so re-confirm doesn't
# double-book.
legacy_untagged = existing_mos.filtered(
lambda m: not m.x_fc_wo_group_tag and not m.x_fc_sale_order_line_ids
)
# Build groups from SO lines that carry plating data # Build groups from SO lines that carry plating data
plating_lines = self.order_line.filtered( plating_lines = self.order_line.filtered(
@@ -121,94 +127,141 @@ class SaleOrder(models.Model):
if not plating_lines: if not plating_lines:
return self._fp_auto_create_mo_legacy() return self._fp_auto_create_mo_legacy()
groups = {} # {tag_or_line_key: [lines]}
for line in plating_lines:
key = line.x_fc_wo_group_tag or ('__line__%d' % line.id)
groups.setdefault(key, []).append(line)
created = [] created = []
adopted = []
# If a legacy untagged MO already exists for this SO, it
# represents the pre-PR "one MO for the whole order" work.
# Adopt it by linking EVERY untagged plating line to it, and
# treat those lines as covered — don't create per-line MOs on
# top of the legacy MO.
untagged_lines = plating_lines.filtered(lambda l: not l.x_fc_wo_group_tag)
tagged_lines = plating_lines - untagged_lines
covered_untagged_ids = set()
if legacy_untagged and untagged_lines:
legacy = legacy_untagged[0]
legacy.write({
'x_fc_sale_order_line_ids': [(4, ln.id) for ln in untagged_lines],
})
adopted.append(legacy)
covered_untagged_ids = set(untagged_lines.ids)
groups = {} # {tag_or_line_key: [lines]}
for line in tagged_lines:
groups.setdefault(line.x_fc_wo_group_tag, []).append(line)
for line in untagged_lines:
if line.id in covered_untagged_ids:
continue # already adopted onto legacy MO
groups['__line__%d' % line.id] = [line]
for key, lines in groups.items(): for key, lines in groups.items():
tag = lines[0].x_fc_wo_group_tag or False tag = lines[0].x_fc_wo_group_tag or False
# Skip if we already have an MO for this (origin, tag) pair. # Skip if we already have an MO for this (origin, tag) pair.
# Untagged keys are 1:1 with lines; use the line ID in sudo
# check via existing MOs' line links.
if tag and tag in existing_tags: if tag and tag in existing_tags:
continue continue
if not tag: if not tag:
# Untagged idempotency — check if any existing MO points # Untagged link-based idempotency (rerun protection)
# at this line via x_fc_sale_order_line_ids.
if Production.search_count([ if Production.search_count([
('origin', '=', self.name), ('origin', '=', self.name),
('x_fc_sale_order_line_ids', 'in', [lines[0].id]), ('x_fc_sale_order_line_ids', 'in', [lines[0].id]),
]): ]):
continue continue
# Resolve product: part catalog's linked product if any, else # Per-group savepoint so one broken group can't block later
# FP-WIDGET fallback. # ones AND can't leave partial state committed.
product = False savepoint_name = 'fp_mo_group_%s' % abs(hash(key))
for ln in lines: self.env.cr.execute('SAVEPOINT %s' % savepoint_name)
pc = ln.x_fc_part_catalog_id try:
if pc and 'product_id' in pc._fields and pc.product_id: # Resolve product: part catalog's linked product if any,
product = pc.product_id # else FP-WIDGET fallback.
break product = False
if not product: for ln in lines:
product = self.env['product.product'].search( pc = ln.x_fc_part_catalog_id
[('default_code', '=', 'FP-WIDGET')], limit=1, if pc and 'product_id' in pc._fields and pc.product_id:
) product = pc.product_id
if not product: break
if not product:
product = self.env['product.product'].search(
[('default_code', '=', 'FP-WIDGET')], limit=1,
)
if not product:
self.env.cr.execute('RELEASE SAVEPOINT %s' % savepoint_name)
self.message_post(body=_(
'Auto-MO skipped (group %s) — no manufacturable '
'product available.'
) % (tag or 'single-line'))
continue
# Recipe: first line's coating -> recipe_id.
recipe = False
for ln in lines:
cc = ln.x_fc_coating_config_id
if cc and 'recipe_id' in cc._fields and cc.recipe_id:
recipe = cc.recipe_id
break
if not recipe:
recipe = self.env['fusion.plating.process.node'].search(
[('node_type', '=', 'recipe')], limit=1,
)
qty = sum(ln.product_uom_qty for ln in lines) or 1
# Start-at-node: first non-blank wins
start_node = False
for ln in lines:
if ln.x_fc_start_at_node_id:
start_node = ln.x_fc_start_at_node_id
break
mo_vals = {
'product_id': product.id,
'product_qty': qty,
'product_uom_id': product.uom_id.id,
'origin': self.name,
'x_fc_wo_group_tag': tag or False,
'x_fc_sale_order_line_ids': [(6, 0, [ln.id for ln in lines])],
}
if recipe and 'x_fc_recipe_id' in Production._fields:
mo_vals['x_fc_recipe_id'] = recipe.id
if start_node:
mo_vals['x_fc_start_at_node_id'] = start_node.id
mo = Production.create(mo_vals)
created.append((mo, tag, len(lines)))
self.env.cr.execute('RELEASE SAVEPOINT %s' % savepoint_name)
except Exception as exc:
self.env.cr.execute('ROLLBACK TO SAVEPOINT %s' % savepoint_name)
self.message_post(body=_( self.message_post(body=_(
'Auto-MO skipped (group %s) — no manufacturable ' 'Auto-MO group %s failed: %s'
'product available.' ) % (tag or 'single-line', exc))
) % (tag or 'single-line'))
continue continue
# Recipe: first line's coating -> recipe_id. if created or adopted:
recipe = False msg_parts = []
for ln in lines: if created:
cc = ln.x_fc_coating_config_id lines_html = '<br/>'.join([
if cc and 'recipe_id' in cc._fields and cc.recipe_id: _('MO <a href="/odoo/manufacturing/%s">%s</a> '
recipe = cc.recipe_id '(%s, %d source line%s)') % (
break mo.id, mo.name, tag or 'untagged',
if not recipe: n, 's' if n != 1 else ''
recipe = self.env['fusion.plating.process.node'].search( )
[('node_type', '=', 'recipe')], limit=1, for mo, tag, n in created
])
msg_parts.append(
_('%d draft MO(s) auto-created:<br/>%s') % (
len(created), lines_html,
)
) )
if adopted:
qty = sum(ln.product_uom_qty for ln in lines) or 1 adopted_html = '<br/>'.join([
# Start-at-node: first non-blank wins _('MO <a href="/odoo/manufacturing/%s">%s</a> '
start_node = False '(legacy, now line-linked)') % (mo.id, mo.name)
for ln in lines: for mo in adopted
if ln.x_fc_start_at_node_id: ])
start_node = ln.x_fc_start_at_node_id msg_parts.append(
break _('%d legacy MO(s) adopted:<br/>%s') % (
len(adopted), adopted_html,
mo_vals = { )
'product_id': product.id, )
'product_qty': qty, self.message_post(body=Markup('<br/><br/>'.join(msg_parts)))
'product_uom_id': product.uom_id.id,
'origin': self.name,
'x_fc_wo_group_tag': tag or False,
'x_fc_sale_order_line_ids': [(6, 0, [ln.id for ln in lines])],
}
if recipe and 'x_fc_recipe_id' in Production._fields:
mo_vals['x_fc_recipe_id'] = recipe.id
if start_node:
mo_vals['x_fc_start_at_node_id'] = start_node.id
mo = Production.create(mo_vals)
created.append((mo, tag, len(lines)))
if created:
lines_html = '<br/>'.join([
_('MO <a href="/odoo/manufacturing/%s">%s</a> '
'(%s, %d source line%s)') % (
mo.id, mo.name, tag or 'untagged',
n, 's' if n != 1 else ''
)
for mo, tag, n in created
])
self.message_post(body=Markup(_(
'%d draft manufacturing order(s) auto-created:<br/>%s'
)) % (len(created), lines_html))
def _fp_auto_create_mo_legacy(self): def _fp_auto_create_mo_legacy(self):
"""Fallback for SOs with no plating order_line data (service lines). """Fallback for SOs with no plating order_line data (service lines).

151
fusion_plating/fusion_plating_configurator/models/sale_order.py
View File

@@ -131,18 +131,44 @@ class SaleOrder(models.Model):
currency_field='currency_id', currency_field='currency_id',
) )
@api.depends('name')
def _compute_wo_completion(self): def _compute_wo_completion(self):
"""Batched: one grouped query across all records in self."""
for rec in self:
rec.x_fc_wo_completion = '0/0'
names = [so.name for so in self if so.name]
if not names:
return
WO = self.env['mrp.workorder'].sudo() WO = self.env['mrp.workorder'].sudo()
rows = WO.read_group(
[('production_id.origin', 'in', names)],
['production_id.origin', 'state'],
['production_id', 'state'],
lazy=False,
)
# Build {origin: {'done': n, 'total': n}}
# read_group returns production_id as (id, name) tuples; we need
# to translate back to origin. Do a small lookup.
mos = self.env['mrp.production'].sudo().search(
[('origin', 'in', names)]
)
mo_to_origin = {m.id: m.origin for m in mos}
totals = {} # {origin: [total, done]}
for r in rows:
mo_id = r['production_id'][0] if r['production_id'] else False
origin = mo_to_origin.get(mo_id)
if not origin:
continue
cnt = r['__count']
bucket = totals.setdefault(origin, [0, 0])
bucket[0] += cnt
if r['state'] == 'done':
bucket[1] += cnt
for rec in self: for rec in self:
if not rec.name: if not rec.name:
rec.x_fc_wo_completion = '0/0'
continue continue
total = WO.search_count([('production_id.origin', '=', rec.name)]) tot, done = totals.get(rec.name, [0, 0])
done = WO.search_count([ rec.x_fc_wo_completion = '%d/%d' % (done, tot) if tot else '0/0'
('production_id.origin', '=', rec.name),
('state', '=', 'done'),
])
rec.x_fc_wo_completion = '%d/%d' % (done, total) if total else '0/0'
# ---- Phase F: quotes list view polish ---- # ---- Phase F: quotes list view polish ----
x_fc_follow_up_date = fields.Date( x_fc_follow_up_date = fields.Date(
@@ -195,10 +221,14 @@ class SaleOrder(models.Model):
def _compute_email_status(self): def _compute_email_status(self):
"""Map state + mail tracking to a single visible pill. """Map state + mail tracking to a single visible pill.
- draft SO with no tracked email sent => draft - state draft => draft
- sent (Odoo state) => sent - state sent => sent (or 'opened' if the customer partner has
- sent + mail opened => opened (detected via mail.message) a read notification for any email message on this SO)
- state=sale/done => won - state sale / done => won
'Opened' is scoped to the CUSTOMER partner's notifications —
not internal CCs — to avoid false positives from sales-ops
viewing the thread.
""" """
for rec in self: for rec in self:
if rec.state in ('sale', 'done'): if rec.state in ('sale', 'done'):
@@ -209,19 +239,17 @@ class SaleOrder(models.Model):
continue continue
# state == 'sent' # state == 'sent'
opened = False opened = False
if rec.id: if rec.id and rec.partner_id:
msgs = self.env['mail.message'].sudo().search([ # Look for any read notification on any email message
('model', '=', 'sale.order'), # of this SO that targeted the customer.
('res_id', '=', rec.id), notif_count = self.env['mail.notification'].sudo().search_count([
('message_type', '=', 'email'), ('mail_message_id.model', '=', 'sale.order'),
], limit=10) ('mail_message_id.res_id', '=', rec.id),
# mail.notification tracks read timestamps ('mail_message_id.message_type', '=', 'email'),
for m in msgs: ('res_partner_id', '=', rec.partner_id.id),
if m.notification_ids.filtered( ('is_read', '=', True),
lambda n: n.is_read ])
): opened = notif_count > 0
opened = True
break
rec.x_fc_email_status = 'opened' if opened else 'sent' rec.x_fc_email_status = 'opened' if opened else 'sent'
@api.depends('order_line.x_fc_part_catalog_id.part_number') @api.depends('order_line.x_fc_part_catalog_id.part_number')
@@ -254,16 +282,33 @@ class SaleOrder(models.Model):
- sum(refunds.mapped('amount_total')) - sum(refunds.mapped('amount_total'))
) )
@api.depends('name')
def _compute_workorder_count(self): def _compute_workorder_count(self):
WO = self.env['mrp.workorder'].sudo()
for rec in self: for rec in self:
if not rec.name: rec.x_fc_workorder_count = 0
rec.x_fc_workorder_count = 0 names = [so.name for so in self if so.name]
continue if not names:
rec.x_fc_workorder_count = WO.search_count([ return
('production_id.origin', '=', rec.name), WO = self.env['mrp.workorder'].sudo()
('state', 'not in', ('done', 'cancel')), rows = WO.read_group(
]) [('production_id.origin', 'in', names),
('state', 'not in', ('done', 'cancel'))],
['production_id'],
['production_id'],
lazy=False,
)
mos = self.env['mrp.production'].sudo().search(
[('origin', 'in', names)]
)
mo_to_origin = {m.id: m.origin for m in mos}
totals = {}
for r in rows:
mo_id = r['production_id'][0] if r['production_id'] else False
origin = mo_to_origin.get(mo_id)
if origin:
totals[origin] = totals.get(origin, 0) + r['__count']
for rec in self:
rec.x_fc_workorder_count = totals.get(rec.name, 0)
def action_view_workorders(self): def action_view_workorders(self):
self.ensure_one() self.ensure_one()
@@ -290,21 +335,41 @@ class SaleOrder(models.Model):
string='Files', compute='_compute_nav_counts', string='Files', compute='_compute_nav_counts',
) )
@api.depends('invoice_ids', 'picking_ids')
def _compute_nav_counts(self): def _compute_nav_counts(self):
NCR = self.env.get('fusion.plating.ncr') # Invoice + picking counts are cheap (related collections).
for rec in self: for rec in self:
rec.x_fc_invoice_count = len(rec.invoice_ids) rec.x_fc_invoice_count = len(rec.invoice_ids)
rec.x_fc_picking_count = len(rec.picking_ids) rec.x_fc_picking_count = len(rec.picking_ids)
rec.x_fc_attachment_count = self.env['ir.attachment'].sudo().search_count([
('res_model', '=', 'sale.order'), # Attachment counts — batched read_group.
('res_id', '=', rec.id), ids = self.ids
]) att_counts = {}
if NCR and 'sale_order_id' in NCR._fields: if ids:
rec.x_fc_ncr_count = NCR.sudo().search_count([ rows = self.env['ir.attachment'].sudo().read_group(
('sale_order_id', '=', rec.id), [('res_model', '=', 'sale.order'),
]) ('res_id', 'in', ids)],
else: ['res_id'], ['res_id'], lazy=False,
rec.x_fc_ncr_count = 0 )
att_counts = {r['res_id']: r['__count'] for r in rows}
for rec in self:
rec.x_fc_attachment_count = att_counts.get(rec.id, 0)
# NCR counts — only if the module is installed.
NCR = self.env.get('fusion.plating.ncr')
ncr_counts = {}
if ids and NCR is not None and 'sale_order_id' in NCR._fields:
rows = NCR.sudo().read_group(
[('sale_order_id', 'in', ids)],
['sale_order_id'], ['sale_order_id'], lazy=False,
)
ncr_counts = {
(r['sale_order_id'][0] if r['sale_order_id'] else False):
r['__count']
for r in rows
}
for rec in self:
rec.x_fc_ncr_count = ncr_counts.get(rec.id, 0)
def action_view_invoices(self): def action_view_invoices(self):
self.ensure_one() self.ensure_one()

6
fusion_plating/fusion_plating_configurator/wizard/fp_direct_order_line.py
View File

@@ -94,9 +94,11 @@ class FpDirectOrderLine(models.TransientModel):
start_at_node_id = fields.Many2one( start_at_node_id = fields.Many2one(
'fusion.plating.process.node', 'fusion.plating.process.node',
string='Start at Node', string='Start at Node',
domain="[('parent_id', 'child_of', coating_config_id and coating_config_id.recipe_id.id)]", domain="[('id', 'child_of', coating_config_id and coating_config_id.recipe_id.id or 0)]",
help='For re-work jobs: pick the recipe step where this job should ' help='For re-work jobs: pick the recipe step where this job should '
'begin. Skips ancestor steps in the generated work order.', 'begin. Pick a coating first — nodes are scoped to its '
'recipe tree. Skips earlier steps in the generated WO but '
'keeps later siblings and sub-processes.',
) )
is_one_off = fields.Boolean( is_one_off = fields.Boolean(
string='One-off Part', string='One-off Part',

18
fusion_plating/fusion_plating_configurator/wizard/fp_direct_order_wizard.py
View File

@@ -235,9 +235,13 @@ class FpDirectOrderWizard(models.TransientModel):
'order_line': [], 'order_line': [],
} }
# 4. One SO line per wizard line # 4. One SO line per wizard line. Cache resolved parts (post
# rev-bump) so the push-to-defaults pass writes to the right
# catalog entry.
resolved_parts = {} # {wizard_line_id: resolved part record}
for line in self.line_ids: for line in self.line_ids:
part = line._get_or_bump_revision() part = line._get_or_bump_revision()
resolved_parts[line.id] = part
header = '%s - %s Rev %s (x%d)' % ( header = '%s - %s Rev %s (x%d)' % (
line.coating_config_id.name, line.coating_config_id.name,
part.name, part.name,
@@ -270,14 +274,14 @@ class FpDirectOrderWizard(models.TransientModel):
so = self.env['sale.order'].create(so_vals) so = self.env['sale.order'].create(so_vals)
so.action_confirm() so.action_confirm()
# 6. Push-to-defaults (C4) — after the part has been resolved / # 6. Push-to-defaults (C4) — uses the resolved part cached
# revision-bumped, write coating + treatments back onto the part # during the build loop so rev-bumped lines write defaults to
# catalog entry so the next order inherits the same defaults. # the NEW revision, not the pre-bump one.
for line in self.line_ids: for line in self.line_ids:
if not line.push_to_defaults: if not line.push_to_defaults or line.is_one_off:
continue continue
part = line.part_catalog_id part = resolved_parts.get(line.id) or line.part_catalog_id
if not part or line.is_one_off: if not part:
continue continue
part.write({ part.write({
'x_fc_default_coating_config_id': line.coating_config_id.id or False, 'x_fc_default_coating_config_id': line.coating_config_id.id or False,