-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat/multi query management #255
base: develop
Are you sure you want to change the base?
Conversation
The only other thing besides my comments above is that you might want to write and test query updates now, since you're already doing most of the work, vs later. |
models/Query.mjs
Outdated
constructor({ | ||
id = null, | ||
pk, | ||
status = 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make the statuses constants via some mechanism like export QUERY_STATUS = Object.freeze({NEW: 0, RUNNING: 1, ...});
services/QueryService.mjs
Outdated
} | ||
|
||
async getQueryById(qid) { | ||
return this._queryStore.retrieveUserById(qid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cut and paste error? should be retrieveQueryBy...
services/QueryService.mjs
Outdated
} | ||
|
||
async getQueryByPk(pk) { | ||
return this._queryStore.retrieveUserByPk(pk); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
Also, we would now need to make POST /query a privileged request, but keep the GETs unprivileged. We should probably address that in this PR also. |
No description provided.