don't autoexecute query on changing datasource #2652

alison985 · 2018-06-30T19:08:59Z

This is a port of a Mozilla feature upstream.

arikfr

I think it's a good idea, but my only concern is that if we leave previous results it might be confusing.

Maybe we should remove previous results when switching data sources?

alison985 · 2018-07-08T14:09:25Z

That makes sense to me Arik. I'm assuming you mean just hide the display of the results from the UI.

There is already a ng-if="showDataset" in the query.html page for the tabs and data of query results. I tried adding $scope.showDataset = false; locally and adding a setter for showDataset to fix this error: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Getter_only but that didn't work so I added a new variable.

arikfr · 2018-07-08T20:08:37Z

That makes sense to me Arik. I'm assuming you mean just hide the display of the results from the UI.

No, actually the important thing is to clear the reference to the last result (latest_query_data_id). Because otherwise someone loading this query will see that it uses some data source that the results don't belong to.

alison985 · 2018-07-08T22:42:41Z

@arikfr That's already happening here: https://github.com/getredash/redash/blob/master/client/app/pages/queries/view.js#L305

alison985 · 2018-07-25T14:03:39Z

@arikfr Do I need to do anything else before this can be merged?

jezdez · 2018-08-14T19:43:02Z

@alison985 Let's wait till Arik is back for this to merge.

jezdez

I think this should be written with a boolean state that describes if the datasource has changed, as a $scope.dataSourceChanged.

Additionally, I think we'll need to set $scope.dataSourceChanged when the query is executed manually to reset the result/viz div.

jezdez · 2018-10-08T12:27:22Z

client/app/pages/queries/view.js

@@ -120,6 +120,7 @@ function QueryViewCtrl(
    Notifications.getPermissions();
  };

+  $scope.dataSourceHasNotChanged = true;


Let's turn around the logic and call this dataSourceChanged with a default of false so we don't have to deal with a double negative below.

@jezdez Updated.

@alison985 Thank you! 😃

alison985 · 2018-10-21T17:40:50Z

@jezdez Ready for Review.

jezdez

Minor issue with inline styles, LGTM otherwise.

jezdez · 2018-11-08T09:15:57Z

client/app/pages/queries/query.html

@@ -217,7 +217,9 @@ <h3>
              <!-- End of Query Execution Status -->

              <!-- tabs and data -->
-              <div ng-if="showDataset" class="flex-fill p-relative">
+              <div ng-if="showDataset && dataSourceChanged" class="flex-fill p-relative">
+                <div class="d-flex flex-column p-absolute" style="left: 0; top: 0; right: 0; bottom: 0;">


This opens a new div without closing it, and also uses inline styles. Is this needed or just for debugging purposes?

These lines actually look like the result of a bad merge as they serve no purpose. I've removed them. The in-line styles below were there before me.

arikfr · 2018-11-12T08:24:50Z

client/app/pages/queries/query.html

@@ -217,7 +217,7 @@ <h3>
              <!-- End of Query Execution Status -->

              <!-- tabs and data -->
-              <div ng-if="showDataset" class="flex-fill p-relative">
+              <div ng-if="showDataset && dataSourceChanged" class="flex-fill p-relative">


So... it will show the data set if the data source was changed?

@arikfr This variable was originally dataSourceHasNotChanged but @jezdez wanted to remove the double negative. This is the variable added to handle your comments above to not show results when the datasource has changed. You're right to question this, I will add the ! now.

alison985 · 2018-11-25T20:58:23Z

Ping @jezdez and @arikfr

alison985 · 2018-11-26T03:39:58Z

Docker Hub issue made build fail. When Docker Hub is back up the build should be rerun.

alison985 · 2018-11-30T11:44:14Z

Anything else on this one @jezdez and/or @arikfr?

fixes getredash#2651

chang · 2019-02-13T18:58:17Z

+1 This could be especially dangerous when developing a mutation query on a test database. If the schemas are similar enough, it could successfully and unintentionally auto-execute while switching over to a prod data source.

arikfr

If I understand it correctly, dataSourceChanged is needed only in view. But in view mode, you can't change the data source...

arikfr · 2020-01-21T09:01:07Z

Hi,

(This is a template message, but I mean every word of it. Also you're welcome to reply)

Thank you for making this contribution. While we couldn't bring it to completion and merge, it's still very much appreciated. 🙇

In the past year the Redash code base gone under massive updates: on the backend we moved to Python 3 & RQ instead of Celery and on the frontend we replaced Angular with React. It's very likely this makes this PR irrelevant without significant changes. :-(

I'm closing this PR now. But if you're still interested in making it happen, let me know and I will reopen.

Thanks.

jezdez · 2020-01-22T10:58:38Z

@arikfr I'm a bit confused by your last review comment, you're right that changing the source is only possible in edit mode, but I still see a request happening against the server when the source is changed there, and I believe the query is actually executed. I'd appreciate if you'd double-check this on your side so we can close this.

Here's a screen recording of the current master executing the query when changing a data source (if it's the same type of data source): https://dsc.cloud/a46d84/RhVthKiRVrg1Od79fBzY

arikfr · 2020-01-22T11:09:21Z

I just tried it out on https://redash-preview.netlify.com/ and couldn't reproduce it. And looking at your video again I can see that it's definitely not the latest master. Maybe you didn't rebuild the assets?

jezdez · 2020-01-23T10:39:33Z

Lemme check again, thanks!

jezdez · 2020-01-23T11:06:50Z

You right, @arikfr, that was it :) Sorry for the noise!

alison985 mentioned this pull request Jun 30, 2018

Port not executing query when changing data sources to upstream mozilla/redash#436

Closed

arikfr reviewed Jul 2, 2018

View reviewed changes

jezdez self-requested a review September 7, 2018 19:19

jezdez reviewed Oct 8, 2018

View reviewed changes

jezdez reviewed Nov 8, 2018

View reviewed changes

arikfr reviewed Nov 12, 2018

View reviewed changes

jezdez self-assigned this Jan 23, 2019

jezdez assigned washort Jan 30, 2019

don't autoexecute query on changing datasource

1b709b4

fixes getredash#2651

washort force-pushed the alison_2651 branch from 2d487c7 to 1b709b4 Compare January 30, 2019 19:04

ghost added the in progress label Jan 30, 2019

pr-triage bot added the PR: unreviewed label Jan 30, 2019

Merge branch 'master' into alison_2651

b28ab38

arikfr requested changes Feb 25, 2019

View reviewed changes

pr-triage bot added PR: reviewed-changes-requested and removed PR: unreviewed labels Feb 25, 2019

arikfr removed the PR: reviewed-changes-requested label Mar 6, 2019

weekly-digest bot mentioned this pull request Mar 10, 2019

Weekly Digest (3 March, 2019 - 10 March, 2019) #3560

Closed

arikfr removed the in progress label Oct 27, 2019

weekly-digest bot mentioned this pull request Oct 28, 2019

Weekly Digest (21 October, 2019 - 28 October, 2019) #4308

Closed

arikfr closed this Jan 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

don't autoexecute query on changing datasource #2652

don't autoexecute query on changing datasource #2652

alison985 commented Jun 30, 2018

arikfr left a comment

alison985 commented Jul 8, 2018

arikfr commented Jul 8, 2018

alison985 commented Jul 8, 2018

alison985 commented Jul 25, 2018

jezdez commented Aug 14, 2018

jezdez left a comment

jezdez Oct 8, 2018

alison985 Oct 14, 2018

jezdez Oct 16, 2018

alison985 commented Oct 21, 2018

jezdez left a comment

jezdez Nov 8, 2018

alison985 Nov 11, 2018

arikfr Nov 12, 2018

alison985 Nov 18, 2018

alison985 commented Nov 25, 2018

alison985 commented Nov 26, 2018

alison985 commented Nov 30, 2018

chang commented Feb 13, 2019

arikfr left a comment

arikfr commented Jan 21, 2020

jezdez commented Jan 22, 2020

arikfr commented Jan 22, 2020

jezdez commented Jan 23, 2020

jezdez commented Jan 23, 2020

don't autoexecute query on changing datasource #2652

don't autoexecute query on changing datasource #2652

Conversation

alison985 commented Jun 30, 2018

arikfr left a comment

Choose a reason for hiding this comment

alison985 commented Jul 8, 2018

arikfr commented Jul 8, 2018

alison985 commented Jul 8, 2018

alison985 commented Jul 25, 2018

jezdez commented Aug 14, 2018

jezdez left a comment

Choose a reason for hiding this comment

jezdez Oct 8, 2018

Choose a reason for hiding this comment

alison985 Oct 14, 2018

Choose a reason for hiding this comment

jezdez Oct 16, 2018

Choose a reason for hiding this comment

alison985 commented Oct 21, 2018

jezdez left a comment

Choose a reason for hiding this comment

jezdez Nov 8, 2018

Choose a reason for hiding this comment

alison985 Nov 11, 2018

Choose a reason for hiding this comment

arikfr Nov 12, 2018

Choose a reason for hiding this comment

alison985 Nov 18, 2018

Choose a reason for hiding this comment

alison985 commented Nov 25, 2018

alison985 commented Nov 26, 2018

alison985 commented Nov 30, 2018

chang commented Feb 13, 2019

arikfr left a comment

Choose a reason for hiding this comment

arikfr commented Jan 21, 2020

jezdez commented Jan 22, 2020

arikfr commented Jan 22, 2020

jezdez commented Jan 23, 2020

jezdez commented Jan 23, 2020