Skip to content

Commit

Permalink
Addressed comments from @arikfr
Browse files Browse the repository at this point in the history
  • Loading branch information
Rohith Menon committed Aug 16, 2017
1 parent feab2a7 commit 59b7961
Show file tree
Hide file tree
Showing 5 changed files with 72 additions and 28 deletions.
2 changes: 1 addition & 1 deletion client/app/components/parameter-settings.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ <h4 class="modal-title">{{$ctrl.parameter.name}}</h4>
</div>
<div class="form-group" ng-if="$ctrl.parameter.type === 'query'">
<label>Saved query (single column output)</label>
<ui-select ng-model="$ctrl.parameter.query" theme="bootstrap" reset-search-input="false" on-select="$ctrl.onQuerySelect($item)">
<ui-select ng-model="$ctrl.parameter.queryBasedOption" theme="bootstrap" reset-search-input="false">
<ui-select-match placeholder="Search a query by name">{{$select.selected.name}}</ui-select-match>
<ui-select-choices repeat="q in $ctrl.queries"
refresh="$ctrl.searchQueries($select.search)"
Expand Down
5 changes: 2 additions & 3 deletions client/app/components/parameters.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,8 @@
<option ng-repeat="option in extractEnumOptions(param.enumOptions)" value="{{option}}">{{option}}</option>
</select>
</span>
<span ng-switch-when="query" ng-controller="QueryBasedParameterController">
<select ng-model="param.value" class="form-control" ng-options="queryResult for queryResult in queryResults">
</select>
<span ng-switch-when="query">
<query-based-parameter param="param"></query-based-parameter>
</span>
<input ng-switch-default type="{{param.type}}" class="form-control" ng-model="param.ngModel">
</span>
Expand Down
89 changes: 66 additions & 23 deletions client/app/components/parameters.js
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,15 @@
import template from './parameters.html';
import queryBasedParameterTemplate from './query-based-parameter.html';
import parameterSettingsTemplate from './parameter-settings.html';

const QueryBasedOption = (query) => {
const queryBasedOption = {
id: query.id,
name: query.name,
};
return queryBasedOption;
};

const ParameterSettingsComponent = {
template: parameterSettingsTemplate,
bindings: {
Expand All @@ -13,39 +22,73 @@ const ParameterSettingsComponent = {

this.trustAsHtml = html => $sce.trustAsHtml(html);
this.parameter = this.resolve.parameter;
this.onQuerySelect = (query) => {
this.parameter.query = query;
};
this.searchQueries = (term) => {
if (!term || term.length < 3) {
return;
}

Query.search({ q: term }, (results) => {
this.queries = results;
this.queries = results.map(query => QueryBasedOption(query));
});
};
},
};

function QueryBasedParameterController($scope, Query) {
$scope.queryResults = [];
$scope.$watch('param', () => {
const param = $scope.param;
if (param.query !== null) {
Query.resultById(
{ id: param.query.id },
(result) => {
const queryResult = result.query_result;
const columns = queryResult.data.columns;
const numColumns = columns.length;
if (numColumns > 0 && columns[0].type === 'string') {
$scope.queryResults = queryResult.data.rows.map(row => row[columns[0].name]);
}
});
}
}, true);
}
const QueryBasedParameterComponent = {
template: queryBasedParameterTemplate,
bindings: {
param: '=',
},
controller($scope, Query) {
'ngInject';

this.queryResultOptions = [];
$scope.$watch('$ctrl.param', () => {
if (this.param.queryBasedOption !== null) {
Query.resultById(
{ id: this.param.queryBasedOption.id },
(result) => {
const queryResult = result.query_result;
const columns = queryResult.data.columns;
const numColumns = columns.length;
// If there are multiple columns, check if there is a column
// named 'name' and column named 'value'. If name column is present
// in results, use name from name column. Similar for value column.
// Default: Use first string column for name and value.
if (numColumns > 0) {
let nameColumn = null;
let valueColumn = null;
columns.forEach((column) => {
const columnName = column.name.toLowerCase();
if (column.type === 'string' && columnName === 'name') {
nameColumn = column.name;
}
if (column.type === 'string' && columnName === 'value') {
valueColumn = column.name;
}
// Assign first string column as name and value column.
if (nameColumn === null && column.type === 'string') {
nameColumn = column.name;
}
if (valueColumn === null && column.type === 'string') {
valueColumn = column.name;
}
});
if (nameColumn !== null && valueColumn !== null) {
this.queryResultOptions = queryResult.data.rows.map((row) => {
const queryResultOption = {
name: row[nameColumn],
value: row[valueColumn],
};
return queryResultOption;
});
}
}
});
}
}, true);
},
};

function ParametersDirective($location, $uibModal) {
return {
Expand Down Expand Up @@ -95,6 +138,6 @@ function ParametersDirective($location, $uibModal) {

export default function (ngModule) {
ngModule.directive('parameters', ParametersDirective);
ngModule.controller('QueryBasedParameterController', QueryBasedParameterController);
ngModule.component('queryBasedParameter', QueryBasedParameterComponent);
ngModule.component('parameterSettings', ParameterSettingsComponent);
}
2 changes: 2 additions & 0 deletions client/app/components/query-based-parameter.html
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
<select ng-model="$ctrl.param.value" class="form-control" ng-options="option.value as option.name for option in $ctrl.queryResultOptions">
</select>
2 changes: 1 addition & 1 deletion client/app/services/query.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class Parameter {
this.value = parameter.value;
this.global = parameter.global;
this.enumOptions = parameter.enumOptions;
this.query = parameter.query;
this.queryBasedOption = parameter.queryBasedOption;
}

get ngModel() {
Expand Down

6 comments on commit 59b7961

@vabanin
Copy link

Choose a reason for hiding this comment

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

Hi @rohithmenon,

I'm very like this feature and merged your pull request in my clear fork of Redash.
But unfortunately something doesn't work... and I see blank lists in query based drop-downs.
Could you help me to make it work?

Thank you.

@rohithmenon
Copy link
Owner

@rohithmenon rohithmenon commented on 59b7961 Sep 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@vabanin

Are you seeing blank in the drop down of query results that must appear in drop down or in the selector for queries to be used for drop down?

If latter,
Redash starts showing up queries after you enter 3 characters of the query name. Try entering 3 characters that would match the query name.

If former, I would need a little more information:

  1. What are the columns in the output for the query intended for drop-down?

  2. How many rows are present in the output of the query intended for drop-down?

  3. Are there usages of templates in the intended query?
    The above questions should be answerable even without using this feature.

  4. When using this feature, are there any errors that you are able to see on chrome debug console? [Skip this question if you are not familiar with this]

@vabanin
Copy link

Choose a reason for hiding this comment

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

@rohithmenon

I'm seeing blank list in drop down of query results.
And I see such picture for all tested queries.
It is for example in case of following simple query:

SELECT 'name1' as name
union all 
SELECT 'name2' as name
union all
SELECT 'name3' as name;

So my answers are following:

What are the columns in the output for the query intended for drop-down?

One text column "name".

How many rows are present in the output of the query intended for drop-down?

3

Are there usages of templates in the intended query?

No

When using this feature, are there any errors that you are able to see on chrome debug console? [Skip this question if you are not familiar with this]

Yes! There are following errors:

angular.js:13920 TypeError: Cannot read property 'id' of undefined
    at parameters.js:49
    at p.$digest (angular.js:17524)
    at p.$apply (angular.js:17790)
    at HTMLSelectElement.<anonymous> (angular.js:31142)
    at Wt (angular.js:3497)
    at HTMLSelectElement.n (angular.js:3485)
(anonymous) @ angular.js:13920

@rohithmenon
Copy link
Owner

Choose a reason for hiding this comment

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

I tested with the same query and works on my local version (latest version of redash with this feature available).

Confirming the steps:

  1. You created a query (similar to the names query above), saved, published and executed.
  2. Selected the above query by query-name in the query selector.
  3. And found the results not appearing in the query drop down?

One way I was able to reproduce no result in drop down is when the query powering the drop down was never executed/refreshed. Try executing the query and try the drop down.

@vabanin
Copy link

Choose a reason for hiding this comment

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

You created a query (similar to the names query above), saved, published and executed.

Yes.

Selected the above query by query-name in the query selector.

Yes. And I found that error in chrome debug console mentioned in my previous message appears after this step.

And found the results not appearing in the query drop down?

Yes.

@vabanin
Copy link

Choose a reason for hiding this comment

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

Selected the above query by query-name in the query selector.

Yes. And I found that error in chrome debug console mentioned in my previous message appears after this step.

Additional info - error appears after selection parameter type 'Query based dropdown List'.

Please sign in to comment.