20907 - Pay outstanding balance switching from EFT payment method #1573
Conversation
|
ochiu
requested review from
seeker25,
Jxio,
rodrigo-barraza and
avni-work
as code owners
| @@ -115,14 +116,49 @@ def payment_methods(self): | |||
|
|
|||
| return list(payment_methods) | |||
|
|
|||
| @hybrid_property | |||
There was a problem hiding this comment.
Have you tested the performance scaling of this property over quite a few statements?
There was a problem hiding this comment.
Removed this from hybrid property and into the service. The hybrid property was no longer suitable due to the circular dependency issues.
Tested this on a data load with an account over 1000 statements at once, seemed performant
| @staticmethod | ||
| def get_statement_owing_query(): | ||
| """Get statement query used for amount owing.""" | ||
| from .statement_invoices import StatementInvoices # pylint: disable=import-outside-toplevel |
There was a problem hiding this comment.
not good, this should be done in a service
There was a problem hiding this comment.
Needed to use this as part of the query existing in the model, I originally tried to do this in the statement service, but
service --> model --> service.
Should I just refactor the model and pull out the statement search up to the service?
There was a problem hiding this comment.
Yes, the models should be fairly simple
| @hybrid_property | ||
| def amount_owing(self): | ||
| """Return amount owing on statement.""" | ||
| from .statement_invoices import StatementInvoices # pylint: disable=import-outside-toplevel |
There was a problem hiding this comment.
not good, this should be done in a service
There was a problem hiding this comment.
removed hybrid property and updated search query
ochiu
force-pushed
the
20907-EFT-Outstanding-Balance
branch
from
137648c to
eb8634a
Compare
| pay_account: PaymentAccountModel, | ||
| invoice_total: Decimal): | ||
| """Create payment for consolidated invoices and update invoice references.""" | ||
| # Create consolidated invoice |
|
|
||
| @classmethod | ||
| def _consolidate_invoices_and_pay(cls, auth_account_id: str) -> Payment: | ||
| # Find outstanding invoices and create a payment |
| ) | ||
| ) | ||
|
|
||
| # Combine current and historical records and calculate the date range for when it was active |
| @@ -271,7 +433,12 @@ def get_summary(auth_account_id: str, statement_id: str = None): | |||
| total_due = float(result.total_due) if result else 0 | |||
| oldest_overdue_date = get_local_formatted_date(result.oldest_overdue_date) \ | |||
| if result and result.oldest_overdue_date else None | |||
|
|
|||
| # Unpaid invoice amount total that are not part of a statement yet | |||
There was a problem hiding this comment.
good comment, not obvious by reading the code
pay-api/setup.cfg
Outdated
| @@ -94,7 +94,8 @@ lines_after_imports = 2 | |||
|
|
|||
|
|
|||
| [tool:pytest] | |||
| addopts = --cov=src --cov-report html:htmlcov --cov-report xml:coverage.xml | |||
| addopts = --verbose | |||
| #--cov=src --cov-report html:htmlcov --cov-report xml:coverage.xml | |||
There was a problem hiding this comment.
might want to revert this? I think I use vscode launch.json / settings.json to turn off codecov
|
|
||
| invoice_number: str = invoice_response.get('invoice_number') | ||
|
|
||
| # Mark all invoice references to status CANCELLED, and create a new one for the new invoice number. |
| @@ -378,6 +378,13 @@ def _check_and_handle_payment_method(cls, account: PaymentAccountModel, target_p | |||
| PaymentMethod.EFT.value not in {account.payment_method, target_payment_method}: | |||
| return | |||
|
|
|||
| # Don't allow payment method change from EFT if there is an outstanding balance | |||
There was a problem hiding this comment.
redundant? can read the code below?
Issue #:
bcgov/entity#20907
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the sbc-pay license (Apache 2.0).