Fixing for Genie fusion issue 5613 #6093

khzhu · 2019-05-13T18:01:00Z

What? Why?

Fix #5613

Changes proposed in this pull request:

backend:

add a new myBatis query getSampleCountInMultipleMolecularProfilesForMutationOrFusion to retrieve Mutations or Fusions from the mutation table.
add a new web service getSampleCountInMultipleMolecularProfilesForFusion
modify the existing web service getSampleCountInMultipleMolecularProfiles to use the new myBastis query created.
add a new API endpoint /fusion-genes/fetch

frontend:

add a new Fusion table without Frequency column in the study view

Checks

Runs on Heroku.
Follows 7 rules of great commit messages. For most PRs a single commit should suffice, in some cases multiple topical commits can be useful. During review it is ok to see tiny commits (e.g. Fix reviewer comments), but right before the code gets merged to master or rc branch, any such commits should be squashed since they are useless to the other developers. Definitely avoid merge commits, use rebase instead.
Follows the Google Style Guide.
If this is a feature, the PR is to rc. If this is a bug fix, the PR is to master.

Any screenshots or GIFs?

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

jjgao · 2019-05-14T20:36:03Z

@khzhu wondering why there are 103 commits? would you rebase/squash it?

khzhu · 2019-05-14T20:51:00Z

@JJ, sorry, I guess I only did merge. Rebased and will push back shortly.

jjgao · 2019-05-14T21:22:18Z

@khzhu thanks!

zhx828

Looks good to me

khzhu · 2019-07-22T19:36:29Z

@zhx828 , I've added a new Gene filter for fusions. Please have your final review and let me know if anything else need to be done. Thanks!

ao508

Hi @khzhu, I put together a PR that affects many of these files (#6523). In summary, the changes affect the persistence and service layer classes related to sql statements in MutationMapper.xml that reference the sql id whereInMultipleMolecularProfiles.

If you look at PR #6523 I added a new param called excludeFusions to calls for fetching mutations from multiple molecular profiles. Only the study view controller actually needs to set this value to true since we want to exclude fusion events from affecting the calculated alteration frequencies by gene in the mutated genes study view table.

Since the new method you introduced (getMutationsInMultipleMolecularProfilesByMutationType)[https://github.com//pull/6093/files#diff-9693fa0f2b985bb13b798a1d19d0c2ccR26] also references the sql id whereInMultipleMolecularProfiles in MutationMapper.xml, you will see an exception thrown regarding an unknown parameter excludeFusions coming from the parsing of MutationMapper.xml. To resolve this just add Boolean excludeFusions to the methods you introduced and set the value to false in any controllers/tests/classes that actually use this (with the exception of the StudyViewController).

…ect frequencies

Signed-off-by: Hongxin Zhang <hongxin@cbio.mskcc.org>

sheridancbio

I am approving, but I recommend 2 things -- some whitespace fixes (indentation and reversed comments), and also to please consider removing the data transformation steps in the controller handler into a service class. I see that we already are using studyViewFilterUtil, mutationService, and significantlyMutatedGeneService ... but we are doing additional filtering and sorting here in the controller --- can we wrap all of this into another service class so that the controller just passes the extracted study and sampleIds to a simple service layer function call here? (we can create a FusionGenesFetchService class if needed for example)

sheridancbio · 2019-10-02T17:52:01Z

web/src/main/java/org/cbioportal/web/StudyViewController.java

+    public ResponseEntity<List<MutationCountByGene>> fetchFusionGenes(
+        @ApiParam(required = true, value = "Study view filter")
+        @Valid @RequestBody(required = false) StudyViewFilter studyViewFilter,
+        @ApiIgnore // prevent reference to this attribute in the swagger-ui interface


these @ApiIgnore comments are reversed ... the first attribute is the one needed for @PreAuthorize .. the second is not

@sheridancbio , thank you for the review. I will make corrections.

corrected @ApiIgnore comments.

sheridancbio · 2019-10-02T18:01:01Z

web/src/main/java/org/cbioportal/web/StudyViewController.java

+        if (!filteredSampleIdentifiers.isEmpty()) {
+            List<String> studyIds = new ArrayList<>();
+            List<String> sampleIds = new ArrayList<>();
+            studyViewFilterUtil.extractStudyAndSampleIds(filteredSampleIdentifiers, studyIds, sampleIds);


The logic below seems to go beyond simple argument checking and service selection. I think this qualifies as business logic which should be encapsulated and tested in the service layer. I see that a util class is employed here ... but even still, I think this qualifies as business logic (data transformations). I would not insist on relocation, but I guess I strongly recommend keeping data transformations out of the controller handler.

total agree and very good points! It is not just here but the whole class should be refactored to pull those data transformations out and put into respective service layer. How about we have a follow up PR instead do correction here. What do you think @sheridancbio, @zhx828?

will keep this principle in mind when switching to the new SV table and reimplementing fetch-fusion-gene endpoint.

sheridancbio · 2019-10-02T18:04:19Z

web/src/main/java/org/cbioportal/web/parameter/StudyViewFilter.java

-    private Boolean withCNAData;
-    private RectangleBounds mutationCountVsCNASelection;
+	private List<MutationGeneFilter> mutatedGenes;
+    private List<FusionGeneFilter> fusionGenes;


standardize indentation to uniform 4-space indents (no tabs)

corrected indents.

sheridancbio · 2019-10-02T18:04:45Z

web/src/main/java/org/cbioportal/web/parameter/StudyViewFilter.java

@@ -81,8 +82,14 @@ public void setClinicalDataIntervalFilters(List<ClinicalDataIntervalFilter> clin
        return mutatedGenes;
    }

-    public void setMutatedGenes(List<MutationGeneFilter> mutatedGenes) {
-        this.mutatedGenes = mutatedGenes;
+	public void setMutatedGenes(List<MutationGeneFilter> mutatedGenes) { this.mutatedGenes = mutatedGenes; }


standardize indentation to 4-space indentation (no tabs)

sheridancbio · 2019-10-02T18:32:29Z

The connections to the involvedCancerStudyExtractorInterceptor class look good.

…NIE-fusion-issue-5613 # Conflicts: # web/src/main/java/org/cbioportal/web/StudyViewController.java # web/src/main/java/org/cbioportal/web/parameter/StudyViewFilter.java

Signed-off-by: Hongxin Zhang <hongxin@cbio.mskcc.org>

khzhu requested review from jjgao, onursumer and zhx828 and removed request for jjgao May 13, 2019 18:03

khzhu force-pushed the GENIE-fusion-issue-5613 branch from a3c2edf to ebafefa Compare May 14, 2019 18:08

khzhu force-pushed the GENIE-fusion-issue-5613 branch from ebafefa to cd73c3e Compare May 14, 2019 20:55

zhx828 approved these changes May 14, 2019

View reviewed changes

jjgao assigned khzhu Jul 2, 2019

khzhu force-pushed the GENIE-fusion-issue-5613 branch from cd73c3e to 17123dd Compare July 5, 2019 20:01

khzhu force-pushed the GENIE-fusion-issue-5613 branch 2 times, most recently from e732e91 to ff0d69a Compare July 22, 2019 19:30

zhx828 added the do not merge label Jul 24, 2019

zhx828 self-requested a review July 24, 2019 15:28

khzhu force-pushed the GENIE-fusion-issue-5613 branch from def3bdd to 8e021e5 Compare July 24, 2019 21:42

khzhu force-pushed the GENIE-fusion-issue-5613 branch 2 times, most recently from 2f00e22 to 84444ac Compare August 19, 2019 16:19

ao508 reviewed Aug 22, 2019

View reviewed changes

khzhu force-pushed the GENIE-fusion-issue-5613 branch from 84444ac to 8b586e7 Compare August 27, 2019 19:33

zhx828 requested review from n1zea144 and removed request for onursumer August 27, 2019 19:39

zhx828 removed the do not merge label Aug 27, 2019

khzhu force-pushed the GENIE-fusion-issue-5613 branch from 0bd4aae to b3d95a0 Compare August 28, 2019 07:17

ao508 approved these changes Aug 28, 2019

View reviewed changes

zhx828 removed their request for review August 29, 2019 19:34

zhx828 mentioned this pull request Aug 29, 2019

Exclude fusions from StudyViewController call to get alteration counts by gene #6523

Closed

5 tasks

zhx828 added the do not merge label Aug 30, 2019

n1zea144 approved these changes Sep 5, 2019

View reviewed changes

jjgao temporarily deployed to cbioportal-pr-6093 September 13, 2019 19:02 Inactive

khzhu force-pushed the GENIE-fusion-issue-5613 branch from f5fa996 to 3b6c40e Compare September 30, 2019 21:28

jjgao temporarily deployed to cbioportal-pr-6093 September 30, 2019 21:28 Inactive

zhx828 removed the do not merge label Sep 30, 2019

zhx828 requested a review from sheridancbio October 1, 2019 15:04

jjgao temporarily deployed to cbioportal-pr-6093 October 1, 2019 18:36 Inactive

Kelsey Zhu and others added 3 commits October 1, 2019 15:21

resolve issue cBioPortal#5613 fixing mutated genes table shows incorr…

a3d5abd

…ect frequencies

resolve issue cBioPortal#5613 fixing mutated genes table shows incorr…

0e35e36

…ect frequencies

Include fusion frequency

ba3742f

zhx828 force-pushed the GENIE-fusion-issue-5613 branch from 275dfdb to ba3742f Compare October 2, 2019 14:45

jjgao temporarily deployed to cbioportal-pr-6093 October 2, 2019 14:45 Inactive

Update ChartType to support FusionGenesTable

71aff19

Signed-off-by: Hongxin Zhang <hongxin@cbio.mskcc.org>

jjgao temporarily deployed to cbioportal-pr-6093 October 2, 2019 15:57 Inactive

sheridancbio approved these changes Oct 2, 2019

View reviewed changes

sheridancbio added api backend feature security labels Oct 2, 2019

Merge remote-tracking branch 'origin/GENIE-fusion-issue-5613' into GE…

f41ebec

…NIE-fusion-issue-5613 # Conflicts: # web/src/main/java/org/cbioportal/web/StudyViewController.java # web/src/main/java/org/cbioportal/web/parameter/StudyViewFilter.java

jjgao temporarily deployed to cbioportal-pr-6093 October 4, 2019 15:34 Inactive

resolving code review comments from Rob

e8d44a2

jjgao temporarily deployed to cbioportal-pr-6093 October 4, 2019 15:53 Inactive

Merge remote-tracking branch 'cbio/master' into GENIE-fusion-issue-5613

f1d0533

jjgao had a problem deploying to cbioportal-pr-6093 October 10, 2019 16:50 Failure

update due to file structure change

7133b6c

Signed-off-by: Hongxin Zhang <hongxin@cbio.mskcc.org>

jjgao deployed to cbioportal-pr-6093 October 10, 2019 17:05 View deployment

zhx828 merged commit d219080 into cBioPortal:master Oct 10, 2019

jagnathan deleted the GENIE-fusion-issue-5613 branch June 2, 2021 16:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing for Genie fusion issue 5613 #6093

Fixing for Genie fusion issue 5613 #6093

khzhu commented May 13, 2019 •

edited

Loading

jjgao commented May 14, 2019

khzhu commented May 14, 2019 •

edited

Loading

jjgao commented May 14, 2019

zhx828 left a comment

khzhu commented Jul 22, 2019

ao508 left a comment

sheridancbio left a comment

sheridancbio Oct 2, 2019

khzhu Oct 3, 2019

khzhu Oct 3, 2019

sheridancbio Oct 2, 2019

khzhu Oct 3, 2019

khzhu Oct 3, 2019

sheridancbio Oct 2, 2019

khzhu Oct 3, 2019

sheridancbio Oct 2, 2019

sheridancbio commented Oct 2, 2019

Fixing for Genie fusion issue 5613 #6093

Fixing for Genie fusion issue 5613 #6093

Conversation

khzhu commented May 13, 2019 • edited Loading

What? Why?

Checks

Any screenshots or GIFs?

jjgao commented May 14, 2019

khzhu commented May 14, 2019 • edited Loading

jjgao commented May 14, 2019

zhx828 left a comment

Choose a reason for hiding this comment

khzhu commented Jul 22, 2019

ao508 left a comment

Choose a reason for hiding this comment

sheridancbio 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sheridancbio commented Oct 2, 2019

khzhu commented May 13, 2019 •

edited

Loading

khzhu commented May 14, 2019 •

edited

Loading