Add "View NG-CHM" button on OncoPrint Heatmap tab #2872

ChrisWakefield · 2019-11-12T23:56:15Z

Describe changes proposed in this pull request:

Inserts a fourth button in the OncoPrint Heatmap drop-down. Clicking on the button shows a disclaimer about linking to an external website. Once the user acknowledges, NG-CHM compendium browser opens in a new browser tab.

Though this has the new visual feature of the added button and confirmation dialog, properties must be configured and the study must be selected to see the button. I don't know how to automate the complete scenario.

Checks

Has tests or has a separate issue that describes the types of test that should be created. If no test is included it should explicitly be mentioned in the PR why there is no test.
The commit log is comprehensible. It follows [7 rules of great commit messages]
Is this PR adding logic based on one or more clinical attributes? NO it does not.

Any screenshots or GIFs?

If this is a new visual feature please add a before/after screenshot or gif
here with e.g. GifGrabber.

I can add screenshots of the button and dialog, but currently https://rc.cbioportal.org/api/studies/blca_tcga_pub/molecular-profiles is failing, so the screenshots would have to be against master.

Notify reviewers

@zhx828 @alisman

Based on conversation with Rehan Akbani and Niki Schultz:
insert a fourth button in the Heatmap drop-down. Clicking on the button would show a disclaimer that the user is linking out to an external website and their gene and sample selections may not be the only ones represented in the map. Once the user acknowledges the disclaimer, NG-CHM would open up (perhaps in a new browser tab).

src/pages/resultsView/ResultsViewPageStore.ts

src/shared/components/oncoprint/controls/OncoprintControls.tsx

alisman · 2019-11-19T17:56:22Z

src/pages/resultsView/ResultsViewPageStore.ts

@@ -3263,7 +3301,7 @@ export class ResultsViewPageStore {
    //this is only required to show study name and description on the results page
    //CancerStudy objects for all the cohortIds
    readonly queriedStudies = remoteData({
-        await: () => [this.studyIdToStudy, this.queriedVirtualStudies],
+        await: () => [this.studyIdToStudy, this.queriedVirtualStudies, this.isThereRemoteNGCHM],


this will introduce a hard dependency on the mdanderson site for ALL portals. if it errors, or is slow, then whatever feature depends on queriesStudies will fail. i would make sure that ONLY the link/button depends on remoteNgchmUrl and that it is hidden if it fails. you will have to deal with loading and errors state then. don't worry about loading state. just hidden if it's loading. for error state, just put in some little error text.

Removed the hard dependency. (Still on RC for now.)

alisman · 2019-11-19T18:00:33Z

src/shared/components/oncoprint/controls/ConfirmNgchmModal.tsx

+
+export default class ConfirmNgchmModal extends React.Component {
+
+    state: {show: boolean, callback: () => void } = {


for consistency sake, lets use mobx observable props to handle state instead of react state. so, the component gets @observer decorator and show and callback are properties that have @observable decorator.
There are many devs working on the project and I don't want them getting inconsistent message.

Done, I think. react state is gone. (Still in RC)

alisman · 2019-11-19T18:07:13Z

src/shared/components/oncoprint/controls/OncoprintControls.tsx

@@ -471,6 +473,11 @@ export default class OncoprintControls extends React.Component<
                this.props.handlers.onClickClusterHeatmap &&
                    this.props.handlers.onClickClusterHeatmap();
                break;
+            case EVENT_KEY.viewNGCHM:
+            	if (this.props.state.ngchmButtonActive && this.props.handlers.onClickNGCHM) {
+                    this.confirmModal!.show(this.props.handlers.onClickNGCHM);


this is a bit of a violation of state management practice. The state of this modal should really be kept by parent component and passed in as a prop (or dealt with entirely outside the modal.

Passed as prop. See what you think now. (Still in RC)

alisman · 2019-11-19T18:08:25Z

src/shared/components/oncoprint/controls/ConfirmNgchmModal.tsx

@@ -0,0 +1,52 @@
+import * as React from 'react';


would be nice to have a comment here about what NGCHM is.

Done. Have a look.

alisman · 2019-11-19T18:12:30Z

src/pages/resultsView/ResultsViewPageStore.ts

+        }
+    });
+
+    readonly isThereRemoteNGCHM = remoteData<boolean>({


rather than creating a wrapper promise to return boolean, just put what's in invoke logic down in ngchmButtonActive getter. We're trying to be more judicious about creating lots of wrapper remoteData (mobxpromise), by which i mean, remoteData instances that only exist to process other remoteData and don't actually make any async calls.

Did as you suggested, and it seems to work, though it's not obvious to me why this doesn't cause a race condition other than ngchmButtonActive just happens to never be called before the remoteData is already resolved. (Still in RC for now.)

alisman · 2019-11-19T18:15:48Z

@jjgao is this an integration we want for all portals or should add a configuration flag for it

alisman · 2019-11-19T19:09:21Z

@ChrisWakefield We spin up test instance for all PRs. Link to yours follows. Seems to work nicely. You can make this PR directly to master since it does not involve any backend work. So please rebase on master.

https://cbioportal-f-rc-eoihwt6uno6u0u.herokuapp.com/results/oncoprint?Z_SCORE_THRESHOLD=2.0&cancer_study_id=coadread_tcga_pub&cancer_study_list=coadread_tcga_pub&case_ids=&case_set_id=coadread_tcga_pub_nonhypermut&gene_list=KRAS%20NRAS%20BRAF&gene_set_choice=user-defined-list&genetic_profile_ids_PROFILE_COPY_NUMBER_ALTERATION=coadread_tcga_pub_gistic&genetic_profile_ids_PROFILE_MUTATION_EXTENDED=coadread_tcga_pub_mutations

jjgao · 2019-11-19T23:11:28Z

is this an integration we want for all portals or should add a configuration flag for it.

It's just for public portal, so configuration would be good. The same configuration should also turn on/off (off by default) for the study view integration of NG-CHM.

ChrisWakefield · 2019-11-21T17:12:02Z

@jjgao @alisman

It's just for public portal, so configuration would be good. The same configuration should also turn on/off (off by default) for the study view integration of NG-CHM.

I think this can be for both private or public portals, but yes being able to configure it for a specific installation/deployment would be useful. Where is this configuration flag going to be, and what mechanism do you have in place to enable that? What's a good example?

ChrisWakefield · 2019-11-21T17:18:27Z

@alisman

@ChrisWakefield We spin up test instance for all PRs. Link to yours follows. Seems to work nicely. You can make this PR directly to master since it does not involve any backend work. So please rebase on master.

Thank you.
When I move to master, does that invalidate this PR? Does it mean I cancel this one and create a new PR on master?

alisman · 2019-11-22T21:21:06Z

@ChrisWakefield to "go to master":

git rebase your PR locally. this replays your changes as if you had made them to master branch
cliick edit button at to right of this PR page and you will be allowed to select a new "base" for the PR

alisman · 2019-11-22T21:29:47Z

@ChrisWakefield to add configuration, there are two steps:

Frontend project: use show_genomenexus property as an example. You need to add it to IServerConfig interface, then give it a default (false) in serverConfigDefaults.ts. This is PR Add PDX Hierarchy Tree #1.
Backend: configuration setting will live in portal.properties file. In order to pipe it to frontend, you need to add it to propNameArray in config_service.jsp. This will automatically read it and pass it to frontend. NOTE: A dot (.) in the property name will be transformed to an underscore in frontend. So, skin.documentation_markdown in backend should be referenced as skin_documentation_markdown in frontend.

Sorry this is more complicated than it should be. Happy to assist.

alisman · 2019-12-06T18:53:04Z

@ChrisWakefield any word on this PR?

ChrisWakefield · 2019-12-10T23:20:44Z

@alisman @jjgao
This should now be complete. It's been rebased on master.
The configuration is set to false (disabled) by default, as JJ requested. Is there a way to tweak the config in the test instance just to see it working?

alisman · 2019-12-11T15:51:15Z

@ChrisWakefield test this and it looks good. You can mock configuration options in browser window with by running this in console
frontendConfig.serverConfig.enable_mdacc_ngchm_button = true

It works. I think we can take it from here. We will need to add the configuration to the backend, then turn it on for our production env. THEN, e2e tests will fail due to addition of this button. Then we need to update screenshots! Sound fun? Anyway, I'll have someone here take care of this.

ChrisWakefield · 2019-12-16T17:06:33Z

@alisman
The herokuapp deployment started giving errors on Friday (if not before):
"Cannot GET /"
Is there something I need to do to fix it?

alisman · 2019-12-16T17:09:39Z

@ChrisWakefield it's a Heroku bug. I fix it by deleting app and then clicking "Create review app" again.

alisman · 2019-12-16T17:17:54Z

@ChrisWakefield so, the property will now be available at AppConfig.serverConfig_show_mdacc_heatmap. We need to adjust this PR for that. Can you do that work? We should also hide the tab on study view based on that param.

ChrisWakefield · 2019-12-16T17:23:50Z

@alisman Both done in commit 0051dd6. : )

ChrisWakefield · 2019-12-16T17:39:02Z

@alisman

it's a Heroku bug. I fix it by deleting app and then clicking "Create review app" again.

Ah. It's working now, both OncoPrint and Study view. I hope to demo this here this afternoon, just to show it works on more than just my machine.

alisman · 2019-12-16T21:46:25Z

@ChrisWakefield this will be merged when the backend config change is merged (next patch release will be by end of week)

alisman · 2019-12-19T15:25:27Z

@ChrisWakefield i was going to merge this today but i noticed there's a problem with commit history. We need to squash your commits into one and then rebase just those on master. We don't want all the merges from master to your branch. So, in the end this should just be one commit.

ChrisWakefield · 2019-12-19T18:41:47Z

@alisman
Ok, so I squashed my commits into one, but I still see that git included one of your commits:
Fix handling of data_priority = profileFilter on submission/modificat…
and I don't understand why.
In addition, there are some minor merge conflicts that reappeared with this. I had all those resolved before this. How do I get my commits merged into your master (in my rc) without the conflicts?

alisman · 2019-12-19T19:28:01Z

Hi @ChrisWakefield thanks. I can take if from here! Probably best if I resolve the conflicts. I will probably push to your branch so that we can preserve the PR comment history. If that fails, I'll open one myself

ChrisWakefield · 2019-12-20T22:22:16Z

@alisman @adamabeshouse @inodb @jjgao
Thank you all for all the help! We look forward to seeing it live.
Happy Holidays!

inodb · 2020-01-03T14:48:17Z

@alisman @adamabeshouse this PR also seems to have missed product review. Let's discuss how to improve the process on engineering call

jjgao requested a review from adamabeshouse November 13, 2019 18:25

adamabeshouse requested changes Nov 13, 2019

View reviewed changes

src/pages/resultsView/ResultsViewPageStore.ts Outdated Show resolved Hide resolved

src/shared/components/oncoprint/controls/OncoprintControls.tsx Outdated Show resolved Hide resolved

adamabeshouse approved these changes Nov 15, 2019

View reviewed changes

alisman reviewed Nov 19, 2019

View reviewed changes

jjgao temporarily deployed to cbioportal-f-rc-eoihwt6uno6u0u November 19, 2019 18:14 Inactive

jjgao temporarily deployed to cbioportal-f-rc-eoihwt6uno6u0u December 6, 2019 22:58 Inactive

ChrisWakefield force-pushed the rc branch from b080d3d to 9443a3f Compare December 10, 2019 23:12

jjgao temporarily deployed to cbioportal-f-rc-eoihwt6uno6u0u December 10, 2019 23:13 Inactive

ChrisWakefield changed the base branch from rc to master December 10, 2019 23:13

jjgao temporarily deployed to cbioportal-f-rc-eoihwt6uno6u0u December 13, 2019 18:37 Inactive

ChrisWakefield force-pushed the rc branch from 08e51d2 to 0051dd6 Compare December 13, 2019 23:42

jjgao temporarily deployed to cbioportal-f-rc-eoihwt6uno6u0u December 13, 2019 23:42 Inactive

jjgao temporarily deployed to cbioportal-f-rc-eoihwt6uno6u0u December 13, 2019 23:55 Inactive

jjgao temporarily deployed to cbioportal-f-rc-eoihwt6uno6u0u December 16, 2019 17:07 Inactive

alisman approved these changes Dec 16, 2019

View reviewed changes

ChrisWakefield force-pushed the rc branch from 5570fec to 303f7f3 Compare December 19, 2019 18:27

jjgao temporarily deployed to cbioportal-f-rc-eoihwt6uno6u0u December 19, 2019 18:27 Inactive

Add "View NG-CHM" button on OncoPrint Heatmap tab

12eac09

alisman force-pushed the rc branch from 303f7f3 to 12eac09 Compare December 19, 2019 21:28

jjgao temporarily deployed to cbioportal-f-rc-eoihwt6uno6u0u December 19, 2019 21:29 Inactive

alisman merged commit a33a3a5 into cBioPortal:master Dec 19, 2019

inodb added the feature label Dec 20, 2019

ChrisWakefield deleted the rc branch December 30, 2019 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "View NG-CHM" button on OncoPrint Heatmap tab #2872

Add "View NG-CHM" button on OncoPrint Heatmap tab #2872

ChrisWakefield commented Nov 12, 2019

alisman Nov 19, 2019

ChrisWakefield Dec 6, 2019

alisman Nov 19, 2019

ChrisWakefield Dec 6, 2019

alisman Nov 19, 2019

ChrisWakefield Dec 6, 2019

alisman Nov 19, 2019

ChrisWakefield Dec 6, 2019

alisman Nov 19, 2019

ChrisWakefield Dec 6, 2019

alisman commented Nov 19, 2019

alisman commented Nov 19, 2019 •

edited

Loading

jjgao commented Nov 19, 2019 •

edited

Loading

ChrisWakefield commented Nov 21, 2019

ChrisWakefield commented Nov 21, 2019

alisman commented Nov 22, 2019

alisman commented Nov 22, 2019

alisman commented Dec 6, 2019

ChrisWakefield commented Dec 10, 2019 •

edited

Loading

alisman commented Dec 11, 2019

ChrisWakefield commented Dec 16, 2019

alisman commented Dec 16, 2019

alisman commented Dec 16, 2019

ChrisWakefield commented Dec 16, 2019 •

edited

Loading

ChrisWakefield commented Dec 16, 2019 •

edited

Loading

alisman commented Dec 16, 2019

alisman commented Dec 19, 2019

ChrisWakefield commented Dec 19, 2019

alisman commented Dec 19, 2019

ChrisWakefield commented Dec 20, 2019

inodb commented Jan 3, 2020


		export default class ConfirmNgchmModal extends React.Component {

		state: {show: boolean, callback: () => void } = {

Add "View NG-CHM" button on OncoPrint Heatmap tab #2872

Add "View NG-CHM" button on OncoPrint Heatmap tab #2872

Conversation

ChrisWakefield commented Nov 12, 2019

Checks

Any screenshots or GIFs?

Notify reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alisman commented Nov 19, 2019

alisman commented Nov 19, 2019 • edited Loading

jjgao commented Nov 19, 2019 • edited Loading

ChrisWakefield commented Nov 21, 2019

ChrisWakefield commented Nov 21, 2019

alisman commented Nov 22, 2019

alisman commented Nov 22, 2019

alisman commented Dec 6, 2019

ChrisWakefield commented Dec 10, 2019 • edited Loading

alisman commented Dec 11, 2019

ChrisWakefield commented Dec 16, 2019

alisman commented Dec 16, 2019

alisman commented Dec 16, 2019

ChrisWakefield commented Dec 16, 2019 • edited Loading

ChrisWakefield commented Dec 16, 2019 • edited Loading

alisman commented Dec 16, 2019

alisman commented Dec 19, 2019

ChrisWakefield commented Dec 19, 2019

alisman commented Dec 19, 2019

ChrisWakefield commented Dec 20, 2019

inodb commented Jan 3, 2020

alisman commented Nov 19, 2019 •

edited

Loading

jjgao commented Nov 19, 2019 •

edited

Loading

ChrisWakefield commented Dec 10, 2019 •

edited

Loading

ChrisWakefield commented Dec 16, 2019 •

edited

Loading

ChrisWakefield commented Dec 16, 2019 •

edited

Loading