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

Redirect the linkout endpoint "ln" to curated non-redundant list of studies #3335

Merged
merged 2 commits into from
Jul 29, 2020

Conversation

adamabeshouse
Copy link
Contributor

No description provided.

Adam Abeshouse added 2 commits July 28, 2020 17:30
…quick select button config

Signed-off-by: Adam Abeshouse <abeshoua@mskcc.org>
Signed-off-by: Adam Abeshouse <abeshoua@mskcc.org>
);
if (curatedNonRedundantStudyIdsArray) {
// filter out studies that user doesnt have access to
const allStudies = await client.getAllStudiesUsingGET({});
Copy link
Collaborator

Choose a reason for hiding this comment

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

way to anticipate this. did you manage to test this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I caught this because there's actually one in the curated list that's not available in the public portal lol

Copy link
Collaborator

Choose a reason for hiding this comment

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

so, has that been failing?

@alisman alisman merged commit e84e943 into cBioPortal:master Jul 29, 2020
@tmazor
Copy link
Contributor

tmazor commented Jul 29, 2020

I see this is already merged and on production but I'm testing anyway 😄

It looks good, but there are a few small things that I noticed:

  1. Using ln?q=TP53 brings me to the Mutations tab rather than Cancer Type Summary -- that's not necessarily an issue, just a thing I've noticed
  2. When I tried to run the same query from the homepage for comparison, I noticed that selecting the curated study list says 178 studies & 46959 samples
    image
    But after running the quer, I have 47005 samples?
    image
  3. We should update the last line on the Web API page to say "curated set of non-redundant studies":
    image

@adamabeshouse @alisman

@alisman
Copy link
Collaborator

alisman commented Jul 29, 2020

@tmazor sorry i should have given you an opportunity to test it. @adamabeshouse could the discrepancy have something to do with the study that isn't actually publically accessible?

@alisman
Copy link
Collaborator

alisman commented Jul 29, 2020

Also, @tmazor @adamabeshouse i will check but i think the sample count on homepage comes from counts stored in studies table which are not kept in sync with dynamic counts. I will bring this up in standup

@adamabeshouse
Copy link
Contributor Author

adamabeshouse commented Jul 29, 2020

@alisman Just to summarize what we said in stand-up, I don't think that's a factor because the full list is 179 studies, minus the non-accessible one is 178, which is the number we're seeing in those screenshots.

@tmazor yes, I also thought it was weird to send to mutations, but that's actually the existing behavior in the code at least, regardless of the OQL. was it not doing that before?

@tmazor
Copy link
Contributor

tmazor commented Jul 29, 2020

@adamabeshouse I don't think I'd ever used this ln?... link before, so I don't know what it did before. Maybe there's a historical reason why that behavior was desired? @jjgao do you know why the ln?... link goes to the Mutations tab? In principle I would prefer if it brought a user to Cancer Types Summary to be consistent with running a query from the homepage, but I don't really think it matters all that much so it's probably ok to leave as is.

@adamabeshouse adamabeshouse deleted the redirect-issue branch April 20, 2021 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants