fix: tidy up tests

composer.json

@@ -61,7 +61,7 @@
   "support": {
     "issues": "https://github.com/silvershop/silvershop-core/issues",
     "source": "https://github.com/silvershop/silvershop-core",
-    "docs": "https://github.com/silvershop/silvershop-core/tree/master/docs/en"
+    "docs": "https://github.com/silvershop/silvershop-core/tree/main/docs/en"
   },
   "scripts": {
     "lint": "phpcs -s src/ tests/",

src/Cart/ShoppingCart.php

@@ -147,10 +147,12 @@ public function add(Buyable $buyable, $quantity = 1, $filter = [])
         }
 
         $item = $this->findOrMakeItem($buyable, $quantity, $filter);
+
         if (!$item) {
             return false;
         }
-        if (!$item->_brandnew) {
+
+        if (!$item->brandNew) {
             $item->Quantity += $quantity;
         } else {
             $item->Quantity = $quantity;
@@ -353,8 +355,7 @@ private function findOrMakeItem(Buyable $buyable, $quantity = 1, $filter = [])
             $item->write();
 
             $order->Items()->add($item);
-
-            $item->_brandnew = true; // flag as being new
+            $item->brandNew = true;
         }
 
         return $item;

src/Cart/ShoppingCartController.php

@@ -128,8 +128,9 @@ public static function direct($status = true)
         if (Director::is_ajax()) {
             return (string)$status;
         }
-        if (self::config()->direct_to_cart_page && ($cartlink = CartPage::find_link())) {
-            return Controller::curr()->redirect($cartlink);
+
+        if (self::config()->direct_to_cart_page && ($cart = CartPage::find_link())) {
+            return Controller::curr()->redirect($cart);
         } else {
             return Controller::curr()->redirectBack();
         }
@@ -200,15 +201,26 @@ public function add($request)
 
         if ($product = $this->buyableFromRequest()) {
             $quantity = (int)$request->getVar('quantity');
+
             if (!$quantity) {
                 $quantity = 1;
             }
+
             $result = $this->cart->add($product, $quantity, $request->getVars());
+
+            if ($result) {
+                $response = $this->cart->getMessage();
+            } else {
+                $response = $this->httpError(400, $this->cart->getMessage());
+            }
+        } else {
+            $response = $this->httpError(404);
         }
 
         $this->updateLocale($request);
         $this->extend('updateAddResponse', $request, $response, $product, $quantity, $result);
-        return $response ? $response : self::direct();
+
+        return $response ? $response : self::direct($result);
     }
 
     /**

src/Checkout/Component/Membership.php

@@ -121,9 +121,11 @@ public function validateData(Order $order, array $data)
                     $idfield
                 );
             }
-            $passwordresult = $this->passwordvalidator->validate($data['Password'], $member);
-            if (!$passwordresult->isValid()) {
-                foreach ($passwordresult->getMessages() as $message) {
+
+            $passwordResult = $this->passwordValidator->validate($data['Password'], $member);
+
+            if (!$passwordResult->isValid()) {
+                foreach ($passwordResult->getMessages() as $message) {
                     $result->addError($message['message'], "Password");
                 }
             }

src/Model/OrderItem.php

@@ -18,6 +18,8 @@
  */
 class OrderItem extends OrderAttribute
 {
+    public bool $brandNew = false;
+
     private static $db = [
         'Quantity' => 'Int',
         'UnitPrice' => 'Currency',
@@ -229,7 +231,7 @@ public function removeLink()
     /**
      * @return string
      */
-    public function removeallLink()
+    public function removeAllLink()
     {
         $buyable = $this->Buyable();
         return $buyable ? ShoppingCartController::remove_all_item_link($buyable, $this->uniquedata()) : '';
@@ -238,7 +240,7 @@ public function removeallLink()
     /**
      * @return string
      */
-    public function setquantityLink()
+    public function setQuantityLink()
     {
         $buyable = $this->Buyable();
         return $buyable ? ShoppingCartController::set_quantity_item_link($buyable, $this->uniquedata()) : '';

src/Page/Product.php

@@ -13,7 +13,7 @@
 use SilverShop\Model\Variation\Variation;
 use SilverStripe\AssetAdmin\Forms\UploadField;
 use SilverStripe\Assets\Image;
-use SilverStripe\Control\Director;
+use SilverStripe\Core\Injector\Injector;
 use SilverStripe\Forms\CheckboxField;
 use SilverStripe\Forms\DropdownField;
 use SilverStripe\Forms\FieldList;
@@ -322,25 +322,29 @@ public function getCategories()
     public function canPurchase($member = null, $quantity = 1)
     {
         $global = self::config()->global_allow_purchase;
+
         if (!$global || !$this->AllowPurchase) {
             return false;
         }
-        $allowpurchase = false;
+
+        $allowPurchase = false;
         $extension = self::has_extension(ProductVariationsExtension::class);
+
         if ($extension && Variation::get()->filter('ProductID', $this->ID)->first()) {
             foreach ($this->Variations() as $variation) {
                 if ($variation->canPurchase($member, $quantity)) {
-                    $allowpurchase = true;
+                    $allowPurchase = true;
                     break;
                 }
             }
         } else {
-            $allowpurchase = ($this->sellingPrice() > 0 || self::config()->allow_zero_price);
+            $allowPurchase = ($this->sellingPrice() > 0 || self::config()->allow_zero_price);
         }
 
         // Standard mechanism for accepting permission changes from decorators
         $permissions = $this->extend('canPurchase', $member, $quantity);
-        $permissions[] = $allowpurchase;
+        $permissions[] = $allowPurchase;
+
         return min($permissions);
     }
 
@@ -387,14 +391,20 @@ public function Item()
      */
     public function createItem($quantity = 1, $filter = null)
     {
-        $orderitem = self::config()->order_item;
-        $item = new $orderitem();
+        $orderItem = self::config()->order_item;
+
+        if (!$orderItem) {
+            $orderItem = OrderItem::class;
+        }
+
+        $item = Injector::inst()->create($orderItem);
         $item->ProductID = $this->ID;
+
         if ($filter) {
-            //TODO: make this a bit safer, perhaps intersect with allowed fields
             $item->update($filter);
         }
         $item->Quantity = $quantity;
+
         return $item;
     }
 
@@ -405,9 +415,9 @@ public function createItem($quantity = 1, $filter = null)
     public function sellingPrice()
     {
         $price = $this->BasePrice;
-        //TODO: this is not ideal, because prices manipulations will not happen in a known order
+
         $this->extend('updateSellingPrice', $price);
-        //prevent negative values
+
         $price = $price < 0 ? 0 : $price;
 
         // NOTE: Ideally, this would be dependent on the locale but as of
@@ -489,7 +499,7 @@ public function removeLink()
      *
      * @return string|false link
      */
-    public function removeallLink()
+    public function removeAllLink()
     {
         return ShoppingCartController::remove_all_item_link($this);
     }

templates/SilverShop/Cart/Cart.ss

@@ -53,7 +53,7 @@
                             <% if $RemoveField %>
                                 $RemoveField
                             <% else %>
-                                <a href="$removeallLink" title="<%t SilverShop\Cart\ShoppingCart.RemoveAllTitle "Remove all of &quot;{Title}&quot; from your cart" Title=$TableTitle %>">
+                                <a href="$removeAllLink" title="<%t SilverShop\Cart\ShoppingCart.RemoveAllTitle "Remove all of &quot;{Title}&quot; from your cart" Title=$TableTitle %>">
                                     <img src="$resourceURL('silvershop/core:client/dist/images/remove.gif')" alt="x"/>
                                 </a>
                             <% end_if %>

templates/SilverShop/Cart/SideCart.ss

@@ -30,7 +30,7 @@
                     </p>
                     <p class="quantityprice"><span class="quantity">$Quantity</span> <span class="times">x</span> <span class="unitprice">$UnitPrice.Nice</span></p>
                     <% if $SubTitle %><p class="subtitle">$SubTitle</p><% end_if %>
-                    <a class="remove" href="$removeallLink" title="<%t SilverShop\Cart\ShoppingCart.RemoveTitle "Remove &quot;{Title}&quot; from your cart." Title=$TableTitle %>">x</a>
+                    <a class="remove" href="$removeAllLink" title="<%t SilverShop\Cart\ShoppingCart.RemoveTitle "Remove &quot;{Title}&quot; from your cart." Title=$TableTitle %>">x</a>
                 </div>
             <% end_loop %>
         <% end_with %>

tests/php/Cart/ShoppingCartControllerTest.php

@@ -77,12 +77,13 @@ public function setUp(): void
 
         $this->mp3player = $this->objFromFixture(Product::class, 'mp3player');
         $this->socks = $this->objFromFixture(Product::class, 'socks');
-        //products that can't be purchased
+
+        // products that can't be purchased
         $this->noPurchaseProduct = $this->objFromFixture(Product::class, 'beachball');
         $this->draftProduct = $this->objFromFixture(Product::class, 'tshirt');
         $this->noPriceProduct = $this->objFromFixture(Product::class, 'hdtv');
 
-        //publish some products
+        // publish some products
         $this->mp3player->publishSingle();
         $this->socks->publishSingle();
         $this->noPurchaseProduct->publishSingle();
@@ -94,37 +95,52 @@ public function setUp(): void
     public function testAddToCart()
     {
         // add 2 of the same items via url
-        $this->get(ShoppingCartController::add_item_link($this->mp3player)); //add item via url
-        $this->get(ShoppingCartController::add_item_link($this->mp3player)); //add another
-        $this->get(ShoppingCartController::add_item_link($this->socks)); //add a different product
-        $this->get(ShoppingCartController::add_item_link($this->noPurchaseProduct));  //add a product that you can't add
-        $this->get(ShoppingCartController::add_item_link($this->draftProduct));  //add a product that is draft
-        $this->get(ShoppingCartController::add_item_link($this->noPriceProduct));  //add a product that has no price
+        $url = ShoppingCartController::add_item_link($this->mp3player);
+        $addMp3 = $this->get($url);
+
+        $this->assertEquals(200, $addMp3->getStatusCode(), "Adding the mp3 player should work");
+
+        $secondMp3 = $this->get(ShoppingCartController::add_item_link($this->mp3player));
+        $this->assertEquals(200, $secondMp3->getStatusCode(), "Adding a second mp3 player should work");
+
+        $socks = $this->get(ShoppingCartController::add_item_link($this->socks));
+        $this->assertEquals(200, $socks->getStatusCode(), "Adding socks should work");
+
+        // add a product that you can't add
+        $noPurchaseProduct = $this->get(ShoppingCartController::add_item_link($this->noPurchaseProduct));
+        $this->assertEquals(400, $noPurchaseProduct->getStatusCode(), "Cannot purchase a product if disabled");
+
+        $draftProduct = $this->get(ShoppingCartController::add_item_link($this->draftProduct));
+        $this->assertEquals(404, $draftProduct->getStatusCode(), "Cannot purchase a product that is draft");
+
+        $noPriceProduct = $this->get(ShoppingCartController::add_item_link($this->noPriceProduct));
+        $this->assertEquals(400, $noPriceProduct->getStatusCode(), "Cannot purchase a product");
 
         // See what's in the cart
         $items = ShoppingCart::curr()->Items();
         $this->assertNotNull($items);
 
         $this->assertEquals($items->Count(), 2, 'There are 2 items in the cart');
-        //join needed to provide ProductID
-        $mp3playeritem = $items
+
+        // join needed to provide ProductID
+        $mp3playerItem = $items
             ->innerJoin("SilverShop_Product_OrderItem", "\"SilverShop_OrderItem\".\"ID\" = \"SilverShop_Product_OrderItem\".\"ID\"")
             ->find('ProductID', $this->mp3player->ID);
 
-        $this->assertNotNull($mp3playeritem, "Mp3 player is in cart");
+        $this->assertNotNull($mp3playerItem, "Mp3 player is in cart");
 
         // We have the product that we asserted in our fixture file, with a quantity of 2 in the cart
         $this->assertEquals(
-            $mp3playeritem->ProductID,
+            $mp3playerItem->ProductID,
             $this->mp3player->ID,
             'We have the correct Product ID in the cart.'
         );
-        $this->assertEquals($mp3playeritem->Quantity, 2, 'We have 2 of this product in the cart.');
+        $this->assertEquals($mp3playerItem->Quantity, 2, 'We have 2 of this product in the cart.');
 
-        // set item quantiy
         $this->get(
             ShoppingCartController::set_quantity_item_link($this->mp3player, ['quantity' => 5])
-        ); //add item via url
+        );
+
         $items = ShoppingCart::curr()->Items();
         $mp3playeritem =
             $items->innerJoin("SilverShop_Product_OrderItem", "\"SilverShop_OrderItem\".\"ID\" = \"SilverShop_Product_OrderItem\".\"ID\"")->find(
@@ -166,6 +182,7 @@ public function testRemoveFromCart()
         // add items via url
         $this->get(ShoppingCartController::set_quantity_item_link($this->mp3player, ['quantity' => 5]));
         $this->assertTrue($this->cart->get($this->mp3player) !== false, "mp3player item now exists in cart");
+
         $this->get(ShoppingCartController::add_item_link($this->socks));
         $this->assertTrue($this->cart->get($this->socks) !== false, "socks item now exists in cart");
 
@@ -187,12 +204,13 @@ public function testRemoveFromCart()
             ShoppingCart::curr(),
             null,
             'Cart is clear'
-        ); //items is a databoject set, and will therefore be null when cart is empty.
+        );
     }
 
     public function testSecurityToken()
     {
         $enabled = SecurityToken::is_enabled();
+
         // enable security tokens
         SecurityToken::enable();
 
@@ -201,29 +219,26 @@ public function testSecurityToken()
         $link = ShoppingCartController::add_item_link($this->mp3player);
         $this->assertMatchesRegularExpression('{^shoppingcart/add/SilverShop-Page-Product/' . $productId . '\?SecurityID=[a-f0-9]+$}', $link);
 
-        // should redirect back to the shop
         $response = $this->get($link);
-        $this->assertEquals($response->getStatusCode(), 302);
+        $this->assertEquals($response->getStatusCode(), 200);
 
         // disable security token for cart-links
         Config::modify()->set(ShoppingCartController::class, 'disable_security_token', true);
 
         $link = ShoppingCartController::add_item_link($this->mp3player);
         $this->assertEquals('shoppingcart/add/SilverShop-Page-Product/' . $productId, $link);
 
-        // should redirect back to the shop
         $response = $this->get($link);
-        $this->assertEquals($response->getStatusCode(), 302);
+        $this->assertEquals($response->getStatusCode(), 200);
 
         SecurityToken::disable();
 
         Config::modify()->set(ShoppingCartController::class, 'disable_security_token', false);
         $link = ShoppingCartController::add_item_link($this->mp3player);
         $this->assertEquals('shoppingcart/add/SilverShop-Page-Product/' . $productId, $link);
 
-        // should redirect back to the shop
         $response = $this->get($link);
-        $this->assertEquals($response->getStatusCode(), 302);
+        $this->assertEquals($response->getStatusCode(), 200);
 
         SecurityToken::enable();
         // should now return a 400 status
@@ -243,14 +258,15 @@ public function testVariations()
          */
         $ballRoot = $this->objFromFixture(Product::class, 'ball');
         $ballRoot->publishSingle();
+
         /**
          * @var Product $ball1
          */
-        $ball1 = $this->objFromFixture(Variation::class, 'redlarge');
+        $ball1 = $this->objFromFixture(Variation::class, 'redLarge');
         /**
          * @var Product $ball2
          */
-        $ball2 = $this->objFromFixture(Variation::class, 'redsmall');
+        $ball2 = $this->objFromFixture(Variation::class, 'redSmall');
 
         $this->logInWithPermission('ADMIN');
         $ball1->publishSingle();