From cd01ed0f43029cfacffe86d649703e476df8d318 Mon Sep 17 00:00:00 2001 From: "Luis J. Salvatierra" Date: Tue, 23 Apr 2024 15:20:14 +0200 Subject: [PATCH] [FIX] account_statement_import_online_gocardless: IBAN comparison It is possible that, when making the request to the requisitions endpoint, the IBAN bank account comes with a lower alphanumeric string. When comparing with the sanitized bank account (stored with upper) fails. self.journal_id.bank_account_id.sanitized_acc_number == account_data["iban"] to self.journal_id.bank_account_id.sanitized_acc_number == account_data["iban"].upper() Refactor method _gocardless_finish_requisition to be able to mock the requests made inside and create a unit test. Refactor requests methods. --- .../models/online_bank_statement_provider.py | 123 +++++++++--------- ...ount_statement_import_online_gocardless.py | 39 ++++++ 2 files changed, 101 insertions(+), 61 deletions(-) diff --git a/account_statement_import_online_gocardless/models/online_bank_statement_provider.py b/account_statement_import_online_gocardless/models/online_bank_statement_provider.py index dfdb9724f..c02c91ab6 100644 --- a/account_statement_import_online_gocardless/models/online_bank_statement_provider.py +++ b/account_statement_import_online_gocardless/models/online_bank_statement_provider.py @@ -13,7 +13,7 @@ from odoo.exceptions import UserError from odoo.tools import DEFAULT_SERVER_DATE_FORMAT as DF -GOCARDLESS_ENDPOINT = "https://bankaccountdata.gocardless.com/api/v2" +GOCARDLESS_API = "https://bankaccountdata.gocardless.com/api/v2" REQUESTS_TIMEOUT = 60 @@ -46,6 +46,31 @@ def _get_available_services(self): ("gocardless", "GoCardless"), ] + def _gocardless_get_headers(self, basic=False): + """Generic method for providing the needed request headers.""" + self.ensure_one() + headers = { + "accept": "application/json", + "Content-Type": "application/json", + } + if not basic: + headers["Authorization"] = f"Bearer {self._gocardless_get_token()}" + return headers + + def _gocardless_request(self, endpoint, request_type="get", params=None, data=None): + content = {} + url = url_join(GOCARDLESS_API, endpoint) + response = getattr(requests, request_type)( + url, + data=data, + params=params, + headers=self._gocardless_get_headers(), + timeout=REQUESTS_TIMEOUT, + ) + if response.status_code in [200, 201]: + content = json.loads(response.text) + return response, content + def _gocardless_get_token(self): """Resolve and return the corresponding GoCardless token for doing the requests. If there's still no token, it's requested. If it exists, but it's expired and @@ -59,20 +84,16 @@ def _gocardless_get_token(self): self.gocardless_refresh_token and now > self.gocardless_refresh_expiration ): - url = f"{GOCARDLESS_ENDPOINT}/token/refresh/" + endpoint = "token/refresh" else: - url = f"{GOCARDLESS_ENDPOINT}/token/new/" - response = requests.post( - url, + endpoint = "token/new" + _response, data = self._gocardless_request( + endpoint, + request_type="post", data=json.dumps( {"secret_id": self.username, "secret_key": self.password} ), - headers=self._gocardless_get_headers(basic=True), - timeout=REQUESTS_TIMEOUT, ) - data = {} - if response.status_code == 200: - data = json.loads(response.text) expiration_date = now + relativedelta(seconds=data.get("access_expires", 0)) vals = { "gocardless_token": data.get("access", False), @@ -86,17 +107,6 @@ def _gocardless_get_token(self): self.sudo().write(vals) return self.gocardless_token - def _gocardless_get_headers(self, basic=False): - """Generic method for providing the needed request headers.""" - self.ensure_one() - headers = { - "accept": "application/json", - "Content-Type": "application/json", - } - if not basic: - headers["Authorization"] = f"Bearer {self._gocardless_get_token()}" - return headers - def action_select_gocardless_bank(self): if not self.journal_id.bank_account_id: raise UserError( @@ -137,15 +147,12 @@ def _gocardless_select_bank_instituion(self): country = ( self.journal_id.bank_account_id.company_id or self.journal_id.company_id ).country_id - response = requests.get( - f"{GOCARDLESS_ENDPOINT}/institutions/", - params={"country": country.code}, - headers=self._gocardless_get_headers(), - timeout=REQUESTS_TIMEOUT, + response, data = self._gocardless_request( + "institutions", params={"country": country.code} ) if response.status_code == 400: raise UserError(_("Incorrect country code or country not supported.")) - institutions = json.loads(response.text) + institutions = data # Prepare data for being showed in the JS widget ctx = self.env.context.copy() ctx.update( @@ -172,8 +179,9 @@ def action_check_gocardless_agreement(self): self.gocardless_requisition_ref = str(uuid4()) base_url = self.env["ir.config_parameter"].sudo().get_param("web.base.url") redirect_url = url_join(base_url, "gocardless/response") - response = requests.post( - f"{GOCARDLESS_ENDPOINT}/requisitions/", + _response, data = self._gocardless_request( + "requisitions", + request_type="post", data=json.dumps( { "redirect": redirect_url, @@ -181,15 +189,27 @@ def action_check_gocardless_agreement(self): "reference": self.gocardless_requisition_ref, } ), - headers=self._gocardless_get_headers(), - timeout=REQUESTS_TIMEOUT, ) - if response.status_code == 201: - requisition_data = json.loads(response.text) + if data: + requisition_data = data self.gocardless_requisition_id = requisition_data["id"] # JS code expects here to return a plain link or nothing return requisition_data["link"] + def _gocardless_request_requisition(self): + _response, data = self._gocardless_request( + f"requisitions/{self.gocardless_requisition_id}" + ) + return data + + def _gocardless_request_account(self, account_id): + _response, data = self._gocardless_request(f"accounts/{account_id}") + return data + + def _gocardless_request_agreement(self, agreement_id): + _response, data = self._gocardless_request(f"agreements/enduser/{agreement_id}") + return data + def _gocardless_finish_requisition(self, dry=False): """Once the requisiton to the bank institution has been made, and this is called from the controller assigned to the redirect URL, we check that the IBAN account @@ -203,39 +223,25 @@ def _gocardless_finish_requisition(self, dry=False): process, so no fail message is logged in chatter in this case. """ self.ensure_one() - requisition_response = requests.get( - f"{GOCARDLESS_ENDPOINT}/requisitions/{self.gocardless_requisition_id}/", - headers=self._gocardless_get_headers(), - timeout=REQUESTS_TIMEOUT, - ) - requisition_data = json.loads(requisition_response.text) + requisition_data = self._gocardless_request_requisition() accounts = requisition_data.get("accounts", []) found_account = False accounts_iban = [] for account_id in accounts: - account_response = requests.get( - f"{GOCARDLESS_ENDPOINT}/accounts/{account_id}/", - headers=self._gocardless_get_headers(), - timeout=REQUESTS_TIMEOUT, - ) - if account_response.status_code == 200: - account_data = json.loads(account_response.text) + account_data = self._gocardless_request_account(account_id) + if account_data: accounts_iban.append(account_data["iban"]) if ( self.journal_id.bank_account_id.sanitized_acc_number - == account_data["iban"] + == account_data["iban"].upper() ): found_account = True self.gocardless_account_id = account_data["id"] break if found_account: - agreement_response = requests.get( - f"{GOCARDLESS_ENDPOINT}/agreements/enduser/" - f"{requisition_data['agreement']}/", - headers=self._gocardless_get_headers(), - timeout=REQUESTS_TIMEOUT, + agreement_data = self._gocardless_request_agreement( + requisition_data["agreement"] ) - agreement_data = json.loads(agreement_response.text) self.gocardless_requisition_expiration = datetime.strptime( agreement_data["accepted"], "%Y-%m-%dT%H:%M:%S.%fZ" ) + relativedelta(days=agreement_data["access_valid_for_days"]) @@ -279,19 +285,14 @@ def _gocardless_request_transactions(self, date_since, date_until): now = fields.Datetime.now() if now > date_since and now < date_until: date_until = now - transaction_response = requests.get( - f"{GOCARDLESS_ENDPOINT}/accounts/" - f"{self.gocardless_account_id}/transactions/", + _response, data = self._gocardless_request( + f"accounts/{self.gocardless_account_id}/transactions", params={ "date_from": date_since.strftime(DF), "date_to": date_until.strftime(DF), }, - headers=self._gocardless_get_headers(), - timeout=REQUESTS_TIMEOUT, ) - if transaction_response.status_code == 200: - return json.loads(transaction_response.text) - return {} + return data def _gocardless_obtain_statement_data(self, date_since, date_until): """Called from the cron or the manual pull wizard to obtain transactions for diff --git a/account_statement_import_online_gocardless/tests/test_account_statement_import_online_gocardless.py b/account_statement_import_online_gocardless/tests/test_account_statement_import_online_gocardless.py index 6e6f50405..2b5142411 100644 --- a/account_statement_import_online_gocardless/tests/test_account_statement_import_online_gocardless.py +++ b/account_statement_import_online_gocardless/tests/test_account_statement_import_online_gocardless.py @@ -21,6 +21,14 @@ def setUpClass(cls): cls.now = fields.Datetime.now() cls.currency_eur = cls.env.ref("base.EUR") cls.currency_eur.write({"active": True}) + bank_account = cls.env["res.partner.bank"].create( + { + "acc_number": "NL77ABNA0574908765", + "partner_id": cls.env.ref("base.main_partner").id, + "company_id": cls.env.ref("base.main_company").id, + "bank_id": cls.env.ref("base.res_bank_1").id, + } + ) cls.journal = cls.env["account.journal"].create( { "name": "GoCardless Bank Test", @@ -29,6 +37,7 @@ def setUpClass(cls): "currency_id": cls.currency_eur.id, "bank_statements_source": "online", "online_bank_statement_provider": "gocardless", + "bank_account_id": bank_account.id, } ) cls.provider = cls.journal.online_bank_statement_provider_id @@ -77,6 +86,31 @@ def setUpClass(cls): _provider_class + "._gocardless_request_transactions", return_value=cls.return_value, ) + cls.request_requisition_value = { + "accounts": ["ACCOUNT-ID-1"], + "agreement": "TEST-AGREEMENT-ID", + } + cls.mock_requisition = lambda cls: mock.patch( + _provider_class + "._gocardless_request_requisition", + return_value=cls.request_requisition_value, + ) + cls.request_account_value = { + "id": "ACCOUNT-ID-1", + "iban": "nl77abna0574908765", + } + cls.mock_account = lambda cls: mock.patch( + _provider_class + "._gocardless_request_account", + return_value=cls.request_account_value, + ) + cls.request_agreement_value = { + "id": "TEST-AGREEMENT-ID", + "accepted": cls.now.strftime("%Y-%m-%dT%H:%M:%S.%fZ"), + "access_valid_for_days": 30, + } + cls.mock_agreement = lambda cls: mock.patch( + _provider_class + "._gocardless_request_agreement", + return_value=cls.request_agreement_value, + ) def test_mocked_gocardless(self): vals = { @@ -100,3 +134,8 @@ def test_mocked_gocardless(self): lines = statements.line_ids.sorted(lambda x: x.date) self.assertEqual(len(lines), 2) self.assertEqual(lines.mapped("amount"), [45.0, -15.0]) + + def test_provider_gocardless_finish_requisition(self): + with self.mock_requisition(), self.mock_account(), self.mock_agreement(): + res = self.provider._gocardless_finish_requisition(dry=True) + self.assertTrue(res, "Bank account not found!")