diff --git a/app/Database/Migrations/20260818000000_AddStockLocationDefaultAndSortOrder.php b/app/Database/Migrations/20260818000000_AddStockLocationDefaultAndSortOrder.php index 52e298d6c..cee02b77b 100644 --- a/app/Database/Migrations/20260818000000_AddStockLocationDefaultAndSortOrder.php +++ b/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"); diff --git a/app/Models/Stock_location.php b/app/Models/Stock_location.php index 348384f7b..6b23ab7d8 100644 --- a/app/Models/Stock_location.php +++ b/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]); diff --git a/tests/Models/Stock_locationTest.php b/tests/Models/Stock_locationTest.php index ab39cdee3..0f286cdb9 100644 --- a/tests/Models/Stock_locationTest.php +++ b/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); } }