Clean up and clarify the OgAccess unit tests

tests/src/Unit/OgAccessEntityTest.php

@@ -23,7 +23,8 @@ class OgAccessEntityTest extends OgAccessEntityTestBase {
   public function testAccessByOperation($operation) {
     $group_entity = $this->groupEntity();
     $group_entity->isNew()->willReturn(FALSE);
-    $user_access = $this->ogAccess->userAccessEntity($operation, $this->entity->reveal(), $this->user->reveal());
+
+    $user_access = $this->ogAccess->userAccessEntity($operation, $this->groupContentEntity->reveal(), $this->user->reveal());
 
     // We populate the allowed permissions cache in
     // OgAccessEntityTestBase::setup().
@@ -48,7 +49,7 @@ public function testEntityNew($operation) {
    */
   public function testGetEntityGroups($operation) {
     $this->user->hasPermission(OgAccess::ADMINISTER_GROUP_PERMISSION)->willReturn(TRUE);
-    $user_entity_access = $this->ogAccess->userAccessEntity($operation, $this->entity->reveal(), $this->user->reveal());
+    $user_entity_access = $this->ogAccess->userAccessEntity($operation, $this->groupContentEntity->reveal(), $this->user->reveal());
     $this->assertTrue($user_entity_access->isAllowed());
   }
 

tests/src/Unit/OgAccessEntityTestBase.php

@@ -21,18 +21,20 @@
 
 class OgAccessEntityTestBase extends OgAccessTestBase {
 
-  protected $entity;
-
+  /**
+   * A test group content entity.
+   *
+   * @var \Drupal\Core\Entity\ContentEntityInterface|\Prophecy\Prophecy\ObjectProphecy
+   */
+  protected $groupContentEntity;
+
+  /**
+   * {@inheritdoc}
+   */
   public function setup() {
     parent::setUp();
 
-    $field_definition = $this->prophesize(FieldDefinitionInterface::class);
-    $field_definition->getType()->willReturn(OgGroupAudienceHelper::NON_USER_TO_GROUP_REFERENCE_FIELD_TYPE);
-    $field_definition->getFieldStorageDefinition()
-      ->willReturn($this->prophesize(FieldStorageDefinitionInterface::class)->reveal());
-    $field_definition->getSetting("handler_settings")->willReturn([]);
-    $field_definition->getName()->willReturn($this->randomMachineName());
-
+    // Mock a group content entity.
     $entity_type_id = $this->randomMachineName();
     $bundle = $this->randomMachineName();
 
@@ -44,15 +46,24 @@ public function setup() {
     $entity_type->isSubclassOf(FieldableEntityInterface::class)->willReturn(TRUE);
     $entity_type->id()->willReturn($entity_type_id);
 
-    $this->entity = $this->prophesize(ContentEntityInterface::class);
-    $this->entity->id()->willReturn($entity_id);
-    $this->entity->bundle()->willReturn($bundle);
-    $this->entity->isNew()->willReturn(FALSE);
-    $this->entity->getEntityType()->willReturn($entity_type->reveal());
-    $this->entity->getEntityTypeId()->willReturn($entity_type_id);
+    $this->groupContentEntity = $this->prophesize(ContentEntityInterface::class);
+    $this->groupContentEntity->id()->willReturn($entity_id);
+    $this->groupContentEntity->bundle()->willReturn($bundle);
+    $this->groupContentEntity->isNew()->willReturn(FALSE);
+    $this->groupContentEntity->getEntityType()->willReturn($entity_type->reveal());
+    $this->groupContentEntity->getEntityTypeId()->willReturn($entity_type_id);
 
+    // The group manager is expected to declare that this is not a group.
     $this->groupManager->isGroup($entity_type_id, $bundle)->willReturn(FALSE);
 
+    // Mock retrieval of field definitions.
+    $field_definition = $this->prophesize(FieldDefinitionInterface::class);
+    $field_definition->getType()->willReturn(OgGroupAudienceHelper::NON_USER_TO_GROUP_REFERENCE_FIELD_TYPE);
+    $field_definition->getFieldStorageDefinition()
+      ->willReturn($this->prophesize(FieldStorageDefinitionInterface::class)->reveal());
+    $field_definition->getSetting('handler_settings')->willReturn([]);
+    $field_definition->getName()->willReturn($this->randomMachineName());
+
     $entity_field_manager = $this->prophesize(EntityFieldManagerInterface::class);
     $entity_field_manager->getFieldDefinitions($entity_type_id, $bundle)->willReturn([$field_definition->reveal()]);
 
@@ -64,7 +75,6 @@ public function setup() {
     $entity_type_manager->getDefinition($entity_type_id)->willReturn($entity_type->reveal());
     $entity_type_manager->getStorage($group_type_id)->willReturn($storage->reveal());
 
-
     $container = \Drupal::getContainer();
     $container->set('entity_type.manager', $entity_type_manager->reveal());
     $container->set('entity_field.manager', $entity_field_manager->reveal());

tests/src/Unit/OgAccessHookTest.php

@@ -1,11 +1,7 @@
 <?php
 
-/**
- * @file
- * Contains \Drupal\Tests\og\Unit\OgAccessHookTest.
- */
-
 namespace Drupal\Tests\og\Unit;
+
 use Drupal\Core\Entity\EntityInterface;
 use Drupal\og\OgAccess;
 
@@ -14,6 +10,9 @@
  */
 class OgAccessHookTest extends OgAccessEntityTestBase {
 
+  /**
+   * {@inheritdoc}
+   */
   public function setUp() {
     parent::setUp();
     // Since this is a unit test, we don't enable the module. However, we test
@@ -22,25 +21,38 @@ public function setUp() {
   }
 
   /**
+   * Tests that an entity which is not a group or group content is ignored.
+   *
    * @dataProvider operationProvider
    */
   public function testNotContentEntity($operation) {
     $entity = $this->prophesize(EntityInterface::class);
     $access = og_entity_access($entity->reveal(), $operation, $this->user->reveal());
+
+    // An entity which is not a group or group content should always return
+    // neutral, since we have no opinion over it.
     $this->assertTrue($access->isNeutral());
   }
 
   /**
+   * Tests that an administrator with 'administer group' permission has access.
+   *
    * @dataProvider operationProvider
    */
   public function testGetEntityGroups($operation) {
     $this->user->hasPermission(OgAccess::ADMINISTER_GROUP_PERMISSION)->willReturn(TRUE);
-    $user_entity_access = og_entity_access($this->entity->reveal(), $operation, $this->user->reveal());
+    $user_entity_access = og_entity_access($this->groupContentEntity->reveal(), $operation, $this->user->reveal());
+
+    // @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'?
     if ($operation == 'view') {
       $this->assertTrue($user_entity_access->isNeutral());
     }
     else {
       $this->assertTrue($user_entity_access->isAllowed());
     }
   }
+
 }

tests/src/Unit/OgAccessTestBase.php

@@ -1,10 +1,5 @@
 <?php
 
-/**
- * @file
- * Contains \Drupal\Tests\og\Unit\OgAccessTestBase.
- */
-
 namespace Drupal\Tests\og\Unit;
 
 use Drupal\Core\Cache\Context\CacheContextsManager;
@@ -22,29 +17,50 @@
 use Drupal\user\EntityOwnerInterface;
 use Prophecy\Argument;
 
+/**
+ * Base class for tests of the OgAccess class.
+ */
 class OgAccessTestBase extends UnitTestCase {
 
+  /**
+   * The mocked config handler.
+   *
+   * @var \Drupal\Core\Config\Config|\Prophecy\Prophecy\ObjectProphecy
+   */
   protected $config;
 
   /**
-   * @var \Drupal\user\UserInterface
+   * A mocked test user.
+   *
+   * @var \Drupal\user\UserInterface|\Prophecy\Prophecy\ObjectProphecy
    */
   protected $user;
 
   /**
+   * The entity type ID of the test group.
+   *
    * @var string
    */
   protected $entityTypeId;
 
   /**
+   * The bundle ID of the test group.
+   *
    * @var string
    */
   protected $bundle;
 
+  /**
+   * The mocked test group.
+   *
+   * @var \Drupal\Core\Entity\EntityInterface|\Prophecy\Prophecy\ObjectProphecy
+   */
   protected $group;
 
   /**
-   * @var \Drupal\og\GroupManager
+   * The mocked group manager.
+   *
+   * @var \Drupal\og\GroupManager|\Prophecy\Prophecy\ObjectProphecy
    */
   protected $groupManager;
 
@@ -87,6 +103,7 @@ public function setUp() {
     $this->config->getCacheTags()->willReturn([]);
     $this->config->getCacheMaxAge()->willReturn(0);
 
+    // Create a mocked test user.
     $this->user = $this->prophesize(AccountInterface::class);
     $this->user->isAuthenticated()->willReturn(TRUE);
     $this->user->id()->willReturn(2);
@@ -106,6 +123,7 @@ public function setUp() {
 
     $entity_id = 20;
 
+    // Mock all dependencies for the system under test.
     $account_proxy = $this->prophesize(AccountProxyInterface::class);
     $module_handler = $this->prophesize(ModuleHandlerInterface::class);
 
@@ -148,7 +166,9 @@ public function setUp() {
 
     $reflection_property->setValue($values);
 
-    // Set the allowed permissions cache.
+    // Set the allowed permissions cache. This simulates that the access results
+    // have been retrieved from the database in an earlier pass. This saves us
+    // from having to mock all the database interaction.
     $r = new \ReflectionClass('Drupal\og\OgAccess');
     $reflection_property = $r->getProperty('permissionsCache');
     $reflection_property->setAccessible(TRUE);
@@ -162,19 +182,26 @@ public function setUp() {
   }
 
   /**
+   * Returns a mocked test group.
+   *
    * @param bool $is_owner
+   *   Whether or not this test group should be owned by the test user which is
+   *   used in the test.
    *
-   * @return \Drupal\Core\Entity\EntityInterface
+   * @return \Drupal\Core\Entity\EntityInterface|\Prophecy\Prophecy\ObjectProphecy
+   *   The test group.
    */
   protected function groupEntity($is_owner = FALSE) {
     $group_entity = $this->prophesize(EntityInterface::class);
     if ($is_owner) {
       $group_entity->willImplement(EntityOwnerInterface::class);
+      // Our test user is hardcoded to have UID 2.
       $group_entity->getOwnerId()->willReturn(2);
     }
     $group_entity->getEntityTypeId()->willReturn($this->entityTypeId);
     $group_entity->bundle()->willReturn($this->bundle);
     $group_entity->id()->willReturn($this->randomMachineName());
+
     return $this->addCache($group_entity);
   }
 
@@ -188,6 +215,15 @@ protected function addCache($prophecy) {
     return $prophecy;
   }
 
+  /**
+   * Provides operations to use in access tests.
+   *
+   * @return array
+   *   An array of test operations.
+   *
+   * @todo These are not really 'operations' but rather 'permissions', rename
+   *   this?
+   */
   public function operationProvider() {
     return [
       // In the unit tests we don't really care about the permission name - it