[indexPatterns/create] refactor time field options #11996
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,37 +29,23 @@ uiModules.get('apps/management') | |
| nameIsPattern: false, | ||
| expandable: false, | ||
| nameInterval: _.find(intervals, { name: 'daily' }), | ||
| timeFieldOption: null, | ||
| }; | ||
|
|
||
| // UI state. | ||
| this.timeFieldOptions = []; | ||
| this.timeFieldOptionsError = null; | ||
| this.sampleCount = 5; | ||
| this.samples = null; | ||
| this.existing = null; | ||
| this.nameIntervalOptions = intervals; | ||
| this.patternErrors = []; | ||
|
|
||
| const getTimeFieldOptions = () => { | ||
| const missingPattern = !this.formValues.name; | ||
| const missingInterval = this.formValues.nameIsPattern && !this.formValues.nameInterval; | ||
| if (missingPattern || missingInterval) { | ||
| return Promise.resolve({ options: [] }); | ||
| } | ||
|
|
||
| loadingCount += 1; | ||
|
|
@@ -69,79 +55,82 @@ uiModules.get('apps/management') | |
|
|
||
| return indexPatterns.mapper.getFieldsForIndexPattern(pattern, { | ||
| skipIndexPatternCache: true, | ||
| }); | ||
| }) | ||
| .then(fields => { | ||
| const dateFields = fields.filter(field => field.type === 'date'); | ||
|
|
||
| if (dateFields.length === 0) { | ||
| return { | ||
| options: [ | ||
| { | ||
| display: $translate.instant('KIBANA-INDICES_DONT_CONTAIN_TIME_FIELDS') | ||
| } | ||
| ] | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| options: [ | ||
| { | ||
| display: $translate.instant('KIBANA-NO_DATE_FIELD_DESIRED') | ||
| }, | ||
| ...dateFields.map(field => ({ | ||
| display: field.name, | ||
| fieldName: field.name | ||
| })), | ||
| ] | ||
| }; | ||
| }) | ||
| .catch(err => { | ||
| if (err instanceof IndexPatternMissingIndices) { | ||
| return { | ||
| error: $translate.instant('KIBANA-INDICES_MATCH_PATTERN') | ||
| }; | ||
| } | ||
|
|
||
| throw err; | ||
| }) | ||
| .finally(() => { | ||
| loadingCount -= 1; | ||
| }); | ||
| }; | ||
|
|
||
| const findTimeFieldOption = match => { | ||
| if (!match) return; | ||
|
|
||
| return this.timeFieldOptions.find(option => ( | ||
| // comparison is not done with _.isEqual() because options get a unique | ||
| // `$$hashKey` tag attached to them by ng-repeat | ||
| option.fieldName === match.fieldName && | ||
| option.display === match.display | ||
| )); | ||
| }; | ||
|
|
||
| const pickDefaultTimeFieldOption = () => { | ||
| const noOptions = this.timeFieldOptions.length === 0; | ||
| // options that represent a time field | ||
| const fieldOptions = this.timeFieldOptions.filter(option => !!option.fieldName); | ||
| // options like "I don't want the time filter" or "There are no date fields" | ||
| const nonFieldOptions = this.timeFieldOptions.filter(option => !option.fieldName); | ||
| // if there are multiple field or non-field options then we can't select a default, the user must choose | ||
| const tooManyOptions = fieldOptions.length > 1 || nonFieldOptions.length > 1; | ||
|
|
||
| if (noOptions || tooManyOptions) { | ||
| return null; | ||
| } | ||
|
|
||
| if (fieldOptions.length === 1) { | ||
| return fieldOptions[0]; | ||
| } | ||
|
|
||
| return nonFieldOptions[0]; | ||
| }; | ||
|
|
||
| const resetIndex = () => { | ||
| this.patternErrors = []; | ||
| this.samples = null; | ||
| this.existing = null; | ||
| }; | ||
|
|
||
| function mockIndexPattern(index) { | ||
|
|
@@ -207,39 +196,57 @@ uiModules.get('apps/management') | |
| return loadingCount > 0; | ||
| }; | ||
|
|
||
| this.refreshTimeFieldOptions = () => { | ||
| const prevOption = this.formValues.timeFieldOption; | ||
|
|
||
| loadingCount += 1; | ||
| this.timeFieldOptions = []; | ||
| this.timeFieldOptionsError = null; | ||
| this.formValues.timeFieldOption = null; | ||
| getTimeFieldOptions() | ||
|
There was a problem hiding this comment. Is there a reason we aren't opting for the There was a problem hiding this comment. Unfortunately, There was a problem hiding this comment. Interesting. Why doesn't a simple There was a problem hiding this comment. Manually triggering digest cycles is really a last resort option and can lead to all sorts of issues even in simple apps. There was a problem hiding this comment. That's interesting. I'd love to hear more about this. Do you have any resources and/or past github issues around this? There was a problem hiding this comment.
I agree that sometimes There was a problem hiding this comment. I agree, we should avoid using $apply where possible. I think the way to do that is to follow a few simple rules:
There are probably a few others we can dig up by Googling but the general theme is to just use whatever tools Angular makes available which will tie into the $digest cycle under the hood. These are situations where $apply is inappropriate. This leaves us with the situations where $apply is appropriate: anywhere these tools can't be used. I think if we can confidently assess a call stack and state that it will never touch any of these tools, and therefore never trigger a $digest cycle on its own, then we can safely use $apply. Thoughts @spalger and @chrisronline ? There was a problem hiding this comment. @cjcenizal by that logic there are no function in this PR that qualify unless we reimplement some core services. All of the asynchronicity in this controller is introduced by the If you're suggesting that we write more code that is completely disconnected from angular, that only inter-ops with angular at the last minute using something like My stance in this discussion so far was against using angular services in a non-angular context (like an This specific pr is even worse in my opinion, converting any of the helpers to There was a problem hiding this comment.
Then I guess we can't use $apply. :)
I see, sorry, I didn't pick up on that earlier. I thought we were just talking about how to use $apply correctly. There was a problem hiding this comment. @spalger I think that's an excellent point and something I wasn't considering fully. You're right in that I'm advising to add a layer from angular that needs to go back into angular and that's probably not a good idea unless there is a really good reason. I'm happy with the discussion around this and I 100% agree that we should try and decouple our business logic away from our angular implementation. With that said, I'm fine with the code as-is and appreciate the thoughts! |
||
| .then(({ options, error }) => { | ||
| this.timeFieldOptions = options; | ||
| this.timeFieldOptionsError = error; | ||
| if (!this.timeFieldOptions) { | ||
| return; | ||
| } | ||
|
|
||
| // Restore the preivously selected state, or select the default option in the UI | ||
| const restoredOption = findTimeFieldOption(prevOption); | ||
| const defaultOption = pickDefaultTimeFieldOption(); | ||
| this.formValues.timeFieldOption = restoredOption || defaultOption; | ||
| }) | ||
| .catch(notify.error) | ||
| .finally(() => { | ||
| loadingCount -= 1; | ||
| }); | ||
| }; | ||
|
|
||
| this.createIndexPattern = () => { | ||
| const { | ||
| name, | ||
| timeFieldOption, | ||
| nameIsPattern, | ||
| nameInterval, | ||
| expandable | ||
| } = this.formValues; | ||
|
|
||
| const id = name; | ||
|
|
||
| const timeFieldName = timeFieldOption | ||
| ? timeFieldOption.fieldName | ||
| : undefined; | ||
|
|
||
| // this seems wrong, but it's the original logic... https://git.io/vHYFo | ||
| const notExpandable = (!expandable && this.canExpandIndices()) | ||
| ? true | ||
| : undefined; | ||
|
|
||
| // Only event-time-based index patterns set an intervalName. | ||
| const intervalName = (nameIsPattern && nameInterval) | ||
| ? nameInterval.name | ||
| : undefined; | ||
|
There was a problem hiding this comment. Why There was a problem hiding this comment. In my eyes This is trying to represent "no intervalName", so I don't think |
||
|
|
||
| loadingCount += 1; | ||
| sendCreateIndexPatternRequest(indexPatterns, { | ||
| id, | ||
|
|
@@ -287,7 +294,6 @@ uiModules.get('apps/management') | |
|
|
||
| if (!nameIsPattern) { | ||
| delete this.formValues.nameInterval; | ||
| } else { | ||
| this.formValues.nameInterval = this.formValues.nameInterval || intervals.byName.days; | ||
| this.formValues.name = this.formValues.name || getDefaultPatternForInterval(this.formValues.nameInterval); | ||
|
|
@@ -330,28 +336,23 @@ uiModules.get('apps/management') | |
| .finally(() => { | ||
| // prevent running when no change happened (ie, first watcher call) | ||
| if (!_.isEqual(newVal, oldVal)) { | ||
| this.refreshTimeFieldOptions(); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| $scope.$watchMulti([ | ||
| 'controller.sampleCount' | ||
| ], () => { | ||
| this.refreshTimeFieldOptions(); | ||
| }); | ||
|
|
||
| $scope.$watchMulti([ | ||
| 'controller.isLoading()', | ||
| 'form.name.$error.indexNameInput', | ||
| 'controller.formValues.timeFieldOption' | ||
| ], ([loading, invalidIndexName, timeFieldOption]) => { | ||
| const state = { loading, invalidIndexName, timeFieldOption }; | ||
| this.createButtonText = pickCreateButtonText($translate, state); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️ this comment