Uh oh!
There was an error while loading. Please reload this page.
Implement API to create a grid to store results of a scheduled query - #444
Conversation
* 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
left a comment
There was a problem hiding this comment.
Overall really clean. Just some small comments on constants and names of variables. Was able to follow so not a show stopper.
Great job
| // | ||
| // See API documentation at https://api.plot.ly/v2/ | ||
| // | ||
| // TODO: Refactor as a class with a constructor that takes username as input, |
There was a problem hiding this comment.
Is this to be done as part of this PR? Or will you be creating a separate Issue
There was a problem hiding this comment.
No, that's future TODO (a nice thing to have but it doesn't cause any issues at the moment).
| }).then(res => { | ||
| Logger.log(`Request to Plotly for creating a grid took ${process.hrtime(startTime)[0]} seconds`, 2); | ||
| if (res.status !== 201) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
No, it'd make the code less readable (in this context it's clear 201 is an HTTP status code).
| }); | ||
| } | ||
| export function getGridMetadata(fid, requestor) { |
There was a problem hiding this comment.
Should we use fid or filename? Above it is filename and now it is fid
There was a problem hiding this comment.
we can use getGridMeta as it is more consistent with Plotly nomenclature. Not a big deal though..
| }); | ||
| } | ||
| export function updateGrid(rows, fid, uids, requestor) { |
| accessToken, | ||
| method: 'GET' | ||
| }).then(function(res) { | ||
| if (res.status === 404) { |
There was a problem hiding this comment.
We should maybe flip the codes into constants with names?
There was a problem hiding this comment.
No, it'd make the code less readable (in this context it's clear 404 is an HTTP status code).
There was a problem hiding this comment.
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/
There was a problem hiding this comment.
@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.
| @@ -226,3 +228,8 @@ export function saveSetting(settingName, settingValue) { | |||
| */ | |||
| fs.writeFileSync(getSetting('SETTINGS_PATH'), YAML.stringify(settingsOnFile)); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I sneaked a fix for this in the PR that implemented the CSV connector. See f3df19f .
n-riesco
commented
May 27, 2018
@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
commented
May 27, 2018
@n-riesco Nope. This is a small detail. Looks good and clean. It is a small detail. I would merge it. |
shannonlal
commented
May 27, 2018
💃 |
* To be consistent with Plotly nomenclature: #444 (comment)
| getQueries, | ||
| saveQuery, | ||
| deleteQuery | ||
| } from './Queries.js'; |
There was a problem hiding this comment.
(Not necessarily in this PR) but we should small-case these filenames as well.. !! Queries, Datastore etc, and remove .js extensions to be consistent.. !!
There was a problem hiding this comment.
I want to keep the extensions, because:
- it's grep-friendly
- we have imports that only differ in the extension (e.g.
code-editor.cssandcode-editor.jsx)
There was a problem hiding this comment.
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.
| return newGrid(uniqueFilename, names, rows, username); | ||
| } | ||
| let startTime; | ||
| // Check if the user even exists | ||
| if (!username || !(apiKey || accessToken)) { |
There was a problem hiding this comment.
We can simplify it:
if !(username || apiKey || accessToken) {
There was a problem hiding this comment.
tricky, but not equivalent: the latter doesn't catch the case when username is defined but both apiKey and accessToken are undefined.
| }); | ||
| }); | ||
| it('can create a grid when it registers a query', function() { |
There was a problem hiding this comment.
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.. !!
tarzzz
left a comment
There was a problem hiding this comment.
Left some comments, but nothing blocking.. !!
💃
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
plotlyAPIRequestinto filebackend/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