ClinGen / clincoded

This GCI/VCI 1.0 platform has now been retired, and replaced with our new 2.0 platform:
https://github.com/ClinGen/gene-and-variant-curation-tools/issues
MIT License
25 stars 9 forks source link

VCI Expert Review and Approval - Provisional Interpretation #1423

Closed jimmyzhen closed 6 years ago

jimmyzhen commented 7 years ago

Implement the Provisional Interpretation section, which consists of the following requirements:

  1. Section panel title: Provisional Interpretation
  2. A dropdown of by Individual or on behalf of Group options for the user to select.
  3. Dropdown label: Interpretation being saved 'by Individual' or 'on behalf of Group'
  4. Display of form fields for labels ClinGen Group Name OR Non-ClinGen Group Name. Either one of these fields is required if the on behalf of Group option is selected.
  5. A Save as Provisional Interpretation button for the user to save the selected option from the dropdown.
  6. The dropdown selection is required if the user wants to save the provisional interpretation.
  7. Display of a note with message: Note: Saving as a Provisional Interpretation does not submit it to ClinVar.
  8. Display of the Current Saved Provisional Interpretation, including the timestamp.
jimmyzhen commented 6 years ago

@selinad, @wrightmw,

I've addressed the timestamp and word wrapping issues, and have updated the instance. Please review again: https://jz-approval-workflow.demo.clinicalgenome.org

selinad commented 6 years ago

@jimmyzhen thank you!

Timestamp appears to update on last edit of an evalution (not summary), but this is good if not the best.

I still see wrapping -- not critical, IMHO, for this release -- a long title will be unusual, but I'm not certain the time involved to address image

selinad commented 6 years ago

@jimmyzhen I take it back - it's also wrapping with the shorter titles:

image

jimmyzhen commented 6 years ago

@selinad,

I suspect this is due to the missing CSS in the test instance. It is looking okay in my local instance: image

selinad commented 6 years ago

@jimmyzhen looks fantastic! Thanks for checking it on your local instance. The release candidate will have the missing CSS, correct?

jimmyzhen commented 6 years ago

@selinad,

Yes, it will.

wrightmw commented 6 years ago

Very minor point... in the Provisional/Approval process the field for Affiliations is named "ClinGen Affiliation" but we may wish to change this in future to just "Affiliation"... we already have some Affiliations that are not "ClinGen" and in time there will no doubt be many more.

jimmyzhen commented 6 years ago

@wrightmw Please create a ticket for this so that we can address it in one of the subsequent releases. Thank you.

selinad commented 6 years ago

@jimmyzhen unfortunatley @wrightmw and I both get this message image

when clicking on "View Provisional Summary" after saving a Provisional in the VCI image

selinad commented 6 years ago

@jimmyzhen one thing I now can't remember whether I noticed or is newly introduced.

When you have this screen (just saved a summary and there IS an associated disease) image

If you hit Edit, you get this: image

The good news is the ability to save as Provisional disappears, but the need a disease note appears even though one already exists. It's' not a big deal as when I hit Save again, it gives me provisional option....I think we can fix it for next time unless it is "quick" to remove this button.

selinad commented 6 years ago

@jimmyzhen this is my bad for not catching earlier -- the display here is a bit confusing:

image

Would it be quick to make the Provisional/Approved Status and tag be on the same line as the rest of the info for the interpretation? Or, the "Provisional/Approved Status:" title could be removed and the "Approve" button could go tight after the datestamp on the same line.

If neither of these quick, I'll make a ticket for next time -- I don't think there will be too many variants with multiple interpreters.

selinad commented 6 years ago
selinad commented 6 years ago

@jimmyzhen everything looks great except for the 2 items (and it sounds like one is only because the new VCI Summary page didn't get added to the rc). The other item (disease needed message showing when click Edit on summary) is not critical, but if quick to fix would be less confusing....It seems like anytime the edit is hit, no warning should appear as it should only happen on Save as needed.

Putting in needs work and you can decide. thx @jimmyzhen -- this was a HUGE project done very nicely! THANK YOU!!!

jimmyzhen commented 6 years ago

@selinad @wrightmw,

I have fixed the view provisional/approval summary problem in the VCI. Please try again and let me know there are any problems. Thanks.

selinad commented 6 years ago

@jimmyzhen it works!! Thank you !

image

jimmyzhen commented 6 years ago

@selinad,

If we are gonna make the VCI status(es) inline with the timestamp, they may wrap to the next line (e.g. the APPROVED is in one line and the NEW PROVISIONAL is in the next) when you have something like the screenshot below: image

Would you still prefer to make status(es) inline with timestamp?

selinad commented 6 years ago

thx @jimmyzhen -- would it be quick to slightly increase the space between interpretations for now?

jimmyzhen commented 6 years ago

@selinad It will be slightly quicker.

selinad commented 6 years ago

Implemented in R18 -- great work @jimmyzhen !!!!