diff --git a/superset-frontend/cypress-base/cypress/integration/explore/link.test.js b/superset-frontend/cypress-base/cypress/integration/explore/link.test.js index 1d0ce8791..bbbdfaab0 100644 --- a/superset-frontend/cypress-base/cypress/integration/explore/link.test.js +++ b/superset-frontend/cypress-base/cypress/integration/explore/link.test.js @@ -20,8 +20,14 @@ // Tests for links in the explore UI // *********************************************** +import rison from 'rison'; +import shortid from 'shortid'; import { HEALTH_POP_FORM_DATA_DEFAULTS } from './visualizations/shared.helper'; +const apiURL = (endpoint, queryObject) => { + return `${endpoint}?q=${rison.encode(queryObject)}`; +}; + describe('Test explore links', () => { beforeEach(() => { cy.login(); @@ -73,98 +79,120 @@ describe('Test explore links', () => { }); }); - xit('Test chart save as', () => { + it('Test chart save as AND overwrite', () => { const formData = { ...HEALTH_POP_FORM_DATA_DEFAULTS, viz_type: 'table', metrics: ['sum__SP_POP_TOTL'], groupby: ['country_name'], }; - const newChartName = 'Test chart'; + const newChartName = `Test chart [${shortid.generate()}]`; cy.visitChartByParams(JSON.stringify(formData)); cy.verifySliceSuccess({ waitAlias: '@postJson' }); cy.url().then(url => { cy.get('button[data-target="#save_modal"]').click(); cy.get('.modal-content').within(() => { + cy.get('#saveas-radio').check(); cy.get('input[name=new_slice_name]').type(newChartName); cy.get('button#btn_modal_save').click(); }); - cy.url().should('eq', url); - - cy.visitChartByName(newChartName); cy.verifySliceSuccess({ waitAlias: '@postJson' }); - }); - }); + cy.visitChartByName(newChartName); - xit('Test chart save', () => { - const chartName = 'Test chart'; - cy.visitChartByName(chartName); - cy.verifySliceSuccess({ waitAlias: '@postJson' }); - - cy.get('[data-test=groupby]').within(() => { - cy.get('.Select__clear-indicator').click(); + // Overwriting! + cy.get('button[data-target="#save_modal"]').click(); + cy.get('.modal-content').within(() => { + cy.get('#overwrite-radio').check(); + cy.get('button#btn_modal_save').click(); + }); + cy.verifySliceSuccess({ waitAlias: '@postJson' }); + const query = { + filters: [ + { + col: 'slice_name', + opr: 'eq', + value: newChartName, + }, + ], + }; + cy.request(apiURL('/api/v1/chart/', query)).then(response => { + expect(response.body.count).equals(1); + cy.request('DELETE', `/api/v1/chart/${response.body.ids[0]}`); + }); }); - cy.get('button[data-target="#save_modal"]').click(); - cy.get('.modal-content').within(() => { - cy.get('button#btn_modal_save').click(); - }); - cy.verifySliceSuccess({ waitAlias: '@postJson' }); - cy.request(`/chart/api/read?_flt_3_slice_name=${chartName}`).then( - response => { - cy.request('DELETE', `/chart/api/delete/${response.body.pks[0]}`); - }, - ); }); it('Test chart save as and add to new dashboard', () => { - cy.visitChartByName('Growth Rate'); - cy.verifySliceSuccess({ waitAlias: '@postJson' }); + const chartName = 'Growth Rate'; + const newChartName = `${chartName} [${shortid.generate()}]`; + const dashboardTitle = `Test dashboard [${shortid.generate()}]`; - const dashboardTitle = 'Test dashboard'; - cy.get('button[data-target="#save_modal"]').click(); - cy.get('.modal-content').within(() => { - cy.get('input[name=new_slice_name]').type('New Growth Rate'); - cy.get('input[data-test=add-to-new-dashboard]').check(); - cy.get('input[placeholder="[dashboard name]"]').type(dashboardTitle); - cy.get('button#btn_modal_save').click(); - }); + cy.visitChartByName(chartName); cy.verifySliceSuccess({ waitAlias: '@postJson' }); - cy.request( - `/dashboard/api/read?_flt_3_dashboard_title=${dashboardTitle}`, - ).then(response => { - expect(response.body.pks[0]).not.equals(null); - }); - }); - - it('Test chart save as and add to existing dashboard', () => { - cy.visitChartByName('Most Populated Countries'); - cy.verifySliceSuccess({ waitAlias: '@postJson' }); - const chartName = 'New Most Populated Countries'; - const dashboardTitle = 'Test dashboard'; cy.get('button[data-target="#save_modal"]').click(); cy.get('.modal-content').within(() => { - cy.get('input[name=new_slice_name]').type(chartName); - cy.get('input[data-test=add-to-existing-dashboard]').check(); - cy.get('.save-modal-selector') - .click() - .within(() => { - cy.get('input').type(dashboardTitle); - cy.get('.Select__option--is-focused').trigger('mousedown'); - }); + cy.get('#saveas-radio').check(); + cy.get('input[name=new_slice_name]').click().clear().type(newChartName); + // Add a new option using the "CreatableSelect" feature + cy.get('#dashboard-creatable-select').type( + `${dashboardTitle}{enter}{enter}`, + ); cy.get('button#btn_modal_save').click(); }); cy.verifySliceSuccess({ waitAlias: '@postJson' }); - cy.request(`/chart/api/read?_flt_3_slice_name=${chartName}`).then( - response => { - cy.request('DELETE', `/chart/api/delete/${response.body.pks[0]}`); - }, - ); - cy.request( - `/dashboard/api/read?_flt_3_dashboard_title=${dashboardTitle}`, - ).then(response => { - cy.request('DELETE', `/dashboard/api/delete/${response.body.pks[0]}`); + let query = { + filters: [ + { + col: 'dashboard_title', + opr: 'eq', + value: dashboardTitle, + }, + ], + }; + cy.request(apiURL('/api/v1/dashboard/', query)).then(response => { + expect(response.body.count).equals(1); + }); + + cy.visitChartByName(newChartName); + cy.verifySliceSuccess({ waitAlias: '@postJson' }); + + cy.get('button[data-target="#save_modal"]').click(); + cy.get('.modal-content').within(() => { + cy.get('#overwrite-radio').check(); + cy.get('input[name=new_slice_name]').click().clear().type(newChartName); + // This time around, typing the same dashboard name + // will select the existing one + cy.get('#dashboard-creatable-select').type( + `${dashboardTitle}{enter}{enter}`, + ); + cy.get('button#btn_modal_save').click(); + }); + cy.verifySliceSuccess({ waitAlias: '@postJson' }); + query = { + filters: [ + { + col: 'slice_name', + opr: 'eq', + value: chartName, + }, + ], + }; + cy.request(apiURL('/api/v1/chart/', query)).then(response => { + expect(response.body.count).equals(1); + }); + query = { + filters: [ + { + col: 'dashboard_title', + opr: 'eq', + value: dashboardTitle, + }, + ], + }; + cy.request(apiURL('/api/v1/dashboard/', query)).then(response => { + expect(response.body.count).equals(1); }); }); }); diff --git a/superset-frontend/cypress-base/package-lock.json b/superset-frontend/cypress-base/package-lock.json index 25a2aa953..fba8ed672 100644 --- a/superset-frontend/cypress-base/package-lock.json +++ b/superset-frontend/cypress-base/package-lock.json @@ -5005,6 +5005,11 @@ "inherits": "^2.0.1" } }, + "rison": { + "version": "0.1.1", + "resolved": "https://registry.npmjs.org/rison/-/rison-0.1.1.tgz", + "integrity": "sha1-TcwFV7JBr/YOdheOd5ITVxPzMSA=" + }, "run-parallel": { "version": "1.1.9", "resolved": "https://registry.npmjs.org/run-parallel/-/run-parallel-1.1.9.tgz", diff --git a/superset-frontend/cypress-base/package.json b/superset-frontend/cypress-base/package.json index d021910a1..d1058c33a 100644 --- a/superset-frontend/cypress-base/package.json +++ b/superset-frontend/cypress-base/package.json @@ -9,8 +9,9 @@ "author": "Apcahe", "license": "Apache-2.0", "dependencies": { - "shortid": "^2.2.15", - "@cypress/code-coverage": "^3.8.1" + "@cypress/code-coverage": "^3.8.1", + "rison": "^0.1.1", + "shortid": "^2.2.15" }, "devDependencies": { "cypress": "4.9.0", diff --git a/superset-frontend/spec/javascripts/explore/components/SaveModal_spec.jsx b/superset-frontend/spec/javascripts/explore/components/SaveModal_spec.jsx index a315f7255..789e3bf61 100644 --- a/superset-frontend/spec/javascripts/explore/components/SaveModal_spec.jsx +++ b/superset-frontend/spec/javascripts/explore/components/SaveModal_spec.jsx @@ -22,7 +22,7 @@ import thunk from 'redux-thunk'; import { bindActionCreators } from 'redux'; import { shallow, mount } from 'enzyme'; -import { Modal, Button, Radio } from 'react-bootstrap'; +import { FormControl, Modal, Button, Radio } from 'react-bootstrap'; import sinon from 'sinon'; import fetchMock from 'fetch-mock'; @@ -65,7 +65,7 @@ describe('SaveModal', () => { target: { value: 'mock event target', }, - value: 'mock value', + value: 10, }; const getWrapper = () => @@ -73,19 +73,18 @@ describe('SaveModal', () => { context: { store }, }).dive(); - it('renders a Modal with 7 inputs and 2 buttons', () => { + it('renders a Modal with the right set of components', () => { const wrapper = getWrapper(); expect(wrapper.find(Modal)).toHaveLength(1); - expect(wrapper.find('input')).toHaveLength(2); - expect(wrapper.find(Button)).toHaveLength(2); - expect(wrapper.find(Radio)).toHaveLength(5); + expect(wrapper.find(FormControl)).toHaveLength(1); + expect(wrapper.find(Button)).toHaveLength(3); + expect(wrapper.find(Radio)).toHaveLength(2); }); - it('does not show overwrite option for new slice', () => { - const wrapperNewSlice = getWrapper(); - wrapperNewSlice.setProps({ slice: null }); - expect(wrapperNewSlice.find('#overwrite-radio')).toHaveLength(0); - expect(wrapperNewSlice.find('#saveas-radio')).toHaveLength(1); + it('overwrite radio button is disabled for new slice', () => { + const wrapper = getWrapper(); + wrapper.setProps({ slice: null }); + expect(wrapper.find('#overwrite-radio').prop('disabled')).toBe(true); }); it('disable overwrite option for non-owner', () => { @@ -128,14 +127,11 @@ describe('SaveModal', () => { it('onChange', () => { const wrapper = getWrapper(); - wrapper.instance().onChange('newSliceName', mockEvent); + wrapper.instance().onSliceNameChange(mockEvent); expect(wrapper.state().newSliceName).toBe(mockEvent.target.value); - wrapper.instance().onChange('saveToDashboardId', mockEvent); + wrapper.instance().onDashboardSelectChange(mockEvent); expect(wrapper.state().saveToDashboardId).toBe(mockEvent.value); - - wrapper.instance().onChange('newDashboardName', mockEvent); - expect(wrapper.state().newDashboardName).toBe(mockEvent.target.value); }); describe('saveOrOverwrite', () => { @@ -145,7 +141,7 @@ describe('SaveModal', () => { sinon.stub(defaultProps.actions, 'saveSlice').callsFake(() => Promise.resolve({ data: { - dashboard: '/mock_dashboard/', + dashboard_url: 'http://localhost/mock_dashboard/', slice: { slice_url: '/mock_slice/' }, }, }), @@ -168,10 +164,6 @@ describe('SaveModal', () => { const wrapper = getWrapper(); const saveToDashboardId = 100; - wrapper.setState({ addToDash: 'existing' }); - wrapper.instance().saveOrOverwrite(true); - expect(wrapper.state().alert).toBe('Please select a dashboard'); - wrapper.setState({ saveToDashboardId }); wrapper.instance().saveOrOverwrite(true); const args = defaultProps.actions.saveSlice.getCall(0).args; @@ -182,10 +174,6 @@ describe('SaveModal', () => { const wrapper = getWrapper(); const newDashboardName = 'new dashboard name'; - wrapper.setState({ addToDash: 'new' }); - wrapper.instance().saveOrOverwrite(true); - expect(wrapper.state().alert).toBe('Please enter a dashboard name'); - wrapper.setState({ newDashboardName }); wrapper.instance().saveOrOverwrite(true); const args = defaultProps.actions.saveSlice.getCall(0).args; diff --git a/superset-frontend/src/explore/components/SaveModal.jsx b/superset-frontend/src/explore/components/SaveModal.jsx index cff58cc4f..4ac1a79dd 100644 --- a/superset-frontend/src/explore/components/SaveModal.jsx +++ b/superset-frontend/src/explore/components/SaveModal.jsx @@ -20,11 +20,19 @@ import React from 'react'; import PropTypes from 'prop-types'; import { connect } from 'react-redux'; -import { Modal, Alert, Button, Radio } from 'react-bootstrap'; -import Select from 'src/components/Select'; +import { + Alert, + Button, + FormControl, + FormGroup, + ControlLabel, + Modal, + Radio, +} from 'react-bootstrap'; +import { CreatableSelect } from 'src/components/Select/SupersetStyledSelect'; import { t } from '@superset-ui/translation'; +import ReactMarkdown from 'react-markdown'; -import { supersetURL } from '../../utils/common'; import { EXPLORE_ONLY_VIZ_TYPE } from '../constants'; const propTypes = { @@ -39,28 +47,30 @@ const propTypes = { datasource: PropTypes.object, }; +// Session storage key for recent dashboard +const SK_DASHBOARD_ID = 'save_chart_recent_dashboard'; +const SELECT_PLACEHOLDER = t('**Select** a dashboard OR **create** a new one'); + class SaveModal extends React.Component { constructor(props) { super(props); this.state = { saveToDashboardId: null, - newDashboardName: '', newSliceName: props.sliceName, dashboards: [], alert: null, action: props.can_overwrite ? 'overwrite' : 'saveas', - addToDash: 'noSave', vizType: props.form_data.viz_type, }; + this.onDashboardSelectChange = this.onDashboardSelectChange.bind(this); + this.onSliceNameChange = this.onSliceNameChange.bind(this); } componentDidMount() { this.props.actions.fetchDashboards(this.props.userId).then(() => { const dashboardIds = this.props.dashboards.map( dashboard => dashboard.value, ); - let recentDashboard = sessionStorage.getItem( - 'save_chart_recent_dashboard', - ); + let recentDashboard = sessionStorage.getItem(SK_DASHBOARD_ID); recentDashboard = recentDashboard && parseInt(recentDashboard, 10); if ( recentDashboard !== null && @@ -68,33 +78,22 @@ class SaveModal extends React.Component { ) { this.setState({ saveToDashboardId: recentDashboard, - addToDash: 'existing', }); } }); } - onChange(name, event) { - switch (name) { - case 'newSliceName': - this.setState({ newSliceName: event.target.value }); - break; - case 'saveToDashboardId': - this.setState({ saveToDashboardId: event.value }); - this.changeDash('existing'); - break; - case 'newDashboardName': - this.setState({ newDashboardName: event.target.value }); - break; - default: - break; - } + onSliceNameChange(event) { + this.setState({ newSliceName: event.target.value }); + } + onDashboardSelectChange(event) { + const newDashboardName = event ? event.label : null; + const saveToDashboardId = + event && typeof event.value === 'number' ? event.value : null; + this.setState({ saveToDashboardId, newDashboardName }); } changeAction(action) { this.setState({ action }); } - changeDash(dash) { - this.setState({ addToDash: dash }); - } saveOrOverwrite(gotodash) { this.setState({ alert: null }); this.props.actions.removeSaveModalAlert(); @@ -111,49 +110,20 @@ class SaveModal extends React.Component { } sliceParams.action = this.state.action; sliceParams.slice_name = this.state.newSliceName; - - const addToDash = this.state.addToDash; - sliceParams.add_to_dash = addToDash; - let dashboard = null; - switch (addToDash) { - case 'existing': - dashboard = this.state.saveToDashboardId; - if (!dashboard) { - this.setState({ alert: t('Please select a dashboard') }); - return; - } - sliceParams.save_to_dashboard_id = dashboard; - break; - case 'new': - dashboard = this.state.newDashboardName; - if (dashboard === '') { - this.setState({ alert: t('Please enter a dashboard name') }); - return; - } - sliceParams.new_dashboard_name = dashboard; - break; - default: - dashboard = null; - } - sliceParams.goto_dash = gotodash; + sliceParams.save_to_dashboard_id = this.state.saveToDashboardId; + sliceParams.new_dashboard_name = this.state.newDashboardName; this.props.actions .saveSlice(this.props.form_data, sliceParams) .then(({ data }) => { if (data.dashboard_id === null) { - sessionStorage.removeItem('save_chart_recent_dashboard'); + sessionStorage.removeItem(SK_DASHBOARD_ID); } else { - sessionStorage.setItem( - 'save_chart_recent_dashboard', - data.dashboard_id, - ); + sessionStorage.setItem(SK_DASHBOARD_ID, data.dashboard_id); } // Go to new slice url or dashboard url - if (gotodash) { - window.location.assign(supersetURL(data.dashboard)); - } else { - window.location.assign(data.slice.slice_url); - } + const url = gotodash ? data.dashboard_url : data.slice.slice_url; + window.location.assign(url); }); this.props.onHide(); } @@ -167,9 +137,9 @@ class SaveModal extends React.Component { const canNotSaveToDash = EXPLORE_ONLY_VIZ_TYPE.indexOf(this.state.vizType) > -1; return ( - + - {t('Save A Chart')} + {t('Save Chart')} {(this.state.alert || this.props.alert) && ( @@ -184,97 +154,91 @@ class SaveModal extends React.Component { /> )} - {this.props.slice && ( + - {t('Overwrite chart %s', this.props.slice.slice_name)} + {t('Save (Overwrite)')} - )} - - - {' '} - {t('Save as')}   - - - -
+ + {' '} + {t('Save as ...')}   + +
- - - {t('Do not add to a dashboard')} - - - - {t('Add chart to existing dashboard')} - - + + {t('Chart name')} + + + + {t('Add to dashboard')} + + } + /> + - - +
+ + + +
); diff --git a/superset-frontend/src/explore/main.less b/superset-frontend/src/explore/main.less index 1ce6d7ae4..c1ea11683 100644 --- a/superset-frontend/src/explore/main.less +++ b/superset-frontend/src/explore/main.less @@ -112,10 +112,6 @@ } } -.save-modal-selector { - margin: 10px 0; -} - .list-group { margin-bottom: 10px; } diff --git a/superset-frontend/stylesheets/superset.less b/superset-frontend/stylesheets/superset.less index 36b01133f..aa3d4dba8 100644 --- a/superset-frontend/stylesheets/superset.less +++ b/superset-frontend/stylesheets/superset.less @@ -560,3 +560,35 @@ td.filtered { flex-direction: column; justify-content: center; } + +// Making native radio buttons use brand color +input[type='radio']:after { + width: 15px; + height: 15px; + border-radius: 15px; + top: -2px; + left: -1px; + position: relative; + background-color: #fff; + content: ''; + display: inline-block; + visibility: visible; + border: 2px solid @gray; +} + +input[type='radio']:checked:after { + width: 15px; + height: 15px; + border-radius: 15px; + top: -2px; + left: -1px; + position: relative; + background-color: #fff; + content: ''; + display: inline-block; + visibility: visible; + border: 5px solid @brand-primary; +} +hr { + border-top: 1px solid @gray-light; +} diff --git a/superset/views/core.py b/superset/views/core.py index 42c2f9743..fdf989268 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -813,11 +813,14 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods # Adding slice to a dashboard if requested dash: Optional[Dashboard] = None - if request.args.get("add_to_dash") == "existing": + save_to_dashboard_id = request.args.get("save_to_dashboard_id") + new_dashboard_name = request.args.get("new_dashboard_name") + if save_to_dashboard_id: + # Adding the chart to an existing dashboard dash = cast( Dashboard, db.session.query(Dashboard) - .filter_by(id=int(request.args["save_to_dashboard_id"])) + .filter_by(id=int(save_to_dashboard_id)) .one(), ) # check edit dashboard permissions @@ -836,7 +839,8 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods ), "info", ) - elif request.args.get("add_to_dash") == "new": + elif new_dashboard_name: + # Creating and adding to a new dashboard # check create dashboard permissions dash_add_perm = security_manager.can_access("can_add", "DashboardModelView") if not dash_add_perm: @@ -868,7 +872,7 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods "can_overwrite": is_owner(slc, g.user), "form_data": slc.form_data, "slice": slc.data, - "dashboard_id": dash.id if dash else None, + "dashboard_url": dash.url if dash else None, } if dash and request.args.get("goto_dash") == "true":