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

Fix bug in datamanager.js getMutationProfileIds #1733

Conversation

sheridancbio
Copy link
Contributor

datamanager.js has a function 'getMutationProfileIds' which was returning all retrieved profile ids rather than just the ids of mutation profiles. This was causing mutationmapper.js to not get mutation events if the first profile id returned was a copy number alternation profile (for example)

Changes proposed in this pull request:

  • return list is now filtered to only return mutation profiles

note: we still need to add logic in the handler which starts mutationmapper to choose the "correct" mutation profile if there are more than one.

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.
  • Make sure your commit messages end with a Signed-off-by string (this line
    can be automatically added by git if you run the git-commit command with
    the -s option)
  • If this is a feature, the PR is to rc. If this is a bug fix, the PR is to
    hotfix.

Notify reviewers

@onursumer @adamabeshouse @jjgao

  • instead of returning all profile ids, add filter to only return mutation profiles

@sheridancbio sheridancbio added bug critical rc applies to the `rc` development branch ready to review study page labels Sep 29, 2016
@sheridancbio sheridancbio added this to the 1.3.0 milestone Sep 29, 2016
}));
fetch_promise.resolve(
profiles
.filter(function (p) {return p.genetic_alteration_type == "MUTATION_EXTENDED";})
Copy link
Contributor

Choose a reason for hiding this comment

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

In general it's better to use triple equalities in javascript, double equality does more fuzzy equalities. In this case I don't think it makes a difference but just as a rule I like to use triple equality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixing now

Copy link
Contributor

@adamabeshouse adamabeshouse left a comment

Choose a reason for hiding this comment

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

Looks good besides == should be ===

@sheridancbio
Copy link
Contributor Author

Corrected equality operator

- instead of returning all profile ids, add filter to only return mutation profiles
@sheridancbio sheridancbio force-pushed the fix-datamanager-mutationgeneticprofiles branch from 6dbc3f8 to f1f0f76 Compare September 29, 2016 21:05
@sheridancbio sheridancbio merged commit b08c9a3 into cBioPortal:rc Sep 29, 2016
@sheridancbio sheridancbio deleted the fix-datamanager-mutationgeneticprofiles branch November 15, 2016 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug critical rc applies to the `rc` development branch study page
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants