Improvements to access helper.

o Rename access::remove_group() to access::delete_group() for
  consistency.

o Wrote more unit tests

o Tests found a bug in access::remove_item() .. yay!

3 files changed, 85 insertions, 34 deletions

diff --git a/core/helpers/access.php b/core/helpers/access.php
index 96f4d375..95b9819c 100644
--- a/core/helpers/access.php
+++ b/core/helpers/access.php
@@ -197,7 +197,7 @@ class access_Core {
    * @param Group_Model $group
    * @return void
    */
-  public static function remove_group($group) {
+  public static function delete_group($group) {
     foreach (ORM::factory("permission")->find_all() as $perm) {
       self::_drop_columns($perm->name, $group->id);
     }
@@ -236,9 +236,9 @@ class access_Core {
    * @param Item_Model $item
    * @return void
    */
-  public static function remove_item($item) {
-    ORM::factory("access_intent")->where("item_id", $item->id)->delete();
-    ORM::factory("access_cache")->where("item_id", $item->id)->delete();
+  public static function delete_item($item) {
+    ORM::factory("access_intent")->where("item_id", $item->id)->find()->delete();
+    ORM::factory("access_cache")->where("item_id", $item->id)->find()->delete();
   }
 
   /**
diff --git a/core/helpers/core_event.php b/core/helpers/core_event.php
index 3cf9f12f..f9ab2ed0 100644
--- a/core/helpers/core_event.php
+++ b/core/helpers/core_event.php
@@ -24,7 +24,7 @@ class core_event_Core {
   }
 
   public static function group_before_delete($group) {
-    access::remove_group($group);
+    access::delete_group($group);
   }
 
   public static function photo_created($photo) {
@@ -32,7 +32,7 @@ class core_event_Core {
   }
 
   public static function photo_before_delete($photo) {
-    access::remove_item($photo);
+    access::delete_item($photo);
   }
 
   public static function album_created($album) {
@@ -40,6 +40,6 @@ class core_event_Core {
   }
 
   public static function album_before_delete($album) {
-    access::remove_item($album);
+    access::delete_item($album);
   }
 }
diff --git a/core/tests/Access_Helper_Test.php b/core/tests/Access_Helper_Test.php
index 6250d5c4..0f453f15 100644
--- a/core/tests/Access_Helper_Test.php
+++ b/core/tests/Access_Helper_Test.php
@@ -20,62 +20,113 @@
 class Access_Helper_Test extends Unit_Test_Case {
   private $_group;
 
-  public function setup() {
-    access::register_permission("access_test");
-    $this->_group = group::create("access_test");
-  }
-
   public function teardown() {
-    if ($this->_group) {
-      group::delete($this->_group->id);
+    try {
+      $group = ORM::factory("group")->where("name", "access_test")->find();
+      if ($group->loaded) {
+        group::delete($group->id);
+      }
+    } catch (Exception $e) { }
+
+    try {
       access::delete_permission("access_test");
-    }
+    } catch (Exception $e) { }
   }
 
-  public function new_groups_and_permissions_add_columns_test() {
+  public function groups_and_permissions_are_bound_to_columns_test() {
+    access::register_permission("access_test");
+    $group = group::create("access_test");
+
+    // We have a new column for this perm / group combo
     $fields = Database::instance()->list_fields("access_caches");
-    $this->assert_true(array_key_exists("access_test_{$this->_group->id}", $fields));
-  }
+    $this->assert_true(array_key_exists("access_test_{$group->id}", $fields));
 
-  public function deleting_groups_and_permissions_removes_columns_test() {
-    group::delete($this->_group->id);
     access::delete_permission("access_test");
+    group::delete($group->id);
+
+    // Now the column has gone away
     $fields = Database::instance()->list_fields("access_caches");
-    $this->assert_false(array_key_exists("access_test_{$this->_group->id}", $fields));
-    $this->_group = null;  // So that we don't try to clean this up in teardown
+    $this->assert_false(array_key_exists("access_test_{$group->id}", $fields));
   }
 
+  public function adding_and_removing_items_adds_ands_removes_rows_test() {
+    $item = ORM::factory("item")->add_to_parent(1);
 
-  public function can_view_item_test() {
-  }
+    // Simulate an event
+    access::add_item($item);
 
-  public function cant_view_child_of_hidden_parent_test() {
+    // New rows exist
+    $this->assert_true(ORM::factory("access_cache")->where("item_id", $item->id)->find()->loaded);
+    $this->assert_true(ORM::factory("access_intent")->where("item_id", $item->id)->find()->loaded);
+
+    // Simulate a delete event
+    access::delete_item($item);
+
+    // Rows are gone
+    $this->assert_false(ORM::factory("access_cache")->where("item_id", $item->id)->find()->loaded);
+    $this->assert_false(ORM::factory("access_intent")->where("item_id", $item->id)->find()->loaded);
+
+    $item->delete();
   }
 
-  public function view_permissions_propagate_down_test() {
+  public function can_allow_deny_and_reset_intent_test() {
+    $item = ORM::factory("item")->add_to_parent(1);
+    access::add_item($item);
+    $intent = ORM::factory("access_intent")->where("item_id", $item->id)->find();
+
+    // Allow
+    access::allow(0, "view", $item->id);
+    $this->assert_same(access::ALLOW, $intent->reload()->view_0);
+
+    // Deny
+    access::deny(0, "view", $item->id);
+    $this->assert_same(
+      access::DENY,
+      ORM::factory("access_intent")->where("item_id", $item->id)->find()->view_0);
+
+    // Allow again.  If the initial value was allow, then the first Allow clause above may not
+    // have actually changed any values.
+    access::allow(0, "view", $item->id);
+    $this->assert_same(
+      access::ALLOW,
+      ORM::factory("access_intent")->where("item_id", $item->id)->find()->view_0);
+
+    access::reset(0, "view", $item->id);
+    $this->assert_same(
+      null,
+      ORM::factory("access_intent")->where("item_id", $item->id)->find()->view_0);
+
+    $item->delete();
   }
 
-  public function revoked_view_permissions_cant_be_allowed_lower_down_test() {
+  public function cant_reset_root_item_test() {
+    try {
+      access::reset(0, "view", 1);
+    } catch (Exception $e) {
+      return;
+    }
+    $this->assert_true(false, "Should not be able to reset root intent");
   }
 
-  public function can_reset_intent_test() {
+
+  public function can_view_item_test() {
   }
 
-  public function can_edit_item_test() {
+  public function cant_view_child_of_hidden_parent_test() {
   }
 
-  public function cant_reset_root_item_test() {
+  public function view_permissions_propagate_down_test() {
   }
 
-  public function non_view_permissions_propagate_down_test() {
+  public function revoked_view_permissions_cant_be_allowed_lower_down_test() {
   }
 
-  public function non_view_permissions_can_be_revoked_lower_down_test() {
+  public function can_edit_item_test() {
   }
 
-  public function adding_items_adds_rows_test() {
+  public function non_view_permissions_propagate_down_test() {
   }
 
-  public function removing_items_remove_rows_test() {
+  public function non_view_permissions_can_be_revoked_lower_down_test() {
   }
 }