-
Notifications
You must be signed in to change notification settings - Fork 1
perf(spp_programs): batch create entitlements and payments #128
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.
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
base: 19.0
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -211,42 +211,55 @@ def _prepare_payments(self, cycle, entitlements): | |
| entitlements -= tag_entitlements | ||
| max_batch_size = batch_tag.max_batch_size | ||
|
|
||
| for i, entitlement_id in enumerate(tag_entitlements): | ||
| payment = self.env["spp.payment"].create( | ||
| if not tag_entitlements: | ||
| continue | ||
|
|
||
| # Prefetch bank_ids to avoid N+1 queries | ||
| tag_entitlements.mapped("partner_id.bank_ids.acc_number") | ||
|
|
||
| # Build all payment vals in one pass | ||
| payment_vals_list = [] | ||
| for entitlement_id in tag_entitlements: | ||
| account_number = None | ||
| if entitlement_id.partner_id.bank_ids: | ||
| account_number = entitlement_id.partner_id.bank_ids[0].acc_number | ||
| payment_vals_list.append( | ||
| { | ||
| "name": str(uuid4()), | ||
| "entitlement_id": entitlement_id.id, | ||
| "cycle_id": entitlement_id.cycle_id.id, | ||
| "amount_issued": entitlement_id.initial_amount, | ||
| "payment_fee": entitlement_id.transfer_fee, | ||
| "state": "issued", | ||
| "account_number": account_number, | ||
| } | ||
| ) | ||
|
Comment on lines
+221
to
236
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic for building payment_vals_list = [
{
"name": str(uuid4()),
"entitlement_id": ent.id,
"cycle_id": ent.cycle_id.id,
"amount_issued": ent.initial_amount,
"payment_fee": ent.transfer_fee,
"state": "issued",
"account_number": ent.partner_id.bank_ids[0].acc_number if ent.partner_id.bank_ids else None,
}
for ent in tag_entitlements
] |
||
| if payment.partner_id.bank_ids: | ||
| payment.account_number = payment.partner_id.bank_ids[0].acc_number | ||
| else: | ||
| payment.account_number = None | ||
|
|
||
| if not payments: | ||
| payments = payment | ||
| else: | ||
| payments += payment | ||
| if create_batch: | ||
| if i % max_batch_size == 0: | ||
| curr_batch = self.env["spp.payment.batch"].create( | ||
| { | ||
| "name": str(uuid4()), | ||
| "cycle_id": cycle.id, | ||
| "stats_datetime": fields.Datetime.now(), | ||
| "tag_id": batch_tag.id, | ||
| } | ||
| ) | ||
| if not batches: | ||
| batches = curr_batch | ||
| else: | ||
| batches += curr_batch | ||
| curr_batch.payment_ids = [(4, payment.id)] | ||
| payment.batch_id = curr_batch | ||
| # Batch create all payments for this tag | ||
| tag_payments = self.env["spp.payment"].create(payment_vals_list) | ||
|
|
||
| if not payments: | ||
| payments = tag_payments | ||
| else: | ||
| payments += tag_payments | ||
|
|
||
| if create_batch: | ||
| # Assign payments to batches in chunks of max_batch_size | ||
| for i in range(0, len(tag_payments), max_batch_size): | ||
| batch_payments = tag_payments[i : i + max_batch_size] | ||
| curr_batch = self.env["spp.payment.batch"].create( | ||
| { | ||
| "name": str(uuid4()), | ||
| "cycle_id": cycle.id, | ||
| "stats_datetime": fields.Datetime.now(), | ||
| "tag_id": batch_tag.id, | ||
| } | ||
| ) | ||
| batch_payments.write({"batch_id": curr_batch.id}) | ||
| if not batches: | ||
| batches = curr_batch | ||
| else: | ||
| batches += curr_batch | ||
| return payments, batches | ||
|
|
||
| def _prepare_payments_async(self, cycle, entitlements, entitlements_count): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,172 @@ | ||
| # Part of OpenSPP. See LICENSE file for full copyright and licensing details. | ||
| import uuid | ||
| from unittest.mock import patch | ||
|
|
||
| from odoo import fields | ||
| from odoo.tests import TransactionCase | ||
|
|
||
|
|
||
| class TestBatchEntitlementCreation(TransactionCase): | ||
| """Test that cash entitlement manager creates entitlements in a single | ||
| batch call instead of one-by-one.""" | ||
|
|
||
| def setUp(self): | ||
| super().setUp() | ||
| self.program = self.env["spp.program"].create({"name": f"Test Program {uuid.uuid4().hex[:8]}"}) | ||
| self.journal = self.env["account.journal"].create( | ||
| { | ||
| "name": "Test Journal", | ||
| "type": "bank", | ||
| "code": f"TJ{uuid.uuid4().hex[:4].upper()}", | ||
| } | ||
| ) | ||
| self.program.journal_id = self.journal.id | ||
|
|
||
| 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.manager = self.env["spp.program.entitlement.manager.cash"].create( | ||
| { | ||
| "name": "Test Cash Manager", | ||
| "program_id": self.program.id, | ||
| } | ||
| ) | ||
| self.env["spp.program.entitlement.manager.cash.item"].create( | ||
| { | ||
| "entitlement_id": self.manager.id, | ||
| "amount": 100.0, | ||
| } | ||
| ) | ||
|
|
||
| # Create beneficiaries with cycle memberships | ||
| self.registrants = self.env["res.partner"] | ||
| self.memberships = self.env["spp.cycle.membership"] | ||
| for i in range(5): | ||
| reg = self.env["res.partner"].create({"name": f"Registrant {i}", "is_registrant": True}) | ||
| self.registrants |= reg | ||
| self.memberships |= self.env["spp.cycle.membership"].create( | ||
| { | ||
| "partner_id": reg.id, | ||
| "cycle_id": self.cycle.id, | ||
| "state": "enrolled", | ||
| } | ||
| ) | ||
|
|
||
| def test_cash_manager_batch_creates_entitlements(self): | ||
| """Cash entitlement manager must call create() at most once | ||
| (batch), not once per beneficiary.""" | ||
| original_create = type(self.env["spp.entitlement"]).create | ||
|
|
||
| call_count = 0 | ||
| total_created = 0 | ||
|
|
||
| def counting_create(self_model, vals_list): | ||
| nonlocal call_count, total_created | ||
| call_count += 1 | ||
| if isinstance(vals_list, list): | ||
| total_created += len(vals_list) | ||
| else: | ||
| total_created += 1 | ||
| return original_create(self_model, vals_list) | ||
|
|
||
| with patch.object( | ||
| type(self.env["spp.entitlement"]), | ||
| "create", | ||
| counting_create, | ||
| ): | ||
| self.manager.prepare_entitlements(self.cycle, self.memberships) | ||
|
|
||
| self.assertEqual( | ||
| call_count, | ||
| 1, | ||
| f"create() should be called once (batch), was called {call_count} times", | ||
| ) | ||
| self.assertEqual(total_created, 5) | ||
|
|
||
|
|
||
| class TestBatchPaymentCreation(TransactionCase): | ||
| """Test that payment manager creates payments in a single batch call | ||
| instead of one-by-one.""" | ||
|
|
||
| def setUp(self): | ||
| super().setUp() | ||
| self.program = self.env["spp.program"].create({"name": f"Test Program {uuid.uuid4().hex[:8]}"}) | ||
| self.journal = self.env["account.journal"].create( | ||
| { | ||
| "name": "Test Journal", | ||
| "type": "bank", | ||
| "code": f"TJ{uuid.uuid4().hex[:4].upper()}", | ||
| } | ||
| ) | ||
| self.program.journal_id = self.journal.id | ||
|
|
||
| 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.payment_manager = self.env["spp.program.payment.manager.default"].create( | ||
| { | ||
| "name": "Test Payment Manager", | ||
| "program_id": self.program.id, | ||
| "create_batch": False, | ||
| } | ||
| ) | ||
|
|
||
| # Create approved entitlements | ||
| self.entitlements = self.env["spp.entitlement"] | ||
| for i in range(5): | ||
| reg = self.env["res.partner"].create({"name": f"Registrant {i}", "is_registrant": True}) | ||
| self.entitlements |= self.env["spp.entitlement"].create( | ||
| { | ||
| "partner_id": reg.id, | ||
| "cycle_id": self.cycle.id, | ||
| "initial_amount": 100.0, | ||
| "state": "approved", | ||
| "is_cash_entitlement": True, | ||
| } | ||
| ) | ||
|
|
||
| def test_payment_manager_batch_creates_payments(self): | ||
| """Payment manager must call create() at most once per batch tag | ||
| (batch), not once per entitlement.""" | ||
| original_create = type(self.env["spp.payment"]).create | ||
|
|
||
| call_count = 0 | ||
|
|
||
| def counting_create(self_model, vals_list): | ||
| nonlocal call_count | ||
| call_count += 1 | ||
| return original_create(self_model, vals_list) | ||
|
|
||
| # Add a batch tag so we enter the loop | ||
| batch_tag = self.env["spp.payment.batch.tag"].create( | ||
| { | ||
| "name": "Test Tag", | ||
| "order": 1, | ||
| "domain": "[]", | ||
| "max_batch_size": 500, | ||
| } | ||
| ) | ||
| self.payment_manager.batch_tag_ids = [(4, batch_tag.id)] | ||
|
|
||
| with patch.object( | ||
| type(self.env["spp.payment"]), | ||
| "create", | ||
| counting_create, | ||
| ): | ||
| self.payment_manager._prepare_payments(self.cycle, self.entitlements) | ||
|
|
||
| self.assertEqual( | ||
| call_count, | ||
| 1, | ||
| f"create() should be called once (batch), was called {call_count} times", | ||
| ) | ||
|
Comment on lines
+143
to
+172
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For better test coverage and consistency with call_count = 0
total_created = 0
def counting_create(self_model, vals_list):
nonlocal call_count, total_created
call_count += 1
if isinstance(vals_list, list):
total_created += len(vals_list)
else:
total_created += 1
return original_create(self_model, vals_list)
# Add a batch tag so we enter the loop
batch_tag = self.env["spp.payment.batch.tag"].create(
{
"name": "Test Tag",
"order": 1,
"domain": "[]",
"max_batch_size": 500,
}
)
self.payment_manager.batch_tag_ids = [(4, batch_tag.id)]
with patch.object(
type(self.env["spp.payment"]),
"create",
counting_create,
):
self.payment_manager._prepare_payments(self.cycle, self.entitlements)
self.assertEqual(
call_count,
1,
f"create() should be called once (batch), was called {call_count} times",
)
self.assertEqual(total_created, 5) |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the logic is correct, the loop can be made more direct and Pythonic by iterating over the dictionary's values instead of its keys. This improves readability by removing the extra dictionary lookup
new_entitlements_to_create[ent]in each iteration.