API Endpoint to Download TSV Files for donors by Donor ID #1062

joneubank · 2023-09-30T21:17:05Z

Description of changes

This adds a new API endpoint at POST /clinical/donors/tsv which accepts in the request body an array of donor IDs. It will collect all entity data for the requested donors and return that data in a Zip file with TSVs of each clinical entity.

This endpoint is restricted to Full Read Access only, requiring service level read access.

This will be used by the UI/API for users to request Clinical Data for files/donors that they have embargo level access to. Additional authorization restrictions for this request are managed by the Platform API Gateway.

Type of Change

Bug
Refactor
New Feature
Release Candidate

Checklist before requesting review:

Check branch (code change PRs go to develop not master)
Check copyrights for new files
Manual testing
Regression tests completed and passing (double check number of tests).
Spelling has been checked.
Updated swagger docs accordingly (check it's still valid)
Set validationDependency in meta tag for Argo Dictionary fields used in code

joneubank · 2023-09-30T21:19:11Z

src/clinical/clinical-service.ts

-export type PaginatedClinicalQuery = ClinicalDonorEntityQuery & {
+export type PaginationQuery = {
  page: number;
  pageSize?: number;
  sort: string;
 };

+export type PaginatedClinicalQuery = ClinicalDonorEntityQuery & PaginationQuery;


We had a need to separate the a worker thread task to accept Pagination parameters separate from the Clinical Donor Entity query data, so we can separate out that data for reference to its own type PaginationQuery.

joneubank · 2023-09-30T21:30:19Z

src/clinical/clinical-service.ts

  const taskToRun = WorkerTasks.ExtractEntityDataFromDonors;
-  const taskArgs = [donors as Donor[], totalDonors, allSchemasWithFields, query];
+  const taskArgs = [donors as Donor[], totalDonors, allSchemasWithFields, query.entityTypes, query];


the ExtractEntityDataFromDonors task was updated to require the PaginatedQuery in place of the ClinicalDonorEntityQuery since the new request by DonorId doesn't include much of that request data. The only additional information used from that query type was the list of entities to include in the request, which is now passed as a separate argument query.entityTypes.

I see the type for query implies the property (entityTypes) is guaranteed to be present.
is it safe to assume its actual presence is validated on runtime elsewhere?

joneubank · 2023-10-01T01:38:42Z

src/clinical/api/clinical-api.ts

+    const date = currentDateFormatted();
+    const fileName = `filename=Donor_Clinical_Data_${date}.zip`;


This is sharing the naming convention used for for downloads requested by program submitters. I'm open to suggestions to improve the provided filename. Perhaps a timestamp is a good idea in addition to the current date.

aside from the filename format itself, would it make sense to abstract this into an env variable?

aside from the filename format itself, would it make sense to abstract this into an env variable?

I don't see a reason to have different filenames per env, what's your reason? If you want filename constanta centralized into a constants file I can do that.

Tbh, I'm just used to abstracting constants like this one, because that makes it easier for these values to be updated, without having to rebuild the code "just for a string change".

If you believe this doesn't need to be abstracted, I of course trust your insight.

Good reason. will make the change.

justincorrigible · 2023-10-02T17:04:08Z

src/clinical/api/clinical-api.ts

 } from './types';
+import { loggerFor } from '../../logger';
+const L = loggerFor(__filename);


Could Log, or Logger do the same job, while being a bit more declarative than a single capital letter l?

yes and it would be better. I'm being consistent with the existing pattern in this repo. I would - recommend a separate action to rename them all.

justincorrigible · 2023-10-02T17:09:42Z

src/clinical/api/clinical-api.ts

+
+    const zip = createClinicalZipFile(donorEntityData);
+
+    res.send(zip.toBuffer());


funny, I'd have imagined you'd get an error/warning here saying that you had already sent a response in line 170.
obviously I must be mistaken.

previous use of res didn't send data, just set some headers and status.

yeah... I was under the impression res.status sends a response.
time for me to do some reading and update my understanding.

follow up: https://expressjs.com/en/api.html#res.status
looks like I've been away from coding for too long, starting to forget how to 😢
...I miss creating things

src/clinical/clinical-service.ts

Co-authored-by: Anders Richardsson <[email protected]>

justincorrigible · 2023-10-02T17:45:32Z

src/clinical/clinical-service.ts

  return data;
 };

+export const getDonorEntityData = async (donorIds: number[]) => {
+  const allSchemasWithFields = await dictionaryManager.instance().getSchemasWithFields();


no need to do anything about this one, just felt the need to acknowledge out loud how nervous it makes me to have these long chains without ?. 😆

long optional chaining makes me nervous too.
I actually really like how dictionaryManager does this - throws an error if there's no instance - so you could actually just catch the error and return ... empty array of donorIds in that case?

that node pattern of throw/catch actual errors vs just retuning nulls/undefined is used a lot in this repo and I think it works well.

re ? part of this.... getSchemaWIthFields will never actually be run on undefined, an error will be thrown instead

welp! sounds like that requires a lot of "insider" knowledge on that dictionaryManager.

try+catch sure does sound desirable, but I guess my non-evident point is that this line is a bit "code golf"ey, and could use being broken into separate lines 🤷‍♂️

don't need insider knowledge... the type system informs us of what the output will be. if we can trust the type system then we're safe here.

oh fun! didn't realise TS (intellisense) was visible in PR reviews at Github.com

must get myself that browser extension!

sarcasm acknowledged. agreed that the lack of introspection in the browser code review is limiting.

to be fair, we could review PRs by testing them locally... like we used to back in the day.

Modern tooling has become a crutch for what could be more declarative; which proves hard to keep up in the review process. How do you suggest we improve that?

src/clinical/clinical-service.ts

justincorrigible · 2023-10-02T17:48:19Z

src/clinical/donor-repo.ts

@@ -392,7 +393,17 @@ export const donorDao: DonorRepository = {
      submitterId: { $in: submitterIds },
      programId: programId,
    });
-    const mapped = result.map((d: DonorDocument) => {
+    const mapped = result.map(d => {
+      return MongooseUtils.toPojo(d) as Donor;


you, using as? le wut!

Old code pattern in the db repo module, there are better TS ways to do this now using mongoose. Following convention of the file instead of investing time in a re-write of the DB management.

screenshotted to use without context in the future (context provider is undefineed) :p

Co-authored-by: Anders Richardsson <[email protected]>

justincorrigible · 2023-10-02T17:50:15Z

src/clinical/donor-repo.ts

+    const mapped = result.map(d => {
+      return MongooseUtils.toPojo(d) as Donor;
+    });
+    return F(mapped);


what on earth is F!?

...I know it's just the established pattern, and it's obviously a function, but what does it do!?
without having to change the whole app, at least the import could be renamed in this file.

rewrites the type as DeepReadonly<T>. Again, following the service convention, I do not recommend adopting this pattern elsewhere.

import { F as Freeze } should help... but even then, what's freezing? 😮‍💨

can't update/extend properties dynamically
if I remember correctly it's a very light lib around the native Object.freeze() https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/freeze

justincorrigible

LGTM (except for the deeper "pattern" issues pointed out in the comments, which are beyond the scope of this PR)

joneubank added 4 commits September 30, 2023 16:55

Remove noisey startup logging

51f1376

Repo can fetch donors by ID without program parameter

f12255b

Clinical Service and Task support to get Donors by ID

155bd74

Clinical API Endpoint to download clinical TSVs by Donor ID

fd25c45

joneubank commented Sep 30, 2023

View reviewed changes

joneubank commented Oct 1, 2023

View reviewed changes

joneubank requested review from demariadaniel, justincorrigible and UmmulkiramR October 2, 2023 16:17

justincorrigible reviewed Oct 2, 2023

View reviewed changes

src/clinical/clinical-service.ts Outdated Show resolved Hide resolved

remove unnecessary await on returned promise

53552af

Co-authored-by: Anders Richardsson <[email protected]>

justincorrigible reviewed Oct 2, 2023

View reviewed changes

src/clinical/clinical-service.ts Show resolved Hide resolved

justincorrigible reviewed Oct 2, 2023

View reviewed changes

Line spacing for readability

da8f597

Co-authored-by: Anders Richardsson <[email protected]>

justincorrigible reviewed Oct 2, 2023

View reviewed changes

justincorrigible approved these changes Oct 2, 2023

View reviewed changes

joneubank merged commit 64bcd34 into develop Oct 6, 2023

joneubank deleted the feat/donor-download-mixed-programs branch October 6, 2023 18:20

demariadaniel mentioned this pull request Oct 13, 2023

Rc/1.84.0 #1069

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API Endpoint to Download TSV Files for donors by Donor ID #1062

API Endpoint to Download TSV Files for donors by Donor ID #1062

joneubank commented Sep 30, 2023

joneubank Sep 30, 2023

joneubank Sep 30, 2023 •

edited by justincorrigible

Loading

justincorrigible Oct 2, 2023

joneubank Oct 1, 2023

justincorrigible Oct 2, 2023 •

edited

Loading

joneubank Oct 2, 2023

justincorrigible Oct 2, 2023

joneubank Oct 2, 2023

justincorrigible Oct 2, 2023

joneubank Oct 2, 2023

justincorrigible Oct 2, 2023

joneubank Oct 2, 2023

justincorrigible Oct 2, 2023

justincorrigible Oct 2, 2023 •

edited

Loading

justincorrigible Oct 2, 2023

ciaranschutte Oct 4, 2023

ciaranschutte Oct 4, 2023

justincorrigible Oct 4, 2023

joneubank Oct 4, 2023 •

edited

Loading

justincorrigible Oct 4, 2023 •

edited

Loading

joneubank Oct 4, 2023

justincorrigible Oct 4, 2023

justincorrigible Oct 2, 2023

joneubank Oct 2, 2023 •

edited

Loading

ciaranschutte Oct 4, 2023

justincorrigible Oct 2, 2023

joneubank Oct 2, 2023

justincorrigible Oct 2, 2023

ciaranschutte Oct 4, 2023

justincorrigible left a comment

		const date = currentDateFormatted();
		const fileName = `filename=Donor_Clinical_Data_${date}.zip`;


		const zip = createClinicalZipFile(donorEntityData);

		res.send(zip.toBuffer());

API Endpoint to Download TSV Files for donors by Donor ID #1062

API Endpoint to Download TSV Files for donors by Donor ID #1062

Conversation

joneubank commented Sep 30, 2023

Choose a reason for hiding this comment

joneubank Sep 30, 2023 • edited by justincorrigible Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justincorrigible Oct 2, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justincorrigible Oct 2, 2023 • 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

joneubank Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

justincorrigible Oct 4, 2023 • 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

joneubank Oct 2, 2023 • 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

justincorrigible left a comment

Choose a reason for hiding this comment

joneubank Sep 30, 2023 •

edited by justincorrigible

Loading

justincorrigible Oct 2, 2023 •

edited

Loading

justincorrigible Oct 2, 2023 •

edited

Loading

joneubank Oct 4, 2023 •

edited

Loading

justincorrigible Oct 4, 2023 •

edited

Loading

joneubank Oct 2, 2023 •

edited

Loading