[17.0][MIG] account_statement_import_ofx

[remi-filament]

No description provided.

[github-actions]

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

[rrebollo]

@remi-filament hello, thanks for your contribution. Is there anything I could do to move this forward? Maybe can I take it from here?

[remi-filament]

Hi @rrebollo I am not sure what you mean ?
From my point of view nothing is missing here, except approval and merge, so to move forward you can review it and approve.
Thanks !

[rrebollo]

Great @remi-filament, I'll do a code review but in order to have a runboat instance U need all tests in green.

[rrebollo]

Code review LGTM, nevertheless I think tests rewriting to fit the setUpClass approach would be great.

[rrebollo]

It would be great to translate these tests to the "new" setUpClass approach.

[remi-filament]

I am afraid I have no idea what you mean...
Any pointer would be highly appreciated !
Thanks

[rrebollo]

Sure! Let me start by making it clear: the current implementation works perfectly fine, but I believe there’s room for improvement. This is a desirable enhancement, not a mandatory one.

I’m not an expert in Odoo’s test framework, but I came across this topic through the following reference:
OCA Maintainer Tools - Migration to version 15.0.

One recommendation is:

If there's a test class using setUp to generate test records, consider moving it to setUpClass for better performance, as it runs only once.

I’ve applied this approach during migrations and found it beneficial. For example, here’s a case from the OCA repositories (not authored by me):

bank-statement-import/account_statement_import_file/tests/test_account_statement_import_file.py

Lines 12 to 47 in 7204b64

	@classmethod
	def setUpClass(cls):
	super().setUpClass()
	cls.eur_currency = cls.env["res.currency"].search(
	[("name", "=", "EUR"), ("active", "=", False)], limit=1
	)
	cls.eur_currency.write({"active": True})
	cls.bank_account = cls.env["res.partner.bank"].create(
	{"acc_number": "1111111111", "partner_id": cls.env.company.partner_id.id}
	)
	cls.bank_account_2 = cls.env["res.partner.bank"].create(
	{"acc_number": "3333333333", "partner_id": cls.env.company.partner_id.id}
	)
	cls.journal_1 = cls.env["account.journal"].create(
	{
	"name": "Test asset journal-1",
	"code": "AST-1",
	"type": "bank",
	"bank_account_id": cls.bank_account.id,
	}
	)
	cls.journal_2 = cls.env["account.journal"].create(
	{
	"name": "Test asset journal-2",
	"code": "AST-2",
	"type": "bank",
	"bank_account_id": cls.bank_account.id,
	}
	)
	f_path = file_path("account_statement_import_file/tests/test_file.txt")
	file = base64.b64encode(open(f_path, "rb").read())
	cls.import_wizard = (
	cls.env["account.statement.import"]
	.with_context(journal_id=cls.journal_1.id)
	.create({"statement_file": file, "statement_filename": "Test"})
	)

This demonstrates how some refactoring can replace setUp with setUpClass. You can infer the "rules of the game" directly from the code or conduct your own research on the topic to implement similar improvements.

Thanks again for considering this!

[remi-filament]

Thanks for the links, that was easy, I just force pushed with the update of test class.

[remi-filament]

Great @remi-filament, I'll do a code review but in order to have a runboat instance U need all tests in green.

Thanks, runboat does not need code coverage tests to be green, but for some reason it was not completed. Anyhow I rebased on branch 17.0 and force pushed to get it back.