Fix multiple bugs + handle item delete + add tests

Fixes #7

Author: pionl

README.md

@@ -23,7 +23,7 @@ and updates the other entries based on the models position value.
 
 ## Installation
 
-> Tested in Laravel 5.1 - 5.7, should work in all 5.*/6.* releases
+> Tested in Laravel 5.4 - 8.
 
 **Install via composer**
 

composer.json

@@ -2,10 +2,11 @@
   "name": "pion/laravel-eloquent-position",
   "description": "Position logic for Eloquent models with minimum setup",
   "require": {
-    "laravel/framework": ">=5.1"
+    "laravel/framework": ">=5.5"
   },
   "require-dev": {
-    "squizlabs/php_codesniffer": "2.*"
+    "squizlabs/php_codesniffer": "2.*",
+    "orchestra/testbench": "~3.6.7 || ~3.7.8 || ~3.8.6 || ^4.8 || ^5.2 || ^6.0"
   },
   "scripts": {
     "lint": "./vendor/bin/phpcs --standard=PSR2 src/",
@@ -16,6 +17,11 @@
       "Pion\\Support\\Eloquent\\Position\\": "src/"
     }
   },
+  "autoload-dev": {
+    "psr-4": {
+      "Tests\\": "tests/"
+    }
+  },
   "license": "MIT",
   "authors": [
     {

phpunit.xml

@@ -0,0 +1,29 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+         xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/9.3/phpunit.xsd"
+         backupGlobals="false"
+         backupStaticAttributes="false"
+         bootstrap="vendor/autoload.php"
+         colors="true"
+         convertErrorsToExceptions="true"
+         convertNoticesToExceptions="true"
+         convertWarningsToExceptions="true"
+         processIsolation="false"
+         stopOnFailure="false"
+         verbose="true">
+    <coverage includeUncoveredFiles="false">
+        <include>
+            <directory suffix=".php">src/</directory>
+        </include>
+    </coverage>
+    <testsuites>
+        <testsuite name="Test Suite">
+            <directory suffix="Test.php">./tests/</directory>
+        </testsuite>
+    </testsuites>
+    <php>
+        <server name="DB_CONNECTION" value="testing" />
+        <server name="APP_ENV" value="testing"/>
+        <server name="APP_KEY" value="AckfSECXIvnK5r28GVIWUAxmbBSjTsmF"/>
+    </php>
+</phpunit>

src/Commands/RecalculatePositionCommand.php

@@ -62,24 +62,25 @@ public function handle()
         $groups = $model->getPositionGroup();
 
         // Run sorted query for every entry
-        $modelClass::sorted()->chunk(200, function (Collection $collection) use ($groups, &$positionsByGroup) {
-            /** @var PositionTrait|Model $model */
-            foreach ($collection as $model) {
-                // Builds the group key to get position
-                $groupKey = $this->buildGroupKeyForPosition($model, $groups);
-
-                // Set the new position
-                $model->setPosition($this->getPositionForGroup($groupKey, $positionsByGroup))
-                    ->save();
-            }
-        });
+        $modelClass::sorted()
+            ->chunk(200, function (Collection $collection) use ($groups, &$positionsByGroup) {
+                /** @var PositionTrait|Model $model */
+                foreach ($collection as $model) {
+                    // Builds the group key to get position
+                    $groupKey = $this->buildGroupKeyForPosition($model, $groups);
+
+                    // Set the new position
+                    $model->setPosition($this->getPositionForGroup($groupKey, $positionsByGroup))
+                        ->save();
+                }
+            });
 
         // Render the table layout about the positions
         $this->table([
             'Group', 'Last position'
-        ], collect($positionsByGroup)->map(function ($value, $key) {
+        ], array_map(function ($value, $key) {
             return [$key, $value];
-        }));
+        }, $positionsByGroup));
 
         return true;
     }
@@ -107,7 +108,7 @@ protected function getPositionForGroup($groupKey, &$positionsByGroup)
     /**
      * Builds the group key from the group columns and the values form the model
      *
-     * @param Model|PositionTrait $model  The eloquent model
+     * @param Model|PositionTrait $model The eloquent model
      * @param array               $groups
      *
      * @return string

src/PositionObserver.php

@@ -42,7 +42,7 @@ public function __construct(Dispatcher $events)
      */
     public function saving($model)
     {
-        if (!$model->isPositionUpdateDisabled()) {
+        if ($model->isPositionUpdateDisabled() === false) {
             // Get the position for current and old value
             $position = $model->getPosition();
 
@@ -61,7 +61,23 @@ public function saving($model)
     }
 
     /**
-     * Forces the new position, will be overriden if it's out of maximum bounds.
+     * Updates the position before saving
+     *
+     * @param Model|PositionTrait $model
+     */
+    public function deleted($model)
+    {
+        if ($model->isPositionUpdateDisabled() === false) {
+            // Get the old position
+            $oldPosition = $model->getOriginal($model->getPositionColumn());
+
+            // Append deleted model as last to re-index other models
+            $this->appendLast($model, $oldPosition);
+        }
+    }
+
+    /**
+     * Forces the new position, will be overridden if it's out of maximum bounds.
      *
      * @param Model|PositionTrait $model
      * @param int                 $position

src/Query/LastPositionQuery.php

@@ -1,4 +1,5 @@
 <?php
+
 namespace Pion\Support\Eloquent\Position\Query;
 
 use Pion\Support\Eloquent\Position\Traits\PositionTrait;
@@ -19,7 +20,7 @@ class LastPositionQuery extends AbstractPositionQuery
      * Creates the base query and builds the query
      *
      * @param Model|PositionTrait $model
-     * @param int                 $oldPosition
+     * @param int|null            $oldPosition
      */
     public function __construct($model, $oldPosition)
     {
@@ -39,8 +40,10 @@ public function runQuery($query)
         // Get the last position and move the position
         $lastPosition = $query->max($this->model()->getPositionColumn()) ?: 0;
 
-        // Check if the last position is not same as original position - the same object
-        if (empty($this->oldPosition) || $lastPosition != $this->oldPosition) {
+        if (empty($this->oldPosition) === false) {
+            (new MoveQuery($this->model, $lastPosition, $this->oldPosition))->run();
+        } else if ($this->oldPosition === null || $lastPosition != $this->oldPosition) {
+            // Check if the last position is not same as original position - the same object
             $this->model()->setPosition($lastPosition + 1);
         }
 

src/Query/MoveQuery.php

@@ -1,4 +1,5 @@
 <?php
+
 namespace Pion\Support\Eloquent\Position\Query;
 
 use Pion\Support\Eloquent\Position\Traits\PositionTrait;
@@ -23,22 +24,20 @@ class MoveQuery extends AbstractPositionQuery
     protected $position;
 
     /**
-     * Comparision condition for old position value.
+     * Comparison condition for old position value.
      * In default is for decrement.
      * @var string
      */
-    protected $oldComparisionCondition = '>';
+    protected $oldComparisonCondition = '>';
     /**
-     * Comparision condition for new position value.
+     * Comparison condition for new position value.
      * In default is for decrement.
      * @var string
      */
-    protected $newComparisionCondition = '<=';
+    protected $newComparisonCondition = '<=';
 
 
     /**
-     * PositionQuery constructor.
-     *
      * @param Model|PositionTrait $model
      * @param int                 $position
      * @param int|null            $oldPosition
@@ -56,7 +55,7 @@ public function __construct($model, $position, $oldPosition)
         $this->positionColumn = $model->getPositionColumn();
 
         // Build the comparision condition
-        $this->buildComparisionCondition();
+        $this->buildComparisonCondition();
 
         // Prepare the query
         $this->query = $this->buildQuery();
@@ -76,6 +75,15 @@ public function runQuery($query)
         if ($this->increment) {
             return $query->increment($this->positionColumn);
         } else {
+            // Get the last position and move the position
+            $lastPosition = $query->max($this->positionColumn) ?: 0;
+
+            // If the set position is out of the bounds of current items, force new position
+            if ($this->position > $lastPosition) {
+                $this->position = $lastPosition;
+                $this->model()->setPosition($this->position);
+            }
+
             return $query->decrement($this->positionColumn);
         }
     }
@@ -90,18 +98,18 @@ protected function buildQuery()
         $query = parent::buildQuery();
 
         // Build the where condition for the position. This will ensure to update only required rows
-        return $query->where($this->positionColumn, $this->oldComparisionCondition, $this->oldPosition)
-                    ->where($this->positionColumn, $this->newComparisionCondition, $this->position);
+        return $query->where($this->positionColumn, $this->oldComparisonCondition, $this->oldPosition)
+            ->where($this->positionColumn, $this->newComparisonCondition, $this->position);
     }
 
     /**
-     * Builds the correct comparision condition
+     * Builds the correct comparison condition
      */
-    protected function buildComparisionCondition()
+    protected function buildComparisonCondition()
     {
         if ($this->increment) {
-            $this->oldComparisionCondition = '<';
-            $this->newComparisionCondition = '>=';
+            $this->oldComparisonCondition = '<';
+            $this->newComparisonCondition = '>=';
         }
     }
     //endregion

src/Query/PositionQuery.php

@@ -52,11 +52,11 @@ protected function buildQuery()
         $query = parent::buildQuery();
 
         // Get the last position and move the position
-        $lastPosition = $query->max($this->positionColumn);
+        $lastPosition = $query->max($this->positionColumn) ?: 0;
 
         // If the new position is last position, just update the position or if
         // new position is out of bounds
-        if ($this->position >= $lastPosition) {
+        if ($this->position > $lastPosition) {
             $this->model()->setPosition($lastPosition + 1);
         } else {
             $this->shouldRunQuery = true;

src/Traits/PositionTrait.php

@@ -70,7 +70,7 @@ public function getPositionGroup()
      */
     public function isPositionUpdateDisabled()
     {
-        return $this->positionOption('disablePositionUpdate');
+        return $this->positionOption('disablePositionUpdate', false);
     }
 
     //endregion

tests/GroupItemPositionTest.php

@@ -0,0 +1,36 @@
+<?php
+
+namespace Tests;
+
+use Illuminate\Database\Eloquent\Builder;
+use Tests\Models\AbstractPositionModel;
+use Tests\Models\GroupItem;
+
+class GroupItemPositionTest extends SingleItemPositionTest
+{
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        // Create items for different group - single item position test should contain only its own group
+        $this->createItem('DifferentGroup', null, 2);
+        $this->createItem('DifferentGroup', null, 2);
+        $this->createItem('DifferentGroup', null, 2);
+        $this->createItem('DifferentGroup', null, 2);
+    }
+
+
+    protected function query(): Builder
+    {
+        return GroupItem::query()->where('group', 1);
+    }
+
+    protected function createItem(string $name, string $position = null, int $group = 1): AbstractPositionModel
+    {
+        return GroupItem::create([
+            'name' => $name,
+            'position' => $position,
+            'group' => $group,
+        ]);
+    }
+}