[16.0][FIX] account_payment_return: Post-install test + fallback to load CoA

[victoralmau]

Since odoo/odoo@d0342c8, the default existing company is not getting a CoA automatically, provoking than the current tests fail with the error:

odoo.exceptions.UserError: No journal could be found in company My Company (San Francisco) for any of those types: sale

Thus, we put tests post-install for being sure localization modules are installed, the same as AccountTestInvoicingCommon does, but we don't inherit from it, as it creates an overhead creating 2 new companies and loading their CoA and some more stuff, while we don't need all of that.

Besides, if you don't have l10n_generic_coa installed, you can't use another CoA (like l10n_es) easily, so we put little code to select the first available CoA.

@Tecnativa

[pedrobaeza]

This still seems incorrect. You should call super when the file is not a XML one.

[victoralmau]

Sorry, but why block this if the fix in this related case “Post-install test + fallback to load CoA” is clear?

I at least am not going to defend why this is this way

account-payment/account_payment_return_import_iso20022/wizard/payment_return_import.py

Lines 48 to 62 in 977c1a2

	try:
	_logger.debug(
	"Try parsing as a CAMT Bank to Customer " "Debit Credit Notification."
	)
	return camt_parser.parse(data_file)
	except ValueError:
	try:
	_logger.debug("Try parsing as a PAIN Direct Debit Unpaid " "Report.")
	return pain_parser.parse(data_file)
	except ValueError:
	_logger.debug(
	"Payment return file is not a ISO20022 " "supported file.",
	exc_info=True,
	)
	return super()._parse_file(data_file)

because it will be for something, and changing the behavior I BELIEVE is not the right thing to do.

[pedrobaeza]

It's not changing the behavior, but fixing the behavior in the proper way. All of this have been because passing to post-install has uncovered a problem, and the problem is that the architecture, as it's though, it's not being respected in the iso20022 module, as instead of calling super when the file is not an XML to let the CSV parser to act, it's just aborting the import process. What you have done with the concatenated try/reraise is to play with the exception types to bypass it, but it's not the proper fix.

Look that this same scheme is done on the statement import, and how is handled in N43 format:

https://github.com/OCA/l10n-spain/blob/aaab375c85e33dff2708396690bf00995f69d166/l10n_es_account_statement_import_n43/wizards/account_statement_import_n43.py#L361

so what you have to do is to catch that a file is not XML (or even not the expected XSD format), and if not, call super for letting the rest of the parsers to act.

[victoralmau]

The error in the tests detected was precisely because in

account-payment/account_payment_return_import_iso20022/wizard/camt_parser.py

Line 167 in 977c1a2

root = etree.fromstring(data, parser=etree.XMLParser(recover=True))

there was no try/except, therefore, the test failed

account-payment/account_payment_return_import/tests/test_import_base.py

Line 130 in 977c1a2

with self.assertRaises(UserError):

because it did not return a UserError.

With the current change it ends up calling super() in

account-payment/account_payment_return_import_iso20022/wizard/payment_return_import.py

Line 62 in 977c1a2

return super()._parse_file(data_file)

and for that reason UserError is returned.

The code is not pretty and/or readable, yes, but I guess, that would be another debate.

[pedrobaeza]

Well, let me do it the changes I have in mind by myself and check the current test suite. Maybe that one is not correct as well.

[pedrobaeza]

@victoralmau you can see now what I was talking about. You have to call super, but in the proper method that is doing the iteration.

/ocabot merge patch

[OCA-git-bot]

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-803-by-pedrobaeza-bump-patch, awaiting test results.

[OCA-git-bot]

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