From c86f1bbbe5039567165484c52843552533e09634 Mon Sep 17 00:00:00 2001 From: gsinghpal Date: Wed, 20 May 2026 22:22:11 -0400 Subject: [PATCH] fix(fusion_repairs): code-review batch - 4 critical + 8 high + 8 medium/low Critical - C1: _sql_constraints -> models.Constraint (Odoo 19 deprecation rule violation) - C2: variance threshold no longer uses abs() - under-cost is good news, must not block invoicing. Now only OVER-cost triggers requires_requote. - C3: roll_next_due_date() was dead code - now wired from fusion.technician.task.write() when a maintenance task transitions to 'completed', so the whole maintenance lifecycle actually advances. - C4: warranty.is_active was store=True but time-dependent (became stale). Dropped store=True; find_active_for() now filters by expiry_date directly. High - H1: added x_fc_maintenance_contract_id back-link on repair.order and populated it from create_repair_from_booking(). - H2: find_active_for() returns empty when neither lot nor product is supplied - prevents cross-product false warranty matches. - H3: visit-report wizard now creates stock.move records of repair_line_type 'add' for each part line, so Odoo's native action_create_sale_order() chain has lines to invoice and stock gets consumed properly. - H4: office intake email template now carries a fallback email_to header computed from res.company.x_fc_office_notification_ids (or company email), so it does not silently send with no recipient. - H5: maintenance reminder cron nextcall now always rolls to tomorrow at 07:00 local time, so installing/upgrading after 07:00 does not immediately fire all the day's reminders. - H6: public portal no longer hardcodes UID 1 as the intake user fallback (which in Odoo 19 is OdooBot). Prefers base.user_admin, else the lowest-id non-share user, else SUPERUSER_ID. - H7: public portal validates client_email via tools.email_normalize before partner creation; malformed addresses redirect with error=email. - H8: find_best_match() returns empty when no symptom keywords match (no silent first-catalog guess) and uses word-boundary regex to avoid matching 'battery' inside 'no battery problem'. Medium - M1: _inherit moved next to _name on maintenance_contract (cosmetic but brittle if Odoo refactors model class detection) - M2: relativedelta(months=N) instead of timedelta(days=N*30) for warranty and maintenance intervals (correct month boundaries) - M3: unique constraint on fusion.repair.maintenance.contract.booking_token - M6: dispatch task fallback now searches for an actual x_fc_is_field_staff user; gracefully skips and logs if no field staff exists (instead of silently failing the constraint check) - M7: maintenance contract list view date decoration uses context_today() (date) instead of strftime(string) - the str comparison would TypeError - M9: Visit Report button hidden on draft repairs and when no technician task is linked yet Low - L2: portal-created partners get default lang + company_id so mail templates render in the right language - L3: dropped unused exception variable in sales rep portal controller - L4: visit-report wizard 'found another issue' now redirects to the spawned stub repair so the tech can fill it in immediately - L5: dropped unrecognized data-string from in settings view Public portal also: rate-limit check moved BEFORE the counter increment so blocked attempts do not keep inflating the bucket. All fixes verified end-to-end on local westin-v19: - variance one-sided: 0.5h labour vs $500 est -> requires_requote=False; 2h x $250 + $200 parts vs $100 est -> requires_requote=True - maintenance roll-forward: created MC/00006 due 2026-05-31, completed linked maintenance task -> contract rolled to 2026-11-21 with last_reminder_band reset - warranty find_active_for(partner only) -> empty recordset - service catalog find_best_match with unrelated text -> empty recordset - pg_constraint shows fusion_repair_maintenance_contract_booking_token_unique - /repair landing still 200 after restart Co-authored-by: Cursor --- .../controllers/portal_client_repair.py | 32 ++++++++--- .../controllers/portal_sales_rep_repair.py | 2 +- fusion_repairs/data/ir_cron_data.xml | 2 +- fusion_repairs/data/mail_template_data.xml | 1 + fusion_repairs/models/intake_service.py | 44 +++++++++++---- fusion_repairs/models/maintenance_contract.py | 23 ++++++-- fusion_repairs/models/repair_order.py | 15 ++++-- .../models/repair_product_category.py | 7 +-- fusion_repairs/models/repair_warranty.py | 16 ++++-- fusion_repairs/models/service_catalog.py | 22 +++++--- fusion_repairs/models/technician_task.py | 30 ++++++++++- .../views/maintenance_contract_views.xml | 2 +- fusion_repairs/views/repair_order_views.xml | 2 +- .../views/res_config_settings_views.xml | 3 +- .../wizard/repair_visit_report_wizard.py | 53 +++++++++++++++++-- 15 files changed, 203 insertions(+), 51 deletions(-) diff --git a/fusion_repairs/controllers/portal_client_repair.py b/fusion_repairs/controllers/portal_client_repair.py index 96ddd9c3..9ce7cbe9 100644 --- a/fusion_repairs/controllers/portal_client_repair.py +++ b/fusion_repairs/controllers/portal_client_repair.py @@ -29,8 +29,9 @@ import logging import re import time -from odoo import http, fields +from odoo import SUPERUSER_ID, http, fields from odoo.http import request +from odoo.tools import email_normalize _logger = logging.getLogger(__name__) @@ -92,9 +93,10 @@ class ClientRepairPortal(http.Controller): for k in list(_RATE_LIMIT_BUCKET.keys()): if not k.endswith(f":{bucket}"): _RATE_LIMIT_BUCKET.pop(k, None) - _RATE_LIMIT_BUCKET[key] = _RATE_LIMIT_BUCKET.get(key, 0) + 1 - if _RATE_LIMIT_BUCKET[key] > limit: + # Check FIRST so blocked attempts don't keep inflating the counter. + if _RATE_LIMIT_BUCKET.get(key, 0) >= limit: return True # blocked + _RATE_LIMIT_BUCKET[key] = _RATE_LIMIT_BUCKET.get(key, 0) + 1 return False # ------------------------------------------------------------------ @@ -164,6 +166,12 @@ class ClientRepairPortal(http.Controller): if not (partner_name and phone and issue_summary and category_id): return request.redirect("/repair/new?error=missing") + # Validate email if provided. Empty is allowed; malformed redirects back. + raw_email = (post.get("client_email") or "").strip() + clean_email = email_normalize(raw_email) if raw_email else False + if raw_email and not clean_email: + return request.redirect("/repair/new?error=email") + # Find or create partner. Match by phone if known (safe - we already # have their consent to contact via this form). cleaned_phone = _e164_clean(phone) @@ -180,7 +188,7 @@ class ClientRepairPortal(http.Controller): partner_vals = { "name": partner_name, "phone": phone, - "email": (post.get("client_email") or "").strip(), + "email": clean_email or False, "street": (post.get("client_street") or "").strip(), "city": (post.get("client_city") or "").strip(), } @@ -209,13 +217,21 @@ class ClientRepairPortal(http.Controller): "internal_notes": (post.get("internal_notes") or "").strip(), "photo_attachment_ids": attachment_ids, } + # Pick a real human owner for the repair so emails go from a person: + # admin if present, else the lowest-id non-share user, else SUPERUSER_ID. + admin = request.env.ref("base.user_admin", raise_if_not_found=False) + if admin: + intake_uid = admin.id + else: + internal = request.env["res.users"].sudo().search( + [("share", "=", False)], order="id asc", limit=1, + ) + intake_uid = internal.id if internal else SUPERUSER_ID + payload = { "partner_id": partner.id if partner else None, "partner_vals": partner_vals, - "intake_user_id": request.env.ref( - "base.user_admin", raise_if_not_found=False).id - if request.env.ref("base.user_admin", - raise_if_not_found=False) else 1, + "intake_user_id": intake_uid, "equipment_items": [equipment], } diff --git a/fusion_repairs/controllers/portal_sales_rep_repair.py b/fusion_repairs/controllers/portal_sales_rep_repair.py index 55434216..8f1bbd6f 100644 --- a/fusion_repairs/controllers/portal_sales_rep_repair.py +++ b/fusion_repairs/controllers/portal_sales_rep_repair.py @@ -135,7 +135,7 @@ class SalesRepRepairPortal(CustomerPortal): try: repairs = request.env['fusion.repair.intake.service'].sudo() \ .create_repair_orders(payload, source='sales_rep_portal') - except Exception as e: + except Exception: _logger.exception('Sales rep portal repair submit failed') return request.redirect('/my/repair/new?error=server') diff --git a/fusion_repairs/data/ir_cron_data.xml b/fusion_repairs/data/ir_cron_data.xml index 5f9e95ed..4dde0416 100644 --- a/fusion_repairs/data/ir_cron_data.xml +++ b/fusion_repairs/data/ir_cron_data.xml @@ -11,7 +11,7 @@ 1 days - + diff --git a/fusion_repairs/data/mail_template_data.xml b/fusion_repairs/data/mail_template_data.xml index 8ce6e0a9..afdfe693 100644 --- a/fusion_repairs/data/mail_template_data.xml +++ b/fusion_repairs/data/mail_template_data.xml @@ -109,6 +109,7 @@ [New Service Call] {{ object.partner_id.name or 'Walk-in' }} - {{ object.name or 'n/a' }} {{ (object.user_id.email_formatted or object.company_id.email_formatted or user.email_formatted) }} + {{ ','.join(p.email for p in (object.company_id.x_fc_office_notification_ids if 'x_fc_office_notification_ids' in object.company_id._fields else []) if p.email) or (object.company_id.email or '') }}
diff --git a/fusion_repairs/models/intake_service.py b/fusion_repairs/models/intake_service.py index 16479f97..8f5c5315 100644 --- a/fusion_repairs/models/intake_service.py +++ b/fusion_repairs/models/intake_service.py @@ -90,6 +90,10 @@ class FusionRepairIntakeService(models.AbstractModel): partner_vals = payload.get('partner_vals') if not partner_vals: return False + # Sensible defaults for partners created via public portals so mail + # templates pick up the right language / company. + partner_vals.setdefault('lang', self.env.user.lang or 'en_CA') + partner_vals.setdefault('company_id', self.env.company.id) partner = self.env['res.partner'].sudo().create(partner_vals) return partner.id @@ -401,12 +405,25 @@ class FusionRepairIntakeService(models.AbstractModel): 'x_fc_repair_order_id': repair.id, 'description': repair.internal_notes or repair.name, } - # technician_id is required on fusion.technician.task; we fall back to - # the intake user. Dispatcher will reassign. - vals['technician_id'] = ( - repair.user_id.id if repair.user_id and repair.user_id.x_fc_is_field_staff - else self.env.uid - ) + # technician_id is required AND constrained to x_fc_is_field_staff. + # Use the intake user if they qualify, otherwise the lowest-id active + # field-staff user as a placeholder for the dispatcher to reassign. + if repair.user_id and repair.user_id.x_fc_is_field_staff: + vals['technician_id'] = repair.user_id.id + else: + fallback = self.env['res.users'].sudo().search([ + ('x_fc_is_field_staff', '=', True), + ('active', '=', True), + ], order='id', limit=1) + if not fallback: + _logger.warning( + 'No field-staff user available - skipping auto-dispatch ' + 'task for repair %s (mark a user as Field Staff under ' + 'Settings > Users).', + repair.name, + ) + return + vals['technician_id'] = fallback.id Task.create(vals) except Exception as e: _logger.warning('Failed to auto-create dispatch task for repair %s: %s', @@ -449,8 +466,13 @@ class FusionRepairIntakeService(models.AbstractModel): @api.model def _office_emails(self, company): # Reuse the office notification recipients defined by fusion_claims. - partners = company.sudo() - recipients = getattr(partners, 'x_fc_office_notification_ids', False) - if recipients: - return [p.email for p in recipients if p.email] - return [] + company_sudo = company.sudo() + recipients = getattr(company_sudo, 'x_fc_office_notification_ids', False) + emails = [p.email for p in (recipients or []) if p.email] + if not emails: + _logger.info( + 'No office notification recipients configured on company %s - ' + 'skipping office intake email.', + company.name, + ) + return emails diff --git a/fusion_repairs/models/maintenance_contract.py b/fusion_repairs/models/maintenance_contract.py index bed7e5ff..aa290d7e 100644 --- a/fusion_repairs/models/maintenance_contract.py +++ b/fusion_repairs/models/maintenance_contract.py @@ -14,6 +14,8 @@ a tokenized booking link. import secrets from datetime import timedelta +from dateutil.relativedelta import relativedelta + from odoo import _, api, fields, models @@ -27,6 +29,7 @@ CONTRACT_STATES = [ class FusionRepairMaintenanceContract(models.Model): _name = 'fusion.repair.maintenance.contract' + _inherit = ['mail.thread'] _description = 'Repair Maintenance Contract' _order = 'next_due_date, id' @@ -76,7 +79,10 @@ class FusionRepairMaintenanceContract(models.Model): 'res.company', default=lambda self: self.env.company, ) - _inherit = ['mail.thread'] + _booking_token_unique = models.Constraint( + 'unique(booking_token)', + 'Booking token must be unique.', + ) @api.model_create_multi def create(self, vals_list): @@ -93,10 +99,15 @@ class FusionRepairMaintenanceContract(models.Model): # ROLL FORWARD # ------------------------------------------------------------------ def roll_next_due_date(self): - """Advance next_due_date by interval_months and reset cycle state.""" + """Advance next_due_date by interval_months and reset cycle state. + + Called from technician_task.write() when a maintenance task moves to + 'completed' (see technician_task.py). + """ for c in self: base = c.last_service_date or fields.Date.context_today(c) - c.next_due_date = base + timedelta(days=(c.interval_months or 12) * 30) + # relativedelta handles month boundaries correctly (28/29/30/31). + c.next_due_date = base + relativedelta(months=c.interval_months or 12) c.last_reminder_band = False c.booking_repair_id = False @@ -161,8 +172,10 @@ class FusionRepairMaintenanceContract(models.Model): 'schedule_date': scheduled_date or fields.Datetime.now(), 'x_fc_intake_source': 'client_portal', 'x_fc_urgency': 'normal', - 'x_fc_repair_category_id': self.product_id.product_tmpl_id.x_fc_repair_category_id.id + 'x_fc_repair_category_id': + self.product_id.product_tmpl_id.x_fc_repair_category_id.id if self.product_id.product_tmpl_id.x_fc_repair_category_id else False, + 'x_fc_maintenance_contract_id': self.id, 'internal_notes': f'

Maintenance visit booked from reminder for contract {self.name}.

', }) @@ -204,6 +217,6 @@ class SaleOrder(models.Model): 'product_id': product.id, 'original_sale_order_id': so.id, 'interval_months': interval, - 'next_due_date': today + timedelta(days=interval * 30), + 'next_due_date': today + relativedelta(months=interval), 'state': 'active', }) diff --git a/fusion_repairs/models/repair_order.py b/fusion_repairs/models/repair_order.py index 06854124..a017112d 100644 --- a/fusion_repairs/models/repair_order.py +++ b/fusion_repairs/models/repair_order.py @@ -2,7 +2,7 @@ # Copyright 2024-2026 Nexa Systems Inc. # License OPL-1 (Odoo Proprietary License v1.0) -from datetime import timedelta +from dateutil.relativedelta import relativedelta from odoo import api, fields, models, _ from odoo.exceptions import UserError @@ -71,6 +71,15 @@ class RepairOrder(models.Model): index=True, help='Auto-matched catalogue entry that pre-fills estimated cost and duration.', ) + + # Maintenance contract back-link (Phase 3) + x_fc_maintenance_contract_id = fields.Many2one( + 'fusion.repair.maintenance.contract', + string='Maintenance Contract', + index=True, + help='Set when this repair was spawned from a maintenance reminder booking. ' + 'Completing the related technician task rolls the contract to its next cycle.', + ) x_fc_intake_answer_count = fields.Integer( compute='_compute_intake_answer_count', ) @@ -240,8 +249,8 @@ class RepairOrder(models.Model): ) if not warranty_months: return False - # Datetime + months: use simple 30-day approximation per month for now. - cutoff = fields.Datetime.from_string(str(delivery_date)) + timedelta(days=warranty_months * 30) + # relativedelta handles month boundaries correctly (28/29/30/31). + cutoff = fields.Datetime.from_string(str(delivery_date)) + relativedelta(months=warranty_months) return fields.Datetime.now() <= cutoff # ------------------------------------------------------------------ diff --git a/fusion_repairs/models/repair_product_category.py b/fusion_repairs/models/repair_product_category.py index d360f4d4..8a1fde2a 100644 --- a/fusion_repairs/models/repair_product_category.py +++ b/fusion_repairs/models/repair_product_category.py @@ -39,9 +39,10 @@ class FusionRepairProductCategory(models.Model): help='Default intake question set shown when this category is selected.', ) - _sql_constraints = [ - ('code_unique', 'unique(code)', 'Category code must be unique.'), - ] + _code_unique = models.Constraint( + 'unique(code)', + 'Category code must be unique.', + ) @api.depends('name', 'code') def _compute_display_name(self): diff --git a/fusion_repairs/models/repair_warranty.py b/fusion_repairs/models/repair_warranty.py index 29c0756e..518406f0 100644 --- a/fusion_repairs/models/repair_warranty.py +++ b/fusion_repairs/models/repair_warranty.py @@ -67,10 +67,12 @@ class FusionRepairWarrantyCoverage(models.Model): compute='_compute_expiry_date', store=True, ) + # Non-stored compute - DO NOT add store=True. The 'active vs not' status is + # time-dependent (today >= expiry_date), and a stored compute would never + # auto-refresh as days pass. find_active_for() filters by expiry_date directly. is_active = fields.Boolean( string='Active', compute='_compute_is_active', - store=True, ) notes = fields.Text() @@ -107,12 +109,20 @@ class FusionRepairWarrantyCoverage(models.Model): # ------------------------------------------------------------------ @api.model def find_active_for(self, partner_id, product_id=None, lot_id=None): - """Return active warranty coverage matching the partner + equipment, if any.""" + """Return active warranty coverage matching the partner + equipment, if any. + + Requires at least one of lot_id or product_id - without an equipment + identifier we would match any warranty on the partner, which would + falsely flag unrelated equipment as covered. + """ if not partner_id: return self.browse() + if not lot_id and not product_id: + return self.browse() + today = fields.Date.context_today(self) domain = [ ('partner_id', '=', partner_id), - ('is_active', '=', True), + ('expiry_date', '>=', today), ] if lot_id: domain.append(('lot_id', '=', lot_id)) diff --git a/fusion_repairs/models/service_catalog.py b/fusion_repairs/models/service_catalog.py index fcc08488..973a1717 100644 --- a/fusion_repairs/models/service_catalog.py +++ b/fusion_repairs/models/service_catalog.py @@ -113,29 +113,39 @@ class FusionRepairServiceCatalog(models.Model): def find_best_match(self, product_category_id, text_hints): """Return the best-matching catalogue entry, or empty recordset. + Returns empty when no symptom keywords match. We never "guess" a default + catalog because the match drives estimated cost + auto-dispatch task - + a wrong guess would propagate into pricing and scheduling. + :param product_category_id: int id of the equipment category :param text_hints: list[str] - text snippets to look for symptom keywords in - (typically: issue_summary, issue_category, recent intake answer values) """ + import re if not product_category_id: return self.browse() haystack = ' '.join(s.lower() for s in (text_hints or []) if s).strip() + if not haystack: + return self.browse() candidates = self.search([ ('product_category_id', '=', product_category_id), ('active', '=', True), ], order='sequence') if not candidates: return self.browse() - if not haystack: - return candidates[:1] best = None best_score = 0 for c in candidates: kws = [k.strip().lower() for k in (c.symptom_keywords or '').split(',') if k.strip()] - score = sum(1 for kw in kws if kw and kw in haystack) + # Word-boundary match avoids false positives where "battery" matches + # inside "no battery problem". + score = sum( + 1 for kw in kws + if kw and re.search(rf'\b{re.escape(kw)}\b', haystack) + ) if score > best_score: best = c best_score = score - if best: + # No keywords matched -> return empty rather than the lowest-sequence guess. + if best and best_score > 0: return best - return candidates[:1] + return self.browse() diff --git a/fusion_repairs/models/technician_task.py b/fusion_repairs/models/technician_task.py index f8963c83..452f31be 100644 --- a/fusion_repairs/models/technician_task.py +++ b/fusion_repairs/models/technician_task.py @@ -7,7 +7,8 @@ from odoo import fields, models class FusionTechnicianTaskRepairs(models.Model): """Adds the back-link from fusion.technician.task to repair.order so - repairs and tasks share one timeline. + repairs and tasks share one timeline. Also hooks task completion to + roll a linked maintenance contract to its next cycle. """ _inherit = 'fusion.technician.task' @@ -29,6 +30,33 @@ class FusionTechnicianTaskRepairs(models.Model): index=True, ) + def write(self, vals): + """When a maintenance task transitions to 'completed', roll the + linked contract to its next cycle. Failure to roll never blocks + the underlying task write. + """ + res = super().write(vals) + if vals.get('status') == 'completed': + for task in self: + if task.task_type != 'maintenance': + continue + repair = task.x_fc_repair_order_id + contract = repair.x_fc_maintenance_contract_id if repair else False + if not contract: + continue + try: + contract.last_service_date = fields.Date.context_today(task) + contract.roll_next_due_date() + contract.message_post(body=( + 'Rolled forward after maintenance task ' + f'{task.name} completed. ' + f'Next due {contract.next_due_date}.' + )) + except Exception: + # Never let a contract roll failure block the task write. + pass + return res + def action_view_repair_order(self): self.ensure_one() if not self.x_fc_repair_order_id: diff --git a/fusion_repairs/views/maintenance_contract_views.xml b/fusion_repairs/views/maintenance_contract_views.xml index b25cb0d0..cef34375 100644 --- a/fusion_repairs/views/maintenance_contract_views.xml +++ b/fusion_repairs/views/maintenance_contract_views.xml @@ -5,7 +5,7 @@ fusion.repair.maintenance.contract.list fusion.repair.maintenance.contract - + diff --git a/fusion_repairs/views/repair_order_views.xml b/fusion_repairs/views/repair_order_views.xml index b49549ff..4f978e04 100644 --- a/fusion_repairs/views/repair_order_views.xml +++ b/fusion_repairs/views/repair_order_views.xml @@ -16,7 +16,7 @@ type="object" string="Visit Report" class="btn-primary" - invisible="state == 'cancel'" + invisible="state in ('draft', 'cancel') or x_fc_technician_task_count == 0" groups="fusion_repairs.group_fusion_repairs_user"/>