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.

Sign up for GitHub

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

Fixing bug where an error is displayed when saving a new scripted field #10820

Merged
merged 2 commits into from
Apr 10, 2017
+2 −1
Merged

Fixing bug where an error is displayed when saving a new scripted field #10820

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
47ca365
Fixing bug where an error is displayed when saving a new scripted field
kobelb Mar 20, 2017
fe7eb98
Switching approach to copying fieldNames on instantiation
kobelb Mar 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/ui/public/field_editor/field_editor.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
class="form-control"
data-test-subj="editorFieldName">
</div>
<div ng-if="editor.creating && editor.indexPattern.fields.byName[editor.field.name]" class="hintbox">
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we could replace editor.indexPattern.fields.byName[editor.field.name] with editor.existingFieldNames.includes(editor.field.name)

<div ng-if="!editor.saving && editor.creating && editor.indexPattern.fields.byName[editor.field.name]" class="hintbox">
<p>
<i class="fa fa-danger text-danger"></i>
<strong>Mapping Conflict:</strong>
Expand Down
2 changes: 2 additions & 0 deletions src/ui/public/field_editor/field_editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ uiModules

self.cancel = redirectAway;
self.save = function () {
self.saving = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm always wary of flags like this since they can get stuck in an incorrect state. For instance if indexPattern.save fails on line 75 self.saving will be stuck as true.

Instead, what would you think of creating an array of existing field names on controller instantiation (around line 52) and then use that in our template's conditional instead of the live index pattern?

      self.existingFieldNames = self.indexPattern.fields.map((field) => {
        return field.name;
      });

Copy link
Contributor Author

@kobelb kobelb Mar 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Seems reasonable, I will make it so.

I've found myself using state flags to denote asynchronous actions and their current state a lot to disable buttons, etc. (especially with React/Flux) but you're right that in this scenario the lack of a .catch could potentially cause issues.

It's also probably worthwhile, in another PR, to add in a .catch so if an error is thrown that we display it to the user.


const indexPattern = self.indexPattern;
const fields = indexPattern.fields;
const field = self.field.toActualField();
Expand Down