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

[MIG] stock_exception: migration to 16.0 #1965

Merged
merged 10 commits into from
May 22, 2024

Conversation

mav-adhoc
Copy link
Contributor

No description provided.

@yostashiro
Copy link
Member

@mav-adhoc There is #1957 already. Would you review this instead?

@mav-adhoc mav-adhoc force-pushed the 16.0-mig-stock_exception branch 7 times, most recently from 8f7bf08 to f08171e Compare March 27, 2024 16:44
@mav-adhoc mav-adhoc force-pushed the 16.0-mig-stock_exception branch 7 times, most recently from df383ca to 60d83c1 Compare April 8, 2024 15:12
@mav-adhoc
Copy link
Contributor Author

@yostashiro Hi! This PR, apart from migrating the module to the 16 version, includes new features.

When we were testing the model, in case we choose 'stock.picking' as the model where we wanted to apply the exception, we noticed that the exception also ran when we confirmed sale orders.
This happens because, by confirming sale orders, the action confirm creates the respective records in stock.picking (IN/OUT, depending on the configuration you have) and run the exception.
What we wanted is to have the decision when to run the exception, if at the creation of the stock.picking record or if at the validation of the stock.picking record.

Due to this desire, we created two new boolean fields in the stock.exception class (check_on_validate and check_on_confirm) in order to indicate when to run the exception. If you select check_on_confirm, it will run the exception when you create the stock.picking record. If you select check_on_validate, it will run the exception when you validate the stock.picking record.

@yostashiro
Copy link
Member

@mav-adhoc Thanks for explaining. We currently don't use this module so we may not be doing a thorough review, but I'd suggest splitting the new feature into a separate commit for ease of review and backporting.

@rousseldenis
Copy link
Contributor

/ocabot migration stock_exception

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Apr 11, 2024
@OCA-git-bot
Copy link
Contributor

The migration issue (#1494) has not been updated to reference the current pull request because a previous pull request (#1957) is not closed.
Perhaps you should check that there is no duplicate work.
CC @Saran440

@mav-adhoc mav-adhoc force-pushed the 16.0-mig-stock_exception branch 2 times, most recently from d75cec8 to f32933a Compare May 3, 2024 19:43
@mav-adhoc
Copy link
Contributor Author

@rousseldenis we should close this PR or the previous one?

@mav-adhoc mav-adhoc force-pushed the 16.0-mig-stock_exception branch 2 times, most recently from 31d68db to 111b26e Compare May 6, 2024 14:24
@mav-adhoc
Copy link
Contributor Author

@yostashiro Hi! Can we merge this PR? Or we should merge the previous pull request (#1957) ?

@rousseldenis
Copy link
Contributor

@yostashiro Hi! Can we merge this PR? Or we should merge the previous pull request (#1957) ?

@mav-adhoc The better is to ask @Saran440 what to do. As it was there before this, it is also fair to review that one instead.

@Saran440
Copy link
Member

Saran440 commented May 7, 2024

I checked this PR and #1957
it difference a little bit 8d6c626#diff-6cd14808c7b5610b6aedda5ec36d4cc5e66fd7839054d24788f2461897619687R41

However, I okay to use this PR.
Closed #1957

Copy link
Member

@Saran440 Saran440 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 👍

@rousseldenis
Copy link
Contributor

/ocabot migration stock_exception

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@mav-adhoc
Copy link
Contributor Author

@OCA/tools-maintainers Merge please

1 similar comment
@jcadhoc
Copy link
Contributor

jcadhoc commented May 22, 2024

@OCA/tools-maintainers Merge please

@thomaspaulb
Copy link

This does not belong to tools. @OCA/stock... does not yield me any autofill, @pedrobaeza do you know what the correct ping is?

@rousseldenis
Copy link
Contributor

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-1965-by-rousseldenis-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 00aa189 into OCA:16.0 May 22, 2024
5 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at acfb321. 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.

10 participants