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

061 part 1 - make applicable user roles available in the app #63

Merged
merged 12 commits into from
Aug 22, 2024

Conversation

LDSamson
Copy link
Collaborator

@LDSamson LDSamson commented Aug 20, 2024

This is step 1 to resolve #61 .

After this PR is implemented, the application will:

  1. read all user roles (dependent on the config settings - posit connect, shinyproxy, or shinymanager deployment)
  2. select the roles applicable for ClinSight (as defined in the golem-config.yml)

In the rare case that multiple roles are available, it will select the first role available (in the order as defined in the config file.
If no roles are available, an empty character will be provided as user role.

@LDSamson LDSamson added this to the v0.1.0 milestone Aug 20, 2024
@LDSamson LDSamson force-pushed the ls-061-make_roles_available branch from fe85d15 to 5edc33c Compare August 20, 2024 09:56
@LDSamson LDSamson changed the title Ls 061 make roles available 061 part 1 - make applicable user roles available in the app Aug 20, 2024
@LDSamson
Copy link
Collaborator Author

@jthompson-arcus and @aclark02-arcus this PR is ready. Would one of you be able to review this somehwere this week?

This is the 'base' PR for some (soon available) follow-up PRs that ultimately will make the user role visible in multiple places in the application, and will document the role together with the user name when needed.

@LDSamson
Copy link
Collaborator Author

@jthompson-arcus I noticed that the input of the HTTP header that contains the role information in ShinyProxy needs sanitizing; it gives the roles in all capital letters, and returns a single string, with all roles separated with commas. Therefore, the changes in my latest PR do not yet work properly in the production environment.

I am working on a change to fix this. In general, I will do something like this for both the posit_connect group variable and the shinyproxy one:

trimws(unlist(strsplit(tolower(USER_GROUP_VARIABLE), ",")))

I think if the string is already sanitized, this will not change the outcome. However, please let me know if this is not the case or if this is not desirable for deployment using posit connect

…void confusion), add some unit tests.

(cherry picked from commit 4406c755b79148c5ec4f5825d16ad84227ec7b53)
@jthompson-arcus
Copy link
Collaborator

@jthompson-arcus I noticed that the input of the HTTP header that contains the role information in ShinyProxy needs sanitizing; it gives the roles in all capital letters, and returns a single string, with all roles separated with commas. Therefore, the changes in my latest PR do not yet work properly in the production environment.

I am working on a change to fix this. In general, I will do something like this for both the posit_connect group variable and the shinyproxy one:

trimws(unlist(strsplit(tolower(USER_GROUP_VARIABLE), ",")))

I think if the string is already sanitized, this will not change the outcome. However, please let me know if this is not the case or if this is not desirable for deployment using posit connect

Connect will already parse the object with that deployment. Shiny will see the object as a list.

@LDSamson
Copy link
Collaborator Author

@jthompson-arcus I noticed that the input of the HTTP header that contains the role information in ShinyProxy needs sanitizing; it gives the roles in all capital letters, and returns a single string, with all roles separated with commas. Therefore, the changes in my latest PR do not yet work properly in the production environment.
I am working on a change to fix this. In general, I will do something like this for both the posit_connect group variable and the shinyproxy one:

trimws(unlist(strsplit(tolower(USER_GROUP_VARIABLE), ",")))

I think if the string is already sanitized, this will not change the outcome. However, please let me know if this is not the case or if this is not desirable for deployment using posit connect

Connect will already parse the object with that deployment. Shiny will see the object as a list.

Okay I will fix it so that it also works with posit connect

Copy link
Collaborator

@jthompson-arcus jthompson-arcus left a comment

Choose a reason for hiding this comment

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

Few discussion points below.

Sorry if any comments are addressed in the follow up PRs. I tried to cross reference a little.

R/mod_review_config_fct_helpers.R Outdated Show resolved Hide resolved
R/mod_review_config.R Outdated Show resolved Hide resolved
R/app_server.R Outdated Show resolved Hide resolved
R/app_authentication.R Show resolved Hide resolved
Copy link
Collaborator

@jthompson-arcus jthompson-arcus left a comment

Choose a reason for hiding this comment

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

LGTM

@jthompson-arcus jthompson-arcus merged commit 0c918e0 into dev Aug 22, 2024
2 checks passed
@jthompson-arcus jthompson-arcus deleted the ls-061-make_roles_available branch August 22, 2024 13:36
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.

2 participants