[12.0][IMP] hr_holidays_leave_auto_approve: Auto-approve for everyone #663
Conversation
jarroyomorales
force-pushed
the
12.0-approve-all
branch
2 times, most recently
from
8c4d92a
to
edc3464
Compare
|
This PR has the |
|
@CasVissers I'm sure you would be interested in this PR! |
|
@OCA/human-resources-maintainers Can this be merged? |
|
One more review needed |
|
@alexey-pelykh Isnt 2 the minimum? |
|
2 + 1 PCS |
|
Oh okay! |
|
/ocabot merge |
|
This PR looks fantastic, let's merge it! |
|
I'd also propose for next migration or improvement make a setting field of "auto-approve policy" instead of 2 Boolean fields |
|
@gurneyalex your merge command was aborted due to failed check(s), which you can inspect on this commit of 12.0-ocabot-merge-pr-663-by-gurneyalex-bump-no. After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red. |
@alexey-pelykh It is a good idea, but then we wont be able to autoapprove some leaves and not autoapprove other types. |
|
@jarroyomorales Setting per leave-type of course |
|
@alexey-pelykh Oh sorry, I thought you meant adding a field |
jarroyomorales
force-pushed
the
12.0-approve-all
branch
from
64d96ca
to
1cfc21f
Compare
|
Do we want to introduce this approach in this PR? |
|
OK, I can try, a migration script will be needed too, am i right? |
|
Yes, and I can help with that if needed |
|
@alexey-pelykh Can you review please? If it is okay i will go for the migration script |
|
And please squash to 1 commit |
jarroyomorales
force-pushed
the
12.0-approve-all
branch
from
03eeded
to
7025fbd
Compare
|
@alexey-pelykh Comments attended |
| values.get('holiday_status_id') | ||
| ).auto_approve | ||
| ) | ||
| auto_approve = leave_type.auto_approve_policy != 'no' |
There was a problem hiding this comment.
Extract as def _should_auto_approve method
There was a problem hiding this comment.
Don't you think we can just delete the _get_auto_approve_on_creation() function? It is used to disable the creation message in case the leave can be auto approved, which I dont undersand why. IMHO it is not necessary to do that
There was a problem hiding this comment.
If we're self-approving, we don't want message about user approved own request being sent to himself, BUT there can be other watchers by default. Probably if "only user will receive message" then don't send it, or exclude self from followers, something like that. We can address that as separate PR
| @@ -12,6 +12,13 @@ def _check_approval_update(self, state): | |||
| return | |||
| return super()._check_approval_update(state) | |||
|
|
|||
| @api.multi | |||
| def apply_auto_approve_policy(self): | |||
There was a problem hiding this comment.
Use _ to make this method non-RPC-callable
| def apply_auto_approve_policy(self): | ||
| for record in self: | ||
| policy = record.holiday_status_id.auto_approve_policy | ||
| if (record.can_approve and policy == 'hr') or policy == 'all': |
There was a problem hiding this comment.
Reuse _should_auto_approve ?
if record._should_auto_approve():
record.sudo().action_approve()
There was a problem hiding this comment.
The whole function could be done in one line like this
self.filtered(lambda r: r._should_auto_approve()).sudo().action_approve()but will we have possible future inheritance errors?
There was a problem hiding this comment.
no, it would work normally
|
Direction is great, let's proceed to migration script as well 👍 |
jarroyomorales
force-pushed
the
12.0-approve-all
branch
from
7025fbd
to
11bb40a
Compare
|
@alexey-pelykh Migration done and works as expected. I am keeping the |
hr_holidays_leave_auto_approve/migrations/12.0.2.0.0/post-migration.py
Outdated
Show resolved
Hide resolved
| values.get('holiday_status_id') | ||
| ).auto_approve | ||
| ) | ||
| auto_approve = leave_type.auto_approve_policy != 'no' |
There was a problem hiding this comment.
If we're self-approving, we don't want message about user approved own request being sent to himself, BUT there can be other watchers by default. Probably if "only user will receive message" then don't send it, or exclude self from followers, something like that. We can address that as separate PR
jarroyomorales
force-pushed
the
12.0-approve-all
branch
from
11bb40a
to
44f2984
Compare
|
/ocabot merge |
|
Hey, thanks for contributing! Proceeding to merge this for you. |
|
Congratulations, your PR was merged at 5fd1d77. Thanks a lot for contributing to OCA. ❤️ |
PR related to the discussion in #610 .
Added a new boolean field for leave types
auto_approve_allwhich makes all leave requests of that type be automatically approved by the odoo bot.cc @alexey-pelykh @vmelnych @neilmitchell-goodson