-
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
add settings for user role selection #76
Conversation
includes/safe-svg-settings.php
Outdated
* User role field callback function. | ||
*/ | ||
public function safe_svg_roles_cb() { | ||
$user_roles = get_editable_roles(); |
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.
This will show all roles, even those that can't do anything with media currently. Feels like we should trim this list down to only roles that currently have media upload capabilities to begin with, otherwise I could see confusion when someone sees Subscriber
on this list, even though they can't upload any types of media
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.
I've updated this in the latest push, it will be filtered to roles that have the upload_files permission. I've also added a filter for these roles.
@dkotter looking for an update code review from you here, thanks! |
@faisal-alvi if you're able to review during your OSS week that'll help move this along, thanks! |
I've reviewed the code here a few times and things are looking good from my perspective. Could use help in actually testing this out, want to ensure this works as expected and doesn't introduce any regressions. The main thing I'm concerned about is ensuring that anyone that can currently upload an SVG should still be able to upload an SVG after this update, at least until this new setting has been modified from the default. Edit: looks like also a merge conflict that we can clean up here |
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.
@dhanendran & @csloisel thank you for the great work here!
Note: this is a report of testing only, as the Code Review is already been done by @dkotter.
- ✅ A settings section to select user roles who can upload SVG images is displayed correctly.
- ✅ I have tested and confirmed the functionality is working fine.
- ✅ Only the user roles allowed in settings can upload the SVG.
- ✅ If a user with a specific role is not allowed to upload, the following error is displayed and the SVG image does not upload.
Resolved the merge commit, but seeing PHPUnit failure that we'll want to resolve before merging |
@10up/open-source-practice would be good to get a final code review pass on this before merging in... thanks! |
Description of the Change
Added a settings section to select user roles who can upload SVG images. Based on the role section, a new capability
safe_svg_upload_svg
will be added to those roles. By default, super admins on a network site will have all the capabilities, so they can upload SVG files.Closes #69
How to test the Change
/wp-admin/uploads.php
page/wp-admin/options-media.php
pageAdministrator
role and submit the form/wp-admin/uploads.php
pageChangelog Entry
Feature - Add a new option to select which user roles can upload SVG files.
Credits
Props @dhanendran, @csloisel, @faisal-alvi
Checklist: