-
Notifications
You must be signed in to change notification settings - Fork 31
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
Allow assigning decisions from upload file #777
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jthompson-arcus,
Looks good! Couple of things I noticed that could be tweaked:
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.
And then we can include a note here in the introJS
text about Decision
being an optional field.
Update documentation
Speaking of documenting this new feature, we should update our Get Started
vignette here:
Uploading packages can be done by manually typing one (or more) package names into the prompt or, if you have a long list of packages to asses, it may be more convenient to upload a CSV file of the package names. The CSV file requires two columns: “package” and “version.” However, right now `riskmetric` can only tackle the current version, so the version column is ignored. In the meantime, you can make your life easier by setting version to “0.0.0” for all of your packages in the CSV. |
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...
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?
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?
Thanks! Overall, this PR is operating swell!
Update the "Sample Dataset"
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
I will take a look.
|
Use the test package references when running in test mode
Assigns name to "Batch Upload" instead of the user
@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. |
The folder `data-raw` is not compiled with the package so it should not be used
@jthompson-arcus yes, but it didn't happen during this PR. I noticed it a few PRs ago... |
Sounds good! |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! I noticed #781 while testing, but it's unrelated to this PR, so I created a new issue. Thanks @jthompson-arcus !
closes #663