Skip to content

Conversation

@ivantodorovich
Copy link
Contributor

Depends on #162

Among other things, it removes the workflow engine from this module, making it easy to further migrate it to 11.0, as stated on this comment: #160 (review) by @dreispt

See commit log for details

@max3903 max3903 self-requested a review April 24, 2018 18:35
@max3903 max3903 added this to the 9.0 milestone Apr 24, 2018
Copy link
Member

@max3903 max3903 left a comment

Choose a reason for hiding this comment

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

Same as #162, bump the version

@ivantodorovich
Copy link
Contributor Author

Done!

@ivantodorovich
Copy link
Contributor Author

Pushed some new changes, including a FIX of a critical bug that was undetected before.
I will not change anything else on this PR from now on, other than rebasing when needed.

am_i_approver = fields.Boolean(
related='page_id.am_i_approver'
related='page_id.am_i_approver',
related_sudo=False,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Took me a while to figure it out. Turns out related fields are computed as sudo() by default 👎

if approver_group_id not in user.groups_id:
# if user belongs to 'Knowledge / Manager', he can approve anything
if user.has_group('document_page.group_document_manager'):
return True
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually an improvement, not a FIX.
Also rewrited the code to use has_group instead of manually checking for groups_id

<field name="perm_unlink" eval="True"/>
<field name="perm_create" eval="True"/>
</record>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now, only Approvers can see other people pending Change Requests.

@ivantodorovich
Copy link
Contributor Author

Hi @max3903

I don't think this module receives much attention.
Can we merge anyway? This PR includes a seccurity fix that I think it's important.

(related to #162 and will need rebase)

@max3903
Copy link
Member

max3903 commented Jun 1, 2018

@ivantodorovich Agree. I merged #162. Can you rebase?

@ivantodorovich
Copy link
Contributor Author

Done!

@max3903 max3903 merged commit 20a4c0a into OCA:9.0 Jun 1, 2018
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.

2 participants