-
Notifications
You must be signed in to change notification settings - Fork 157
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 dt_syndicatable_capabilities
filter to push ui
#473
add dt_syndicatable_capabilities
filter to push ui
#473
Conversation
Just checking in on this PR - looks like the CI build is failing but I'm not too sure why... Is there anything I can do (or need to change) to help it pass the appropriate checks? I also popped a question on issue #473 about documentation - is the inline code doc block enough or is there another place I should document the new filter? |
@pragmatic-tf the doc block should be sufficient, as once that gets merged it should appear on our technical documentation site: https://10up.github.io/distributor/. As for the failing build, I'm uncertain what's going on there and will see if we can figure that out while reviewing this PR. Thanks for checking in and for the PR, happy holidays! |
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.
capabilities
is a plural word, so from the filter name, I would expect passing multiple capabilities to this filter. We have two options here:
- change the filter name to the singular
- or change the code to handle multiple capabilities
cc @jeffpaul
Hey @dinhtungdu 👋 totally get where you're coming from, and it's a good point. Use of the plural was in order to stay consistent with a few other capability filters in the rest of the plugin codebase: ☝️ The above all handle single capability filters, but use the plural, so it all depends on whether the preference is to maintain consistency or go for semantic correctness? More than happy to update the PR for either. It's worth noting that |
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.
@pragmatic-tf thanks for the head up, I haven't checked the naming convention, so I'm changing my review now.
About the multiple capabilities, we can do it by looping through the array and apply current_user_can
for each but it will break the current convention, so let's ignore it for now.
Description of the Change
Adds a new filter to the user capability check found in the
syndicatable()
function, which controls access to the Push UI in the WP admin bar.Alternate Designs
N/A
Benefits
This will allow other plugins to dynamically update the capability required to enable the push UI in the WP admin bar.
Possible Drawbacks
None that I can foresee - the code addition doesn't alter any existing functionality by itself.
Verification Process
After applying the code change, I tested the filter on a local development install by changing the capability to allow different user roles access to the Push UI.
read
allowed a test Subscriber to use the Push UIedit_published_posts
prevented a test Contributor from using the Push UI but still allowed a test Author to use it.dt_test_cap
- prevented any users from accessing the Push UI, except super-admins.dt_test_cap
custom capability to the Author role, then allowed a test Author to access the Push UI.Checklist:
Applicable Issues
See: #472