Repository navigation
#2516 change permissions check to match core - #2571
nullvariable wants to merge 3 commits into
Conversation
Current coverage is 94.62%@@ develop #2571 diff @@
==========================================
Files 11 11
Lines 3644 3644
Methods 172 172
Messages 0 0
Branches 0 0
==========================================
+ Hits 3447 3448 +1
+ Misses 197 196 -1
Partials 0 0
|
Can you make sure there's a test case included for the scenario outlined on the original issue? |
|
@danielbachhuber sure, the existing test case seemed to work well, it tests if a subscriber can upload (a role which does not have upload_files), there is also a test case for uploads working for a user with edit_posts, which in a default role setup overlaps with admin/editor/author being the only roles with upload_files. So the test I would write would add a new role with only the upload files capability and then test if that role can upload a file, does that sound like what you're looking for? |
Yep |
extended the setup function to create a custom role with 'upload_files' permission and created two tests which check if a user with upload_files can create attachments, and also if a user with upload attachments but no edit permissions is rejected when attempting to add an attachment to a post they do not have permissions on.
|
@danielbachhuber that should do the trick |
|
This looks good to me, presuming this is core's behaviour which I haven't actually verified. Given updating the attachment probably does require |
|
@joehoyle I reviewed all the places that I could find in core here: #2516 (comment) as best I could tell with core you can create an attachment without |
| 'role' => 'contributor', | ||
| ) ); | ||
| //matches core capabilities for subscriber role and adds upload_files | ||
| _x( 'File upload role', 'User role' ); |
There was a problem hiding this comment.
What's this thing doing here? :) Don't think it's needed?
There was a problem hiding this comment.
well 'File upload role' in line 25 should be translated but doesn't need to be for tests I guess
There was a problem hiding this comment.
Right, but you are not assigning it to anything, no?
There was a problem hiding this comment.
Correct. We'd either need to add a variable and pass it to the add_role function or just remove it since our goal here isn't really to test translations. I can modify it either way, but I'd lean towards removing it since it's not the goal of the test and won't really have an impact either way.
|
From PR review today, these are the outstanding TODOs here:
@nullvariable we're moving this to needs-refresh (discussion) |
|
@nullvariable Thanks for your contribution here. I verified this is actually how core works (as well as that is possible given the variance) and created a refreshed patch (cleaning up the unit test a bit) in #2743. |
6 participants
update permissions check to match core. Removes checks for create/edit_post permissions in favor of upload_files. Should resolve #2516