[18.0][FIX] product_secondary_unit: UoM conversion in secondary unit mixin

[AungKoKoLin1997]

• Replace _get_factor_line() with uom._compute_quantity() to fix incorrect conversion when line UoM differs from product's base UoM. The previous method failed for certain UoM combinations

Related: OCA/stock-logistics-warehouse#2496

@qrtl QT6285

[OCA-git-bot]

Hi @sergio-teruel,
some modules you are maintaining are being modified, check this out!

[yostashiro]

Code review. LGTM.

[nobuQuartile]

LGTM Code review

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

[kanda999]

Code review.

[AungKoKoLin1997]

In my last commit, I add the configuration for qweb reports presentation.

[yostashiro]

Partial review.

[HviorForgeFlow]

Code review 👍

[mav-adhoc]

LGTM

[HviorForgeFlow]

@rousseldenis could we prioritize merging this, I merged a PR that is working on the top of this new changes, I didn't realize, and now we are blocking many edge users.

OCA/purchase-workflow#2932

[Reyes4711-S73]

LGTM

[HviorForgeFlow]

@pedrobaeza @legalsylvain could you merge this? Appreciate

[pedrobaeza]

Sorry, but the text putting that the previous code fails for certain conversion is not enough for me. In fact, there are no regression tests for those cases. I only see that in the test, the base UoM is changed, making me suspect that something is altering the result of the previous tests.

@AungKoKoLin1997 can you explain the problems? This is also mixing something that is not a fix and the main PR subject. It's OK having it in a different commit, but both should be discussed separately IMO. Starting with, this is only used for sales/purchase, so they should be in their respective modules. OK to have the common selection options here, but not the fields, as if you install only this module and sale, you will see useless options in the configuration. Or am I misunderstanding anything?

[AungKoKoLin1997]

@pedrobaeza
In my first commit, I added a new test case for the UOM conversion fix.

OK to have the common selection options here, but not the fields, as if you install only this module and sale, you will see useless options in the configuration.

I added non-stored fields to hide the options if the related modules aren’t installed, preventing user confusion.
Could you please review?

[pedrobaeza]

I still don't like that. Please add them directly in sale_secondary_unit / purchase_secondary_unit, not here. And please split in 2 PRs for discussing both separatetly.

[AungKoKoLin1997]

@pedrobaeza
The reason I’m adding these fields in this module (instead of sale_order_secondary_unit and purchase_order_secondary_unit) is that I’m planning to introduce an account_move_secondary_unit module. In that module, I’d like to reuse the Sale/Purchase configurations depending on the document type (customer invoice vs. vendor bill).

If the configuration lives only in the sale_secondary_unit / purchase_secondary_unit, then account_move_secondary_unit would likely need to depend on both modules, which I’d prefer to avoid if possible.

So I’m leaning toward adding these fields in product_secondary_unit, so the configuration can be shared and reused across Sale, Purchase, and Invoicing.

[pedrobaeza]

OK, please open a separate PR for that feature, and explain in commit message and in comments that intention and the reason for that. I personally don't like that configurations anyway, as if you install the modules, you should see them. I think you should go instead with security groups.

[AungKoKoLin1997]

@pedrobaeza @yostashiro @rousseldenis @HviorForgeFlow @Reyes4711-S73 @kanda999 @nobuQuartile
I made a separate PR for the report presentation settings. I also hide the settings when the related child modules aren’t installed by using security groups.

#2211

[pedrobaeza]

/ocabot merge major

[OCA-git-bot]

This PR looks fantastic, let's merge it!
Prepared branch 18.0-ocabot-merge-pr-2182-by-pedrobaeza-bump-major, awaiting test results.

[OCA-git-bot]

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