Skip to content
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 - nsf tweaks #1385

Merged
merged 10 commits into from
Jan 24, 2024
Merged

17534 - nsf tweaks #1385

merged 10 commits into from
Jan 24, 2024

Conversation

rodrigo-barraza
Copy link
Collaborator

Adding cfs_account_id field to non_sufficient_funds, and adding the CSV reconciliation description to non_sufficient_funds

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).

Adding cfs_account_id field to non_sufficient_funds, and adding the CSV reconciliation description to non_sufficient_funds
Copy link

codecov bot commented Jan 23, 2024

Codecov Report

Attention: 115 lines in your changes are missing coverage. Please review.

Comparison is base (79924ce) 91.45% compared to head (85d6dbe) 91.98%.
Report is 83 commits behind head on main.

❗ Current head 85d6dbe differs from pull request most recent head eb28cd2. Consider uploading reports for the commit eb28cd2 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1385      +/-   ##
==========================================
+ Coverage   91.45%   91.98%   +0.52%     
==========================================
  Files         186      190       +4     
  Lines       11319    12437    +1118     
==========================================
+ Hits        10352    11440    +1088     
- Misses        967      997      +30     
Flag Coverage Δ
bcolapi ?
eventlistenerqueue 82.00% <100.00%> (+0.18%) ⬆️
payadmin ∅ <ø> (?)
paymentjobs 83.35% <94.64%> (+3.13%) ⬆️
paymentreconciliationsqueue 92.38% <93.89%> (+0.93%) ⬆️
reportapi ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
jobs/payment-jobs/tasks/common/dataclasses.py 100.00% <100.00%> (ø)
jobs/payment-jobs/tasks/common/enums.py 100.00% <ø> (ø)
jobs/payment-jobs/tasks/distribution_task.py 97.70% <100.00%> (-0.06%) ⬇️
...ayment-jobs/tasks/ejv_partner_distribution_task.py 99.17% <100.00%> (+0.03%) ⬆️
jobs/payment-jobs/tasks/ejv_payment_task.py 96.61% <100.00%> (+0.14%) ⬆️
.../payment-jobs/tasks/statement_notification_task.py 79.74% <100.00%> (+47.92%) ⬆️
jobs/payment-jobs/tasks/statement_task.py 91.39% <100.00%> (+10.96%) ⬆️
pay-api/src/pay_api/config.py 99.35% <100.00%> (+<0.01%) ⬆️
pay-api/src/pay_api/models/__init__.py 100.00% <100.00%> (ø)
pay-api/src/pay_api/models/custom_query.py 100.00% <100.00%> (ø)
... and 68 more

... and 23 files with indirect coverage changes

@rodrigo-barraza rodrigo-barraza changed the title nsf tweaks 17534 - nsf tweaks Jan 23, 2024
@@ -55,15 +57,16 @@ class NonSufficientFundsSchema: # pylint: disable=too-few-public-methods
"""Used to search for NSF records."""

id: int
description: str
cfs_account: int
invoice_id: int
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still need invoice id?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could do away with it if there is no need for OPS to have this information about a specific invoice, and we're currently still using it on the DB queries (but could query around it).

Copy link
Collaborator

@seeker25 seeker25 Jan 24, 2024

Choose a reason for hiding this comment

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

I think it's a bit confusing to leave it in there (or at least as invoice_id), because the NSF pertains to multiple invoice ids (grouped up by the invoice_number) and you're not creating multiple rows.. Maybe we could use a better name for it instead?
nsf_fee_invoice_id? that way it's distinct from the other invoice_ids that are NSF

Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@seeker25 seeker25 marked this pull request as ready for review January 24, 2024 16:51
@seeker25 seeker25 requested review from Jxio and ochiu as code owners January 24, 2024 16:51
@seeker25 seeker25 merged commit 88faa50 into main Jan 24, 2024
16 of 21 checks passed
@seeker25 seeker25 deleted the nsf_tweaks branch July 31, 2024 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants