FIX: reliably reorder link in custom sections (#24188)

Two changes were introduced:
1. Reorder links on sidebar section is removed. Clicking and holding the mouse for 250ms was unintuitive;
2. Fixed bugs when reorder is done in edit modal.

app/assets/javascripts/discourse/app/components/modal/sidebar-section-form.hbs

@@ -51,6 +51,7 @@
           @link={{link}}
           @deleteLink={{this.deleteLink}}
           @reorderCallback={{this.reorder}}
+          @setDraggedLinkCallback={{this.setDraggedLink}}
         />
       {{/each}}
 
@@ -71,6 +72,7 @@
             @link={{link}}
             @deleteLink={{this.deleteLink}}
             @reorderCallback={{this.reorder}}
+            @setDraggedLinkCallback={{this.setDraggedLink}}
           />
         {{/each}}
         <DButton

app/assets/javascripts/discourse/app/components/modal/sidebar-section-form.js

@@ -384,38 +384,36 @@ export default class SidebarSectionForm extends Component {
   }
 
   @bind
-  reorder(linkFromId, linkTo, above) {
-    if (linkFromId === linkTo.objectId) {
+  setDraggedLink(link) {
+    this.draggedLink = link;
+  }
+
+  @bind
+  reorder(targetLink, above) {
+    if (this.draggedLink === targetLink) {
       return;
     }
-    let linkFrom = this.transformedModel.links.find(
-      (link) => link.objectId === linkFromId
-    );
-    if (!linkFrom) {
-      linkFrom = this.transformedModel.secondaryLinks.find(
-        (link) => link.objectId === linkFromId
-      );
-    }
 
-    if (linkFrom.isPrimary) {
-      this.transformedModel.links.removeObject(linkFrom);
+    if (this.draggedLink.isPrimary) {
+      this.transformedModel.links.removeObject(this.draggedLink);
     } else {
-      this.transformedModel.secondaryLinks?.removeObject(linkFrom);
+      this.transformedModel.secondaryLinks?.removeObject(this.draggedLink);
     }
 
-    if (linkTo.isPrimary) {
-      const toPosition = this.transformedModel.links.indexOf(linkTo);
-      linkFrom.segment = "primary";
+    if (targetLink.isPrimary) {
+      const toPosition = this.transformedModel.links.indexOf(targetLink);
+      this.draggedLink.segment = "primary";
       this.transformedModel.links.insertAt(
         above ? toPosition : toPosition + 1,
-        linkFrom
+        this.draggedLink
       );
     } else {
-      linkFrom.segment = "secondary";
-      const toPosition = this.transformedModel.secondaryLinks.indexOf(linkTo);
+      this.draggedLink.segment = "secondary";
+      const toPosition =
+        this.transformedModel.secondaryLinks.indexOf(targetLink);
       this.transformedModel.secondaryLinks.insertAt(
         above ? toPosition : toPosition + 1,
-        linkFrom
+        this.draggedLink
       );
     }
   }

app/assets/javascripts/discourse/app/components/sidebar/common/custom-section.hbs

@@ -19,15 +19,6 @@
         @fullReload={{link.fullReload}}
         @href={{link.value}}
         @class={{link.linkDragCss}}
-        {{(if
-          this.section.reorderable
-          (modifier
-            "draggable"
-            didStartDrag=link.didStartDrag
-            didEndDrag=link.didEndDrag
-            dragMove=link.dragMove
-          )
-        )}}
       />
     {{else}}
       <Sidebar::SectionLink
@@ -48,15 +39,6 @@
         @suffixType={{link.suffixType}}
         @currentWhen={{link.currentWhen}}
         @class={{link.linkDragCss}}
-        {{(if
-          this.section.reorderable
-          (modifier
-            "draggable"
-            didStartDrag=link.didStartDrag
-            didEndDrag=link.didEndDrag
-            dragMove=link.dragMove
-          )
-        )}}
       />
     {{/if}}
   {{/each}}

app/assets/javascripts/discourse/app/components/sidebar/section-form-link.js

@@ -1,6 +1,7 @@
 import Component from "@glimmer/component";
 import { tracked } from "@glimmer/tracking";
 import { action } from "@ember/object";
+import discourseLater from "discourse-common/lib/later";
 
 export default class SectionFormLink extends Component {
   @tracked dragCssClass;
@@ -17,14 +18,14 @@ export default class SectionFormLink extends Component {
   @action
   dragHasStarted(event) {
     event.dataTransfer.effectAllowed = "move";
-    event.dataTransfer.setData("linkId", this.args.link.objectId);
+    this.args.setDraggedLinkCallback(this.args.link);
     this.dragCssClass = "dragging";
   }
 
   @action
   dragOver(event) {
     event.preventDefault();
-    if (!this.dragCssClass) {
+    if (this.dragCssClass !== "dragging") {
       if (this.isAboveElement(event)) {
         this.dragCssClass = "drag-above";
       } else {
@@ -44,25 +45,23 @@ export default class SectionFormLink extends Component {
       this.dragCount === 0 &&
       (this.dragCssClass === "drag-above" || this.dragCssClass === "drag-below")
     ) {
-      this.dragCssClass = null;
+      discourseLater(() => {
+        this.dragCssClass = null;
+      }, 10);
     }
   }
 
   @action
   dropItem(event) {
     event.stopPropagation();
-    this.dragCounter = 0;
-    this.args.reorderCallback(
-      parseInt(event.dataTransfer.getData("linkId"), 10),
-      this.args.link,
-      this.isAboveElement(event)
-    );
+    this.dragCount = 0;
+    this.args.reorderCallback(this.args.link, this.isAboveElement(event));
     this.dragCssClass = null;
   }
 
   @action
   dragEnd() {
-    this.dragCounter = 0;
+    this.dragCount = 0;
     this.dragCssClass = null;
   }
 }

app/assets/javascripts/discourse/app/lib/sidebar/common/community-section/section.js

@@ -41,7 +41,6 @@ export default class CommunitySection {
   @tracked links;
   @tracked moreLinks;
 
-  reorderable = false;
   hideSectionHeader = true;
 
   constructor({ section, owner }) {

app/assets/javascripts/discourse/app/lib/sidebar/section-link.js

@@ -1,11 +1,6 @@
 import { tracked } from "@glimmer/tracking";
 import RouteInfoHelper from "discourse/lib/sidebar/route-info-helper";
 import { defaultHomepage } from "discourse/lib/utilities";
-import discourseLater from "discourse-common/lib/later";
-import { bind } from "discourse-common/utils/decorators";
-
-const TOUCH_SCREEN_DELAY = 300;
-const MOUSE_DELAY = 250;
 
 export default class SectionLink {
   @tracked linkDragCss;
@@ -46,100 +41,4 @@ export default class SectionLink {
   get externalOrFullReload() {
     return this.external || this.fullReload || this.withAnchor;
   }
-
-  @bind
-  didStartDrag(event) {
-    // 0 represents left button of the mouse
-    if (event.button === 0 || event.targetTouches) {
-      this.startMouseY = this.#calcMouseY(event);
-      this.willDrag = true;
-
-      discourseLater(
-        () => {
-          this.delayedStart(event);
-        },
-        event.targetTouches ? TOUCH_SCREEN_DELAY : MOUSE_DELAY
-      );
-    }
-  }
-
-  delayedStart(event) {
-    if (this.willDrag) {
-      const currentMouseY = this.#calcMouseY(event);
-
-      if (currentMouseY === this.startMouseY) {
-        event.stopPropagation();
-        event.preventDefault();
-        this.mouseY = this.#calcMouseY(event);
-        this.linkDragCss = "drag";
-        this.section.disable();
-        this.drag = true;
-      }
-    }
-  }
-
-  @bind
-  didEndDrag() {
-    this.linkDragCss = null;
-    this.mouseY = null;
-    this.section.enable();
-    this.section.reorder();
-    this.willDrag = false;
-    this.drag = false;
-  }
-
-  @bind
-  dragMove(event) {
-    const moveMouseY = this.#calcMouseY(event);
-
-    if (this.willDrag && moveMouseY !== this.startMouseY && !this.drag) {
-      /**
-       * If mouse position is different, it means that it is a scroll and not drag and drop action.
-       * In that case, we want to do nothing and keep original behaviour.
-       */
-      this.willDrag = false;
-      return;
-    } else {
-      /**
-       * Otherwise, event propagation should be stopped as we have our own handler for drag and drop.
-       */
-      event.stopPropagation();
-      event.preventDefault();
-    }
-
-    this.startMouseY = moveMouseY;
-
-    if (!this.drag) {
-      return;
-    }
-
-    const currentMouseY = this.#calcMouseY(event);
-    const distance = currentMouseY - this.mouseY;
-
-    if (!this.linkHeight) {
-      this.linkHeight = document.getElementsByClassName(
-        "sidebar-section-link-wrapper"
-      )[0].clientHeight;
-    }
-
-    if (distance >= this.linkHeight) {
-      if (this.section.links.indexOf(this) !== this.section.links.length - 1) {
-        this.section.moveLinkDown(this);
-        this.mouseY = currentMouseY;
-      }
-    }
-
-    if (distance <= -this.linkHeight) {
-      if (this.section.links.indexOf(this) !== 0) {
-        this.section.moveLinkUp(this);
-        this.mouseY = currentMouseY;
-      }
-    }
-  }
-
-  #calcMouseY(event) {
-    return Math.round(
-      event.targetTouches ? event.targetTouches[0].clientY : event.y
-    );
-  }
 }

app/assets/javascripts/discourse/app/lib/sidebar/section.js

@@ -15,8 +15,6 @@ export default class Section {
   @tracked dragCss;
   @tracked links;
 
-  reorderable = true;
-
   constructor({ section, owner }) {
     setOwner(this, owner);
 

app/controllers/sidebar_sections_controller.rb

@@ -83,17 +83,6 @@ class SidebarSectionsController < ApplicationController
     render_serialized(sidebar_section, SidebarSectionSerializer)
   end
 
-  def reorder
-    sidebar_section = SidebarSection.find_by(id: reorder_params["sidebar_section_id"])
-    @guardian.ensure_can_edit!(sidebar_section)
-    order = reorder_params["links_order"].map(&:to_i).each_with_index.to_h
-    set_order(sidebar_section, order)
-
-    render_serialized(sidebar_section, SidebarSectionSerializer)
-  rescue Discourse::InvalidAccess
-    render json: failed_json, status: 403
-  end
-
   def destroy
     sidebar_section = SidebarSection.find_by(id: section_params["id"])
     @guardian.ensure_can_delete!(sidebar_section)

config/routes.rb

@@ -1607,7 +1607,6 @@ Discourse::Application.routes.draw do
     delete "user-status" => "user_status#clear"
 
     resources :sidebar_sections, only: %i[index create update destroy]
-    post "/sidebar_sections/reorder" => "sidebar_sections#reorder"
     put "/sidebar_sections/reset/:id" => "sidebar_sections#reset"
 
     get "*url", to: "permalinks#show", constraints: PermalinkConstraint.new

spec/requests/sidebar_sections_controller_spec.rb

@@ -370,72 +370,6 @@ RSpec.describe SidebarSectionsController do
     end
   end
 
-  describe "#reorder" do
-    fab!(:user2) { Fabricate(:user) }
-    fab!(:sidebar_section) { Fabricate(:sidebar_section, user: user) }
-    fab!(:sidebar_url_1) { Fabricate(:sidebar_url, name: "tags", value: "/tags") }
-    fab!(:sidebar_url_2) { Fabricate(:sidebar_url, name: "categories", value: "/categories") }
-    fab!(:sidebar_url_3) { Fabricate(:sidebar_url, name: "topic", value: "/t/1") }
-
-    fab!(:section_link_1) do
-      Fabricate(:sidebar_section_link, sidebar_section: sidebar_section, linkable: sidebar_url_1)
-    end
-
-    fab!(:section_link_2) do
-      Fabricate(:sidebar_section_link, sidebar_section: sidebar_section, linkable: sidebar_url_2)
-    end
-
-    fab!(:section_link_3) do
-      Fabricate(:sidebar_section_link, sidebar_section: sidebar_section, linkable: sidebar_url_3)
-    end
-
-    it "sorts links" do
-      expect(sidebar_section.sidebar_urls.pluck(:id)).to eq(
-        [sidebar_url_1.id, sidebar_url_2.id, sidebar_url_3.id],
-      )
-
-      sign_in(user)
-
-      post "/sidebar_sections/reorder.json",
-           params: {
-             sidebar_section_id: sidebar_section.id,
-             links_order: [sidebar_url_2.id, sidebar_url_3.id, sidebar_url_1.id],
-           }
-
-      expect(response.status).to eq(200)
-
-      expect(sidebar_section.reload.sidebar_urls.pluck(:id)).to eq(
-        [sidebar_url_2.id, sidebar_url_3.id, sidebar_url_1.id],
-      )
-    end
-
-    it "returns 403 when a user tries to reorder a section that doesn't belong to them" do
-      sign_in(user2)
-
-      post "/sidebar_sections/reorder.json",
-           params: {
-             sidebar_section_id: sidebar_section.id,
-             links_order: [sidebar_url_2.id, sidebar_url_3.id, sidebar_url_1.id],
-           }
-
-      expect(response.status).to eq(403)
-
-      expect(sidebar_section.reload.sidebar_urls.pluck(:id)).to eq(
-        [sidebar_url_1.id, sidebar_url_2.id, sidebar_url_3.id],
-      )
-    end
-
-    it "returns 403 for an non user" do
-      post "/sidebar_sections/reorder.json",
-           params: {
-             sidebar_section_id: sidebar_section.id,
-             links_order: [sidebar_url_2.id, sidebar_url_3.id, sidebar_url_1.id],
-           }
-
-      expect(response.status).to eql(403)
-    end
-  end
-
   describe "#destroy" do
     fab!(:sidebar_section) { Fabricate(:sidebar_section, user: user) }
 

spec/system/custom_sidebar_sections_spec.rb

@@ -178,9 +178,12 @@ describe "Custom sidebar sections", type: :system do
       ["Sidebar Tags", "Sidebar Categories", "Sidebar Latest"],
     )
 
-    tags_link = find(".sidebar-section-link[data-link-name='Sidebar Tags']")
-    latest_link = find(".sidebar-section-link[data-link-name='Sidebar Latest']")
+    sidebar.edit_custom_section("My section")
+
+    tags_link = find(".draggable[data-link-name='Sidebar Tags']")
+    latest_link = find(".draggable[data-link-name='Sidebar Latest']")
     tags_link.drag_to(latest_link, html5: true, delay: 0.4)
+    section_modal.save
 
     expect(sidebar.primary_section_links("my-section")).to eq(
       ["Sidebar Categories", "Sidebar Tags", "Sidebar Latest"],