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

Show inherited acls #761

Closed
wants to merge 3 commits into from
Closed

Conversation

nenadalm
Copy link
Contributor

@nenadalm nenadalm commented Feb 12, 2020

This pr adds inherited advanced rules into the sidebar together with rules for current file. This allows seeing all advanced rules for the file with possibility of changing any rule (rule for current file will be changed in case inherited rule is shown).

When rule is removed in the ui, it is removed from the list and if it was inherited, it will be shown there again after reopening (maybe this should be changed somehow?).

Signed-off-by: Miloslav Nenadal <nenadalm@gmail.com>
@janpekar
Copy link

I recommend do not allow to delete permission, that is inherited.

- inherited acls cannot be removed
- if non inherited acl is removed, inherited acl takes it's place if any


Signed-off-by: Miloslav Nenadal <nenadalm@gmail.com>
@nenadalm
Copy link
Contributor Author

I recommend do not allow to delete permission, that is inherited.

I removed the confusing behaviour and now inherited rules cannot be removed. In case they're changed and changed rule is removed, inherited rule takes it's place in the ui.

@juliusknorr juliusknorr added 3. to review Items that need to be reviewed enhancement labels Feb 18, 2020
@juliusknorr
Copy link
Member

Thanks a lot for your pull request, looks good from a first quick view, but I'll make sure to give it a proper review and test later this week.

@juliusknorr
Copy link
Member

This is a really nice enhancement. Now from a UX perspective I think it would make sense to still somehow indicate that the rule is shown because it is inherited from a parent node. cc @jancborchardt

image

The group2 entry is fully inherited from the parent (same as user1, but user1 was added as an individual rule to the "Child" directory). I would propose to make sure that the permission icons also get the same opacity level in that case.

Signed-off-by: Miloslav Nenadal <nenadalm@gmail.com>
@nenadalm nenadalm force-pushed the show_inherited_acls branch from 7590814 to 1d0783c Compare March 11, 2020 11:10
@nenadalm
Copy link
Contributor Author

opacity fixed

@jancborchardt
Copy link
Member

I would propose to make sure that the permission icons also get the same opacity level in that case.

Yep, good proposal. @nenadalm thanks for the contribution and the adjustment! :)

@wiswedel
Copy link
Contributor

so can this PR be approved and merged then, @juliushaertl?

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Works like a charm and code looks good.

@juliusknorr
Copy link
Member

Let me rebase so we can get this in

@juliusknorr
Copy link
Member

I've pushed a rebased version of this in #999 since i didn't have push access to your repo. Let me close this PR in favor of the new one then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Items that need to be reviewed enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants