From b3e328c9ff4c3e19df4b6d18da947b759fe0c201 Mon Sep 17 00:00:00 2001
From: Bharat Mediratta <bharat@menalto.com>
Date: Thu, 14 Jan 2010 21:04:09 -0800
Subject: Begin the process of converting to model based validation.  Right now
 only Albums_Controller::update() supports the pattern.  All form and
 controller based validation happening when editing an album has been moved
 over.

Model based validation means that our REST controllers share the
same validation as web controllers.  We'll have consistency
enforced at the model level, which is a Good Thing.

The basic pattern is now:
1) Rules are in the model

2) ORM::validate() (which is called by ORM::save() but you can
   call it directly, too) checks the model for all the rules and
   throws an ORM_Validation_Exception if there are failures

3) Actions are no longer taken when you call Item_Model::__set().
   Instead, they're all queued up and executed when you call
   Item_Model::save().

Notes:
- item::validate_xxx() functions are now in Item_Model::
- We still call $form->validate() because the form can have
 rules (and forms triggered by events will likely continue to
 have rules.
---
 modules/gallery/controllers/albums.php | 51 ++++++++++++----------------------
 1 file changed, 17 insertions(+), 34 deletions(-)

(limited to 'modules/gallery/controllers/albums.php')

diff --git a/modules/gallery/controllers/albums.php b/modules/gallery/controllers/albums.php
index 2eeefdf1..8ad3ff72 100644
--- a/modules/gallery/controllers/albums.php
+++ b/modules/gallery/controllers/albums.php
@@ -129,42 +129,27 @@ class Albums_Controller extends Items_Controller {
     access::required("edit", $album);
 
     $form = album::get_edit_form($album);
-    if ($valid = $form->validate()) {
-      if ($album->id != 1 &&
-          $form->edit_item->dirname->value != $album->name ||
-          $form->edit_item->slug->value != $album->slug) {
-        // Make sure that there's not a conflict
-        if ($row = db::build()
-            ->select(array("name", "slug"))
-            ->from("items")
-            ->where("parent_id", "=", $album->parent_id)
-            ->where("id", "<>", $album->id)
-            ->and_open()
-            ->where("name", "=", $form->edit_item->dirname->value)
-            ->or_where("slug", "=", $form->edit_item->slug->value)
-            ->close()
-            ->execute()
-            ->current()) {
-          if ($row->name == $form->edit_item->dirname->value) {
-            $form->edit_item->dirname->add_error("name_conflict", 1);
-          }
-          if ($row->slug == $form->edit_item->slug->value) {
-            $form->edit_item->slug->add_error("slug_conflict", 1);
-          }
-          $valid = false;
-        }
-      }
-    }
-
-    if ($valid) {
+    try {
+      $valid = $form->validate();
       $album->title = $form->edit_item->title->value;
       $album->description = $form->edit_item->description->value;
       $album->sort_column = $form->edit_item->sort_order->column->value;
       $album->sort_order = $form->edit_item->sort_order->direction->value;
-      if ($album->id != 1) {
-        $album->rename($form->edit_item->dirname->value);
-      }
+      $album->name = $form->edit_item->dirname->value;
       $album->slug = $form->edit_item->slug->value;
+      $album->validate();
+    } catch (ORM_Validation_Exception $e) {
+      // Translate ORM validation errors into form error messages
+      foreach ($e->validation->errors() as $key => $error) {
+        if ($key == "name") {
+          $key = "dirname";
+        }
+        $form->edit_item->inputs[$key]->add_error($error, 1);
+      }
+      $valid = false;
+    }
+
+    if ($valid) {
       $album->save();
       module::event("item_edit_form_completed", $album, $form);
 
@@ -180,9 +165,7 @@ class Albums_Controller extends Items_Controller {
         print json_encode(array("result" => "success"));
       }
     } else {
-      print json_encode(
-        array("result" => "error",
-              "form" => $form->__toString()));
+      print json_encode(array("result" => "error", "form" => (string) $form));
     }
   }
 
-- 
cgit v1.2.3


From 5809949ae8ff87cd5acf56c528e6dc2af6619513 Mon Sep 17 00:00:00 2001
From: Bharat Mediratta <bharat@menalto.com>
Date: Fri, 15 Jan 2010 11:28:05 -0800
Subject: Don't use Input directly to get album names, etc.  Use the form
 fields.

---
 modules/gallery/controllers/albums.php | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

(limited to 'modules/gallery/controllers/albums.php')

diff --git a/modules/gallery/controllers/albums.php b/modules/gallery/controllers/albums.php
index 8ad3ff72..9f8c16ea 100644
--- a/modules/gallery/controllers/albums.php
+++ b/modules/gallery/controllers/albums.php
@@ -95,16 +95,16 @@ class Albums_Controller extends Items_Controller {
     access::required("view", $album);
     access::required("add", $album);
 
-    $input = Input::instance();
     $form = album::get_add_form($album);
     if ($form->validate()) {
       $new_album = album::create(
         $album,
-        $input->post("name"),
-        $input->post("title", $input->post("name")),
-        $input->post("description"),
+        $form->add_album->inputs["name"]->value,
+        $form->add_album->title->value ?
+          $form->add_album->title->value : $form->add_album->inputs["name"]->value,
+        $form->add_album->description->value,
         identity::active_user()->id,
-        $input->post("slug"));
+        $form->add_album->slug->value);
 
       log::success("content", "Created an album",
                    html::anchor("albums/$new_album->id", "view album"));
-- 
cgit v1.2.3


From 50e3cc5837df7b0ae8e2d43a3dacee7500ba6db8 Mon Sep 17 00:00:00 2001
From: Bharat Mediratta <bharat@menalto.com>
Date: Fri, 15 Jan 2010 12:15:20 -0800
Subject: Move model business logic out of album::create() and into
 Item_Model::save().  This makes creating albums similar to editing them and
 makes it difficult to create an album poorly.  I expect to be able to remove
 a lot of code from the photo and movie helper because it's duplicated here.

In order to do this, I refactored ORM_MPTT::add_to_parent() into
ORM_MPTT::save() so we now add it to the parent when we do save.  This
allows us to call save() only once which saves a database call per
add.

The Albums_Controller logic is roughly the same as before.

Haven't updated the tests yet, they're going to fail miserably since
many of them depend on album::create() which is now gone.
---
 modules/gallery/controllers/albums.php | 42 +++++++++++--------
 modules/gallery/helpers/album.php      | 67 ------------------------------
 modules/gallery/libraries/ORM_MPTT.php | 59 +++++++++++++-------------
 modules/gallery/models/item.php        | 75 +++++++++++++++++++++++++++++-----
 4 files changed, 118 insertions(+), 125 deletions(-)

(limited to 'modules/gallery/controllers/albums.php')

diff --git a/modules/gallery/controllers/albums.php b/modules/gallery/controllers/albums.php
index 9f8c16ea..7658a913 100644
--- a/modules/gallery/controllers/albums.php
+++ b/modules/gallery/controllers/albums.php
@@ -96,29 +96,35 @@ class Albums_Controller extends Items_Controller {
     access::required("add", $album);
 
     $form = album::get_add_form($album);
-    if ($form->validate()) {
-      $new_album = album::create(
-        $album,
-        $form->add_album->inputs["name"]->value,
-        $form->add_album->title->value ?
-          $form->add_album->title->value : $form->add_album->inputs["name"]->value,
-        $form->add_album->description->value,
-        identity::active_user()->id,
-        $form->add_album->slug->value);
+    try {
+      $valid = $form->validate();
+      $album = ORM::factory("item");
+      $album->type = "album";
+      $album->parent_id = $parent_id;
+      $album->name = $form->add_album->inputs["name"]->value;
+      $album->title = $form->add_album->title->value ?
+        $form->add_album->title->value : $form->add_album->inputs["name"]->value;
+      $album->description = $form->add_album->description->value;
+      $album->slug = $form->add_album->slug->value;
+      $album->validate();
+    } catch (ORM_Validation_Exception $e) {
+      // Translate ORM validation errors into form error messages
+      foreach ($e->validation->errors() as $key => $error) {
+        $form->add_album->inputs[$key]->add_error($error, 1);
+      }
+      $valid = false;
+    }
 
+    if ($valid) {
+      $album->save();
       log::success("content", "Created an album",
-                   html::anchor("albums/$new_album->id", "view album"));
+                   html::anchor("albums/$album->id", "view album"));
       message::success(t("Created album %album_title",
-                         array("album_title" => html::purify($new_album->title))));
+                         array("album_title" => html::purify($album->title))));
 
-      print json_encode(
-        array("result" => "success",
-              "location" => $new_album->url()));
+      print json_encode(array("result" => "success", "location" => $album->url()));
     } else {
-      print json_encode(
-        array(
-          "result" => "error",
-          "form" => $form->__toString()));
+      print json_encode(array("result" => "error", "form" => (string) $form));
     }
   }
 
diff --git a/modules/gallery/helpers/album.php b/modules/gallery/helpers/album.php
index 52759414..e99770e9 100644
--- a/modules/gallery/helpers/album.php
+++ b/modules/gallery/helpers/album.php
@@ -24,71 +24,6 @@
  * Note: by design, this class does not do any permission checking.
  */
 class album_Core {
-  /**
-   * Create a new album.
-   * @param integer $parent_id id of parent album
-   * @param string  $name the name of this new album (it will become the directory name on disk)
-   * @param integer $title the title of the new album
-   * @param string  $description (optional) the longer description of this album
-   * @param string  $slug (optional) the url component for this photo
-   * @return Item_Model
-   */
-  static function create($parent, $name, $title, $description=null, $owner_id=null, $slug=null) {
-    if (!$parent->loaded() || !$parent->is_album()) {
-      throw new Exception("@todo INVALID_PARENT");
-    }
-
-    if (strpos($name, "/")) {
-      throw new Exception("@todo NAME_CANNOT_CONTAIN_SLASH");
-    }
-
-    // We don't allow trailing periods as a security measure
-    // ref: http://dev.kohanaphp.com/issues/684
-    if (rtrim($name, ".") != $name) {
-      throw new Exception("@todo NAME_CANNOT_END_IN_PERIOD");
-    }
-
-    if (empty($slug)) {
-      $slug = item::convert_filename_to_slug($name);
-    }
-
-    $album = ORM::factory("item");
-    $album->type = "album";
-    $album->title = $title;
-    $album->description = $description;
-    $album->name = $name;
-    $album->owner_id = $owner_id;
-    $album->thumb_dirty = 1;
-    $album->resize_dirty = 1;
-    $album->slug = $slug;
-    $album->sort_column = "created";
-    $album->sort_order = "ASC";
-
-    // Randomize the name or slug if there's a conflict
-    // @todo Improve this.  Random numbers are not user friendly
-    while (ORM::factory("item")
-           ->where("parent_id", "=", $parent->id)
-           ->and_open()
-           ->where("name", "=", $album->name)
-           ->or_where("slug", "=", $album->slug)
-           ->close()
-           ->find()->id) {
-      $rand = rand();
-      $album->name = "{$name}-$rand";
-      $album->slug = "{$slug}-$rand";
-    }
-
-    $album = $album->add_to_parent($parent);
-    mkdir($album->file_path());
-    mkdir(dirname($album->thumb_path()));
-    mkdir(dirname($album->resize_path()));
-
-    // @todo: publish this from inside Item_Model::save() when we refactor to the point where
-    // there's only one save() happening here.
-    module::event("item_created", $album);
-
-    return $album;
-  }
 
   static function get_add_form($parent) {
     $form = new Forge("albums/create/{$parent->id}", "", "post", array("id" => "g-add-album-form"));
@@ -97,10 +32,8 @@ class album_Core {
     $group->input("title")->label(t("Title"));
     $group->textarea("description")->label(t("Description"));
     $group->input("name")->label(t("Directory name"))
-      ->callback("item::validate_no_slashes")
       ->error_messages("no_slashes", t("The directory name can't contain the \"/\" character"));
     $group->input("slug")->label(t("Internet Address"))
-      ->callback("item::validate_url_safe")
       ->error_messages(
         "not_url_safe",
         t("The internet address should contain only letters, numbers, hyphens and underscores"));
diff --git a/modules/gallery/libraries/ORM_MPTT.php b/modules/gallery/libraries/ORM_MPTT.php
index ed77cac9..46ae0af8 100644
--- a/modules/gallery/libraries/ORM_MPTT.php
+++ b/modules/gallery/libraries/ORM_MPTT.php
@@ -40,44 +40,43 @@ class ORM_MPTT_Core extends ORM {
   }
 
   /**
-   * Add this node as a child of the parent provided.
+   * Overload ORM::save() to update the MPTT tree when we add new items to the hierarchy.
    *
    * @chainable
-   * @param integer $parent_id the id of the parent node
-   * @return ORM
+   * @return  ORM
    */
-  function add_to_parent($parent) {
-    $this->lock();
-    $parent->reload();  // Assume that the prior lock holder may have changed the parent
+  function save() {
+    if (!$this->loaded()) {
+      $this->lock();
+      $parent = ORM::factory("item")->where("id", "=", $this->parent_id)->find();
 
-    try {
-      // Make a hole in the parent for this new item
-      $this->db_builder
-        ->update($this->table_name)
-        ->set("left_ptr", new Database_Expression("`left_ptr` + 2"))
-        ->where("left_ptr", ">=", $parent->right_ptr)
-        ->execute();
-      $this->db_builder
-        ->update($this->table_name)
-        ->set("right_ptr", new Database_Expression("`right_ptr` + 2"))
-        ->where("right_ptr", ">=", $parent->right_ptr)
-        ->execute();
-      $parent->right_ptr += 2;
+      try {
+        // Make a hole in the parent for this new item
+        $this->db_builder
+          ->update($this->table_name)
+          ->set("left_ptr", new Database_Expression("`left_ptr` + 2"))
+          ->where("left_ptr", ">=", $parent->right_ptr)
+          ->execute();
+        $this->db_builder
+          ->update($this->table_name)
+          ->set("right_ptr", new Database_Expression("`right_ptr` + 2"))
+          ->where("right_ptr", ">=", $parent->right_ptr)
+          ->execute();
+        $parent->right_ptr += 2;
 
-      // Insert this item into the hole
-      $this->left_ptr = $parent->right_ptr - 2;
-      $this->right_ptr = $parent->right_ptr - 1;
-      $this->parent_id = $parent->id;
-      $this->level = $parent->level + 1;
-      $this->save();
-      $parent->reload();
-    } catch (Exception $e) {
+        // Insert this item into the hole
+        $this->left_ptr = $parent->right_ptr - 2;
+        $this->right_ptr = $parent->right_ptr - 1;
+        $this->parent_id = $parent->id;
+        $this->level = $parent->level + 1;
+      } catch (Exception $e) {
+        $this->unlock();
+        throw $e;
+      }
       $this->unlock();
-      throw $e;
     }
 
-    $this->unlock();
-    return $this;
+    return parent::save();
   }
 
   /**
diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php
index 33b36ff1..e929f30d 100644
--- a/modules/gallery/models/item.php
+++ b/modules/gallery/models/item.php
@@ -25,7 +25,8 @@ class Item_Model extends ORM_MPTT {
     "name"        => array("rules" => array("length[0,255]", "required")),
     "title"       => array("rules" => array("length[0,255]", "required")),
     "slug"        => array("rules" => array("length[0,255]", "required")),
-    "description" => array("rules" => array("length[0,65535]"))
+    "description" => array("rules" => array("length[0,65535]")),
+    "parent_id"   => array("rules" => array("Item_Model::valid_parent"))
   );
 
   /**
@@ -355,7 +356,10 @@ class Item_Model extends ORM_MPTT {
   }
 
   /**
+   * Handle any business logic necessary to create an item.
    * @see ORM::save()
+   *
+   * @return ORM Item_Model
    */
   public function save() {
     $significant_changes = $this->changed;
@@ -366,12 +370,55 @@ class Item_Model extends ORM_MPTT {
     if (!empty($this->changed) && $significant_changes) {
       $this->updated = time();
       if (!$this->loaded()) {
+        // Create a new item.  Use whatever fields are set, and specify defaults for the rest.
         $this->created = $this->updated;
         $this->weight = item::get_max_weight();
         $this->rand_key = ((float)mt_rand()) / (float)mt_getrandmax();
-      } else {
-        $send_event = 1;
+        $this->thumb_dirty = 1;
+        $this->resize_dirty = 1;
+        if (empty($this->sort_column)) {
+          $this->sort_column = "created";
+        }
+        if (empty($this->sort_order)) {
+          $this->sort_order = "ASC";
+        }
+        if (empty($this->owner_id)) {
+          $this->owner_id = identity::active_user()->id;
+        }
+        if (empty($this->slug)) {
+          $tmp = pathinfo($this->name, PATHINFO_FILENAME);
+          $tmp = preg_replace("/[^A-Za-z0-9-_]+/", "-", $tmp);
+          $this->slug = trim($tmp, "-");
+        }
+
+        // Randomize the name or slug if there's a conflict
+        // @todo Improve this.  Random numbers are not user friendly
+        $base_name = $this->name;
+        $base_slug = $this->slug;
+        while (ORM::factory("item")
+               ->where("parent_id", "=", $this->parent_id)
+               ->and_open()
+               ->where("name", "=", $this->name)
+               ->or_where("slug", "=", $this->slug)
+               ->close()
+               ->find()->id) {
+          $rand = rand();
+          $this->name = "$base_name-$rand";
+          $this->slug = "$base_slug-$rand";
+        }
+
+        parent::save();
 
+        // Call this after we finish saving so that the paths are correct.
+        if ($this->is_album()) {
+          mkdir($this->file_path());
+          mkdir(dirname($this->thumb_path()));
+          mkdir(dirname($this->resize_path()));
+        }
+
+        module::event("item_created", $this);
+      } else {
+        // Update an existing item
         if ($this->original()->name != $this->name) {
           $this->rename($this->name);
           $this->relative_path_cache = null;
@@ -394,14 +441,11 @@ class Item_Model extends ORM_MPTT {
             ->where("right_ptr", "<", $this->right_ptr)
             ->execute();
         }
+        parent::save();
+        module::event("item_updated", $this->original(), $this);
       }
     }
 
-    parent::save();
-
-    if (isset($send_event)) {
-      module::event("item_updated", $this->original(), $this);
-    }
     return $this;
   }
 
@@ -663,7 +707,7 @@ class Item_Model extends ORM_MPTT {
   public function valid_slug(Validation $v, $value) {
     if (preg_match("/[^A-Za-z0-9-_]/", $value)) {
       $v->add_error("slug", "not_url_safe");
-    } else if ($row = db::build()
+    } else if (db::build()
         ->from("items")
         ->where("parent_id", "=", $this->parent_id)
         ->where("id", "<>", $this->id)
@@ -682,7 +726,7 @@ class Item_Model extends ORM_MPTT {
       $v->add_error("name", "no_slashes");
     } else if (rtrim($value, ".") !== $value) {
       $v->add_error("name", "no_trailing_period");
-    } else if ($row = db::build()
+    } else if (db::build()
                ->from("items")
                ->where("parent_id", "=", $this->parent_id)
                ->where("id", "<>", $this->id)
@@ -691,4 +735,15 @@ class Item_Model extends ORM_MPTT {
       $v->add_error("name", "conflict");
     }
   }
+
+  /**
+   * Make sure that the parent id refers to an album.
+   */
+  static function valid_parent($value) {
+    return db::build()
+      ->from("items")
+      ->where("id", "=", $value)
+      ->where("type", "=", "album")
+      ->count_records() == 1;
+  }
 }
-- 
cgit v1.2.3


From 512910962d62a2011d7770da1b6e68bd6bbad983 Mon Sep 17 00:00:00 2001
From: Bharat Mediratta <bharat@menalto.com>
Date: Tue, 19 Jan 2010 19:24:46 -0800
Subject: Change "dirname" to "name" in the edit album form.  I'd rather have
 consistency between field names than deal with underlying issues with Forge
 bitching about the "name" property.

---
 modules/gallery/controllers/albums.php           | 5 +----
 modules/gallery/helpers/album.php                | 4 ++--
 modules/gallery/tests/Albums_Controller_Test.php | 6 +++---
 3 files changed, 6 insertions(+), 9 deletions(-)

(limited to 'modules/gallery/controllers/albums.php')

diff --git a/modules/gallery/controllers/albums.php b/modules/gallery/controllers/albums.php
index 7658a913..a378f3ee 100644
--- a/modules/gallery/controllers/albums.php
+++ b/modules/gallery/controllers/albums.php
@@ -141,15 +141,12 @@ class Albums_Controller extends Items_Controller {
       $album->description = $form->edit_item->description->value;
       $album->sort_column = $form->edit_item->sort_order->column->value;
       $album->sort_order = $form->edit_item->sort_order->direction->value;
-      $album->name = $form->edit_item->dirname->value;
+      $album->name = $form->edit_item->inputs["name"]->value;
       $album->slug = $form->edit_item->slug->value;
       $album->validate();
     } catch (ORM_Validation_Exception $e) {
       // Translate ORM validation errors into form error messages
       foreach ($e->validation->errors() as $key => $error) {
-        if ($key == "name") {
-          $key = "dirname";
-        }
         $form->edit_item->inputs[$key]->add_error($error, 1);
       }
       $valid = false;
diff --git a/modules/gallery/helpers/album.php b/modules/gallery/helpers/album.php
index e99770e9..55282252 100644
--- a/modules/gallery/helpers/album.php
+++ b/modules/gallery/helpers/album.php
@@ -52,7 +52,7 @@ class album_Core {
     $group->input("title")->label(t("Title"))->value($parent->title);
     $group->textarea("description")->label(t("Description"))->value($parent->description);
     if ($parent->id != 1) {
-      $group->input("dirname")->label(t("Directory Name"))->value($parent->name)
+      $group->input("name")->label(t("Directory Name"))->value($parent->name)
         ->rules("required")
         ->error_messages(
           "conflict", t("There is already a movie, photo or album with this name"))
@@ -65,7 +65,7 @@ class album_Core {
           "not_url_safe",
           t("The internet address should contain only letters, numbers, hyphens and underscores"));
     } else {
-      $group->hidden("dirname")->value($parent->name);
+      $group->hidden("name")->value($parent->name);
       $group->hidden("slug")->value($parent->slug);
     }
 
diff --git a/modules/gallery/tests/Albums_Controller_Test.php b/modules/gallery/tests/Albums_Controller_Test.php
index 8ba2c7bc..26dc2571 100644
--- a/modules/gallery/tests/Albums_Controller_Test.php
+++ b/modules/gallery/tests/Albums_Controller_Test.php
@@ -31,9 +31,9 @@ class Albums_Controller_Test extends Unit_Test_Case {
     $album = test::random_album();
 
     // Randomize to avoid conflicts.
-    $new_dirname = "new_name_" . rand();
+    $new_name = "new_name_" . rand();
 
-    $_POST["dirname"] = $new_dirname;
+    $_POST["name"] = $new_name;
     $_POST["title"] = "new title";
     $_POST["description"] = "new description";
     $_POST["column"] = "weight";
@@ -49,7 +49,7 @@ class Albums_Controller_Test extends Unit_Test_Case {
     ob_end_clean();
 
     $this->assert_equal(json_encode(array("result" => "success")), $results);
-    $this->assert_equal($new_dirname, $album->name);
+    $this->assert_equal($new_name, $album->name);
     $this->assert_equal("new title", $album->title);
     $this->assert_equal("new description", $album->description);
   }
-- 
cgit v1.2.3