Case Spreadsheet Download - #722
Conversation
Also, refactor spreadsheet flow of information and add spreadsheet streaming.
Also, add VariantSampleSpreadsheet headers and note embedding.
… bm-case-spreadsheet-ui
…ap-portal into bm-case-spreadsheet-ui
…ap-portal into bm-case-spreadsheet-ui
Bm case spreadsheet UI
…ap-portal into drr_case_spreadsheet
Also, comment out columns that could never be retrieved for VariantSamples from search.
willronchetti
left a comment
willronchetti
left a comment
There was a problem hiding this comment.
I have no obvious objection to this, just some relatively small comments that may add up in total. Kudos to you for really going above and beyond with tests though, those look great.
I would say though some of this can probably be moved to snovault for easier bootstrapping for such things in smaht-portal. Your call if you want to take this on now or leave it for another time.
| CASE_SPREADSHEET_ENDPOINT = "case_search_spreadsheet" | ||
| CASE_SPREADSHEET_URL = format_to_url(CASE_SPREADSHEET_ENDPOINT) | ||
| VARIANT_SAMPLE_SPREADSHEET_ENDPOINT = "variant_sample_search_spreadsheet" | ||
| VARIANT_SAMPLE_SPREADSHEET_URL = format_to_url(VARIANT_SAMPLE_SPREADSHEET_ENDPOINT) |
There was a problem hiding this comment.
Structurally speaking, I would consider refactoring the route configuration and moving a common implementation into snovault ie: /spreadsheet/{type_name} - that way you can write implementers in downstream applications for various types while making the overall logic available across portals.
Ultimately I'm going to suggest we merge this as developed into CGAP but refactor the core components of it into snovault so it can be re-used in SMaHT.
| def get_case_rows( | ||
| items_for_spreadsheet: Iterable[JsonObject], | ||
| ) -> Iterator[Iterable[str]]: | ||
| return CaseSpreadsheet(items_for_spreadsheet).yield_rows() | ||
|
|
||
|
|
||
| def get_spreadsheet_response( | ||
| file_name: str, spreadsheet_rows: Iterator[List[str]], file_format: str | ||
| ) -> Response: | ||
| return SpreadsheetGenerator( | ||
| file_name, spreadsheet_rows, file_format=file_format | ||
| ).get_streaming_response() |
There was a problem hiding this comment.
I won't make this comment everywhere but docstrings would be helpful to at least give high level info, no need to do argument information as well though as I think between type annotations and good naming you are covered there.
| result = [] | ||
| case_accession = self.spreadsheet_request.get_case_accession() | ||
| if case_accession: | ||
| result.append(["#", "Case Accession:", "", case_accession]) |
There was a problem hiding this comment.
This type of CSV structure generation is a good candidate for another helper (it is repeated in several places below)
| result.append(["#", "Filters Selected:", "", readable_filter_blocks]) | ||
| return result | ||
|
|
||
| def _get_row_for_item(self, item_to_evaluate: JsonObject) -> List[str]: |
There was a problem hiding this comment.
What is JsonObject getting you over dict?
| ("ID", "URL path to the variant", "@id"), | ||
| ("Chrom (hg38)", "Chromosome (hg38)", "variant.CHROM"), | ||
| ("Pos (hg38)", "Start position (hg38)", "variant.POS"), | ||
| ("Chrom (hg19)", "Chromosome (hg19)", "variant.hg19_chr"), | ||
| ("Pos (hg19)", "Start position (hg19)", "variant.hg19_pos"), | ||
| ("Ref", "Reference Nucleotide", "variant.REF"), | ||
| ("Alt", "Alternate Nucleotide", "variant.ALT"), |
There was a problem hiding this comment.
This should definitely be pulled from schema no?
| @dataclass(frozen=True) | ||
| class Item: | ||
| ATID = "@id" | ||
| PROJECT = "project" | ||
|
|
||
| properties: JsonObject | ||
|
|
||
| @property | ||
| def _atid(self) -> str: | ||
| return self.properties.get(self.ATID, "") | ||
|
|
||
| @property | ||
| def _project(self) -> LinkTo: | ||
| return self.properties.get(self.PROJECT, "") | ||
|
|
||
| def get_properties(self) -> JsonObject: | ||
| return self.properties | ||
|
|
||
| def get_atid(self) -> str: | ||
| return self._atid | ||
|
|
||
| def get_project(self) -> LinkTo: | ||
| return self._project |
There was a problem hiding this comment.
Good candidate for snovault/extension here in CGAP
| @dataclass(frozen=True) | ||
| class VariantConsequence(Item): | ||
| # Schema constants | ||
| IMPACT = "impact" | ||
| IMPACT_HIGH = "HIGH" | ||
| IMPACT_LOW = "LOW" | ||
| IMPACT_MODERATE = "MODERATE" | ||
| IMPACT_MODIFIER = "MODIFIER" | ||
| VAR_CONSEQ_NAME = "var_conseq_name" | ||
|
|
There was a problem hiding this comment.
At a meta level what are you trying to accomplish? It feels like you are duplicating very specific details of the data model to achieve some structure... I worry following/maintaining this structure will be burdensome, though maybe not when the data model is relatively stable.
| sortBy, sortColumns, | ||
| hiddenColumns, addHiddenColumn, removeHiddenColumn, columnDefinitions | ||
| hiddenColumns, addHiddenColumn, removeHiddenColumn, columnDefinitions, | ||
| requestedCompoundFilterSet |
There was a problem hiding this comment.
Name requestedCompoundFilterSet is somewhat redundant no? requestedFilterSet I think would be fine.
| EXPECTED_VARIANT_SAMPLE_SPACER_ROW = [ | ||
| "## -------------------------------------------------------" | ||
| ] |
There was a problem hiding this comment.
Should this not be an imported constant?
| @view_config( | ||
| name='spreadsheet', | ||
| context=VariantSampleList, | ||
| request_method='GET', | ||
| permission='view', | ||
| validators=[validate_spreadsheet_file_format], | ||
| ) | ||
| @debug_log | ||
| def variant_sample_list_spreadsheet(context: VariantSampleList, request: Request): |
There was a problem hiding this comment.
Why have this one here with the others in batch_download.py? I honestly feel they may be better suited here since they are directly data model related.
3 participants
Here, we add the ability to download a spreadsheet for Case items from a search.
Previous code for spreadsheet downloads for VariantSamples was heavily refactored to provide more modular, tested, shared code for the two spreadsheets as well as any future spreadsheets required.
Additional features include:
_statsto subrequests (PR)item_models.py) containing classes for selected items to obtain select properties. This could be more broadly useful and may deserve its own repo so can be imported outside the portal. Thoughts?search/compound_search.pyto share constants and expand filter set search to a request with only global flags.