[16.0][MIG] sale_commission_partial_settlement: Migration to 16.0

[dalonsofl]

Migration from version 14.0 to 16.0

[odooNextev]

Functional ok

[amarcosg]

Code review LGTM!

[OCA-git-bot]

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

[Daniel-Melian]

I have functionally reviewed the addon and identified an unexpected behavior regarding commission recalculation.

When using the "Full Amount" payment amount type, if a commission is already generated and subsequently deleted, re-triggering the settlement generates the commission again as expected.
However, with the "Paid Amount" type, the process differs: you need to first break the reconciliation between the payments and the invoice, then re-reconcile them to allow the commission to be generated once more.
This inconsistency between the "Full Amount" and "Paid Amount" processes may lead to unexpected results and could cause workflow disruptions.

Steps to Reproduce:

Use the "Full Amount" type to generate a commission, delete it, and observe the behavior on re-triggering the settlement.
Use the "Paid Amount" type, generate a commission, delete it, and note that reconciliation must be broken and re-established before the commission can be regenerated.

[francesco-ooops]

@Daniel-Melian we've identified the same issue in these days, we're working on a fix that you can then fw-port to 16

[francesco-ooops]

Please include this fix: #575

[rousseldenis]

/ocabot migration sale_commission_partial_settlement

[dalonsofl]

Hi @francesco-ooops @Daniel-Melian !

I have made the changes related to the fix you mentioned, if you can check that everything is correct.

Thanks!

Regards

[Daniel-Melian]

@dalonsofl I have reviewed the changes, and indeed it is now regenerating correctly.

Thanks!

[SirAionTech]

@dalonsofl I am seeing some strange conflicts when rebasing this branch locally on 16.0, and also looks like some commits are repeated, like 42e9914 and bf48b2f.
Could you please check? Thanks!

[francesco-ooops]

@dalonsofl can you remove the extra modules from this PR?

[dalonsofl]

Hi @francesco-ooops @SirAionTech !

I think it should be resolved by now.

Regards

[francesco-ooops]

@dalonsofl I don't think it's correct to include the fix applied in #575 in the migration commit, wdyt @SirAionTech ?

[SirAionTech]

@dalonsofl I don't think it's correct to include the fix applied in #575 in the migration commit, wdyt @SirAionTech ?

I haven't looked at the new code yet, so I can only say that generally the migration commit should only contain what is needed for the old module to work in the new version.

[dalonsofl]

@dalonsofl I don't think it's correct to include the fix applied in #575 in the migration commit, wdyt @SirAionTech ?

I haven't looked at the new code yet, so I can only say that generally the migration commit should only contain what is needed for the old module to work in the new version.

So, because this reason, it makes sense the fixed applied in #575 is in the migration commit because it is necessary for worlking properly, isn't it?

[francesco-ooops]

You probably should cherry pick it Il gio 19 dic 2024, 13:28 David Alonso ***@***.***> ha scritto:

…

@dalonsofl <https://github.com/dalonsofl> I don't think it's correct to include the fix applied in #575 <#575> in the migration commit, wdyt @SirAionTech <https://github.com/SirAionTech> ? I haven't looked at the new code yet, so I can only say that generally the migration commit should only contain what is needed for the old module to work in the new version. So, because this reason, it makes sense the fixed applied in #575 <#575> is in the migration commit because it is necessary for worlking properly, isn't it? — Reply to this email directly, view it on GitHub <#561 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANHLKUOUUDMM7BRL7ZMONTD2GK3WBAVCNFSM6AAAAABOBAUED6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNJTGY4DOMZXGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[SirAionTech]

@dalonsofl I don't think it's correct to include the fix applied in #575 in the migration commit, wdyt @SirAionTech ?

I haven't looked at the new code yet, so I can only say that generally the migration commit should only contain what is needed for the old module to work in the new version.

So, because this reason, it makes sense the fixed applied in #575 is in the migration commit because it is necessary for worlking properly, isn't it?

No because the fix in #575 is not needed for the module to work in 16.0: it is needed for the module to work in general, and has been implemented when the module was in 14.0.

A migration from 14.0 to 16.0 should have:

• commits from 14.0, squashed if needed.
  Somewhere in here there is also the commit of [14.0][FIX] sale_commission_partial_settlement: handle settlement deletion #575
• 1 commit for automatic pre-commit adjustments because 16.0 configuration might be different than 14.0 configuration
• 1 migration commit: this adapts the module (that before this commit works in 14.0) to work correctly in 16.0.

The full description of what should be done, and how, is in https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-16.0.