Skip to content
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

🐛 tumour related fields in Specimen should only be required if specimen is tumour #54

Closed
hknahal opened this issue Mar 3, 2020 · 17 comments
Assignees
Labels
bug Something isn't working

Comments

@hknahal
Copy link
Contributor

hknahal commented Mar 3, 2020

Describe the bug

Currently, the data submitter is expected to submit clinical data for the tumour-related specific fields when the tumour_normal_designation is Normal which is incorrect.

Expected behaviour

The following tumour-related fields should only need to be completed only if tumour_normal_designation is Tumour. If tumour_normal_designation is Normal, then these fields should not be filled in:

pathological_tumour_staging_system
pathological_T_category
pathological_N_category
pathological_M_category
pathological_stage_group
tumour_grading_system
tumour_grade
percent_tumour_cells
percent_proliferating_cells
percent_stromal_cells
percent_necrosis

@kcullion
Copy link

kcullion commented Mar 4, 2020

@hknahal related to this ticket, do you need another ticket to change the "required" to "dependency" in the dictionary?
Screen Shot 2020-03-04 at 9 30 28 AM

@hknahal
Copy link
Contributor Author

hknahal commented Mar 4, 2020

@kcullion - sure, we can make a separate ticket for changing "required" to "dependency". This ticket is more about creating a script validation.

@kcullion
Copy link

kcullion commented Mar 4, 2020

thanks @hknahal I made a ticket and assigned it to you: #56

@wajiha-oicr
Copy link
Contributor

Ok so I just spoke with @blabadi. This is possible, but it would not be done within a script that runs upon upload. These fields sitting in specimen depend on data from another file sample_registration. This can be done upon the validate submission step, but not the upload file step.

@kcullion
Copy link

kcullion commented Mar 4, 2020

@wajiha-oicr ok that sounds good. so will you make them NOT required on upload, then when user hits "validate submission" the submitter will see if they are missing any because it's a tumour?

@wajiha-oicr
Copy link
Contributor

wajiha-oicr commented Mar 4, 2020

I will have to remove the required flag on these fields so the user can continue with their upload, and then ensure the user included these fields ( if and only if the designation is set to tumour) from within the validate submission step
.

@wajiha-oicr
Copy link
Contributor

@wajiha-oicr ok that sounds good. so will you make them NOT required on upload, then when user hits "validate submission" the submitter will see if they are missing any because it's a tumour?

yes exactly

@kcullion
Copy link

kcullion commented Mar 4, 2020

sounds great. Thanks @wajiha-oicr and @blabadi

@wajiha-oicr
Copy link
Contributor

@hknahal so just to be crystal clear, not only are these fields not required when the tumour designation is set to normal, but they shouldn't be filled in at all? I.e. throw an error if someone is providing values for these fields when the corresponding tumour designation is set to normal. Is the correct?

@hknahal
Copy link
Contributor Author

hknahal commented Mar 4, 2020

Yes, if the tumour_normal_designation is Normal, these fields should not be filled in at all (ie. it should throw an error). These fields should only be filled in if tumour_normal_designation is Tumour. This is because these fields are only relevant for tumour specimens (for example, you can only grade and stage tumour specimens)

@wajiha-oicr
Copy link
Contributor

So I was testing my changes, and they work as intended upon the validate submission step. However, I came across an edge case that could potentially be very frustrating for a user:

  • User has registered a sample with a donor whose tumour normal designation set to 'Normal'.
  • User uploads a specimen.tsv file, filling in the details for that donor, and fills out a couple of the fields that they shouldn't be filling out. For example, they fill out the pathological_tumour_staging_system field and set it to AJCC 3rd Edition. They don't fill out pathological_T_category though.
  • They try to upload the file. A script tells them that when pathological_tumour_staging_system is set to an AJCC value, the pathological_T_cateogry is required to be filled in. This script has no knowledge that tumour normal designation was set to normal, so it is rejecting the empty value.
  • The user fills in the value, reuploads the file, and is able to move ahead
  • The user clicks on validate submission
  • The validation tells them that fields were wrongly filled in. It tells them that they were not allowed to fill in pathological_tumour_staging_system, or pathological_T_category.

The user is left confused wondering why they had to first FILL in a value only to be told they weren't allowed to fill in those values.

Since the script has no knowledge about the value of the tumour designation, it is behaving as intended. However I can see this being annoying to a user in these edge cases.

Not sure what can be done about this at the moment.

@kcullion
Copy link

kcullion commented Mar 4, 2020

thanks @wajiha-oicr I see what you mean and it would be frustrating. I have a feeling the submitters would probably know what the specimen designation is (normal or tumour) when filling out the specimen file and probably would know to leave those tumour fields blank for the normals. @hknahal what do you think? Do you think the submitter will just get used to what fields go with normal and what go with tumour?

@hknahal also, was there a reason the tumour_normal_designation was in the sample-registration file and not the specimen file?

@wajiha-oicr
Copy link
Contributor

@kcullion

These are the two error messages I wrote for this ticket. Please provide your feedback on them.

For a missing field

<missing field name> is a required field because the corresponding record in < the reference schema>.tsv had < the reference field name> set to <the reference field value>.

For example: "pathological_tumour_staging_system is a required field because the corresponding record in sample_registration.tsv had tumour_normal_desgination set to Tumour."

For when a value for a field was provided when it shouldn't have been

<provided field name> should not be filled in. This field must be left empty because the corresponding record in < the reference schema>.tsv had <the reference field name> set to <the reference field value>.

For example: "pathological_tumour_staging_system should not be filled in. This field must be left empty because the corresponding record in sample_registration.tsv had tumour_normal_desgination set to Normal."

@kcullion
Copy link

kcullion commented Mar 5, 2020

Thanks @wajiha-oicr! well done :)

I think we have been making them more general and not related to the files. so I would suggest something consistent with the other errors (eg when submitter didn't specify cause of death)

pathological_tumour_staging_system must be provided when the tumour_normal_designation is Tumour.

and

pathological_tumour_staging_system should not be provided when the tumour_normal_designation is Normal.

blabadi added a commit to icgc-argo/argo-clinical that referenced this issue Mar 6, 2020
@kcullion
Copy link

kcullion commented Mar 6, 2020

@wajiha-oicr this is looking really good!
Confirmed with @christinayung - can you make percent_inflammatory_tissue required ONLY for tumour specimen

Thanks

@kcullion
Copy link

kcullion commented Mar 6, 2020

Tested in production and just noticed a couple fields that weren't required if tumour. Made a new ticket icgc-argo/argo-docs#139

@rosibaj
Copy link
Contributor

rosibaj commented Mar 10, 2020

tested on prod - looks great!

@rosibaj rosibaj closed this as completed Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants