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] sale_exception: Migration to 16.0 #2344

Merged
merged 55 commits into from
Feb 9, 2023

Conversation

sbejaoui
Copy link
Contributor

finalize #2266

atchuthan and others added 30 commits November 16, 2022 16:34
Rule should only count available stock from selected warehouse from sale order.
Without warehouse in context, it currently sums all stock available in all warehouses.
The previous parent menu has an action attached, and with these modules,
it can't be clicked.
* [FIX] sale_exception confirm wizard
* [FIX] add test of test_all_draft_orders & flake8 & readme.rst
* [FIX] improve codecov test
* [FIX] delete odl translation
* [FIX] add Camptocamp to author
* [FIX] readme.rst
Raise SingletonError when you have a recordset with multiple sale order.
This restore the capability to write on multiple sale.order
Currently, it's located at the root menu because. It's a migration bug because the old menu was removed from 11.0.
The method called by 'sale_check_exception' has a side effect, it writes
on 'exception.rule' + on the Many2many relation between it and
sale.order(.line). When decorated by @api.constrains, any error during
the method will be caught and re-raised as "ValidationError".
This part of code is very prone to concurrent updates as 2 sales having
the same exception will both write on the same 'exception.rule'.
A concurrent update (OperationalError) is re-raised as ValidationError,
and then is not retried properly.

Calling the same method in create/write has the same effect than
@api.constrains without shadowing the exception type.

Full explanation:
OCA/server-tools#1642
* [IMP] sale_exception: show menu only to Exception Rule Managers
* [IMP] Reindent lines to 4 spaces
* [IMP] Improve information messages that display exceptions
* [REM] Remove exception_ids fields from the sale order view, because it's no longer necessary and it allowed users to accidentaly deactivate a Sale Exception Rule using the active widget.
* [IMP] Rename exception_ids_formatted to exceptions_summary
Disable for now as the solution is not obvious.

Partially fixes OCA#972
Other ways it provokes linter fail
TestSaleOrder was useless and making sale order tests run twice.
@sbejaoui
Copy link
Contributor Author

Copy link
Contributor

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

LGTM (Code review)

Copy link
Contributor

@rousseldenis rousseldenis left a comment

Choose a reason for hiding this comment

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

/ocabot migration sale_exception

LGTM. Code review

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Jan 25, 2023
@OCA-git-bot OCA-git-bot mentioned this pull request Jan 25, 2023
99 tasks
@OCA-git-bot
Copy link
Contributor

The migration issue (#2215) has been updated to reference the current pull request.
however, a previous pull request was referenced : #2266.
Perhaps you should check that there is no duplicate work.
CC : @matiasperalta1

@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). 🤖

@rousseldenis
Copy link
Contributor

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

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

@OCA-git-bot OCA-git-bot merged commit 35ed9a4 into OCA:16.0 Feb 9, 2023
@OCA-git-bot
Copy link
Contributor

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

@CasVissers-360ERP
Copy link

@sbejaoui I think something went wrong in the migration here? Decoration danger is always active?
https://watch.screencastify.com/v/1DLtEQbp4FOR1uBwhYMz

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.