Fix reordering when tags are treated as categories

app/Http/Controllers/ItemController.php

@@ -98,9 +98,17 @@ public function dash(Request $request): View
      */
     public function setOrder(Request $request)
     {
-        $order = array_filter($request->input('order'));
-        foreach ($order as $o => $id) {
+        // Drop blanks only: "0" is the home dashboard tag, which is a valid
+        // id when categories are being reordered.
+        $order = array_filter(
+            (array) $request->input('order', []),
+            fn ($id) => $id !== null && $id !== ''
+        );
+        foreach (array_values($order) as $o => $id) {
             $item = Item::find($id);
+            if ($item === null) {
+                continue;
+            }
             $item->order = $o;
             $item->save();
         }

resources/assets/js/app.js

@@ -53,27 +53,46 @@ $.when($.ready).then(() => {
       }); */
 
   const sortableEl = document.getElementById("sortable");
-  let sortable;
+  const sortables = [];
   if (sortableEl !== null) {
-    // eslint-disable-next-line no-undef
-    sortable = Sortable.create(sortableEl, {
-      disabled: true,
-      animation: 150,
-      forceFallback: !(
-        navigator.userAgent.toLowerCase().indexOf("firefox") > -1
-      ),
-      draggable: ".item-container",
-      onEnd() {
-        const idsInOrder = sortable.toArray();
-        $.post(`${base}order`, { order: idsInOrder });
-      },
-    });
-    // prevent Firefox drag behavior
-    if (navigator.userAgent.toLowerCase().indexOf("firefox") > -1) {
-      sortable.option("setData", (dataTransfer) => {
-        dataTransfer.setData("Text", "");
+    const isFirefox = navigator.userAgent.toLowerCase().indexOf("firefox") > -1;
+    const createSortable = (el, draggable, handle) => {
+      // eslint-disable-next-line no-undef
+      const instance = Sortable.create(el, {
+        disabled: true,
+        animation: 150,
+        forceFallback: !isFirefox,
+        draggable,
+        ...(handle ? { handle } : {}),
+        onEnd(evt) {
+          // eslint-disable-next-line no-undef
+          $.post(`${base}order`, { order: Sortable.get(evt.to).toArray() });
+        },
       });
+      // prevent Firefox drag behavior
+      if (isFirefox) {
+        instance.option("setData", (dataTransfer) => {
+          dataTransfer.setData("Text", "");
+        });
+      }
+      sortables.push(instance);
+    };
+
+    // In categories mode the items are nested inside .category blocks, so
+    // the categories get their own sortable (dragged by their title bar,
+    // whose link is draggable="false" so the native drag source is the
+    // block rather than the anchor) and each category sorts its own items.
+    // The item sortables deliberately share no `group`: moving an item
+    // between categories is a tag change that /order cannot express.
+    const categoryEls = Array.from(sortableEl.querySelectorAll(".category"));
+    if (categoryEls.length > 0) {
+      createSortable(sortableEl, ".category", ".category > .title");
+    }
+    (categoryEls.length > 0 ? categoryEls : [sortableEl]).forEach((el) => {
+      createSortable(el, ".item-container");
+    });
 
+    if (isFirefox) {
       sortableEl.addEventListener("dragstart", (event) => {
         const { target } = event;
         if (target.nodeName.toLowerCase() === "a") {
@@ -84,6 +103,9 @@ $.when($.ready).then(() => {
       });
     }
   }
+  const setSortableDisabled = (disabled) => {
+    sortables.forEach((instance) => instance.option("disabled", disabled));
+  };
 
   $("#main")
     .on("mouseenter", "#sortable .item", function () {
@@ -263,10 +285,10 @@ $.when($.ready).then(() => {
         $(".item-edit").hide();
         $("#app").removeClass("sidebar");
         $("#sortable .tooltip").css("display", "");
-        if (sortable !== undefined) sortable.option("disabled", true);
+        setSortableDisabled(true);
       } else {
         $("#sortable .tooltip").css("display", "none");
-        if (sortable !== undefined) sortable.option("disabled", false);
+        setSortableDisabled(false);
         setTimeout(() => {
           $(".add-item").fadeIn();
           $(".item-edit").fadeIn();

resources/views/sortable.blade.php

@@ -4,7 +4,7 @@
             @foreach($categories as $category)
                 <?php $apps = $category->children; ?>
                 <div class="category item-containerz" data-name="{{ $category->title }}" data-id="{{ $category->id }}">
-                <div class="title"><a href="{{ $category->link }}" style="{{ $category->colour ? 'color: ' . $category->colour .';' : '' }}">{{ $category->title }}</a></div>
+                <div class="title"><a draggable="false" href="{{ $category->link }}" style="{{ $category->colour ? 'color: ' . $category->colour .';' : '' }}">{{ $category->title }}</a></div>
                 @foreach($apps as $app)
                     @include('item')
                 @endforeach

tests/Feature/AjaxPostEndpointsTest.php

@@ -38,6 +38,45 @@ public function test_order_endpoint_persists_the_new_item_order(): void
         $this->assertSame(1, (int) $first->fresh()->order);
     }
 
+    public function test_order_endpoint_reorders_categories_including_the_home_tag(): void
+    {
+        $this->seed();
+
+        // The seeder creates the home dashboard tag at id 0; it is a valid
+        // category and must not be dropped as a "falsy" id.
+        $media = Item::factory()->create(['title' => 'Media', 'type' => 1, 'order' => 0]);
+        $work = Item::factory()->create(['title' => 'Work', 'type' => 1, 'order' => 1]);
+
+        $response = $this->post('/order', [
+            'order' => [$work->id, '0', $media->id],
+        ]);
+
+        $response->assertStatus(200);
+
+        $this->assertSame(0, (int) $work->fresh()->order);
+        $this->assertSame(1, (int) Item::find(0)->order);
+        $this->assertSame(2, (int) $media->fresh()->order);
+    }
+
+    public function test_order_endpoint_skips_ids_that_no_longer_exist(): void
+    {
+        $this->seed();
+
+        $first = Item::factory()->create(['order' => 5]);
+        $second = Item::factory()->create(['order' => 9]);
+
+        $response = $this->post('/order', [
+            'order' => [$second->id, 999999, $first->id],
+        ]);
+
+        $response->assertStatus(200);
+
+        // Positions are taken from the posted list, so the missing id leaves
+        // a gap rather than shifting the items after it.
+        $this->assertSame(0, (int) $second->fresh()->order);
+        $this->assertSame(2, (int) $first->fresh()->order);
+    }
+
     public function test_appload_returns_null_for_the_none_selection(): void
     {
         $this->seed();

tests/Feature/DashTest.php

@@ -16,7 +16,7 @@ class DashTest extends TestCase
      * Helpers
      */
 
-    private function addPinnedItemWithTitleToDB($title)
+    private function addPinnedItemWithTitleToDB($title, $tagId = 0): Item
     {
         $item = Item::factory()
             ->create([
@@ -26,23 +26,21 @@ private function addPinnedItemWithTitleToDB($title)
 
         ItemTag::factory()->create([
             'item_id' => $item->id,
-            'tag_id' => 0,
+            'tag_id' => $tagId,
         ]);
+
+        return $item;
     }
 
-    private function addTagWithTitleToDB($title)
+    private function addTagWithTitleToDB($title, array $attributes = []): Item
     {
-        Item::factory()
-            ->create([
+        return Item::factory()
+            ->create($attributes + [
                 'title' => $title,
                 'type' => 1,
             ]);
     }
 
-    /**
-     * Test Cases
-     */
-
     public function test_loads_empty_dash(): void
     {
         $this->seed();
@@ -89,17 +87,37 @@ public function test_dash_exposes_the_configured_default_tag(): void
         Setting::where('key', 'treat_tags_as')->update(['value' => 'tags']);
         Setting::where('key', 'default_tag')->update(['value' => 'home-dashboard']);
 
-        Item::factory()->create([
-            'title' => 'Home',
-            'url' => 'home-dashboard',
-            'type' => 1,
-            'pinned' => 1,
-            'user_id' => 0,
-        ]);
+        $this->addTagWithTitleToDB('Home', ['url' => 'home-dashboard', 'pinned' => 1, 'user_id' => 0]);
 
         $response = $this->get('/');
 
         $response->assertStatus(200);
         $response->assertSee('data-default-tag="home-dashboard"', false);
     }
+
+    public function test_categories_mode_renders_sortable_category_and_item_markup(): void
+    {
+        $this->seed();
+
+        Setting::where('key', 'treat_tags_as')->update(['value' => 'categories']);
+
+        $tag = $this->addTagWithTitleToDB('Media', ['url' => 'media', 'pinned' => 1, 'user_id' => 0]);
+        $app = $this->addPinnedItemWithTitleToDB('Plex', $tag->id);
+
+        $response = $this->get('/');
+
+        $response->assertStatus(200);
+        // The category block and the item inside it both carry the id that
+        // Sortable posts to /order.
+        $response->assertSee('class="category item-containerz" data-name="Media" data-id="' . $tag->id . '"', false);
+        $response->assertSee('data-name="Plex" data-id="' . $app->id . '"', false);
+        // The item is rendered inside its category block (what the nested
+        // per-category sortable relies on), and the title link opts out of
+        // native dragging so the category itself is the drag source.
+        $html = $response->getContent();
+        $categoryPos = strpos($html, 'data-id="' . $tag->id . '"');
+        $itemPos = strpos($html, 'data-id="' . $app->id . '"');
+        $this->assertLessThan($itemPos, $categoryPos);
+        $this->assertStringContainsString('<a draggable="false" href=', $html);
+    }
 }