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

Multiple variant tags filtering #3502

Merged
merged 31 commits into from
Sep 7, 2023
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
71e905d
Add multiple variant tags filtering feature.
ShifaSZ Jul 20, 2023
424e66b
Fix codacy and ALL filter.
ShifaSZ Jul 20, 2023
cc216dc
Merge branch 'dev' into multiple-variant-tag-filtering
ShifaSZ Jul 25, 2023
f13dc6c
Update category and all option controls.
ShifaSZ Jul 26, 2023
ab47e9c
Update tests.
ShifaSZ Jul 26, 2023
f3b94bf
Remove unused imports.
ShifaSZ Jul 26, 2023
48f0111
Update reducer to prevent reloading.
ShifaSZ Jul 27, 2023
de8cbd4
Correct clear loaded tags.
ShifaSZ Jul 27, 2023
83e3979
Use a constant for delimiter and update db filter.
ShifaSZ Aug 3, 2023
fed9cdd
Merge branch 'dev' into multiple-variant-tag-filtering
ShifaSZ Aug 3, 2023
ee46678
Update multiple tags DB queries and tests.
ShifaSZ Aug 3, 2023
5162ae9
A better state key generation.
ShifaSZ Aug 3, 2023
561822e
Update the TAG_OPTIONS constant.
ShifaSZ Aug 4, 2023
303981c
Merge branch 'dev' into multiple-variant-tag-filtering
ShifaSZ Aug 7, 2023
b45b050
Remove project saved-variant multi-tag filtering.
ShifaSZ Aug 7, 2023
6b7a039
Update the option value and filters props.
ShifaSZ Aug 9, 2023
7ea4f23
Save the variants to the state per tag.
ShifaSZ Aug 10, 2023
e46151c
Update the getSelectedTag function.
ShifaSZ Aug 10, 2023
f05bc63
Merge branch 'dev' into multiple-variant-tag-filtering
ShifaSZ Aug 10, 2023
eacc42c
fix tests.
ShifaSZ Aug 11, 2023
b2e440e
Update the per-tag saved-variant data state.
ShifaSZ Aug 11, 2023
72711c2
Add tests for the selector for the summary data.
ShifaSZ Aug 11, 2023
de1927b
Merge branch 'dev' of https://github.com/broadinstitute/seqr into mul…
hanars Sep 5, 2023
03185c6
clean up selected tag logic
hanars Sep 6, 2023
5072223
Merge branch 'dev' of https://github.com/broadinstitute/seqr into mul…
hanars Sep 6, 2023
fae6d42
update multi tag summary data selector
hanars Sep 6, 2023
664394f
clean up selectors
hanars Sep 6, 2023
fb288fe
fix tests
hanars Sep 6, 2023
cc04e2c
diff cleanup
hanars Sep 6, 2023
00e37d4
diff cleanup
hanars Sep 6, 2023
91d3959
better display and behavior
hanars Sep 6, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions seqr/views/apis/summary_data_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,11 @@ def saved_variants_page(request, tag):
if tag == 'ALL':
saved_variant_models = SavedVariant.objects.exclude(varianttag=None)
else:
tag_type = VariantTagType.objects.get(name=tag, project__isnull=True)
saved_variant_models = SavedVariant.objects.filter(varianttag__variant_tag_type=tag_type)
tags = tag.split(';')
hanars marked this conversation as resolved.
Show resolved Hide resolved
tag_type = VariantTagType.objects.filter(name__in=tags, project__isnull=True)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be tag_types plural, as it is a collection of models not a single model

saved_variant_models = SavedVariant.objects.all()
for tt in tag_type:
saved_variant_models = saved_variant_models.filter(varianttag__variant_tag_type=tt).distinct()

saved_variant_models = saved_variant_models.filter(family__project__guid__in=get_project_guids_user_can_view(request.user))

Expand Down
15 changes: 12 additions & 3 deletions seqr/views/apis/summary_data_api_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,11 @@ def test_saved_variants_page(self):
expected_variant_guids.add('SV0000002_1248367227_r0390_100')
self.assertSetEqual(set(response.json()['savedVariantsByGuid'].keys()), expected_variant_guids)

multi_tag_url = reverse(saved_variants_page, args=['Review;Tier 1 - Novel gene and phenotype'])
response = self.client.get('{}?gene=ENSG00000135953'.format(multi_tag_url))
self.assertEqual(response.status_code, 200)
self.assertSetEqual(set(response.json()['savedVariantsByGuid'].keys()), {'SV0000001_2103343353_r0390_100'})
hanars marked this conversation as resolved.
Show resolved Hide resolved

def test_hpo_summary_data(self):
url = reverse(hpo_summary_data, args=['HP:0002011'])
self.check_require_login(url)
Expand Down Expand Up @@ -231,14 +236,18 @@ class LocalSummaryDataAPITest(AuthenticationTestCase, SummaryDataAPITest):
MANAGER_VARIANT_GUID = 'SV0000006_1248367227_r0004_non'


def assert_has_expected_calls(self, users, skip_group_call_idxs=None):
def assert_has_expected_calls(self, users, skip_group_call_idxs=None, has_ws_access_level_call=False):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function is only used twice so theres no reused behavior for mock_get_ws_access_level at all anymore - it wold be better to take that check out of the helper and explicitly add the correct check for it after calling this helper

calls = [mock.call(user) for user in users]
self.mock_list_workspaces.assert_has_calls(calls)
group_calls = [call for i, call in enumerate(calls) if i in skip_group_call_idxs] if skip_group_call_idxs else calls
self.mock_get_groups.assert_has_calls(group_calls)
self.mock_get_ws_acl.assert_not_called()
self.mock_get_group_members.assert_not_called()
self.mock_get_ws_access_level.assert_not_called()
if has_ws_access_level_call:
self.mock_get_ws_access_level.assert_called_with(
self.analyst_user, 'my-seqr-billing', 'anvil-1kg project nåme with uniçøde')
else:
self.mock_get_ws_access_level.assert_not_called()

# Test for permissions from AnVIL only
class AnvilSummaryDataAPITest(AnvilAuthenticationTestCase, SummaryDataAPITest):
Expand All @@ -254,4 +263,4 @@ def test_saved_variants_page(self):
super(AnvilSummaryDataAPITest, self).test_saved_variants_page()
assert_has_expected_calls(self, [
self.no_access_user, self.manager_user, self.manager_user, self.analyst_user, self.analyst_user
], skip_group_call_idxs=[2])
], skip_group_call_idxs=[2], has_ws_access_level_call=True)
10 changes: 7 additions & 3 deletions ui/pages/Project/components/SavedVariants.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
VARIANT_PER_PAGE_FIELD,
EXCLUDED_TAG_NAME,
REVIEW_TAG_NAME,
TAG_URL_DELIMITER,
} from 'shared/utils/constants'
import UpdateButton from 'shared/components/buttons/UpdateButton'
import { LargeMultiselect, Dropdown } from 'shared/components/form/Inputs'
Expand Down Expand Up @@ -142,12 +143,15 @@ class BaseProjectSavedVariants extends React.PureComponent {
project.variantTagTypes.map(type => type.category).filter(category => category),
)]

const isCategory = categoryOptions.includes(newTag)
updateTableField('categoryFilter')(isCategory ? newTag : null)
const lastNewTag = newTag.length > 0 ? newTag[newTag.length - 1] : null
const isCategory = categoryOptions.includes(lastNewTag)
const [firstTag, ...otherTag] = newTag
const updatedTag = firstTag === ALL_FILTER || categoryOptions.includes(firstTag) ? otherTag : newTag
updateTableField('categoryFilter')(isCategory ? lastNewTag : null)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behaviors include: if the latest selected option is a category, remove all selected options; otherwise if the previous (or the first) selected option is a category, remove the category option.
The issue is that when a category is selected, the category disappears from the option list.
No category is selected:

After a category (Collaboration) is selected:

Copy link
Collaborator

@hanars hanars Aug 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if a category is selected, all the tags in the category should be shown as selected and should be added to the list of selected tag breadcrumbs, the category itself should remain in the dropdown and clicking it again will have no effect

Also, selected tags need to be the color of the tag, similar to how they behave in the UI for adding tags to a variant

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the category itself should remain in the dropdown and clicking it again will have no effect

How about the category also disappear from the dropdown list? It is much easier to implement and it doesn't make sense to click on a selected category.

return getSavedVariantsLinkPath({
projectGuid: project.projectGuid,
analysisGroupGuid: match.params.analysisGroupGuid,
tag: !isCategory && newTag !== ALL_FILTER && newTag,
tag: !isCategory && lastNewTag !== ALL_FILTER && (updatedTag || []).join(TAG_URL_DELIMITER),
familyGuid: match.params.familyGuid,
})
}
Expand Down
36 changes: 9 additions & 27 deletions ui/pages/SummaryData/components/SavedVariants.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,19 @@ import { Form, Button } from 'semantic-ui-react'

import { getGenesById } from 'redux/selectors'
import {
REVIEW_TAG_NAME,
KNOWN_GENE_FOR_PHENOTYPE_TAG_NAME,
VARIANT_SORT_FIELD,
VARIANT_PER_PAGE_FIELD,
VARIANT_TAGGED_DATE_FIELD,
SHOW_ALL,
TAG_URL_DELIMITER,
} from 'shared/utils/constants'
import { StyledForm } from 'shared/components/form/FormHelpers'
import AwesomeBar from 'shared/components/page/AwesomeBar'
import SavedVariants from 'shared/components/panel/variants/SavedVariants'
import { HorizontalSpacer } from 'shared/components/Spacers'

import { loadSavedVariants, updateAllProjectSavedVariantTable } from '../reducers'
import { SUMMARY_PAGE_SAVED_VARIANT_TAGS } from '../constants'

const GENE_SEARCH_CATEGORIES = ['genes']

Expand All @@ -29,29 +29,7 @@ const FILTER_FIELDS = [
VARIANT_PER_PAGE_FIELD,
]

const TAG_OPTIONS = [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what was the advantage of moving this? The constant is only ever used here so it is better to keep it in the file. In general, PRs should really focus on changing the behavior needed for the actual functional change, adding unrelated changes makes reviewing the actual functional changes harder

Copy link
Contributor Author

@ShifaSZ ShifaSZ Aug 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you are right. I used the tag options for the reducer to update the tag-loaded state before my latest update. We don't need it now.

'Tier 1 - Novel gene and phenotype',
'Tier 1 - Novel gene for known phenotype',
'Tier 1 - Phenotype expansion',
'Tier 1 - Phenotype not delineated',
'Tier 1 - Novel mode of inheritance',
'Tier 1 - Known gene, new phenotype',
'Tier 2 - Novel gene and phenotype',
'Tier 2 - Novel gene for known phenotype',
'Tier 2 - Phenotype expansion',
'Tier 2 - Phenotype not delineated',
'Tier 2 - Known gene, new phenotype',
KNOWN_GENE_FOR_PHENOTYPE_TAG_NAME,
REVIEW_TAG_NAME,
'Send for Sanger validation',
'Sanger validated',
'Sanger did not confirm',
'Confident AR one hit',
'Analyst high priority',
'seqr MME (old)',
'Submit to Clinvar',
'Share with KOMP',
].map(name => ({
const TAG_OPTIONS = SUMMARY_PAGE_SAVED_VARIANT_TAGS.map(name => ({
value: name,
text: name,
key: name,
Expand All @@ -67,8 +45,12 @@ TAG_OPTIONS.push({

const PAGE_URL = '/summary_data/saved_variants'

const getUpdateTagUrl =
(selectedTag, match) => `${PAGE_URL}/${selectedTag}${match.params.gene ? `/${match.params.gene}` : ''}`
const getUpdateTagUrl = (selectedTag, match) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A multiple dropdown should have a list as its value, and it should therefore handle adding or removing elements to that list smoothly, and you should not be writing this sort of custom logic

Copy link
Contributor Author

@ShifaSZ ShifaSZ Aug 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the custom logic is about the SHOW_ALL option. We need to remove the existing SHOW_ALL when adding a new option and remove all current options if the last option is SHOW_ALL.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

show all should not act like a multi-option - there should be no way to select "show all" and also select other tags. Is there a reason users should even have the show all option anymore on the summary page? Presumably, if they send a request with no tags we will return results for all tags, so just have "All" be something thats shown by default when the tag list is empty should be suufficient

const lastTag = selectedTag.length > 0 ? selectedTag[selectedTag.length - 1] : null
const [firstTag, ...otherTag] = selectedTag
const updatedTag = firstTag === SHOW_ALL ? otherTag : lastTag !== SHOW_ALL && selectedTag
return `${PAGE_URL}/${(updatedTag || [SHOW_ALL]).join(TAG_URL_DELIMITER)}${match.params.gene ? `/${match.params.gene}` : ''}`
}

const getGeneHref = tag => selectedGene => `${PAGE_URL}/${tag || SHOW_ALL}/${selectedGene.key}`

Expand Down
28 changes: 25 additions & 3 deletions ui/pages/SummaryData/constants.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
/* eslint-disable import/prefer-default-export */

import React from 'react'
import { Link } from 'react-router-dom'

import { successStoryTypeDisplay } from 'shared/utils/constants'
import { successStoryTypeDisplay, KNOWN_GENE_FOR_PHENOTYPE_TAG_NAME, REVIEW_TAG_NAME } from 'shared/utils/constants'

const formatIDLink =
row => <Link to={`/project/${row.project_guid}/family_page/${row.family_guid}`} target="_blank">{row.family_id}</Link>
Expand All @@ -20,3 +18,27 @@ export const SUCCESS_STORY_COLUMNS = [
{ name: 'success_story', content: 'Success Story', style: { minWidth: '564px' } },
{ name: 'discovery_tags', content: 'Discovery Tags', format: formatDiscoveryTags, noFormatExport: true, style: { minWidth: '400px' } },
]

export const SUMMARY_PAGE_SAVED_VARIANT_TAGS = [
'Tier 1 - Novel gene and phenotype',
'Tier 1 - Novel gene for known phenotype',
'Tier 1 - Phenotype expansion',
'Tier 1 - Phenotype not delineated',
'Tier 1 - Novel mode of inheritance',
'Tier 1 - Known gene, new phenotype',
'Tier 2 - Novel gene and phenotype',
'Tier 2 - Novel gene for known phenotype',
'Tier 2 - Phenotype expansion',
'Tier 2 - Phenotype not delineated',
'Tier 2 - Known gene, new phenotype',
KNOWN_GENE_FOR_PHENOTYPE_TAG_NAME,
REVIEW_TAG_NAME,
'Send for Sanger validation',
'Sanger validated',
'Sanger did not confirm',
'Confident AR one hit',
'Analyst high priority',
'seqr MME (old)',
'Submit to Clinvar',
'Share with KOMP',
]
7 changes: 4 additions & 3 deletions ui/pages/SummaryData/reducers.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { combineReducers } from 'redux'

import { loadingReducer, createSingleValueReducer, createSingleObjectReducer } from 'redux/utils/reducerFactories'
import { RECEIVE_DATA, REQUEST_SAVED_VARIANTS } from 'redux/utils/reducerUtils'
import { SHOW_ALL, SORT_BY_XPOS } from 'shared/utils/constants'
import { SHOW_ALL, SORT_BY_XPOS, TAG_URL_DELIMITER } from 'shared/utils/constants'
import { HttpRequestHelper } from 'shared/utils/httpRequestHelper'

// action creators and reducers in one file as suggested by https://github.com/erikras/ducks-modular-redux
Expand Down Expand Up @@ -43,8 +43,9 @@ export const loadSuccessStory = successStoryTypes => (dispatch) => {

export const loadSavedVariants = ({ tag, gene = '' }) => (dispatch, getState) => {
// Do not load if already loaded
const stateKey = `${tag ? tag.split(TAG_URL_DELIMITER).sort().join(TAG_URL_DELIMITER) : ''}${gene}`
if (tag) {
if (getState().savedVariantTags[tag]) {
if (getState().savedVariantTags[stateKey]) {
return
}
} else if (!gene) {
Expand All @@ -57,7 +58,7 @@ export const loadSavedVariants = ({ tag, gene = '' }) => (dispatch, getState) =>
if (tag && !gene) {
dispatch({
type: RECEIVE_SAVED_VARIANT_TAGS,
updates: { [tag]: true },
updates: { [stateKey]: true },
})
}
dispatch({ type: RECEIVE_DATA, updatesById: responseJson })
Expand Down
9 changes: 6 additions & 3 deletions ui/shared/components/panel/variants/SavedVariants.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { getSavedVariantsIsLoading, getSavedVariantsLoadingError } from 'redux/s
import {
DISCOVERY_CATEGORY_NAME,
VARIANT_PAGINATION_FIELD,
TAG_URL_DELIMITER,
} from 'shared/utils/constants'

import ExportTableButton from '../../buttons/ExportTableButton'
Expand Down Expand Up @@ -65,7 +66,7 @@ class SavedVariants extends React.PureComponent {

navigateToTag = (e, data) => {
const { history, getUpdateTagUrl, match } = this.props
history.push(getUpdateTagUrl(data.value, match))
history.push(getUpdateTagUrl(data.value.length > 0 ? data.value : [ALL_FILTER], match))
}

showAllFilters = () => {
Expand All @@ -81,9 +82,10 @@ class SavedVariants extends React.PureComponent {
const { showAllFilters } = this.state
const { familyGuid, variantGuid, tag } = match.params

const appliedTagCategoryFilter = tag || (variantGuid ? null : (tableState.categoryFilter || ALL_FILTER))
const tags = tag ? tag.split(TAG_URL_DELIMITER) : tag
const appliedTagCategoryFilter = tags || (variantGuid ? [] : [(tableState.categoryFilter || ALL_FILTER)])

let shownFilters = (discoveryFilters && appliedTagCategoryFilter === DISCOVERY_CATEGORY_NAME) ?
let shownFilters = (discoveryFilters && appliedTagCategoryFilter === [DISCOVERY_CATEGORY_NAME]) ?
discoveryFilters : filters
const hasHiddenFilters = !showAllFilters && shownFilters.length > MAX_FILTERS
if (hasHiddenFilters) {
Expand Down Expand Up @@ -111,6 +113,7 @@ class SavedVariants extends React.PureComponent {
{`Showing ${shownSummary} ${filteredVariantsCount} `}
<Dropdown
inline
multiple
options={tagOptions}
value={appliedTagCategoryFilter}
onChange={this.navigateToTag}
Expand Down
7 changes: 6 additions & 1 deletion ui/shared/components/panel/variants/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
VARIANT_SORT_LOOKUP,
SHOW_ALL,
VARIANT_EXPORT_DATA,
TAG_URL_DELIMITER,
} from 'shared/utils/constants'
import {
getVariantTagsByGuid, getVariantNotesByGuid, getSavedVariantsByGuid, getAnalysisGroupsByGuid, getGenesById, getUser,
Expand Down Expand Up @@ -135,8 +136,12 @@ export const getPairedSelectedSavedVariants = createSelector(
} else if (tag === MME_TAG_NAME) {
pairedVariants = matchingVariants(pairedVariants, ({ mmeSubmissions = [] }) => mmeSubmissions.length)
} else if (tag && tag !== SHOW_ALL) {
const tags = tag.split(TAG_URL_DELIMITER)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't doing the split here means all the above logic for MME/notes would not work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MME Submission/Has Notes won't work for multiple-tag filtering. They are not required since they are not on the summary data options list. But we should have a more systemic solution for handling tag/tags here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we filter the tags in the backend and don't support the project saved-variant multiple-tag filtering, these changes are unnecessary.

pairedVariants = matchingVariants(
pairedVariants, ({ tagGuids }) => tagGuids.some(tagGuid => tagsByGuid[tagGuid].name === tag),
pairedVariants, ({ tagGuids }) => {
const tagNames = tagGuids.map(tagGuid => tagsByGuid[tagGuid].name)
return tags.every(tagName => tagNames.includes(tagName))
},
)
} else if (!(familyGuid || analysisGroupGuid)) {
pairedVariants = matchingVariants(pairedVariants, ({ tagGuids }) => tagGuids.length)
Expand Down
2 changes: 2 additions & 0 deletions ui/shared/utils/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -1042,6 +1042,8 @@ export const KNOWN_GENE_FOR_PHENOTYPE_TAG_NAME = 'Known gene for phenotype'
export const DISCOVERY_CATEGORY_NAME = 'CMG Discovery Tags'
export const MME_TAG_NAME = 'MME Submission'

export const TAG_URL_DELIMITER = ';'

export const SORT_BY_FAMILY_GUID = 'FAMILY_GUID'
export const SORT_BY_XPOS = 'XPOS'
const SORT_BY_PATHOGENICITY = 'PATHOGENICITY'
Expand Down
Loading