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

[explore] Modal to edit chart properties #9051

Merged
merged 5 commits into from
Feb 4, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions superset/assets/src/explore/actions/exploreActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -145,3 +145,8 @@ export function createNewSlice(
form_data,
};
}

export const SLICE_UPDATED = 'SLICE_UPDATED';
export function sliceUpdated(slice) {
return { type: SLICE_UPDATED, slice };
}
27 changes: 24 additions & 3 deletions superset/assets/src/explore/components/DisplayQueryButton.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import ModalTrigger from './../../components/ModalTrigger';
import Button from '../../components/Button';
import RowCountLabel from './RowCountLabel';
import { prepareCopyToClipboardTabularData } from '../../utils/common';
import PropertiesModal from './PropertiesModal';

registerLanguage('markdown', markdownSyntax);
registerLanguage('html', htmlSyntax);
Expand All @@ -58,6 +59,7 @@ const propTypes = {
queryResponse: PropTypes.object,
chartStatus: PropTypes.string,
latestQueryFormData: PropTypes.object.isRequired,
slice: PropTypes.object,
};
const defaultProps = {
animation: true,
Expand All @@ -75,9 +77,12 @@ export default class DisplayQueryButton extends React.PureComponent {
error: null,
filterText: '',
sqlSupported: datasource && datasource.split('__')[1] === 'table',
isPropertiesModalOpen: false,
};
this.beforeOpen = this.beforeOpen.bind(this);
this.changeFilterText = this.changeFilterText.bind(this);
this.openPropertiesModal = this.openPropertiesModal.bind(this);
this.closePropertiesModal = this.closePropertiesModal.bind(this);
}
beforeOpen(endpointType) {
this.setState({ isLoading: true });
Expand Down Expand Up @@ -113,6 +118,12 @@ export default class DisplayQueryButton extends React.PureComponent {
redirectSQLLab() {
this.props.onOpenInEditor(this.props.latestQueryFormData);
}
openPropertiesModal() {
this.setState({ isPropertiesModalOpen: true });
}
closePropertiesModal() {
this.setState({ isPropertiesModalOpen: false });
}
renderQueryModalBody() {
if (this.state.isLoading) {
return <Loading />;
Expand Down Expand Up @@ -222,6 +233,19 @@ export default class DisplayQueryButton extends React.PureComponent {
pullRight
id="query"
>
{this.props.slice && (
<>
<MenuItem onClick={this.openPropertiesModal}>
{t('Edit properties')}
</MenuItem>
<PropertiesModal
slice={this.props.slice}
show={this.state.isPropertiesModalOpen}
onHide={this.closePropertiesModal}
animation={this.props.animation}
/>
</>
)}
<ModalTrigger
isMenuItem
animation={this.props.animation}
Expand All @@ -230,7 +254,6 @@ export default class DisplayQueryButton extends React.PureComponent {
bsSize="large"
beforeOpen={() => this.beforeOpen('query')}
modalBody={this.renderQueryModalBody()}
eventKey="1"
/>
<ModalTrigger
isMenuItem
Expand All @@ -240,7 +263,6 @@ export default class DisplayQueryButton extends React.PureComponent {
bsSize="large"
beforeOpen={() => this.beforeOpen('results')}
modalBody={this.renderResultsModalBody()}
eventKey="2"
/>
<ModalTrigger
isMenuItem
Expand All @@ -250,7 +272,6 @@ export default class DisplayQueryButton extends React.PureComponent {
bsSize="large"
beforeOpen={() => this.beforeOpen('samples')}
modalBody={this.renderSamplesModalBody()}
eventKey="2"
/>
{this.state.sqlSupported && (
<MenuItem eventKey="3" onClick={this.redirectSQLLab.bind(this)}>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ const propTypes = {
chartStatus: PropTypes.string,
latestQueryFormData: PropTypes.object,
queryResponse: PropTypes.object,
slice: PropTypes.object,
};

export default function ExploreActionButtons({
Expand All @@ -41,6 +42,7 @@ export default function ExploreActionButtons({
chartStatus,
latestQueryFormData,
queryResponse,
slice,
}) {
const exportToCSVClasses = cx('btn btn-default btn-sm', {
'disabled disabledButton': !canDownload,
Expand Down Expand Up @@ -89,6 +91,7 @@ export default function ExploreActionButtons({
latestQueryFormData={latestQueryFormData}
chartStatus={chartStatus}
onOpenInEditor={actions.redirectSQLLab}
slice={slice}
/>
</div>
);
Expand Down
239 changes: 239 additions & 0 deletions superset/assets/src/explore/components/PropertiesModal.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,239 @@
/**
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
import React, { useState, useEffect, useRef } from 'react';
import { connect } from 'react-redux';
import { bindActionCreators } from 'redux';
import {
Button,
Modal,
Row,
Col,
FormControl,
FormGroup,
} from 'react-bootstrap';
import Dialog from 'react-bootstrap-dialog';
import Select from 'react-select';
import { t } from '@superset-ui/translation';
import { SupersetClient } from '@superset-ui/connection';

import { sliceUpdated } from '../actions/exploreActions';
import getClientErrorObject from '../../utils/getClientErrorObject';

function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
// The wrapper is a separate component so that hooks only run when the modal opens
return (
<Modal show={show} onHide={onHide} animation={animation} bsSize="large">
<PropertiesModal slice={slice} onHide={onHide} onSave={onSave} />
</Modal>
);
}

function PropertiesModal({ slice, onHide, onSave }) {
const [submitting, setSubmitting] = useState(false);
const errorDialog = useRef();
const [ownerOptions, setOwnerOptions] = useState(null);

// values of form inputs
const [name, setName] = useState(slice.slice_name || '');
const [description, setDescription] = useState(slice.description || '');
const [cacheTimeout, setCacheTimeout] = useState(
slice.cache_timeout != null ? slice.cache_timeout : '',
);
const [owners, setOwners] = useState(null);

function showError({ error, statusText }) {
errorDialog.current.show({
title: 'Error',
bsSize: 'medium',
bsStyle: 'danger',
actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
body: error || statusText || t('An error has occurred'),
});
}

async function fetchOwners() {
try {
const response = await SupersetClient.get({
endpoint: `/api/v1/chart/${slice.slice_id}`,
});
setOwners(
response.json.result.owners.map(owner => ({
value: owner.id,
label: owner.username,
})),
);
} catch (response) {
const clientError = await getClientErrorObject(response);
showError(clientError);
}
}

// get the owners of this slice
useEffect(() => {
fetchOwners();
}, []);

// get the list of users who can own a chart
useEffect(() => {
SupersetClient.get({
endpoint: `/api/v1/chart/related/owners`,
}).then(res => {
setOwnerOptions(
res.json.result.map(item => ({
value: item.value,
label: item.text,
})),
);
});
}, []);

const onSubmit = async event => {
event.stopPropagation();
event.preventDefault();
suddjian marked this conversation as resolved.
Show resolved Hide resolved
setSubmitting(true);
const payload = {
slice_name: name || null,
description: description || null,
cache_timeout: cacheTimeout || null,
suddjian marked this conversation as resolved.
Show resolved Hide resolved
owners: owners.map(o => o.value),
};
try {
const res = await SupersetClient.put({
Copy link
Member

Choose a reason for hiding this comment

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

missed this before, please use complete words for variable names

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 Thanks for the review!

endpoint: `/api/v1/chart/${slice.slice_id}`,
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(payload),
});
// update the redux state
onSave(res.json.result);
onHide();
} catch (res) {
const clientError = await getClientErrorObject(res);
showError(clientError);
}
setSubmitting(false);
};

return (
<form onSubmit={onSubmit}>
<Modal.Header closeButton>
<Modal.Title>Edit Chart Properties</Modal.Title>
</Modal.Header>
<Modal.Body>
<Row>
<Col md={6}>
<h3>{t('Basic Information')}</h3>
<FormGroup>
<label className="control-label" htmlFor="name">
{t('Name')}
</label>
<FormControl
name="name"
type="text"
bsSize="sm"
value={name}
onChange={event => setName(event.target.value)}
/>
</FormGroup>
<FormGroup>
<label className="control-label" htmlFor="description">
{t('Description')}
</label>
<FormControl
name="description"
type="text"
componentClass="textarea"
bsSize="sm"
value={description}
onChange={event => setDescription(event.target.value)}
style={{ maxWidth: '100%' }}
Copy link
Member

Choose a reason for hiding this comment

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

Inline styles!? Not sure the exact issue this addresses, but we can probably just slap this on all FormControl components via the theme if needed, so it needn't be repeated in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps. If you think it's worth messing around with global styles for this specific thing I'll do it. Without that line, you can grab+drag the textarea out into the right column of the modal layout.

/>
<p className="help-block">
{t(
'The description can be displayed as widget headers in the dashboard view. Supports markdown.',
)}
</p>
</FormGroup>
</Col>
<Col md={6}>
<h3>{t('Configuration')}</h3>
<FormGroup>
<label className="control-label" htmlFor="cacheTimeout">
{t('Cache Timeout')}
</label>
<FormControl
name="cacheTimeout"
type="text"
bsSize="sm"
value={cacheTimeout}
onChange={event =>
setCacheTimeout(event.target.value.replace(/[^0-9]/, ''))
Copy link
Member

Choose a reason for hiding this comment

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

I think if you made the FormControl's input type "number" then you could get rid of this logic

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I did at first, but I wasn't sure if we wanted to allow non-integers. Do you have any insight into whether that would be ok?

Copy link
Member Author

Choose a reason for hiding this comment

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

The marshmallow schema on the endpoint wants an integer

Copy link
Member

Choose a reason for hiding this comment

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

I suppose that when sending the request, you could do a parseInt on the value to remove the decimal. Maybe that's sufficient then? At least we don't need to run the validator onChange then

Copy link
Member Author

Choose a reason for hiding this comment

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

parseInt can return NaN for certain inputs. An input of type="number" allows using "e" and decimal points. I'd prefer not to open the NaN door, personally.

Copy link
Member Author

@suddjian suddjian Jan 31, 2020

Choose a reason for hiding this comment

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

We could use type="number alongside the regex filter to get the extra little browser widgets that come with number inputs if you think that'd be valuable, but I think the regex does a job here that isn't offered by html.

}
/>
<p className="help-block">
{t(
'Duration (in seconds) of the caching timeout for this chart. Note this defaults to the datasource/table timeout if undefined.',
)}
</p>
</FormGroup>
<h3 style={{ marginTop: '1em' }}>{t('Access')}</h3>
<FormGroup>
<label className="control-label" htmlFor="owners">
{t('Owners')}
</label>
<Select
name="owners"
multi
isLoading={!ownerOptions}
value={owners}
options={ownerOptions || []}
onChange={setOwners}
disabled={!owners || !ownerOptions}
/>
<p className="help-block">
{t('A list of users who can alter the chart')}
</p>
</FormGroup>
</Col>
</Row>
</Modal.Body>
<Modal.Footer>
<Button
type="submit"
bsSize="sm"
bsStyle="primary"
className="m-r-5"
disabled={submitting}
>
{t('Save')}
</Button>
<Button type="button" bsSize="sm" onClick={onHide}>
{t('Cancel')}
</Button>
<Dialog ref={errorDialog} />
</Modal.Footer>
</form>
);
}

function mapDispatchToProps(dispatch) {
return bindActionCreators({ onSave: sliceUpdated }, dispatch);
}

export { PropertiesModalWrapper };
export default connect(null, mapDispatchToProps)(PropertiesModalWrapper);
9 changes: 9 additions & 0 deletions superset/assets/src/explore/reducers/exploreReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,15 @@ export default function exploreReducer(state = {}, action) {
can_overwrite: action.can_overwrite,
};
},
[actions.SLICE_UPDATED]() {
return {
...state,
slice: {
...state.slice,
...action.slice,
},
};
},
};
if (action.type in actionHandlers) {
return actionHandlers[action.type]();
Expand Down
1 change: 1 addition & 0 deletions superset/models/slice.py
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ def data(self) -> Dict[str, Any]:
logging.exception(e)
d["error"] = str(e)
return {
"cache_timeout": self.cache_timeout,
"datasource": self.datasource_name,
"description": self.description,
"description_markeddown": self.description_markeddown,
Expand Down
Loading