search

pharmaR / riskassessment

Risk Assessment Demo App: https://rinpharma.shinyapps.io/riskassessment
https://pharmar.github.io/riskassessment/
Other
98 stars 24 forks source link

Allow assigning decisions from upload file #777

Closed jthompson-arcus closed 1 month ago

jthompson-arcus commented 1 month ago

closes #663

jthompson-arcus commented 1 month ago

Update the "Sample Dataset"

I think it'd make sense to include a Decision column here so people know it's an option. Note that it would be nice to show off an example in the context of the current config file. So, we can't assume that each org will adopt the recommended low/ medium/ high risk options. So maybe our example file should choose to make shiny whatever the first value in the config's decision category field.

I will update the template to include a decision column. I think we should just leave it blank for a few reasons. 1) The template is bundled with the package so it's not straightforward to make it more dynamic and depend on the decisions available. I think the template should just work if any user downloads it. 2) The decision column is only allowable for users with the correct credentials. If we add a decision in, non-credentialed users would have to edit it before upload.

I suppose we could think about making this more dynamic where the template can vary depending on a users credentials and the decision categories contained in the app, but I would like that to be a different PR.

Update documentation

Speaking of documenting this new feature, we should update our Get Started vignette here:

https://github.com/pharmaR/riskassessment/blob/a3e080185b45127c62b64e0a30b9ac5fb6058977/vignettes/riskassessment.Rmd#L99

I will take a look.

Decision By

I uploaded shiny with a "Low Risk" decision and saw that the username is accurately recorded in the Decision By field. What are your thoughts on keeping track of the upload method in this field only for CSV uploads? So, if the pkg was uploaded via a CSV, it would read "admin (csv batch )" or something similar? That way, we'll know if the user spent time in the app doing a proper review or if they made up their mind from the start on these pkgs...

I had originally not used the user name. I'm a little torn because any one with final decision privileges can assign during upload with a csv. I did add an overall comment saying that it was assigned in the upload process. I think it makes most sense to tie it back to the process.

Case sensitivity error

I created a csv that looked like this and got denied! I would have expected this to work. But I found out if I capitalized to "Low Risk", it worked! As such, I'm wondering if we should drop the case sensitivity to improve the user experience. Perhaps we could use a little tolower() magic? Thoughts?

The problem with removing this case sensitivity is that we do not enforce sensitive case for decision categories in the app itself, but I'm open to the idea.

Case sensitive variable name

Speaking of case sensitivity, I also uploaded the following csv with a capital "D" and observed that the "Low Risk" decision wasn't applied. While on the topic, I would think that none of these columns should be case sensitive. Perhaps we could use a little tolower() magic?

I will fix this.

jthompson-arcus commented 1 month ago

@aclark02-arcus did the select inputs get really narrow for you as well? They look off on the upload package tab with the larger buttons next to them.

aclark02-arcus commented 1 month ago

@aclark02-arcus did the select inputs get really narrow for you as well? They look off on the upload package tab with the larger buttons next to them.

@jthompson-arcus yes, but it didn't happen during this PR. I noticed it a few PRs ago...

aclark02-arcus commented 1 month ago

I will update the template to include a decision column. I think we should just leave it blank for a few reasons.

Sounds good!

jthompson-arcus commented 1 month ago

@aclark02-arcus I made all suggested changes except making the decision non-case sensitive. I'm not opposed to this, but we would need to make sure the decisions themselves are not case sensitive first.