fix(models): ensure dense sort_order range and clear values for deleted locations

- Simplified sort_order assignment logic to maintain dense 0..N-1 range for active locations.
- Updated deleted rows to set sort_order as NULL instead of pushing to a higher value.
- Adjusted tests and migrations to reflect the new behavior.

Signed-off-by: objecttothis <17935339+objecttothis@users.noreply.github.com>

app/Database/Migrations/20260818000000_AddStockLocationDefaultAndSortOrder.php

@@ -24,16 +24,29 @@ class AddStockLocationDefaultAndSortOrder extends Migration
             'sort_order' => [
                 'type'       => 'TINYINT',
                 'unsigned'   => true,
-                'null'       => false,
-                'default'    => 0,
+                'null'       => true,
+                'default'    => null,
                 'after'      => 'is_default',
             ],
         ]);
 
-        // Backfill sort_order for every row (deleted included, since the column is unique table-wide),
-        // ranking non-deleted rows alphabetically first so upgraded installs see no visible reorder.
-        $this->db->query('SET @rank := -1');
-        $this->db->query("UPDATE $table SET sort_order = (@rank := @rank + 1) ORDER BY deleted ASC, location_name ASC");
+        // Backfill sort_order for non-deleted rows only, ranking alphabetically so upgraded
+        // installs see no visible reorder. Deleted rows get NULL: they don't occupy a sort
+        // position, so the active range always stays a dense 0..N-1 regardless of how many
+        // locations have been deleted over the life of the install.
+        $this->db->query("
+            UPDATE $table AS destination
+            INNER JOIN (
+                SELECT
+                    location_id,
+                    ROW_NUMBER() OVER (ORDER BY location_name ASC, location_id ASC) - 1 AS new_sort_order
+                FROM $table
+                WHERE deleted = 0
+            ) AS ranked ON ranked.location_id = destination.location_id
+            SET destination.sort_order = ranked.new_sort_order
+        ");
+
+        $this->db->query("UPDATE $table SET sort_order = NULL WHERE deleted = 1");
 
         // Default to the oldest (lowest location_id) non-deleted location so behavior is unchanged until an admin picks one explicitly.
         $this->db->query("UPDATE $table SET is_default = 1 WHERE deleted = 0 ORDER BY location_id ASC LIMIT 1");

app/Models/Stock_location.php

@@ -176,9 +176,9 @@ class Stock_location extends Model
     /**
      * Saves sort order of locations.
      * A rotation can still collide against the unique sort_order index mid-batch, so
-     * values are shifted to a temp range first, then to final. Temp offset uses the
-     * table-wide MAX(sort_order), not count($orderedLocationIds), since sort_order can
-     * already exceed the row count (uncompacted inserts).
+     * values are shifted to a temp range first, then to final. The temp offset only
+     * needs to clear count($orderedLocationIds), since active sort_order values are
+     * always kept as a dense 0..N-1 range (deleted rows are NULL, not part of it).
      */
     public function saveSortOrder(array $orderedLocationIds): bool
     {
@@ -187,10 +187,7 @@ class Stock_location extends Model
         }
 
         $table = $this->db->prefixTable('stock_locations');
-
-        $builder = $this->db->table('stock_locations');
-        $maxSortOrder = $builder->selectMax('sort_order')->get()->getRow()->sort_order;
-        $tempOffset = $maxSortOrder + 1;
+        $tempOffset = count($orderedLocationIds);
 
         $this->db->transStart();
 
@@ -257,8 +254,7 @@ class Stock_location extends Model
             $this->db->transStart();
 
             $builder = $this->db->table('stock_locations');
-            $maxSortOrder = $builder->selectMax('sort_order')->get()->getRow()->sort_order;
-            $locationDataToSave['sort_order'] = $maxSortOrder + 1;
+            $locationDataToSave['sort_order'] = $builder->where('deleted', 0)->countAllResults();
 
             $builder = $this->db->table('stock_locations');
             $builder->insert($locationDataToSave);
@@ -348,14 +344,12 @@ class Stock_location extends Model
     {
         $this->db->transStart();
 
-        // Push the deleted row's sort_order past the current table-wide max so it can
-        // never collide with the small active range saveSortOrder() reuses (0..N-1 / N..2N-1).
-        $builder = $this->db->table('stock_locations');
-        $maxSortOrder = $builder->selectMax('sort_order')->get()->getRow()->sort_order;
-
+        // Deleted rows don't occupy a sort position, so sort_order is cleared rather than
+        // pushed to some new high value. This keeps the active range a dense 0..N-1 no
+        // matter how many locations have been deleted over the life of the install.
         $builder = $this->db->table('stock_locations');
         $builder->where('location_id', $locationId);
-        $builder->update(['deleted' => 1, 'sort_order' => $maxSortOrder + 1]);
+        $builder->update(['deleted' => 1, 'sort_order' => null]);
 
         $builder = $this->db->table('permissions');
         $builder->delete(['location_id' => $locationId]);

tests/Models/Stock_locationTest.php

@@ -22,6 +22,8 @@ class Stock_locationTest extends CIUnitTestCase
 
     public static function setUpBeforeClass(): void
     {
+        parent::setUpBeforeClass();
+
         $seeder = Database::seeder('tests');
         $seeder->call('TestDatabaseBootstrapSeeder');
     }
@@ -42,9 +44,7 @@ class Stock_locationTest extends CIUnitTestCase
 
     private function nextSortOrder(): int
     {
-        $max = $this->db->table('stock_locations')->selectMax('sort_order')->get()->getRow()->sort_order;
-
-        return $max === null ? 0 : ((int) $max) + 1;
+        return $this->db->table('stock_locations')->where('deleted', 0)->countAllResults();
     }
 
     private function insertLocation(string $name, bool $isDefault = false, bool $withPermissions = true): int
@@ -187,18 +187,21 @@ class Stock_locationTest extends CIUnitTestCase
         $this->assertLessThan($sortOrders[$idB], $sortOrders[$idC]);
         $this->assertLessThan($sortOrders[$idA], $sortOrders[$idB]);
 
-        $distinctCount = $this->db->table('stock_locations')->distinct()->select('sort_order')->get()->getNumRows();
-        $totalCount = $this->db->table('stock_locations')->countAllResults();
+        $distinctCount = $this->db->table('stock_locations')
+            ->distinct()
+            ->select('sort_order')
+            ->where('deleted', 0)
+            ->get()
+            ->getNumRows();
+        $totalCount = $this->db->table('stock_locations')->where('deleted', 0)->countAllResults();
         $this->assertSame($totalCount, $distinctCount, 'sort_order values must remain unique after rotation');
     }
 
-    public function testDeletePushesSortOrderPastTableMax(): void
+    public function testDeleteClearsSortOrder(): void
     {
         $idA = $this->insertLocation('Xi', false, false);
         $idB = $this->insertLocation('Omicron', false, false);
 
-        $maxBefore = (int) $this->db->table('stock_locations')->selectMax('sort_order')->get()->getRow()->sort_order;
-
         $result = $this->stockLocation->delete($idB);
 
         $this->assertTrue($result);
@@ -206,7 +209,7 @@ class Stock_locationTest extends CIUnitTestCase
         $deletedRow = $this->db->table('stock_locations')->where('location_id', $idB)->get()->getRow();
 
         $this->assertEquals(1, $deletedRow->deleted);
-        $this->assertGreaterThan($maxBefore, $deletedRow->sort_order);
+        $this->assertNull($deletedRow->sort_order);
     }
 
     public function testDeleteExcludesLocationFromGetAll(): void
@@ -224,7 +227,7 @@ class Stock_locationTest extends CIUnitTestCase
 
     public function testSaveValueNewInsertAssignsNextSortOrder(): void
     {
-        $maxBefore = (int) $this->db->table('stock_locations')->selectMax('sort_order')->get()->getRow()->sort_order;
+        $countBefore = $this->db->table('stock_locations')->where('deleted', 0)->countAllResults();
 
         $locationData = ['location_name' => 'Sigma New Location'];
 
@@ -236,6 +239,6 @@ class Stock_locationTest extends CIUnitTestCase
             ->get()
             ->getRow();
 
-        $this->assertSame($maxBefore + 1, (int) $savedRow->sort_order);
+        $this->assertSame($countBefore, (int) $savedRow->sort_order);
     }
 }