BUG Fix form saving

code/extensions/ContentReviewCMSExtension.php

@@ -6,12 +6,13 @@
  */
 class ContentReviewCMSExtension extends LeftAndMainExtension
 {
+
     /**
      * @var array
      */
     private static $allowed_actions = array(
         "reviewed",
-        "save_review",
+        "AddReviewForm",
     );
 
     /**
@@ -25,40 +26,42 @@ class ContentReviewCMSExtension extends LeftAndMainExtension
      */
     public function reviewed($data, Form $form)
     {
-        if (!isset($data["ID"])) {
+        $record = $this->findRecord($data);
-            throw new SS_HTTPResponse_Exception("No record ID", 404);
-        }
-
-        $id = (int) $data["ID"];
-        $record = SiteTree::get()->byID($id);
-
-        if (!$record || !$record->ID) {
-            throw new SS_HTTPResponse_Exception("Bad record ID #{$id}", 404);
-        }
-
         if (!$record->canEdit()) {
             return Security::permissionFailure($this->owner);
         }
 
+        // Populate and respond
+        $form = $this->AddReviewForm();
+        $form->loadDataFrom($record);
+        return $form->forTemplate();
+    }
+
+    /**
+     * Form handler for this page
+     *
+     * @return CMSForm
+     */
+    public function AddReviewForm()
+    {
         $reviewNotes = TextareaField::create("ReviewNotes", _t("ContentReview.REVIEWNOTES", "Review notes"));
         $reviewNotes->setDescription(_t("ContentReview.REVIEWNOTESDESCRIPTION ", "Add comments for the content of this page."));
         $fields = new FieldList();
-        $fields->push(HiddenField::create("ID", "ID", $id));
+        $fields->push(HiddenField::create("ID"));
         $fields->push($reviewNotes);
 
         $actions = new FieldList(
             FormAction::create("save_review", _t("ContentReview.SAVE", "Save"))
         );
 
-        $form = CMSForm::create($this->owner, "EditForm", $fields, $actions)->setHTMLID("Form_EditForm");
+        $form = CMSForm::create($this->owner, "AddReviewForm", $fields, $actions)->setHTMLID("Form_EditForm");
         $form->setResponseNegotiator($this->owner->getResponseNegotiator());
-        $form->loadDataFrom($record);
         $form->disableDefaultAction();
 
         // TODO Can't merge $FormAttributes in template at the moment
         $form->setTemplate($this->owner->getTemplatesWithSuffix("LeftAndMain_EditForm"));
 
-        return $form->forTemplate();
+        return $form;
     }
 
     /**
@@ -73,19 +76,9 @@ class ContentReviewCMSExtension extends LeftAndMainExtension
      */
     public function save_review($data, Form $form)
     {
-        if (!isset($data["ID"])) {
+        $page = $this->findRecord($data);
-            throw new SS_HTTPResponse_Exception("No record ID", 404);
+        if (!$page->canEdit()) {
-        }
+            return Security::permissionFailure($this->owner);
-
-        $id = (int) $data["ID"];
-        $page = SiteTree::get()->byID($id);
-
-        if ($page && !$page->canEdit()) {
-            return Security::permissionFailure();
-        }
-
-        if (!$page || !$page->ID) {
-            throw new SS_HTTPResponse_Exception("Bad record ID #{$id}", 404);
         }
 
         $page->addReviewNote(Member::currentUser(), $data["ReviewNotes"]);
@@ -93,4 +86,30 @@ class ContentReviewCMSExtension extends LeftAndMainExtension
 
         return $this->owner->redirect($this->owner->Link("show/" . $page->ID));
     }
+
+    /**
+     * Find the page this form is updating
+     *
+     * @param array $data Form data
+     * @return SiteTree Record
+     * @throws SS_HTTPResponse_Exception
+     */
+    protected function findRecord($data)
+    {
+        if (empty($data["ID"])) {
+            throw new SS_HTTPResponse_Exception("No record ID", 404);
+        }
+
+        $page = null;
+        $id = $data["ID"];
+        if (is_numeric($id)) {
+            $page = SiteTree::get()->byID($id);
+        }
+
+        if (!$page || !$page->ID) {
+            throw new SS_HTTPResponse_Exception("Bad record ID #{$id}", 404);
+        }
+
+        return $page;
+    }
 }

tests/ContentReviewBaseTest.php

@@ -6,6 +6,7 @@
  */
 abstract class ContentReviewBaseTest extends FunctionalTest
 {
+
     /**
      * @var bool
      */
@@ -38,5 +39,7 @@ abstract class ContentReviewBaseTest extends FunctionalTest
                 SiteTree::add_extension("Translatable");
             }
         }
+
+        parent::tearDown();
     }
 }

tests/ContentReviewCMSPageEditControllerTest.php

@@ -88,7 +88,7 @@ class ContentReviewCMSPageEditControllerTest extends ContentReviewBaseTest
             "ReviewNotes"        => "This is the best page ever",
         );
 
-        $response = $this->post("admin/pages/edit/EditForm", $data);
+        $response = $this->post("admin/pages/edit/AddReviewForm", $data);
 
         $this->assertEquals("OK", $response->getStatusDescription());
         $this->assertEquals(200, $response->getStatusCode());