-
Notifications
You must be signed in to change notification settings - Fork 0
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
fix to resolve incorrect merging of therapies into treatments #1187
Conversation
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.
Thanks @UmmulkiramR .
Simple solution is good, I am just concerned about the error handling. I acknowledge it should not (or should very rarely) throw an error, but I'd like to make sure we aren't going to end up in a confusing data state if it does occur. Can you comment on how an error case will be resolved?
Object.keys( | ||
activeSubmission.clinicalEntities, | ||
) as (keyof typeof activeSubmission.clinicalEntities)[], |
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'm not going to ask for changes to this type assertion, I think this is reasonably contained so as not to cause us problems, but I wanted to quickly comment on why its necessary.
Object.keys
returns string[]
and not strings with the constant values of the types properties, which is why the assertion is necessary. It does this because TS does not constrain objects to have ONLY the properties of the listed type.
Consider this (very silly) example:
const existingData = { num: 123, str: 'asdf', bool: true };
type SimpleType = { num: number; str: string };
const mySimpleData: SimpleType = existingData;
const dataKeys = Object.keys(mySimpleData);
Here, dataKeys will have the value ["num", "str", "bool"]
even though bool
is not a property of SimpleType
. Thus we can't assert dataKeys as (keyof SimpleType)[]
If we really do need to limit our array to only have values in the key of the type, a filter should be used:
const isKeyofSimpleType = (input: string): input is 'num' | 'str' => ['num', 'str'].includes(input);
const dataKeys = Object.keys(mySimpleData).filter(isKeyofSimpleType);
As written, this will be tough to maintain the type and the filter to always match (risk of code drift with future changes) - a schema library can help with this.
throw new Errors.StateConflict( | ||
`Donor ${donorId} has not been registered but is part of the activeSubmission, merge cannot be completed.`, | ||
); |
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.
Throwing errors from a loop. Do we want the entire process to break, or just skip this step?
I also notice the invoking method does not have a try catch. Will it stop the entire data submission process? What state will it be left in, will it be partially changed and saved?
At very least, we need to log what issue has occurred with some info about the expected consequences and what actions (if any) are needed.
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.
To ensure there is minimal impact on the existing functioning of the code, all this fix does is break up one big method into 2 parts ensuring the treatments are processed before the treatment types. Any error handling that was a part of the original big function has been taken exactly in the 2 new functions. The error being thrown from within the loop was how it was earlier and that wasn't changed to avoid inducing new bugs and ensure all error scenarios are handled exactly the way it was handled before.
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.
Thank you for checking on this and providing the explanation. I will approve it as is.
* 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)
The code will now merge treatment records before merging therapies.