Bug/1056 clinical download #721
Conversation
|
investigating failing build in CI |
| summary: Download clinical data for all available files from filter. | ||
| description: Download clinical data for all available files from filter. |
There was a problem hiding this comment.
thinking this may be a valid nuance?
| summary: Download clinical data for all available files from filter. | |
| description: Download clinical data for all available files from filter. | |
| summary: Download clinical data for all available files from (optional) filters. | |
| description: Download clinical data for all available files from (optional) filters. |
There was a problem hiding this comment.
thinking further on this, the "all" part of this endpoint implies to me no filtering should be possible. I mean why would a query give you partial results, unless it is due to pagination or hard limits?
| @@ -0,0 +1,95 @@ | |||
| import { ADVERTISED_HOST } from 'config'; | |||
There was a problem hiding this comment.
naive question: is config an npm package or an internal module with an absolute path? if the latter, this looks weird in the same set of imports as the rest.
| try { | ||
| const queryClient = createQueryClient({ requestConfig }); | ||
| const queryVariables = { filters: requestFilter }; | ||
| // need two queries to pass "total" into hits filter to specify offset range |
There was a problem hiding this comment.
not sure it's helpful, just sharing unfinished/raw thoughts I got from reading this:
without sufficient context, this comment doesn't really add much clarity, neither of the intent nor the results. that is to say that line 75 is not exactly readable in terms of what the server will do with the values passed in.
"totalData" in contrast to "partialData", "uncertainData" or "doubtfulData"?
There was a problem hiding this comment.
...for instance, line 76 does say "this gets the total number of hits" -> a single absolute numerical value. clear. but totalData would be that "total hits" + what "total" else?
not sure I'm making sense... but the code + comment seem opaque/arcane to me, is ultimately my point.
There was a problem hiding this comment.
ok. rereading these lines a few more times, I think the issue I'm having is in how the code is separated into different lines; and that forces variable creation for every step, which in turn brings out how naming things is hard... cognitive burden multiplied.
so, suggestion: it occurs to me lines 75 - 76 could look more like the following (which I realize mixes then and await), but conceptually explains better the intent:
const totalHits = await queryClient({
query: makeRequest({ query: totalQuery, variables: queryVariables })
}).then(response => get(response, 'data.file.hits.total'));
...maybe query client should also take a callback as an optional param?
"in case you want to do something with the response".
const totalHits = await queryClient({
query: makeRequest({ query: totalQuery, variables: queryVariables }),
callback: response => get(response, 'data.file.hits.total'),
});
this doesn't really explain why the "totalHits" is needed, which is then easier to describe in the comment
| @@ -172,6 +160,63 @@ const createClincalApiRouter = async (esClient: Client, egoClient: EgoClient) => | |||
| }); | |||
| } | |||
| }); | |||
|
|
|||
| router.use('/donors/data-for-all-files', async (req: AuthenticatedRequest, res) => { | |||
There was a problem hiding this comment.
if a filter is passed, this should be read as "'some' data about 'some' of the files".
I think "all" here is either redundant or misleading, unless it actually means "'some' data for all the files", which is not immediately explicit in the name
Description of changes
Type of Change