From 560501224575088aff26233715d92789c16ea8e7 Mon Sep 17 00:00:00 2001 From: gsinghpal Date: Wed, 27 May 2026 13:44:51 -0400 Subject: [PATCH] =?UTF-8?q?fix(billing):=20importer=20review=20fixes=20?= =?UTF-8?q?=E2=80=94=20surface=20failures,=20validate,=20dedupe?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves findings from the post-build review: - C1: a partial import was indistinguishable from success. action_run_import now logs failed rows at ERROR (survives nexa's log_level=warn) and the wizard shows red/amber banners with failed/skipped counts. - H3: an unrecognized billing_cycle silently fell back to monthly (wrong plan AND price). Now raised per-row -> failed[], never silently mis-billed. - M5: a NULL plan price silently became a $0 line. Prices now preserve NULL-vs-0.0; a missing price for the subscription's cycle is failed[]. - H2: post-connect query/schema errors now become a clean UserError, not a raw SQL traceback (matches the connection-error path). - M4: per-row failures now record the exception type and log a traceback. - MED#3: charge plan_id set explicitly False so re-runs re-assert the shadow-safe NULL even if it was changed between runs. - HIGH-edge: re-run only rewrites x_fc_* on existing subs; partner_id/plan_id/ line are set at creation only (never rewrite immutable fields). - account_link: partner email match is now case-insensitive (=ilike) to avoid duplicate partners against a differently-cased pre-existing partner. Shadow-safety invariant unchanged and re-confirmed. 52/52 green on odoo-trial. --- .../models/account_link.py | 4 +- .../tests/test_importer.py | 23 ++++++ .../views/import_wizard_views.xml | 7 ++ .../wizards/import_wizard.py | 76 +++++++++++++++---- 4 files changed, 94 insertions(+), 16 deletions(-) diff --git a/fusion_centralize_billing/models/account_link.py b/fusion_centralize_billing/models/account_link.py index e8db15a0..d91361ca 100644 --- a/fusion_centralize_billing/models/account_link.py +++ b/fusion_centralize_billing/models/account_link.py @@ -46,7 +46,9 @@ class FusionBillingAccountLink(models.Model): return existing partner = self.env['res.partner'] if email: - partner = partner.search([('email', '=', email)], limit=1) + # case-insensitive so a pre-existing partner with a differently-cased email + # (created via the web UI or another sync) is reused, not duplicated. + partner = partner.search([('email', '=ilike', email)], limit=1) if not partner: partner = partner.create({'name': name or external_id, 'email': email, **(extra or {})}) return self.create({ diff --git a/fusion_centralize_billing/tests/test_importer.py b/fusion_centralize_billing/tests/test_importer.py index 849ec152..9ffdcf01 100644 --- a/fusion_centralize_billing/tests/test_importer.py +++ b/fusion_centralize_billing/tests/test_importer.py @@ -211,6 +211,29 @@ class TestImporterErrorIsolation(TransactionCase): self.assertTrue(summary['failed'], "the bad row must be recorded in failed[]") self.assertTrue(any(f['kind'] == 'user' for f in summary['failed'])) + def test_unknown_billing_cycle_is_failed_not_silently_monthly(self): + data = _fixture() + data['subscriptions'][0]['billing_cycle'] = 'annual' # not monthly/yearly + summary = self.Wizard._import_rows(data) + self.assertFalse(self.env['sale.order'].search( + [('x_fc_nexacloud_subscription_id', '=', 's-1')]), + "an unrecognized billing_cycle must NOT silently create a monthly sub") + self.assertTrue(any(f['kind'] == 'subscription' and f['id'] == 's-1' + for f in summary['failed'])) + + def test_missing_price_for_cycle_is_failed_not_zero(self): + data = _fixture() + data['plans'][0]['price_yearly'] = None # s-2 is yearly -> no price for it + summary = self.Wizard._import_rows(data) + # the yearly sub fails (no silent $0 line); the monthly one still imports + self.assertFalse(self.env['sale.order'].search( + [('x_fc_nexacloud_subscription_id', '=', 's-2')]), + "a missing price for the cycle must NOT silently create a $0 line") + self.assertTrue(self.env['sale.order'].search( + [('x_fc_nexacloud_subscription_id', '=', 's-1')])) + self.assertTrue(any(f['kind'] == 'subscription' and f['id'] == 's-2' + for f in summary['failed'])) + @tagged('post_install', '-at_install') class TestImporterReadGuard(TransactionCase): diff --git a/fusion_centralize_billing/views/import_wizard_views.xml b/fusion_centralize_billing/views/import_wizard_views.xml index a680d1f5..b38eaf3c 100644 --- a/fusion_centralize_billing/views/import_wizard_views.xml +++ b/fusion_centralize_billing/views/import_wizard_views.xml @@ -5,6 +5,13 @@ fusion.billing.import.wizard
+ + diff --git a/fusion_centralize_billing/wizards/import_wizard.py b/fusion_centralize_billing/wizards/import_wizard.py index 90caa7ac..a0ba599c 100644 --- a/fusion_centralize_billing/wizards/import_wizard.py +++ b/fusion_centralize_billing/wizards/import_wizard.py @@ -33,12 +33,25 @@ class FusionBillingImportWizard(models.TransientModel): default=True, help="Read and report what would be imported, without writing anything.") result_summary = fields.Text(readonly=True) + failed_count = fields.Integer(readonly=True) + skipped_count = fields.Integer(readonly=True) def action_run_import(self): self.ensure_one() data = self._read_nexacloud_rows() summary = self._import_rows(data, dry_run=self.dry_run) + failed = summary.get("failed") or [] + skipped = summary.get("skipped") or [] self.result_summary = json.dumps(summary, indent=2, default=str) + self.failed_count = len(failed) + self.skipped_count = len(skipped) + # A partial billing import must be loud, not buried in the JSON. Log at ERROR + # so it survives nexa's log_level=warn (INFO is suppressed there). + if failed: + _logger.error("NexaCloud import: %s row(s) FAILED%s: %s", + len(failed), " (dry-run)" if self.dry_run else "", failed) + if skipped: + _logger.warning("NexaCloud import: %s row(s) skipped: %s", len(skipped), skipped) return { "type": "ir.actions.act_window", "res_model": self._name, @@ -83,6 +96,13 @@ class FusionBillingImportWizard(models.TransientModel): "current_period_start, current_period_end FROM subscriptions") data["subscriptions"] = [dict(r) for r in cur.fetchall()] return data + except psycopg2.Error as e: + # A query/schema error (e.g. a renamed/missing column) gets the same clean + # operator message as a connection failure — not a raw SQL traceback. We + # never return a partial `data` (the return is the last statement in `try`). + raise UserError( + "Failed reading from the NexaCloud database — the source schema may " + "have changed. Underlying error:\n%s" % e) finally: conn.close() @@ -127,8 +147,10 @@ class FusionBillingImportWizard(models.TransientModel): partner_by_user[str(u["id"])] = link.partner_id self._bump(summary, created, "partners") except Exception as e: # noqa: BLE001 - per-row isolation + _logger.exception("NexaCloud import: user row %s failed", u.get("id")) summary["failed"].append( - {"kind": "user", "id": str(u.get("id")), "error": str(e)}) + {"kind": "user", "id": str(u.get("id")), + "error": "%s: %s" % (type(e).__name__, e)}) for p in data.get("plans", []): try: @@ -137,8 +159,10 @@ class FusionBillingImportWizard(models.TransientModel): plan_ctx_by_id[str(p["id"])] = ctx self._bump(summary, created, "plans") except Exception as e: # noqa: BLE001 + _logger.exception("NexaCloud import: plan row %s failed", p.get("id")) summary["failed"].append( - {"kind": "plan", "id": str(p.get("id")), "error": str(e)}) + {"kind": "plan", "id": str(p.get("id")), + "error": "%s: %s" % (type(e).__name__, e)}) for s in data.get("subscriptions", []): partner = partner_by_user.get(str(s.get("user_id") or "")) @@ -154,8 +178,10 @@ class FusionBillingImportWizard(models.TransientModel): service, partner, ctx, recurrence_plans, s) self._bump(summary, created, "subscriptions") except Exception as e: # noqa: BLE001 + _logger.exception("NexaCloud import: subscription row %s failed", s.get("id")) summary["failed"].append( - {"kind": "subscription", "id": str(s.get("id")), "error": str(e)}) + {"kind": "subscription", "id": str(s.get("id")), + "error": "%s: %s" % (type(e).__name__, e)}) _logger.info("NexaCloud import summary: %s", summary) return summary @@ -234,8 +260,12 @@ class FusionBillingImportWizard(models.TransientModel): Charge = self.env["fusion.billing.charge"] plan_code = str(prow["id"]) name = prow.get("name") or plan_code - price_monthly = float(prow.get("price_monthly") or 0.0) - price_yearly = float(prow.get("price_yearly") or 0.0) + # Preserve NULL vs 0.0: a missing price must NOT silently become a $0 line. + # The subscription import raises on a missing price for its cycle (-> failed[]). + raw_monthly = prow.get("price_monthly") + raw_yearly = prow.get("price_yearly") + price_monthly = float(raw_monthly) if raw_monthly is not None else None + price_yearly = float(raw_yearly) if raw_yearly is not None else None created = False sub_code = "NC-PLAN-%s" % plan_code @@ -244,7 +274,7 @@ class FusionBillingImportWizard(models.TransientModel): sub_product = Product.create({ "name": "NexaCloud %s" % name, "default_code": sub_code, "type": "service", "recurring_invoice": True, - "list_price": price_monthly}) + "list_price": price_monthly or 0.0}) created = True ov_code = "NC-CPU-OVG-%s" % plan_code @@ -261,7 +291,10 @@ class FusionBillingImportWizard(models.TransientModel): "price_per_unit": CPU_RATE_PER_CORE_HOUR, "unit_batch": CPU_SECONDS_PER_CORE_HOUR, "charge_model": "standard", - # plan_id intentionally omitted (NULL) — shadow safety guarantee #3 + # Shadow safety guarantee #3: plan_id MUST stay NULL so the rating cron + # never auto-mutates order lines. Set it explicitly (not just omitted) so a + # re-run re-asserts NULL even if someone set it on the charge between runs. + "plan_id": False, } charge = Charge.search( [("plan_code", "=", plan_code), ("metric_id", "=", metric.id)], limit=1) @@ -280,20 +313,29 @@ class FusionBillingImportWizard(models.TransientModel): SaleOrder = self.env["sale.order"] SaleOrderLine = self.env["sale.order.line"] sub_ext = str(srow["id"]) - cycle = (srow.get("billing_cycle") or "monthly").lower() + cycle = (srow.get("billing_cycle") or "").strip().lower() + if cycle not in ("monthly", "yearly"): + raise UserError( + "Subscription %s has an unrecognized billing_cycle %r — cannot pick a " + "plan/price." % (sub_ext, srow.get("billing_cycle"))) rec_plan = recurrence_plans["yearly"] if cycle == "yearly" else recurrence_plans["monthly"] price = plan_ctx["price_yearly"] if cycle == "yearly" else plan_ctx["price_monthly"] + if price is None: + raise UserError( + "Subscription %s is billed %s but its plan has no %s price." % ( + sub_ext, cycle, cycle)) product = plan_ctx["sub_product"] - order_vals = { - "partner_id": partner.id, "plan_id": rec_plan.id, - "x_fc_nexacloud_subscription_id": sub_ext, + # x_fc_* are always (re-)written; identity fields (partner_id/plan_id/order_line) + # are set ONLY at creation, so a re-run never rewrites immutable fields on an + # order that may since have been confirmed. + shadow_vals = { "x_fc_nexacloud_deployment_id": str(srow.get("deployment_id") or ""), "x_fc_billing_service_id": service.id, "x_fc_shadow": True, } existing = SaleOrder.search( [("x_fc_nexacloud_subscription_id", "=", sub_ext)], limit=1) if existing: - existing.write(order_vals) + existing.write(shadow_vals) line = existing.order_line.filtered(lambda l: l.product_id == product) line_vals = {"product_uom_qty": 1, "price_unit": price} if line: @@ -304,9 +346,13 @@ class FusionBillingImportWizard(models.TransientModel): order = existing created = False else: - order_vals["order_line"] = [(0, 0, { - "product_id": product.id, "product_uom_qty": 1, "price_unit": price})] - order = SaleOrder.create(order_vals) + order = SaleOrder.create({ + "partner_id": partner.id, "plan_id": rec_plan.id, + "x_fc_nexacloud_subscription_id": sub_ext, + "order_line": [(0, 0, { + "product_id": product.id, "product_uom_qty": 1, "price_unit": price})], + **shadow_vals, + }) created = True # guarantee the explicit price stuck (a pricelist compute may have overwritten it) line = order.order_line.filtered(lambda l: l.product_id == product)