Added simplified cohort sample UI.#2373
Conversation
|
Back-end has been merged (so the endpoints are now available in WebAPI. Need a review on the UI side. |
|
The create sample form validation creates some errors now. Expected behaviour: Situation 1: no values have been filled and no button has been pressed. Expected behaviour: All fields are empty, no warnings are shown. Situation 2. Sampling without age criteria Expected behaviour: if no value is filled for age criteria, a sample is created with no age criteria applied. |
|
Hello, Good catch on the second one, I misunderstood the validation logic for a required field. I can fix that. |
|
Ok, I fixed the optional age range validation, and also noticed that the validation called for non-equal values, so I added that check too. Thanks for checking! |
| this.patientCountError=ko.pureComputed(() => !(this.patientCount() > 0)); // this works because null == 0 | ||
| this.isAgeRange =ko.pureComputed(() => ['between','notBetween'].includes(this.sampleAgeType())); | ||
| this.firstAgeError = ko.pureComputed(() => this.firstAge() != null && this.firstAge() < 0); | ||
| this.isAgeRangeError = ko.pureComputed(() => this.isAgeRange() && (this.firstAgeError() || (this.secondAge() != null && this.secondAge() < 0) || this.firstAge() == this.secondAge())); |
There was a problem hiding this comment.
Not sure if this is encoded, it think that another age range condition is firstAge <= secondAge.
There was a problem hiding this comment.
Actually, the age order doesn't matter (I was surprised to find this) that's why the validation error message says that it's an error if they are equal. When the form is submitted, the smaller value is put as firstAge, and the larger is secondAge.
There was a problem hiding this comment.
I retested with the new configuration, there's still some issues there
- If one age field is provided in
betweenornotBetween, the form accepts the input, but the backend rejects it because it requires two ages. I think this input should be rejected in the form. - If first the "between" dropdown is selected, the second input is filled, and then "Less than" is selected with no text input, the data still includes an age criterion
"age":{"value":null,"mode":"lessThan","min":null,"max":null}. If no age is provided in the current dropdown also no age criterion should be sent to the backend ("age": null). - The behaviour of the age selection would be more consistent if it either allowed any dropdown option without any input to indicate no age criterion (this does not work for between/notBetween), or to have a dedicated "Any age" dropdown option.
- If no age was selected, the sample table shows "Random age" as age selection. Perhaps "Any age" would be more appropriate. Likewise, instead of "Mix", "Any gender" could be shown.
| this.newSampleCreatingLoader(true); | ||
| sampleService.createSample(payload, {cohortDefinitionId, sourceKey}) | ||
| .then(res => { | ||
| if(res.ok) { |
There was a problem hiding this comment.
If the server rejects the input, res is undefined and the console throws an error that res.ok does not exist. The following would avoid an error:
| if(res.ok) { | |
| if(res && res.ok) { |
|
Thanks for the hints, I'll work on these changes. |
Use await and try..catch to handle errors.
|
Ok, I've made the changes you've suggested. This includes moving to an 'async..await' form of handling promises so that try..catch works predictably. The issue with prior code was how .catch() handlers were being attached to the promises, leading to then() being invoked even in case of error. Now, the then() will only get fired when the request was successful, and the catch() and finally() will be handled within he context of the await. |
|
I'm hoping that the final reviews are completed, and can get sign-off on this PR. Please let me know if you have any additional comments, as I'm anxious/excited to close this out. |
|
Yes, looks good to me. Thanks for your hard work in making this suitable for merge! |
3 participants
Depends on OHDSI/WebAPI#1657.
Thank you Andries Purnal (Andries.Purnal@ucb.com), Florence Giesen (Florence.Giesen@ucb.com), @MaximMoinat and The Hyve for their implementation, which this PR is based on.
This is the simplified cohort sample functionality which allows a sample to be pulled from a source, and then navigated to the current patient profile UI.