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

Refactor the explore view #1252

Merged
merged 3 commits into from
Oct 7, 2016
Merged

Conversation

mistercrunch
Copy link
Member

No description provided.

if not self.datasource_access(viz_obj.datasource):
return Response(
json.dumps(
{'error': "You don't have access to this datasource"}),
Copy link
Member Author

Choose a reason for hiding this comment

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

i18n

@mistercrunch mistercrunch force-pushed the refact_explore2 branch 5 times, most recently from 53be27a to 31a6f24 Compare October 5, 2016 20:02
@@ -1598,7 +1603,11 @@ def sqllab_viz(self):
data = json.loads(request.args.get('data'))
table_name = data.get('datasourceName')
viz_type = data.get('chartType')
table = db.session.query(models.SqlaTable).filter_by(table_name=table_name).first()
table = (
Copy link
Member

Choose a reason for hiding this comment

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

should we add a schema here to the filter?

Copy link
Member Author

Choose a reason for hiding this comment

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

risk of collisions in the context of unit tests is close to zero

Copy link
Member

Choose a reason for hiding this comment

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

this code belongs to the views.py @mistercrunch

Copy link
Member Author

Choose a reason for hiding this comment

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

oh right, I was just linting here, but we should tackle this

href = Href(
'/caravel/explore/{self.datasource.type}/'
base_endpoint + '{self.datasource.type}/'
Copy link
Member

Choose a reason for hiding this comment

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

s/
base_endpoint + '{self.datasource.type}/{self.datasource.id}/'.format(**locals()))
/
'{base_endpoint}/{self.datasource.type}/{self.datasource.id}/'.format(**locals())) ?

druid_ds = db.session.query(DruidDatasource).filter_by(
datasource_name="test_click").first()
col_names = set([c.column_name for c in druid_ds.columns])
assert {"affiliate_id", "campaign", "first_seen"} == col_names
Copy link
Member

Choose a reason for hiding this comment

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

s/assert/self.AssertEquals - for better error logging

Copy link
Member Author

Choose a reason for hiding this comment

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

let's do a blanket PR to replace all for this later

@bkyryliuk
Copy link
Member

Great PR, 🚢

@@ -5,4 +5,5 @@ export CARAVEL_CONFIG=tests.caravel_test_config
set -e
caravel/bin/caravel version -v
export SOLO_TEST=1
nosetests tests.core_tests:CoreTests.test_public_user_dashboard_access
#nosetests tests.core_tests:CoreTests.test_slice_endpoint
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this commented out?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh this is just a little helper file to run a specific tests without re-running all the prep work (loading examples, init, ...). typically would just point it to whatever I'm iterating on at the time. I'll change it to receive an argument in the future.

@mistercrunch mistercrunch merged commit f70d301 into apache:master Oct 7, 2016
@mistercrunch mistercrunch deleted the refact_explore2 branch October 7, 2016 23:24
@dpgaspar dpgaspar mentioned this pull request Feb 11, 2020
12 tasks
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.12.0 labels Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants