Clinical id service integration #1176

UmmulkiramR · 2024-04-15T16:31:08Z

Donor Repository methods have been modified to fetch the entity ids from the ID service before saving the donor in the collection.
ID generator is the one making the calls to the ID service.

demariadaniel · 2024-04-15T17:45:10Z

src/clinical/donor-repo.ts

+	async updateAll(donors: Donor[]) {
+		console.log('donors array in updateAll2: ' + donors[0].submitterId);
+		const newDonors = donors.map(async (donor) => {
+			// await someFunction();


Unused commented code to remove

done. removed.

demariadaniel · 2024-04-15T17:45:21Z

src/clinical/donor-repo.ts

+			),
+		);
+
+		return result; // newDonors.map((donor) =>  donor.then((d) => { return F(MongooseUtils.toPojo(d) as Donor);}).then(res => res));


Here as well

demariadaniel · 2024-04-15T17:46:37Z

src/clinical/id-generator.ts

+
+	try {
+		const response = await axios.get(
+			// `http://localhost:9001/${req.programId}/${req.submitterId}/${req.submitterDonorId}/${req.testInterval}/${req.family_relative_id}/${req.comorbidityTypeCode}/${req.entityType}`,


demariadaniel · 2024-04-15T17:49:19Z

src/decorators/index.ts

@@ -123,11 +123,13 @@ const scopeCheckGenerator = (
 				scopesGenerator(programId),
 				request,
 				response,
-			);
-			if (unauthorizedResponse !== undefined) {
+			); // REMOVE THIS


Seems pretty clear what to do here 😄

demariadaniel · 2024-04-15T17:53:50Z

src/clinical/donor-repo.ts

 		unsetIsNewFlagForUpdate(newDonor);
-
+		console.log('newDonor._id' + newDonor._id);


There are a lot of console.logs added and I'm not sure which are intentional and which are left over from testing

It would be good to replace those you want to keep with the L logger used elsewhere in the app; do a search for const L or L.info for examples

removed all console logs.

demariadaniel · 2024-04-15T17:57:46Z

src/clinical/id-generator.ts

+	entityType: 'null',
+};
+
+export interface PartialDonor {


This basically duplicates type Donor, could we achieve the same thing doing interface PartialDonor extends Partial<Donor> {} or something similar?

that's a good idea. Thanks.

demariadaniel · 2024-04-15T18:04:23Z

src/clinical/id-generator.ts

+		entityType: ClinicalEntitySchemaNames.DONOR,
+	});
+	donor.donorId = donorId;
+	console.log(donor.donorId);


This should print more than just a number, it needs context, at the very least: donor.donorId: ${donor.donorId} similar to the other logs in this function

demariadaniel · 2024-04-15T18:06:10Z

src/submission/submission-api.ts

@@ -313,7 +313,7 @@ class SubmissionController {

 	// GQL Query Methods
 	async commitActiveSubmissionData(programId: string, egoToken: string, versionId: string) {
-		queryHasProgramWriteAccess(programId, egoToken);
+		// queryHasProgramWriteAccess(programId, egoToken); // REMOVE THIS


demariadaniel · 2024-04-15T18:29:13Z

src/clinical/id-generator.ts

+			config.getConfig().tokenUrl() +
+				`${req.programId}/${req.submitterId}/${req.submitterDonorId}/${req.testInterval}/${req.family_relative_id}/${req.comorbidityTypeCode}/${req.entityType}`,
+			headers,
+		);


I would typically define this url in a variable which is then passed to axios.get rather than putting logic in the function arguments

const url = `${config.getConfig().tokenUrl()}/${req.programId}/${req.submitterId}/${req.submitterDonorId}/${req.testInterval}/${req.family_relative_id}/${req.comorbidityTypeCode}/${req.entityType}`; const response = await axios.get(url, headers);

Also note I added a trailing slash between tokenUrl and the string of paths req.programId/ etc ... if youre defining the tokenUrl with a slash then this is not needed

done. Thanks.

demariadaniel · 2024-04-15T18:47:02Z

src/clinical/id-generator.ts

+				`${req.programId}/${req.submitterId}/${req.submitterDonorId}/${req.testInterval}/${req.family_relative_id}/${req.comorbidityTypeCode}/${req.entityType}`,
+			headers,
+		);
+		console.log('getId response: ' + response.data.entityId + ' - ' + response.data.entityType);


Just FYI string concatenation like this is generally out of favour compared to template literals

`getId response: ${response.data.entityId} - ${response.data.entityType}`

String + value (concatenation) can sometimes have unexpected results when parsing a value that is not originally a string
Template literals are also slightly cleaner to read + write

have removed all unwanted console logs

Dan's suggestion, for clarity

Suggested change

console.log('getId response: ' + response.data.entityId + ' - ' + response.data.entityType);

console.log(`getId response: ${response.data.entityId} - ${response.data.entityType}`);

demariadaniel · 2024-04-15T18:58:17Z

src/clinical/id-generator.ts

+				submitterDonorId: bm.clinicalInfo.submitter_donor_id as string,
+				testInterval: bm.clinicalInfo.test_interval as string,
+				entityType: ClinicalEntitySchemaNames.BIOMARKER,
+			});


When grabbing many values from the same object you can use destructuring variable assignment to simplify things:

const { submitter_donor_id, submitter_specimen_id, submitter_primary_diagnosis_id, submitter_follow_up_id, submitter_treatment_id, test_interval, } = bm.clinicalInfo; const submitterBiomarkerId = submitter_specimen_id?.toString() ?? submitter_primary_diagnosis_id?.toString() ?? submitter_follow_up_id?.toString() ?? submitter_treatment_id?.toString() ?? 'null'; const id = await getId({ ...request, programId: donor.programId as string, submitterId: submitterBiomarkerId, submitterDonorId: submitter_donor_id as string, testInterval: test_interval as string, entityType: ClinicalEntitySchemaNames.BIOMARKER, });

Also generally prefer descriptive variable names -- biomarker vs bm

I have changed bm to biomarker and for all other entities. But I'd prefer not to use the destructured variable assignment here for now

demariadaniel

Functionality looks good but there's some clean up to do and some style recommendations before merging

ciaranschutte · 2024-04-28T00:09:21Z

@demariadaniel @UmmulkiramR

!!'null' // true
!!null // false

justincorrigible · 2024-04-29T14:43:04Z

src/clinical/id-generator.ts

+}
+
+const request = {
+	programId: 'null',


As discussed over zoom, this was made as a conscious choice, WIP in the context of query params in http requests, geared towards representing the difference between empty values for a param (null), vs missing parameters altogether (undefined). Will be addressed in this PR.

have replaced the the null with a '-'

demariadaniel · 2024-05-01T18:22:23Z

src/clinical/id-generator.ts

+}
+
+export async function setEntityIds(donor: PartialDonor) {
+	const submitterDonorId = donor.submitterId as string;


One more thing I want to take a look at, I think we can update PartialDonor like this:

export interface PartialDonor extends Partial<Donor> { submitterId: string; programId: string; }

and then you can remove repeated use of as string in this function and elsewhere. These two keys are typically required keys in Donor so I think it fits your usage.

We try to avoid using as because it can create situations where you're misleading Typescript. For example, donor.submitterId could actually be undefined and TS wouldn't know.

Tagging @justincorrigible (may want to ask Jon?) because as usage is something we've discussed frequently before so thought I'd get a second opinion

* develop: fix to resolve incorrect merging of therapies into treatments (#1187) Chore / Move Clinical Service Types (#1185) ❌ Sort Invalid Clinical Records first (#1184) RC 1.87.1 Remove unused argument (#1179) 🏷️ 1141 Add Exception Manifest to Donor tsv (#1178) Fixed Resolver (#1177) Add Exceptions to Program TSV (#1175)

joneubank

Generally workable solution. The ID path is shockingly long and I am wondering if this is a configuration/design concern with the ID service instead of a problem with the implementation here.

My only feedback for changes is to listen to the TS errors and perform required type checks instead of just asserting new types. TS can't provide us any meaningful protection against invalid types if we keep telling it that it is wrong.

src/clinical/id-generator.ts

joneubank · 2024-06-26T14:36:36Z

src/clinical/id-generator.ts

+		const url =
+			config.getConfig().idServiceUrl() +
+			`${req.programId}/${req.submitterDonorId}/${req.submitterSpecimenId}/${req.submitterSampleId}/${req.submitterPrimaryDiagnosisId}/${req.submitterFollowUpId}/${req.submitterTreatmentId}/${req.testInterval}/${req.family_relative_id}/${req.comorbidityTypeCode}/${req.entityType}`;


Strongly encourage the use of URL building libraries for this specific purpose: https://www.npmjs.com/package/url-join

See also the JS URL constructor: https://developer.mozilla.org/en-US/docs/Web/API/URL/URL

src/clinical/id-generator.ts

joneubank · 2024-06-26T14:38:39Z

src/clinical/id-generator.ts

+interface IdGenerationRequest {
+	programId: string;
+	submitterSpecimenId: string;
+	submitterSampleId: string;
+	submitterPrimaryDiagnosisId: string;
+	submitterFollowUpId: string;
+	submitterTreatmentId: string;
+	submitterDonorId: string;
+	testInterval: string;
+	family_relative_id: string;
+	comorbidityTypeCode: string;
+	entityType: string;
+}
+
+const request = {
+	programId: '-',
+	submitterSpecimenId: '-',
+	submitterSampleId: '-',
+	submitterPrimaryDiagnosisId: '-',
+	submitterFollowUpId: '-',
+	submitterTreatmentId: '-',
+	submitterDonorId: '-',
+	testInterval: '-',
+	family_relative_id: '-',
+	comorbidityTypeCode: '-',
+	entityType: '-',
+};


Probably just want to use a Partial version of this object and then let the consuming function convert the undefined values to whatever palceholder string we want. This is simpler than requiring the user submitting the request to understand that '-' is a placeholder value.

src/clinical/id-generator.ts

joneubank · 2024-06-26T14:40:44Z

src/clinical/id-generator.ts

+	const submitterDonorId = donor.submitterId as string;
+	// -- DONOR --
+	const donorId = await getId({
+		...request,


This relates to my previous comment about using a Partial type for input. You can do this ...request filling inside of the getId function so that we don't need to remember to do this when calling the function.

src/clinical/id-generator.ts

src/utils.ts

joneubank · 2024-06-26T14:51:16Z

src/clinical/donor-repo.ts

 export const donorDao: DonorRepository = {
 	async insertDonors(donors: Donor[]) {
-		await mongoose.connection.db.collection('donors').insertMany(donors);
+		const donorsWithIds = await setEntityIdsForDonors(donors);
+		await mongoose.connection.db.collection('donors').insertMany(donorsWithIds);
 	},
 	async updateDonor(donor: Donor) {
+		const donorsWithIds = await setEntityIds(donor);


Both setEntityIds functions throw errors when network errors occur, should there be some sort of error handling here to handle failures or is this now expected to be handled by the caller of updateDonor and insertDonors?

src/clinical/id-generator.ts

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

UmmulkiramR added 4 commits April 9, 2024 07:27

added id generation logic

dddca11

added id generation logic

c4b472d

updated donor-repo calls

1ae27f8

added config values for token and id-service url

ec4a991

UmmulkiramR requested a review from demariadaniel April 15, 2024 16:31

demariadaniel reviewed Apr 15, 2024

View reviewed changes

demariadaniel requested changes Apr 15, 2024

View reviewed changes

justincorrigible reviewed Apr 29, 2024

View reviewed changes

made changes to incorporate review feedback

f893255

UmmulkiramR requested review from demariadaniel and justincorrigible April 29, 2024 20:46

demariadaniel reviewed May 1, 2024

View reviewed changes

UmmulkiramR linked an issue May 15, 2024 that may be closed by this pull request

Integrate ID generation with the Clinical Service. #1094

Open

UmmulkiramR added 4 commits June 11, 2024 10:25

pkg json update

1a7ff29

missing return statement

7221bbc

changed some error handling during id generation

cb48085

UmmulkiramR requested a review from joneubank June 25, 2024 14:16

joneubank reviewed Jun 26, 2024

View reviewed changes

review updates

79d9f6b

justincorrigible reviewed Aug 12, 2024

View reviewed changes

src/clinical/id-generator.ts Outdated Show resolved Hide resolved

Update src/clinical/id-generator.ts

40e926e

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

joneubank self-requested a review August 12, 2024 13:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clinical id service integration #1176

Clinical id service integration #1176

UmmulkiramR commented Apr 15, 2024

demariadaniel Apr 15, 2024

UmmulkiramR Apr 29, 2024

demariadaniel Apr 15, 2024

demariadaniel Apr 15, 2024

demariadaniel Apr 15, 2024

demariadaniel Apr 15, 2024

UmmulkiramR Apr 29, 2024

demariadaniel Apr 15, 2024

UmmulkiramR Apr 29, 2024

demariadaniel Apr 15, 2024 •

edited

Loading

demariadaniel Apr 15, 2024

demariadaniel Apr 15, 2024

UmmulkiramR Apr 29, 2024

demariadaniel Apr 15, 2024

UmmulkiramR Apr 29, 2024

justincorrigible Aug 12, 2024

demariadaniel Apr 15, 2024 •

edited

Loading

demariadaniel Apr 15, 2024

UmmulkiramR Apr 29, 2024

demariadaniel left a comment

ciaranschutte commented Apr 28, 2024

justincorrigible Apr 29, 2024

UmmulkiramR Apr 29, 2024

demariadaniel May 1, 2024

joneubank left a comment

joneubank Jun 26, 2024

joneubank Jun 26, 2024

joneubank Jun 26, 2024

joneubank Jun 26, 2024

		unsetIsNewFlagForUpdate(newDonor);

		console.log('newDonor._id' + newDonor._id);

	console.log('getId response: ' + response.data.entityId + ' - ' + response.data.entityType);
	console.log(`getId response: ${response.data.entityId} - ${response.data.entityType}`);

Clinical id service integration #1176

Are you sure you want to change the base?

Clinical id service integration #1176

Conversation

UmmulkiramR commented Apr 15, 2024

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

demariadaniel Apr 15, 2024 • 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

demariadaniel Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demariadaniel left a comment

Choose a reason for hiding this comment

ciaranschutte commented Apr 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joneubank left a 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

demariadaniel Apr 15, 2024 •

edited

Loading

demariadaniel Apr 15, 2024 •

edited

Loading