-
Notifications
You must be signed in to change notification settings - Fork 12
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
[IMP] statechart: improve _compute_sc_has_allowed_events #40
base: 14.0
Are you sure you want to change the base?
Conversation
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.
It's a somewhat tricky solution. I didn't like the fact that it depends on the context.
As I understand it, we are trying to exclude certain events from the user notification/dashboard. I prefer a more explicit field than the priority. For example: "exclude_from_allowed_event." takes true or false.
@api.depends("sc_state") | ||
def _compute_sc_has_allowed_events(self): | ||
priorities = self.env.context.get("priorities") |
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.
priorities = self.env.context.get("priorities") | |
priorities = self.env.context.get("priorities", []) |
1c52070
to
e37e365
Compare
@@ -316,7 +316,8 @@ class can override the statechart with another one adding new events, | |||
|
|||
@api.model | |||
def _get_sc_event_allowed_field_names(self): | |||
event_names = self._statechart.events_for() | |||
ignore_for_has_allowed_events = self.env.context.get("ignore_for_has_allowed_events", []) | |||
event_names = [name for name in self._statechart.events_for() if name not in ignore_for_has_allowed_events] |
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.
nit: you can put the if name not in ignore_for_has_allowed_events
in the return
statement, to avoid creating a temporary list.
return [ | ||
_sc_make_event_allowed_field_name(event_name) for event_name in event_names | ||
] |
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.
return [ | |
_sc_make_event_allowed_field_name(event_name) for event_name in event_names | |
] | |
return [ | |
_sc_make_event_allowed_field_name(event_name) for event_name in event_names if event_name not in ignore_for_has_allowed_event | |
] |
e37e365
to
ee7254d
Compare
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
This needs a test. It should be easy to add in |
ee7254d
to
c21bb32
Compare
Sorry for the delay, I'm just now seeing your review. I've added the test as you asked. |
… has_allowed_event compute
c21bb32
to
15d918b
Compare
This PR has the |
1 similar comment
This PR has the |
This commits makes it possible to ignore some events when computing has_allowed_events by passing them in the context key
ignore_for_has_allowed_events
.For example, our record has a cancel action on each state but we don't want to take that action into account when getting the records with allowed_events
Our use case is showing all the records that need an action in a dashboard. But the records with a cancel action are not actually in need of an action.