-
Notifications
You must be signed in to change notification settings - Fork 46
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 #4958 - IGV button names and IGV MT overview link #4976
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4976 +/- ##
=======================================
Coverage 84.76% 84.76%
=======================================
Files 320 320
Lines 19359 19359
=======================================
Hits 16410 16410
Misses 2949 2949 ☔ View full report in Codecov by Sentry. |
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.
Nice! Simpler for the users, which will be pleased.
A couple of minor things:
- For consistency, would be nice to deactivate the MT button when the mt_bam are missing (see my comment on the case sidebar)
- Add a tooltip on variant page, explaining the the DNA button us deactivated because the bam files are missing:
scout/server/blueprints/cases/templates/cases/collapsible_actionbar.html
Outdated
Show resolved
Hide resolved
Let me know when you are ready for another review on this one @dnil. I was thinking that perhaps we can merge this and create a new release with what we have today? |
Good points! Both should now be addressed; happy for a new review when you have time! |
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.
Still some issues with the double boolean when the variant is an MT variant. I would simplify the code so it's more readable, even if you repeat the code button twice (the if and the else), check my comment!
@@ -29,6 +30,7 @@ About changelog [here](https://keepachangelog.com/en/1.0.0/) | |||
- STRs variants export | |||
- ClinVar submission enquiry status for all submissions after the latest | |||
|
|||
|
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.
@@ -355,11 +355,17 @@ <h6>Phenotype terms (HPO):</h6> | |||
{% endif %} | |||
</div> | |||
<div class="col-3"> | |||
{% if variant.is_mitochondrial and case.mt_bams or case.bam_files %} |
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.
Perhaps to keep it simple one could keep the original if and else (which were working) and in the case of else put the code for a button which is deactivated and has the tooltip. Because honestly it would be easier to maintain..
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.
Sure, though that will be a bit redundant. I would prefer not to keep this statement, as it is very hard to read. Jinja dont really make there operator priority super clear, and it lacks parenthesis to disambiguate
(variant.is_mitochondrial and case.mt_bams) or case.bam_files
from variant.is_mitochondrial and (case.mt_bams or case.bam_files)
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.
Sure, though that will be a bit redundant. I would prefer not to keep this statement, as it is very hard to read. Jinja dont really make there operator priority super clear, and it lacks parenthesis to disambiguate
(variant.is_mitochondrial and case.mt_bams) or case.bam_files
fromvariant.is_mitochondrial and (case.mt_bams or case.bam_files)
You can also keep it as it was:
else: "No alignments available"
Ok, here goes - a bit more redundant html, but the logic is easier and actually works for all the views and missing/not missing combos instead. You win some and loose some. 😉 |
<div class="panel-default"> | ||
{{ comments_panel(institute, case, current_user, comments) }} | ||
</div> | ||
<div class="col-xs-12 col-md-12">{{ smn_individuals_table(case, institute, tissue_types) }}</div> |
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.
ws
</span> | ||
{% endif %} | ||
{% if case.vcf_files.vcf_sv %} | ||
<span class="d-flex"> |
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.
ws
<span class="me-3"> | ||
<form action="{{url_for('variants.sv_variants', institute_id=institute._id, case_name=case.display_name) }}"> | ||
<input type="hidden" id="hgnc_symbols" name="hgnc_symbols" value="SMN1, SMN2"></input> | ||
<input type="hidden" id="gene_panels" name="gene_panels" value="['']"></input> | ||
<button type="submit" class="btn btn-secondary btn-sm" target="_blank" rel="noopener" data-bs-toggle="tooltip" title="Structural variants view filtered for the genes SMN1 and SMN2">SVs</button></span> | ||
<button type="submit" class="btn btn-secondary btn-sm" target="_blank" rel="noopener" data-bs-toggle="tooltip" title="Structural variants view filtered for the genes SMN1 and SMN2">SVs</button> |
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.
formatting error fix en passant
<div class="col-sm-12">{{activity_panel(events)}}</div> | ||
</div> | ||
<span class="me-3" | ||
{% if not case.bam_files %}title="Alignment file(s) missing" data-bs-toggle="tooltip"{% endif %}> |
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.
We could loop these, but there will only ever be two.
|
||
{{ modal_synopsis() }} | ||
<div class="row"> |
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.
same actual order as before
{% else %} | ||
- BAM file(s) missing | ||
{% endif %} | ||
{% if variant.is_mitochondrial and not case.mt_bams %} |
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.
a little repetitive, but an easier read
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 like it, redability is important
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.
Works great now! 👍🏻
<div href="#" class="bg-dark list-group-item d-inline-block text-white"> | ||
{% if case.bam_files %} | ||
<div href="#" class="bg-dark list-group-item d-inline-block text-white" | ||
{% if not case.bam_files %} data-bs-toggle="tooltip" title="Alignment file(s) missing" {% endif %}> |
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.
👍🏻
{% else %} | ||
- BAM file(s) missing | ||
{% endif %} | ||
{% if variant.is_mitochondrial and not case.mt_bams %} |
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 like it, redability is important
<form action="{{url_for('alignviewers.sashimi_igv', institute_id=case['owner'], case_name=case['display_name']) }}" target="_blank" rel="noopener"> | ||
<button role="submit" data-bs-toggle="tooltip" data-bs-placement="top" title="Available in build GRCh{{ case.rna_genome_build or '38' }}" class="btn btn-xs form-control btn-secondary">R<span class="menu-collapsed">NA splicing</span></button> | ||
</div> | ||
<div href="#" class="bg-dark list-group-item d-inline-block text-white" |
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.
@@ -76,7 +76,7 @@ | |||
disabled" title="Alignment file(s) missing" data-bs-toggle="tooltip" aria-disabled="true"><span class="fa fa-times-circle fa-fw me-1"></span>IGV mtDNA | |||
</a> | |||
</span> | |||
{% elif not case.bam_files %} | |||
{% elif not case.bam_files and not variant.is_mitochondrial %} |
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.
💯
Quality Gate passedIssues Measures |
This PR changes the names on IGV buttons (to IGV DNA, IGV RNA, IGV MT), avoiding the "viewer" tautology and the use of "splicing" when we also have full RNA alignments. Also adds an IGV MT overview button from the collapsible toolbar. Also added a magnifying glass icon for existing data, and use the ring-X icon consistently for missing data.
Partly by following the style on one page, I reverted to disable and tooltip to indicate missing alignments. The tooltips are not really available without viewing source on disabled buttons in the current bootstrap. It would be possible to get them displayed by moving them to regular html title tooltips, or a separate js tooltip class, setting
pointer-events: none;
to avoid the bootstrap pointer block, if we feel this is important.Testing on cg-vm1 server (Clinical Genomics Stockholm)
Prepare for testing
scout-stage
and the server iscg-vm1
.ssh <USER.NAME>@cg-vm1.scilifelab.se
sudo -iu hiseq.clinical
ssh localhost
podman ps
systemctl --user stop scout.target
systemctl --user start scout@<this_branch>
systemctl --user status scout.target
scout-stage
) to be used for testing by other users.Testing on hasta server (Clinical Genomics Stockholm)
Prepare for testing
ssh <USER.NAME>@hasta.scilifelab.se
us; paxa -u <user> -s hasta -r scout-stage
. You can also use the WSGI Pax app available at https://pax.scilifelab.se/.conda activate S_scout; pip freeze | grep scout-browser
bash /home/proj/production/servers/resources/hasta.scilifelab.se/update-tool-stage.sh -e S_scout -t scout -b <this_branch>
us; scout --version
paxa
procedure, which will release the allocated resource (scout-stage
) to be used for testing by other users.How to test:
Expected outcome:
The functionality should be working
Take a screenshot and attach or copy/paste the output.
Review: