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][IMP] rma : propagate cancelation #539

Conversation

florian-dacosta
Copy link
Contributor

Replace #429

With current code, in the case you create a RMA group with 2 rma lines, if you cancel one rma line, the whole picking related to the rma line is canceled, so the second rma line can't be done.

The goal of this PR is to avoid this, and also to manage the propagation of the cancelation on all origin stock moves related to the rma line.
For destination moves, it should be configurable on the stock rules.

Like commented in #429
I don't understand the use case covered with the current code. A example on how to reproduce a process where it is usefull would certainly help me!

I'll set the PR in draft for now, since the goal for now is more to discuss on how can we avoid the problematic of the rma group use while continuing covering the current use case (which I don't get)

@AaronHForgeFlow @DavidFIB

  • If you may try carifying the current use case covered with an example, I would appreciate it.
  • If the problematic about the RMA group I explain is not clear for you, I can detail the steps to reproduce the issue.

@DavidJForgeFlow
Copy link
Contributor

Hi @florian-dacosta,
IMO this seems to be a good option, as you say this should fix the behavior of the RMA Groups while maintining the base functionality for the RMA Lines. I would appreciate some tests to make sure the features works as expected, as it wasn't before tested. Thank you.

Also the user @DavidFIB is an outdated profile, so you can tag @DavidJForgeFlow instead!

@florian-dacosta
Copy link
Contributor Author

Thanks for the feedback.
Well, like I said, I did not observe (while testing) the cancelation in chain in case of multi step picking, with the current code.
So I am not sure I "maintain" the base functionality... But the cancelation in chain in case of multi step picking seems to be what is expected so I hope it is fine.

I will add some tests about it, and once ok, will remove the WIP mark of the PR.

@florian-dacosta florian-dacosta force-pushed the 16-propagate-cancel-mutliple-step-rma-line branch 2 times, most recently from 1c03e23 to 6ae7556 Compare September 26, 2024 07:24
@florian-dacosta florian-dacosta marked this pull request as ready for review September 26, 2024 07:24
@florian-dacosta florian-dacosta force-pushed the 16-propagate-cancel-mutliple-step-rma-line branch from 6ae7556 to f92559e Compare September 26, 2024 07:48
When canceling a rma order line, it will also cancel the previous (orig) steps.
Following steps (dest) can be managed with the propagate cancel option of the stock rules.
This commit also avoid canceling a whole picking which is problematic in case of the use of RMA groups
@florian-dacosta florian-dacosta force-pushed the 16-propagate-cancel-mutliple-step-rma-line branch from f92559e to f05e403 Compare September 26, 2024 09:30
@florian-dacosta
Copy link
Contributor Author

@DavidJForgeFlow @AaronHForgeFlow
Here, I have added a test.
My main goal here is to avoid canceling the whole picking when a single rma line is canceled, in the case of rma.order with multiple lines.

If you play the new test without the changes on action_rma_cancel (on present v16), you will see it will fail on a test where the picking is canceled, where it should not. This is what I want to prevent with this PR.

Let me know what you think.

Copy link
Contributor

@DavidJForgeFlow DavidJForgeFlow 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

@AaronHForgeFlow AaronHForgeFlow left a comment

Choose a reason for hiding this comment

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

I think this makes sense. Cancelling only the stock moves related to the rma line should be enough. I will do a last check before the merge and hopefully merge it.

@florian-dacosta
Copy link
Contributor Author

Hello @AaronHForgeFlow
You last check was not conclusive ?

@AaronHForgeFlow
Copy link
Contributor

Hello @AaronHForgeFlow You last check was not conclusive ?

Hi @florian-dacosta sorry for the wait. I think this is ok to merge. However, we have a complex project that may be impacted by this change, and the person handling it is currently on vacation. Hopefully next week we can merge it.

Copy link
Contributor

@LoisRForgeFlow LoisRForgeFlow left a comment

Choose a reason for hiding this comment

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

Code review, from my side it makes sense 👍

@AaronHForgeFlow
Copy link
Contributor

@florian-dacosta Thank you for the fix and sorry for the wait, I am merging this now. I did additional functional tests. All good.

@AaronHForgeFlow AaronHForgeFlow merged commit e60e71e into ForgeFlow:16.0 Oct 22, 2024
4 checks passed
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