Clean up and clarify the OgAccess unit tests #230
Conversation
|
This is still in progress since it builds on top of #226. |
pfrenssen
changed the title
|
Great, #226 is in so this is unblocked! |
| // @todo This is strange, 'view' is not part of the operations supplied by | ||
| // ::operationProvider(). And why would a group administrator be allowed | ||
| // access to all operations, except 'view'? Shouldn't this also return | ||
| // 'allowed'? |
There was a problem hiding this comment.
This is puzzling to me. It seems to be the intention to hand off the 'view' permission to core, but this is counterintuitive. What would be the use case of not granting 'view' access to a group admin, while all the other permissions and operations are granted?
There was a problem hiding this comment.
I'll have to go in and check - I don't really remember what's going on here ;)
There was a problem hiding this comment.
this is related to the incorrect crud operation To permission name bug/ feature we have.
The gist of things is - OG's hook_entity_access() cares only about non-view operations. If it's a view we are neutral about it. The reason is that for example node access should be determined by the node grants, and not on the fly.
So this
$user_entity_access = og_entity_access($this->entity->reveal(), $operation, $this->user->reveal());should probably change to something like
$permission_name = OgPermissionHandler::getPermissionFromEntityOperation($entity, 'view');
$user_entity_access = og_entity_access($this->entity->reveal(), $permissions_name, $this->user->reveal());It can probably be a follow up, as it's out of the scope of this PR. So maybe open an issue instead of the @todo?
There was a problem hiding this comment.
sorry I mean $permission_name = OgPermissionHandler::getPermissionFromEntityOperation($entity, 'view'); should happen inside \Drupal\og\OgAccess::userAccessEntity, so the test can remain as is.
|
This also conflicts alot with my PR. Seems like this can totally wait?
|
|
@pfrenssen cut me a break, #225 should at least get a look in first, after #226 was merged :) |
|
I'm relentless! :D No this is obviously low priority, let's not have it complicate more important PRs. |
|
#225 is in, as far as I know this will not impact anything else any more. I merged in the latest changes from 8.x-1.x. |
| * @return array | ||
| * An array of test operations. | ||
| * | ||
| * @todo These are not really 'operations' but rather 'permissions', rename |
There was a problem hiding this comment.
permissionsProvider() sounds better.
|
Thanks for the review. I am going to have a look at this again later. I have other fish to fry at the moment :) |
|
Found the time to address this. Tests will now probably fail due to https://www.drupal.org/node/2752315 |
|
This is becoming like a game of whack-a-mole, issue #2752315: Fix visibility of AssertLegacyTrait::assertNoEscaped() is now fixed, and just a few hours later a similar issue popped up: issue #2757139: Fix visibility of AssertLegacyTrait::buildXPathQuery(). I have posted a fix in that issue, if by tomorrow it is not accepted I will make a workaround for us so we can continue working on OG. |
|
The bug in core is fixed! I have restarted the test and now everything is green. This is ready for review again! |
|
Thanks! |
OgDeleteOrphans Simple Plugin does not delete content
The OgAccess unit tests are sparsely documented which makes them very confusing to work with. Also it has a property which is very vaguely named
$entity, but it is not clear when looking at the tests whether this is a group, or group content, or any other entity.Let's add some documentation and do some general spring cleaning.