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

Disable editing controls for decorator if user lacks permissions. #2736

Merged
merged 3 commits into from Aug 25, 2016

Conversation

@dennisoelkers
Copy link
Member

@dennisoelkers dennisoelkers commented Aug 24, 2016

Description

This change does the following when a user lacks the
'decorators:edit:${streamId}' permission:

  • disables items in action menu, when user lacks
  • disables controls to create decorator
  • disables ability to reorder decorators by drag & drop

Fixes #2730.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
@dennisoelkers dennisoelkers added this to the 2.1.0 milestone Aug 24, 2016
@bernd bernd self-assigned this Aug 24, 2016
<MenuItem divider/>
<MenuItem onSelect={this._handleDeleteClick}>Delete</MenuItem>
</DropdownButton>
{decoratorActionsMenu}

This comment has been minimized.

@edmundoa

edmundoa Aug 24, 2016
Member

For better consistency with other parts of the UI, I would hide the whole drop-down menu, as both actions depend on the same permission to be set.

This comment has been minimized.

@dennisoelkers

dennisoelkers Aug 25, 2016
Author Member

My general rule of thumb for hiding functionality vs. disabling it is that:

  • hiding functionality which the user will never be able to use (i.e. requiring admin privileges)
  • disabling functionality which is not available in this context, but might be at some other point

This way it is transparent for the user (although in this case we could add some helpful tooltips) that the functionality exists, but is not usable (yet) and the layout stays the same.

We do not do that consistently, but that's not because the way we do it now is the right one in most places, but because we do not have a convention for this yet. :)

This comment has been minimized.

@edmundoa

edmundoa Aug 25, 2016
Member

As far as I remember, we hide most buttons users have no permissions to use, but there's no convention, that is very true.

This comment has been minimized.

@edmundoa

edmundoa Aug 25, 2016
Member

By the way, I didn't want to imply the "old" way was the right one, just more consistent with what I had in my head. So 👍 on this from my side.

This comment has been minimized.

@dennisoelkers

dennisoelkers Aug 25, 2016
Author Member

Yeah, I agree with you on that one and your remark is totally correct. It's just a matter of balancing consistency and choosing the strategy which seems to make the most sense. I am also unsure which one's more important here. :)

@bernd bernd removed their assignment Aug 24, 2016
@edmundoa
Copy link
Member

@edmundoa edmundoa commented Aug 24, 2016

Could you please rebase your changes with the current master? Thank you!

This change does the following when a user lacks the
'decorators:edit:${streamId}' permission:

  * disables items in action menu, when user lacks
  * disables controls to create decorator
  * disables ability to reorder decorators by drag & drop

Fixes #2730.
@dennisoelkers dennisoelkers force-pushed the issue-2730 branch from b21f946 to 470a534 Aug 25, 2016
@edmundoa
Copy link
Member

@edmundoa edmundoa commented Aug 25, 2016

LGTM 👍

@edmundoa edmundoa merged commit 77d8066 into master Aug 25, 2016
4 checks passed
4 checks passed
ci-server-integration Jenkins build graylog2-server-integration-pr 1321 has succeeded
Details
ci-web-linter Jenkins build graylog-pr-linter-check 804 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@edmundoa edmundoa deleted the issue-2730 branch Aug 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.