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

warn when neither reader nor admin roles are selected for a user #3337

Merged
merged 3 commits into from Jan 11, 2017

Conversation

@kroepke
Copy link
Member

@kroepke kroepke commented Jan 11, 2017

disallow saving without at least one of these roles present (because lots of pages require at least the reader permissions)

fixes #2116

disallow saving without at least one of these roles present (because lots of pages require at least the reader permissions)

 fixes #2116
@kroepke kroepke added this to the 2.2.0 milestone Jan 11, 2017
@kroepke kroepke requested a review from edmundoa Jan 11, 2017
@edmundoa edmundoa self-assigned this Jan 11, 2017
Copy link
Member

@edmundoa edmundoa left a comment

Apart from the mentioned issues, this works well in the tests I did on the edit user page. Unfortunately, it is still possible to not include the reader/admin roles in the create user form.

As the Input containing both usages of RolesSelect is basically the same, I would suggest adding a new component that takes care of displaying the Input and validating the roles as these changes do.

render() {
const user = this.props.user;
if (!this.state.roles) {
return <Spinner />;
}
let rolesAlert = null;
const roles = this.state.newRoles;
if (roles != null && roles.indexOf('Reader') === -1 && roles.indexOf('Admin') === -1) {

This comment has been minimized.

@edmundoa

edmundoa Jan 11, 2017
Member

You could replace roles.indexOf() with roles.includes() for better reading experience. The polyfill we use should take care of converting it if the browser does not support it.

This comment has been minimized.

@kroepke

kroepke Jan 11, 2017
Author Member

Nice, didn't know about it!

let rolesAlert = null;
const roles = this.state.newRoles;
if (roles != null && roles.indexOf('Reader') === -1 && roles.indexOf('Admin') === -1) {
rolesAlert = <Alert bsStyle="danger" role="alert" className={EditRolesFormStyle.rolesMissingAlert}>

This comment has been minimized.

@edmundoa

edmundoa Jan 11, 2017
Member

Small nitpick: this triggers a react/wrap-multilines styling warning.

This comment has been minimized.

@kroepke

kroepke Jan 11, 2017
Author Member

fixed

kroepke added 2 commits Jan 11, 2017
switch to using Array#includes for clarity
@kroepke
Copy link
Member Author

@kroepke kroepke commented Jan 11, 2017

I opted for a simpler approach by duplication the few lines of code, rather than passing around styling props and the validation state.

Copy link
Member

@edmundoa edmundoa left a comment

LGTM 👍

@edmundoa edmundoa merged commit aabe9cf into master Jan 11, 2017
4 checks passed
4 checks passed
@garybot2
ci-web-linter Jenkins build graylog-pr-linter-check 1266 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
licence/cla Contributor License Agreement is signed.
Details
@edmundoa edmundoa deleted the issue-2116 branch Jan 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants