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

[16.0][MIG] account_payment_purchase: Migration to 16.0 #1004

Merged
merged 45 commits into from
Jun 1, 2023

Conversation

ramiadavid
Copy link
Contributor

@ramiadavid ramiadavid commented Dec 20, 2022

@ramiadavid ramiadavid mentioned this pull request Dec 20, 2022
14 tasks
@ramiadavid ramiadavid marked this pull request as ready for review December 20, 2022 07:10
@etobella
Copy link
Member

/ocabot migration account_payment_purchase

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Dec 20, 2022
Copy link
Member

@flotho flotho left a comment

Choose a reason for hiding this comment

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

code review,
tested on runboat,
LGTM
Thanks @alexis-via

@ramiadavid ramiadavid force-pushed the 16.0-mig-account_payment_purchase branch from f90f230 to 5c2fe99 Compare December 30, 2022 08:36
@ramiadavid ramiadavid force-pushed the 16.0-mig-account_payment_purchase branch from 5c2fe99 to 1102ba6 Compare January 3, 2023 09:03
Copy link
Contributor

@RodrigoBM RodrigoBM left a comment

Choose a reason for hiding this comment

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

LGTM.

@OCA-git-bot
Copy link
Contributor

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). 🤖

@RodrigoBM
Copy link
Contributor

@etobella can you merge it?

@@ -84,8 +84,8 @@ def test_purchase_order_invoicing(self):
invoice = self.env["account.move"].create(
{"partner_id": self.partner.id, "move_type": "in_invoice"}
)
with Form(invoice) as inv:
inv.purchase_id = self.purchase
invoice.purchase_id = self.purchase
Copy link
Member

Choose a reason for hiding this comment

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

Why are you removing the form? It was done this way in order to keep the same logic done by odoo interface

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etobella AssertionError: can't write on invisible field purchase_id

Copy link
Member

Choose a reason for hiding this comment

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

But with this, you are not testing the real thing, as onchanges are not triggered.

Copy link

@simahawk simahawk Feb 23, 2023

Choose a reason for hiding this comment

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

Having a quick look at how is implemented in core: it sucks. It seems to go against the principle of not having business logic in onchanges. Unfortunately, if you don't set force_save="1" on the field it does not work and - BTW - the field is emptied at the end of the onchange.
Also, if you look at _set_purchase_orders they call this onchange explicitly 🤦
Finally, there's no test on their onchange on purchase_id, likely because they cannot test it 😄

However, if you want to keep it like this you should call _onchange_partner_id too IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simahawk

If I'm not mistaken, the function that is called when the purchase_id field is modified is _onchange_purchase_auto_complete

https://github.com/odoo/odoo/blob/08cf96d9ba169287a7e8d7dece4ac0110fa01742/addons/purchase/models/account_invoice.py#L33:L34

And this is already being called in the test, exactly the same as in the rest of the tests that I have not modified.

invoice._onchange_purchase_auto_complete()

Isn't that correct?

Copy link
Member

@astirpe astirpe Mar 16, 2023

Choose a reason for hiding this comment

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

I just faced the same issue while porting module account_move_line_purchase_info to V16. I think that this part of test may be written the same way as in standard Odoo:
https://github.com/odoo/odoo/blob/16.0/addons/purchase/tests/test_purchase_order_report.py#L76-L79

Something like this:

        with Form(invoice) as inv:
            inv.purchase_vendor_bill_id = self.env['purchase.bill.union'].browse(-self.purchase.id)

@@ -84,8 +84,8 @@ def test_purchase_order_invoicing(self):
invoice = self.env["account.move"].create(
{"partner_id": self.partner.id, "move_type": "in_invoice"}
)
with Form(invoice) as inv:
inv.purchase_id = self.purchase
invoice.purchase_id = self.purchase
Copy link

@simahawk simahawk Feb 23, 2023

Choose a reason for hiding this comment

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

Having a quick look at how is implemented in core: it sucks. It seems to go against the principle of not having business logic in onchanges. Unfortunately, if you don't set force_save="1" on the field it does not work and - BTW - the field is emptied at the end of the onchange.
Also, if you look at _set_purchase_orders they call this onchange explicitly 🤦
Finally, there's no test on their onchange on purchase_id, likely because they cannot test it 😄

However, if you want to keep it like this you should call _onchange_partner_id too IMO.

@classmethod
def setUpClass(cls):
super().setUpClass()
cls.journal = cls.env["account.journal"].create(

Choose a reason for hiding this comment

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

Suggested change
cls.journal = cls.env["account.journal"].create(
cls.env = cls.env(context=dict(cls.env.context, tracking_disable=True))
cls.journal = cls.env["account.journal"].create(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this done

@StephaneMangin
Copy link

cc @ramiadavid Any update about the reviews ?

@ramiadavid ramiadavid force-pushed the 16.0-mig-account_payment_purchase branch from 1102ba6 to b10a653 Compare March 24, 2023 19:15
@HaraldPanten
Copy link

@ramiadavid is this PR ready to review? THX

@ramiadavid
Copy link
Contributor Author

@HaraldPanten I think yes

@HaraldPanten
Copy link

@ramiadavid Could you rebase, please? I'll test it in runboat. THX!

cubells and others added 20 commits June 1, 2023 11:26
Currently translated at 100.0% (9 of 9 strings)

Translation: bank-payment-12.0/bank-payment-12.0-account_payment_purchase
Translate-URL: https://translation.odoo-community.org/projects/bank-payment-12-0/bank-payment-12-0-account_payment_purchase/pt_BR/
Currently translated at 100.0% (9 of 9 strings)

Translation: bank-payment-12.0/bank-payment-12.0-account_payment_purchase
Translate-URL: https://translation.odoo-community.org/projects/bank-payment-12-0/bank-payment-12-0-account_payment_purchase/ca/
Currently translated at 100.0% (8 of 8 strings)

Translation: bank-payment-13.0/bank-payment-13.0-account_payment_purchase
Translate-URL: https://translation.odoo-community.org/projects/bank-payment-13-0/bank-payment-13-0-account_payment_purchase/pt_BR/
Currently translated at 100.0% (8 of 8 strings)

Translation: bank-payment-13.0/bank-payment-13.0-account_payment_purchase
Translate-URL: https://translation.odoo-community.org/projects/bank-payment-13-0/bank-payment-13-0-account_payment_purchase/es_AR/
Currently translated at 100.0% (8 of 8 strings)

Translation: bank-payment-14.0/bank-payment-14.0-account_payment_purchase
Translate-URL: https://translation.odoo-community.org/projects/bank-payment-14-0/bank-payment-14-0-account_payment_purchase/fr_FR/
Currently translated at 62.5% (5 of 8 strings)

Translation: bank-payment-14.0/bank-payment-14.0-account_payment_purchase
Translate-URL: https://translation.odoo-community.org/projects/bank-payment-14-0/bank-payment-14-0-account_payment_purchase/sv/
…or bank PO

- Only issue a warning message if the PO had a non-blank payment mode or bank.
…ccount

Without this fix, the partner_bank_id was being reset to False on change of 'company_id'
in the _onchange_purchase_auto_complete method in account.move in the following unit test:
def test_from_purchase_order_invoicing_bank

That's because in commit 9be9766 we switched to
getting payment method from env.ref('account.account_payment_method_manual_out'), which has
bank_account_required == False by default, while before that we were creating payment method for
which we were explicitly setting bank_account_required as True

Also tag unit test as post_install
@ramiadavid ramiadavid force-pushed the 16.0-mig-account_payment_purchase branch from b10a653 to 3094e7e Compare June 1, 2023 09:27
@ramiadavid
Copy link
Contributor Author

@HaraldPanten done

Copy link

@HaraldPanten HaraldPanten left a comment

Choose a reason for hiding this comment

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

THX David,

Functional review LGTM.

@pedrobaeza GO?

@pedrobaeza
Copy link
Member

/ocabot migration account_payment_purchase
/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

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

@OCA-git-bot OCA-git-bot merged commit 2fcc748 into OCA:16.0 Jun 1, 2023
@OCA-git-bot
Copy link
Contributor

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

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.