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

Filter owners select by text input #9337

Merged
merged 12 commits into from
Apr 7, 2020
Merged

Filter owners select by text input #9337

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
fbf2189
filter owners select by text input
suddjian Mar 20, 2020
8ce7ffa
use rison
suddjian Mar 21, 2020
4daa3e2
fix backend owners filter logic
suddjian Mar 27, 2020
bd5600b
use fullname, not username on owners inputs
suddjian Mar 31, 2020
a6dafd9
fix some tests
suddjian Apr 2, 2020
c5e6568
fixing tests
suddjian Apr 3, 2020
73170b8
deterministic tests
suddjian Apr 3, 2020
8beaf8b
appease linter
suddjian Apr 3, 2020
bd2563c
add back search by username
suddjian Apr 6, 2020
7ab9026
more comprehensive filter test
suddjian Apr 6, 2020
0c5ab1b
add clarifying text
suddjian Apr 6, 2020
bc2831b
formatting...
suddjian Apr 7, 2020
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
290 changes: 144 additions & 146 deletions superset-frontend/package-lock.json
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 3 additions & 1 deletion superset-frontend/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,8 @@
"@superset-ui/translation": "^0.12.8",
"@types/classnames": "^2.2.9",
"@types/react-json-tree": "^0.6.11",
"@types/react-select": "^1.2.1",
"@types/rison": "0.0.6",
"@vx/responsive": "^0.0.195",
"abortcontroller-polyfill": "^1.1.9",
"aphrodite": "^2.3.1",
Expand Down Expand Up @@ -156,6 +158,7 @@
"redux-thunk": "^2.1.0",
"redux-undo": "^1.0.0-beta9-9-7",
"regenerator-runtime": "^0.13.3",
"rison": "^0.1.1",
"shortid": "^2.2.6",
"urijs": "^1.18.10",
"use-query-params": "^0.4.5"
Expand All @@ -177,7 +180,6 @@
"@types/react": "^16.9.23",
"@types/react-dom": "^16.9.5",
"@types/react-redux": "^7.1.7",
"@types/react-select": "^3.0.10",
"@types/react-table": "^7.0.2",
"@types/react-ultimate-pagination": "^1.2.0",
"@types/yargs": "12 - 15",
Expand Down
126 changes: 61 additions & 65 deletions superset-frontend/src/dashboard/components/PropertiesModal.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,9 @@ import React from 'react';
import PropTypes from 'prop-types';
import { Row, Col, Button, Modal, FormControl } from 'react-bootstrap';
import Dialog from 'react-bootstrap-dialog';
import Select from 'react-select';
import { Async as SelectAsync } from 'react-select';
import AceEditor from 'react-ace';
import rison from 'rison';
import { t } from '@superset-ui/translation';
import { SupersetClient } from '@superset-ui/connection';
import '../stylesheets/buttons.less';
Expand Down Expand Up @@ -55,18 +56,18 @@ class PropertiesModal extends React.PureComponent {
json_metadata: '',
},
isDashboardLoaded: false,
ownerOptions: null,
isAdvancedOpen: false,
};
this.onChange = this.onChange.bind(this);
this.onMetadataChange = this.onMetadataChange.bind(this);
this.onOwnersChange = this.onOwnersChange.bind(this);
this.save = this.save.bind(this);
this.toggleAdvanced = this.toggleAdvanced.bind(this);
this.loadOwnerOptions = this.loadOwnerOptions.bind(this);
this.handleErrorResponse = this.handleErrorResponse.bind(this);
}

componentDidMount() {
this.fetchOwnerOptions();
this.fetchDashboardDetails();
}

Expand All @@ -90,41 +91,42 @@ class PropertiesModal extends React.PureComponent {
// datamodel, the dashboard could probably just be passed as a prop.
SupersetClient.get({
endpoint: `/api/v1/dashboard/${this.props.dashboardId}`,
})
.then(response => {
const dashboard = response.json.result;
this.setState(state => ({
isDashboardLoaded: true,
values: {
...state.values,
dashboard_title: dashboard.dashboard_title || '',
slug: dashboard.slug || '',
json_metadata: dashboard.json_metadata || '',
},
}));
const initialSelectedValues = dashboard.owners.map(owner => ({
value: owner.id,
label: owner.username,
}));
this.onOwnersChange(initialSelectedValues);
})
.catch(err => console.error(err));
}).then(response => {
const dashboard = response.json.result;
this.setState(state => ({
isDashboardLoaded: true,
values: {
...state.values,
dashboard_title: dashboard.dashboard_title || '',
slug: dashboard.slug || '',
json_metadata: dashboard.json_metadata || '',
},
}));
const initialSelectedOwners = dashboard.owners.map(owner => ({
value: owner.id,
label: `${owner.first_name} ${owner.last_name}`,
Copy link
Member

Choose a reason for hiding this comment

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

In general, formatting like this isn't always applicable as names written as "Firstname Lastname" is very Western centric, and doesn't handle the East Asian norm of "Familyname Givenname" or even cultures with multiple last names.

While modifying how names are stored in the database is certainly not in scope for this PR, I would recommend that we continue to use username as the way to reference owners within the UI until we have a better data structure for human names

One of my favorite articles on the topic: https://www.kalzumeus.com/2010/06/17/falsehoods-programmers-believe-about-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.

I completely agree about the name problem, and have been looking for a chance to fix that. But using "firstname lastname" like this is in line with all the existing UI, so this is not a new problem. And usernames are not easily guessable in all Superset environments. I'd prefer to solve the name problem properly rather than working around it, and use this format for now.

Copy link
Member

Choose a reason for hiding this comment

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

I guess this is fine for now then, but we really should prioritize fixing this. It falls in line with a lot of the other work that was done to shore up the data model in the metadata database

}));
this.onOwnersChange(initialSelectedOwners);
}, this.handleErrorResponse);
}

fetchOwnerOptions() {
SupersetClient.get({
endpoint: `/api/v1/dashboard/related/owners`,
})
.then(response => {
loadOwnerOptions(input = '') {
const query = rison.encode({ filter: input });
return SupersetClient.get({
endpoint: `/api/v1/dashboard/related/owners?q=${query}`,
}).then(
response => {
const options = response.json.result.map(item => ({
value: item.value,
label: item.text,
}));
this.setState({
ownerOptions: options,
});
})
.catch(err => console.error(err));
return { options };
},
badResponse => {
this.handleErrorResponse(badResponse);
return { options: [] };
},
);
}

updateFormState(name, value) {
Expand All @@ -142,6 +144,17 @@ class PropertiesModal extends React.PureComponent {
}));
}

async handleErrorResponse(response) {
const { error, statusText } = await getClientErrorObject(response);
this.dialog.show({
title: 'Error',
bsSize: 'medium',
bsStyle: 'danger',
actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
body: error || statusText || t('An error has occurred'),
});
}

save(e) {
e.preventDefault();
e.stopPropagation();
Expand All @@ -157,38 +170,21 @@ class PropertiesModal extends React.PureComponent {
json_metadata: values.json_metadata || null,
owners,
}),
})
.then(({ json }) => {
this.props.addSuccessToast(t('The dashboard has been saved'));
this.props.onDashboardSave({
id: this.props.dashboardId,
title: json.result.dashboard_title,
slug: json.result.slug,
jsonMetadata: json.result.json_metadata,
ownerIds: json.result.owners,
});
this.props.onHide();
})
.catch(response =>
getClientErrorObject(response).then(({ error, statusText }) => {
this.dialog.show({
title: 'Error',
bsSize: 'medium',
bsStyle: 'danger',
actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
body: error || statusText || t('An error has occurred'),
});
}),
);
}).then(({ json }) => {
this.props.addSuccessToast(t('The dashboard has been saved'));
this.props.onDashboardSave({
id: this.props.dashboardId,
title: json.result.dashboard_title,
slug: json.result.slug,
jsonMetadata: json.result.json_metadata,
ownerIds: json.result.owners,
});
this.props.onHide();
}, this.handleErrorResponse);
}

render() {
const {
ownerOptions,
values,
isDashboardLoaded,
isAdvancedOpen,
} = this.state;
const { values, isDashboardLoaded, isAdvancedOpen } = this.state;
return (
<Modal show={this.props.show} onHide={this.props.onHide} bsSize="lg">
<form onSubmit={this.save}>
Expand Down Expand Up @@ -242,14 +238,14 @@ class PropertiesModal extends React.PureComponent {
<label className="control-label" htmlFor="owners">
{t('Owners')}
</label>
<Select
<SelectAsync
name="owners"
multi
isLoading={!ownerOptions}
value={values.owners}
options={ownerOptions || []}
loadOptions={this.loadOwnerOptions}
onChange={this.onOwnersChange}
disabled={!ownerOptions || !isDashboardLoaded}
disabled={!isDashboardLoaded}
filterOption={() => true} // options are filtered at the api
/>
<p className="help-block">
{t('Owners is a list of users who can alter the dashboard.')}
Expand Down
59 changes: 33 additions & 26 deletions superset-frontend/src/explore/components/PropertiesModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ import {
} from 'react-bootstrap';
// @ts-ignore
import Dialog from 'react-bootstrap-dialog';
import Select from 'react-select';
import { Async as SelectAsync, Option } from 'react-select';
import rison from 'rison';
import { t } from '@superset-ui/translation';
import { SupersetClient, Json } from '@superset-ui/connection';
import Chart from 'src/types/Chart';
Expand Down Expand Up @@ -70,15 +71,14 @@ export default function PropertiesModalWrapper({
function PropertiesModal({ slice, onHide, onSave }: InternalProps) {
const [submitting, setSubmitting] = useState(false);
const errorDialog = useRef<any>(null);
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<any[] | null>(null);
const [owners, setOwners] = useState<Option[] | null>(null);

function showError({ error, statusText }: any) {
errorDialog.current.show({
Expand All @@ -90,7 +90,7 @@ function PropertiesModal({ slice, onHide, onSave }: InternalProps) {
});
}

async function fetchOwners() {
async function fetchChartData() {
try {
const response = await SupersetClient.get({
endpoint: `/api/v1/chart/${slice.slice_id}`,
Expand All @@ -99,7 +99,7 @@ function PropertiesModal({ slice, onHide, onSave }: InternalProps) {
setOwners(
chart.owners.map((owner: any) => ({
value: owner.id,
label: owner.username,
label: `${owner.first_name} ${owner.last_name}`,
})),
);
} catch (response) {
Expand All @@ -110,34 +110,41 @@ function PropertiesModal({ slice, onHide, onSave }: InternalProps) {

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

// get the list of users who can own a chart
useEffect(() => {
SupersetClient.get({
endpoint: `/api/v1/chart/related/owners`,
}).then(res => {
const { result } = res.json as Json;
setOwnerOptions(
result.map((item: any) => ({
const loadOptions = (input = '') => {
const query = rison.encode({ filter: input });
return SupersetClient.get({
endpoint: `/api/v1/chart/related/owners?q=${query}`,
}).then(
response => {
const { result } = response.json as Json;
const options = result.map((item: any) => ({
value: item.value,
label: item.text,
})),
);
});
}, []);
}));
return { options };
},
badResponse => {
getClientErrorObject(badResponse).then(showError);
return { options: [] };
},
);
};

const onSubmit = async (event: React.FormEvent) => {
event.stopPropagation();
event.preventDefault();
setSubmitting(true);
const payload = {
const payload: { [key: string]: any } = {
slice_name: name || null,
description: description || null,
cache_timeout: cacheTimeout || null,
owners: owners!.map(o => o.value),
};
if (owners) {
payload.owners = owners.map(o => o.value);
}
try {
const res = await SupersetClient.put({
endpoint: `/api/v1/chart/${slice.slice_id}`,
Expand Down Expand Up @@ -229,14 +236,14 @@ function PropertiesModal({ slice, onHide, onSave }: InternalProps) {
<label className="control-label" htmlFor="owners">
{t('Owners')}
</label>
<Select
name="owners"
<SelectAsync
multi
isLoading={!ownerOptions}
value={owners}
options={ownerOptions || []}
name="owners"
value={owners || []}
loadOptions={loadOptions}
onChange={setOwners}
disabled={!owners || !ownerOptions}
disabled={!owners}
filterOption={() => true} // options are filtered at the api
/>
<p className="help-block">
{t('A list of users who can alter the chart')}
Expand Down
9 changes: 7 additions & 2 deletions superset/charts/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@
)
from superset.constants import RouteMethod
from superset.models.slice import Slice
from superset.views.base_api import BaseSupersetModelRestApi
from superset.views.base_api import BaseSupersetModelRestApi, RelatedFieldFilter
from superset.views.filters import FilterRelatedOwners

logger = logging.getLogger(__name__)

Expand All @@ -65,6 +66,8 @@ class ChartRestApi(BaseSupersetModelRestApi):
"description",
"owners.id",
"owners.username",
"owners.first_name",
"owners.last_name",
"dashboards.id",
"dashboards.dashboard_title",
"viz_type",
Expand Down Expand Up @@ -116,7 +119,9 @@ class ChartRestApi(BaseSupersetModelRestApi):
"slices": ("slice_name", "asc"),
"owners": ("first_name", "asc"),
}
filter_rel_fields_field = {"owners": "first_name"}
related_field_filters = {
"owners": RelatedFieldFilter("first_name", FilterRelatedOwners)
}
allowed_rel_fields = {"owners"}

@expose("/", methods=["POST"])
Expand Down
9 changes: 7 additions & 2 deletions superset/dashboards/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@
)
from superset.models.dashboard import Dashboard
from superset.views.base import generate_download_headers
from superset.views.base_api import BaseSupersetModelRestApi
from superset.views.base_api import BaseSupersetModelRestApi, RelatedFieldFilter
from superset.views.filters import FilterRelatedOwners

logger = logging.getLogger(__name__)

Expand All @@ -68,6 +69,8 @@ class DashboardRestApi(BaseSupersetModelRestApi):
"json_metadata",
"owners.id",
"owners.username",
"owners.first_name",
"owners.last_name",
"changed_by_name",
"changed_by_url",
"changed_by.username",
Expand Down Expand Up @@ -113,7 +116,9 @@ class DashboardRestApi(BaseSupersetModelRestApi):
"slices": ("slice_name", "asc"),
"owners": ("first_name", "asc"),
}
filter_rel_fields_field = {"owners": "first_name"}
related_field_filters = {
"owners": RelatedFieldFilter("first_name", FilterRelatedOwners)
}
allowed_rel_fields = {"owners"}

@expose("/", methods=["POST"])
Expand Down
Loading