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.

Sign up for GitHub

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

Jump to bottom

Multiple variant tags filtering #3502

Merged
merged 31 commits into from
Sep 7, 2023
Merged

Multiple variant tags filtering #3502

Show file tree
Hide file tree
Changes from 15 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
View file
Open in desktop
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
View file
Open in desktop
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: 8 additions & 2 deletions ui/pages/SummaryData/components/SavedVariants.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
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'
Expand Down Expand Up @@ -67,8 +68,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 •
edited
Loading

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 All @@ -82,6 +87,7 @@ const BaseSavedVariants = React.memo(({ loadVariants, geneDetail, ...props }) =>
filters={FILTER_FIELDS}
getUpdateTagUrl={getUpdateTagUrl}
loadVariants={loadVariants}
multiple
additionalFilter={
<StyledForm inline>
<Form.Field
Expand Down
12 changes: 9 additions & 3 deletions ui/shared/components/panel/variants/SavedVariants.jsx
View file
Open in desktop
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 @@ -59,6 +60,7 @@ class SavedVariants extends React.PureComponent {
loadVariants: PropTypes.func,
additionalFilter: PropTypes.node,
tableSummaryComponent: PropTypes.elementType,
multiple: PropTypes.bool,
}

state = { showAllFilters: false }
Expand All @@ -76,14 +78,17 @@ class SavedVariants extends React.PureComponent {
const {
match, tableState, filters, discoveryFilters, totalPages, variantsToDisplay, totalVariantsCount, firstRecordIndex,
tableSummaryComponent, loading, filteredVariantsCount, tagOptions, additionalFilter, updateTableField,
variantExportConfig, loadVariants, error,
variantExportConfig, loadVariants, error, multiple,
} = this.props
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 tagCategoryFilters = tags || (variantGuid ? [] : [(tableState.categoryFilter || ALL_FILTER)])
const firstAppliedTagCategoryFilter = tagCategoryFilters.length > 0 ? tagCategoryFilters[0] : null
const appliedTagCategoryFilter = multiple ? tagCategoryFilters : firstAppliedTagCategoryFilter
Copy link
Collaborator

Choose a reason for hiding this comment

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

this functionality should not be shared, its really page specific and hard to understand whats happening here. Having a dropdownValue prop explicitly passed through here would make the page specific logic much clearer


let shownFilters = (discoveryFilters && appliedTagCategoryFilter === DISCOVERY_CATEGORY_NAME) ?
let shownFilters = (discoveryFilters && firstAppliedTagCategoryFilter === DISCOVERY_CATEGORY_NAME) ?
Copy link
Collaborator

Choose a reason for hiding this comment

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

similar to above, discoveryFilters is page specific so its confusing to have this logic here, it would be better to move this logic into the page-specific component and conditionally pass the correct filters prop

discoveryFilters : filters
const hasHiddenFilters = !showAllFilters && shownFilters.length > MAX_FILTERS
if (hasHiddenFilters) {
Expand Down Expand Up @@ -111,6 +116,7 @@ class SavedVariants extends React.PureComponent {
{`Showing ${shownSummary} ${filteredVariantsCount} `}
<Dropdown
inline
multiple={multiple}
options={tagOptions}
value={appliedTagCategoryFilter}
onChange={this.navigateToTag}
Expand Down
17 changes: 14 additions & 3 deletions ui/shared/components/panel/variants/selectors.js
View file
Open in desktop
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,9 +136,19 @@ export const getPairedSelectedSavedVariants = createSelector(
} else if (tag === MME_TAG_NAME) {
pairedVariants = matchingVariants(pairedVariants, ({ mmeSubmissions = [] }) => mmeSubmissions.length)
} else if (tag && tag !== SHOW_ALL) {
pairedVariants = matchingVariants(
pairedVariants, ({ tagGuids }) => tagGuids.some(tagGuid => tagsByGuid[tagGuid].name === tag),
)
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.

if (tags.length === 1) {
pairedVariants = matchingVariants(
pairedVariants, ({ tagGuids }) => tagGuids.some(tagGuid => tagsByGuid[tagGuid].name === tag),
)
} else {
pairedVariants = matchingVariants(
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
View file
Open in desktop
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