Enable freeform-select with fetched column values for filter values #1697

vera-liu · 2016-11-28T18:38:11Z

Credits to: @the-dcruz https://github.com/airbnb/superset/pull/1061
Done:

db migration to add filter_select_enabled
add freeform-multi option for Selectfield
modify formatFilter() function on query to accomodate filter-select

To enable:
in edit table mode:

Select for filter-value:

needs-review @mistercrunch @ascott @bkyryliuk

ascott · 2016-11-29T05:56:59Z

superset/models.py

@@ -871,6 +872,7 @@ class SqlaTable(Model, Queryable, AuditMixinNullable, ImportMixin):
    default_endpoint = Column(Text)
    database_id = Column(Integer, ForeignKey('dbs.id'), nullable=False)
    is_featured = Column(Boolean, default=False)
+    filter_select_enabled = Column(Boolean, default=False)


is there any reason not to enable filter select all of the time? it seems like a better user experience to have the values available for filtering in a select rather than a text input.

I think the original incentive was that filter method only retrieves samples of the values, not all of them, for some columns there could be way too many values to be displayed, and user prefer to directly add values in input, separated by comma, instead of wrapping each value in single quotes and add it as an option. (The reason for wrapping values in quote is to accomodate values with comma in them)

ok thx for the explanation 👍

ascott · 2016-11-29T06:01:14Z

superset/viz.py

+        """
+        Returns the URL to retrieve column values used in the filter dropdown
+        """
+        d = self.orig_form_data.copy()


could we use a more descriptive variable name here and on line 160? i'm assuming they stand for data and ordered_data? nice to be more specific.

ascott · 2016-11-29T06:13:00Z

superset/views.py

+            datasource_class).filter_by(id=datasource_id).first()
+
+        if not datasource:
+            flash(__("The datasource seems to have been deleted"), "alert")


we have a constant, DATASOURCE_MISSING_ERR, for this error message here:
https://github.com/airbnb/superset/blob/master/superset/views.py#L81

ascott · 2016-11-29T06:14:54Z

superset/views.py

+        datasource_access = self.can_access(
+            'datasource_access', datasource.perm)
+        if not (all_datasource_access or datasource_access):
+            flash(__("You don't seem to have access to this datasource"),


could use ALL_DATASOURCE_ACCESS_ERR here. https://github.com/airbnb/superset/blob/master/superset/views.py#L79

ascott · 2016-11-29T06:24:15Z

superset/views.py

+            'all_datasource_access', 'all_datasource_access')
+        datasource_access = self.can_access(
+            'datasource_access', datasource.perm)
+        if not (all_datasource_access or datasource_access):


looks like we have datasource access methods on the base view we could use here: https://github.com/airbnb/superset/blob/master/superset/views.py#L54

if not (self.all_datasource_access() or self.datasource_access(datasource))

ascott · 2016-11-29T06:27:02Z

superset/assets/javascripts/explorev2/components/Filter.jsx

@@ -65,13 +84,23 @@ export default class Filter extends React.Component {
            onChange={this.changeOp.bind(this, this.props.filter)}
          />
          <div className="col-lg-6">
-            <input
+            {this.props.filter_select ?


this might be easier to read if we pull it out into it's own method, like renderFilterInput

ascott · 2016-11-29T06:29:25Z

superset/assets/javascripts/explorev2/components/SelectField.jsx

-        />
-        {selectWrap}
+      <div>
+      {selectWrap}


2 space indent needed

ascott · 2016-11-29T06:31:57Z

this is looking good. i'm curious to hear other thoughts on this: should we just enable filter selects by default, and not have an option to enable/disable? seems to me that the select with column values is a much better ux and we should be choosing smart defaults over too much configuration. @mistercrunch @bkyryliuk thoughts?

ascott · 2016-12-14T19:39:59Z

superset/assets/javascripts/explorev2/components/Filter.jsx

@@ -65,13 +84,23 @@ export default class Filter extends React.Component {
            onChange={this.changeOp.bind(this, this.props.filter)}
          />
          <div className="col-lg-6">
-            <input
+            {this.props.renderFilterSelect ?
+            (<SelectField


looks like this should be indented 2 spaces. might be cleaner to pull this out to another method and call that here {this.renderFilterFormField()}

ascott · 2016-12-14T19:53:01Z

superset/assets/javascripts/explorev2/components/SelectField.jsx

@@ -42,8 +42,18 @@ export default class SelectField extends React.Component {
    if (this.props.freeForm) {
      // For FreeFormSelect, insert value into options if not exist
      const values = this.props.choices.map((c) => c[0]);
-      if (values.indexOf(this.props.value) === -1) {
-        options.push({ value: this.props.value, label: this.props.value });
+      if (this.props.value) {


might be cleaner to pull this out into another method and add a comment about how this is for supporting the free form select field options.

ascott · 2016-12-14T20:03:24Z

this generally LGTM. there are a lot of code changes here, would probably be beneficial to write some tests as well, but could come in another PR if you want to get this out asap.

- db migration to add filter_select_enabled - add freeform-multi option for Selectfield - modify formatFilter() function on query to accomodate filter-select

ascott reviewed Nov 29, 2016

View reviewed changes

superset/assets/javascripts/explorev2/components/SelectField.jsx

/>

{selectWrap}

<div>

{selectWrap}

Copy link

ascott Nov 29, 2016

Choose a reason for hiding this comment

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

2 space indent needed

vera-liu force-pushed the freeform_select_filter branch from a0e32c7 to 25c620d Compare December 2, 2016 20:24

ascott reviewed Dec 14, 2016

View reviewed changes

vera-liu force-pushed the freeform_select_filter branch from 25c620d to 7ab2756 Compare December 15, 2016 22:58

vera-liu added 7 commits December 16, 2016 14:00

Enable freeform-select with fetched column values for filter values

15f856b

- db migration to add filter_select_enabled - add freeform-multi option for Selectfield - modify formatFilter() function on query to accomodate filter-select

Fix js tests

7fb6d47

Fix codeclimate issue

ca4ad5a

Changes based on comments

ab94273

Add test for filter endpoint

390cff6

Extract out renderFilterFormField function from render

27c28a4

Fix landscape issues

0aae4b6

vera-liu force-pushed the freeform_select_filter branch from 2192819 to 0aae4b6 Compare December 16, 2016 22:05

vera-liu merged commit 6732f01 into apache:master Dec 16, 2016

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.15.1 labels Feb 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable freeform-select with fetched column values for filter values #1697

Enable freeform-select with fetched column values for filter values #1697

vera-liu commented Nov 28, 2016

ascott Nov 29, 2016 •

edited

Loading

vera-liu Nov 29, 2016 •

edited

Loading

ascott Dec 14, 2016

ascott Nov 29, 2016 •

edited

Loading

ascott Nov 29, 2016

ascott Nov 29, 2016

ascott Nov 29, 2016

ascott Nov 29, 2016

ascott Nov 29, 2016

ascott commented Nov 29, 2016 •

edited

Loading

ascott Dec 14, 2016

ascott Dec 14, 2016

ascott commented Dec 14, 2016

Enable freeform-select with fetched column values for filter values #1697

Enable freeform-select with fetched column values for filter values #1697

Conversation

vera-liu commented Nov 28, 2016

ascott Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

vera-liu Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascott Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascott commented Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascott commented Dec 14, 2016

ascott Nov 29, 2016 •

edited

Loading

vera-liu Nov 29, 2016 •

edited

Loading

ascott Nov 29, 2016 •

edited

Loading

ascott commented Nov 29, 2016 •

edited

Loading