Skip to content

perf(spp_programs): replace Python uniqueness checks with SQL constraints #116

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kneckinator wants to merge 1 commit into
base: 19.0
Choose a base branch
from
Open

perf(spp_programs): replace Python uniqueness checks with SQL constraints #116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion spp_programs/__manifest__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
"name": "OpenSPP Programs",
"summary": "Manage cash and in-kind entitlements, integrate with inventory, and enhance program management features for comprehensive social protection and agricultural support.",
"category": "OpenSPP/Core",
"version": "19.0.2.0.0",
"version": "19.0.2.1.0",
"sequence": 1,
"author": "OpenSPP.org",
"website": "https://github.com/OpenSPP/OpenSPP2",
Expand Down
90 changes: 90 additions & 0 deletions spp_programs/migrations/19.0.2.1.0/pre-migration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
# Part of OpenSPP. See LICENSE file for full copyright and licensing details.
import logging

_logger = logging.getLogger(__name__)


def migrate(cr, version):
"""Deduplicate data before adding SQL UNIQUE constraints.

Removes duplicate rows using ROW_NUMBER() OVER (PARTITION BY ...),
keeping the earliest record (lowest id) for each unique combination.
"""
if not version:
return

_deduplicate_program_memberships(cr)
_deduplicate_cycle_memberships(cr)
_deduplicate_entitlement_codes(cr)


def _deduplicate_program_memberships(cr):
"""Remove duplicate (partner_id, program_id) rows from spp_program_membership."""
cr.execute(
"""
DELETE FROM spp_program_membership
WHERE id IN (
SELECT id FROM (
SELECT id,
ROW_NUMBER() OVER (
PARTITION BY partner_id, program_id
ORDER BY id
) AS rn
FROM spp_program_membership
) sub
WHERE rn > 1
)
"""
)
if cr.rowcount:
_logger.info("Deduplicated %d duplicate program membership rows", cr.rowcount)


def _deduplicate_cycle_memberships(cr):
"""Remove duplicate (partner_id, cycle_id) rows from spp_cycle_membership."""
cr.execute(
"""
DELETE FROM spp_cycle_membership
WHERE id IN (
SELECT id FROM (
SELECT id,
ROW_NUMBER() OVER (
PARTITION BY partner_id, cycle_id
ORDER BY id
) AS rn
FROM spp_cycle_membership
) sub
WHERE rn > 1
)
"""
)
if cr.rowcount:
_logger.info("Deduplicated %d duplicate cycle membership rows", cr.rowcount)
Comment on lines +21 to +62
Copy link

@gemini-code-assist gemini-code-assist bot Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The functions _deduplicate_program_memberships and _deduplicate_cycle_memberships contain nearly identical SQL logic. To improve maintainability and reduce code duplication, consider refactoring them into a single, generic helper function. This would make the script more concise and provide a reusable pattern for similar deduplication tasks in future migrations.

Here is an example of how you could refactor it:

def _deduplicate_records(cr, table_name, partition_by_cols, log_message):
    """Generic helper to delete duplicate records from a table."""
    # Using .format() for table/column names is safe here as they are not from user input.
    query = """
        DELETE FROM {table}
        WHERE id IN (
            SELECT id FROM (
                SELECT id,
                       ROW_NUMBER() OVER (
                           PARTITION BY {columns}
                           ORDER BY id
                       ) AS rn
                FROM {table}
            ) sub
            WHERE rn > 1
        )
    """.format(table=table_name, columns=', '.join(partition_by_cols))
    cr.execute(query)
    if cr.rowcount:
        _logger.info(log_message, cr.rowcount)

def _deduplicate_program_memberships(cr):
    """Remove duplicate (partner_id, program_id) rows from spp_program_membership."""
    _deduplicate_records(
        cr,
        "spp_program_membership",
        ["partner_id", "program_id"],
        "Deduplicated %d duplicate program membership rows",
    )

def _deduplicate_cycle_memberships(cr):
    """Remove duplicate (partner_id, cycle_id) rows from spp_cycle_membership."""
    _deduplicate_records(
        cr,
        "spp_cycle_membership",
        ["partner_id", "cycle_id"],
        "Deduplicated %d duplicate cycle membership rows",
    )

This approach centralizes the deletion logic, making the script easier to read and maintain.



def _deduplicate_entitlement_codes(cr):
"""Remove duplicate code values from spp_entitlement.

For duplicate codes, regenerates codes for the newer records rather
than deleting them, since entitlements may have financial significance.
"""
cr.execute(
"""
UPDATE spp_entitlement
SET code = code || '-' || id::text
WHERE id IN (
SELECT id FROM (
SELECT id,
ROW_NUMBER() OVER (
PARTITION BY code
ORDER BY id
) AS rn
FROM spp_entitlement
WHERE code IS NOT NULL
) sub
WHERE rn > 1
)
"""
)
if cr.rowcount:
_logger.info("Deduplicated %d entitlement rows with duplicate codes", cr.rowcount)
29 changes: 9 additions & 20 deletions spp_programs/models/cycle_membership.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# Part of OpenSPP. See LICENSE file for full copyright and licensing details.
from odoo import _, api, fields, models
from odoo import _, fields, models
from odoo.exceptions import ValidationError


Expand All @@ -8,6 +8,14 @@ class SPPCycleMembership(models.Model):
_description = "Cycle Membership"
_order = "partner_id asc,id desc"

_sql_constraints = [
(
"unique_partner_cycle",
"UNIQUE(partner_id, cycle_id)",
"Beneficiary must be unique per cycle.",
),
]

partner_id = fields.Many2one("res.partner", "Registrant", help="A beneficiary", required=True, index=True)
cycle_id = fields.Many2one("spp.cycle", "Cycle", help="A cycle", required=True, index=True)
enrollment_date = fields.Date(default=lambda self: fields.Datetime.now())
Expand All @@ -24,25 +32,6 @@ class SPPCycleMembership(models.Model):
copy=False,
)

@api.constrains("partner_id", "cycle_id")
def _check_unique_partner_per_cycle(self):
# Prefetch partner_id and cycle_id to avoid N+1 queries in loop
self.mapped("partner_id")
self.mapped("cycle_id")

for record in self:
if record.partner_id and record.cycle_id:
existing = self.search(
[
("partner_id", "=", record.partner_id.id),
("cycle_id", "=", record.cycle_id.id),
("id", "!=", record.id),
],
limit=1,
)
if existing:
raise ValidationError(_("Beneficiary must be unique per cycle."))

def _compute_display_name(self):
res = super()._compute_display_name()
# Prefetch cycle_id and partner_id to avoid N+1 queries in loop
Expand Down
14 changes: 0 additions & 14 deletions spp_programs/models/entitlement.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,20 +139,6 @@ def _generate_code(self):
payment_status = fields.Selection([("paid", "Paid"), ("notpaid", "Not Paid")], compute="_compute_payment_status")
payment_date = fields.Date(compute="_compute_payment_status")

@api.constrains("code")
def _check_unique_code(self):
for record in self:
if record.code:
existing = self.search(
[
("code", "=", record.code),
("id", "!=", record.id),
],
limit=1,
)
if existing:
raise ValidationError(_("The entitlement code must be unique."))

@api.constrains("valid_from", "valid_until")
def _check_valid_dates(self):
for record in self:
Expand Down
22 changes: 8 additions & 14 deletions spp_programs/models/entitlement_base_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,14 @@ class SPPEntitlement(models.Model):
_order = "partner_id asc,id desc"
_check_company_auto = True

_sql_constraints = [
(
"unique_code",
"UNIQUE(code)",
"Entitlement code must be unique.",
),
]

@api.model
def _generate_code(self):
return str(uuid4())[4:-8][3:]
Expand Down Expand Up @@ -87,20 +95,6 @@ def _generate_code(self):
payment_status = fields.Selection([("paid", "Paid"), ("notpaid", "Not Paid")], compute="_compute_payment_status")
payment_date = fields.Date(compute="_compute_payment_status")

@api.constrains("code")
def _check_unique_code(self):
for record in self:
if record.code:
existing = self.search(
[
("code", "=", record.code),
("id", "!=", record.id),
],
limit=1,
)
if existing:
raise ValidationError(_("The entitlement code must be unique."))

@api.constrains("valid_from", "valid_until")
def _check_valid_dates(self):
for record in self:
Expand Down
29 changes: 9 additions & 20 deletions spp_programs/models/program_membership.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
from lxml import etree

from odoo import _, api, fields, models
from odoo.exceptions import UserError, ValidationError
from odoo.exceptions import UserError

from . import constants

Expand All @@ -19,6 +19,14 @@ class SPPProgramMembership(models.Model):
_inherits = {"res.partner": "partner_id"}
_order = "id desc"

_sql_constraints = [
(
"unique_partner_program",
"UNIQUE(partner_id, program_id)",
"Beneficiary must be unique per program.",
),
]

partner_id = fields.Many2one(
"res.partner",
"Registrant",
Expand Down Expand Up @@ -52,25 +60,6 @@ class SPPProgramMembership(models.Model):

registrant_id = fields.Integer(string="Registrant ID", related="partner_id.id")

@api.constrains("partner_id", "program_id")
def _check_unique_partner_per_program(self):
# Prefetch partner_id and program_id to avoid N+1 queries in loop
self.mapped("partner_id")
self.mapped("program_id")

for record in self:
if record.partner_id and record.program_id:
existing = self.search(
[
("partner_id", "=", record.partner_id.id),
("program_id", "=", record.program_id.id),
("id", "!=", record.id),
],
limit=1,
)
if existing:
raise ValidationError(_("Beneficiary must be unique per program."))

# TODO: Implement exit reasons
# exit_reason_id = fields.Many2one("Exit Reason") Default: Completed, Opt-Out, Other

Expand Down
1 change: 1 addition & 0 deletions spp_programs/tests/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,4 @@
from . import test_eligibility_cel_integration
from . import test_compliance_cel
from . import test_create_program_wizard_cel
from . import test_sql_constraints
Loading
Loading