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

[Rollup] Metrics multiple selection #42927

Merged
merged 23 commits into from
Aug 15, 2019
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
b643f7e
remove duplicate columns declaration
jloleysens Aug 6, 2019
d3e08f3
First iteration of select all menu working
jloleysens Aug 7, 2019
2e24223
A lot of bugfixes and started on row-level selection using flatMap
jloleysens Aug 7, 2019
428a17a
Wip on row selection
jloleysens Aug 8, 2019
d6eda9e
Fix formatting
jloleysens Aug 8, 2019
82dd844
More bug fixes and and first iteration of row selection and column se…
jloleysens Aug 8, 2019
d01303e
Minor refactors, cleaned up logging. Still has nasty UX bug on Popove…
jloleysens Aug 8, 2019
6d7e3f2
Added translations and empty tests
jloleysens Aug 8, 2019
58e5bf8
Tests are WiP
jloleysens Aug 8, 2019
50f531d
Move checkboxes out of popover
jloleysens Aug 9, 2019
08ed087
Implemented first iteration of select all functionality
jloleysens Aug 9, 2019
674048b
Move row select all into checkbox area
jloleysens Aug 9, 2019
74458f8
Remove unused code and simplify select all if-else
jloleysens Aug 9, 2019
0427cd9
First iteration of tests
jloleysens Aug 9, 2019
120de8e
fix tests and update name data-test-subj name
jloleysens Aug 9, 2019
1394f75
Merge branch 'master' of github.com:elastic/kibana into feature/rollu…
jloleysens Aug 9, 2019
2c3617d
Merge branch 'master' of github.com:elastic/kibana into feature/rollu…
jloleysens Aug 12, 2019
139559f
Move checkboxes into popover with button
jloleysens Aug 13, 2019
ba67cf4
- Fix tests
jloleysens Aug 13, 2019
a2a8661
Remove extra newline
jloleysens Aug 13, 2019
c428a8a
Merge branch 'master' of github.com:elastic/kibana into feature/rollu…
jloleysens Aug 13, 2019
0505101
- Moved constants from metrics to own file so that it's more re-usable
jloleysens Aug 14, 2019
0e96e99
Merge branch 'master' of github.com:elastic/kibana into feature/rollu…
jloleysens Aug 15, 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
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const initTestBed = registerTestBed(JobCreate, { store: rollupJobsStore });

export const setup = (props) => {
const testBed = initTestBed(props);
const { component, form } = testBed;
const { component, form, table } = testBed;

// User actions
const clickNextStep = () => {
Expand Down Expand Up @@ -68,6 +68,30 @@ export const setup = (props) => {
}
};

// Helpers for the metrics step in job creation.
// Mostly placed here for now to make test file a bit smaller and specific
// to tests.
const getFieldListTableRows = () => {
const { rows } = table.getMetaData('rollupJobMetricsFieldList');
return rows;
};

const getFieldListTableRow = (row) => {
const rows = getFieldListTableRows();
return rows[row];
};

const getFieldChooserColumnForRow = (row) => {
const selectedRow = getFieldListTableRow(row);
const [,, fieldChooserColumn] = selectedRow.columns;
return fieldChooserColumn;
};

const getSelectAllInputForRow = (row) => {
const fieldChooser = getFieldChooserColumnForRow(row);
return fieldChooser.reactWrapper.find('input').first();
};

// Misc
const getEuiStepsHorizontalActive = () => component.find('.euiStepHorizontal-isSelected').text();

Expand All @@ -84,5 +108,11 @@ export const setup = (props) => {
...testBed.form,
fillFormFields,
},
metrics: {
getFieldListTableRows,
getFieldListTableRow,
getFieldChooserColumnForRow,
getSelectAllInputForRow,
}
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ describe('Create Rollup Job, step 5: Metrics', () => {
let getEuiStepsHorizontalActive;
let goToStep;
let table;
let metrics;

beforeAll(() => {
({ server, httpRequestsMockHelpers } = setupEnvironment());
Expand All @@ -56,6 +57,7 @@ describe('Create Rollup Job, step 5: Metrics', () => {
getEuiStepsHorizontalActive,
goToStep,
table,
metrics,
} = setup());
});

Expand Down Expand Up @@ -175,6 +177,9 @@ describe('Create Rollup Job, step 5: Metrics', () => {
}
};

const numericTypeMetrics = ['avg', 'max', 'min', 'sum', 'value_count'];
const dateTypeMetrics = ['max', 'min', 'value_count'];

it('should have an empty field list', async () => {
await goToStep(5);

Expand All @@ -189,7 +194,6 @@ describe('Create Rollup Job, step 5: Metrics', () => {
});

it('should have "avg", "max", "min", "sum" & "value count" metrics for *numeric* fields', () => {
const numericTypeMetrics = ['avg', 'max', 'min', 'sum', 'value_count'];
addFieldToList('numeric');
numericTypeMetrics.forEach(type => {
try {
Expand All @@ -203,11 +207,10 @@ describe('Create Rollup Job, step 5: Metrics', () => {
const { rows: [firstRow] } = table.getMetaData('rollupJobMetricsFieldList');
const columnWithMetricsCheckboxes = 2;
const metricsCheckboxes = firstRow.columns[columnWithMetricsCheckboxes].reactWrapper.find('input');
expect(metricsCheckboxes.length).toBe(numericTypeMetrics.length);
expect(metricsCheckboxes.length).toBe(numericTypeMetrics.length + 1 /* add one for select all */);
});

it('should have "max", "min", & "value count" metrics for *date* fields', () => {
const dateTypeMetrics = ['max', 'min', 'value_count'];
addFieldToList('date');

dateTypeMetrics.forEach(type => {
Expand All @@ -222,7 +225,7 @@ describe('Create Rollup Job, step 5: Metrics', () => {
const { rows: [firstRow] } = table.getMetaData('rollupJobMetricsFieldList');
const columnWithMetricsCheckboxes = 2;
const metricsCheckboxes = firstRow.columns[columnWithMetricsCheckboxes].reactWrapper.find('input');
expect(metricsCheckboxes.length).toBe(dateTypeMetrics.length);
expect(metricsCheckboxes.length).toBe(dateTypeMetrics.length + 1 /* add one for select all */);
});

it('should not allow to go to the next step if at least one metric type is not selected', () => {
Expand All @@ -247,12 +250,157 @@ describe('Create Rollup Job, step 5: Metrics', () => {

const columnsFirstRow = fieldListRows[0].columns;
// The last column is the eui "actions" column
const deleteButton = columnsFirstRow[columnsFirstRow.length - 1].reactWrapper.find('button');
const deleteButton = columnsFirstRow[columnsFirstRow.length - 1].reactWrapper.find('button').last();
deleteButton.simulate('click');

({ rows: fieldListRows } = table.getMetaData('rollupJobMetricsFieldList'));
expect(fieldListRows[0].columns[0].value).toEqual('No metrics fields added');
});
});

describe('when using multi-selectors', () => {
let getSelectAllInputForRow;
let getFieldChooserColumnForRow;
let getFieldListTableRows;

beforeEach(async () => {
httpRequestsMockHelpers.setIndexPatternValidityResponse({ numericFields, dateFields });
await goToStep(5);
await addFieldToList('numeric');
await addFieldToList('date');
find('rollupJobSelectAllMetricsPopoverButton').simulate('click');
({ getSelectAllInputForRow, getFieldChooserColumnForRow, getFieldListTableRows } = metrics);
});

const expectAllFieldChooserInputs = (fieldChooserColumn, expected) => {
const inputs = fieldChooserColumn.reactWrapper.find('input');
inputs.forEach((input) => {
expect(input.props().checked).toBe(expected);
});
};

it('should select all of the fields in a row', async () => {
// The last column is the eui "actions" column
const selectAllCheckbox = getSelectAllInputForRow(0);
selectAllCheckbox.simulate('change', { checked: true });
const fieldChooserColumn = getFieldChooserColumnForRow(0);
expectAllFieldChooserInputs(fieldChooserColumn, true);
});

it('should deselect all of the fields in a row ', async () => {
const selectAllCheckbox = getSelectAllInputForRow(0);
selectAllCheckbox.simulate('change', { checked: true });

let fieldChooserColumn = getFieldChooserColumnForRow(0);
expectAllFieldChooserInputs(fieldChooserColumn, true);

selectAllCheckbox.simulate('change', { checked: false });
fieldChooserColumn = getFieldChooserColumnForRow(0);
expectAllFieldChooserInputs(fieldChooserColumn, false);
});

it('should select all of the metric types across rows (column-wise)', () => {
const selectAllAvgCheckbox = find('rollupJobMetricsSelectAllCheckbox-avg');
selectAllAvgCheckbox.first().simulate('change', { checked: true });

const rows = getFieldListTableRows();

rows
.filter((row) => {
const [, metricTypeCol ] = row.columns;
return metricTypeCol.value === 'numeric';
})
.forEach((row, idx) => {
const fieldChooser = getFieldChooserColumnForRow(idx);
fieldChooser.reactWrapper.find('input').forEach(input => {
const props = input.props();
if (props['data-test-subj'].endsWith('avg')) {
expect(props.checked).toBe(true);
} else {
expect(props.checked).toBe(false);
}
});
});
});

it('should deselect all of the metric types across rows (column-wise)', () => {
const selectAllAvgCheckbox = find('rollupJobMetricsSelectAllCheckbox-avg');
selectAllAvgCheckbox.last().simulate('change', { checked: true });
selectAllAvgCheckbox.last().simulate('change', { checked: false });
Copy link
Contributor

Choose a reason for hiding this comment

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

can you explain why both of these lines are needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do!


const rows = getFieldListTableRows();

rows.forEach((row, idx) => {
const [, metricTypeCol ] = row.columns;
if (metricTypeCol.value !== 'numeric') {
return;
}
const fieldChooser = getFieldChooserColumnForRow(idx);
fieldChooser.reactWrapper.find('input').forEach(input => {
expect(input.props().checked).toBe(false);
});
});
});

it('should correctly select across rows and columns', async () => {
/**
* Tricky test case where we want to determine that the column-wise and row-wise
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for adding these comments!

* selections are interacting correctly with each other.
*
* We will select avg (numeric) and max (numeric + date) to ensure that we are
* testing across metrics types too. The plan is:
*
* 1. Select all avg column-wise
* 2. Select all max column-wise
* 3. Select and deselect row-wise the first numeric metric row
* 4. Expect the avg and max select all checkboxes to be unchecked
* 5. Select all on the last date metric row-wise
* 6. Select then deselect all max column-wise
* 7. Expect everything but all and max to be selected on the last date metric row
*
* Let's a go!
*/

// 1.
find('rollupJobMetricsSelectAllCheckbox-avg').first().simulate('change', { checked: true });
// 2.
find('rollupJobMetricsSelectAllCheckbox-max').first().simulate('change', { checked: true });

const selectAllCheckbox = getSelectAllInputForRow(0);

// 3.
// Select All
selectAllCheckbox.simulate('change', { checked: true });
// Deselect All
selectAllCheckbox.simulate('change', { checked: false });
Copy link
Contributor

Choose a reason for hiding this comment

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

similar question as above - are these two steps needed? by default, will the checkboxes not be unselected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case we want to see that selecting some checkboxes column wise, then selecting the row-wise and then de-selecting them row-wise doesn't leave us in a weird state where some checkboxes (esp. All) are still selected for that row. Will add comment though :).


// 4.
expect(find('rollupJobMetricsSelectAllCheckbox-avg').first().props().checked).toBe(false);
expect(find('rollupJobMetricsSelectAllCheckbox-max').first().props().checked).toBe(false);

let rows = getFieldListTableRows();
// 5.
getSelectAllInputForRow(rows.length - 1).simulate('change', { checked: true });

// 6.
find('rollupJobMetricsSelectAllCheckbox-max').first().simulate('change', { checked: true });
find('rollupJobMetricsSelectAllCheckbox-max').first().simulate('change', { checked: false });

rows = getFieldListTableRows();
const lastRowFieldChooserColumn = getFieldChooserColumnForRow(rows.length - 1);
// 7.
lastRowFieldChooserColumn.reactWrapper.find('input').forEach((input) => {
const props = input.props();
if (
props['data-test-subj'].endsWith('max') ||
props['data-test-subj'].endsWith('All')
) {
expect(props.checked).toBe(false);
} else {
expect(props.checked).toBe(true);
}
});
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export const FieldList = ({
type: 'icon',
color: 'danger',
onClick: (field) => onRemoveField(field),
}]
}],
});
} else {
extendedColumns = columns;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ export class FieldChooser extends Component {
fields: PropTypes.array.isRequired,
selectedFields: PropTypes.array.isRequired,
onSelectField: PropTypes.func.isRequired,
columns: PropTypes.array.isRequired,
prompt: PropTypes.string,
dataTestSubj: PropTypes.string,
}
Expand Down Expand Up @@ -136,7 +135,6 @@ export class FieldChooser extends Component {
</EuiFlyout>
);
};

return (
<Fragment>
<EuiButton
Expand Down
Loading