[indexPatterns/create] disable time-based options when needed #12011

spalger · 2017-05-25T20:44:13Z

When the time field option "I don't want to use the time filter" is chosen the options for wildcard expanding and time-based patterns are not hidden. This fixes that by adding a controller.canUseTimePattern() method that is used in the view to remove the irrelevant sections when necessary.

chrisronline

Looks good! I had a few questions and one small request

chrisronline · 2017-05-26T13:18:24Z

...ugins/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.js

+    // if timeFieldOption has a fieldName it's a time field, otherwise
+    // it's a way to opt-out of the time field or an indication that there
+    // are no fields available
+    return !!this.formValues.timeFieldOption.fieldName;


I've seen Boolean( used in other places. Are we trying to standardize on one or the other?

Either works for me, I don't think we have picked one.

chrisronline · 2017-05-26T13:38:10Z

...ugins/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.js

+    return (
+      this.isTimeBased() &&
+        !this.formValues.nameIsPattern &&
+        _.includes(this.formValues.name, '*')


Out of scope for this PR, but isn't this.formValues.name a string? If it's a string, we probably don't need to use the lodash .includes( method which is normally used on collections.

Correct, I imagine it's used here to automatically handle the scenario when formValues.name is not a string (null/undefined for instance)

chrisronline · 2017-05-26T13:42:05Z

...ugins/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.js

      ? true
      : undefined;

    // Only event-time-based index patterns set an intervalName.
-    const intervalName = (nameIsPattern && nameInterval)
+    const intervalName = (this.canUseTimePattern() && nameIsPattern && nameInterval)


Since the select field that sets the nameInterval is now hidden behind a more aggressive ng-if, do you think we only need to check if nameInterval exists at this point? It feels the first two conditions are redundant

Well, ng-if doesn't unset the model value when the inputs are removed from the view, so it's possible that the name interval is still set when the name isn't actually a time pattern

chrisronline · 2017-05-26T13:44:52Z

...ugins/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.js

@@ -320,24 +338,28 @@ uiModules.get('apps/management')

    let lastPromise;
    resetIndex();
-    samplePromise = lastPromise = updateSamples()
+    samplePromise = lastPromise = Promise.resolve()


This whole samplePromise and lastPromise is very confusing. While you're in here, do you mind adding some clarifying comments explaining what these do and why they are necessary?

Happy to, do you think it would help it I made it look more like this? https://github.com/elastic/kibana/pull/12007/files#diff-f5a949c353fd77eda951919524c3b7ecR199

I think it'd be clearer if we use the pattern you linked to. I find this:

samplePromise = lastPromise = Promise.resolve()

to be a real head scratcher.

…s/disable-time-based-opts

cjcenizal

Overall this looks great, I just think the async-request-tracking logic would be clarified.

cjcenizal · 2017-05-26T22:20:27Z

...ins/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.html

@@ -160,7 +160,7 @@
          </div>

          <!-- Use event times checkbox -->
-          <div class="kuiVerticalRhythm">
+          <div class="kuiVerticalRhythm" ng-if="controller.canUseTimePattern()">


Maybe put onto multiple lines for a consistent pattern?

<div class="kuiVerticalRhythm" ng-if="controller.canUseTimePattern()" >

cjcenizal · 2017-05-26T22:21:46Z

...ugins/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.js

@@ -282,7 +300,7 @@ uiModules.get('apps/management')

  $scope.$watchMulti([
    'controller.formValues.nameIsPattern',
-    'controller.formValues.nameInterval.name'
+    'controller.formValues.nameInterval.name',


Some people would shake their heads and tsk at these types of "unrelated" changes, but I applaud you, sir.

cjcenizal · 2017-05-26T22:25:47Z

...ugins/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.js

@@ -320,24 +338,28 @@ uiModules.get('apps/management')

    let lastPromise;
    resetIndex();
-    samplePromise = lastPromise = updateSamples()
+    samplePromise = lastPromise = Promise.resolve()


I think it'd be clearer if we use the pattern you linked to. I find this:

samplePromise = lastPromise = Promise.resolve()

to be a real head scratcher.

spalger · 2017-05-26T22:48:47Z

@cjcenizal @chrisronline thanks for taking a look, hope I've covered everything

cjcenizal

Nice!

…s/disable-time-based-opts

chrisronline · 2017-05-30T13:44:10Z

LGTM!

* [indexPatterns/create] disable time-based options when needed * !! -> Boolean() * [indexPatterns/create] refactor sample deduping for clarity * [indexPattern/create] wrap html attributes (cherry picked from commit 185912d)

spalger · 2017-05-30T18:40:47Z

5.5/5.x: 15d4d83

…c#12011) * [indexPatterns/create] disable time-based options when needed * !! -> Boolean() * [indexPatterns/create] refactor sample deduping for clarity * [indexPattern/create] wrap html attributes

spalger added :Management blocked v5.5.0 v6.0.0 labels May 25, 2017

spalger requested review from chrisronline and cjcenizal May 25, 2017 20:44

[indexPatterns/create] disable time-based options when needed

800b112

spalger force-pushed the index-patterns/disable-time-based-opts branch from 598be43 to 800b112 Compare May 26, 2017 00:26

spalger added review and removed blocked labels May 26, 2017

chrisronline suggested changes May 26, 2017

View reviewed changes

spalger added 2 commits May 26, 2017 12:56

Merge branch 'master' of github.com:elastic/kibana into index-pattern…

b0a7864

…s/disable-time-based-opts

Merge branch 'master' of github.com:elastic/kibana into index-pattern…

b1b4944

…s/disable-time-based-opts

cjcenizal suggested changes May 26, 2017

View reviewed changes

spalger added 3 commits May 26, 2017 15:45

!! -> Boolean()

55eba84

[indexPatterns/create] refactor sample deduping for clarity

4252744

[indexPattern/create] wrap html attributes

ce8b8a2

cjcenizal approved these changes May 26, 2017

View reviewed changes

Merge branch 'master' of github.com:elastic/kibana into index-pattern…

1c5a6c0

…s/disable-time-based-opts

chrisronline approved these changes May 30, 2017

View reviewed changes

spalger merged commit 185912d into elastic:master May 30, 2017

spalger deleted the index-patterns/disable-time-based-opts branch May 30, 2017 18:40

This was referenced May 31, 2017

[IndexPatterns] Support cross cluster search #11114

Merged

[indexPatterns/create] remove check for interval/name in name watcher #12101

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[indexPatterns/create] disable time-based options when needed #12011

[indexPatterns/create] disable time-based options when needed #12011

spalger commented May 25, 2017 •

edited

Loading

chrisronline left a comment

chrisronline May 26, 2017

spalger May 26, 2017

chrisronline May 26, 2017

spalger May 26, 2017

chrisronline May 26, 2017

spalger May 26, 2017

chrisronline May 26, 2017

spalger May 26, 2017

cjcenizal May 26, 2017

cjcenizal left a comment

cjcenizal May 26, 2017

cjcenizal May 26, 2017

cjcenizal May 26, 2017

spalger commented May 26, 2017

cjcenizal left a comment

chrisronline commented May 30, 2017

spalger commented May 30, 2017

[indexPatterns/create] disable time-based options when needed #12011

[indexPatterns/create] disable time-based options when needed #12011

Conversation

spalger commented May 25, 2017 • edited Loading

chrisronline left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger commented May 26, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

chrisronline commented May 30, 2017

spalger commented May 30, 2017

spalger commented May 25, 2017 •

edited

Loading