Use oncokb biological data without token #3182

zhx828 · 2020-04-29T23:52:26Z

This is a part of fix for oncokb/oncokb#1975

we no longer require a token to display biological data from oncokb(it connects to public.api.oncokb.org)
we allow users to configure the cbioportal instance to use www.oncokb.org to include clinical data
Levels column will be disabled for mutation mapper annotation track

Here is the tooltip when portal connects to the public instance:

onursumer

Looks good in general, just a minor comment for one of the computed values

onursumer · 2020-04-30T00:10:34Z

packages/react-mutation-mapper/src/component/mutationMapper/MutationMapper.tsx

+        return this.store.oncoKbInfo.result
+            ? this.store.oncoKbInfo.result.publicInstance
+            : ONCOKB_DEFAULT_INFO_PUBLIC_INSTANCE;


Isn't it possible here to just do:

Suggested change

return this.store.oncoKbInfo.result

? this.store.oncoKbInfo.result.publicInstance

: ONCOKB_DEFAULT_INFO_PUBLIC_INSTANCE;

return this.store.usingOncoKbPublicInstance;

Oh good catch. This is must been there before I wrote the method in store

i feel like it's odd to have something in all caps that is not a string or number constant.

Hmm, I never thought about restricting all caps to string and number constants. We already have similar default values in react-mutation-mapper though.

According to airbnb javascript style guideline, it seems okay to have all caps for other types of constants as well: https://github.com/airbnb/javascript/#naming--uppercase

Oh, ONCOKB_DEFAULT_INFO_PUBLIC_INSTANCE is a boolean variable: https://github.com/cBioPortal/cbioportal-frontend/pull/3182/files#diff-c27e93a0aa919b85e6be54dc7efb1c85R33
Having the name instance for a boolean variable is a bit confusing here.

alisman · 2020-05-04T18:58:21Z

packages/react-mutation-mapper/src/component/oncokb/OncoKbTooltip.tsx

@@ -14,6 +14,7 @@ export interface IOncoKbTooltipProps {
    hugoSymbol: string;
    isCancerGene: boolean;
    geneNotExist: boolean;
+    usingPublicInstance: boolean;


lets keek this usingPublicOncoKBInstance

alisman · 2020-05-04T19:09:27Z

@zhx828 I'm curious why we have to call the info service to find out whether we're using public instance or now.

Code looks fine. I do wonder if this bit of state (using public instance) or not is kind of a global state and could be read directly off the stores (which have reference to the appstore) by components rather than passed down to them though the component heirarchy. The downside of this might be that "leaf" components would need reference to the stores, but perhaps they already have it? If so, then maybe this could be accomplished with less code change. Just an idea.

zhx828 · 2020-05-04T19:17:43Z

@alisman thanks for the suggestion. There are two ways of implementing this as I know.

On the backend, similar to oncokbToken, we could return the status of the instance in the serverConfig. But that status needs to be managed once the application is alive since it still needs to reach out to the info endpoint. The timing is tricky. At the moment, we only configure portal instance with few oncokb properties, we do not manage any actually oncokb data on the backend. Thought that would complicate the situation.
I never thought about adding a global info state, but that's probably a good idea. The main places where use oncokb is the react-mutation-mapper(RMM) package. I don't think our store is integrate with it much. We still need to pass info status to the sub components. Or maybe I'm wrong. @onursumer

onursumer · 2020-05-04T19:57:18Z

@zhx828 @alisman react-mutation-mapper does not know anything about the stores defined in cbioportal-frontend, but there is MutationMapperStore that we pass down to the certain components, but probably not all the way down to the leaf components.

Having access to a global state info might be a handicap for modularity though. For example, if one day we want to move OncoKB related components into a separate package, importing a dependency from react-mutation-mapper would be a problem. Of course we can always refactor the code when we need to do these kind of separation.

onursumer · 2020-05-04T20:00:49Z

packages/react-mutation-mapper/src/component/column/Annotation.tsx

@@ -39,6 +40,7 @@ export type AnnotationProps = {
    hotspotData?: RemoteData<IHotspotIndex | undefined>;
    oncoKbData?: RemoteData<IOncoKbData | Error | undefined>;
    oncoKbCancerGenes?: RemoteData<CancerGene[] | Error | undefined>;
+    usingOncoKbPublicInstance: boolean;


should we also rename this to usingPublicOncoKbInstance for consistency?

onursumer · 2020-05-04T20:02:47Z

packages/react-mutation-mapper/src/component/track/OncoKbTrackTooltip.tsx

@@ -8,12 +8,16 @@ import { IndicatorQueryResp } from 'oncokb-ts-api-client';
 import OncoKbSummaryTable from '../oncokb/OncoKbSummaryTable';

 type OncoKbTrackTooltipProps = {
+    usingOncoKbPublicInstance: boolean;


this one too?

Suggested change

usingOncoKbPublicInstance: boolean;

usingPublicOncoKbInstance: boolean;

alisman · 2020-05-04T21:42:51Z

@zhx828 not sure i understand But that status needs to be managed once the application is alive since it still needs to reach out to the info endpoint.. you mean it can change during the life of the application and we don't want to have redeploy to handle that?

@zhx828 @onursumer on issue of global state, understood. sounds like you made the right decision

zhx828 · 2020-05-05T14:19:14Z

@alisman I doubt the status, whether it's a public oncokb instance, will change. But it does not feel right to me when a portal backend state relies on an API and that API call needs to be triggered when the portal is deployed. What if at that time, the endpoint failed, should we manage to refetch the info?
I think it's better handle in the frontend, the only downside is we need to aware of the logic whenever oncokb is used.

@jjgao @alisman @onursumer any thoughts?

OncoKB no longer requires a token to be used for biological data. But if the clinical data is needed, the user will need to configure the proper token. Change OncoKB Card interface based on whether the oncokb instance is public Disable the levels column in oncokb track if the instance is public Signed-off-by: Hongxin Zhang <hongxin@cbio.mskcc.org>

zhx828 requested review from alisman, jjgao and onursumer April 29, 2020 23:52

zhx828 added DO NOT MERGE ready to review labels Apr 29, 2020

onursumer reviewed Apr 30, 2020

View reviewed changes

zhx828 force-pushed the use-public-oncokb-instance branch from 0f8e55b to 9731897 Compare May 1, 2020 15:36

zhx828 removed the DO NOT MERGE label May 1, 2020

jjgao added this to the Sprint 19 Y2020 milestone May 4, 2020

alisman reviewed May 4, 2020

View reviewed changes

onursumer reviewed May 4, 2020

View reviewed changes

zhx828 added the DO NOT MERGE label May 5, 2020

zhx828 force-pushed the use-public-oncokb-instance branch from cdd1859 to 4f9c432 Compare May 5, 2020 21:57

zhx828 requested review from onursumer and alisman May 5, 2020 21:57

zhx828 removed the DO NOT MERGE label May 5, 2020

onursumer approved these changes May 5, 2020

View reviewed changes

zhx828 merged commit bdb93e7 into cBioPortal:master May 7, 2020

zhx828 deleted the use-public-oncokb-instance branch May 7, 2020 20:04

zhx828 added the feature label May 7, 2020

inodb added cl-external-api External API section of changelog. Changes to handle an external API (i.e. not cBioPortal API) and removed feature labels May 7, 2020

inodb changed the title ~~Allow cbioportal use oncokb biological data without token~~ Use oncokb biological data without token May 7, 2020

inodb mentioned this pull request May 7, 2020

Allow user to annotate biological information without registering oncokb/oncokb#1975

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use oncokb biological data without token #3182

Use oncokb biological data without token #3182

zhx828 commented Apr 29, 2020 •

edited

onursumer left a comment

onursumer Apr 30, 2020

zhx828 Apr 30, 2020

alisman Apr 30, 2020

onursumer Apr 30, 2020

onursumer Apr 30, 2020

onursumer Apr 30, 2020

alisman May 4, 2020

alisman commented May 4, 2020

zhx828 commented May 4, 2020

onursumer commented May 4, 2020

onursumer May 4, 2020

onursumer May 4, 2020

alisman commented May 4, 2020

zhx828 commented May 5, 2020

	usingOncoKbPublicInstance: boolean;
	usingPublicOncoKbInstance: boolean;

Use oncokb biological data without token #3182

Use oncokb biological data without token #3182

Conversation

zhx828 commented Apr 29, 2020 • edited

onursumer left a 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 May 4, 2020

zhx828 commented May 4, 2020

onursumer commented May 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alisman commented May 4, 2020

zhx828 commented May 5, 2020

zhx828 commented Apr 29, 2020 •

edited