-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[18.0][IMP] base_exception: Allow filtering records before exception evalua… #3405
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
base: 18.0
Are you sure you want to change the base?
[18.0][IMP] base_exception: Allow filtering records before exception evalua… #3405
Conversation
|
Hi @hparfr, @sebastienbeau, |
simahawk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Minor remark only.
| filter_domain = fields.Char( | ||
| string="Apply on records", | ||
| default="[]", | ||
| help="If present, this condition must be satisfied to trigger the exception.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| help="If present, this condition must be satisfied to trigger the exception.", | |
| help="If present, this condition must be satisfied to trigger the exception, no matter the Exception type.", |
hparfr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
Can you explain why / how to use in the usage.md.
I guess it's a kind of a pre-filter to skip the test of exception on some records. Is it ?
|
This PR has the |
|
@hparfr Yes it's a pre filter to segregate records from a same model where an exception could apply or not. However, there are no configuration instructions or USAGE in the readme of this module, so I'm not sure it would make much sense to describe this filter when there isn't any other instructions on how to configure exceptions... 🤷 |
It's never too late add some doc. At least a small word the source code can be good enough. The reason you want to add this filtering feature was not obvious to me because I don't see the benefit in my case (mainly detect by domain exceptions): the filtering is already done with the domain. If I understand correctly, the prefilter has positive impact on perfs for detecting exception by python code and negative for detecting exceptions by domain. |
…tion