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

Allow clearing optional values in plugins #2114

Merged
merged 1 commit into from Apr 20, 2016
Merged

Allow clearing optional values in plugins #2114

merged 1 commit into from Apr 20, 2016

Conversation

@edmundoa
Copy link
Member

@edmundoa edmundoa commented Apr 20, 2016

These changes allow users to clear out optional values on pluggable entities forms.

Fixes #2108

Allow clearing out optional values on pluggable entities. Fixes #2108
@@ -51,7 +51,12 @@
try {
value = Integer.parseInt(String.valueOf(entry.getValue()));
} catch (NumberFormatException e) {
throw new ValidationException(field, e.getMessage());
// If a numeric field is optional and not provided, use null as value
if ("true".equals(String.valueOf(fieldDescription.get("is_optional")))) {
Copy link
Contributor

@joschi joschi Apr 20, 2016

Choose a reason for hiding this comment

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

This expression could be simplified using Boolean#valueOf(String):

if (Boolean.valueOf(fieldDescription.get("is_optional")))

Loading

Copy link
Member Author

@edmundoa edmundoa Apr 20, 2016

Choose a reason for hiding this comment

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

Not really, since fieldDescription values are of type Object. In the end we could cast the value in is_optional to a String, but that won't work if the value is actually a Boolean.

Loading

Copy link
Contributor

@joschi joschi Apr 20, 2016

Choose a reason for hiding this comment

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

Loading

@joschi joschi self-assigned this Apr 20, 2016
@joschi joschi added this to the 2.0.0 milestone Apr 20, 2016
@joschi
Copy link
Contributor

@joschi joschi commented Apr 20, 2016

LGTM. 👍

Loading

@joschi joschi merged commit a7e2330 into master Apr 20, 2016
4 checks passed
Loading
@joschi joschi deleted the issue-2108 branch Apr 20, 2016
edmundoa added a commit that referenced this issue Apr 21, 2016
Changes in #2114 caused another issue with the pluggable entities forms:
default values were shown in forms, but never submitted to the server.

These changes move the logic taking care of default values to the
`ConfigurationForm` component, also removing incorrect uses of
`defaultValue` in inputs.

In this way, the object containing the data will contain the default
values at the beginning, and they will be overriden by any input the
user type in them.

Refs #2108
bernd added a commit that referenced this issue Apr 21, 2016
Changes in #2114 caused another issue with the pluggable entities forms:
default values were shown in forms, but never submitted to the server.

These changes move the logic taking care of default values to the
`ConfigurationForm` component, also removing incorrect uses of
`defaultValue` in inputs.

In this way, the object containing the data will contain the default
values at the beginning, and they will be overriden by any input the
user type in them.

Refs #2108
@magicdude4eva
Copy link

@magicdude4eva magicdude4eva commented Apr 22, 2016

Is it possible to determine the Graylog version within an Alarmcallback (preferably during getRequestedConfiguration)?

This would then allow me to set default values for Graylog 2.0.0 (as a user can now delete the defaults) and for older versions not populating the defaults.

Loading

@joschi
Copy link
Contributor

@joschi joschi commented Apr 22, 2016

@magicdude4eva We are using GitHub issues for tracking bugs in Graylog itself, but this doesn't look like one. Please post your questions to our public mailing list or join the #graylog channel on freenode IRC.

Thank you!

Loading

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.

None yet

3 participants