diff --git a/spp_registry/README.rst b/spp_registry/README.rst index 1ead9f1ac..da0eeb46a 100644 --- a/spp_registry/README.rst +++ b/spp_registry/README.rst @@ -139,6 +139,29 @@ Dependencies Changelog ========= +19.0.2.2.3 +~~~~~~~~~~ + +- fix(registry): repair the stored ``status``/``is_ended`` computes on + ``spp.group.membership`` once the clock crosses ``ended_date``. Both + fields depend only on ``ended_date`` compared against *now*, so a + future-dated departure never took effect once the clock crossed it — + rosters, metrics, API search and downstream gates kept treating the + member as active indefinitely. Writing a future ``ended_date`` now + schedules a lightweight, index-served repair cron in the minute after + that moment (staleness window ≈1–2 minutes), and a daily sweep + self-heals everything else: rows already stale in existing databases + (drained in committed batches, resuming across runs until the backlog + is gone) and rows written behind the ORM, including + ``is_ended = NULL`` rows that raw-SQL consumers treated as ended + (#417) +- upgrade note: this version adds a partial index on + ``spp_group_membership.ended_date``, built with a write-blocking + ``CREATE INDEX`` during the module upgrade. On a very large registry, + pre-create it concurrently before upgrading and the upgrade will skip + the build: + ``CREATE INDEX CONCURRENTLY IF NOT EXISTS spp_group_membership__ended_date_index ON spp_group_membership (ended_date) WHERE ended_date IS NOT NULL;`` + 19.0.2.2.2 ~~~~~~~~~~ diff --git a/spp_registry/__manifest__.py b/spp_registry/__manifest__.py index 81e898bf8..c8e43552f 100644 --- a/spp_registry/__manifest__.py +++ b/spp_registry/__manifest__.py @@ -3,7 +3,7 @@ { "name": "OpenSPP Registry", "category": "OpenSPP/Core", - "version": "19.0.2.2.2", + "version": "19.0.2.2.3", "sequence": 1, "author": "OpenSPP.org", "website": "https://github.com/OpenSPP/OpenSPP2", @@ -33,6 +33,7 @@ # "data/id_types.xml", "data/vocabularies.xml", "data/res_users.xml", + "data/ir_cron.xml", # Security "security/privileges.xml", "security/groups.xml", diff --git a/spp_registry/data/ir_cron.xml b/spp_registry/data/ir_cron.xml new file mode 100644 index 000000000..5490e21f9 --- /dev/null +++ b/spp_registry/data/ir_cron.xml @@ -0,0 +1,52 @@ + + + + + Registry: Recompute Ended Group Memberships + + code + model._cron_recompute_ended_status() + 1 + days + + + + + + + + + Registry: Repair Clock-Crossed Group Memberships + + code + model._cron_repair_crossed_ended_status() + 1 + days + + + + + diff --git a/spp_registry/models/group_membership.py b/spp_registry/models/group_membership.py index 467170fd6..0423a0557 100644 --- a/spp_registry/models/group_membership.py +++ b/spp_registry/models/group_membership.py @@ -1,6 +1,7 @@ # Part of OpenSPP. See LICENSE file for full copyright and licensing details. import logging +from datetime import timedelta from odoo import _, api, fields, models from odoo.exceptions import ValidationError @@ -51,7 +52,9 @@ def _compute_has_group_membership_type_codes(self): rec.has_group_membership_type_codes = has_codes start_date = fields.Datetime(default=lambda self: fields.Datetime.now()) - ended_date = fields.Datetime() + # btree_not_null: a plain btree would index every NULL ended_date (the + # open-membership majority) for no query benefit, amplifying writes. + ended_date = fields.Datetime(index="btree_not_null") status = fields.Selection( [("inactive", "Inactive"), ("active", " ")], compute="_compute_status", @@ -122,14 +125,22 @@ def _name_search(self, name, domain=None, operator="ilike", limit=100, order=Non domain = [("group", operator, name)] + domain return self._search(domain, limit=limit, order=order) + @api.model + def _is_ended_as_of(self, ended_date, now): + """Single home of the "membership is ended at time T" predicate. + + Used by both stored computes, the archiving onchange and the + repair-cron domains; keep external consumers (raw-SQL readers of + ``is_ended`` in spp_registry/models/group.py, spp_api_v2_gis, + spp_cel_domain, spp_gis_report) in mind when changing it (#421). + """ + return bool(ended_date and ended_date <= now) + @api.depends("ended_date") def _compute_is_ended(self): + now = fields.Datetime.now() for rec in self: - is_ended = False - if rec.ended_date and rec.ended_date <= fields.Datetime.now(): - is_ended = True - - rec.is_ended = is_ended + rec.is_ended = self._is_ended_as_of(rec.ended_date, now) def _invalidate_group_metrics(self, groups): """Schedule metric invalidation for affected groups. @@ -159,6 +170,8 @@ def write(self, vals): affected_groups |= self.mapped("group") self._invalidate_group_metrics(affected_groups) + if self and "ended_date" in vals: + self._schedule_ended_status_repair([vals["ended_date"]]) return res @api.model_create_multi @@ -167,6 +180,9 @@ def create(self, vals_list): # Invalidate metrics for all affected groups groups = res.mapped("group") self._invalidate_group_metrics(groups) + # Read the dates back from the records, not vals_list: a missing + # key can still be filled from a default_ended_date context key. + self._schedule_ended_status_repair(res.mapped("ended_date")) return res def unlink(self): @@ -204,12 +220,272 @@ def open_group_form(self): @api.depends("ended_date") def _compute_status(self): + now = fields.Datetime.now() for record in self: - # check if memebership end date available and less than current date - if record.ended_date and record.ended_date <= fields.Datetime.now(): - record.status = "inactive" - else: - record.status = "active" + # check if membership end date available and less than current date + record.status = "inactive" if self._is_ended_as_of(record.ended_date, now) else "active" + + def _schedule_ended_status_repair(self, ended_dates): + """Point the repair cron at every future ``ended_date`` being written. + + The stored computes go stale the moment the clock crosses + ``ended_date`` (see ``_cron_recompute_ended_status``); a persistent + ``ir.cron.trigger`` at that time shrinks the staleness window from + the sweep cadence to a couple of minutes. Each moment is rounded up + to the next full minute *past* ``ended_date`` — unconditionally, + because the cron machinery consumes triggers against the database + clock while the repair predicate compares against the application + clock, so a trigger firing exactly at ``ended_date`` could be + consumed while the row still reads as not-yet-ended. Triggers + already pending for the same minute are reused, so a cohort + departure written one row per call files a single trigger; two + concurrent transactions can still race to a duplicate, and a + trigger orphaned by a later date change survives to its moment — + both harmless, each just runs one idempotent index-served sweep. + """ + now = fields.Datetime.now() + at_list = set() + for ended in ended_dates: + ended = fields.Datetime.to_datetime(ended) + if not ended or ended <= now: + continue + at_list.add(ended.replace(second=0, microsecond=0) + timedelta(minutes=1)) + if not at_list: + return + # raise_if_not_found=False: if the noupdate cron record has been + # deleted, membership writes must degrade to the daily sweep, not + # break the core registry write path over a latency optimisation. + cron = self.env.ref( + "spp_registry.cron_repair_crossed_membership_ended_status", + raise_if_not_found=False, + ) + if not cron: + return + pending = ( + self.env["ir.cron.trigger"] # nosemgrep: odoo-sudo-without-context + .sudo() # system table ordinary users cannot read; _trigger creates as sudo anyway + .search([("cron_id", "=", cron.id), ("call_at", "in", list(at_list))]) + ) + at_list -= set(pending.mapped("call_at")) + if at_list: + cron._trigger(at=at_list) + + @api.model + def _crossed_ended_status_domains(self, now): + """Domains selecting rows the clock has crossed: ``ended_date`` is + past but a stored column still reads active. One conjunctive leg + per stale column — no ORs, so both legs are served by the partial + ``ended_date`` index. These are the only stale states the mere + passage of time can produce, so the trigger-driven cron sweeps + just these. + """ + return [ + [("ended_date", "<=", now), ("is_ended", "=", False)], + [("ended_date", "<=", now), ("status", "!=", "inactive")], + ] + + @api.model + def _stale_ended_status_domains(self, now): + """All domains selecting rows whose stored ``status``/``is_ended`` + disagree with the clock. Beyond the crossed legs this adds the + reactivation legs, which only guard rows whose ``ended_date`` was + cleared or future-moved behind the ORM and are expected to match + nothing. The two ``ended_date IS NULL`` legs cannot be served by + the partial index (each probe is a full table scan), which is why + only the daily safety net runs them — never the trigger-driven + cron. If a registry ever accumulates enough non-ORM drift for + those scans to matter, give them partial indexes or a last-swept + watermark (see #421 for the planned collapse of the status legs). + """ + return self._crossed_ended_status_domains(now) + [ + # Not (or no longer) ended, still stored as ended. + [("ended_date", "=", False), ("is_ended", "=", True)], + [("ended_date", "=", False), ("status", "!=", "active")], + [("ended_date", ">", now), ("is_ended", "=", True)], + [("ended_date", ">", now), ("status", "!=", "active")], + ] + + @api.model + def _repair_null_is_ended(self, now, batch_size): + """Repair rows holding ``is_ended = NULL`` that should read active. + + Rows written behind the ORM can leave ``is_ended`` NULL, and every + raw-SQL consumer of the column treats NULL as ended. The ORM + cannot repair the open-membership case — NULL reads back from the + cache as False, so a recompute writes nothing — hence this one + SQL-level leg mirroring ``_is_ended_as_of``. (NULL rows whose + ``ended_date`` has passed need no special casing: the computed + True differs from the cached False, so the ORM legs repair them.) + + Expected to match nothing on a healthy database (the column has a + Python default, so it was backfilled at creation) — but the + ``is_ended IS NULL`` probe has no index behind it, so even proving + that costs a full table scan, which is why only the daily safety + net calls this, never the trigger-driven cron. The UPDATE stamps + ``write_date``/``write_uid`` the way the ORM legs' flush does, so + ``write_date``-keyed consumers (incremental syncs, the API's + ``changed_by``) see the repair. Work is bounded to ``batch_size`` + rows per statement, with progress committed between batches. + + Returns ``(repaired, time_left)``: the repaired memberships and + the cron time budget reported by the last progress commit — falsy + when the budget ran out with NULL rows possibly left. + """ + self.flush_model(["is_ended", "ended_date"]) + repaired = self.browse() + time_left = float("inf") + while True: + self.env.cr.execute( + "UPDATE spp_group_membership " + "SET is_ended = false, write_date = %s, write_uid = %s " + "WHERE id IN (SELECT id FROM spp_group_membership " + "WHERE is_ended IS NULL AND (ended_date IS NULL OR ended_date > %s) LIMIT %s) " + "RETURNING id", + (now, self.env.uid, now, batch_size), + ) + ids = [row[0] for row in self.env.cr.fetchall()] + if not ids: + break + batch = self.browse(ids) + batch.invalidate_recordset() + self._invalidate_group_metrics(batch.mapped("group")) + repaired |= batch + if len(ids) < batch_size: + self.env["ir.cron"]._commit_progress(len(ids), remaining=0) + break + # A full batch may not be the last: report the backlog so a + # budget-exhausted run is continued ASAP instead of waiting a + # full sweep interval on a "no work remaining" report. + time_left = self.env["ir.cron"]._commit_progress(len(ids), remaining=1) + if not time_left: + break + return repaired, time_left + + @api.model + def _cron_repair_crossed_ended_status(self, batch_size=5000): + """Trigger-driven repair: only the index-served crossed legs. + + ``_schedule_ended_status_repair`` points this cron at every future + ``ended_date`` written, so it can run many times a day and must + stay cheap — no NULL-drift legs (each of those probes is a full + table scan; the daily ``_cron_recompute_ended_status`` sweeps + them). See that method for the full story and the shared + semantics. + """ + return self._repair_stale_ended_status(batch_size, full=False) + + @api.model + def _cron_recompute_ended_status(self, batch_size=5000): + """Daily safety net: repair stored ``status``/``is_ended`` on every + row whose stored values disagree with the clock. + + Both computes depend only on ``ended_date`` and compare it against + *now*, so a recompute fires on a write to ``ended_date`` but never + when time passes it: a future-dated departure would stay stored as + active forever. Writes of a future ``ended_date`` schedule the + lightweight ``_cron_repair_crossed_ended_status`` in the minute + after that moment (``_schedule_ended_status_repair``), so this + daily sweep self-heals what triggers cannot cover: rows already + stale in pre-existing databases and rows written behind the ORM, + including the full-scan NULL-drift legs and + ``_repair_null_is_ended``. It finds stale rows and re-triggers the + computes through the normal ORM path. Archived rows are included — + the UI onchange archives memberships ended in the past, and those + must be repaired too. ``active`` itself is deliberately left + untouched: archiving changes record visibility everywhere and is a + separate decision from the stored computes (see issue #417). + + Two write-path side effects intentionally differ from a real + write: the recompute flush still stamps ``write_uid``/``write_date`` + (repaired rows show the cron user as last modified — e.g. + ``changed_by`` in the API's membership history reads ``write_uid``; + the raw NULL-repair UPDATE stamps the same columns itself), and + the repair is invisible to ``spp_audit`` write-rules, which hook + ``write()``. Both are accepted: the repair only restores what a + timely recompute would have stored. + + Rows are repaired in ``batch_size`` chunks, each committed via + ``ir.cron._commit_progress``: a serialization failure rolls back + only its own chunk, and a backlog larger than the cron time budget + drains across runs — a partially-done run is rescheduled ASAP + instead of waiting a full sweep interval. ``_commit_progress`` is + Odoo 19's sanctioned batching API for cron work and the deliberate + exception to the AGENTS.md checklist's "no ``cr.commit()`` in + loops" rule, which targets ad-hoc commits. Because of those + commits, calling this outside a cron (e.g. from a shell) commits + the current transaction. The chunk size follows the 5,000-record + cap in docs/principles/performance-scalability.md. + + Returns the repaired memberships. + """ + return self._repair_stale_ended_status(batch_size, full=True) + + @api.model + def _repair_stale_ended_status(self, batch_size, full): + """Shared repair loop; see ``_cron_recompute_ended_status``.""" + if batch_size < 1: + raise ValueError("batch_size must be a positive number of rows") + memberships = self.with_context(active_test=False) + repaired_ids = set() + time_left = float("inf") + if full: + null_repaired, time_left = memberships._repair_null_is_ended(fields.Datetime.now(), batch_size) + repaired_ids.update(null_repaired.ids) + domains = self._stale_ended_status_domains if full else self._crossed_ended_status_domains + pass_no = 0 + while time_left: + # Re-read the clock every pass: a row whose ended_date is + # crossed while the run is in flight recomputes to the very + # values a stale `now` would keep selecting it for. + now = fields.Datetime.now() + legs = domains(now) + # Rotate the starting leg each pass so one direction's large + # backlog cannot starve the other directions within a run. + offset = pass_no % len(legs) + pass_no += 1 + chunk = memberships.browse() + exhausted = True + for leg in legs[offset:] + legs[:offset]: + quota = batch_size - len(chunk) + if quota <= 0: + exhausted = False + break + found = memberships.search(leg, limit=quota) + if len(found) == quota: + # The leg may hold more. Exhaustion must be tracked + # per leg: the legs overlap and the union + # de-duplicates, so a short chunk alone is no proof + # the backlog is drained. + exhausted = False + chunk |= found + if not chunk: + if repaired_ids: + # Close the progress report so a drained backlog isn't + # left marked partially done (and pointlessly + # rescheduled ASAP). + self.env["ir.cron"]._commit_progress(0, remaining=0) + break + chunk.modified(["ended_date"]) + # The recompute flushes through low-level SQL and bypasses this + # model's write() override, so the metric-invalidation hook must + # be called explicitly. + self._invalidate_group_metrics(chunk.mapped("group")) + repaired_ids.update(chunk.ids) + self.env.flush_all() + # `remaining` is a drained/not-drained signal, not a count. + if exhausted: + self.env["ir.cron"]._commit_progress(len(chunk), remaining=0) + break + time_left = self.env["ir.cron"]._commit_progress(len(chunk), remaining=1) + if not time_left: + _logger.info("[spp.registry] Ended-status backlog remains; the cron will be re-triggered to continue") + repaired = memberships.browse(repaired_ids) + if repaired: + _logger.info( + "[spp.registry] Repaired ended-status on %d group membership(s)", + len(repaired), + ) + return repaired @api.constrains("ended_date") def _check_ended_date(self): @@ -219,8 +495,7 @@ def _check_ended_date(self): @api.onchange("ended_date") def _onchange_ended_date(self): + now = fields.Datetime.now() for record in self: - record.active = True # if ended date is less than current date, set active to false - if record.ended_date and record.ended_date <= fields.Datetime.now(): - record.active = False + record.active = not self._is_ended_as_of(record.ended_date, now) diff --git a/spp_registry/readme/HISTORY.md b/spp_registry/readme/HISTORY.md index aed786664..f75e01fb1 100644 --- a/spp_registry/readme/HISTORY.md +++ b/spp_registry/readme/HISTORY.md @@ -1,3 +1,8 @@ +### 19.0.2.2.3 + +- fix(registry): repair the stored `status`/`is_ended` computes on `spp.group.membership` once the clock crosses `ended_date`. Both fields depend only on `ended_date` compared against *now*, so a future-dated departure never took effect once the clock crossed it — rosters, metrics, API search and downstream gates kept treating the member as active indefinitely. Writing a future `ended_date` now schedules a lightweight, index-served repair cron in the minute after that moment (staleness window ≈1–2 minutes), and a daily sweep self-heals everything else: rows already stale in existing databases (drained in committed batches, resuming across runs until the backlog is gone) and rows written behind the ORM, including `is_ended = NULL` rows that raw-SQL consumers treated as ended (#417) +- upgrade note: this version adds a partial index on `spp_group_membership.ended_date`, built with a write-blocking `CREATE INDEX` during the module upgrade. On a very large registry, pre-create it concurrently before upgrading and the upgrade will skip the build: `CREATE INDEX CONCURRENTLY IF NOT EXISTS spp_group_membership__ended_date_index ON spp_group_membership (ended_date) WHERE ended_date IS NOT NULL;` + ### 19.0.2.2.2 - fix(registry): let an ID type be used again after its ID was removed. Removing an ID through a change request keeps the row and marks it Invalid, and the old uniqueness rule counted those dead rows — so the registrant was left with an Invalid ID and no way to add a valid one of the same type. Uniqueness now applies to live IDs only, and is refused before the write so the message names the ID type rather than surfacing a database error (#1136) diff --git a/spp_registry/static/description/index.html b/spp_registry/static/description/index.html index 063ba1ff7..f65bf0c23 100644 --- a/spp_registry/static/description/index.html +++ b/spp_registry/static/description/index.html @@ -518,6 +518,30 @@

Changelog

+

19.0.2.2.3

+ +
+

19.0.2.2.2

-
+

19.0.2.2.1

-
+

19.0.2.1.4

  • fix(registry): remove the dead @api.constrains("age") @@ -552,7 +576,7 @@

    19.0.2.1.4

    dropped
-
+

19.0.2.1.3

  • fix(registry): show an ID Status column on the group form @@ -563,7 +587,7 @@

    19.0.2.1.3

    (#1110)
-
+

19.0.2.1.1

  • fix(views): add reusable x2many_no_padding JS widget that @@ -573,7 +597,7 @@

    19.0.2.1.1

    don’t bloat the layout (#943).
-
+

19.0.2.0.0

  • Initial migration to OpenSPP2
    • diff --git a/spp_registry/tests/__init__.py b/spp_registry/tests/__init__.py index 2177f1fd9..a5fc2ccb5 100644 --- a/spp_registry/tests/__init__.py +++ b/spp_registry/tests/__init__.py @@ -10,6 +10,7 @@ from . import test_phone_number from . import test_reg_id from . import test_membership_constraints +from . import test_membership_status_cron from . import test_registrant_misc from . import test_group_aggregation from . import test_res_config_settings diff --git a/spp_registry/tests/test_membership_status_cron.py b/spp_registry/tests/test_membership_status_cron.py new file mode 100644 index 000000000..0afec4827 --- /dev/null +++ b/spp_registry/tests/test_membership_status_cron.py @@ -0,0 +1,532 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Repair of stored ``status``/``is_ended`` stale against the clock — see +``_cron_recompute_ended_status`` (#417) for the full story. + +The stale state cannot be produced through the ORM (writing ``ended_date`` +recomputes at write time), so these tests age rows behind the ORM's back with +raw SQL — exactly how production rows drift. +""" + +from datetime import timedelta +from unittest.mock import patch + +from odoo import fields +from odoo.tests import tagged + +from .test_membership_constraints import MembershipCommon +from .test_metric_invalidation import _patch_invalidate_funnel + + +@tagged("post_install", "-at_install") +class TestMembershipEndedStatusCron(MembershipCommon): + """``_cron_recompute_ended_status`` — repair rows the clock has crossed.""" + + def _make_membership(self, individual, **vals): + vals.update({"group": self.group.id, "individual": individual.id}) + return self.Membership.create(vals) + + def _age_row(self, rec, ended_date=None, active=True): + """Rewrite the date window (and ``active``) behind the ORM's back so + the stored computes keep their now-wrong values. Defaults to a + departure one year ago; ``start_date`` is derived so the row stays + consistent with the start/end constraint.""" + if ended_date is None: + ended_date = fields.Datetime.now() - timedelta(days=365) + start_date = ended_date - timedelta(days=365) + self.env.flush_all() + self.env.cr.execute( + "UPDATE spp_group_membership SET start_date = %s, ended_date = %s, active = %s WHERE id = %s", + (start_date, ended_date, active, rec.id), + ) + rec.invalidate_recordset() + + def _read_stored_columns(self, rec): + """Read ``status``/``is_ended`` straight from the SQL columns, the way + the raw-SQL consumers of ``is_ended`` do — bypassing the ORM cache, + which would recompute pending fields on read and mask a missing + column write.""" + self.env.flush_all() + self.env.cr.execute( + "SELECT status, is_ended FROM spp_group_membership WHERE id = %s", + (rec.id,), + ) + return self.env.cr.fetchone() + + def _run_cron(self, model=None, time_left=float("inf"), method="_cron_recompute_ended_status", **kwargs): + """Run a repair cron with ``ir.cron._commit_progress`` stubbed out. + + The real method commits, which on a TestCursor releases the test + savepoint and leaks this test's rows into the rest of the class. + The stub records ``(processed, remaining)`` per chunk so tests can + assert the chunking behaviour; ``time_left`` is what the stub + reports back as the remaining cron time budget. ``method`` picks + the cron entrypoint (daily full sweep by default). + """ + calls = [] + + def fake_commit_progress(_cron, processed=0, remaining=None, **_kw): + calls.append((processed, remaining)) + return time_left + + # `is None`, not `or`: a model handle is an empty recordset and + # therefore falsy — `model or ...` would silently fall back to the + # superuser-bound self.Membership. + target = self.Membership if model is None else model + with patch.object( + type(self.env["ir.cron"]), + "_commit_progress", + autospec=True, + side_effect=fake_commit_progress, + ): + repaired = getattr(target, method)(**kwargs) + return repaired, calls + + def test_cron_ends_membership_the_clock_has_crossed(self): + rec = self._make_membership(self.individual_a) + self._age_row(rec) + + # Stale precondition: departed a year ago, still stored as active. + self.assertEqual(self._read_stored_columns(rec), ("active", False)) + self.assertEqual(rec.status, "active") + self.assertFalse(rec.is_ended) + + repaired, _calls = self._run_cron() + + self.assertIn(rec, repaired) + self.assertEqual(rec.status, "inactive") + self.assertTrue(rec.is_ended) + # The SQL columns must be repaired too — four consumers read + # is_ended in raw SQL and never see the ORM cache. + self.assertEqual(self._read_stored_columns(rec), ("inactive", True)) + + def test_cron_reactivates_membership_whose_end_moved_to_future(self): + past = fields.Datetime.now() - timedelta(days=365) + rec = self._make_membership( + self.individual_a, + start_date=past - timedelta(days=1), + ended_date=past, + ) + self.assertEqual(rec.status, "inactive") + self.assertTrue(rec.is_ended) + + # The end date is pushed to the future behind the ORM's back; the + # stored "inactive" is now wrong in the other direction. + future = fields.Datetime.now() + timedelta(days=365) + self._age_row(rec, future) + self.assertEqual(rec.status, "inactive") + self.assertTrue(rec.is_ended) + + repaired, _calls = self._run_cron() + + self.assertIn(rec, repaired) + self.assertEqual(rec.status, "active") + self.assertFalse(rec.is_ended) + self.assertEqual(self._read_stored_columns(rec), ("active", False)) + + def test_cron_repairs_archived_rows(self): + rec = self._make_membership(self.individual_a) + self._age_row(rec, active=False) + self.assertEqual(rec.status, "active") + self.assertFalse(rec.is_ended) + + self._run_cron() + + self.assertEqual(rec.status, "inactive") + self.assertTrue(rec.is_ended) + # The cron repairs the computes only; archiving stays as it was. + self.assertFalse(rec.active) + + def test_cron_repairs_null_is_ended_row(self): + # A raw INSERT that omits the nullable computed columns: every + # raw-SQL consumer treats NULL is_ended as ended, and the ORM alone + # cannot repair NULL -> False (the cache reads NULL as False, so a + # recompute writes nothing). + rec = self._make_membership(self.individual_a) + old_stamp = fields.Datetime.now() - timedelta(days=365) + self.env.flush_all() + self.env.cr.execute( + "UPDATE spp_group_membership SET is_ended = NULL, status = NULL, ended_date = NULL, " + "write_date = %s WHERE id = %s", + (old_stamp, rec.id), + ) + rec.invalidate_recordset() + self.assertEqual(self._read_stored_columns(rec), (None, None)) + + repaired, _calls = self._run_cron() + + self.assertIn(rec, repaired) + self.assertEqual(self._read_stored_columns(rec), ("active", False)) + # The raw repair stamps write metadata the way the ORM legs' flush + # does, so write_date-keyed consumers (incremental syncs, the + # API's changed_by) see the repair. + self.env.cr.execute( + "SELECT write_date, write_uid FROM spp_group_membership WHERE id = %s", + (rec.id,), + ) + write_date, write_uid = self.env.cr.fetchone() + self.assertGreater(write_date, old_stamp) + self.assertEqual(write_uid, self.env.uid) + + def test_cron_repairs_null_columns_with_past_ended_date(self): + # NULL is_ended/status with a departure already in the past must be + # repaired by the ORM legs: the computed True/"inactive" differs + # from the cached False, so — unlike the NULL-and-active case — a + # normal recompute does write. Pins the ORM-internals assumption + # documented on _repair_null_is_ended. + rec = self._make_membership(self.individual_a) + now = fields.Datetime.now() + self.env.flush_all() + self.env.cr.execute( + "UPDATE spp_group_membership SET is_ended = NULL, status = NULL, " + "start_date = %s, ended_date = %s WHERE id = %s", + (now - timedelta(days=730), now - timedelta(days=365), rec.id), + ) + rec.invalidate_recordset() + self.assertEqual(self._read_stored_columns(rec), (None, None)) + + repaired, _calls = self._run_cron() + + self.assertIn(rec, repaired) + self.assertEqual(self._read_stored_columns(rec), ("inactive", True)) + + def test_cron_repairs_memberships_of_disabled_registrants(self): + # Two global ir.rule records hide memberships of disabled + # registrants from ordinary users; the cron record pins + # user_id=base.user_root precisely so the sweep sees them. An + # officer-run sweep misses the row, the root-run sweep repairs it. + rec = self._make_membership(self.individual_a) + self._age_row(rec) + self.individual_a.disabled = fields.Datetime.now() + # Flush: the rule domain is evaluated in SQL, which must see the + # disabled stamp, not the pending cache value. + self.env.flush_all() + + officer = self._make_user("status_cron_officer", ["spp_registry.group_registry_officer"]) + missed, _calls = self._run_cron(model=self.Membership.with_user(officer)) + self.assertNotIn(rec, missed) + self.assertEqual(self._read_stored_columns(rec), ("active", False)) + + repaired, _calls = self._run_cron() + self.assertIn(rec, repaired) + self.assertEqual(self._read_stored_columns(rec), ("inactive", True)) + + def test_cron_leaves_correct_rows_untouched(self): + open_ended = self._make_membership(self.individual_a) + past = fields.Datetime.now() - timedelta(days=365) + already_ended = self._make_membership( + self.individual_b, + start_date=past - timedelta(days=1), + ended_date=past, + ) + + repaired, _calls = self._run_cron() + + # An over-matching domain would sweep these rows in; they must not + # be selected at all, not merely end up with unchanged values. + self.assertFalse(repaired & (open_ended | already_ended)) + self.assertEqual(open_ended.status, "active") + self.assertFalse(open_ended.is_ended) + self.assertEqual(already_ended.status, "inactive") + self.assertTrue(already_ended.is_ended) + + def test_cron_invalidates_group_metrics(self): + rec = self._make_membership(self.individual_a) + self._age_row(rec) + + # The recompute flushes through low-level SQL and bypasses write(), + # so the cron must call the metric-invalidation funnel itself. + with _patch_invalidate_funnel(self.env) as funnel: + self._run_cron() + + self.assertTrue(funnel.called) + invalidated = self.env["res.partner"].browse() + for call in funnel.call_args_list: + invalidated |= call.args[0] + self.assertIn(self.group, invalidated) + + def test_cron_drains_backlog_in_batches(self): + carol = self.Partner.create({"name": "Carol", "is_registrant": True, "is_group": False}) + rows = self.Membership.browse() + for individual in (self.individual_a, self.individual_b, carol): + rec = self._make_membership(individual) + self._age_row(rec) + rows |= rec + + repaired, calls = self._run_cron(batch_size=2) + + # One run drains the whole backlog in batch_size chunks, each + # reported (and committed) through _commit_progress. Assertions + # stay scoped to the three rows this test created — ambient stale + # rows on a demo-seeded DB may only add to the totals. + self.assertTrue(all(rec in repaired for rec in rows)) + for rec in rows: + self.assertEqual(rec.status, "inactive") + self.assertTrue(rec.is_ended) + self.assertGreaterEqual(len(calls), 2) + self.assertGreaterEqual(sum(processed for processed, _remaining in calls), len(rows)) + self.assertTrue(all(processed <= 2 for processed, _remaining in calls)) + # A short final chunk reports the backlog as drained. (Guarded: if + # ambient stale rows ever pad the backlog to a multiple of the + # batch size, the last full chunk legitimately reports more work.) + if calls[-1][0] < 2: + self.assertEqual(calls[-1][1], 0) + + def test_cron_resumes_after_time_budget_exhausted(self): + # The production path for any real backlog: _commit_progress + # reports no time left, the run stops after its current chunk with + # the backlog flagged, and the rescheduled run finishes the job. + carol = self.Partner.create({"name": "Carol", "is_registrant": True, "is_group": False}) + rows = self.Membership.browse() + for individual in (self.individual_a, self.individual_b, carol): + rec = self._make_membership(individual) + self._age_row(rec) + rows |= rec + + first, calls = self._run_cron(batch_size=1, time_left=0.0) + + self.assertEqual(len(calls), 1) + self.assertEqual(calls[0][0], 1) + self.assertNotEqual(calls[0][1], 0) # backlog reported as remaining + self.assertEqual(len(first & rows), 1) + + second, _calls = self._run_cron(batch_size=1) + + self.assertEqual((first | second) & rows, rows) + for rec in rows: + self.assertEqual(rec.status, "inactive") + self.assertTrue(rec.is_ended) + + def test_overlapping_legs_do_not_end_run_early(self): + # Rows stale on both columns match the first two legs at once and + # the chunk union de-duplicates, so a chunk shorter than + # batch_size is no proof the backlog is drained — exhaustion must + # be tracked per leg, or rows stale on one column only are left + # behind with the run reported fully done. + status_only = self.Membership.browse() + for name in ("Dave", "Erin"): + individual = self.Partner.create({"name": name, "is_registrant": True, "is_group": False}) + rec = self._make_membership(individual) + self._age_row(rec) + # is_ended already correct; only status is stale (leg 2 only). + self.env.cr.execute( + "UPDATE spp_group_membership SET is_ended = true WHERE id = %s", + (rec.id,), + ) + rec.invalidate_recordset() + status_only |= rec + # Newer ids — searched first under `id desc` — stale on both. + carol = self.Partner.create({"name": "Carol", "is_registrant": True, "is_group": False}) + both_stale = self.Membership.browse() + for individual in (self.individual_a, self.individual_b, carol): + rec = self._make_membership(individual) + self._age_row(rec) + both_stale |= rec + + repaired, calls = self._run_cron(batch_size=4) + + # One run repairs every row: pass 1 fills mostly from leg 1 and + # its leg-2 quota only re-finds chunk rows, so a union-size + # drained check would have stopped here with the status-only rows + # unrepaired until the next daily sweep. + rows = both_stale | status_only + self.assertEqual(repaired & rows, rows) + for rec in rows: + self.assertEqual(self._read_stored_columns(rec), ("inactive", True)) + self.assertEqual(calls[-1][1], 0) + + def test_crossed_cron_repairs_only_index_served_legs(self): + crossed = self._make_membership(self.individual_a) + self._age_row(crossed) + null_row = self._make_membership(self.individual_b) + self.env.flush_all() + self.env.cr.execute( + "UPDATE spp_group_membership SET is_ended = NULL, status = NULL, ended_date = NULL WHERE id = %s", + (null_row.id,), + ) + null_row.invalidate_recordset() + + repaired, _calls = self._run_cron(method="_cron_repair_crossed_ended_status") + + self.assertIn(crossed, repaired) + self.assertEqual(self._read_stored_columns(crossed), ("inactive", True)) + # NULL drift needs full-scan probes; the trigger-driven cron runs + # per departure and must leave those to the daily safety net. + self.assertNotIn(null_row, repaired) + self.assertEqual(self._read_stored_columns(null_row), (None, None)) + + repaired, _calls = self._run_cron() + + self.assertIn(null_row, repaired) + self.assertEqual(self._read_stored_columns(null_row), ("active", False)) + + def _make_null_rows(self): + rows = self.Membership.browse() + for individual in (self.individual_a, self.individual_b): + rows |= self._make_membership(individual) + self.env.flush_all() + self.env.cr.execute( + "UPDATE spp_group_membership SET is_ended = NULL, ended_date = NULL WHERE id IN %s", + (tuple(rows.ids),), + ) + rows.invalidate_recordset() + return rows + + def test_null_repair_drains_in_batches(self): + rows = self._make_null_rows() + + repaired, calls = self._run_cron(batch_size=1) + + self.assertEqual(repaired & rows, rows) + for rec in rows: + self.assertEqual(self._read_stored_columns(rec), ("active", False)) + # Every batch is bounded and reported; the run closes with the + # backlog reported drained. + self.assertTrue(all(processed <= 1 for processed, _remaining in calls)) + self.assertGreaterEqual(sum(processed for processed, _remaining in calls), len(rows)) + self.assertEqual(calls[-1][1], 0) + + def test_null_repair_stops_when_time_budget_exhausted(self): + # A NULL row with no ended_date matches none of the ORM legs, so + # the NULL loop itself must report the backlog: with the budget + # gone after one batch, the run stops flagged partially done and + # is continued ASAP — not a day later on a "fully done" report. + rows = self._make_null_rows() + + first, calls = self._run_cron(batch_size=1, time_left=0.0) + + self.assertEqual(calls, [(1, 1)]) + self.assertEqual(len(first & rows), 1) + + second, _calls = self._run_cron(batch_size=1) + + self.assertEqual((first | second) & rows, rows) + for rec in rows: + self.assertEqual(self._read_stored_columns(rec), ("active", False)) + + def test_future_ended_date_schedules_cron_trigger(self): + cron = self.env.ref("spp_registry.cron_repair_crossed_membership_ended_status") + Trigger = self.env["ir.cron.trigger"] + # Every departure is scheduled in the minute *after* ended_date — + # even a minute-aligned one: the cron machinery consumes triggers + # against the database clock while the repair predicate compares + # the application clock, so an exact-time trigger could be + # consumed a hair before the row reads as ended. + future = fields.Datetime.now().replace(second=0) + timedelta(days=30) + + before = Trigger.search([("cron_id", "=", cron.id)]) + rec = self._make_membership(self.individual_a, ended_date=future) + created = Trigger.search([("cron_id", "=", cron.id)]) - before + self.assertEqual(len(created), 1) + self.assertEqual(created.call_at, future + timedelta(minutes=1)) + + # A mid-minute departure lands in the next full minute as well, + # never scheduled at or before the date itself. + later = future + timedelta(days=5, seconds=30) + before = Trigger.search([("cron_id", "=", cron.id)]) + rec.write({"ended_date": later}) + created = Trigger.search([("cron_id", "=", cron.id)]) - before + self.assertEqual(len(created), 1) + self.assertEqual(created.call_at, later.replace(second=0) + timedelta(minutes=1)) + + def test_default_ended_date_context_schedules_cron_trigger(self): + # default_get fills a missing ended_date from the context; the + # trigger must still be scheduled (create reads the records back, + # not the raw vals). + cron = self.env.ref("spp_registry.cron_repair_crossed_membership_ended_status") + Trigger = self.env["ir.cron.trigger"] + future = fields.Datetime.now().replace(second=0) + timedelta(days=30) + + before = Trigger.search([("cron_id", "=", cron.id)]) + rec = self.Membership.with_context(default_ended_date=future).create( + {"group": self.group.id, "individual": self.individual_a.id} + ) + self.assertEqual(rec.ended_date, future) + created = Trigger.search([("cron_id", "=", cron.id)]) - before + self.assertEqual(len(created), 1) + self.assertEqual(created.call_at, future + timedelta(minutes=1)) + + def test_past_ended_date_schedules_no_cron_trigger(self): + # A past departure is recomputed correctly at write time; only a + # future one needs the clock-crossing repair scheduled. + cron = self.env.ref("spp_registry.cron_repair_crossed_membership_ended_status") + Trigger = self.env["ir.cron.trigger"] + past = fields.Datetime.now() - timedelta(days=365) + + before = Trigger.search([("cron_id", "=", cron.id)]) + rec = self._make_membership( + self.individual_a, + start_date=past - timedelta(days=1), + ended_date=past, + ) + rec.write({"ended_date": past + timedelta(days=1)}) + self.assertFalse(Trigger.search([("cron_id", "=", cron.id)]) - before) + + def test_same_minute_departures_share_one_trigger(self): + # A cohort exit written one membership per call must not file one + # trigger row per membership — pending triggers for the same + # minute are reused (ir.cron._trigger itself never de-duplicates + # across calls). + cron = self.env.ref("spp_registry.cron_repair_crossed_membership_ended_status") + Trigger = self.env["ir.cron.trigger"] + future = fields.Datetime.now().replace(second=0) + timedelta(days=30) + + before = Trigger.search([("cron_id", "=", cron.id)]) + self._make_membership(self.individual_a, ended_date=future) + self._make_membership(self.individual_b, ended_date=future + timedelta(seconds=30)) + created = Trigger.search([("cron_id", "=", cron.id)]) - before + self.assertEqual(len(created), 1) + self.assertEqual(created.call_at, future + timedelta(minutes=1)) + + def test_missing_trigger_cron_degrades_to_daily_sweep(self): + # The trigger cron is a latency optimisation. If the noupdate + # record was deleted by an admin, writing a membership must still + # succeed — the daily sweep repairs the row eventually — rather + # than raising from env.ref on the core registry write path. + self.env.ref("spp_registry.cron_repair_crossed_membership_ended_status").unlink() + future = fields.Datetime.now() + timedelta(days=30) + + rec = self._make_membership(self.individual_a, ended_date=future) + + self.assertEqual(rec.ended_date, future) + + def test_cron_record_registered(self): + # Daily full safety net — the common path is the per-departure + # trigger on the crossed cron below. + cron = self.env.ref("spp_registry.cron_recompute_membership_ended_status") + self.assertEqual(cron.model_id.model, "spp.group.membership") + self.assertTrue(cron.active) + self.assertIn("_cron_recompute_ended_status", cron.code) + self.assertEqual(cron.interval_number, 1) + self.assertEqual(cron.interval_type, "days") + # Trigger target for exact-time repairs: crossed legs only, so a + # per-departure run stays index-served. + crossed = self.env.ref("spp_registry.cron_repair_crossed_membership_ended_status") + self.assertEqual(crossed.model_id.model, "spp.group.membership") + self.assertTrue(crossed.active) + self.assertIn("_cron_repair_crossed_ended_status", crossed.code) + # Superuser pin on both: the unsudo'd searches must see + # memberships of disabled registrants despite the global ir.rule + # pair. + root = self.env.ref("base.user_root") + self.assertEqual(cron.user_id, root) + self.assertEqual(crossed.user_id, root) + + def test_scheduled_sweeps_are_offset(self): + # Both crons sweep the same two crossed legs, and + # `_order = "id desc"` makes their `search(leg, limit=quota)` + # calls return the same rows in the same order. With a shared + # nextcall two cron workers pick the two jobs at once (each + # ir_cron row is taken with FOR NO KEY UPDATE SKIP LOCKED, so + # neither blocks the other) and UPDATE the same ids; under + # Odoo's REPEATABLE READ the run that commits second dies with + # "could not serialize access due to concurrent update" and is + # logged as a failure. The window is widest on the first sweep + # after upgrading a registry with a stale backlog. Their + # scheduled runs must therefore start apart — both being daily, + # the initial offset is preserved on every later run. + daily = self.env.ref("spp_registry.cron_recompute_membership_ended_status") + crossed = self.env.ref("spp_registry.cron_repair_crossed_membership_ended_status") + self.assertGreaterEqual( + abs(daily.nextcall - crossed.nextcall), + timedelta(minutes=30), + )