search

ForgeFlow / stock-rma

Stock RMA
GNU Affero General Public License v3.0
38 stars 41 forks source link

[16][rma_sale][rma_sale_mrp] Create rma line from stock move line instead of sale order line by default and when possible + manage kit components #525

Closed florian-dacosta closed 2 months ago

florian-dacosta commented 4 months ago

I did refactore the way we add rma lines from a sale order. Today, sometimes quantities are taken from the stock move lines (when tracking management is active for the product) and sometimes quantity is taken from the sale order line (if tracking is not active) I think it is confusing for the user to have this difference of behavior (which he can't be aware) and, based on the sale order line, we can't manage the case where we want to add the components of a kit.

This PR do the following : 1) Always take the information from the stock move line when possible : the only case where we will take information from the sale order line is when we want to add a kit product. 2) Add an option in rma_sale_mrp, at company level, to choose if we add rma line for the kit product or for the components. (by default, we take the kit, so the present behavior is not changed) 3) Fix some bugs : a) Today, if you have a sale order line with 2 products, but you ship only one and have a backorder (or even cancel the other shipment), the rma line created from the sale order will have a quantity of 2 instead of 1. b) bug when you use lot and return. Order 1 product with tracking management. Ship it. Then return it, then re-ship it. When you create the rma line from the sale order, the quantity will be 3, instead of 1.

About rma_sale_mrp, I see it is not set as auto-install. Any reason why ?

@AaronHForgeFlow I did close the previous PR and opened this one because I totally refactored it and added the option on rma_sale_mrp. It seemed cleaner to do a new PR rather than use the other already approved one.

If you could review this one, it would be great !

AaronHForgeFlow commented 4 months ago

Functional tests are good on my side. @ChrisOForgeFlow what do you think about this PR?

@florian-dacosta I think it is ok rma_sale_mrp, if you have installed sale_mrp you are supposed to use kits and compute the cost prices based on the cost of the delivered products. You added a flag in the company so users can decide if using the split by components or not to use it, so I think it is ok to keep it.

florian-dacosta commented 2 months ago

@AaronHForgeFlow Do you think we can get it merge? Or maybe get another review from forgeflow's team? When merged I'll gadly port it to v17. It would be nice to have this merged before the 18

AaronHForgeFlow commented 2 months ago

Hi @florian-dacosta for me this is fine. The only thing is that we we could split what it is an improvement from what it is a fix it would be great. The method _get_lot_quantity_from_move_lines is for taking the lot for the kit right? It is part of the new feature and not for the fix, right?

florian-dacosta commented 2 months ago

@AaronHForgeFlow It would really not be easy to split I think No _get_lot_quantity_from_move_lines is there to compute the quantity we want for the rma line, in the case Odoo looks at the stock.move.line. This is actually needed for the fix of the 1st bug. But this is also needed in order to be able to add the components of a kit. So it is needed for both

AaronHForgeFlow commented 2 months ago

Ok, I will do a final check with my colleagues and hopefully merge it.

AaronHForgeFlow commented 2 months ago

Thank you @florian-dacosta , I will keep it mind this MR in this migration https://github.com/ForgeFlow/stock-rma/pull/516, if you create a PR to the branch of that PR I would appreciate it.

florian-dacosta commented 2 months ago

Thanks for the help @AaronHForgeFlow I'll do the PR in 2 weeks for version 17 !