Implement API to create a grid to store results of a scheduled query by n-riesco · Pull Request #444 · plotly/falcon

n-riesco · 2018-05-27T13:50:23Z

This API is needed for the upcoming UI for scheduling queries.

Closes #440

I've also taken the opportunity to move all the code invoking plotlyAPIRequest into file backend/persistent/plotly-api.js.

@tarzzz , could you review this PR, please?

@shannonlal, this is a good PR to get familiar with the API in the backend that connects to Plotly.

cc/ @jackparmer

* Refactor code to retrieve a user's credentials into a function.

* Move all the code that invokes `plotlyAPIRequest` into `backend/persistent/plotly-api.js`. * Ensure grids created for testing are deleted when the test completes.

* POST queries accepts field `filename` to create a grid for storing the results of the scheduled query. Closes #440

shannonlal

Overall really clean. Just some small comments on constants and names of variables. Was able to follow so not a show stopper.

Great job

shannonlal · 2018-05-27T14:49:44Z

backend/persistent/plotly-api.js

+//
+// See API documentation at https://api.plot.ly/v2/
+//
+// TODO: Refactor as a class with a constructor that takes username as input,


Is this to be done as part of this PR? Or will you be creating a separate Issue

No, that's future TODO (a nice thing to have but it doesn't cause any issues at the moment).

shannonlal · 2018-05-27T14:51:14Z

backend/persistent/QueryScheduler.js

+        }).then(res => {
+            Logger.log(`Request to Plotly for creating a grid took ${process.hrtime(startTime)[0]} seconds`, 2);
+
+            if (res.status !== 201) {


Should be putting this into a constants file with actual codes? I noticed below we have 200, 401, etc. If we used names would make code easier to read

No, it'd make the code less readable (in this context it's clear 201 is an HTTP status code).

shannonlal · 2018-05-27T15:37:05Z

backend/persistent/plotly-api.js

+    });
+}
+
+export function getGridMetadata(fid, requestor) {


Should we use fid or filename? Above it is filename and now it is fid

Plotly's API uses fid.

we can use getGridMeta as it is more consistent with Plotly nomenclature. Not a big deal though..

shannonlal · 2018-05-27T15:37:19Z

backend/persistent/plotly-api.js

+    });
+}
+
+export function updateGrid(rows, fid, uids, requestor) {


fid or filename?

Plotly's API uses fid.

shannonlal · 2018-05-27T15:38:28Z

backend/persistent/plotly-api.js

+        accessToken,
+        method: 'GET'
+    }).then(function(res) {
+        if (res.status === 404) {


We should maybe flip the codes into constants with names?

No, it'd make the code less readable (in this context it's clear 404 is an HTTP status code).

I think that using human readable constant names is generally a good approach. While it's clear that it's an HTTP status code, there are so many specific HTTP status codes, that it's not clear sometimes what an HTTP status code means exactly (e.g. 409 is not a very common status code).

https://philsturgeon.uk/http/2015/08/16/avoid-hardcoding-http-status-codes/

@Gzork One can always open a node session and run:

> http.STATUS_CODES[409] 'Conflict'

I can't think of any REST API that isn't defined in terms of numeric status codes. Let's say, I'm doing a review, and I come across an uncommon status code written like: if (res.status === CONFLICT) {...}. Then, I need to stop my review, and find where CONFLICT has been defined, to be able to confirm that the right status code has been used.

shannonlal · 2018-05-27T15:40:32Z

backend/settings.js

@@ -226,3 +228,8 @@ export function saveSetting(settingName, settingValue) {
     */
    fs.writeFileSync(getSetting('SETTINGS_PATH'), YAML.stringify(settingsOnFile));


We should put an issue on this. If you remember when we tried to pump out the Athena client on this line it would cause an error.

I sneaked a fix for this in the PR that implemented the CSV connector. See f3df19f .

👍 Thanks

n-riesco · 2018-05-27T16:15:02Z

@shannonlal I'm sorry about the quick replies (I'm rushing to wrap up a few things). If you want to discuss further the introduction of STATUS constants or Plotly API, let's open an issue for it.

shannonlal · 2018-05-27T16:26:34Z

@n-riesco Nope. This is a small detail. Looks good and clean. It is a small detail. I would merge it.

shannonlal · 2018-05-27T16:26:40Z

💃

* To be consistent with Plotly nomenclature: #444 (comment)

tarzzz · 2018-05-28T14:38:10Z

backend/persistent/QueryScheduler.js

+    getQueries,
+    saveQuery,
+    deleteQuery
+} from './Queries.js';


(Not necessarily in this PR) but we should small-case these filenames as well.. !! Queries, Datastore etc, and remove .js extensions to be consistent.. !!

I want to keep the extensions, because:

it's grep-friendly

we have imports that only differ in the extension (e.g. code-editor.css and code-editor.jsx)

The idea is to move to lower case, whenever we have a chance (instead of having a huge big PR). We don't really want to spend time on it.

tarzzz · 2018-05-28T16:12:33Z

test/backend/utils.js

+
+    return newGrid(uniqueFilename, names, rows, username);
+}
+


tarzzz · 2018-05-28T16:15:41Z

backend/persistent/QueryScheduler.js

+        let startTime;
+
+        // Check if the user even exists
+        if (!username || !(apiKey || accessToken)) {


We can simplify it:

if !(username || apiKey || accessToken) {

tricky, but not equivalent: the latter doesn't catch the case when username is defined but both apiKey and accessToken are undefined.

tarzzz · 2018-05-28T16:25:50Z

test/backend/routes.queries.spec.js

+                });
+        });
+
+        it('can create a grid when it registers a query', function() {


It is better if you split this into two commits, one where you move the code into a different file, and anotherwhere you do updates/add new code.

Easier to review, and something to consider for next time.. !!

OK, will do next time.

tarzzz

Left some comments, but nothing blocking.. !!

💃

n-riesco added 4 commits May 26, 2018 20:48

PlotlyAPI: use lowercase filenames

26ec584

settings: add getCredentials(username)

c26fa9d

* Refactor code to retrieve a user's credentials into a function.

PlotlyAPI: refactor

b3504bf

* Move all the code that invokes `plotlyAPIRequest` into `backend/persistent/plotly-api.js`. * Ensure grids created for testing are deleted when the test completes.

routes: extend entrypoint POST queries

3aa1c34

* POST queries accepts field `filename` to create a grid for storing the results of the scheduled query. Closes #440

shannonlal reviewed May 27, 2018

View reviewed changes

PlotlyAPI: rename getGridMetadat to getGridMeta

728888b

* To be consistent with Plotly nomenclature: #444 (comment)

tarzzz reviewed May 28, 2018

View reviewed changes

test/backend/utils.js

return newGrid(uniqueFilename, names, rows, username);

}

Copy link

Contributor

tarzzz May 28, 2018

Choose a reason for hiding this comment

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

👍

tarzzz reviewed May 28, 2018

View reviewed changes

tarzzz approved these changes May 28, 2018

View reviewed changes

n-riesco merged commit 1b98662 into master May 28, 2018

n-riesco deleted the plotlyapi/create-grid branch June 5, 2018 15:09

		@@ -226,3 +228,8 @@ export function saveSetting(settingName, settingValue) {
		*/
		fs.writeFileSync(getSetting('SETTINGS_PATH'), YAML.stringify(settingsOnFile));

Uh oh!

Conversation

n-riesco commented May 27, 2018

Uh oh!

shannonlal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n-riesco commented May 27, 2018

Uh oh!

shannonlal commented May 27, 2018

Uh oh!

shannonlal commented May 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarzzz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants