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

[15.0][MIG] account_payment_term_partner_holiday: Migration to version 15.0 #523

Merged

Conversation

LudLaf
Copy link
Contributor

@LudLaf LudLaf commented Jul 19, 2022

TT37271

@Tecnativa

Standard migration to version 15.0

@LudLaf LudLaf mentioned this pull request Jul 19, 2022
22 tasks
victoralmau and others added 6 commits July 19, 2022 17:40
…_in_holiday() to filtered correctly when day_to is less than date to compare [IMP] Improve is_date_in_holiday() to work fine in child partners [IMP] Add constraint to holidays to prevent month_to < month_from

[FIX] account_payment_term_partner_holiday: Change month_from and month_to fields to convert in 2 characters to order correctly holidays + Script migration
…ds account_payment_term_extension and change the way to set correctly due date according payment term lines definition (not +1 day always)
@LudLaf LudLaf force-pushed the 15.0-mig-account_payment_term_partner_holiday branch 2 times, most recently from e8fc9bf to b717be7 Compare July 19, 2022 16:45
@LudLaf
Copy link
Contributor Author

LudLaf commented Jul 19, 2022

@chienandalu please review.

@pedrobaeza please review.

Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Thanks @LudLaf This should be changed thou:

Comment on lines 37 to 44
def action_post(self):
"""Inject a context for getting the partner when computing payment term.
The trade-off is that we should split the call to super record per record,
but it shouldn't impact in performance.
"""
for item in self:
_item = item.with_context(move_partner_id=item.partner_id.id)
return super(AccountMove, _item).action_post()
Copy link
Member

Choose a reason for hiding this comment

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

Not a migration bug but it should be fixed though. This code will allways exit in the first loop iteration. So it will break the expected behavior when several invoices are posted with this method. Attending to the core method design (odoo/odoo/addons/account/models/account_move.py#L3081-L3086), this would be IMO a better approach:

Suggested change
def action_post(self):
"""Inject a context for getting the partner when computing payment term.
The trade-off is that we should split the call to super record per record,
but it shouldn't impact in performance.
"""
for item in self:
_item = item.with_context(move_partner_id=item.partner_id.id)
return super(AccountMove, _item).action_post()
def action_post(self):
"""Inject a context for getting the partner when computing payment term.
"""
for move in self:
super(AccountMove, self.with_context(move_partner_id=move.partner_id.id)).action_post()
return False

ping @victoralmau

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, you are right, that is incorrect.

Reviewing in v13 I see that action_post() method does not recompute the payment terms lines (this is only done by form when changing some fields), therefore I think that method could be removed directly. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

@LudLaf LudLaf force-pushed the 15.0-mig-account_payment_term_partner_holiday branch 2 times, most recently from e641b93 to 1eeee6f Compare July 20, 2022 09:59
@LudLaf LudLaf force-pushed the 15.0-mig-account_payment_term_partner_holiday branch from 1eeee6f to 0df8043 Compare July 20, 2022 10:26
Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Let's go with it :)

@LudLaf
Copy link
Contributor Author

LudLaf commented Jul 20, 2022

All changes have been applied. Thank you for your comments.

@pedrobaeza
Copy link
Member

/ocabot migration account_payment_term_partner_holiday
/ocabot merge nobump

@OCA-git-bot OCA-git-bot added this to the 15.0 milestone Jul 20, 2022
@OCA-git-bot
Copy link
Contributor

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

@OCA-git-bot OCA-git-bot merged commit 91c9df6 into OCA:15.0 Jul 20, 2022
@OCA-git-bot
Copy link
Contributor

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

@pedrobaeza pedrobaeza deleted the 15.0-mig-account_payment_term_partner_holiday branch July 20, 2022 10:53
victoralmau added a commit to Tecnativa/account-payment that referenced this pull request Jul 21, 2022
victoralmau added a commit to Tecnativa/account-payment that referenced this pull request Jul 22, 2022
antoniospneto pushed a commit to Engenere/account-payment that referenced this pull request Dec 9, 2023
antoniospneto pushed a commit to Engenere/account-payment that referenced this pull request Feb 4, 2024
PabloCatalinaCastello pushed a commit to PabloCatalinaCastello/account-payment that referenced this pull request Jul 2, 2024
PabloCatalinaCastello pushed a commit to PabloCatalinaCastello/account-payment that referenced this pull request Jul 4, 2024
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.

5 participants