18099 - PAY API - endpoint to add EFT Allowed flag to an account #1307

Jxio · 2023-10-25T22:10:15Z

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

codecov · 2023-10-25T22:15:11Z

Codecov Report

Merging #1307 (5b9735f) into main (79924ce) will increase coverage by 0.56%.
Report is 30 commits behind head on main.
The diff coverage is 95.58%.

@@            Coverage Diff             @@
##             main    #1307      +/-   ##
==========================================
+ Coverage   91.45%   92.02%   +0.56%     
==========================================
  Files         186      199      +13     
  Lines       11319    11750     +431     
==========================================
+ Hits        10352    10813     +461     
+ Misses        967      937      -30

Flag	Coverage Δ
eventlistenerqueue	`82.00% <100.00%> (+0.18%)`	⬆️
payadmin	`∅ <ø> (?)`
payapi	`93.76% <94.54%> (+0.03%)`	⬆️
paymentjobs	`83.51% <93.10%> (+3.28%)`	⬆️
paymentreconciliationsqueue	`92.71% <97.50%> (+1.26%)`	⬆️

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

Files	Coverage Δ
.../payment-jobs/tasks/statement_notification_task.py	`79.74% <100.00%> (+47.92%)`	⬆️
pay-api/src/pay_api/models/__init__.py	`100.00% <100.00%> (ø)`
pay-api/src/pay_api/models/eft_file.py	`100.00% <100.00%> (ø)`
...-api/src/pay_api/models/eft_process_status_code.py	`100.00% <100.00%> (ø)`
pay-api/src/pay_api/models/eft_short_names.py	`100.00% <100.00%> (ø)`
pay-api/src/pay_api/models/eft_transaction.py	`100.00% <100.00%> (ø)`
pay-api/src/pay_api/models/invoice.py	`100.00% <100.00%> (ø)`
pay-api/src/pay_api/models/payment.py	`95.87% <ø> (ø)`
pay-api/src/pay_api/models/payment_account.py	`97.72% <100.00%> (+0.05%)`	⬆️
pay-api/src/pay_api/models/statement.py	`97.67% <100.00%> (+0.05%)`	⬆️
... and 23 more

... and 7 files with indirect coverage changes

seeker25 · 2023-10-25T22:26:14Z

Shouldn't this be an endpoint you can use to enable EFT on a user's account? Aka they can't choose EFT until someone goes into the backend and flips the switch via notebook?

enabled_eft probably should just be in the payment_accounts model

a resource should be built that can be called via notebook that sets enable_eft - if enable_eft is enabled on their payment account, it should show or hide the extra options in the payment methods? etc

seeker25 · 2023-10-25T22:29:35Z

@ochiu

FAILED tests/integration/test_worker_queue.py::test_update_credit_payment - sqlalchemy.exc.StatementError: (builtins.KeyError) 'LEGISLATIVE_TIMEZONE'

ochiu · 2023-10-25T22:39:10Z

@ochiu

FAILED tests/integration/test_worker_queue.py::test_update_credit_payment - sqlalchemy.exc.StatementError: (builtins.KeyError) 'LEGISLATIVE_TIMEZONE'

@Jxio @seeker25 caused by a recent change to the invoice model to have a default for the overdue_date and it uses the current_local_time() function requiring the LEGISLATIVE_TIMEZONE.

@Jxio would you be able to add the below to the config.py for the event_listener in this PR please? If preferred I can also submit another PR. Just let me know thanks!

LEGISLATIVE_TIMEZONE = os.getenv('LEGISLATIVE_TIMEZONE', 'America/Vancouver')

seeker25 · 2023-10-25T22:42:10Z

I figured as much

Jxio · 2023-10-25T22:43:36Z

@seeker25

I probably got misunderstanding on this ticket. Is it not a flag just indicate if eft allowed? -> if an account had EFT invoices historically, then it allows to have EFT payment option?

seeker25 · 2023-10-25T22:44:49Z

Yeah but we don't want to enable eft for every client only the 300 in the list mostly. Thus why we need a flag to enable or disable on an account basis.

queue_services/events-listener/src/events_listener/config.py

seeker25 · 2023-10-25T23:06:50Z

Add enable_eft to datamodel for payment_accounts
This flag should control if the EFT payment method can be switched to in the payment method screen
Case by case basis, so a route needs to be built that a notebook can be ran to enable EFT to peoples accounts, payload org_id probably
When this flag is added.. it should trigger a welcome email.. so it needs to fire down a queue message and call the account mailer

ochiu · 2023-10-27T15:21:21Z

pay-api/migrations/versions/2023_10_26_2ef58b39cafc_add_eft_enable_to_payment_accounts.py

+
+def upgrade():
+    op.execute("set statement_timeout=20000;")
+    op.add_column('payment_accounts', sa.Column('eft_enable', sa.Boolean, nullable=True))


We can probably default the new flag to false and make it not nullable.

ok sounds good

ochiu · 2023-10-27T15:24:57Z

pay-api/migrations/versions/2023_10_26_7d5231f2b9ac_update_eft_enable_payment_accounts.py

+    # eft_enable set to true when it has invoices paid with EFT/DIRECT_PAY payment method historically
+    op.execute("set statement_timeout=20000;")
+    conn = op.get_bind()
+    conn.execute('UPDATE payment_accounts \


I don't think this is necessary, we want to enable specific accounts via notebook. It will also give us flexibility to enable a subset of accounts if we want and we want to send a welcome email when we enable it for accounts.

A new route/endpoint is beneficial as we can secure it differently than the existing payment account endpoints and have separate logic like the welcome email when we enable this flag.

I removed this file, it gonna handle by ticket 18100

seeker25 · 2023-10-27T20:26:17Z

lint + unit tests before merging pls

seeker25 · 2023-10-27T20:36:24Z

pay-api/src/pay_api/services/payment_account.py

@@ -564,6 +577,7 @@ def asdict(self, **kwargs):
        # to handle PAD 3 day period..UI needs bank details even if PAD is not activated
        is_future_pad = (self.payment_method == PaymentMethod.DRAWDOWN.value) and (self._is_pad_in_pending_activation())
        show_cfs_details = is_ob_or_pad or is_future_pad
+        d['eft_enable'] = self._dao.eft_enable


I don't think you need this? because you've added it up here:

I tested with postman, when the eft_enable is false, it doesn't show up. I'm removing eft_enable = fields.Boolean(...)

Might be able to mess with something in here -^

seeker25 · 2023-10-27T20:37:30Z

Something else I noticed, what about the route?

Jxio · 2023-10-27T20:42:31Z

Something else I noticed, what about the route?

a separate route? I don't think we need it. it can be passed with payment-accounts route:

seeker25 · 2023-10-27T20:44:08Z

That's the GET, we still need a PATCH route so we can set ENABLE_EFT, I see routes for GET, POST, PUT

It needs to check for the SYSTEM role

seeker25 · 2023-10-30T15:27:13Z

pay-api/src/pay_api/services/payment_account.py

+    @classmethod
+    def enable_eft(cls, auth_account_id: str) -> PaymentAccount:
+        """Enable EFT on the payment account."""
+        current_app.logger.debug('<patch_account')


This isn't part of patch account, this is enable_eft - patch account calls enable_eft

seeker25 · 2023-10-30T15:31:35Z

pay-api/src/pay_api/resources/v1/account.py

+    except ServiceUnavailableException as exception:
+        return exception.response()
+
+    status = HTTPStatus.ACCEPTED \


Why would this be ACCEPTED or OK? I don't see you providing any option to disable eft

I'd just return OK

pay-api/src/pay_api/resources/v1/account.py

pay-api/tests/unit/services/test_payment_account.py

sonarcloud · 2023-10-30T17:15:28Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

add eft flag to "get payment account route"

d9cc82d

Jxio added the enhancement New feature or request label Oct 25, 2023

Jxio self-assigned this Oct 25, 2023

Jxio requested review from seeker25 and ochiu as code owners October 25, 2023 22:10

fix event_listener issue

7b1257d

ochiu reviewed Oct 25, 2023

View reviewed changes

queue_services/events-listener/src/events_listener/config.py Outdated Show resolved Hide resolved

fix queue_listener issue

cbdfefc

add eft_enable to payment_accounts datamodel

cadaf7c

ochiu reviewed Oct 27, 2023

View reviewed changes

make eft_enable non-nullable

baf1678

seeker25 approved these changes Oct 27, 2023

View reviewed changes

lint error fix

7699601

seeker25 reviewed Oct 27, 2023

View reviewed changes

add a route to enable eft

a8449dc

seeker25 reviewed Oct 30, 2023

View reviewed changes

add eft_enable column to payment_accounts_version

9d0385a

seeker25 reviewed Oct 30, 2023

View reviewed changes

pay-api/src/pay_api/resources/v1/account.py Outdated Show resolved Hide resolved

Jxio added 2 commits October 30, 2023 09:02

fix from code review

98a4e3f

unit test fix

9b3b5c3

seeker25 approved these changes Oct 30, 2023

View reviewed changes

seeker25 reviewed Oct 30, 2023

View reviewed changes

pay-api/tests/unit/services/test_payment_account.py Show resolved Hide resolved

Jxio added 2 commits October 30, 2023 09:57

add back missing test payload for eft_enable

6bd564c

fix flake8 error

5b9735f

seeker25 merged commit 69d2fd3 into bcgov:main Oct 30, 2023
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

18099 - PAY API - endpoint to add EFT Allowed flag to an account #1307

18099 - PAY API - endpoint to add EFT Allowed flag to an account #1307

Jxio commented Oct 25, 2023

codecov bot commented Oct 25, 2023 •

edited

Loading

seeker25 commented Oct 25, 2023 •

edited

Loading

seeker25 commented Oct 25, 2023

ochiu commented Oct 25, 2023

seeker25 commented Oct 25, 2023

Jxio commented Oct 25, 2023 •

edited

Loading

seeker25 commented Oct 25, 2023 •

edited

Loading

seeker25 commented Oct 25, 2023 •

edited

Loading

ochiu Oct 27, 2023

Jxio Oct 27, 2023

ochiu Oct 27, 2023

Jxio Oct 27, 2023

seeker25 commented Oct 27, 2023 •

edited

Loading

seeker25 Oct 27, 2023 •

edited

Loading

Jxio Oct 27, 2023

seeker25 Oct 27, 2023

seeker25 Oct 27, 2023

seeker25 commented Oct 27, 2023

Jxio commented Oct 27, 2023

seeker25 commented Oct 27, 2023 •

edited

Loading

seeker25 Oct 30, 2023 •

edited

Loading

seeker25 Oct 30, 2023 •

edited

Loading

sonarcloud bot commented Oct 30, 2023

18099 - PAY API - endpoint to add EFT Allowed flag to an account #1307

18099 - PAY API - endpoint to add EFT Allowed flag to an account #1307

Conversation

Jxio commented Oct 25, 2023

codecov bot commented Oct 25, 2023 • edited Loading

Codecov Report

seeker25 commented Oct 25, 2023 • edited Loading

seeker25 commented Oct 25, 2023

ochiu commented Oct 25, 2023

seeker25 commented Oct 25, 2023

Jxio commented Oct 25, 2023 • edited Loading

seeker25 commented Oct 25, 2023 • edited Loading

seeker25 commented Oct 25, 2023 • edited Loading

ochiu Oct 27, 2023

Choose a reason for hiding this comment

Jxio Oct 27, 2023

Choose a reason for hiding this comment

ochiu Oct 27, 2023

Choose a reason for hiding this comment

Jxio Oct 27, 2023

Choose a reason for hiding this comment

seeker25 commented Oct 27, 2023 • edited Loading

seeker25 Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

Jxio Oct 27, 2023

Choose a reason for hiding this comment

seeker25 Oct 27, 2023

Choose a reason for hiding this comment

seeker25 Oct 27, 2023

Choose a reason for hiding this comment

seeker25 commented Oct 27, 2023

Jxio commented Oct 27, 2023

seeker25 commented Oct 27, 2023 • edited Loading

seeker25 Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

seeker25 Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

sonarcloud bot commented Oct 30, 2023

codecov bot commented Oct 25, 2023 •

edited

Loading

seeker25 commented Oct 25, 2023 •

edited

Loading

Jxio commented Oct 25, 2023 •

edited

Loading

seeker25 commented Oct 25, 2023 •

edited

Loading

seeker25 commented Oct 25, 2023 •

edited

Loading

seeker25 commented Oct 27, 2023 •

edited

Loading

seeker25 Oct 27, 2023 •

edited

Loading

seeker25 commented Oct 27, 2023 •

edited

Loading

seeker25 Oct 30, 2023 •

edited

Loading

seeker25 Oct 30, 2023 •

edited

Loading