You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/03/20 22:55:46 UTC
[GitHub] [incubator-superset] suddjian opened a new pull request #9337:
Filter owners select by text input
suddjian opened a new pull request #9337: Filter owners select by text input
URL: https://github.com/apache/incubator-superset/pull/9337
### CATEGORY
- [ ] Bug Fix
- [x] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
The owners input on the Properties modals for charts+dashboards currently makes one request for all the users at mount. This doesn't work on installations with hundreds of users, due to API response limits. This PR loads owner options on each keystroke so you can search for users.
I used `react-select`'s async functionality, which also caches results to reduce api calls.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
Haven't typed anything yet
<img width="429" alt="Screen Shot 2020-03-20 at 1 12 13 PM" src="https://user-images.githubusercontent.com/1858430/77202943-89615d00-6aac-11ea-9eba-e138961fa8b8.png">
After typing, results are filtered
<img width="438" alt="Screen Shot 2020-03-20 at 1 12 23 PM" src="https://user-images.githubusercontent.com/1858430/77202964-92eac500-6aac-11ea-95e4-3d99023ee51f.png">
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
@graceguo-supercat @nytai
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on a change in pull request
#9337: Filter owners select by text input
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #9337: Filter owners select by text input
URL: https://github.com/apache/incubator-superset/pull/9337#discussion_r395926142
##########
File path: superset-frontend/src/dashboard/components/PropertiesModal.jsx
##########
@@ -90,41 +90,41 @@ 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.username,
+ }));
+ this.onOwnersChange(initialSelectedOwners);
+ }, this.handleErrorResponse);
}
- fetchOwnerOptions() {
- SupersetClient.get({
- endpoint: `/api/v1/dashboard/related/owners`,
- })
- .then(response => {
+ loadOwnerOptions(input = '') {
+ return SupersetClient.get({
+ endpoint: `/api/v1/dashboard/related/owners?filter=${input}`,
Review comment:
I tested this and it seemed to work fine. What's the advantage of changing it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9337: Filter owners select by text input
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9337: Filter owners select by text input
URL: https://github.com/apache/incubator-superset/pull/9337#discussion_r395932633
##########
File path: superset-frontend/src/dashboard/components/PropertiesModal.jsx
##########
@@ -90,41 +90,41 @@ 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.username,
+ }));
+ this.onOwnersChange(initialSelectedOwners);
+ }, this.handleErrorResponse);
}
- fetchOwnerOptions() {
- SupersetClient.get({
- endpoint: `/api/v1/dashboard/related/owners`,
- })
- .then(response => {
+ loadOwnerOptions(input = '') {
+ return SupersetClient.get({
+ endpoint: `/api/v1/dashboard/related/owners?filter=${input}`,
Review comment:
Did it really work?
You can always use Swagger to access API docs: http://localhost:8080/swaggerview/v1
Maybe, react-select is filtering the server response for you, but the server is not actually filtering
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9337: Filter owners select by text input
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9337: Filter owners select by text input
URL: https://github.com/apache/incubator-superset/pull/9337#discussion_r404337104
##########
File path: tests/base_api_tests.py
##########
@@ -158,19 +158,13 @@ def test_get_filter_related_owners(self):
API: Test get filter related owners
"""
self.login(username="admin")
- argument = {"filter": "a"}
Review comment:
you could still sort the results in the response and compare? If the order doesn't matter, then we should at least ensure the same users get returned
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9337: Filter owners select by text input
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9337: Filter owners select by text input
URL: https://github.com/apache/incubator-superset/pull/9337#discussion_r404335268
##########
File path: superset-frontend/src/dashboard/components/PropertiesModal.jsx
##########
@@ -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}`,
Review comment:
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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9337: Filter owners select by text input
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9337: Filter owners select by text input
URL: https://github.com/apache/incubator-superset/pull/9337#discussion_r404336980
##########
File path: superset/views/filters.py
##########
@@ -0,0 +1,44 @@
+# 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.
+from flask_appbuilder.models.filters import BaseFilter
+from flask_babel import lazy_gettext
+
+from superset import security_manager
+
+# pylint: disable=too-few-public-methods
+
+
+class FilterRelatedOwners(BaseFilter):
+ """
+ A filter to allow searching for related owners of a resource.
+
+ Use in the api by adding something like:
+ related_field_filters = {
+ "owners": RelatedFieldFilter("first_name", FilterRelatedOwners),
+ }
+ """
+
+ name = lazy_gettext("Owner")
+ arg_name = "owners"
+
+ def apply(self, query, value):
+ user_model = security_manager.user_model
+ like_value = "%" + value + "%"
+ return query.filter(
+ # could be made to handle spaces between names more gracefully
+ (user_model.first_name + " " + user_model.last_name).ilike(like_value)
Review comment:
I agree that appending (while not strictly correct) is an ok way to handle first names and last names. but I still think it makes sense to include the username `or` filter, even if it might not be clear why they were included. Returning too many results is better than not returning the one you're looking for
Or perhaps the select box should include both the first name last name combo and the username in parens afterwards. I think Jira uses a similar pattern so that you can search both by name and email when tagging someone. Github also does a similar thing with name/username
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on issue #9337: Filter
owners select by text input
Posted by GitBox <gi...@apache.org>.
etr2460 commented on issue #9337: Filter owners select by text input
URL: https://github.com/apache/incubator-superset/pull/9337#issuecomment-608540714
@suddjian: is this ready for review again? If so, would you mind fixing the merge conflict?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on issue #9337: Filter
owners select by text input
Posted by GitBox <gi...@apache.org>.
suddjian commented on issue #9337: Filter owners select by text input
URL: https://github.com/apache/incubator-superset/pull/9337#issuecomment-602728793
I'd prefer not to merge this until I can make 2 changes to the `/related/owners` api:
- search should return results that match either the full name or the username
- search should be case-insensitive
I'm working on those and will add them to this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9337: Filter owners select by text input
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9337: Filter owners select by text input
URL: https://github.com/apache/incubator-superset/pull/9337#discussion_r395932633
##########
File path: superset-frontend/src/dashboard/components/PropertiesModal.jsx
##########
@@ -90,41 +90,41 @@ 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.username,
+ }));
+ this.onOwnersChange(initialSelectedOwners);
+ }, this.handleErrorResponse);
}
- fetchOwnerOptions() {
- SupersetClient.get({
- endpoint: `/api/v1/dashboard/related/owners`,
- })
- .then(response => {
+ loadOwnerOptions(input = '') {
+ return SupersetClient.get({
+ endpoint: `/api/v1/dashboard/related/owners?filter=${input}`,
Review comment:
Did it really work?
You can always use Swagger to access API docs: http://localhost:8080/swaggerview/v1
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io commented on issue #9337: Filter
owners select by text input
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9337: Filter owners select by text input
URL: https://github.com/apache/incubator-superset/pull/9337#issuecomment-601964791
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9337?src=pr&el=h1) Report
> Merging [#9337](https://codecov.io/gh/apache/incubator-superset/pull/9337?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/232925b7bf0e2ffbe23029a77a97223663830968&el=desc) will **increase** coverage by `0.00%`.
> The diff coverage is `29.78%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9337?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9337 +/- ##
=======================================
Coverage 58.96% 58.96%
=======================================
Files 374 374
Lines 12137 12143 +6
Branches 2992 2992
=======================================
+ Hits 7156 7160 +4
- Misses 4802 4804 +2
Partials 179 179
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9337?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...erset-frontend/src/dashboard/components/Header.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9337/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0hlYWRlci5qc3g=) | `41.79% <ø> (ø)` | |
| [...rontend/src/explore/components/PropertiesModal.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9337/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9Qcm9wZXJ0aWVzTW9kYWwudHN4) | `13.43% <10.52%> (+0.73%)` | :arrow_up: |
| [...ntend/src/dashboard/components/PropertiesModal.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9337/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL1Byb3BlcnRpZXNNb2RhbC5qc3g=) | `48.64% <42.85%> (+2.81%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9337?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9337?src=pr&el=footer). Last update [232925b...a19adb2](https://codecov.io/gh/apache/incubator-superset/pull/9337?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9337: Filter owners select by text input
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9337: Filter owners select by text input
URL: https://github.com/apache/incubator-superset/pull/9337#discussion_r404219806
##########
File path: tests/base_api_tests.py
##########
@@ -158,19 +158,13 @@ def test_get_filter_related_owners(self):
API: Test get filter related owners
"""
self.login(username="admin")
- argument = {"filter": "a"}
Review comment:
can we keep this test too? I think it's relevant as it tests multiple different users being returned
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on a change in pull request
#9337: Filter owners select by text input
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #9337: Filter owners select by text input
URL: https://github.com/apache/incubator-superset/pull/9337#discussion_r404293364
##########
File path: superset-frontend/src/dashboard/components/PropertiesModal.jsx
##########
@@ -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}`,
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9337: Filter owners select by text input
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9337: Filter owners select by text input
URL: https://github.com/apache/incubator-superset/pull/9337#discussion_r404219360
##########
File path: superset/views/filters.py
##########
@@ -0,0 +1,44 @@
+# 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.
+from flask_appbuilder.models.filters import BaseFilter
+from flask_babel import lazy_gettext
+
+from superset import security_manager
+
+# pylint: disable=too-few-public-methods
+
+
+class FilterRelatedOwners(BaseFilter):
+ """
+ A filter to allow searching for related owners of a resource.
+
+ Use in the api by adding something like:
+ related_field_filters = {
+ "owners": RelatedFieldFilter("first_name", FilterRelatedOwners),
+ }
+ """
+
+ name = lazy_gettext("Owner")
+ arg_name = "owners"
+
+ def apply(self, query, value):
+ user_model = security_manager.user_model
+ like_value = "%" + value + "%"
+ return query.filter(
+ # could be made to handle spaces between names more gracefully
+ (user_model.first_name + " " + user_model.last_name).ilike(like_value)
Review comment:
should this `or` a bunch of filters together? I'd imagine this should support searching within a username string too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 merged pull request #9337: Filter
owners select by text input
Posted by GitBox <gi...@apache.org>.
etr2460 merged pull request #9337: Filter owners select by text input
URL: https://github.com/apache/incubator-superset/pull/9337
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9337: Filter owners select by text input
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9337: Filter owners select by text input
URL: https://github.com/apache/incubator-superset/pull/9337#discussion_r395924882
##########
File path: superset-frontend/src/dashboard/components/PropertiesModal.jsx
##########
@@ -90,41 +90,41 @@ 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.username,
+ }));
+ this.onOwnersChange(initialSelectedOwners);
+ }, this.handleErrorResponse);
}
- fetchOwnerOptions() {
- SupersetClient.get({
- endpoint: `/api/v1/dashboard/related/owners`,
- })
- .then(response => {
+ loadOwnerOptions(input = '') {
+ return SupersetClient.get({
+ endpoint: `/api/v1/dashboard/related/owners?filter=${input}`,
Review comment:
This should be `/api/v1/dashboard/related/owners/?q=(filter:${input})`
using Rison or it's equivalent in JSON
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on issue #9337: Filter
owners select by text input
Posted by GitBox <gi...@apache.org>.
suddjian commented on issue #9337: Filter owners select by text input
URL: https://github.com/apache/incubator-superset/pull/9337#issuecomment-608823890
@etr2460 should be good to go now :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian edited a comment on issue #9337:
Filter owners select by text input
Posted by GitBox <gi...@apache.org>.
suddjian edited a comment on issue #9337: Filter owners select by text input
URL: https://github.com/apache/incubator-superset/pull/9337#issuecomment-602728793
I'd prefer not to merge this until I can make 2 changes to the `/related/owners` api:
- search should return results that match either the full name or the username (currently only matches first name)
- search should be case-insensitive
I'm working on those and will add them to this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on a change in pull request
#9337: Filter owners select by text input
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #9337: Filter owners select by text input
URL: https://github.com/apache/incubator-superset/pull/9337#discussion_r404293388
##########
File path: superset/views/filters.py
##########
@@ -0,0 +1,44 @@
+# 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.
+from flask_appbuilder.models.filters import BaseFilter
+from flask_babel import lazy_gettext
+
+from superset import security_manager
+
+# pylint: disable=too-few-public-methods
+
+
+class FilterRelatedOwners(BaseFilter):
+ """
+ A filter to allow searching for related owners of a resource.
+
+ Use in the api by adding something like:
+ related_field_filters = {
+ "owners": RelatedFieldFilter("first_name", FilterRelatedOwners),
+ }
+ """
+
+ name = lazy_gettext("Owner")
+ arg_name = "owners"
+
+ def apply(self, query, value):
+ user_model = security_manager.user_model
+ like_value = "%" + value + "%"
+ return query.filter(
+ # could be made to handle spaces between names more gracefully
+ (user_model.first_name + " " + user_model.last_name).ilike(like_value)
Review comment:
I originally had it `or` the full name and the username, but @dpgaspar gave feedback that it was strange to get results by username when username is not displayed in the UI, so I removed that.
Using an `or` on the first name and last name would have the effect that you cannot search by full name, so appending them first is preferable. But ultimately a full text search solution would probably be best. These are product/design questions, so I'd prefer to merge this the way it is and iterate on it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9337: Filter owners select by text input
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9337: Filter owners select by text input
URL: https://github.com/apache/incubator-superset/pull/9337#discussion_r404217905
##########
File path: superset-frontend/src/dashboard/components/PropertiesModal.jsx
##########
@@ -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}`,
Review comment:
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/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on a change in pull request
#9337: Filter owners select by text input
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #9337: Filter owners select by text input
URL: https://github.com/apache/incubator-superset/pull/9337#discussion_r404293419
##########
File path: tests/base_api_tests.py
##########
@@ -158,19 +158,13 @@ def test_get_filter_related_owners(self):
API: Test get filter related owners
"""
self.login(username="admin")
- argument = {"filter": "a"}
Review comment:
The endpoint doesn't sort results, so you can't have a deterministic test with multiple results.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9337: Filter owners select by text input
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9337: Filter owners select by text input
URL: https://github.com/apache/incubator-superset/pull/9337#discussion_r395924980
##########
File path: superset-frontend/src/explore/components/PropertiesModal.tsx
##########
@@ -110,34 +109,39 @@ 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(() => {
+ const loadOptions = (input = '') =>
SupersetClient.get({
- endpoint: `/api/v1/chart/related/owners`,
- }).then(res => {
- const { result } = res.json as Json;
- setOwnerOptions(
- result.map((item: any) => ({
+ endpoint: `/api/v1/chart/related/owners?filter=${input}`,
Review comment:
Same here
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org