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

[Vis: Default editor] EUIficate time interval control #34991

Merged
merged 37 commits into from
May 3, 2019
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
b822888
EUIficate time interval control
maryia-lapata Apr 12, 2019
db04376
Merge branch 'master' into vis-editor/time-interval
maryia-lapata Apr 15, 2019
9d20af2
Update tests
maryia-lapata Apr 15, 2019
4f397f6
Remove empty option, update fix tests
maryia-lapata Apr 15, 2019
7911aa8
Bind vis to scope for react component only
maryia-lapata Apr 15, 2019
fd642b0
Merge branch 'master' into vis-editor/time-interval
maryia-lapata Apr 16, 2019
32604d0
Merge branch 'master' into vis-editor/time-interval
maryia-lapata Apr 17, 2019
735542e
Combine two interval inputs into one EuiCombobox
maryia-lapata Apr 18, 2019
7b794c3
Merge branch 'master' into vis-editor/time-interval
maryia-lapata Apr 22, 2019
e42d471
Add error message
maryia-lapata Apr 22, 2019
5d0d388
Add migration script; remove unused translations
maryia-lapata Apr 22, 2019
6f32466
Update fuctional tests
maryia-lapata Apr 22, 2019
62d52c6
Update unit test
maryia-lapata Apr 22, 2019
deebd42
Update tests; refactoring
maryia-lapata Apr 23, 2019
c8d0d02
Use flow to invoke several functions
maryia-lapata Apr 23, 2019
f2c4d56
Update test
maryia-lapata Apr 23, 2019
e772718
Refactoring
maryia-lapata Apr 23, 2019
70c4844
Reset options when timeBase
maryia-lapata Apr 23, 2019
9d3340c
Add type for editorConfig prop
maryia-lapata Apr 23, 2019
57fe4b5
Merge branch 'master' into vis-editor/time-interval
maryia-lapata Apr 23, 2019
2c803f5
Merge branch 'master' into vis-editor/time-interval
maryia-lapata Apr 24, 2019
9d24249
Add placeholder
maryia-lapata Apr 24, 2019
ab8fb50
Fix lint errors
maryia-lapata Apr 24, 2019
ffa9172
Call write after interval changing
maryia-lapata Apr 24, 2019
cfa1205
Merge branch 'master' into vis-editor/time-interval
maryia-lapata Apr 25, 2019
224634e
Merge branch 'master' into vis-editor/time-interval
maryia-lapata Apr 29, 2019
0b67d5f
Fix code review comments
maryia-lapata Apr 29, 2019
43aaaed
Make replace for model name global
maryia-lapata Apr 29, 2019
030b036
Revert error catch
maryia-lapata Apr 29, 2019
5571977
Merge branch 'master' into vis-editor/time-interval
maryia-lapata Apr 29, 2019
5f77403
Remove old dependency
maryia-lapata Apr 29, 2019
17f7bde
Add unit test for migration test
maryia-lapata Apr 29, 2019
f986a9c
Merge branch 'master' into vis-editor/time-interval
maryia-lapata Apr 30, 2019
155af1d
Fix message
maryia-lapata Apr 30, 2019
5542184
Fix code review comments
maryia-lapata Apr 30, 2019
aaf9e05
Merge branch 'master' into vis-editor/time-interval
maryia-lapata May 1, 2019
f531484
Update functional test
maryia-lapata May 1, 2019
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
111 changes: 72 additions & 39 deletions src/legacy/core_plugins/kibana/migrations.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/

import { cloneDeep, get, omit, has } from 'lodash';
import { cloneDeep, get, omit, has, flow } from 'lodash';

function migrateIndexPattern(doc) {
const searchSourceJSON = get(doc, 'attributes.kibanaSavedObjectMeta.searchSourceJSON');
Expand Down Expand Up @@ -57,6 +57,76 @@ function migrateIndexPattern(doc) {
doc.attributes.kibanaSavedObjectMeta.searchSourceJSON = JSON.stringify(searchSource);
}

// [TSVB] Migrate percentile-rank aggregation (value -> values)
const migratePercentileRankAggregation = doc => {
const visStateJSON = get(doc, 'attributes.visState');
let visState;

if (visStateJSON) {
try {
visState = JSON.parse(visStateJSON);
} catch (e) {
// Let it go, the data is invalid and we'll leave it as is
}
if (visState && visState.type === 'metrics') {
const series = get(visState, 'params.series') || [];

series.forEach(part => {
(part.metrics || []).forEach(metric => {
if (metric.type === 'percentile_rank' && has(metric, 'value')) {
metric.values = [metric.value];

delete metric.value;
}
});
});
return {
...doc,
attributes: {
...doc.attributes,
visState: JSON.stringify(visState),
},
};
}
}
return doc;
};

// Migrate date histogram aggregation (remove customInterval)
const migrateDateHistogramAggregation = doc => {
const visStateJSON = get(doc, 'attributes.visState');
let visState;

if (visStateJSON) {
try {
visState = JSON.parse(visStateJSON);
} catch (e) {
// Let it go, the data is invalid and we'll leave it as is
}

if (visState && visState.aggs) {
visState.aggs.forEach(agg => {
if (agg.type === 'date_histogram' && agg.params && agg.params.interval === 'custom') {
agg.params.interval = agg.params.customInterval;
delete agg.params.customInterval;
}

if (get(agg, 'params.customBucket.type', null) === 'date_histogram'
&& agg.params.customBucket.params
&& agg.params.customBucket.params.interval === 'custom'
) {
agg.params.customBucket.params.interval = agg.params.customBucket.params.customInterval;
delete agg.params.customBucket.params.customInterval;
}
});
doc.attributes.visState = JSON.stringify(visState);
maryia-lapata marked this conversation as resolved.
Show resolved Hide resolved
}
}
return doc;
};

const executeMigrations710 = flow(migratePercentileRankAggregation, migrateDateHistogramAggregation);

function removeDateHistogramTimeZones(doc) {
const visStateJSON = get(doc, 'attributes.visState');
if (visStateJSON) {
Expand Down Expand Up @@ -185,44 +255,7 @@ export const migrations = {
}
},
'7.0.1': removeDateHistogramTimeZones,
'7.1.0': doc => {
// [TSVB] Migrate percentile-rank aggregation (value -> values)
const migratePercentileRankAggregation = doc => {
const visStateJSON = get(doc, 'attributes.visState');
let visState;

if (visStateJSON) {
try {
visState = JSON.parse(visStateJSON);
} catch (e) {
// Let it go, the data is invalid and we'll leave it as is
}
if (visState && visState.type === 'metrics') {
const series = get(visState, 'params.series') || [];

series.forEach(part => {
(part.metrics || []).forEach(metric => {
if (metric.type === 'percentile_rank' && has(metric, 'value')) {
metric.values = [metric.value];

delete metric.value;
}
});
});
return {
...doc,
attributes: {
...doc.attributes,
visState: JSON.stringify(visState),
},
};
}
}
return doc;
};

return migratePercentileRankAggregation(doc);
}
'7.1.0': doc => executeMigrations710(doc)
},
dashboard: {
'7.0.0': (doc) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ describe('editor', function () {
beforeEach(ngMock.inject(function () {
field = _.sample(indexPattern.fields);
interval = _.sample(intervalOptions);
params = render({ field: field, interval: interval });
params = render({ field: field, interval: interval.val });
}));

it('renders the field editor', function () {
Expand All @@ -112,11 +112,11 @@ describe('editor', function () {
});

it('renders the interval editor', function () {
expect(agg.params.interval).to.be(interval);
expect(agg.params.interval).to.be(interval.val);

expect(params).to.have.property('interval');
expect(params.interval).to.have.property('$el');
expect(params.interval.modelValue()).to.be(interval);
expect($scope.agg.params.interval).to.be(interval.val);
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,8 @@ describe('params', function () {
expect(output.params).to.have.property('interval', '1d');
});

it('ignores invalid intervals', function () {
const output = writeInterval('foo');
expect(output.params).to.have.property('interval', '0ms');
it('throws error when interval is invalid', function () {
expect(() => writeInterval('foo')).to.throw('TypeError: "foo" is not a valid interval.');
});

it('automatically picks an interval', function () {
Expand Down
9 changes: 8 additions & 1 deletion src/legacy/ui/public/agg_types/agg_param.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,17 @@ import { AggConfig } from '../vis';
interface AggParam {
type: string;
name: string;
options?: AggParamOption[];
required?: boolean;
displayName?: string;
onChange?(agg: AggConfig): void;
shouldShow?(agg: AggConfig): boolean;
}

export { AggParam };
interface AggParamOption {
val: string;
display: string;
enabled?(agg: AggConfig): void;
}

export { AggParam, AggParamOption };
6 changes: 0 additions & 6 deletions src/legacy/ui/public/agg_types/buckets/_interval_options.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,5 @@ export const intervalOptions = [
defaultMessage: 'Yearly',
}),
val: 'y'
},
{
display: i18n.translate('common.ui.aggTypes.buckets.intervalOptions.customDisplayName', {
defaultMessage: 'Custom',
}),
val: 'custom'
}
];
45 changes: 22 additions & 23 deletions src/legacy/ui/public/agg_types/buckets/date_histogram.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,11 @@
import _ from 'lodash';
import chrome from '../../chrome';
import moment from 'moment-timezone';
import '../directives/validate_date_interval';
import { BucketAggType } from './_bucket_agg_type';
import { TimeBuckets } from '../../time_buckets';
import { createFilterDateHistogram } from './create_filter/date_histogram';
import { intervalOptions } from './_interval_options';
import intervalTemplate from '../controls/time_interval.html';
import { TimeIntervalParamEditor } from '../controls/time_interval';
import { timefilter } from '../../timefilter';
import { DropPartialsParamEditor } from '../controls/drop_partials';
import { i18n } from '@kbn/i18n';
Expand All @@ -35,11 +34,7 @@ const detectedTimezone = moment.tz.guess();
const tzOffset = moment().format('Z');

function getInterval(agg) {
const interval = _.get(agg, ['params', 'interval']);
if (interval && interval.val === 'custom') {
return _.get(agg, ['params', 'customInterval']);
}
return interval;
return _.get(agg, ['params', 'interval']);
}

export function setBounds(agg, force) {
Expand All @@ -59,7 +54,13 @@ export const dateHistogramBucketAgg = new BucketAggType({
date: true
},
makeLabel: function (agg) {
const output = this.params.write(agg);
let output = '';
try {
output = this.params.write(agg);
} catch (e) {
// if interval is invalid, return empty string
Copy link
Contributor

Choose a reason for hiding this comment

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

What can cause a write failure? Is it always a good idea to swallow the error? Should we log or do something to indicate a failure here? (I don't know the answers to these questions. I just want to be sure this is what we really want to do. Silent catches are often a source of bugs.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Essentially a write failure is caused by an invalid time interval. But in fact the reason is that this makeLabel method is invoked every digest cycle.
On master when a user types an invalid interval, TypeError is printed in console.log. Since currently this error doesn't break UI, and to not break some logic that might be based on such exception, I will leave the code as it is. (I reverted the changes)

return '';
}
const field = agg.getFieldDisplayName();
return i18n.translate('common.ui.aggTypes.buckets.dateHistogramLabel', {
defaultMessage: '{fieldName} per {intervalDescription}',
Expand Down Expand Up @@ -99,7 +100,7 @@ export const dateHistogramBucketAgg = new BucketAggType({
return agg.getIndexPattern().timeFieldName;
},
onChange: function (agg) {
if (_.get(agg, 'params.interval.val') === 'auto' && !agg.fieldIsTimeField()) {
if (_.get(agg, 'params.interval') === 'auto' && !agg.fieldIsTimeField()) {
delete agg.params.interval;
}

Expand All @@ -118,18 +119,22 @@ export const dateHistogramBucketAgg = new BucketAggType({
},
{
name: 'interval',
type: 'optioned',
deserialize: function (state) {
editorComponent: TimeIntervalParamEditor,
deserialize: function (state, agg) {
// For upgrading from 7.0.x to 7.1.x - intervals are now stored as key of options or custom value
if (state === 'custom') return _.get(agg, 'params.customInterval');
maryia-lapata marked this conversation as resolved.
Show resolved Hide resolved

const interval = _.find(intervalOptions, { val: state });
return interval || _.find(intervalOptions, function (option) {
// For upgrading from 4.0.x to 4.1.x - intervals are now stored as 'y' instead of 'year',
// but this maps the old values to the new values
return Number(moment.duration(1, state)) === Number(moment.duration(1, option.val));
});

// For upgrading from 4.0.x to 4.1.x - intervals are now stored as 'y' instead of 'year',
// but this maps the old values to the new values
if (!interval && state === 'year') {
return 'y';
}
return state;
},
default: 'auto',
options: intervalOptions,
editor: intervalTemplate,
modifyAggConfigOnSearchRequestStart: function (agg) {
setBounds(agg, true);
},
Expand Down Expand Up @@ -177,12 +182,6 @@ export const dateHistogramBucketAgg = new BucketAggType({
return field && field.name && field.name === agg.getIndexPattern().timeFieldName;
},
},

{
name: 'customInterval',
default: '2h',
write: _.noop
},
{
name: 'format'
},
Expand Down
1 change: 0 additions & 1 deletion src/legacy/ui/public/agg_types/buckets/histogram.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import _ from 'lodash';

import { toastNotifications } from 'ui/notify';
import '../directives/validate_date_interval';
import '../directives/input_number';
import chrome from '../../chrome';
import { BucketAggType } from './_bucket_agg_type';
Expand Down
7 changes: 6 additions & 1 deletion src/legacy/ui/public/agg_types/controls/string.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ function StringParamEditor({
[value]
);

const onChange = (ev: React.ChangeEvent<HTMLInputElement>) => {
const textValue = ev.target.value;
setValidity(aggParam.required ? !!textValue : true);
maryia-lapata marked this conversation as resolved.
Show resolved Hide resolved
setValue(textValue);
};
return (
<EuiFormRow
label={aggParam.displayName || aggParam.name}
Expand All @@ -50,7 +55,7 @@ function StringParamEditor({
<EuiFieldText
value={value || ''}
data-test-subj={`visEditorStringInput${agg.id}${aggParam.name}`}
onChange={ev => setValue(ev.target.value)}
onChange={onChange}
fullWidth={true}
onBlur={setTouched}
isInvalid={isInvalid}
Expand Down
58 changes: 0 additions & 58 deletions src/legacy/ui/public/agg_types/controls/time_interval.html

This file was deleted.

Loading