From 2cab29060506014d2f1749571ad8c096e4320beb Mon Sep 17 00:00:00 2001 From: Travis Semple Date: Thu, 4 Jul 2024 16:27:14 -0700 Subject: [PATCH 1/8] Refactor payment account service so it's more dynamic and less boiler plate we can reuse this for other services that are complicated, also this should set us up to be able to have multiple CFS accounts active at once. --- .../pay_api/factory/payment_system_factory.py | 6 +- .../src/pay_api/services/payment_account.py | 289 ++---------------- 2 files changed, 31 insertions(+), 264 deletions(-) diff --git a/pay-api/src/pay_api/factory/payment_system_factory.py b/pay-api/src/pay_api/factory/payment_system_factory.py index b9000b55f..0b1b21990 100644 --- a/pay-api/src/pay_api/factory/payment_system_factory.py +++ b/pay-api/src/pay_api/factory/payment_system_factory.py @@ -12,7 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. """Factory to manage creation of pay system service.""" -from datetime import datetime +from datetime import datetime, timezone from flask import current_app @@ -109,8 +109,8 @@ def create(**kwargs): @staticmethod def _validate_and_throw_error(instance: PaymentSystemService, payment_account: PaymentAccount): if isinstance(instance, PadService): - is_in_pad_confirmation_period = payment_account.pad_activation_date > \ - datetime.now(payment_account.pad_activation_date.tzinfo) + is_in_pad_confirmation_period = payment_account.pad_activation_date.replace(tzinfo=timezone.utc) > \ + datetime.now(tz=timezone.utc) is_cfs_account_in_pending_status = payment_account.cfs_account_status == \ CfsAccountStatus.PENDING_PAD_ACTIVATION.value diff --git a/pay-api/src/pay_api/services/payment_account.py b/pay-api/src/pay_api/services/payment_account.py index 14d95fc32..4dad6bc41 100644 --- a/pay-api/src/pay_api/services/payment_account.py +++ b/pay-api/src/pay_api/services/payment_account.py @@ -16,7 +16,7 @@ from datetime import date, datetime, timedelta, timezone from decimal import Decimal -from typing import Any, Dict, List, Optional, Tuple +from typing import Any, Dict, List, Tuple from cattr import Converter from flask import current_app @@ -61,284 +61,51 @@ class PaymentAccount(): # pylint: disable=too-many-instance-attributes, too-man def __init__(self): """Initialize service.""" - self.__dao = None - self._id: Optional[int] = None - self._auth_account_id: Optional[str] = None - self._name: Optional[str] = None - self._payment_method: Optional[str] = None - self._pad_activation_date: Optional[datetime] = None - self._pad_tos_accepted_by: Optional[str] = None - self._pad_tos_accepted_date: Optional[datetime] = None - self._credit: Optional[Decimal] = None - - self._cfs_account: Optional[str] = None - self._cfs_party: Optional[str] = None - self._cfs_site: Optional[str] = None - - self._bank_number: Optional[str] = None - self._bank_branch_number: Optional[str] = None - self._bank_account_number: Optional[str] = None - - self._bcol_user_id: Optional[str] = None - self._bcol_account: Optional[str] = None - - self._cfs_account_id: Optional[int] = None - self._cfs_account_status: Optional[str] = None - self._billable: Optional[bool] = None - self._eft_enable: Optional[bool] = None + super().__setattr__('_PaymentAccount__dao', None) + self.cfs_account: str = None + self.cfs_party: str = None + self.cfs_site: str = None + self.bank_number: str = None + self.bank_branch_number: str = None + self.bank_account_number: str = None + self.cfs_account_id: int = None + self.cfs_account_status: str = None @property def _dao(self): if not self.__dao: - self.__dao = PaymentAccountModel() + super().__setattr__('_PaymentAccount__dao', PaymentAccountModel()) return self.__dao @_dao.setter def _dao(self, value: PaymentAccountModel): self.__dao = value - self.id: int = self._dao.id - self.auth_account_id: str = self._dao.auth_account_id - self.name: str = self._dao.name - self.payment_method: str = self._dao.payment_method - self.bcol_user_id: str = self._dao.bcol_user_id - self.bcol_account: str = self._dao.bcol_account - self.pad_activation_date: datetime = self._dao.pad_activation_date - self.pad_tos_accepted_by: str = self._dao.pad_tos_accepted_by - self.pad_tos_accepted_date: datetime = self._dao.pad_tos_accepted_date - self.credit: Decimal = self._dao.credit - self.billable: bool = self._dao.billable - self.eft_enable: bool = self._dao.eft_enable - - cfs_account: CfsAccountModel = CfsAccountModel.find_effective_by_account_id(self.id) - if cfs_account: + if hasattr(self.__dao, 'id') and (cfs_account := CfsAccountModel.find_effective_by_account_id(self.__dao.id)): self.cfs_account: str = cfs_account.cfs_account self.cfs_party: str = cfs_account.cfs_party self.cfs_site: str = cfs_account.cfs_site - self.bank_number: str = cfs_account.bank_number self.bank_branch_number: str = cfs_account.bank_branch_number self.bank_account_number: str = cfs_account.bank_account_number self.cfs_account_id: int = cfs_account.id self.cfs_account_status: str = cfs_account.status - @property - def id(self): - """Return the _id.""" - return self._id - - @id.setter - def id(self, value: int): - """Set the id.""" - self._id = value - self._dao.id = value - - @property - def cfs_account_id(self): - """Return the cfs_account_id.""" - return self._cfs_account_id - - @cfs_account_id.setter - def cfs_account_id(self, value: int): - """Set the cfs_account_id.""" - self._cfs_account_id = value - self._dao.cfs_account_id = value - - @property - def auth_account_id(self): - """Return the auth_account_id.""" - return self._auth_account_id - - @auth_account_id.setter - def auth_account_id(self, value: str): - """Set the auth_account_id.""" - if self._auth_account_id != value: - self._auth_account_id = value - self._dao.auth_account_id = value - - @property - def name(self): - """Return the name.""" - return self._name - - @name.setter - def name(self, value: str): - """Set the name.""" - if self._name != value: - self._name = value - self._dao.name = value - - @property - def payment_method(self): - """Return the payment_method.""" - return self._payment_method - - @payment_method.setter - def payment_method(self, value: int): - """Set the payment_method.""" - if self._payment_method != value: - self._payment_method = value - self._dao.payment_method = value - - @property - def cfs_account(self): - """Return the cfs_account.""" - return self._cfs_account - - @cfs_account.setter - def cfs_account(self, value: int): - """Set the cfs_account.""" - self._cfs_account = value - - @property - def cfs_party(self): - """Return the cfs_party.""" - return self._cfs_party - - @cfs_party.setter - def cfs_party(self, value: int): - """Set the cfs_party.""" - self._cfs_party = value - - @property - def cfs_site(self): - """Return the cfs_site.""" - return self._cfs_site - - @cfs_site.setter - def cfs_site(self, value: int): - """Set the cfs_site.""" - self._cfs_site = value - - @property - def bank_number(self): - """Return the bank_number.""" - return self._bank_number - - @bank_number.setter - def bank_number(self, value: int): - """Set the bank_number.""" - self._bank_number = value - - @property - def bank_branch_number(self): - """Return the bank_branch_number.""" - return self._bank_branch_number - - @bank_branch_number.setter - def bank_branch_number(self, value: int): - """Set the bank_branch_number.""" - self._bank_branch_number = value - - @property - def bank_account_number(self): - """Return the bank_account_number.""" - return self._bank_account_number - - @bank_account_number.setter - def bank_account_number(self, value: int): - """Set the bank_account_number.""" - self._bank_account_number = value - - @property - def bcol_user_id(self): - """Return the bcol_user_id.""" - return self._bcol_user_id - - @bcol_user_id.setter - def bcol_user_id(self, value: int): - """Set the bcol_user_id.""" - self._bcol_user_id = value - self._dao.bcol_user_id = value - - @property - def pad_activation_date(self): - """Return the pad_activation_date.""" - return self._pad_activation_date - - @pad_activation_date.setter - def pad_activation_date(self, value: datetime): - """Set the pad_activation_date.""" - self._pad_activation_date = value - self._dao.pad_activation_date = value - - @property - def pad_tos_accepted_by(self): - """Return the pad_tos_accepted_by.""" - return self._pad_tos_accepted_by - - @pad_tos_accepted_by.setter - def pad_tos_accepted_by(self, value: datetime): - """Set the pad_tos_accepted_by.""" - self._pad_tos_accepted_by = value - self._dao.pad_tos_accepted_by = value - - @property - def pad_tos_accepted_date(self): - """Return the pad_tos_accepted_date.""" - return self._pad_tos_accepted_date - - @pad_tos_accepted_date.setter - def pad_tos_accepted_date(self, value: datetime): - """Set the pad_tos_accepted_by.""" - self._pad_tos_accepted_date = value - self._dao.pad_tos_accepted_date = value - - @property - def bcol_account(self): - """Return the bcol_account.""" - return self._bcol_account - - @bcol_account.setter - def bcol_account(self, value: int): - """Set the bcol_account.""" - self._bcol_account = value - self._dao.bcol_account = value - - @property - def cfs_account_status(self): - """Return the cfs_account_status.""" - return self._cfs_account_status - - @cfs_account_status.setter - def cfs_account_status(self, value: int): - """Set the cfs_account_status.""" - self._cfs_account_status = value - self._dao.cfs_account_status = value - - @property - def credit(self): - """Return the credit.""" - return self._credit - - @credit.setter - def credit(self, value: float): - """Set the credit.""" - self._credit = value - self._dao.credit = value - - @property - def billable(self): - """Return the billable.""" - return self._billable - - @billable.setter - def billable(self, value: bool): - """Set the billable.""" - if self._billable != value: - self._billable = value - self._dao.billable = value - - @property - def eft_enable(self): - """Return the eft_enable.""" - return self._eft_enable - - @eft_enable.setter - def eft_enable(self, value: bool): - """Set the eft_enable.""" - if self._eft_enable != value: - self._eft_enable = value - self._dao.eft_enable = value + def __getattr__(self, name): + """Dynamic way of getting the properties from the DAO, anything not in __init__.""" + if hasattr(self._dao, name): + return getattr(self._dao, name) + return None + + def __setattr__(self, name, value): + """Dynamic way of setting the properties from the DAO.""" + # Prevent recursion by checking if the attribute name starts with '__' (private attribute) + if name == '_PaymentAccount__dao': + super().__setattr__(name, value) + elif hasattr(self._dao, name): + if getattr(self._dao, name) != value: + setattr(self._dao, name, value) + else: + super().__setattr__(name, value) def save(self): """Save the information to the DB.""" From 366ab0b390d177700050f36e8be212b7f56489da Mon Sep 17 00:00:00 2001 From: Travis Semple Date: Thu, 4 Jul 2024 16:36:03 -0700 Subject: [PATCH 2/8] Small touch ups, unit test to go --- pay-api/src/pay_api/services/payment_account.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pay-api/src/pay_api/services/payment_account.py b/pay-api/src/pay_api/services/payment_account.py index 4dad6bc41..6e996f68f 100644 --- a/pay-api/src/pay_api/services/payment_account.py +++ b/pay-api/src/pay_api/services/payment_account.py @@ -61,7 +61,7 @@ class PaymentAccount(): # pylint: disable=too-many-instance-attributes, too-man def __init__(self): """Initialize service.""" - super().__setattr__('_PaymentAccount__dao', None) + self.__dao = None self.cfs_account: str = None self.cfs_party: str = None self.cfs_site: str = None @@ -74,7 +74,7 @@ def __init__(self): @property def _dao(self): if not self.__dao: - super().__setattr__('_PaymentAccount__dao', PaymentAccountModel()) + self.__dao = PaymentAccountModel() return self.__dao @_dao.setter @@ -94,6 +94,7 @@ def __getattr__(self, name): """Dynamic way of getting the properties from the DAO, anything not in __init__.""" if hasattr(self._dao, name): return getattr(self._dao, name) + current_app.logger.warning('Attribute %s not found in services.PaymentAccount', name) return None def __setattr__(self, name, value): From b256ae155dfff11812d42d137d033c063feb95e9 Mon Sep 17 00:00:00 2001 From: Travis Semple Date: Thu, 4 Jul 2024 16:42:06 -0700 Subject: [PATCH 3/8] small comment --- pay-api/src/pay_api/services/payment_account.py | 1 + 1 file changed, 1 insertion(+) diff --git a/pay-api/src/pay_api/services/payment_account.py b/pay-api/src/pay_api/services/payment_account.py index 6e996f68f..7bc0347f6 100644 --- a/pay-api/src/pay_api/services/payment_account.py +++ b/pay-api/src/pay_api/services/payment_account.py @@ -102,6 +102,7 @@ def __setattr__(self, name, value): # Prevent recursion by checking if the attribute name starts with '__' (private attribute) if name == '_PaymentAccount__dao': super().__setattr__(name, value) + # _dao uses __dao, thus why we need to check before for __dao. elif hasattr(self._dao, name): if getattr(self._dao, name) != value: setattr(self._dao, name, value) From 9230b4d5dad0925a1480f56bbd42f597a70dd9d4 Mon Sep 17 00:00:00 2001 From: Travis Semple Date: Thu, 4 Jul 2024 16:43:22 -0700 Subject: [PATCH 4/8] Add in comment with intent --- pay-api/src/pay_api/services/payment_account.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pay-api/src/pay_api/services/payment_account.py b/pay-api/src/pay_api/services/payment_account.py index 7bc0347f6..a5ed6718f 100644 --- a/pay-api/src/pay_api/services/payment_account.py +++ b/pay-api/src/pay_api/services/payment_account.py @@ -80,6 +80,9 @@ def _dao(self): @_dao.setter def _dao(self, value: PaymentAccountModel): self.__dao = value + # In discussions with John to see if we can have multiple CFS accounts enabled at once. + # This way we could pay for BCOL and PAD/EFT/ONLINE banking at the same time. + # The code below may change. if hasattr(self.__dao, 'id') and (cfs_account := CfsAccountModel.find_effective_by_account_id(self.__dao.id)): self.cfs_account: str = cfs_account.cfs_account self.cfs_party: str = cfs_account.cfs_party From 46a6fd524d66b1805865e02445baca06d97b6297 Mon Sep 17 00:00:00 2001 From: Travis Semple Date: Thu, 4 Jul 2024 16:44:56 -0700 Subject: [PATCH 5/8] Comment fix --- pay-api/src/pay_api/services/payment_account.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pay-api/src/pay_api/services/payment_account.py b/pay-api/src/pay_api/services/payment_account.py index a5ed6718f..86f98f504 100644 --- a/pay-api/src/pay_api/services/payment_account.py +++ b/pay-api/src/pay_api/services/payment_account.py @@ -81,8 +81,8 @@ def _dao(self): def _dao(self, value: PaymentAccountModel): self.__dao = value # In discussions with John to see if we can have multiple CFS accounts enabled at once. - # This way we could pay for BCOL and PAD/EFT/ONLINE banking at the same time. - # The code below may change. + # This way we could pay for BCOL and PAD/EFT/ONLINE BANKING at the same time. + # DIRECT_PAY should already work without a CFS account. if hasattr(self.__dao, 'id') and (cfs_account := CfsAccountModel.find_effective_by_account_id(self.__dao.id)): self.cfs_account: str = cfs_account.cfs_account self.cfs_party: str = cfs_account.cfs_party From cdcf3f31ca8634f0a6b04c35a46cf9deea6a0aa9 Mon Sep 17 00:00:00 2001 From: Travis Semple Date: Thu, 4 Jul 2024 16:45:23 -0700 Subject: [PATCH 6/8] comments --- pay-api/src/pay_api/services/payment_account.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pay-api/src/pay_api/services/payment_account.py b/pay-api/src/pay_api/services/payment_account.py index 86f98f504..6c4575733 100644 --- a/pay-api/src/pay_api/services/payment_account.py +++ b/pay-api/src/pay_api/services/payment_account.py @@ -102,7 +102,7 @@ def __getattr__(self, name): def __setattr__(self, name, value): """Dynamic way of setting the properties from the DAO.""" - # Prevent recursion by checking if the attribute name starts with '__' (private attribute) + # Prevent recursion by checking if the attribute name starts with '__' (private attribute). if name == '_PaymentAccount__dao': super().__setattr__(name, value) # _dao uses __dao, thus why we need to check before for __dao. From b3daa03de431e9a07ce4da22f4fec9e96e6b3d4d Mon Sep 17 00:00:00 2001 From: Travis Semple Date: Thu, 4 Jul 2024 16:46:06 -0700 Subject: [PATCH 7/8] minor tweak --- pay-api/src/pay_api/services/payment_account.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pay-api/src/pay_api/services/payment_account.py b/pay-api/src/pay_api/services/payment_account.py index 6c4575733..c6b948e69 100644 --- a/pay-api/src/pay_api/services/payment_account.py +++ b/pay-api/src/pay_api/services/payment_account.py @@ -97,7 +97,7 @@ def __getattr__(self, name): """Dynamic way of getting the properties from the DAO, anything not in __init__.""" if hasattr(self._dao, name): return getattr(self._dao, name) - current_app.logger.warning('Attribute %s not found in services.PaymentAccount', name) + current_app.logger.warning('Attribute %s not found.', name) return None def __setattr__(self, name, value): From 6132afa2b51c0cde99bbb36d2a76a6605952f41a Mon Sep 17 00:00:00 2001 From: Travis Semple Date: Thu, 4 Jul 2024 17:29:04 -0700 Subject: [PATCH 8/8] Small unit test --- .../unit/services/test_payment_account.py | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/pay-api/tests/unit/services/test_payment_account.py b/pay-api/tests/unit/services/test_payment_account.py index da1a626dd..5ade04ca0 100644 --- a/pay-api/tests/unit/services/test_payment_account.py +++ b/pay-api/tests/unit/services/test_payment_account.py @@ -27,6 +27,7 @@ from pay_api.models import EFTCredit as EFTCreditModel from pay_api.models import EFTShortnames as EFTShortnameModel from pay_api.models import Invoice as InvoiceModel +from pay_api.models import PaymentAccount as PaymentAccountModel from pay_api.models import StatementRecipients as StatementRecipientModel from pay_api.models import StatementSettings as StatementSettingsModel from pay_api.services.payment_account import PaymentAccount as PaymentAccountService @@ -383,3 +384,34 @@ def test_eft_payment_method_settings(session, client, jwt, app, admin_users_mock assert statement_settings is not None assert statement_settings.frequency == StatementFrequency.MONTHLY.value + + +def test_payment_account_service_with_cfs_account(session): + """Small test to make sure CFS details are working correctly.""" + payment_account = PaymentAccountModel() + payment_account.flush() + + cfs_account = '123' + cfs_party = '456' + cfs_site = '789' + bank_number = '001' + bank_branch_number = '002' + bank_account_number = '003' + CfsAccountModel( + account_id=payment_account.id, + cfs_account=cfs_account, + cfs_party=cfs_party, + cfs_site=cfs_site, + bank_number=bank_number, + bank_branch_number=bank_branch_number, + bank_account_number=bank_account_number, + status=CfsAccountStatus.PENDING.value + ).flush() + + payment_account_service = PaymentAccountService.find_by_id(payment_account.id) + assert payment_account_service.cfs_account == cfs_account + assert payment_account_service.cfs_party == cfs_party + assert payment_account_service.cfs_site == cfs_site + assert payment_account_service.bank_number == bank_number + assert payment_account_service.bank_branch_number == bank_branch_number + assert payment_account_service.bank_account_number == bank_account_number