-
Notifications
You must be signed in to change notification settings - Fork 40
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
17534 - New NSF resource implementation #1341
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1341 +/- ##
==========================================
+ Coverage 91.45% 91.90% +0.44%
==========================================
Files 186 206 +20
Lines 11319 12607 +1288
==========================================
+ Hits 10352 11586 +1234
- Misses 967 1021 +54
Flags with carried forward coverage won't be shown. Click here to find out more.
|
queue_services/payment-reconciliations/src/reconciliations/payment_reconciliations.py
Outdated
Show resolved
Hide resolved
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.
see above
also:
Missing migration
Unit tests not passing
Code quality issue sonar cloud
Title of the PR isn't clear what you're doing
|
||
def asdict(self): | ||
"""Return the Non-Sufficient Funds as a python dict.""" | ||
non_sufficient_funds_schema = NonSufficientFundsSchema() |
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.
CATTRS
assert 'total_amount' in find_non_sufficient_funds | ||
assert 'total_amount_remaining' in find_non_sufficient_funds | ||
assert 'nsf_amount' in find_non_sufficient_funds | ||
assert 'total' in find_non_sufficient_funds |
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.
Check for the expected values as well. Might be useful to do multiple line items since there is totaling logic.
"""Return all Non-Sufficient Funds invoices.""" | ||
results, total, aggregate_totals = NonSufficientFundsService.query_all_non_sufficient_funds_invoices( | ||
account_id=account_id) | ||
invoice_schema = InvoiceSchema(exclude=('receipts', 'references', '_links')) |
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.
any reason why we can't use InvoiceSearchModel
or a variation of it instead?
"""Create Non-Sufficient Funds statement pdf.""" | ||
current_app.logger.debug('<generate_non_sufficient_funds_statement_pdf') | ||
invoice = NonSufficientFundsService.find_all_non_sufficient_funds_invoices(account_id=account_id) | ||
cfs_account: CfsAccountModel = CfsAccountModel.find_by_account_id(account_id) |
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 returns a list of cfs_accounts
, not just one - should probably be looking for the one with the highest ID / newest perhaps?
|
||
template_vars = { | ||
'suspendedOn': datetime.strptime(account['suspendedOn'], '%Y-%m-%dT%H:%M:%S%z').strftime('%B %-d, %Y'), | ||
'accountNumber': cfs_account[0].cfs_account, |
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.
not ideal, you need to choose the correct one
'templateVars': template_vars, | ||
'populatePageNumber': True | ||
} | ||
current_app.logger.info('Invoice PDF Dict %s', invoice_pdf_dict) |
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.
should this be debug? not info?
@@ -121,5 +123,5 @@ def from_row(cls, row: PaymentLineItem): | |||
|
|||
https://www.attrs.org/en/stable/init.html | |||
""" | |||
return cls(gst=row.gst, pst=row.pst, description=row.description, | |||
filing_type_code=row.fee_schedule.filing_type_code) | |||
return cls(total=row.total, gst=row.gst, pst=row.pst, service_fees=row.service_fees, |
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.
Does this change the CSO endpoint? if it does, we'll have to warn them
" # NOTE CSO uses this model for reconciliation, so it needs to follow the spec fairly closely.
#
sbc-pay/docs/docs/api_contract/pay-api-1.0.3.yaml
Line 1591 in e722e68
Invoice: |
"
Ahhh good they're already in the spec - probably should have them test their apps in TEST when this is pushed to TEST
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
Covered by #1377 |
Issue #:
https://github.com/bcgov/entity/issues/
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).