-
Notifications
You must be signed in to change notification settings - Fork 304
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
RFC83: Make virtual study available for all users on their landing pages #4923
Conversation
✅ Deploy Preview for cbioportalfrontend ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@alisman Hi! Could you please provide some guidance on which tests I should write for this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Only a few minor questions.
I understand tests will still be added. I can also review those once available 👍
@@ -17,6 +17,14 @@ export default class sessionServiceAPI { | |||
return `${getSessionUrl()}/virtual_study`; | |||
} | |||
|
|||
getPublicVirtualStudyServiceUrl() { | |||
//FIXME change url after moving the code to the session controller |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we fixing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, here it is 2aea3c5
//map public virtual study to cancer study | ||
const _publicVirtualStudies = publicVirtualStudies | ||
.map(publicVirtualStudy => { | ||
// TODO: temp fix for when virtual study data is not of expeceted format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will this be added or is it a separate ticket?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nea, let's just clean it. 2ac225d
@@ -14,6 +14,7 @@ import { FilteredCancerTreeView } from '../StudyListLogic'; | |||
import { | |||
CancerTreeNode, | |||
CancerTypeWithVisibility, | |||
PUBLIC_VIRTUAL_STUDY_NAME, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, 6528be0
Parametrise public vs url instead of replacing it in a result url
Public VS functionality won't even work for legacy VS json format
Fix cBioPortal/cbioportal# (see https://help.github.com/en/articles/closing-issues-using-keywords)
Describe changes proposed in this pull request:
Backend cBioPortal/cbioportal#10829
Checks
Any screenshots or GIFs?
If this is a new visual feature please add a before/after screenshot or gif
here with e.g. Giphy CAPTURE or Peek
Notify reviewers
Read our Pull request merging
policy. It can help to figure out who worked on the
file before you. Please use
git blame <filename>
to determine thatand notify them either through slack or by assigning them as a reviewer on the PR