Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[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

Conversation

florian-dacosta
Copy link
Contributor

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
Copy link
Contributor

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.

…ve lines by default

This allow to manage phantom bom products, by creating rma lines for the components instead of the kit if the option is activated.
@florian-dacosta florian-dacosta force-pushed the 16-rma-phantom-bom-components-refactore branch from 2dce769 to 820af8c Compare July 26, 2024 08:12
@florian-dacosta
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor

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

Copy link
Contributor

@ChrisOForgeFlow ChrisOForgeFlow left a comment

Choose a reason for hiding this comment

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

LGTM, only one comment, thanks

rma_sale/wizards/rma_add_sale.py Outdated Show resolved Hide resolved
Copy link

@cormaza cormaza left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ChrisOForgeFlow ChrisOForgeFlow left a comment

Choose a reason for hiding this comment

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

LGTM

@AaronHForgeFlow AaronHForgeFlow merged commit f83a0de into ForgeFlow:16.0 Aug 1, 2024
4 checks passed
@AaronHForgeFlow
Copy link
Contributor

Thank you @florian-dacosta , I will keep it mind this MR in this migration #516, if you create a PR to the branch of that PR I would appreciate it.

@florian-dacosta
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants