-
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
💊 Feat/1194 Drug Info Validation #1195
Conversation
import { entities as dictionaryEntities } from '@overturebio-stack/lectern-client'; | ||
import { DeepReadonly } from 'deep-freeze'; |
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.
Several alphabetized imports auto-updated
| RadiationFieldsEnum | ||
| SurgeryFieldsEnum | ||
| CommonTherapyFields | ||
| CommonTherapyFieldsEnum |
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.
Standardize variable names
@@ -19,26 +19,28 @@ | |||
|
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.
Main validation logic here
for (const [drugKey, drugValue] of Object.entries(drugFields)) { | ||
if (!drugValue) { | ||
const data = { [drugKey]: drugValue }; | ||
missingFields.push(data); | ||
} | ||
} | ||
|
||
missingFields.forEach((field) => { | ||
const [drugKey] = Object.entries(field)[0]; | ||
const fieldName = drugKey as ClinicalFields; |
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.
Not 100% clear why missingField
is getting an object if the drugValue
is falsy
Later on the missingFields
iterator, drugValue
isn't used.
drugValue
could still be a string "" or number 0 and be falsey
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.
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 think both comments can be addressed by condensing the logic: Condense validation
(passes unit tests)
This is just a block that was not optimized and cleaned up. Extra verbose logic from working out the solution.
Thanks!
} | ||
|
||
missingFields.forEach((field) => { | ||
const [drugKey] = Object.entries(field)[0]; |
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.
why not just name this fieldName
instead of new var assignment?
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.
See above
Link to Issue
#1194
Description
Test files are included in ticket
Checklist
Type of Change
Checklist before requesting review:
develop
not master)validationDependency
in meta tag for Argo Dictionary fields used in code