Scheduled query browser (Stage 2) #476

mfix22 · 2018-07-04T00:31:25Z

Here is the Stage 2 PR for the Scheduled Queries projects.

Just FYI we still have more tests to write, but we are going to include those with Stage 3 to try and balance the front and backend work 👍 Hope that works.

Also FYI: we will be mostly offline tomorrow for the 4th of July, but probably will have time to make some quick fixes in the morning!

n-riesco · 2018-07-04T07:46:25Z

static/css/Preview.css

@@ -17,6 +17,13 @@ th, td {
    overflow: auto;
 }

+.scheduleButton {


this style rule should go into app/components/Settings/Preview/code-editor.css

n-riesco · 2018-07-04T07:49:39Z

Please, see some examples of Falcon's naming conventions in https://github.com/plotly/falcon-sql-client/blob/master/CODING_GUIDELINES.md .

TODO

rename "app/components/Settings/Scheduler/createModal.jsx" to "app/components/Settings/scheduler/create-modal.jsx"
rename "app/components/Settings/Scheduler/loginModal.jsx" to "app/components/Settings/scheduler/login-modal.jsx"
rename "app/components/Settings/Scheduler/previewModal.jsx" to "app/components/Settings/scheduler/preview-modal.jsx"
rename "app/components/Settings/Scheduler/scheduler.css" to "app/components/Settings/scheduler/scheduler.css"
rename "app/components/Settings/Scheduler/scheduler.jsx" to "app/components/Settings/scheduler/scheduler.jsx"
rename "app/components/Settings/Scheduler/sql.jsx" to "app/components/Settings/scheduler/sql.jsx"
rename "test/app/components/Settings/Scheduler/createModal.jsx" to "test/app/components/Settings/scheduler/create-modal.jsx"
remove "app/components/Settings/Scheduler/util.js"

n-riesco · 2018-07-04T08:09:53Z

app/components/Settings/Scheduler/util.js

@@ -0,0 +1,14 @@
+import {DIALECTS} from '../../../constants/constants.js';
+
+export function mapDialect(dialect) {


Duplicate code: code-editor.jsx already defines this code.

Please, add the following untested code to app/constants/constants.js:

export function getHighlightMode(dialect) { if (!SQL_DIALECTS_USING_EDITOR.includes(dialect)) { return 'text/plain'; } return { [DIALECTS.APACHE_SPARK]: 'text/x-sparksql', [DIALECTS.MYSQL]: 'text/x-mysql', [DIALECTS.SQLITE]: 'text/x-sqlite', [DIALECTS.MARIADB]: 'text/x-mariadb', [DIALECTS.ORACLE]: 'text/x-plsql', [DIALECTS.POSTGRES]: 'text/x-pgsql', [DIALECTS.REDSHIFT]: 'text/x-pgsql', [DIALECTS.MSSQL]: 'text/x-mssql' }[dialect] || 'text/x-sql'; }

And update the following files accordingly:

app/components/Settings/Preview/code-editor.jsx

app/components/Settings/Scheduler/createModal.jsx

app/components/Settings/Scheduler/updateModal.jsx

n-riesco · 2018-07-04T09:30:29Z

~~The access token for testing expired yesterday.~~
~~I've just updated master with a new token.~~
~~Please, rebase this PR against master.~~

n-riesco · 2018-07-04T10:25:31Z

For this PR, please:

For SQL connectors (those listed in SQL_DIALECTS_USING_EDITOR), remove the button Create Scheduled Query from <Scheduler> (we don't want to duplicate the functionality in the Query panel)
remove Filename from <PreviewModal> (at the moment Falcon's backend can't list filenames and thus Falcon won't be able to warn the user whether a filename is already in use) (@tarzzz how does Chart Studio generate the filenames?)

TO DISCUSS:

This PR has convinced me that the Schedule panel, should be a subpanel under the Query panel:

we don't want the user to switch back and forth between the Query and Schedule panels
we don't want the Schedule panel to duplicate the functionality in the Query panel (note the query panel provides a database overview and name auto-completion)

n-riesco · 2018-07-04T10:45:51Z

app/actions/sessions.js

+        connectionId
+      }
+    )).then((res) => {
+      dispatch(createScheduledQueryAction(res));


Is createScheduledQueryAction necessary? It doesn't seem to be used anywhere else.

If not, let's just do:

dispatch({ type: 'CREATE_SCHEDULED_QUERY', payload: res });

Scratch the above comment. I'll come back to you about this.

n-riesco · 2018-07-04T13:20:39Z

When I first read the code, I found confusing and I missed the significance of having the following pairs of actions:

setScheduledQueries and getScheduledQueries
createScheduledQuery and createScheduledQueryAction
updateScheduledQuery and updateScheduledQueryAction
deleteScheduledQuery and deleteScheduledQueryAction

As the actions getScheduledQueries, createScheduledQueryAction, updateScheduledQueryAction and deleteScheduledQueryAction are only used internally, I'd rather have them removed and avoid the confusion and the time spent to learn their significance.

I've tested the change below and it works for me:

diff --git a/app/actions/sessions.js b/app/actions/sessions.js
index 1c9738d..754da7d 100644
--- a/app/actions/sessions.js
+++ b/app/actions/sessions.js
@@ -9,10 +9,6 @@ export const mergeTabMap = createAction('MERGE_TAB_MAP');
 export const setTab = createAction('SET_TAB');
 export const setTable = createAction('SET_TABLE');
 export const setIndex = createAction('SET_INDEX');
-export const setScheduledQueries = createAction('SET_SCHEDULED_QUERIES');
-export const createScheduledQueryAction = createAction('CREATE_SCHEDULED_QUERY');
-export const updateScheduledQueryAction = createAction('UPDATE_SCHEDULED_QUERY');
-export const deleteScheduledQueryAction = createAction('DELETE_SCHEDULED_QUERY');
 export const mergeConnections = createAction('MERGE_CONNECTIONS');
 export const updateConnection = createAction('UPDATE_CREDENTIAL');
 export const deleteConnection = createAction('DELETE_CREDENTIAL');
@@ -148,7 +144,10 @@ export function getScheduledQueries() {
             'GET',
             'scheduledQueriesRequest'
         )).then((json => {
-            dispatch(setScheduledQueries(json));
+            dispatch({
+                type: 'SET_SCHEDULED_QUERIES',
+                payload: json
+            });
             return json;
         }));
     };
@@ -171,8 +170,11 @@ export function createScheduledQuery(connectionId, payload = {}) {
         connectionId
       }
     )).then((res) => {
-      dispatch(createScheduledQueryAction(res));
-      return res;
+        dispatch({
+            type: 'CREATE_SCHEDULED_QUERY',
+            payload: res
+        });
+        return res;
     });
   };
 }
@@ -196,8 +198,11 @@ export function updateScheduledQuery(connectionId, payload = {}) {
       payload.filename,
       body
     )).then((res) => {
-      dispatch(updateScheduledQueryAction(body));
-      return res;
+        dispatch({
+            type: 'UPDATE_SCHEDULED_QUERY',
+            payload: body
+        });
+        return res;
     });
   };
 }
@@ -209,8 +214,11 @@ export function deleteScheduledQuery(fid) {
       'DELETE',
       'createScheduledQueryRequest'
     )).then((res) => {
-      dispatch(deleteScheduledQueryAction(fid));
-      return res;
+        dispatch({
+            type: 'DELETE_SCHEDULED_QUERY',
+            payload: fid
+        });
+        return res;
     });
   };
 }

n-riesco · 2018-07-04T13:22:23Z

app/components/Settings/Preview/Preview.react.js

@@ -347,6 +348,7 @@ class Preview extends Component {

                                            dialect={dialect}
                                            runQuery={this.runQuery}
+                                            scheduleQuery={this.props.openScheduler}


is it possible to rename this property to openScheduler? (this seems to be the name used elsewhere)

n-riesco · 2018-07-04T13:28:00Z

app/components/Settings/Preview/Preview.react.js

+                                                  className="btn btn-outline"
+                                              >
+                                                  Upload Dataset (auto-updating)
+                                              </button>


remove this button as it doesn't do anything in this PR.

n-riesco · 2018-07-04T13:30:26Z

app/components/Settings/Preview/code-editor.jsx

@@ -25,6 +25,7 @@ export default class CodeEditor extends React.Component {

        dialect: PropTypes.string,
        runQuery: PropTypes.func,
+        scheduleQuery: PropTypes.func,


see this comment

n-riesco · 2018-07-04T13:40:41Z

app/components/Settings/Scheduler/previewModal.jsx

+};
+const boxStyle = { boxSizing: 'border-box', width: '50%' };
+
+export class PreviewModal extends Component {


Add description; e.g.: "implements a modal window to view details of a scheduled query"

n-riesco · 2018-07-04T13:41:17Z

app/components/Settings/Scheduler/createModal.jsx

+  onChange: PropTypes.func
+};
+
+class CreateModal extends Component {


Add description; e.g.: "implements a modal window to schedule a new query"

n-riesco · 2018-07-04T13:52:47Z

app/components/Settings/Scheduler/createModal.jsx

+    { label: 'Run weekly', value: 7 * 24 * 60 * 60 }
+];
+
+const styles = {


Not a blocking change, but these styles should go into its own file, e.g. create-modal.css; (having so many <xxxx style={styles.xxxx}> is a maintenance burden).

n-riesco · 2018-07-04T13:55:29Z

app/components/Settings/Scheduler/loginModal.jsx

+import { Row, Column } from '../../layout.jsx';
+import Modal from '../../modal.jsx';
+
+const styles = {


Not a blocking change, but these styles should go into its own file, e.g. login-modal.css; (having so many <xxxx style={styles.xxxx}> is a maintenance burden).

tarzzz · 2018-07-04T13:56:35Z

(@tarzzz how does Chart Studio generate the filenames?)

@n-riesco Its automatically filled in if the user hasn't supplied any filename (during creation/updates). The default naming is "Grid 121" (or "Filetype unique_number").

n-riesco · 2018-07-04T13:59:51Z

@tarzzz At the moment Falcon uses https://github.com/plotly/falcon-sql-client/blob/6797d9e9c1411a23d1cffc25ccfdd63a7f4e0431/backend/persistent/plotly-api.js#L97-L120 to create a new grid. How should we do so that this function doesn't require a filename?

tarzzz · 2018-07-04T14:09:23Z

How should we do so that this function doesn't require a filename?

The best way to handle this is to hit the lookup endpoint with the exists parameter to check if the filename exists on server(its an optimized endpoint and is reliably fast): https://api.plot.ly/v2/files#lookup

n-riesco · 2018-07-04T14:24:20Z

app/components/Settings/Scheduler/previewModal.jsx

+        this.close = this.close.bind(this);
+    }
+
+    componentDidUpdate(prevProps) {


use componentWillReceiveProps instead

Are you sure you want to use componentWillReceiveProps? That API is deprecated in react@^16.3.0. No worries either way, just wanted to make sure you knew 😄

n-riesco · 2018-07-04T14:35:27Z

app/components/Settings/Scheduler/previewModal.jsx

+            });
+            this.setState({ editing: false, confirmedDelete: false });
+        } else {
+            this.toggleEditing();


This is a bit confusing to read because I have to remember what the current value of this.state.editing is.

Since toggleEditting() is only used here, please remove toggleEditing() and use this.setState({editing: true}); instead.

Definitely (:

n-riesco · 2018-07-04T14:35:59Z

app/components/Settings/Scheduler/previewModal.jsx

+        }
+    }
+
+    onDelete() {


n-riesco · 2018-07-04T14:48:39Z

app/components/Settings/Scheduler/previewModal.jsx

+        }
+    }
+
+    close() {


At the moment, the modal window closes as soon as the user clicks a button. With this behaviour, users can't tell whether the request is still being processed or it has failed.

Ideally, we want to tell the user whether the request failed and if it did fail we don't want to close the modal window.

n-riesco · 2018-07-04T14:52:02Z

app/components/Settings/Scheduler/previewModal.jsx

+                            >
+                                {editing ? (
+                                    <div>
+                                        <CodeMirror


There is an issue with size of the cursor. See below:

n-riesco · 2018-07-04T15:01:17Z

test/app/components/Settings/Scheduler/scheduler.test.jsx

+
+        expect(component.find(Modal).get(2).props.open).toBe(true);
+
+        const modalSqlElements = component


needs updating (test failure here). see https://circleci.com/gh/plotly/falcon-sql-client/5113

n-riesco · 2018-07-04T15:07:31Z

I still need to review the changes to app/components/Settings/Settings.react.js

I'll do it tomorrow.

I noticed there is a race that allows users to open the schedule panel first and then immediately after switch to the query panel before it's ready.

n-riesco · 2018-07-05T12:22:24Z

app/components/Settings/Settings.react.js

+                              updateScheduledQuery={updateScheduledQuery}
+                              deleteScheduledQuery={deleteScheduledQuery}
+                              initialCode={this.state.scheduledQuery}
+                              openLogin={this.updateSelectedPanel.bind(this, 3)}


Shall we define this.openLogin (as was done with `this.openScheduler)?

n-riesco · 2018-07-05T12:37:41Z

app/components/Settings/Settings.react.js

+    }
+
+    openScheduler() {
+      this.setState({ scheduledQuery: this.props.preview.code }, () => {


I don't understand how this code works. Why do we need to set scheduleQuery before opening the panel and unset it immediately after?

Also note that the value of this.props.preview.code isn't properly set before the connection has successfully connected to the database and retrieve the list of available tables.

When I reviewed the previous PR, I thought it was OK for the schedule panel to open before the Query panel is ready (because Falcon runs the scheduled queries even if the user isn't connected).

Now, I think it's easier if we stop the Scheduler panel from being open before the connection is ready.

How about having <Tab disabled={queryPanelDisabled}>Schedule</Tab> here?

n-riesco · 2018-07-05T12:53:59Z

test/app/components/Settings/Scheduler/createModal.jsx

+    let CreateModal;
+    beforeAll(() => {
+        configure({ adapter: new Adapter() });
+        global.document.createRange = function() {


In the tests we already have, polyfills and imports are run in the global context.
See:
https://github.com/jakedex/falcon-sql-client/blob/498420c042490870b6669819ddfb8a1047cd2627/test/app/components/Settings/Preview/chart-editor.test.jsx#L5-L11

Cool. I moved it to the global context, but if I'm not mistaken, you all don't already have polyfills for document.createRange so I'll keep the one I wrote 👍

briandennis · 2018-07-05T18:45:06Z

For SQL connectors (those listed in SQL_DIALECTS_USING_EDITOR), remove the button Create Scheduled Query from (we don't want to duplicate the functionality in the Query panel)

@n-riesco for connection dialects that don't use the query panel editor, is it still meaningful to display a query input within the create scheduled query modal?

How are scheduled queries currently handled for dialects which are not contained in SQL_DIALECTS_USING_EDITOR?

nicolaskruchten · 2018-07-05T20:28:32Z

Re the filename I would love it if the user could specify a filename and if the API doesn't like it that we bubble that error through. If the API currently doesn't return an informative message, let's change that on the API side to enable this feature /cc @tarzzz FYI

nicolaskruchten · 2018-07-05T20:28:46Z

I also think we should ask for and store a Query Name, independent of the filename

tarzzz · 2018-07-05T20:33:23Z

If the API currently doesn't return an informative message, let's change that on the API side to enable this feature

Yep, It's already covered in API .. ⬇️

Sorry, a file already exists with this name.

mfix22 · 2018-07-05T20:49:41Z

Re the filename I would love it if the user could specify a filename and if the API doesn't like it that we bubble that error through. If the API currently doesn't return an informative message, let's change that on the API side to enable this feature /cc @tarzzz FYI

@nicolaskruchten I currently have this implemented, but maybe since we are adding "Query Name"/"Query Title" we can just uncomment it and enable this with Stage 3?

n-riesco · 2018-07-05T22:31:45Z

@briandennis

for connection dialects that don't use the query panel editor, is it still meaningful to display a query input within the create scheduled query modal?

yes, it is possible to have scheduled queries for those connectors too.
if you want to test it, you can try the apache drill connector:

falcon provides credentials to a test server (see screen capture below)

and the test query:

SELECT * FROM s3.root.`sample-data.parquet` LIMIT 10

How are scheduled queries currently handled for dialects which are not contained in SQL_DIALECTS_USING_EDITOR?

Same as for other connectors (it already works with your PR)

n-riesco · 2018-07-06T12:24:11Z

app/components/Settings/Preview/code-editor.jsx

                <a
                    className="btn btn-primary runButton"
                    onClick={runQuery}
-                    disabled={!isLoading}
+                    disabled={isLoading}


nice catch!

n-riesco · 2018-07-06T12:57:34Z

app/components/Settings/scheduler/create-modal.jsx

+                error: 'Please enter the query to be scheduled above.'
+            });
+        }
+        // if (!this.state.filename || !this.state.filename.length) {


Please, ignore this comment for now. The second condition isn't necessary, if (!this.state.code) is enough, because an empty string is converted to false.

n-riesco · 2018-07-06T13:03:04Z

app/components/Settings/scheduler/scheduler.jsx

                    onClickAway={this.closePreview}
                    query={this.state.selectedQuery}
+                    onSave={this.handleUpdate}


I've just noticed while testing. We need to disable onSave and onDelete when !loggedIn.

n-riesco · 2018-07-06T13:16:41Z

test/app/components/Settings/scheduler/create-modal.test.jsx

+        }
+    };
+};
+const CreateModal = require('../../../../../app/components/Settings/scheduler/create-modal.jsx').default;


I'm not terribly familiar with jest. Don't we need to use jest.unmock?

All the other tests use it; e.g. https://github.com/plotly/falcon-sql-client/blob/824b3920738f0f51d631aee2421aa9f1ae23135b/test/app/components/Settings/Preview/chart-editor.test.jsx#L10

Also, is there a reason for using require instead of import?

There is no need to use jest.unmock in this test since we aren't mocking anything using jest.mock. If anything I should just delete global.document.createRange in the afterAll hook.

Also the reason for require instead of import is because require is run sequentially after global.document.createRange = function() { and import is hoisted to the top. Typically this doesn't matter either but CodeMirror has side-effects at the top level which is causing failures on import.

n-riesco · 2018-07-06T13:24:49Z

@mfix22 @briandennis the PR looks very good and ready to merge (after addressing the few minor comments I posted today).

I'm aiming at merging tonight when I'm back.

n-riesco · 2018-07-06T21:17:08Z

@mfix22 I've tested 508ca66 and it doesn't look good:

How about hiding the buttons instead?

Please, don't forget about the comment here.

mfix22 · 2018-07-06T22:13:18Z

@n-riesco yes for sure! I was planning on doing it and then forgot. Just pushed the update. Thanks for catching that! 👍

n-riesco · 2018-07-09T15:24:15Z

I've tested the latest changes and it looks good to me.

There is an issue that I can't reproduce reliably (sometimes I need to click a schedule twice to be able to open the PreviewModal).

I'm merging so that we can progress to stage-3.

Scheduled query browser (Stage 2)

498420c

mfix22 closed this Jul 4, 2018

mfix22 reopened this Jul 4, 2018

n-riesco reviewed Jul 4, 2018

View reviewed changes

app/components/Settings/Scheduler/previewModal.jsx Outdated

}

}

onDelete() {

Copy link

Contributor

n-riesco Jul 4, 2018

Choose a reason for hiding this comment

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

neat!

n-riesco reviewed Jul 4, 2018

View reviewed changes

n-riesco reviewed Jul 5, 2018

View reviewed changes

mfix22 added 4 commits July 5, 2018 10:37

lowercase Scheduler/ imports

26e257c

Move create-modal styles to css file

6403926

Remove filename input, generate 8 digit grid name

f7aa830

Remove 'Create Scheduled Query' button for dialects using editor

5eebf5b

mfix22 added 3 commits July 5, 2018 12:00

Extract login-modal styles to CSS file

706c067

Add loading and error states for create

9360312

Preview modal loading and error states

e07b8b4

Error message from server in modal

cab0265

Prefill filename from generateFilename()

fea40e4

Pull out intervalType

87a8bc2

n-riesco reviewed Jul 6, 2018

View reviewed changes

mfix22 added 2 commits July 6, 2018 08:55

this.state.filename 👍

c61dc54

Don't call create update delete w/o log in

508ca66

Remove buttons completely when not logged in

013c64b

Fix bool proptypes warning

f5c8a19

n-riesco merged commit e31cc51 into plotly:master Jul 9, 2018

mfix22 deleted the stage-2 branch July 10, 2018 15:53

		@@ -0,0 +1,14 @@
		import {DIALECTS} from '../../../constants/constants.js';

		export function mapDialect(dialect) {


		expect(component.find(Modal).get(2).props.open).toBe(true);

		const modalSqlElements = component

Scheduled query browser (Stage 2) #476

Scheduled query browser (Stage 2) #476

Conversation

mfix22 commented Jul 4, 2018

n-riesco Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

n-riesco commented Jul 4, 2018 • edited by mfix22 Loading

n-riesco Jul 4, 2018 • edited by mfix22 Loading

Choose a reason for hiding this comment

n-riesco commented Jul 4, 2018 • edited Loading

n-riesco commented Jul 4, 2018 • edited by mfix22 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n-riesco commented Jul 4, 2018

n-riesco Jul 4, 2018 • 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

Choose a reason for hiding this comment

tarzzz commented Jul 4, 2018

n-riesco commented Jul 4, 2018

tarzzz commented Jul 4, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n-riesco commented Jul 4, 2018

Choose a reason for hiding this comment

n-riesco Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

n-riesco Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briandennis commented Jul 5, 2018

nicolaskruchten commented Jul 5, 2018

nicolaskruchten commented Jul 5, 2018

tarzzz commented Jul 5, 2018

mfix22 commented Jul 5, 2018

n-riesco commented Jul 5, 2018 • 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

n-riesco commented Jul 6, 2018

n-riesco commented Jul 6, 2018

mfix22 commented Jul 6, 2018

n-riesco commented Jul 9, 2018

n-riesco Jul 4, 2018 •

edited

Loading

n-riesco commented Jul 4, 2018 •

edited by mfix22

Loading

n-riesco Jul 4, 2018 •

edited by mfix22

Loading

n-riesco commented Jul 4, 2018 •

edited

Loading

n-riesco commented Jul 4, 2018 •

edited by mfix22

Loading

n-riesco Jul 4, 2018 •

edited

Loading

n-riesco Jul 5, 2018 •

edited

Loading

n-riesco Jul 5, 2018 •

edited

Loading

n-riesco commented Jul 5, 2018 •

edited

Loading