Feat/#1186 disable cross field validation for survival time #1214

ciaranschutte · 2024-11-06T12:16:40Z

Link to Issue

icgc-argo/roadmap#1186

Description

disables time conflict validation on follow_up, specimen and treatment if exception exists

Checklist

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

ciaranschutte · 2024-11-06T12:17:17Z

src/submission/exceptions/exceptions.ts

+export const checkForExceptions = async (
+	record: DeepReadonly<SubmittedClinicalRecord>,
+	field: string,
+	schema: string,
+): Promise<boolean> => {
+	const programId = record['program_id'] as string;
+
+	const programAdditionalSearchParams = {
+		exceptions: { requested_core_field: field },
+	};
+
+	const entityAdditionalSearchParams = {
+		[`${schema}.requested_core_field`]: field,
+	};
+
+	// retrieve submitted exceptions for program id (both program level and entity level)
+	const programException = await programExceptionRepository.find(
+		programId,
+		programAdditionalSearchParams,
+	);
+	const entityException = await entityExceptionRepository.find(
+		programId,
+		entityAdditionalSearchParams,
+	);
+
+	return notEmpty(programException) || notEmpty(entityException);
+};


check if program or entity exception exists

ciaranschutte · 2024-11-06T12:18:06Z

src/submission/validation-clinical/followUp.ts

@@ -24,13 +24,82 @@ import {
 	SubmissionValidationOutput,
 	DonorVitalStatusValues,
 } from '../submission-entities';
-import { ClinicalEntitySchemaNames, FollowupFieldsEnum } from '../../common-model/entities';
+import {


changes here mirror changes to other validation files.
big diff hash because I moved an inline function out of a function body for consistency

src/submission/validation-clinical/followUp.ts

src/submission/exceptions/exceptions.ts

ciaranschutte · 2024-11-13T02:16:22Z

CI failing due to tests. #1216 should fix it.

joneubank

Here are my comments about the types used in the code provided, this is mostly about having your functions be more strict/opinionated about their interface/parameters.

joneubank · 2024-11-12T19:42:14Z

src/exception/property-exceptions/repo/entity.ts

@@ -66,6 +66,8 @@ const EntityExceptionModel =
 	mongoose.models.EntityException ||
 	mongoose.model<EntityException>('EntityException', entityExceptionSchema);

+type OptionalSearchParams = Record<string, string>;


Why do we need this type alias? This is used in only one place, so we could just write Record<string, string> inline.

joneubank · 2024-11-12T19:49:43Z

src/exception/property-exceptions/repo/program.ts

+type OptionalSearchParams = {
+	exceptions: Partial<{
+		requested_core_field: string;
+	}>;
+};


Again this is only used in one place, why not declare it inline?

The use of Partial could be smart if we expect this to expand, but maybe it would be simpler to just make this:

optionalSearchParams?: { requested_core_field?: string }

Since we don't currently need the extra layer in the object and there is only 1 property.

This comment is really similar to the comment on the repo's find function, where what is important is the interface that we export. This interface as written looks like it is trying to match the shape of data from another source, instead of requesting only data that is meaningful to this function's context.

joneubank · 2024-11-12T19:55:30Z

src/exception/property-exceptions/repo/entity.ts

+	async find(
+		programId: string,
+		optionalSearchParams?: OptionalSearchParams,
+	): Promise<EntityException | null> {


The optionalSearchParams parameter, being a record of any strings, is very loose for this interface - there is no structure or guidance for what a caller should be sending here.

Reading through your implementation, you are directly sending the properties from this object into the mongo search query. That means the caller, coming from outside the repo file, needs to know about the structure of the repo for these queries to be useful.

I would consider thinking about what parameters a developer might want to send here and how to ensure that those values will always result in a valid query into the database. For our current task, this might be a field like requested_core_field?: string

src/submission/exceptions/exceptions.ts

joneubank · 2024-11-12T20:37:26Z

src/submission/exceptions/exceptions.ts

+export const checkForExceptions = async (
+	record: DeepReadonly<SubmittedClinicalRecord>,
+	field: string,
+	schema: string,


Because both field and schema just accept a string, there is a possibility of a developer mixing up the order of these. It may be helpful to make this function take an object with named properties as an input. This will also improve readability when calling this function.

Compare for example:

// not clear that the arguments are in the wrong order checkForExceptions(myTreatmentRecord, 'treatment', 'treatment_type'); // vs: checkForExceptions({ record: myTreatmentRecord, schema: 'treatment', field: 'treatment_type' });

joneubank · 2024-11-15T15:53:38Z

I am proposing a change to the expected behaviour detailed in the ticket. Instead of having our cross file validations check for the survival_time exception, we should just check if there is a survival_time value. If there is no value, then we don't apply the checks (they would always fail). If there is no value and also no exception, then the validation would fail in the donor entity validation, so we don't need to worry about that in the time interval cross validations.

I'll add a comment to the issue to the same effect and create a new PR into this branch with the proposed changes.

ciaranschutte · 2024-11-18T16:21:51Z

tested locally.

demariadaniel · 2024-11-18T18:34:35Z

src/submission/validation-clinical/utils.ts

-		errors.push(
-			buildSubmissionError(
-				submittedRecord,
-				DataValidationErrors.NOT_ENOUGH_INFO_TO_VALIDATE,


Source of testing errors -- this is removed and not replaced elsewhere. The tests are looking for the NOT_ENOUGH_INFO_TO_VALIDATE error and it's not found.

…it test

ciaranschutte · 2024-11-18T18:58:53Z

@joneubank please see updated unit tests. I believe these updates match what we discussed in so far as the interval checks are on donor schema itself.

ciaranschutte commented Nov 6, 2024

View reviewed changes

ciaranschutte requested review from joneubank and justincorrigible November 6, 2024 12:20

demariadaniel reviewed Nov 7, 2024

View reviewed changes

src/submission/validation-clinical/followUp.ts Show resolved Hide resolved

demariadaniel reviewed Nov 7, 2024

View reviewed changes

src/submission/validation-clinical/followUp.ts Show resolved Hide resolved

demariadaniel reviewed Nov 7, 2024

View reviewed changes

src/submission/exceptions/exceptions.ts Show resolved Hide resolved

ciaranschutte requested a review from demariadaniel November 13, 2024 08:05

joneubank reviewed Nov 15, 2024

View reviewed changes

ciaranschutte requested a review from joneubank November 18, 2024 16:23

justincorrigible changed the title ~~Feat/#1186 disable cross file for surivival time~~ Feat/#1186 disable cross field validation for survival time Nov 18, 2024

Ciaran Schutte and others added 11 commits November 18, 2024 11:43

add program exception check

0c8faf1

add check for exception in specimen validation

ce250d5

fix conditional search param object creation

adfa693

add exception lookup check to treatment

8398bd9

add follow_up exception check

af27a92

add entity exception check also

2bafd2c

fix type error

8348018

handling if program_id is undefined

74e8d8d

Skip time interval checks when survival_time value is missing

fa5f018

cleanup exception code

636b573

fix TS build error

ceb65c0

ciaranschutte force-pushed the feat/#1186_disable_cross_file_for_surivival_time branch from 8e1859d to ceb65c0 Compare November 18, 2024 16:45

demariadaniel reviewed Nov 18, 2024

View reviewed changes

Ciaran Schutte added 2 commits November 18, 2024 13:46

remove specimen_acquisition_interval from not info validation info un…

d4e05b0

…it test

remove survival_time assertion on follow up unit test

823d444

joneubank approved these changes Nov 18, 2024

View reviewed changes

ciaranschutte merged commit c4aa428 into develop Nov 18, 2024
2 checks passed

ciaranschutte deleted the feat/#1186_disable_cross_file_for_surivival_time branch November 18, 2024 21:32

ciaranschutte mentioned this pull request Nov 18, 2024

RC/1.88.0 #1218

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/#1186 disable cross field validation for survival time #1214

Feat/#1186 disable cross field validation for survival time #1214

ciaranschutte commented Nov 6, 2024 •

edited

Loading

ciaranschutte Nov 6, 2024

ciaranschutte Nov 6, 2024 •

edited

Loading

ciaranschutte commented Nov 13, 2024

joneubank left a comment

joneubank Nov 12, 2024

joneubank Nov 12, 2024

joneubank Nov 12, 2024

joneubank Nov 12, 2024

joneubank commented Nov 15, 2024 •

edited

Loading

ciaranschutte commented Nov 18, 2024

demariadaniel Nov 18, 2024

ciaranschutte commented Nov 18, 2024

Feat/#1186 disable cross field validation for survival time #1214

Feat/#1186 disable cross field validation for survival time #1214

Conversation

ciaranschutte commented Nov 6, 2024 • edited Loading

Link to Issue

Description

Checklist

Type of Change

Checklist before requesting review:

ciaranschutte Nov 6, 2024

Choose a reason for hiding this comment

ciaranschutte Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

ciaranschutte commented Nov 13, 2024

joneubank left a comment

Choose a reason for hiding this comment

joneubank Nov 12, 2024

Choose a reason for hiding this comment

joneubank Nov 12, 2024

Choose a reason for hiding this comment

joneubank Nov 12, 2024

Choose a reason for hiding this comment

joneubank Nov 12, 2024

Choose a reason for hiding this comment

joneubank commented Nov 15, 2024 • edited Loading

ciaranschutte commented Nov 18, 2024

demariadaniel Nov 18, 2024

Choose a reason for hiding this comment

ciaranschutte commented Nov 18, 2024

ciaranschutte commented Nov 6, 2024 •

edited

Loading

ciaranschutte Nov 6, 2024 •

edited

Loading

joneubank commented Nov 15, 2024 •

edited

Loading