Skip to content

Conversation

@the-dcruz
Copy link
Contributor

Overview

This PR address the issue #933, which is to replace the filter TextField with a FreeFormSelect to make it easier for the user to know what types of values are possible for a filter.

If their desired value is not in the list, they can still type in their custom value. (Especially common for regex operation)

The feature must be enabled on the table/datasource.

Technical Changes

  • Added new API endpoint /filter/<datasource_type>/<datasource_id>/<column>/
  • Added new function values_for_column to models.DruidDatasource and models.SqlaTable (migration script included)
    • values_for_column uses the same querystring as query
    • Currently limited to 500 values
  • Added new boolean column to datasources and tables to indicate allowing filter value select
  • add_filter in explore.jsx will now fix the filter IDs upon render instead of by prepForm on query submit

Screenshots

How filter looks:
screen shot 2016-08-15 at 2 07 56 pm

Option on table:
screen shot 2016-08-15 at 2 12 18 pm

TODO for Future PR

  • Entire filter can be redone in React
  • Cache filter endpoint

@mistercrunch

@the-dcruz
Copy link
Contributor Author

What causes this issue in travis-ci?

ERROR: InvocationError: could not find executable 'CODECLIMATE_REPO_TOKEN=6f24fafe1a2c958b09c0dd98e1794088285f45124de1efdcc011315957379ada'

@the-dcruz the-dcruz force-pushed the filter-value-selector branch 3 times, most recently from 5f91ec2 to 0931c77 Compare August 15, 2016 23:45
@mistercrunch
Copy link
Member

master was failing build from a bad commit of mine attempting to setup coverage in Code Climate, my bad. Tests should pass once you rebase this branch.

caravel/views.py Outdated
datasource_class = models.SqlaTable \
if datasource_type == "table" else models.DruidDatasource
datasources = (
db.session.query(datasource_class).all()
Copy link
Contributor

Choose a reason for hiding this comment

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

we could probably filter the datasource by id here

@the-dcruz the-dcruz force-pushed the filter-value-selector branch 4 times, most recently from 4b2939c to 1253f4a Compare August 16, 2016 19:12
column_name,
from_dttm,
to_dttm,
limit=500,):
Copy link
Contributor

Choose a reason for hiding this comment

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

trailing comma

@the-dcruz the-dcruz closed this Aug 16, 2016
@the-dcruz the-dcruz reopened this Aug 16, 2016
@the-dcruz the-dcruz force-pushed the filter-value-selector branch 2 times, most recently from 7f0b887 to 9603229 Compare August 16, 2016 21:03
@the-dcruz
Copy link
Contributor Author

@mistercrunch @xrmx Could you guys take look when you get a chance? Thanks!

@@ -18,47 +18,16 @@ require('bootstrap');

require('./../caravel-select2.js');

require('../../node_modules/bootstrap-toggle/js/bootstrap-toggle.min.js');
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this already gone in master?

@mistercrunch
Copy link
Member

This looks solid on first glance, I'll try to install it in our staging and play with it today.

@the-dcruz the-dcruz force-pushed the filter-value-selector branch from 9603229 to 1987d3d Compare August 19, 2016 17:51
caravel/views.py Outdated
:param column: Column name to retrieve values for
:return:
"""
# TODO: Cache endpoint by user, datasource and column
Copy link
Contributor

Choose a reason for hiding this comment

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

Speaking of cache was wondering if it would make sense to wrap flask-cache's Cache or just create some helpers to avoid spreading the compression and python3 quirks around. What do you think @mistercrunch?

@the-dcruz
Copy link
Contributor Author

bump

@the-dcruz the-dcruz force-pushed the filter-value-selector branch from 1987d3d to 740c212 Compare August 29, 2016 18:13
@the-dcruz the-dcruz force-pushed the filter-value-selector branch from 07e4263 to 97648d9 Compare September 1, 2016 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants