[16.0][FIX] account_statement_import_online_gocardless: IBAN comparison

[ljsalvatierra-factorlibre]

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

I did refactor the method _gocardless_finish_requisition to be able to mock the requests made inside and create a unit test.

[ljsalvatierra-factorlibre]

Hi @pedrobaeza could you take a look please? I've seen that you have recently contributed to the same addon.

[pedrobaeza]

Please don't put [WIP] in the title, but use Draft status, as now, although you remove it, on mails, it still puts the WIP subject, so this makes people think that is not ready yet:

Can you please expose the problem this is handling? It should be always put on the initial comment, and also on the commit message.

[ljsalvatierra-factorlibre]

Please don't put [WIP] in the title, but use Draft status, as now, although you remove it, on mails, it still puts the WIP subject, so this makes people think that is not ready yet:

Can you please expose the problem this is handling? It should be always put on the initial comment, and also on the commit message.

I am aware, won't happen again, thank you.
I've updated the description, sorry for the confussion.

[ljsalvatierra-factorlibre]

Hi @pedrobaeza is it ok to merge?

[pedrobaeza]

And please put the same explanation of the main comment in the commit expanded message (third line after a blank line and following)

[pedrobaeza]

Sorry for asking for more modifications, but thinking twice, I think we can refactor this better, having only one method:

    def _gocardless_request(self, path):
        response = requests.get(
            f"{GOCARDLESS_ENDPOINT}/path",
            headers=self._gocardless_get_headers(),
            timeout=REQUESTS_TIMEOUT,
        )
        if response.status_code == 200:
            return json.loads(response.text)
        return {}

and calling it with the multiple variants:

self._gorcardless_request(f"requisitions/{self.gocardless_requisition_id}/")
self._gorcardless_request(f"accounts/{account_id}/")
...

My only doubt is the mock for working properly.

[ljsalvatierra-factorlibre]

Sorry for asking for more modifications, but thinking twice, I think we can refactor this better, having only one method:

    def _gocardless_request(self, path):
        response = requests.get(
            f"{GOCARDLESS_ENDPOINT}/path",
            headers=self._gocardless_get_headers(),
            timeout=REQUESTS_TIMEOUT,
        )
        if response.status_code == 200:
            return json.loads(response.text)
        return {}

and calling it with the multiple variants:

self._gorcardless_request(f"requisitions/{self.gocardless_requisition_id}/")
self._gorcardless_request(f"accounts/{account_id}/")
...

My only doubt is the mock for working properly.

I'll take a look thank you, I think it would be possible rewriting the side_effect method returning a different value depending on the arguments.

[OCA-git-bot]

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

[pedrobaeza]

Any news on this?

[ljsalvatierra-factorlibre]

Any news on this?

Hi, It's in my queue, but currently drowned on more urgent work, I'll try to take a look this week, sorry for the inconvenience.

[ljsalvatierra-factorlibre]

Any news on this?

Hi @pedrobaeza sorry for the delay, please take a look and tell me what you think.

[pedrobaeza]

Please squash comments into one. It's better to amend the previous comment than adding one extra. And finally, [UPD] is not a valid commit tag.

[ljsalvatierra-factorlibre]

Please squash comments into one. It's better to amend the previous comment than adding one extra. And finally, [UPD] is not a valid commit tag.

Done

[pedrobaeza]

Thanks for the refactoring:

/ocabot merge minor

[OCA-git-bot]

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-686-by-pedrobaeza-bump-minor, awaiting test results.

[OCA-git-bot]

Congratulations, your PR was merged at 747c454. Thanks a lot for contributing to OCA. ❤️