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

Update study links in results page query summary #2942

Merged
merged 1 commit into from
Dec 23, 2019

Conversation

kalletlak
Copy link
Member

@kalletlak kalletlak commented Dec 19, 2019

samples: Pick<Sample, "sampleId" | "studyId">[],
hasVirtualStudies: boolean,
sampleLists?: Pick<SampleList, "category">[]) {
const hasAllCaseLists = _.some(sampleLists, sampleList => sampleList.category === 'all_cases_in_study');
let studyViewFilterHash: string | undefined;
if (samples.length > 0 && (hasVirtualStudies || !hasAllCaseLists)) {
const sampleIdentifiers = samples.map(sample => ({
sampleId: sample.sampleId,
Copy link
Member

Choose a reason for hiding this comment

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

If we are going to remove studyIds later on in study view, we better use uniqueSampleId here

Copy link
Member Author

Choose a reason for hiding this comment

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

This is equivalent of sampleIdentifiers in StudyViewFilter, which doesn't support uniqueSampleIds

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, I misunderstood the structure. Sorry about that.

@alisman
Copy link
Collaborator

alisman commented Dec 23, 2019

If I do a combined query and select a subset of patients, i see the wrong count in study name.

I think below, the study name should be Combined Study (142 Samples)

image

@kalletlak
Copy link
Member Author

@alisman looks like an existing bug.
@jjgao should I change it to Combined Study (<all samples count> Samples)?

@alisman
Copy link
Collaborator

alisman commented Dec 23, 2019

@kalletlak i'm pretty sure it should be yes. it's exactly the same scenario (where a user has chosen studies and then selected a case list)

@kalletlak
Copy link
Member Author

@alisman updated

@alisman alisman merged commit 745bb58 into cBioPortal:master Dec 23, 2019
@inodb inodb added the bug label Jan 2, 2020
@kalletlak kalletlak deleted the fix-6925 branch July 8, 2024 01:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Study link too long when cohort list represented in json from results page
4 participants