[14.0][MIG] purchase_procurement_analytic

[rousseldenis]

No description provided.

[AaronHForgeFlow]

Hi @rousseldenis what is the status of this PR? is the module still needed?

[rousseldenis]

Hi @rousseldenis what is the status of this PR? is the module still needed?

This is currently in production for months and ready for review.

[rousseldenis]

@AaronHForgeFlow I've updated this by reactivating tests, and introducing a grouping strategy option. Per line or per order.

@cubells @pedrobaeza @carlosdauden Review is welcome.

[pedrobaeza]

/ocabot migration purchase_procurement_analytic

[pedrobaeza]

Not using this anymore, but a quick code review done. Everything seems good except tests

[pedrobaeza]

Uhm, a weird commit separation. Isn't this overlapping with the module procurement_purchase_no_grouping?

[rousseldenis]

Uhm, a weird commit separation. Isn't this overlapping with the module procurement_purchase_no_grouping?

Mmmh, yes and no. This is oriented on analytic account only.

[pedrobaeza]

OK, for me, 2 PO lines with different analytic account should never get merged, but if you think so. Please put anyways the default behavior for not grouping.

[rousseldenis]

OK, for me, 2 PO lines with different analytic account should never get merged, but if you think so.

Neither do I. In standard, analytic account is not managed, so both procurements generate one PO line with merged quantities.

As this module introduces analytic account transmission to PO, you can choose:

• To make one PO per analytic account (default behaviour as this was the behaviour of former versions of this module).
• To reuse same draft PO with different analytic accounts on lines BUT merging only lines with same analytic account.

So, user can choose which behaviour fits its need.

[AaronHForgeFlow]

Functional review, works as specifications

[airlessproject]

Hi all, just a heads up to something we discovered with @rousseldenis in another PR, that most of the functionality in purchase_procurement_analytic is already in procurement_mto_analytic, and the latter module appears in 11.0 and is already migrated on 15.0, whereas purchase_procurement_analytic is still on 10.0.
So I think it would be good to check whether it's needed to migrate purchase_procurement_analytic at all, or maybe all of its functionality went into procurement_mto_analytic starting from 11.0 and carried on in next versions from there. If that's the case maybe the features introduced in purchase_procurement_analytic can just be moved in procurement_mto_analytic and continue with that module (maybe backport the features in 14.0 if they're needed there as well)

[rousseldenis]

Hi all, just a heads up to something we discovered with @rousseldenis in another PR, that most of the functionality in purchase_procurement_analytic is already in procurement_mto_analytic, and the latter module appears in 11.0 and is already migrated on 15.0, whereas purchase_procurement_analytic is still on 10.0. So I think it would be good to check whether it's needed to migrate purchase_procurement_analytic at all, or maybe all of its functionality went into procurement_mto_analytic starting from 11.0 and carried on in next versions from there. If that's the case maybe the features introduced in purchase_procurement_analytic can just be moved in procurement_mto_analytic and continue with that module (maybe backport the features in 14.0 if they're needed there as well)

Mmmh, in fact, MTO is a particular case of purchase.

I would say it should be better to take into account MTO in purchase module than the contrary.

So, the good option for me is to depends on this in mto module and remove all duplicated code there.

[airlessproject]

Hi all, just a heads up to something we discovered with @rousseldenis in another PR, that most of the functionality in purchase_procurement_analytic is already in procurement_mto_analytic, and the latter module appears in 11.0 and is already migrated on 15.0, whereas purchase_procurement_analytic is still on 10.0. So I think it would be good to check whether it's needed to migrate purchase_procurement_analytic at all, or maybe all of its functionality went into procurement_mto_analytic starting from 11.0 and carried on in next versions from there. If that's the case maybe the features introduced in purchase_procurement_analytic can just be moved in procurement_mto_analytic and continue with that module (maybe backport the features in 14.0 if they're needed there as well)

Mmmh, in fact, MTO is a particular case of purchase.

I would say it should be better to take into account MTO in purchase module than the contrary.

So, the good option for me is to depends on this in mto module and remove all duplicated code there.

Not diving too deeply in both codes at this moment, I think that if we remove the duplicated code, none will remain, because basically all that procurement_mto_analytic does is transfer the analytic account from the SO into the PO lines via procurement, which I believe is what exactly purchase_procurement_analytic does (and probably more). That's why I think that it's most wise to merge them in one module. Also I'd again point out the fact that purchase_procurement_analytic is currently on 10.0 (which is a long time ago), and procurement_mto_analytic is on 15.0 already.

@rousseldenis I think this may be a decision beyond both you and me, and maybe the official maintainers can check further.

[rousseldenis]

Hi all, just a heads up to something we discovered with @rousseldenis in another PR, that most of the functionality in purchase_procurement_analytic is already in procurement_mto_analytic, and the latter module appears in 11.0 and is already migrated on 15.0, whereas purchase_procurement_analytic is still on 10.0. So I think it would be good to check whether it's needed to migrate purchase_procurement_analytic at all, or maybe all of its functionality went into procurement_mto_analytic starting from 11.0 and carried on in next versions from there. If that's the case maybe the features introduced in purchase_procurement_analytic can just be moved in procurement_mto_analytic and continue with that module (maybe backport the features in 14.0 if they're needed there as well)

Mmmh, in fact, MTO is a particular case of purchase.
I would say it should be better to take into account MTO in purchase module than the contrary.
So, the good option for me is to depends on this in mto module and remove all duplicated code there.

Not diving too deeply in both codes at this moment, I think that if we remove the duplicated code, none will remain, because basically all that procurement_mto_analytic does is transfer the analytic account from the SO into the PO lines via procurement, which I believe is what exactly purchase_procurement_analytic does (and probably more). That's why I think that it's most wise to merge them in one module. Also I'd again point out the fact that purchase_procurement_analytic is currently on 10.0 (which is a long time ago), and procurement_mto_analytic is on 15.0 already.

@rousseldenis I think this may be a decision beyond both you and me, and maybe the official maintainers can check further.

@airlessproject That does not impact any functional nor technical change as if both modules are installed, behaviour would be the same.

As you can see, I'm declaring myself as this module maintainer:

So, of course we can ask review. But, duplicate code is always to avoid. My comment above is a fair solution

[airlessproject]

Hi all, just a heads up to something we discovered with @rousseldenis in another PR, that most of the functionality in purchase_procurement_analytic is already in procurement_mto_analytic, and the latter module appears in 11.0 and is already migrated on 15.0, whereas purchase_procurement_analytic is still on 10.0. So I think it would be good to check whether it's needed to migrate purchase_procurement_analytic at all, or maybe all of its functionality went into procurement_mto_analytic starting from 11.0 and carried on in next versions from there. If that's the case maybe the features introduced in purchase_procurement_analytic can just be moved in procurement_mto_analytic and continue with that module (maybe backport the features in 14.0 if they're needed there as well)

Mmmh, in fact, MTO is a particular case of purchase.
I would say it should be better to take into account MTO in purchase module than the contrary.
So, the good option for me is to depends on this in mto module and remove all duplicated code there.

Not diving too deeply in both codes at this moment, I think that if we remove the duplicated code, none will remain, because basically all that procurement_mto_analytic does is transfer the analytic account from the SO into the PO lines via procurement, which I believe is what exactly purchase_procurement_analytic does (and probably more). That's why I think that it's most wise to merge them in one module. Also I'd again point out the fact that purchase_procurement_analytic is currently on 10.0 (which is a long time ago), and procurement_mto_analytic is on 15.0 already.
@rousseldenis I think this may be a decision beyond both you and me, and maybe the official maintainers can check further.

@airlessproject That does not impact any functional nor technical change as if both modules are installed, behaviour would be the same.

As you can see, I'm declaring myself as this module maintainer:

So, of course we can ask review. But, duplicate code is always to avoid. My comment above is a fair solution

Oh, apologies for this, but for sure having more opinions is better :)

I think it's best to cross-check both modules and as I said previously, I fear that if we remove duplicated code from procurement_mto_analytic, probably there will be nothing left (it's a very small module with a few models with one method each).

[airlessproject]

Hi all, just a heads up to something we discovered with @rousseldenis in another PR, that most of the functionality in purchase_procurement_analytic is already in procurement_mto_analytic, and the latter module appears in 11.0 and is already migrated on 15.0, whereas purchase_procurement_analytic is still on 10.0. So I think it would be good to check whether it's needed to migrate purchase_procurement_analytic at all, or maybe all of its functionality went into procurement_mto_analytic starting from 11.0 and carried on in next versions from there. If that's the case maybe the features introduced in purchase_procurement_analytic can just be moved in procurement_mto_analytic and continue with that module (maybe backport the features in 14.0 if they're needed there as well)

Mmmh, in fact, MTO is a particular case of purchase.
I would say it should be better to take into account MTO in purchase module than the contrary.
So, the good option for me is to depends on this in mto module and remove all duplicated code there.

Not diving too deeply in both codes at this moment, I think that if we remove the duplicated code, none will remain, because basically all that procurement_mto_analytic does is transfer the analytic account from the SO into the PO lines via procurement, which I believe is what exactly purchase_procurement_analytic does (and probably more). That's why I think that it's most wise to merge them in one module. Also I'd again point out the fact that purchase_procurement_analytic is currently on 10.0 (which is a long time ago), and procurement_mto_analytic is on 15.0 already.
@rousseldenis I think this may be a decision beyond both you and me, and maybe the official maintainers can check further.

@airlessproject That does not impact any functional nor technical change as if both modules are installed, behaviour would be the same.
As you can see, I'm declaring myself as this module maintainer:

So, of course we can ask review. But, duplicate code is always to avoid. My comment above is a fair solution

Oh, apologies for this, but for sure having more opinions is better :)

I think it's best to cross-check both modules and as I said previously, I fear that if we remove duplicated code from procurement_mto_analytic, probably there will be nothing left (it's a very small module with a few models with one method each).

@rousseldenis sorry for the extra comments, I just now compared the two modules and see there is extra functionality in procurement_mto_analytic, for sale order lines (and stock move) in particular. I wrongly thought that was also included in purchase_procurement_analytic.

I think your approach to remove the duplicate code from there and make it depend on purchase_procurement_analytic is the way to go. But for that to happen, I think first it will be needed for this PR to be merged, then also migrated to 15.0. And also the removing of duplicate code in procurement_mto_analytic should be done both in 14.0 and 15.0, for consistency.

Thanks and apologies again for wasting a bit of our time :)

[rousseldenis]

Thanks and apologies again for wasting a bit of our time :)

👍 Debate is never time wasting 😃

[AaronHForgeFlow]

Looking at the commit history, procurement_mto_analytic was created from purchase_procurement_analytic, it was actually a module rename. I think now that renaming was not necessary at that time, but I don't like the idea of renaming it again.

For convention, procurement_mto_analytic should stay, and if there are improvements here that are not in the procurement_mto_analytic module then we can do a PR to introduce those improvements/fixes.

I also see that procurement_mto_analytic has no declared maintainer so @rousseldenis could also propose himself to be a maintainer for that module.

[rousseldenis]

For convention, procurement_mto_analytic should stay, and if there are improvements here that are not in the procurement_mto_analytic module then we can do a PR to introduce those improvements/fixes.

That's why I wanted to depends on procurement_purchase_analytic for duplicated code and keep both (we can do that in a further PR) as this will not break anything IMHO.

I also see that procurement_mto_analytic has no declared maintainer so @rousseldenis could also propose himself to be a maintainer for that module.

I can do it also even if I don't use it yet

[AaronHForgeFlow]

That's why I wanted to depends on procurement_purchase_analytic for duplicated code and keep both (we can do that in a further PR) as this will not break anything IMHO.

I am fine with that :)

[rousseldenis]

@AaronHForgeFlow @Cedric-Pigeon @airlessproject

Do we agree to remove duplicate code from procurement_mto_analytic (purchase.order.line and stock.rule) and add a depends there on this module ?

[AaronHForgeFlow]

@AaronHForgeFlow @Cedric-Pigeon @airlessproject

Do we agree to remove duplicate code from procurement_mto_analytic (purchase.order.line and stock.rule) and add a depends there on this module ?

Ok for me!

[github-actions]

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.