PLAT-452 Update logic to include editor role #512
Conversation
The editing operation is covered by the standard permissions.
|
@pvhee what was your bhat suggestion for this ? |
|
@bimsonz something like:
That should only work with this fix, try commenting your code to make sure it doesn't work without your fix. |
| return AccessResult::allowed(); | ||
| if (in_array('reviewer', $account->getRoles()) || in_array('editor', $account->getRoles())) { | ||
| if ($op == 'view') { | ||
| return AccessResult::allowed(); |
There was a problem hiding this comment.
Only the op view is checked, how would this work to give edit access to other editors?
There was a problem hiding this comment.
as mentioned in the first comment on this thread, the edit operation seems to be covered by the built in "edit" permissions, would like this tested thoroughly though.
There was a problem hiding this comment.
Ok, so currently the permissions were already set up fine, and this change is simply about giving editor roles access to see unpublished content?
There was a problem hiding this comment.
As far as im aware, yes, speaking to liv, her main requirements were reviewers are able to see any page regardless of status and editors, edit.. @pvhee please also check the test site, details provided below
There was a problem hiding this comment.
Yep, test is working fine!
I'm just confused on this, cause this means that it could be working right now on production already, simply by adding the "reviewer" role to all the editor accounts...
There was a problem hiding this comment.
The reviewer only has view permissions though, they cant edit anything @pvhee
|
two users: http://pr-512-tx3mbsqmxtu74.eu.platform.sh/test-editor-rights Both are able to view and edit this unpublished post... |
|
@pvhee ive added an initial test but im not sure how id go about logging in with a different editor user considering i didnt define one in the first place, sure the syntax is easy, right ? |
| @api | ||
| Scenario: Check two different editors both have access to edit a single page | ||
| Given I am logged in as a user with the "editor" role | ||
| And I am viewing a "page" content with "Test page" title |
There was a problem hiding this comment.
@bimsonz if you do vendor/bin/behat -dl you see step definitions you can use. You probably need Given an **unpublished** and repeat the Given I am logged in as a user with the "editor" role step to log in again. Have a look at how they're defined, it isn't that hard
There was a problem hiding this comment.
Wicked, thanks for the point in the right direction, maybe we should add that command to view the definitions to some sort of 'developing bhat test' doc?
| /** | ||
| * @Given I am viewing an unpublished :arg1 with the title :arg2 | ||
| */ | ||
| public function iAmViewingAnUnpublishedWithTheTitle($type, $title) |
There was a problem hiding this comment.
@pvhee i had to add this.. dont like thats its dup code.. im still a mega bhat novice, maybe you can get it so i dont need it..?
There was a problem hiding this comment.
Hmm, you could reuse https://github.com/comicrelief/campaign/blob/develop/profiles/cr/tests/behat/features/bootstrap/DrupalCRFeatureContext.php#L391, no?
Then, write the behat step as Given an unpublished "page" content with the title "Unpublished page"
|
@bimsonz Some more feedback on Travis tests: could you run this locally as well and add |
…e/PLAT-452_unpublished_visible_to_editors
…e/PLAT-452_unpublished_visible_to_editors
|
Should be ready now @pvhee |
| @@ -26,3 +26,10 @@ Feature: User | |||
| Given I am on "/admin/people" | |||
| Then I should get a "403" HTTP response | |||
| And I should see "You are not authorized to access this page." | |||
|
|
|||
| @api @javascript | |||
There was a problem hiding this comment.
@bimsonz can we take off @javascript then? not needed and it'll slow down tests on Travis
There was a problem hiding this comment.
Ah sorry! done! @pvhee
|
This also is the reason this failed btw, shouldn't now fail with @javascript taken off so will merge once Travis goes green |
Fixes https://jira.comicrelief.com/browse/PLAT-452
Changes proposed in this pull request
The editing operation is covered by the standard permissions.