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.0][MIG] stock_picking_operation_quick_change #1232

Merged

Conversation

florian-dacosta
Copy link
Contributor

Standard Migration.
I am wondering if we could change some things though.

  1. Do we really need the button for outgoing pickings ? It seems to me it is more usefull to change dest location on reception, or even internal, but not on delivery ?
    I would hide it on outgoing pickings

  2. The button is available for all stock users... But it changes the location, to be consistent, it should be available for the group Manage Multiple Stock Locations only IMO

  3. The button is only in the operation tab. When picking is configured to show the detailed operations, I think the button should be there, and then hiden from the operation tab ?

I can change the module accordingly, what do you think ?
@duyanhk15 @leemannd @yankinmax @cyrilmanuel @pedrobaeza @sergio-teruel @LoisRForgeFlow

@florian-dacosta florian-dacosta force-pushed the 16.0-mig-stock_picking_operation_quick_change branch from a73cf49 to cb3d81d Compare February 23, 2023 11:56
@LoisRForgeFlow
Copy link
Contributor

LoisRForgeFlow commented Feb 27, 2023

@florian-dacosta My opinion for each of the points:

  1. I agree with you, but since the README explicitly describes that use case, I would check with the original authors because maybe they had a case that really needed the feature in deliveries. It is an old module, maybe it did make sense at the time but it doesn't anymore.
  2. Yes, hiding the button when multilocations are not activated seems a good idea.
  3. I'm more hesitant with this one. The detailed operations (stock.move.lines) may have more specific locations applied (due to putaway strategies) and this modules changes de locations of the stock.moves. Users may think that the feature is meant to adjust the bin destination location, but I think if the wizard affects stock.moves is more suitable to change de "general" demand location on the stock move. Just something to have in mind if you decide to change it.

@florian-dacosta
Copy link
Contributor Author

3. I'm more hesitant with this one. The detailed operations (stock.move.lines) may have more specific locations applied (due to putaway strategies) and this modules changes de locations of the stock.moves. Users may think that the feature is meant to adjust the bin destination location, but I think if the wizard affects stock.moves is more suitable to change de "general" demand location on the stock move. Just something to have in mind if you decide to change it.

No, AFAIK, it does change the location on stock.move.line and not stock.move.

@LoisRForgeFlow
Copy link
Contributor

  1. I'm more hesitant with this one. The detailed operations (stock.move.lines) may have more specific locations applied (due to putaway strategies) and this modules changes de locations of the stock.moves. Users may think that the feature is meant to adjust the bin destination location, but I think if the wizard affects stock.moves is more suitable to change de "general" demand location on the stock move. Just something to have in mind if you decide to change it.

No, AFAIK, it does change the location on stock.move.line and not stock.move.

You are right, my bad. Then your suggestion makes total sense.

Thanks!

@github-actions
Copy link

github-actions bot commented Jul 2, 2023

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.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jul 2, 2023
@rousseldenis
Copy link
Contributor

/ocabot migration stock_picking_operation_quick_change

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Jul 3, 2023
@OCA-git-bot OCA-git-bot mentioned this pull request Jun 23, 2023
70 tasks
@github-actions github-actions bot removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jul 9, 2023
Copy link

github-actions bot commented Mar 3, 2024

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.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Mar 3, 2024
@florian-dacosta
Copy link
Contributor Author

I did the changes 2 and 3 of the PR description and not the one since I am not sure it will be ok for every one.
This PR is ready

@florian-dacosta florian-dacosta force-pushed the 16.0-mig-stock_picking_operation_quick_change branch from fb4389b to 34abf72 Compare April 2, 2024 12:41
@florian-dacosta
Copy link
Contributor Author

Test failure seems unrelated and is already failing in main 16.0 branch

@rousseldenis
Copy link
Contributor

Test failure seems unrelated and is already failing in main 16.0 branch

@florian-dacosta Yes, it seems this is caused by: odoo/odoo@ca9aab9

I need to take care of it. 😶‍🌫️

@github-actions github-actions bot removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Apr 7, 2024
@florian-dacosta florian-dacosta force-pushed the 16.0-mig-stock_picking_operation_quick_change branch from 34abf72 to 1cf38fe Compare April 12, 2024 15:31
@florian-dacosta florian-dacosta force-pushed the 16.0-mig-stock_picking_operation_quick_change branch from 1cf38fe to a6ab78c Compare October 30, 2024 15:22
@florian-dacosta
Copy link
Contributor Author

I havejust included last fix on v15 from #1331

@sergiocorato @sergio-teruel @CarlosRoca13 @Ricardoalso @gbrito @FrancescoBallerini

Any chance to have a review to get this finally merged ?
Thanks

@rousseldenis
Copy link
Contributor

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-1232-by-rousseldenis-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 8661d80 into OCA:16.0 Oct 30, 2024
10 of 12 checks passed
@OCA-git-bot
Copy link
Contributor

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

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

Successfully merging this pull request may close these issues.