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 issue for unauthorized studies global message #4138

Closed
wants to merge 1 commit into from

Conversation

dianab0
Copy link
Contributor

@dianab0 dianab0 commented Jan 21, 2022

This PR fixes two problems:

  • The SELECT ALL button selected unauthorized studies
  • The message for unauthorized studies would not be rendered when there were no study tag placeholders.

Plus, it allows the use of {$.studyId} placeholder in the global message of unauthorized studies.

@dianab0 dianab0 requested review from pvannierop and alisman January 21, 2022 13:53
Copy link
Contributor

@pvannierop pvannierop left a comment

Choose a reason for hiding this comment

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

One or two sentences should be added to the documentation that this is possible.

src/shared/lib/JsonPathUtils.ts Outdated Show resolved Hide resolved
@dianab0 dianab0 requested a review from pvannierop January 31, 2022 11:36
@dianab0
Copy link
Contributor Author

dianab0 commented Jan 31, 2022

Docs update PR: cBioPortal/cbioportal#9272

@dianab0
Copy link
Contributor Author

dianab0 commented Feb 1, 2022

Added fix for issue cBioPortal/cbioportal#9255

@pvannierop pvannierop self-assigned this Feb 1, 2022
result[studyId] = [studyId];
if (isQueriedStudyAuthorized(study)) {
result[studyId] = [studyId];
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

it seems at least possible that an unauthorized study could make it's way into a virtual study from a user that has privelages to it. see below where we add studyids from virtual studies. should we do same check? or perhaps we should just fail so that a user will know that there is a "problem" with virtual study.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't add it because the unauthorized studies feature shouldn't apply to the virtual studies. Also in the StudyViewPageStore, I see that the isQueriedStudyAuthorized is added only to the physical studies.

Copy link
Contributor

Choose a reason for hiding this comment

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

@alisman

it seems at least possible that an unauthorized study could make it's way into a virtual study from a user that has privelages to it.

This is already the case when not running with the unauthorized studies feature. Or am I missing something in your feedback?

- Fixed the issue where the html inside the global message would not
be interpreted by the browser, when there was no placeholders.
- Fixed issue 9255 where SELECT ALL selects unauth studies as well
- Allow {$.studyId} placeholder in the unauth studies message
@dianab0 dianab0 force-pushed the unauth-studies-fix branch from b10bb88 to c69238a Compare February 5, 2022 09:53
@dianab0
Copy link
Contributor Author

dianab0 commented Feb 5, 2022

Rebased and pushed everything (including fixes for review comments) under one single commit

@pvannierop pvannierop closed this Feb 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants