Skip to content

Conversation

@quirino95
Copy link

No description provided.

@quirino95 quirino95 force-pushed the 16.0-mig-account_payment_tier_validation branch from dc3aa85 to 4aaf1b3 Compare July 18, 2025 12:34
Copy link

@SirPyTech SirPyTech left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!
Please check the migration process that is used in OCA for migrating to this major version: https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-16.0.
This can be adapted to a backward-migration (like this one, from 18.0 to 16.0) adapting a few steps, for instance

git format-patch --keep-subject --stdout origin/16.0..origin/15.0 -- $module | git am -3 --keep

in this case should be

git format-patch --keep-subject --stdout origin/16.0..origin/18.0 -- $module | git am -3 --keep

Also note that commits should be squashed according to https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests.
Right now the commits are
image
and some of them should be squashed together.


Please fix the tests because right now they are failing:
image
If the CI fails then this PR can't be merged.
The first failure is:

2025-07-18 12:37:19,765 330 ERROR odoo odoo.addons.account_payment_line.tests.test_account_payment_line: FAIL: TestAccountPaymentLines.test_01_customer_payment
Traceback (most recent call last):
  File "/__w/account-payment/account-payment/account_payment_line/tests/test_account_payment_line.py", line 201, in test_01_customer_payment
    self.assertTrue(new_payment.is_reconciled)
AssertionError: False is not true

(from https://github.com/OCA/account-payment/actions/runs/16370675214/job/46258021642?pr=859#step:8:273)
I know it's not the module you are migrating, but if this PR is causing it to fail you have two options:

  • (best) adapt the code so that it doesn't cause a failure in other modules
  • mark both modules as not compatible and split the CI so that they are tested independently.
    See https://github.com/OCA/oca-addons-repo-template/blob/d1483d585edc2323e8faba77c9294e5e2a4176b8/copier.yml#L147-L153:

    Are there in this repo modules that don't get along with their friends? If so, list them here (YAML format) and they will be tested in separate jobs.
    Beware, if rebel modules should stay separated in groups, you should join them with commas, which could be misinterpreted by YAML.
    Example: ["rebel_module_1,rebel_module_2", even_more_rebel_module]


You might want to look at related implementations like:


_tier_validation_manual_config = False

def action_post(self):

Choose a reason for hiding this comment

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

This method is overwriting core's method defined in https://github.com/odoo/odoo/blob/b0be89a67d0ce458db99f4331e21ed56d3c4774a/addons/account/models/account_payment.py#L934 without calling it with super, this breaks the inheritance chain.
Please add a call to super's method.

Choose a reason for hiding this comment

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

👍

"the recipient bank account must be manually validated. "
"You should go on the partner bank account of %(partner)s"
"in order to validate it.",
method_name=self.payment_method_line_id.name,

Choose a reason for hiding this comment

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

Suggested change
method_name=self.payment_method_line_id.name,
method_name=payment.payment_method_line_id.name,

Choose a reason for hiding this comment

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

👍

@quirino95 quirino95 force-pushed the 16.0-mig-account_payment_tier_validation branch 5 times, most recently from c534c8c to d4bc558 Compare July 22, 2025 14:58
@quirino95
Copy link
Author

Thank you @SirPyTech !
I did format-patch and squashed commits. I've updated also behavior in account_payment.py by taking code from the "previous attempt", and as consequence all tests are now ok!

Copy link

@SirPyTech SirPyTech left a comment

Choose a reason for hiding this comment

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

chore: Please check again the commit history, right now it's
Image
but according to https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-16.0 it should be:

  1. All commits that come from git format-patch (mentioned in #859 (review)), merged according to https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests#mergesquash-the-commits-generated-by-bots-or-weblate
  2. An automatic commit generated by pre-commit
  3. The [MIG] commit with the needed manual changes to make the module work in 16.0

Right now there are some commits after the [MIG] one that shouldn't be there.


@alexeirivera87 we are reviving your old PR #664, would you like to have a look?

Choose a reason for hiding this comment

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

question: I don't think this file is needed in 16.0 (maybe not even in 18.0), could you please check and remove it eventually?

Copy link
Author

Choose a reason for hiding this comment

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

Confirm, it's not needed. Removed.

Choose a reason for hiding this comment

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

👍


_tier_validation_manual_config = False

def action_post(self):

Choose a reason for hiding this comment

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

👍

"the recipient bank account must be manually validated. "
"You should go on the partner bank account of %(partner)s"
"in order to validate it.",
method_name=self.payment_method_line_id.name,

Choose a reason for hiding this comment

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

👍

Choose a reason for hiding this comment

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

chore: This file (and most of the module) is an almost exact copy of https://github.com/OCA/account-payment/pull/664/files#diff-9cef5e44f9cd3a54a978109fc6e89d61feb1d100206e820022a24efbf5e2a3ca, please at least add the author @alexeirivera87 as a co-author (see https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors) of the commit.
And I dare say they also deserve an author spot in the module itself.

Copy link
Author

Choose a reason for hiding this comment

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

Of course, I've added Alexei as co-author in my last commit.

Choose a reason for hiding this comment

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

👍

_inherit = ["account.payment", "tier.validation"]
_state_from = ["draft"]
_state_to = ["posted"]
_cancel_state = "canceled"

Choose a reason for hiding this comment

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

suggestion: This is already declared in tier.validation (https://github.com/OCA/server-ux/blob/a6a3fb51c0f85f2031da1aba09328c1deb3b2fb4/base_tier_validation/models/tier_validation.py#L34), is it really needed here?

Choose a reason for hiding this comment

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

👍

class AccountPayment(models.Model):
_name = "account.payment"
_inherit = ["account.payment", "tier.validation"]
_state_from = ["draft"]

Choose a reason for hiding this comment

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

suggestion: This is already declared in tier.validation (https://github.com/OCA/server-ux/blob/a6a3fb51c0f85f2031da1aba09328c1deb3b2fb4/base_tier_validation/models/tier_validation.py#L32), is it really needed here?

Choose a reason for hiding this comment

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

👍

_cancel_state = "canceled"

_tier_validation_manual_config = False

Choose a reason for hiding this comment

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

question: comparing this file with https://github.com/OCA/account-payment/pull/664/files#diff-9cef5e44f9cd3a54a978109fc6e89d61feb1d100206e820022a24efbf5e2a3ca I see here we are missing the override of _get_under_validation_exceptions, was it causing any issues? Otherwise I'd say we can keep it.

Copy link
Author

Choose a reason for hiding this comment

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

No issues keeping that method.

Choose a reason for hiding this comment

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

👍

Comment on lines 62 to 83
def _get_need_validation(self):
"""
Needed because state is a related field not stored,
_compute_need_validation (field need_validation) in model
tier_validation, does not properly work when executing button
"Validate"
"""
self.ensure_one()
if isinstance(self.id, models.NewId):
return False
tiers = self.env["tier.definition"].search([("model", "=", self._name)])
valid_tiers = any([self.evaluate_tier(tier) for tier in tiers])
return (
not self.review_ids
and valid_tiers
and self._context.get("validate_in_progress")
)

Choose a reason for hiding this comment

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

suggestion: This is a slightly modified copy (the only change being the context usage) of an old version of _compute_need_validation (https://github.com/OCA/server-ux/blob/abf06d7e9365080987a2181205cbf017da736671/base_tier_validation/models/tier_validation.py#L216-L225):

    def _compute_need_validation(self):
        for rec in self:
            if isinstance(rec.id, models.NewId):
                rec.need_validation = False
                continue
            tiers = self.env["tier.definition"].search([("model", "=", self._name)])
            valid_tiers = any([rec.evaluate_tier(tier) for tier in tiers])
            rec.need_validation = (
                not rec.review_ids and valid_tiers and rec._check_state_from_condition()
            )

Right now the code has been improved (https://github.com/OCA/server-ux/blob/a6a3fb51c0f85f2031da1aba09328c1deb3b2fb4/base_tier_validation/models/tier_validation.py#L251-L271):

    def _compute_need_validation(self):
        for rec in self:
            if isinstance(rec.id, models.NewId):
                rec.need_validation = False
                continue
            tiers = (
                self.env["tier.definition"]
                .with_context(active_test=True)
                .search(
                    [
                        ("model", "=", self._name),
                        ("company_id", "in", [False] + rec._get_company().ids),
                    ]
                )
            )
            valid_tiers = tiers.filtered(lambda x: rec.evaluate_tier(x))
            requested_tiers = rec.review_ids.filtered(
                lambda x: x.status != "pending"
            ).mapped("definition_id")
            new_tiers = valid_tiers - requested_tiers
            rec.need_validation = new_tiers and rec._check_state_from_condition()

, for instance with multi-company management, could you please update this code accordingly?

Or even better, we could try adding a depends_context to an override of _compute_need_validation so that we won't need to keep this copy updated, what do you think?

Copy link
Author

Choose a reason for hiding this comment

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

I went with the update of the code, after several attempts to get override to work. As mentioned in docstring, _compute_need_validation in this case doesn't work properly, so I preferred to keep _get_need_validation, improved with multi-company management.

Choose a reason for hiding this comment

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

That's fine, I understand it's not easy to adapt an override to work with the new context value.

But what about the last few lines of the above method?

            valid_tiers = tiers.filtered(lambda x: rec.evaluate_tier(x))
            requested_tiers = rec.review_ids.filtered(
                lambda x: x.status != "pending"
            ).mapped("definition_id")
            new_tiers = valid_tiers - requested_tiers
            rec.need_validation = new_tiers and rec._check_state_from_condition()

I see they are still missing in the latest code of this PR:

        valid_tiers = tiers.filtered(lambda x: self.evaluate_tier(x))
        return (
            not self.review_ids
            and valid_tiers
            and self._context.get("validate_in_progress")
        )

Choose a reason for hiding this comment

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

Here your reply from #859 (comment):

using the "new code", behavior changes and method doesn't work anymore as expected. No ValidationError is throwed so validation is always ok.
Looking at server-ux module, i saw that status field in tier.review in 18.0 has a new waiting selection, missing in 16.0. I did some attempts to fix behavior with new code but without success.
Anyway, all tests are ok and the behavior is the expected one, exactly as in v18.

What I am proposing has nothing to do with 18.0: the additional code has been added in 16.0 (where there is no waiting status`) with OCA/server-ux@a64e791, in the commit itself and its PR OCA/server-ux#928 you can find the reason behind it.

From a more generic point of view: the implementation in this model is kind of a needed hack for tier validation to work in a model where state field is related, so some of the usual methods in tier.validation do not get triggered properly.
The needed hack consists of using the context validate_in_progress to recognize when we need to validate the record: this is then used in places like _check_state_conditions_custom, while in tier.validation it is done in _check_state_conditions and _check_state_from_condition.
So this model has to re-implement all the places where such methods are used and:

  • if there is _check_state_from_condition then use the context validate_in_progress
  • if there is _check_state_conditions then use _check_state_conditions_custom instead

This has been done where needed: write and need_validation methods (see https://github.com/OCA/server-ux/blob/23e96d195dd78187789f42264a206350e470b01c/base_tier_validation/models/tier_validation.py).
As I suggested in #859 (comment), doing this with overrides would be more maintainable, but since it's technically harder, it's ok to copy/paste the methods.

If the copy-paste is not in sync (like in this piece of code), that would cause an unexpected behavior because payments validation would behave differently than any other tier-validated model (like invoices, when account_move_tier_validation is installed).

Choose a reason for hiding this comment

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

👍

@quirino95 quirino95 force-pushed the 16.0-mig-account_payment_tier_validation branch from d4bc558 to 2b37547 Compare July 24, 2025 13:04
Copy link

@SirPyTech SirPyTech left a comment

Choose a reason for hiding this comment

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

issue: The commits history looks duplicated:
Image

Have a look at the 1st commit of this PR (b07ee98), the digest in the readme is 0baca...:
Image.
The first commits in 18.0 for this module are (note that they are in reversed order: latest one is above the others): Image

According to https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests they should all be merged together, so the digest of the resulting commit should be the one in the latest commit (7851a7e): Image
That is: 6e4f14....

So the bot's commit has not been merged, it has been skipped.

I'm afraid other commits might be affected too, please have a look.

Choose a reason for hiding this comment

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

👍

Choose a reason for hiding this comment

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

👍

_cancel_state = "canceled"

_tier_validation_manual_config = False

Choose a reason for hiding this comment

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

👍

_inherit = ["account.payment", "tier.validation"]
_state_from = ["draft"]
_state_to = ["posted"]
_cancel_state = "canceled"

Choose a reason for hiding this comment

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

👍

class AccountPayment(models.Model):
_name = "account.payment"
_inherit = ["account.payment", "tier.validation"]
_state_from = ["draft"]

Choose a reason for hiding this comment

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

👍

Comment on lines 62 to 83
def _get_need_validation(self):
"""
Needed because state is a related field not stored,
_compute_need_validation (field need_validation) in model
tier_validation, does not properly work when executing button
"Validate"
"""
self.ensure_one()
if isinstance(self.id, models.NewId):
return False
tiers = self.env["tier.definition"].search([("model", "=", self._name)])
valid_tiers = any([self.evaluate_tier(tier) for tier in tiers])
return (
not self.review_ids
and valid_tiers
and self._context.get("validate_in_progress")
)

Choose a reason for hiding this comment

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

That's fine, I understand it's not easy to adapt an override to work with the new context value.

But what about the last few lines of the above method?

            valid_tiers = tiers.filtered(lambda x: rec.evaluate_tier(x))
            requested_tiers = rec.review_ids.filtered(
                lambda x: x.status != "pending"
            ).mapped("definition_id")
            new_tiers = valid_tiers - requested_tiers
            rec.need_validation = new_tiers and rec._check_state_from_condition()

I see they are still missing in the latest code of this PR:

        valid_tiers = tiers.filtered(lambda x: self.evaluate_tier(x))
        return (
            not self.review_ids
            and valid_tiers
            and self._context.get("validate_in_progress")
        )

payment.amount = 100
payment.action_post()
self.assertEqual(payment.state, "in_process")
self.assertEqual(payment.state, "draft")

Choose a reason for hiding this comment

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

question: Looks like the in_process status of 18.0 corresponds to the posted status in 16.0, so why is draft used here instead?

Copy link
Author

Choose a reason for hiding this comment

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

Updated test using posted instead of draft (added also the two lines before in order to don't break tests.

Copy link
Author

Choose a reason for hiding this comment

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

Can't reply to previous thread (_get_need_validation), so wrote response here: using the "new code", behavior changes and method doesn't work anymore as expected. No ValidationError is throwed so validation is always ok.
Looking at server-ux module, i saw that status field in tier.review in 18.0 has a new waiting selection, missing in 16.0. I did some attempts to fix behavior with new code but without success.
Anyway, all tests are ok and the behavior is the expected one, exactly as in v18.

Choose a reason for hiding this comment

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

Can't reply to previous thread (_get_need_validation)

The thread is #859 (comment)

Choose a reason for hiding this comment

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

Updated test using posted instead of draft (added also the two lines before in order to don't break tests.

👍

@quirino95 quirino95 force-pushed the 16.0-mig-account_payment_tier_validation branch from 2b37547 to a4feb43 Compare July 28, 2025 12:28
Copy link

@SirPyTech SirPyTech left a comment

Choose a reason for hiding this comment

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

Now the commits history is fine 👏

Comment on lines 62 to 83
def _get_need_validation(self):
"""
Needed because state is a related field not stored,
_compute_need_validation (field need_validation) in model
tier_validation, does not properly work when executing button
"Validate"
"""
self.ensure_one()
if isinstance(self.id, models.NewId):
return False
tiers = self.env["tier.definition"].search([("model", "=", self._name)])
valid_tiers = any([self.evaluate_tier(tier) for tier in tiers])
return (
not self.review_ids
and valid_tiers
and self._context.get("validate_in_progress")
)

Choose a reason for hiding this comment

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

Here your reply from #859 (comment):

using the "new code", behavior changes and method doesn't work anymore as expected. No ValidationError is throwed so validation is always ok.
Looking at server-ux module, i saw that status field in tier.review in 18.0 has a new waiting selection, missing in 16.0. I did some attempts to fix behavior with new code but without success.
Anyway, all tests are ok and the behavior is the expected one, exactly as in v18.

What I am proposing has nothing to do with 18.0: the additional code has been added in 16.0 (where there is no waiting status`) with OCA/server-ux@a64e791, in the commit itself and its PR OCA/server-ux#928 you can find the reason behind it.

From a more generic point of view: the implementation in this model is kind of a needed hack for tier validation to work in a model where state field is related, so some of the usual methods in tier.validation do not get triggered properly.
The needed hack consists of using the context validate_in_progress to recognize when we need to validate the record: this is then used in places like _check_state_conditions_custom, while in tier.validation it is done in _check_state_conditions and _check_state_from_condition.
So this model has to re-implement all the places where such methods are used and:

  • if there is _check_state_from_condition then use the context validate_in_progress
  • if there is _check_state_conditions then use _check_state_conditions_custom instead

This has been done where needed: write and need_validation methods (see https://github.com/OCA/server-ux/blob/23e96d195dd78187789f42264a206350e470b01c/base_tier_validation/models/tier_validation.py).
As I suggested in #859 (comment), doing this with overrides would be more maintainable, but since it's technically harder, it's ok to copy/paste the methods.

If the copy-paste is not in sync (like in this piece of code), that would cause an unexpected behavior because payments validation would behave differently than any other tier-validated model (like invoices, when account_move_tier_validation is installed).

payment.amount = 100
payment.action_post()
self.assertEqual(payment.state, "in_process")
self.assertEqual(payment.state, "draft")

Choose a reason for hiding this comment

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

Can't reply to previous thread (_get_need_validation)

The thread is #859 (comment)

payment.amount = 100
payment.action_post()
self.assertEqual(payment.state, "in_process")
self.assertEqual(payment.state, "draft")

Choose a reason for hiding this comment

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

Updated test using posted instead of draft (added also the two lines before in order to don't break tests.

👍

@quirino95 quirino95 force-pushed the 16.0-mig-account_payment_tier_validation branch from a4feb43 to 89f0adc Compare July 31, 2025 08:07
@quirino95
Copy link
Author

Thank you Simone for the explanation. Now it's clearer for me, so I updated code.
I hope now everything is fine.

Comment on lines 62 to 83
def _get_need_validation(self):
"""
Needed because state is a related field not stored,
_compute_need_validation (field need_validation) in model
tier_validation, does not properly work when executing button
"Validate"
"""
self.ensure_one()
if isinstance(self.id, models.NewId):
return False
tiers = self.env["tier.definition"].search([("model", "=", self._name)])
valid_tiers = any([self.evaluate_tier(tier) for tier in tiers])
return (
not self.review_ids
and valid_tiers
and self._context.get("validate_in_progress")
)

Choose a reason for hiding this comment

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

👍

@quirino95 quirino95 force-pushed the 16.0-mig-account_payment_tier_validation branch 2 times, most recently from 0ea9e0a to 939b696 Compare August 1, 2025 13:00
@quirino95
Copy link
Author

quirino95 commented Aug 1, 2025

After OCA/server-ux#1135, I had to change a couple of occurences of validated field, which not exists in server-ux for 16.0. I replaced it with validation_status = 'validated'

@github-actions
Copy link

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.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Nov 30, 2025
@rvalyi
Copy link
Member

rvalyi commented Nov 30, 2025

/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-859-by-rvalyi-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Nov 30, 2025
Signed-off-by rvalyi
@OCA-git-bot
Copy link
Contributor

@rvalyi your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-859-by-rvalyi-bump-nobump.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@SirPyTech SirPyTech force-pushed the 16.0-mig-account_payment_tier_validation branch from 939b696 to ed4ff70 Compare December 1, 2025 08:38
@SirPyTech
Copy link

Rebased to check merge failure

@quirino95 quirino95 force-pushed the 16.0-mig-account_payment_tier_validation branch from ed4ff70 to febdd7b Compare December 1, 2025 13:08
@quirino95
Copy link
Author

Fixed tests failure!

@github-actions github-actions bot removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Dec 7, 2025
@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). 🤖

@SirPyTech
Copy link

/ocabot merge nobump

(from #859 (comment))

@rvalyi please try again

@alexeirivera87
Copy link

@SirPyTech @quirino95

Thanks for considering my old PR

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.

8 participants