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

Only show extractor actions on string fields #3404

Merged
merged 3 commits into from Jan 23, 2017
Merged

Only show extractor actions on string fields #3404

merged 3 commits into from Jan 23, 2017

Conversation

@joschi
Copy link
Contributor

@joschi joschi commented Jan 20, 2017

Extractors only work on strings, so the web interface should only show the extractor actions for message fields of type "string" and hide them for all other types.

Fixes #3396

Extractors only work on strings, so the web interface should only show the
extractor actions for message fields of type "string" and hide them for all
other types.

Fixes #3396
@joschi joschi added this to the 2.2.0 milestone Jan 20, 2017
Copy link
Member

@edmundoa edmundoa left a comment

This is only one way of creating extractors, it is also possible to create new extractors for a field in the search page, using MessageFieldSearchActions.jsx

</div>
);
} else {
return (<div className="message-field-actions pull-right"/>);

This comment has been minimized.

@edmundoa

edmundoa Jan 20, 2017
Member

We should display something in this case, I would keep a disabled drop-down with a message saying that extractors cannot be applied to non-string fields.

This comment has been minimized.

@edmundoa

edmundoa Jan 20, 2017
Member

For example, something like we do in here:

editRulesLink = (
<OverlayElement overlay={defaultStreamTooltip} placement="top" useOverlay={isDefaultStream}>
<LinkContainer disabled={isDefaultStream} to={Routes.stream_edit(stream.id)}>
<Button bsStyle="info">Manage Rules</Button>
</LinkContainer>
</OverlayElement>
);

This comment has been minimized.

@joschi

joschi Jan 20, 2017
Author Contributor

Personally, I find it more obvious if the dropdown list was simply missing, but for the sake of consistency I've added back the dropdown and disabled it for non-string fields.

This comment has been minimized.

@edmundoa

edmundoa Jan 20, 2017
Member

👍 In this case I think it makes a big difference if you know the system well or not. In our case, we know why the dropdown is not shown, so it may actually save us time, as we know which fields are not strings very easily. On the other hand, for somebody just starting to use Graylog, they would be confused because some fields are missing the dropdown for non-obvious reasons. So far we have (mostly) aligned with the second "persona".

@edmundoa edmundoa self-assigned this Jan 20, 2017
@joschi joschi force-pushed the issue-3396 branch from 6e46b4c to 2225479 Jan 23, 2017
Copy link
Member

@edmundoa edmundoa left a comment

Just a couple of style issues, other than that looks good to me and solves the issue.

</DropdownButton>
</div>
);
let messageField = this.props.message.fields[this.props.fieldName];

This comment has been minimized.

@edmundoa

edmundoa Jan 23, 2017
Member

Small nitpick: can we use const instead of let in here? The variable is never modified afterwards.

@@ -23,6 +23,18 @@ const MessageFieldSearchActions = React.createClass({
);
},
render() {
let messageField = this.props.message.fields[this.props.fieldName];

This comment has been minimized.

@edmundoa

edmundoa Jan 23, 2017
Member

Small nitpick: can we use const instead of let in here? The variable is never modified afterwards.

title="Select extractor type"
key={1}
id={`select-extractor-type-dropdown-field-${this.props.fieldName}`}>
<MenuItem key={`menu-item-disabled`} disabled={true}>

This comment has been minimized.

@edmundoa

edmundoa Jan 23, 2017
Member

Here another couple of nitpicks:

  • key can be a regular string
  • We don't need to pass the boolean value to the disabled flag (it breaks the react/jsx-boolean-value rule)
</ul>
</li>);
} else {
extractors = (<MenuItem disabled={true}>Extractors can only be used with string fields</MenuItem>);

This comment has been minimized.

@edmundoa

edmundoa Jan 23, 2017
Member

Same as in the other file:

  • key can be a regular string
  • We don't need to pass the boolean value to the disabled flag (it breaks the react/jsx-boolean-value rule)
Copy link
Member

@edmundoa edmundoa left a comment

LGTM 👍

@edmundoa edmundoa merged commit c6aca7e into master Jan 23, 2017
4 checks passed
4 checks passed
@garybot2
ci-web-linter Jenkins build graylog-pr-linter-check 1311 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-3396 branch Jan 23, 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.

None yet

2 participants