-
Notifications
You must be signed in to change notification settings - Fork 1
perf(spp_programs): fetch fund balance once per approval batch #127
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -406,13 +406,16 @@ def approve_entitlements(self, entitlements): | |||||
| entitlements.mapped("partner_id.property_account_payable_id") | ||||||
| entitlements.mapped("journal_id.currency_id") | ||||||
|
|
||||||
| # Fetch fund balance once for the whole batch instead of per entitlement | ||||||
| fund_balance = self.check_fund_balance(entitlements[0].cycle_id.program_id.id) | ||||||
|
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. This line will raise an
Suggested change
|
||||||
|
|
||||||
| state_err = 0 | ||||||
| message = "" | ||||||
| sw = 0 | ||||||
| for rec in entitlements: | ||||||
| if rec.state in ("draft", "pending_validation"): | ||||||
| fund_balance = self.check_fund_balance(rec.cycle_id.program_id.id) - amt | ||||||
| if fund_balance >= rec.initial_amount: | ||||||
| remaining_balance = fund_balance - amt | ||||||
| if remaining_balance >= rec.initial_amount: | ||||||
| amt += rec.initial_amount | ||||||
| # Prepare journal entry (account.move) via account.payment | ||||||
| amount = rec.initial_amount | ||||||
|
|
@@ -459,7 +462,7 @@ def approve_entitlements(self, entitlements): | |||||
| + "is insufficient for the entitlement: %(entitlement)s" | ||||||
| ) % { | ||||||
| "program": rec.cycle_id.program_id.name, | ||||||
| "fund": fund_balance, | ||||||
| "fund": remaining_balance, | ||||||
| "entitlement": rec.code, | ||||||
| } | ||||||
| # Stop the process and return an error | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,150 @@ | ||
| # 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 TestFundBalanceOptimization(TransactionCase): | ||
| """Test that fund balance is fetched once per approval batch, not per entitlement. | ||
|
|
||
| The approve_entitlements methods previously called check_fund_balance() | ||
| (2 SQL queries) inside the per-entitlement loop. Now the balance is | ||
| fetched once and tracked via a running total in Python. | ||
| """ | ||
|
Comment on lines
+9
to
+15
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. It's great that you've added tests for this optimization. However, there's a critical edge case that is not covered: when You could add a test like this: def test_approve_entitlements_with_empty_list(self):
"""approve_entitlements should not fail with an empty list of entitlements."""
manager = self._create_default_manager()
# For DefaultCashEntitlementManager
state_err, message = manager.approve_entitlements(self.env["spp.entitlement"])
self.assertEqual(state_err, 0)
self.assertEqual(message, "")
# For SppCashEntitlementManager
cash_manager = self._create_cash_manager()
state_err, message = cash_manager.approve_entitlements(self.env["spp.entitlement"])
self.assertEqual(state_err, 0)
self.assertEqual(message, "") |
||
|
|
||
| def setUp(self): | ||
| super().setUp() | ||
| self.program = self.env["spp.program"].create({"name": f"Test Program {uuid.uuid4().hex[:8]}"}) | ||
| # Create a journal for the program | ||
| 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.registrants = self.env["res.partner"] | ||
| for i in range(5): | ||
| self.registrants |= self.env["res.partner"].create({"name": f"Registrant {i}", "is_registrant": True}) | ||
|
|
||
| def _create_default_manager(self): | ||
| return self.env["spp.program.entitlement.manager.default"].create( | ||
| { | ||
| "name": "Test Default Manager", | ||
| "program_id": self.program.id, | ||
| "amount_per_cycle": 100.0, | ||
| } | ||
| ) | ||
|
|
||
| def _create_cash_manager(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": manager.id, | ||
| "amount": 100.0, | ||
| } | ||
| ) | ||
| return manager | ||
|
|
||
| def _create_entitlements(self, count=5, amount=100.0): | ||
| """Create multiple entitlements in pending_validation state.""" | ||
| entitlements = self.env["spp.entitlement"] | ||
| for i in range(count): | ||
| entitlements |= self.env["spp.entitlement"].create( | ||
| { | ||
| "partner_id": self.registrants[i].id, | ||
| "cycle_id": self.cycle.id, | ||
| "initial_amount": amount, | ||
| "state": "pending_validation", | ||
| "is_cash_entitlement": True, | ||
| } | ||
| ) | ||
| return entitlements | ||
|
|
||
| def test_default_manager_calls_check_fund_balance_once(self): | ||
| """DefaultCashEntitlementManager.approve_entitlements must call | ||
| check_fund_balance at most once per batch.""" | ||
| manager = self._create_default_manager() | ||
| entitlements = self._create_entitlements(count=3) | ||
|
|
||
| with patch.object( | ||
| type(manager), | ||
| "check_fund_balance", | ||
| wraps=manager.check_fund_balance, | ||
| ) as mock_check: | ||
| # Set fund balance high enough to approve all | ||
| mock_check.return_value = 10000.0 | ||
| manager.approve_entitlements(entitlements) | ||
| self.assertEqual( | ||
| mock_check.call_count, | ||
| 1, | ||
| f"check_fund_balance should be called exactly once, was called {mock_check.call_count} times", | ||
| ) | ||
|
|
||
| def test_cash_manager_calls_check_fund_balance_once(self): | ||
| """SppCashEntitlementManager.approve_entitlements must call | ||
| check_fund_balance at most once per batch.""" | ||
| manager = self._create_cash_manager() | ||
| entitlements = self._create_entitlements(count=3) | ||
|
|
||
| with patch.object( | ||
| type(manager), | ||
| "check_fund_balance", | ||
| wraps=manager.check_fund_balance, | ||
| ) as mock_check: | ||
| mock_check.return_value = 10000.0 | ||
| manager.approve_entitlements(entitlements) | ||
| self.assertEqual( | ||
| mock_check.call_count, | ||
| 1, | ||
| f"check_fund_balance should be called exactly once, was called {mock_check.call_count} times", | ||
| ) | ||
|
|
||
| def test_fund_balance_insufficient_stops_early(self): | ||
| """When fund runs out mid-batch, remaining entitlements are not approved.""" | ||
| manager = self._create_default_manager() | ||
| entitlements = self._create_entitlements(count=3, amount=100.0) | ||
|
|
||
| with patch.object( | ||
| type(manager), | ||
| "check_fund_balance", | ||
| return_value=250.0, | ||
| ): | ||
| state_err, message = manager.approve_entitlements(entitlements) | ||
| self.assertEqual(state_err, 1) | ||
| self.assertIn("insufficient", message) | ||
|
|
||
| def test_fund_balance_running_total_correct(self): | ||
| """Running total must correctly track cumulative approved amounts.""" | ||
| manager = self._create_default_manager() | ||
| entitlements = self._create_entitlements(count=3, amount=100.0) | ||
|
|
||
| with patch.object( | ||
| type(manager), | ||
| "check_fund_balance", | ||
| return_value=300.0, | ||
| ): | ||
| state_err, _message = manager.approve_entitlements(entitlements) | ||
| self.assertEqual(state_err, 0) | ||
| # All 3 should be approved (300 fund, 3x100 = 300) | ||
| for ent in entitlements: | ||
| ent.invalidate_recordset(["state"]) | ||
| self.assertEqual(ent.state, "approved") | ||
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.
This line will raise an
IndexErrorifentitlementsis an empty list. You should handle this case to prevent a crash. A simple way is to use a conditional expression to avoid accessing the list if it's empty.