Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[17.0][MIG] account_statement_import_move_line: Migration to version 17.0 #732

Merged

Conversation

pilarvargas-tecnativa
Copy link

@pilarvargas-tecnativa pilarvargas-tecnativa commented Oct 25, 2024

cc @Tecnativa TT51242

@pedrobaeza @victoralmau please review

…atement_import_move_line

It was renamed already in v14, so it must be this new name here, but it
was incorrectly migrated and merged. For keeping compatibility, a copy
is done instead.
@pilarvargas-tecnativa pilarvargas-tecnativa force-pushed the 17.0-mig-account_statement_import_move_line branch 2 times, most recently from bb215d7 to 8e15b15 Compare October 29, 2024 15:27
@pilarvargas-tecnativa pilarvargas-tecnativa marked this pull request as ready for review October 29, 2024 15:30
@pedrobaeza
Copy link
Member

/ocabot migration account_statement_import_move_line

@OCA-git-bot OCA-git-bot added this to the 17.0 milestone Oct 29, 2024
@OCA-git-bot OCA-git-bot mentioned this pull request Oct 29, 2024
11 tasks
@pilarvargas-tecnativa pilarvargas-tecnativa force-pushed the 17.0-mig-account_statement_import_move_line branch 2 times, most recently from 0a5d507 to 630da1e Compare October 30, 2024 06:52
Copy link
Member

@victoralmau victoralmau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code and functional review OK

bank_journal = self.env["account.journal"].search(
[("type", "=", "bank")], limit=1
)
statement.journal_id = bank_journal
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not necessary because it will be defined automatically when a line is created; if you should specify in the _prepare_statement_line_vals() method the journal_id that corresponds.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An statement assigns journals of type "bank" or "cash" to its lines when they are created. In version 15, statement lines did not have an associated journal and the journal of the statement was set. The wizard imports invoice lines that may have a journal other than ‘bank’ or ‘cash’ and when creating them, it must be done with the journal associated with the extract if it already contains previous lines that have established it or, as is done when creating them from the form, establish by default a journal of type ‘bank’ in this case, as they are created from that journal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Being a computed field, you need to infer the journal other way. Include it in the wizard, and fill it depending on the context (I think it's included in the context when coming from the dashboard).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have decided to keep the current logic for cases where a statement with added lines already exists, in case more lines need to be added, limiting the visibility of the current button to the condition "not journal_id."

Additionally, I have added the option to import lines from the dashboard by creating a new statement.

If at some point you permanently lose the form view of the statement, you can continue with the new dashboard option.

@pilarvargas-tecnativa pilarvargas-tecnativa force-pushed the 17.0-mig-account_statement_import_move_line branch 2 times, most recently from b8c8ca1 to 593a240 Compare October 31, 2024 16:37
@pedrobaeza
Copy link
Member

Maybe it's better to reorder the commits to put the fix before migration commits.

…ment lines

The ‘ref’ field is not visible on invoices of type ‘out_invoice’.
In the tests, it is adding value to this field and therefore does not
fail to create a bank statement line where the ‘payment_ref’ field is
required. To solve this, the value of the ‘payment_ref’ field of the
bank statement lines is set as the reference of a supplier invoice
(‘ref’ in invoice lines) or as the reference of the payment
‘payment_reference’ (‘name’ in invoice lines).
@pilarvargas-tecnativa pilarvargas-tecnativa force-pushed the 17.0-mig-account_statement_import_move_line branch from 593a240 to f2e2d73 Compare October 31, 2024 18:47
@pilarvargas-tecnativa pilarvargas-tecnativa force-pushed the 17.0-mig-account_statement_import_move_line branch from f2e2d73 to 86111d3 Compare November 4, 2024 06:41
@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 17.0-ocabot-merge-pr-732-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 7572dc8 into OCA:17.0 Nov 4, 2024
5 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@pedrobaeza pedrobaeza deleted the 17.0-mig-account_statement_import_move_line branch November 4, 2024 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants