From c1f3cb86a806fc193035b7c7774bb02896673bf7 Mon Sep 17 00:00:00 2001 From: Ken Lewerentz Date: Thu, 2 Apr 2026 15:00:15 +0700 Subject: [PATCH 1/4] perf: replace OFFSET pagination with ID-range batching in async jobs OFFSET N causes PostgreSQL to scan and discard N rows, making later batches progressively slower. This replaces all async job dispatchers with NTILE-based ID-range batching that uses WHERE id BETWEEN min_id AND max_id, which is O(1) via the primary key index. --- spp_programs/models/cycle.py | 16 +- spp_programs/models/managers/cycle_manager.py | 10 +- .../models/managers/cycle_manager_base.py | 46 ++- .../models/managers/pagination_utils.py | 63 +++ .../models/managers/program_manager.py | 28 +- spp_programs/models/programs.py | 16 +- spp_programs/tests/__init__.py | 1 + spp_programs/tests/test_keyset_pagination.py | 388 ++++++++++++++++++ 8 files changed, 541 insertions(+), 27 deletions(-) create mode 100644 spp_programs/models/managers/pagination_utils.py create mode 100644 spp_programs/tests/test_keyset_pagination.py diff --git a/spp_programs/models/cycle.py b/spp_programs/models/cycle.py index 3cf0b6f5..d4ae4351 100644 --- a/spp_programs/models/cycle.py +++ b/spp_programs/models/cycle.py @@ -614,7 +614,9 @@ def _get_beneficiaries_domain(self, states=None): return domain @api.model - def get_beneficiaries(self, state, offset=0, limit=None, order=None, count=False, last_id=None): + def get_beneficiaries( + self, state, offset=0, limit=None, order=None, count=False, last_id=None, min_id=None, max_id=None + ): """ Get beneficiaries by state with pagination support. @@ -624,9 +626,12 @@ def get_beneficiaries(self, state, offset=0, limit=None, order=None, count=False :param order: Sort order :param count: If True, return count instead of records :param last_id: For cursor-based pagination - ID of last record from previous batch (more efficient) + :param min_id: For ID-range pagination - minimum record ID (inclusive) + :param max_id: For ID-range pagination - maximum record ID (inclusive) :return: Recordset or count - Note: For large datasets, use cursor-based pagination with last_id parameter instead of offset. + Note: For large datasets, prefer min_id/max_id (ID-range) or last_id (cursor) + pagination over offset-based pagination. """ if isinstance(state, str): state = [state] @@ -635,7 +640,12 @@ def get_beneficiaries(self, state, offset=0, limit=None, order=None, count=False if count: return self.env["spp.cycle.membership"].search_count(domain, limit=limit) - # Use cursor-based pagination if last_id is provided (more efficient) + # ID-range pagination (best for parallel job dispatch) + if min_id is not None and max_id is not None: + domain = domain + [("id", ">=", min_id), ("id", "<=", max_id)] + return self.env["spp.cycle.membership"].search(domain, order=order or "id") + + # Cursor-based pagination (good for sequential iteration) if last_id is not None: domain = domain + [("id", ">", last_id)] return self.env["spp.cycle.membership"].search(domain, limit=limit, order=order or "id") diff --git a/spp_programs/models/managers/cycle_manager.py b/spp_programs/models/managers/cycle_manager.py index 66f08349..c46c7534 100644 --- a/spp_programs/models/managers/cycle_manager.py +++ b/spp_programs/models/managers/cycle_manager.py @@ -23,17 +23,19 @@ def mark_prepare_entitlement_as_done(self, cycle, msg): cycle._compute_total_entitlements_count() return - def _prepare_entitlements(self, cycle, offset=0, limit=None, do_count=False): + def _prepare_entitlements(self, cycle, offset=0, limit=None, min_id=None, max_id=None, do_count=False): """Prepare Entitlements Get the beneficiaries and generate their entitlements. :param cycle: The cycle - :param offset: Optional integer value for the ORM search offset - :param limit: Optional integer value for the ORM search limit + :param offset: Optional integer value for the ORM search offset (deprecated, use min_id/max_id) + :param limit: Optional integer value for the ORM search limit (deprecated, use min_id/max_id) + :param min_id: Minimum record ID for ID-range pagination (inclusive) + :param max_id: Maximum record ID for ID-range pagination (inclusive) :param do_count: Boolean - set to False to not run compute function :return: """ - super()._prepare_entitlements(cycle, offset, limit, do_count) + super()._prepare_entitlements(cycle, offset, limit, min_id=min_id, max_id=max_id, do_count=do_count) if do_count: # Update Statistics cycle._compute_inkind_entitlements_count() diff --git a/spp_programs/models/managers/cycle_manager_base.py b/spp_programs/models/managers/cycle_manager_base.py index 11c161d1..22e6a0b1 100644 --- a/spp_programs/models/managers/cycle_manager_base.py +++ b/spp_programs/models/managers/cycle_manager_base.py @@ -11,6 +11,7 @@ from odoo.addons.job_worker.delay import group from .. import constants +from .pagination_utils import compute_id_ranges _logger = logging.getLogger(__name__) @@ -515,21 +516,32 @@ def _check_eligibility_async(self, cycle, beneficiaries_count): cycle.message_post(body=_("Eligibility check of %s beneficiaries started.", beneficiaries_count)) cycle.write({"is_locked": True, "locked_reason": "Eligibility check of beneficiaries"}) + states = ("draft", "enrolled", "not_eligible") + id_ranges = compute_id_ranges( + self.env.cr, + "spp_cycle_membership", + "cycle_id = %s AND state IN %s", + (cycle.id, states), + self.MAX_ROW_JOB_QUEUE, + ) + jobs = [] - for i in range(0, beneficiaries_count, self.MAX_ROW_JOB_QUEUE): - jobs.append( - self.delayable(channel="cycle")._check_eligibility(cycle, offset=i, limit=self.MAX_ROW_JOB_QUEUE) - ) + for min_id, max_id in id_ranges: + jobs.append(self.delayable(channel="cycle")._check_eligibility(cycle, min_id=min_id, max_id=max_id)) main_job = group(*jobs) main_job.on_done(self.delayable(channel="cycle").mark_check_eligibility_as_done(cycle)) main_job.delay() - def _check_eligibility(self, cycle, beneficiaries=None, offset=0, limit=None, do_count=False): + def _check_eligibility( + self, cycle, beneficiaries=None, offset=0, limit=None, min_id=None, max_id=None, do_count=False + ): if beneficiaries is None: beneficiaries = cycle.get_beneficiaries( ["draft", "enrolled", "not_eligible"], offset=offset, limit=limit, + min_id=min_id, + max_id=max_id, order="id", ) @@ -585,26 +597,38 @@ def _prepare_entitlements_async(self, cycle, beneficiaries_count): } ) + id_ranges = compute_id_ranges( + self.env.cr, + "spp_cycle_membership", + "cycle_id = %s AND state IN %s", + (cycle.id, ("enrolled",)), + self.MAX_ROW_JOB_QUEUE, + ) + jobs = [] - for i in range(0, beneficiaries_count, self.MAX_ROW_JOB_QUEUE): - jobs.append(self.delayable(channel="cycle")._prepare_entitlements(cycle, i, self.MAX_ROW_JOB_QUEUE)) + for min_id, max_id in id_ranges: + jobs.append(self.delayable(channel="cycle")._prepare_entitlements(cycle, min_id=min_id, max_id=max_id)) main_job = group(*jobs) main_job.on_done( self.delayable(channel="cycle").mark_prepare_entitlement_as_done(cycle, _("Entitlement Ready.")) ) main_job.delay() - def _prepare_entitlements(self, cycle, offset=0, limit=None, do_count=False): + def _prepare_entitlements(self, cycle, offset=0, limit=None, min_id=None, max_id=None, do_count=False): """Prepare Entitlements Get the beneficiaries and generate their entitlements. :param cycle: The cycle - :param offset: Optional integer value for the ORM search offset - :param limit: Optional integer value for the ORM search limit + :param offset: Optional integer value for the ORM search offset (deprecated, use min_id/max_id) + :param limit: Optional integer value for the ORM search limit (deprecated, use min_id/max_id) + :param min_id: Minimum record ID for ID-range pagination (inclusive) + :param max_id: Maximum record ID for ID-range pagination (inclusive) :param do_count: Boolean - set to False to not run compute function :return: """ - beneficiaries = cycle.get_beneficiaries(["enrolled"], offset=offset, limit=limit, order="id") + beneficiaries = cycle.get_beneficiaries( + ["enrolled"], offset=offset, limit=limit, min_id=min_id, max_id=max_id, order="id" + ) ent_manager = self.program_id.get_manager(constants.MANAGER_ENTITLEMENT) if not ent_manager: raise UserError(_("No Entitlement Manager defined.")) diff --git a/spp_programs/models/managers/pagination_utils.py b/spp_programs/models/managers/pagination_utils.py new file mode 100644 index 00000000..6052160d --- /dev/null +++ b/spp_programs/models/managers/pagination_utils.py @@ -0,0 +1,63 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Keyset pagination utilities for async job dispatch. + +OFFSET-based pagination causes PostgreSQL to scan and discard N rows for +OFFSET N, making later batches progressively slower (O(N) per batch). + +This module provides ID-range batching using the NTILE window function, +which pre-computes (min_id, max_id) boundaries in a single query. Each +job then uses WHERE id BETWEEN min_id AND max_id, which is O(1) via the +primary key index regardless of batch position. +""" + +import math + + +def compute_id_ranges(cr, table, where_clause, params, batch_size): + """Compute ID-range boundaries for parallel job dispatch. + + Uses PostgreSQL's NTILE window function to split matching rows into + roughly equal-sized buckets, then returns the (min_id, max_id) of each. + + :param cr: Database cursor + :param table: Table name (e.g. 'spp_program_membership') + :param where_clause: SQL WHERE clause without 'WHERE' keyword + (e.g. 'program_id = %s AND state IN %s') + :param params: Tuple of parameters for the WHERE clause + :param batch_size: Target number of rows per batch + :return: List of (min_id, max_id) tuples, ordered by min_id + """ + # Get total count to calculate number of batches + cr.execute( + f"SELECT COUNT(*) FROM {table} WHERE {where_clause}", # noqa: S608 # nosec B608 + params, + ) + total = cr.fetchone()[0] + if total == 0: + return [] + + num_batches = math.ceil(total / batch_size) + if num_batches <= 1: + cr.execute( + f"SELECT MIN(id), MAX(id) FROM {table} WHERE {where_clause}", # noqa: S608 # nosec B608 + params, + ) + row = cr.fetchone() + return [(row[0], row[1])] + + # Use NTILE to split rows into equal-sized buckets, then get + # the min/max ID of each bucket as the range boundaries. + cr.execute( + f""" + SELECT MIN(id) AS min_id, MAX(id) AS max_id + FROM ( + SELECT id, NTILE(%s) OVER (ORDER BY id) AS tile + FROM {table} + WHERE {where_clause} + ) sub + GROUP BY tile + ORDER BY min_id + """, # noqa: S608 # nosec B608 + (num_batches, *params), + ) + return cr.fetchall() diff --git a/spp_programs/models/managers/program_manager.py b/spp_programs/models/managers/program_manager.py index eccb41a2..e309e360 100644 --- a/spp_programs/models/managers/program_manager.py +++ b/spp_programs/models/managers/program_manager.py @@ -8,6 +8,7 @@ from odoo.addons.job_worker.delay import group from ..programs import SPPProgram +from .pagination_utils import compute_id_ranges _logger = logging.getLogger(__name__) @@ -184,28 +185,43 @@ def _enroll_eligible_registrants_async(self, states, members_count): program.message_post(body=_("Eligibility check of %s beneficiaries started.", members_count)) program.write({"is_locked": True, "locked_reason": "Eligibility check of beneficiaries"}) + if isinstance(states, str): + states = [states] + + id_ranges = compute_id_ranges( + self.env.cr, + "spp_program_membership", + "program_id = %s AND state IN %s", + (program.id, tuple(states)), + self.MAX_ROW_JOB_QUEUE, + ) + jobs = [] - for i in range(0, members_count, self.MAX_ROW_JOB_QUEUE): + for min_id, max_id in id_ranges: jobs.append( self.delayable(channel="program_manager")._enroll_eligible_registrants( - states, i, self.MAX_ROW_JOB_QUEUE + states, min_id=min_id, max_id=max_id ) ) main_job = group(*jobs) main_job.on_done(self.delayable(channel="program_manager").mark_enroll_eligible_as_done()) main_job.delay() - def _enroll_eligible_registrants(self, states, offset=0, limit=None, do_count=False): + def _enroll_eligible_registrants(self, states, offset=0, limit=None, min_id=None, max_id=None, do_count=False): """Enroll Eligible Registrants :param states: List of states to be used in domain filter - :param offset: Optional integer value for the ORM search offset - :param limit: Optional integer value for the ORM search limit + :param offset: Optional integer value for the ORM search offset (deprecated, use min_id/max_id) + :param limit: Optional integer value for the ORM search limit (deprecated, use min_id/max_id) + :param min_id: Minimum record ID for ID-range pagination (inclusive) + :param max_id: Maximum record ID for ID-range pagination (inclusive) :param do_count: Boolean - set to False to not run compute functions :return: Integer - count of not enrolled members """ program = self.program_id - members = program.get_beneficiaries(state=states, offset=offset, limit=limit, order="id") + members = program.get_beneficiaries( + state=states, offset=offset, limit=limit, min_id=min_id, max_id=max_id, order="id" + ) member_before = members diff --git a/spp_programs/models/programs.py b/spp_programs/models/programs.py index e086c6d2..0aa25ec9 100644 --- a/spp_programs/models/programs.py +++ b/spp_programs/models/programs.py @@ -314,7 +314,9 @@ def get_managers(self, kind): return [el.manager_ref_id for el in managers] @api.model - def get_beneficiaries(self, state=None, offset=0, limit=None, order=None, count=False, last_id=None): + def get_beneficiaries( + self, state=None, offset=0, limit=None, order=None, count=False, last_id=None, min_id=None, max_id=None + ): """ Get program beneficiaries with pagination support. @@ -324,9 +326,12 @@ def get_beneficiaries(self, state=None, offset=0, limit=None, order=None, count= :param order: Sort order :param count: If True, return count instead of records :param last_id: For cursor-based pagination - ID of last record from previous batch (more efficient) + :param min_id: For ID-range pagination - minimum record ID (inclusive) + :param max_id: For ID-range pagination - maximum record ID (inclusive) :return: Recordset or count - Note: For large datasets, use cursor-based pagination with last_id parameter instead of offset. + Note: For large datasets, prefer min_id/max_id (ID-range) or last_id (cursor) + pagination over offset-based pagination. """ self.ensure_one() if isinstance(state, str): @@ -337,7 +342,12 @@ def get_beneficiaries(self, state=None, offset=0, limit=None, order=None, count= if count: return self.env["spp.program.membership"].search_count(domain, limit=limit) - # Use cursor-based pagination if last_id is provided (more efficient) + # ID-range pagination (best for parallel job dispatch) + if min_id is not None and max_id is not None: + domain = domain + [("id", ">=", min_id), ("id", "<=", max_id)] + return self.env["spp.program.membership"].search(domain, order=order or "id") + + # Cursor-based pagination (good for sequential iteration) if last_id is not None: domain = domain + [("id", ">", last_id)] return self.env["spp.program.membership"].search(domain, limit=limit, order=order or "id") diff --git a/spp_programs/tests/__init__.py b/spp_programs/tests/__init__.py index 344574d9..acb863ea 100644 --- a/spp_programs/tests/__init__.py +++ b/spp_programs/tests/__init__.py @@ -33,3 +33,4 @@ from . import test_managers from . import test_cycle_auto_approve_fund_check from . import test_bulk_membership +from . import test_keyset_pagination diff --git a/spp_programs/tests/test_keyset_pagination.py b/spp_programs/tests/test_keyset_pagination.py new file mode 100644 index 00000000..f4b2c5b0 --- /dev/null +++ b/spp_programs/tests/test_keyset_pagination.py @@ -0,0 +1,388 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Tests for Phase 6: ID-based keyset pagination. + +These tests verify that async job dispatch uses ID-range batching (via NTILE) +instead of OFFSET-based pagination. OFFSET N causes PostgreSQL to scan N rows +then discard them, making later batches O(N) slower. +""" + +import uuid +from unittest.mock import patch + +from odoo import fields +from odoo.tests import TransactionCase + + +class TestComputeIdRanges(TransactionCase): + """Test the compute_id_ranges helper function.""" + + def setUp(self): + super().setUp() + self.program = self.env["spp.program"].create({"name": f"Test Program {uuid.uuid4().hex[:8]}"}) + + def _create_memberships(self, count): + """Create program memberships and return their IDs sorted.""" + partners = self.env["res.partner"].create( + [{"name": f"Registrant {i}", "is_registrant": True} for i in range(count)] + ) + memberships = self.env["spp.program.membership"].create( + [ + { + "partner_id": p.id, + "program_id": self.program.id, + "state": "draft", + } + for p in partners + ] + ) + return sorted(memberships.ids) + + def test_compute_id_ranges_returns_covering_ranges(self): + """All records must be covered by exactly one range.""" + from ..models.managers.pagination_utils import ( + compute_id_ranges, + ) + + ids = self._create_memberships(10) + ranges = compute_id_ranges( + self.env.cr, + "spp_program_membership", + "program_id = %s AND state IN %s", + (self.program.id, tuple(["draft"])), + batch_size=3, + ) + # Every original ID should fall within exactly one range + covered = set() + for min_id, max_id in ranges: + covered.update(i for i in ids if min_id <= i <= max_id) + self.assertEqual(covered, set(ids), "All IDs must be covered by ranges") + + def test_compute_id_ranges_batch_count(self): + """Number of ranges should be ceil(total / batch_size).""" + from ..models.managers.pagination_utils import ( + compute_id_ranges, + ) + + self._create_memberships(10) + ranges = compute_id_ranges( + self.env.cr, + "spp_program_membership", + "program_id = %s AND state IN %s", + (self.program.id, tuple(["draft"])), + batch_size=3, + ) + # 10 records / batch_size 3 = ceil(10/3) = 4 ranges + self.assertEqual(len(ranges), 4) + + def test_compute_id_ranges_single_batch(self): + """When total <= batch_size, return a single range.""" + from ..models.managers.pagination_utils import ( + compute_id_ranges, + ) + + ids = self._create_memberships(3) + ranges = compute_id_ranges( + self.env.cr, + "spp_program_membership", + "program_id = %s AND state IN %s", + (self.program.id, tuple(["draft"])), + batch_size=10, + ) + self.assertEqual(len(ranges), 1) + self.assertEqual(ranges[0], (min(ids), max(ids))) + + def test_compute_id_ranges_empty_table(self): + """Empty result set should return empty list.""" + from ..models.managers.pagination_utils import ( + compute_id_ranges, + ) + + ranges = compute_id_ranges( + self.env.cr, + "spp_program_membership", + "program_id = %s AND state IN %s", + (self.program.id, tuple(["draft"])), + batch_size=10, + ) + self.assertEqual(ranges, []) + + def test_compute_id_ranges_no_overlap(self): + """Ranges must not overlap (each ID in exactly one range).""" + from ..models.managers.pagination_utils import ( + compute_id_ranges, + ) + + self._create_memberships(20) + ranges = compute_id_ranges( + self.env.cr, + "spp_program_membership", + "program_id = %s AND state IN %s", + (self.program.id, tuple(["draft"])), + batch_size=5, + ) + for i in range(len(ranges) - 1): + self.assertLess( + ranges[i][1], + ranges[i + 1][0], + f"Range {i} max_id must be less than range {i + 1} min_id", + ) + + +class TestGetBeneficiariesIdRange(TransactionCase): + """Test min_id/max_id support in get_beneficiaries().""" + + def setUp(self): + super().setUp() + self.program = self.env["spp.program"].create({"name": f"Test Program {uuid.uuid4().hex[:8]}"}) + partners = self.env["res.partner"].create( + [{"name": f"Registrant {i}", "is_registrant": True} for i in range(10)] + ) + self.memberships = self.env["spp.program.membership"].create( + [ + { + "partner_id": p.id, + "program_id": self.program.id, + "state": "draft", + } + for p in partners + ] + ) + self.sorted_ids = sorted(self.memberships.ids) + + # Also create a cycle for cycle-level tests + self.cycle = self.env["spp.cycle"].create( + { + "name": "Test Cycle", + "program_id": self.program.id, + "start_date": fields.Date.today(), + "end_date": fields.Date.today(), + } + ) + self.cycle_memberships = self.env["spp.cycle.membership"].create( + [ + { + "partner_id": p.id, + "cycle_id": self.cycle.id, + "state": "draft", + } + for p in partners + ] + ) + self.cycle_sorted_ids = sorted(self.cycle_memberships.ids) + + def test_program_get_beneficiaries_with_id_range(self): + """get_beneficiaries with min_id/max_id returns only records in range.""" + mid = self.sorted_ids[4] # 5th record + end = self.sorted_ids[7] # 8th record + result = self.program.get_beneficiaries(state="draft", min_id=mid, max_id=end) + result_ids = sorted(result.ids) + expected = [i for i in self.sorted_ids if mid <= i <= end] + self.assertEqual(result_ids, expected) + + def test_program_get_beneficiaries_id_range_no_offset(self): + """min_id/max_id should not use offset internally.""" + # If offset were used, we'd get wrong results + result = self.program.get_beneficiaries( + state="draft", + min_id=self.sorted_ids[0], + max_id=self.sorted_ids[-1], + ) + self.assertEqual(len(result), 10) + + def test_cycle_get_beneficiaries_with_id_range(self): + """Cycle get_beneficiaries with min_id/max_id returns only records in range.""" + mid = self.cycle_sorted_ids[3] + end = self.cycle_sorted_ids[6] + result = self.cycle.get_beneficiaries(state="draft", min_id=mid, max_id=end) + result_ids = sorted(result.ids) + expected = [i for i in self.cycle_sorted_ids if mid <= i <= end] + self.assertEqual(result_ids, expected) + + +class TestAsyncDispatchUsesIdRanges(TransactionCase): + """Verify async dispatch methods use ID ranges, not OFFSET.""" + + def setUp(self): + super().setUp() + self.program = self.env["spp.program"].create({"name": f"Test Program {uuid.uuid4().hex[:8]}"}) + self.cycle = self.env["spp.cycle"].create( + { + "name": "Test Cycle", + "program_id": self.program.id, + "start_date": fields.Date.today(), + "end_date": fields.Date.today(), + } + ) + + def test_enroll_eligible_async_uses_compute_id_ranges(self): + """_enroll_eligible_registrants_async must use compute_id_ranges for dispatch.""" + partners = self.env["res.partner"].create( + [{"name": f"Registrant {i}", "is_registrant": True} for i in range(10)] + ) + self.env["spp.program.membership"].create( + [ + { + "partner_id": p.id, + "program_id": self.program.id, + "state": "draft", + } + for p in partners + ] + ) + + manager = self.env["spp.program.manager.default"].create( + { + "name": "Test Manager", + "program_id": self.program.id, + } + ) + + # Verify compute_id_ranges is called by the async method + with patch( + "odoo.addons.spp_programs.models.managers.program_manager.compute_id_ranges", + wraps=None, + return_value=[(1, 5), (6, 10)], + ) as mock_ranges: + # Also patch delayable to avoid actual job creation + with patch.object(type(manager), "delayable", return_value=manager): + try: + manager._enroll_eligible_registrants_async(["draft"], 10) + except Exception: # pylint: disable=except-pass + pass + + mock_ranges.assert_called_once() + call_args = mock_ranges.call_args + # Verify it was called with the right table + self.assertEqual(call_args[0][1], "spp_program_membership") + + def test_enroll_eligible_registrants_accepts_id_range(self): + """_enroll_eligible_registrants must accept min_id/max_id params.""" + partners = self.env["res.partner"].create( + [{"name": f"Registrant {i}", "is_registrant": True} for i in range(5)] + ) + memberships = self.env["spp.program.membership"].create( + [ + { + "partner_id": p.id, + "program_id": self.program.id, + "state": "draft", + } + for p in partners + ] + ) + sorted_ids = sorted(memberships.ids) + + manager = self.env["spp.program.manager.default"].create( + { + "name": "Test Manager", + "program_id": self.program.id, + } + ) + + # Create a simple eligibility manager that passes everyone through + elig_manager = self.env["spp.program.membership.manager.default"].create( + { + "name": "Test Elig Manager", + "program_id": self.program.id, + } + ) + self.env["spp.eligibility.manager"].create( + { + "program_id": self.program.id, + "manager_ref_id": f"spp.program.membership.manager.default,{elig_manager.id}", + } + ) + + # Call with min_id/max_id - should only process records in range + mid = sorted_ids[1] + end = sorted_ids[3] + manager._enroll_eligible_registrants(["draft"], min_id=mid, max_id=end) + # Should have enrolled records in range + in_range = [i for i in sorted_ids if mid <= i <= end] + enrolled = self.env["spp.program.membership"].browse(in_range).filtered(lambda m: m.state == "enrolled") + self.assertEqual(len(enrolled), len(in_range)) + + def test_check_eligibility_accepts_id_range(self): + """_check_eligibility must accept min_id/max_id params.""" + partners = self.env["res.partner"].create( + [{"name": f"Registrant {i}", "is_registrant": True} for i in range(5)] + ) + cycle_memberships = self.env["spp.cycle.membership"].create( + [ + { + "partner_id": p.id, + "cycle_id": self.cycle.id, + "state": "draft", + } + for p in partners + ] + ) + sorted_ids = sorted(cycle_memberships.ids) + + # Create eligibility manager + elig_manager = self.env["spp.program.membership.manager.default"].create( + { + "name": "Test Elig Manager", + "program_id": self.program.id, + } + ) + self.env["spp.eligibility.manager"].create( + { + "program_id": self.program.id, + "manager_ref_id": f"spp.program.membership.manager.default,{elig_manager.id}", + } + ) + + cycle_manager = self.env["spp.cycle.manager.default"].create( + { + "name": "Test Cycle Manager", + "program_id": self.program.id, + } + ) + + # Call with min_id/max_id + mid = sorted_ids[1] + end = sorted_ids[3] + count = cycle_manager._check_eligibility(self.cycle, min_id=mid, max_id=end) + # Should have processed only records in range + in_range = [i for i in sorted_ids if mid <= i <= end] + self.assertEqual(count, len(in_range)) + + def test_prepare_entitlements_accepts_id_range(self): + """_prepare_entitlements must accept min_id/max_id params.""" + partners = self.env["res.partner"].create( + [{"name": f"Registrant {i}", "is_registrant": True} for i in range(5)] + ) + self.env["spp.cycle.membership"].create( + [ + { + "partner_id": p.id, + "cycle_id": self.cycle.id, + "state": "enrolled", + } + for p in partners + ] + ) + + cycle_manager = self.env["spp.cycle.manager.default"].create( + { + "name": "Test Cycle Manager", + "program_id": self.program.id, + } + ) + + # Verify the method accepts min_id/max_id without TypeError. + # UserError is expected since no entitlement manager is configured. + from odoo.exceptions import UserError + + try: + cycle_manager._prepare_entitlements( + self.cycle, + min_id=0, + max_id=999999999, + ) + except TypeError as e: + if "min_id" in str(e) or "max_id" in str(e): + self.fail("_prepare_entitlements must accept min_id/max_id params") + except UserError: + pass # Expected: no entitlement manager configured From c16fb3191995c1a018059bfc99a1294ac460f3b8 Mon Sep 17 00:00:00 2001 From: Ken Lewerentz Date: Thu, 2 Apr 2026 15:09:17 +0700 Subject: [PATCH 2/4] fix: guard against race condition in compute_id_ranges Handle the unlikely case where records are deleted between the COUNT and MIN/MAX queries, which would cause a TypeError on None values. --- spp_programs/models/managers/pagination_utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spp_programs/models/managers/pagination_utils.py b/spp_programs/models/managers/pagination_utils.py index 6052160d..b7878046 100644 --- a/spp_programs/models/managers/pagination_utils.py +++ b/spp_programs/models/managers/pagination_utils.py @@ -43,7 +43,7 @@ def compute_id_ranges(cr, table, where_clause, params, batch_size): params, ) row = cr.fetchone() - return [(row[0], row[1])] + return [(row[0], row[1])] if row and row[0] is not None else [] # Use NTILE to split rows into equal-sized buckets, then get # the min/max ID of each bucket as the range boundaries. From 81b4a5adc3e0ae46d73dd2474e3989ca006aa964 Mon Sep 17 00:00:00 2001 From: Ken Lewerentz Date: Thu, 2 Apr 2026 15:13:14 +0700 Subject: [PATCH 3/4] test: add coverage for async dispatch methods Cover the _check_eligibility_async, _prepare_entitlements_async, and the isinstance(states, str) branch in _enroll_eligible_registrants_async. --- spp_programs/tests/test_keyset_pagination.py | 111 +++++++++++++++++++ 1 file changed, 111 insertions(+) diff --git a/spp_programs/tests/test_keyset_pagination.py b/spp_programs/tests/test_keyset_pagination.py index f4b2c5b0..9d05a84b 100644 --- a/spp_programs/tests/test_keyset_pagination.py +++ b/spp_programs/tests/test_keyset_pagination.py @@ -386,3 +386,114 @@ def test_prepare_entitlements_accepts_id_range(self): self.fail("_prepare_entitlements must accept min_id/max_id params") except UserError: pass # Expected: no entitlement manager configured + + def test_check_eligibility_async_uses_compute_id_ranges(self): + """_check_eligibility_async must use compute_id_ranges for dispatch.""" + partners = self.env["res.partner"].create( + [{"name": f"Registrant {i}", "is_registrant": True} for i in range(5)] + ) + self.env["spp.cycle.membership"].create( + [ + { + "partner_id": p.id, + "cycle_id": self.cycle.id, + "state": "draft", + } + for p in partners + ] + ) + + cycle_manager = self.env["spp.cycle.manager.default"].create( + { + "name": "Test Cycle Manager", + "program_id": self.program.id, + } + ) + + with patch( + "odoo.addons.spp_programs.models.managers.cycle_manager_base.compute_id_ranges", + return_value=[(1, 3), (4, 6)], + ) as mock_ranges: + with patch.object(type(cycle_manager), "delayable", return_value=cycle_manager): + try: + cycle_manager._check_eligibility_async(self.cycle, 5) + except Exception: # pylint: disable=except-pass + pass + + mock_ranges.assert_called_once() + self.assertEqual(mock_ranges.call_args[0][1], "spp_cycle_membership") + + def test_prepare_entitlements_async_uses_compute_id_ranges(self): + """_prepare_entitlements_async must use compute_id_ranges for dispatch.""" + partners = self.env["res.partner"].create( + [{"name": f"Registrant {i}", "is_registrant": True} for i in range(5)] + ) + self.env["spp.cycle.membership"].create( + [ + { + "partner_id": p.id, + "cycle_id": self.cycle.id, + "state": "enrolled", + } + for p in partners + ] + ) + + cycle_manager = self.env["spp.cycle.manager.default"].create( + { + "name": "Test Cycle Manager", + "program_id": self.program.id, + } + ) + + with patch( + "odoo.addons.spp_programs.models.managers.cycle_manager_base.compute_id_ranges", + return_value=[(1, 3), (4, 6)], + ) as mock_ranges: + with patch.object(type(cycle_manager), "delayable", return_value=cycle_manager): + try: + cycle_manager._prepare_entitlements_async(self.cycle, 5) + except Exception: # pylint: disable=except-pass + pass + + mock_ranges.assert_called_once() + self.assertEqual(mock_ranges.call_args[0][1], "spp_cycle_membership") + + def test_enroll_eligible_async_handles_string_state(self): + """_enroll_eligible_registrants_async must handle string state arg.""" + partners = self.env["res.partner"].create( + [{"name": f"Registrant {i}", "is_registrant": True} for i in range(5)] + ) + self.env["spp.program.membership"].create( + [ + { + "partner_id": p.id, + "program_id": self.program.id, + "state": "draft", + } + for p in partners + ] + ) + + manager = self.env["spp.program.manager.default"].create( + { + "name": "Test Manager", + "program_id": self.program.id, + } + ) + + # Pass a string instead of list — the isinstance branch should convert it + with patch( + "odoo.addons.spp_programs.models.managers.program_manager.compute_id_ranges", + return_value=[(1, 5)], + ) as mock_ranges: + with patch.object(type(manager), "delayable", return_value=manager): + try: + manager._enroll_eligible_registrants_async("draft", 5) + except Exception: # pylint: disable=except-pass + pass + + mock_ranges.assert_called_once() + # Verify the states param was converted from string to tuple + call_params = mock_ranges.call_args[0][3] + self.assertIsInstance(call_params[1], tuple) From b805be1e33677c92bcf8a428bd415d9b1535cbcd Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 17 Apr 2026 10:31:41 +0800 Subject: [PATCH 4/4] docs: bump version to 19.0.2.0.8, add changelog for ID-range pagination --- spp_programs/README.rst | 10 +++++++++ spp_programs/__manifest__.py | 2 +- spp_programs/readme/HISTORY.md | 6 ++++++ spp_programs/static/description/index.html | 25 ++++++++++++++++------ 4 files changed, 35 insertions(+), 8 deletions(-) diff --git a/spp_programs/README.rst b/spp_programs/README.rst index 891c63fa..61b3d267 100644 --- a/spp_programs/README.rst +++ b/spp_programs/README.rst @@ -254,6 +254,16 @@ Dependencies Changelog ========= +19.0.2.0.8 +~~~~~~~~~~ + +- Replace OFFSET pagination with NTILE-based ID-range batching in all + async job dispatchers +- Add ``compute_id_ranges()`` utility using PostgreSQL NTILE window + function +- Add ``min_id``/``max_id`` support to ``get_beneficiaries()`` on + program and cycle + 19.0.2.0.7 ~~~~~~~~~~ diff --git a/spp_programs/__manifest__.py b/spp_programs/__manifest__.py index f84a56ae..1de62a0c 100644 --- a/spp_programs/__manifest__.py +++ b/spp_programs/__manifest__.py @@ -4,7 +4,7 @@ "name": "OpenSPP Programs", "summary": "Manage programs, cycles, beneficiary enrollment, entitlements (cash and in-kind), payments, and fund tracking for social protection.", "category": "OpenSPP/Core", - "version": "19.0.2.0.7", + "version": "19.0.2.0.8", "sequence": 1, "author": "OpenSPP.org", "website": "https://github.com/OpenSPP/OpenSPP2", diff --git a/spp_programs/readme/HISTORY.md b/spp_programs/readme/HISTORY.md index f7990cef..2e335856 100644 --- a/spp_programs/readme/HISTORY.md +++ b/spp_programs/readme/HISTORY.md @@ -1,3 +1,9 @@ +### 19.0.2.0.8 + +- Replace OFFSET pagination with NTILE-based ID-range batching in all async job dispatchers +- Add `compute_id_ranges()` utility using PostgreSQL NTILE window function +- Add `min_id`/`max_id` support to `get_beneficiaries()` on program and cycle + ### 19.0.2.0.7 - Bulk membership creation using raw SQL INSERT ON CONFLICT DO NOTHING for program and cycle memberships diff --git a/spp_programs/static/description/index.html b/spp_programs/static/description/index.html index 8006ba87..a11c13ed 100644 --- a/spp_programs/static/description/index.html +++ b/spp_programs/static/description/index.html @@ -658,6 +658,17 @@

Changelog

+

19.0.2.0.8

+
    +
  • Replace OFFSET pagination with NTILE-based ID-range batching in all +async job dispatchers
    • +
  • Add compute_id_ranges() utility using PostgreSQL NTILE window +function
    • +
  • Add min_id/max_id support to get_beneficiaries() on +program and cycle
    • +
+
+

19.0.2.0.7

  • Bulk membership creation using raw SQL INSERT ON CONFLICT DO NOTHING @@ -666,7 +677,7 @@

    19.0.2.0.7

    _add_beneficiaries with bulk SQL path
-
+

19.0.2.0.6

  • Remove unused entitlement_base_model.py (dead code, never imported)
    • @@ -675,34 +686,34 @@

    19.0.2.0.6

    payment, and fund tests (172 → 492 tests)
-
+

19.0.2.0.5

  • Batch create entitlements and payments instead of one-by-one ORM creates
-
+

19.0.2.0.4

  • Fetch fund balance once per approval batch instead of per entitlement
-
+

19.0.2.0.3

  • Replace cycle computed fields (total_amount, entitlements_count, approval flags) with SQL aggregation queries
-
+

19.0.2.0.2

  • Add composite indexes for frequent query patterns on entitlements and program memberships
-
+

19.0.2.0.1

  • Replace Python-level uniqueness checks with SQL UNIQUE constraints for @@ -711,7 +722,7 @@

    19.0.2.0.1

    constraint creation
-
+

19.0.2.0.0

  • Initial migration to OpenSPP2