[12.0][MIG] account_invoice_line_default_account #1143

SirTakobi · 2022-04-21T15:02:57Z

Migrating/Adding account_invoice_line_default_account to 12.0.

This poor module has a troubled history: as far as I have found, it was brought from launchpad's 6.1 (unmerged) to 8.0 with #23, then #189, and finally #356 (currently still open).

Then a migration to 10.0 happened in #351 (currently still open).

I tried to put together something like a consistent commit history and this is the result, with a last commit for migrating to 12.0.

Additional notes:

I added some tests 🎉
There is a fixup commit because I couldn't find where the __openerp__ was removed between [8.0] account_invoice_line_default_account #356 and [10.0] account_invoice_line_default_account #351, I didn't squash it in order not to mix authorships, but I can do it if @jbaudoux agrees
About the roadmap:
- For account.invoice.line._onchange_account_id I already tried substituting write with update but that results in not updating the partner (can be verified with the tests),
- For account.invoice.line.account_id, currently the module assumes the previous default method is the one defined in account but that might not be True; retrieving the field's default inside the defined method is useless because it has already been changed to the defined method
If you have any suggestion it is welcome

Looking at the linked PRs and commits, @jbaudoux you might want to have a look :)

…Therp) and Alexandre Fayolle (CampToCamp): - No hyphenation in module description - Better module name - Typo's fixed - Streamline access to context dictionary: context.get('key') (../account-financial-tools-6.1 rev 100.1.2)

(../account-financial-tools-6.1 rev 100.1.3)

(../account-financial-tools-6.1 rev 100.1.4)

…pplier (../account-financial-tools-6.1 rev 101)

(../account-financial-tools-6.1 rev 103)

jbaudoux

Thanks for adding tests.

Some minor remarks.

I remember when I ported to v10 that lot of code had to be rewritten, that's why I squashed all the old commits. I don't think it is useful to recover all those old commits and I would squash them all into a single commit or at least reduce them a lot.

jbaudoux · 2022-04-21T15:49:46Z

account_invoice_line_default_account/models/account_invoice_line.py

+ account_id = fields.Many2one(default=_account_id_default)
+
+ @api.onchange('account_id')
+ def _onchange_account_id(self):


maybe better to use a different name to not have to call super

Thanks for the review!
I can see no harm in calling super, actually it looks to me like a common thing to call super when overriding a method; why should this case be treated in a different way?

Before api.onchange, there could be only a single method called on an onchange. With api.onchange you can have multiple different methods. The conditions are not to test if super has to be called or not but if this method has to be executed. So I find the current implementation hard to read.
Nevertheless it will work. Just a minor remark.

That makes perfect sense: overriding the method is really needed only when you want to edit the method's behavior, but that does not have to be the way to go with the onchange methods.
I have done this change too, with a bit of explanation in the comments, let me know if it looks better now :)

Looks clear now

jbaudoux · 2022-04-21T15:49:55Z

account_invoice_line_default_account/models/account_invoice_line.py

+ def _onchange_account_id(self):
+ if not self.account_id or not self.partner_id or self.product_id:
+ return super(AccountInvoiceLine, self)._onchange_account_id()
+ if self._context.get('journal_id'):


self.env.context

Thanks, done :)

rafaelbn · 2022-04-21T21:13:05Z

/ocabot migration account_invoice_line_default_account

[ADD] account_invoice_line_default_account [IMP] account_invoice_line_default_account: updated with new OCA guidelines [FIX] pep8, README.rst [FIX] view_load is deprecated [FIX] fields.property definition have changed [FIX] from . import + contributors [FIX] account_invoice_line_default_account: Adding the partner in the context of invoice line of customer invoice. It's there in standard in supplier invoices but not in customer invoices and it's required by this module [ADD]account_invoice_line_default_account: default income account on partner similar to default expense account [ADD] translations fr/nl [FIX] __openerp__ file [IMP] look and feel [FIX] pep8

account_invoice_line_default_account: search based on xmlid as name is translatable [FIX] pylint

SirTakobi · 2022-04-22T13:17:50Z

I remember when I ported to v10 that lot of code had to be rewritten, that's why I squashed all the old commits. I don't think it is useful to recover all those old commits and I would squash them all into a single commit or at least reduce them a lot.

@jbaudoux I squashed most of the commits, and added the __openerp__.py removal in 10.0 migration, let me know if it looks better now.
Just as reference, I have kept the previous history in https://github.com/SirTakobi/account-invoicing/tree/12.0-mig-account_invoice_line_default_account-bkp

Can you update your review? Github is not allowing me to re-request you to do it

/ocabot migration account_invoice_line_default_account

@rafaelbn thanks, I'm sorry I forgot to do those things

jbaudoux · 2022-04-22T14:42:14Z

account_invoice_line_sequence/__manifest__.py

@@ -16,6 +16,9 @@
 'data': ['views/account_invoice_view.xml',
 'views/report_invoice.xml'],
 'depends': ['account'],
+ 'excludes': [


Why the 2 modules cannot work together?

I wrote it in the commit message:

This module is not compatible with account_invoice_line_sequence because them both add to the context of invoice lines in invoice form view

jbaudoux

LGTM

eLBati · 2022-04-26T08:10:14Z

.travis.yml

+ - TESTS="1" ODOO_REPO="OCA/OCB" EXCLUDE="account_invoice_line_default_account"
+ - TESTS="1" ODOO_REPO="OCA/OCB" EXCLUDE="account_invoice_line_sequence"
+ - TESTS="1" ODOO_REPO="odoo/odoo" MAKEPOT="1" EXCLUDE="account_invoice_line_default_account"
+ - TESTS="1" ODOO_REPO="odoo/odoo" MAKEPOT="1" EXCLUDE="account_invoice_line_sequence"


Can runboat be informed about this?

https://runboat.odoo-community.org/api/v1/builds/bf7e8fb88-cda7-4c02-a0c2-5717acb6d8bf/init-log

Runboat currently does not support conflicting addons, see sbidoul/runboat#36

eLBati

Proposing SirTakobi#1 to avoid basing on context and thus incompatibilities

SirTakobi · 2022-05-03T07:14:43Z

Proposing SirTakobi#1 to avoid basing on context and thus incompatibilities

@eLBati there are some things that do not really convince me in SirTakobi#1.

Instead of working on the linked PR, can we go on with the merge of this migration alone? Since we already have enough approvals, this module might finally be merged for the first time! 😄
Then you will be able to propose that PR directly to 12.0.
Unless you think there is something in this PR that might be blocking for the merge.

SirTakobi · 2022-05-11T08:47:35Z

@OCA/accounting-maintainers can this be merged or do I have to change something? Thanks!

eLBati · 2022-05-11T18:04:45Z

Unless you think there is something in this PR that might be blocking for the merge

Uhm yes, this SirTakobi#1

This module is not compatible with account_invoice_line_sequence because them both add to the context of invoice lines in invoice form view

…which is always executed while adding invoice lines

SirTakobi · 2022-05-17T15:12:34Z

Unless you think there is something in this PR that might be blocking for the merge

Uhm yes, this SirTakobi#1

Now that you explained those changes, I merged them :)

Just for future reference, my doubts were about choosing the onchange on product_id field specifically, but we clarified that that onchange is called on any line creation from UI; so the onchange can do the job that was previously (when onchanges were not called on line creation) performed from the line's view using the context.

…s account set on line

SirTakobi · 2022-05-17T15:54:41Z

@eLBati can you update your review now? Thanks!

eLBati

Thanks

SirTakobi · 2022-05-24T13:03:28Z

@OCA/accounting-maintainers can this be merged? There are a few happy reviewers already 😄

SirTakobi · 2022-06-29T09:38:10Z

@OCA/accounting-maintainers can this be merged? Thanks!

SirTakobi · 2022-07-13T07:31:57Z

@OCA/accounting-maintainers can this be merged? Thanks!

sergiocorato · 2022-07-13T20:43:34Z

/ocabot merge nobump

OCA-git-bot · 2022-07-13T20:43:44Z

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

OCA-git-bot · 2022-07-13T21:36:49Z

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

NL66278 and others added 6 commits April 21, 2022 09:50

[FIX] refactored code to make it more mantainable.

9c48b73

(../account-financial-tools-6.1 rev 100.1.3)

[FIX] Correction of author name.

6376775

(../account-financial-tools-6.1 rev 100.1.4)

[ADD] Module to track default expense account on invoice lines per su…

437ca6b

…pplier (../account-financial-tools-6.1 rev 101)

[FIX] account_invoice_line_default_account: Correction of author name.

82d239d

(../account-financial-tools-6.1 rev 103)

[MIG] ported module account_invoice_line_default_account to 7.0

06607d5

SirTakobi force-pushed the 12.0-mig-account_invoice_line_default_account branch from 4a08ff6 to 7100a50 Compare April 21, 2022 15:04

jbaudoux reviewed Apr 21, 2022

View reviewed changes

OCA-git-bot added this to the 12.0 milestone Apr 21, 2022

OCA-git-bot mentioned this pull request Apr 21, 2022

Migration to version 12.0 #427

Closed

30 tasks

SirTakobi force-pushed the 12.0-mig-account_invoice_line_default_account branch 2 times, most recently from 8a10d2b to fc1935f Compare April 22, 2022 12:44

SirTakobi mentioned this pull request Apr 22, 2022

Support repos with conflicting addons sbidoul/runboat#36

Open

SirTakobi force-pushed the 12.0-mig-account_invoice_line_default_account branch from fc1935f to ed04280 Compare April 22, 2022 13:02

jbaudoux added 2 commits April 22, 2022 15:10

[MIG] account_invoice_line_default_account: Migration to 10.0

42608c4

account_invoice_line_default_account: search based on xmlid as name is translatable [FIX] pylint

SirTakobi force-pushed the 12.0-mig-account_invoice_line_default_account branch from ed04280 to f5d3e2f Compare April 22, 2022 13:12

SirTakobi marked this pull request as ready for review April 22, 2022 13:29

SirTakobi force-pushed the 12.0-mig-account_invoice_line_default_account branch from f5d3e2f to 28c8e70 Compare April 22, 2022 13:39

jbaudoux reviewed Apr 22, 2022

View reviewed changes

rafaelbn added the needs review label Apr 23, 2022

jbaudoux approved these changes Apr 25, 2022

View reviewed changes

rafaelbn approved these changes Apr 25, 2022

View reviewed changes

OCA-git-bot added the approved label Apr 25, 2022

eLBati reviewed Apr 26, 2022

View reviewed changes

eLBati suggested changes Apr 26, 2022

View reviewed changes

OCA-git-bot removed the approved label Apr 26, 2022

eLBati mentioned this pull request Apr 27, 2022

REF account_invoice_line_default_account using _onchange_product_id, which is always executed while adding invoice lines SirTakobi/account-invoicing#1

Merged

eLBati mentioned this pull request Apr 27, 2022

[12.0] ADD l10n_it_fatturapa_in_invoice_line_default_account: Conto di costo predefinito su fornitore OCA/l10n-italy#2771

Closed

SirTakobi requested a review from eLBati May 4, 2022 07:35

SirTakobi and others added 4 commits May 17, 2022 17:07

[MIG] account_invoice_line_default_account: Migration to 12.0

cf3cc2c

This module is not compatible with account_invoice_line_sequence because them both add to the context of invoice lines in invoice form view

REF account_invoice_line_default_account using _onchange_product_id, …

69b1fc0

…which is always executed while adding invoice lines

ADD italian translation

ae1931a

ADD setup

cec3b69

SirTakobi force-pushed the 12.0-mig-account_invoice_line_default_account branch from 5750f24 to cec3b69 Compare May 17, 2022 15:07

[FIX] account_invoice_line_default_account: Do not overwrite product'…

97767db

…s account set on line

eLBati approved these changes May 19, 2022

View reviewed changes

OCA-git-bot added the approved label May 19, 2022

OCA-git-bot added the bot is merging ⏳ label Jul 13, 2022

OCA-git-bot merged commit d7c8d7f into OCA:12.0 Jul 13, 2022

OCA-git-bot added merged 🎉 and removed bot is merging ⏳ labels Jul 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[12.0][MIG] account_invoice_line_default_account #1143

[12.0][MIG] account_invoice_line_default_account #1143

SirTakobi commented Apr 21, 2022 •

edited

Loading

jbaudoux left a comment •

edited

Loading

jbaudoux Apr 21, 2022

SirTakobi Apr 22, 2022

jbaudoux Apr 22, 2022

SirTakobi Apr 22, 2022

jbaudoux Apr 22, 2022

jbaudoux Apr 21, 2022

SirTakobi Apr 22, 2022

rafaelbn commented Apr 21, 2022

SirTakobi commented Apr 22, 2022 •

edited

Loading

jbaudoux Apr 22, 2022

SirTakobi Apr 22, 2022

jbaudoux left a comment

eLBati Apr 26, 2022

SirTakobi May 3, 2022

eLBati left a comment

SirTakobi commented May 3, 2022

SirTakobi commented May 11, 2022

eLBati commented May 11, 2022

SirTakobi commented May 17, 2022

SirTakobi commented May 17, 2022

eLBati left a comment

SirTakobi commented May 24, 2022

SirTakobi commented Jun 29, 2022

SirTakobi commented Jul 13, 2022

sergiocorato commented Jul 13, 2022

OCA-git-bot commented Jul 13, 2022

OCA-git-bot commented Jul 13, 2022

[12.0][MIG] account_invoice_line_default_account #1143

[12.0][MIG] account_invoice_line_default_account #1143

Conversation

SirTakobi commented Apr 21, 2022 • edited Loading

jbaudoux left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelbn commented Apr 21, 2022

SirTakobi commented Apr 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbaudoux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eLBati left a comment

Choose a reason for hiding this comment

SirTakobi commented May 3, 2022

SirTakobi commented May 11, 2022

eLBati commented May 11, 2022

SirTakobi commented May 17, 2022

SirTakobi commented May 17, 2022

eLBati left a comment

Choose a reason for hiding this comment

SirTakobi commented May 24, 2022

SirTakobi commented Jun 29, 2022

SirTakobi commented Jul 13, 2022

sergiocorato commented Jul 13, 2022

OCA-git-bot commented Jul 13, 2022

OCA-git-bot commented Jul 13, 2022

SirTakobi commented Apr 21, 2022 •

edited

Loading

jbaudoux left a comment •

edited

Loading

SirTakobi commented Apr 22, 2022 •

edited

Loading