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

Jump to bottom

feat(explore-popover): Show disabled 'Save' button in explore popover #21318

Merged
merged 7 commits into from
Dec 7, 2022
Merged

feat(explore-popover): Show disabled 'Save' button in explore popover #21318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8d24fd3
Show disabled 'Save' button in explore popover when there are no unsa…
Sep 3, 2022
3b9b7d5
Update popover components so that they show a disabled primary button…
Sep 30, 2022
73324b8
Fix associated tests for popover components
Oct 4, 2022
ec21987
Re-run CI
Oct 4, 2022
95cf2e4
Fix Jest tests
Oct 6, 2022
c762fdc
Improvements to tests
Oct 15, 2022
2962bde
Addresses comments
michael-s-molina Dec 6, 2022
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
6 changes: 2 additions & 4 deletions ...t-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,10 +406,8 @@ const ColumnSelectPopover = ({
{t('Close')}
</Button>
<Button
disabled={!stateIsValid}
buttonStyle={
hasUnsavedChanges && stateIsValid ? 'primary' : 'default'
}
disabled={!stateIsValid || !hasUnsavedChanges}
buttonStyle="primary"
buttonSize="small"
onClick={onSave}
data-test="ColumnEdit#save"
Expand Down
3 changes: 1 addition & 2 deletions .../components/controls/FilterControl/AdhocFilterEditPopover/AdhocFilterEditPopover.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ describe('AdhocFilterEditPopover', () => {

it('prevents saving if the filter is invalid', () => {
const { wrapper } = setup();
expect(wrapper.find(Button).find({ disabled: true })).not.toExist();
expect(wrapper.find(Button).find({ disabled: true })).toExist();
wrapper
.instance()
.onAdhocFilterChange(simpleAdhocFilter.duplicateWith({ operator: null }));
Expand All @@ -133,7 +133,6 @@ describe('AdhocFilterEditPopover', () => {

it('highlights save if changes are present', () => {
const { wrapper } = setup();
expect(wrapper.find(Button).find({ buttonStyle: 'primary' })).not.toExist();
wrapper.instance().onAdhocFilterChange(sqlAdhocFilter);
expect(wrapper.find(Button).find({ buttonStyle: 'primary' })).toExist();
});
Expand Down
8 changes: 5 additions & 3 deletions ...t-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopover/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,10 +245,12 @@ export default class AdhocFilterEditPopover extends React.Component {
</Button>
<Button
data-test="adhoc-filter-edit-popover-save-button"
disabled={!stateIsValid || !this.state.isSimpleTabValid}
buttonStyle={
hasUnsavedChanges && stateIsValid ? 'primary' : 'default'
disabled={
!stateIsValid ||
!this.state.isSimpleTabValid ||
!hasUnsavedChanges
}
buttonStyle="primary"
buttonSize="small"
className="m-r-5"
onClick={this.onSave}
Expand Down
6 changes: 3 additions & 3 deletions .../components/controls/MetricControl/AdhocMetricEditPopover/AdhocMetricEditPopover.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ describe('AdhocMetricEditPopover', () => {

it('prevents saving if no column or aggregate is chosen', () => {
const { wrapper } = setup();
expect(wrapper.find(Button).find({ disabled: true })).not.toExist();
expect(wrapper.find(Button).find({ disabled: false })).not.toExist();
wrapper.instance().onColumnChange(null);
expect(wrapper.find(Button).find({ disabled: true })).toExist();
wrapper.instance().onColumnChange(columns[0].column_name);
Expand All @@ -109,9 +109,9 @@ describe('AdhocMetricEditPopover', () => {

it('highlights save if changes are present', () => {
const { wrapper } = setup();
expect(wrapper.find(Button).find({ buttonStyle: 'primary' })).not.toExist();
expect(wrapper.find(Button).find({ disabled: true })).toExist();
wrapper.instance().onColumnChange(columns[1].column_name);
expect(wrapper.find(Button).find({ buttonStyle: 'primary' })).toExist();
expect(wrapper.find(Button).find({ disabled: true })).not.toExist();
});

it('will initiate a drag when clicked', () => {
Expand Down
42 changes: 27 additions & 15 deletions .../components/controls/MetricControl/AdhocMetricEditPopover/AdhocMetricEditPopover.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
*/
import userEvent from '@testing-library/user-event';
import React from 'react';
import { render, screen } from 'spec/helpers/testing-library';
import { render, screen, waitFor, within } from 'spec/helpers/testing-library';
import AdhocMetric from 'src/explore/components/controls/MetricControl/AdhocMetric';
import AdhocMetricEditPopover from '.';

Expand All @@ -35,10 +35,15 @@ const createProps = () => ({
},
savedMetricsOptions: [
{
id: 65,
id: 64,
metric_name: 'count',
expression: 'COUNT(*)',
},
{
id: 65,
metric_name: 'sum',
expression: 'sum(num)',
},
],
adhocMetric: new AdhocMetric({ isNew: true }),
datasource: {
Expand Down Expand Up @@ -118,28 +123,35 @@ test('Clicking on "Close" should call onClose', () => {
expect(props.onClose).toBeCalledTimes(1);
});

test('Clicking on "Save" should call onChange and onClose', () => {
test('Clicking on "Save" should call onChange and onClose', async () => {
const props = createProps();
render(<AdhocMetricEditPopover {...props} />);
expect(props.onChange).toBeCalledTimes(0);
expect(props.onClose).toBeCalledTimes(0);
userEvent.click(
screen.getByRole('combobox', {
name: 'Select saved metrics',
}),
);
const sumOption = await waitFor(() =>
within(document.querySelector('.rc-virtual-list')!).getByText('sum'),
);
userEvent.click(sumOption);
userEvent.click(screen.getByRole('button', { name: 'Save' }));
expect(props.onChange).toBeCalledTimes(1);
expect(props.onChange).toBeCalledWith(
{
id: 64,
metric_name: 'count',
expression: 'COUNT(*)',
},
{
id: 64,
metric_name: 'count',
expression: 'COUNT(*)',
},
);
expect(props.onClose).toBeCalledTimes(1);
});

test('Clicking on "Save" should not call onChange and onClose', () => {
Copy link
Member

Choose a reason for hiding this comment

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

I believe we need to separate the tests here for both calling and not calling

const props = createProps();
render(<AdhocMetricEditPopover {...props} />);
expect(props.onChange).toBeCalledTimes(0);
expect(props.onClose).toBeCalledTimes(0);
userEvent.click(screen.getByRole('button', { name: 'Save' }));
expect(props.onChange).toBeCalledTimes(0);
expect(props.onClose).toBeCalledTimes(0);
});

test('Should switch to tab:Simple', () => {
const props = createProps();
props.getCurrentTab.mockImplementation(tab => {
Expand Down
6 changes: 2 additions & 4 deletions ...t-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,10 +487,8 @@ export default class AdhocMetricEditPopover extends React.PureComponent {
{t('Close')}
</Button>
<Button
disabled={!stateIsValid}
buttonStyle={
hasUnsavedChanges && stateIsValid ? 'primary' : 'default'
}
disabled={!stateIsValid || !hasUnsavedChanges}
buttonStyle="primary"
buttonSize="small"
data-test="AdhocMetricEdit#save"
onClick={this.onSave}
Expand Down