UX: auto fix order when reordering categories (#6149)

* set correct position number when moving up/down

* UX: drop 'fix order' and auto re-order subcategory

- auto "fix position" on save
- place subcategories after parent category and maintain the relative
positions on save

app/assets/javascripts/discourse/controllers/reorder-categories.js.es6

@@ -11,7 +11,7 @@ import Ember from "ember";
 export default Ember.Controller.extend(ModalFunctionality, Ember.Evented, {
   @on("init")
   _fixOrder() {
-    this.send("fixIndices");
+    this.fixIndices();
   },
 
   @computed("site.categories")
@@ -26,17 +26,6 @@ export default Ember.Controller.extend(ModalFunctionality, Ember.Evented, {
     "categoriesSorting"
   ),
 
-  showFixIndices: function() {
-    const cats = this.get("categoriesOrdered");
-    const len = cats.get("length");
-    for (let i = 0; i < len; i++) {
-      if (cats.objectAt(i).get("position") !== i) {
-        return true;
-      }
-    }
-    return false;
-  }.property("categoriesOrdered.@each.position"),
-
   showApplyAll: function() {
     let anyChanged = false;
     this.get("categoriesBuffered").forEach(bc => {
@@ -45,21 +34,56 @@ export default Ember.Controller.extend(ModalFunctionality, Ember.Evented, {
     return anyChanged;
   }.property("categoriesBuffered.@each.hasBufferedChanges"),
 
-  saveDisabled: Ember.computed.or("showApplyAll", "showFixIndices"),
+  saveDisabled: Ember.computed.alias("showApplyAll"),
 
   moveDir(cat, dir) {
     const cats = this.get("categoriesOrdered");
     const curIdx = cats.indexOf(cat);
     const desiredIdx = curIdx + dir;
     if (desiredIdx >= 0 && desiredIdx < cats.get("length")) {
-      const curPos = cat.get("position");
-      cat.set("position", curPos + dir);
       const otherCat = cats.objectAt(desiredIdx);
-      otherCat.set("position", curPos - dir);
+      otherCat.set("position", curIdx);
+      cat.set("position", desiredIdx);
       this.send("commit");
     }
   },
 
+  /**
+    1. Make sure all categories have unique position numbers.
+    2. Place sub-categories after their parent categories while maintaining the
+        same relative order.
+
+        e.g.
+          parent/c1         parent
+          parent      =>    parent/c1
+          other             parent/c2
+          parent/c2         other
+  **/
+  fixIndices() {
+    const categories = this.get("categoriesOrdered");
+    const subcategories = {};
+
+    categories.forEach(category => {
+      const parentCategoryId = category.get("parent_category_id");
+
+      if (parentCategoryId) {
+        subcategories[parentCategoryId] = subcategories[parentCategoryId] || [];
+        subcategories[parentCategoryId].push(category);
+      }
+    });
+
+    for (let i = 0, position = 0; i < categories.get("length"); ++i) {
+      const category = categories.objectAt(i);
+
+      if (!category.get("parent_category_id")) {
+        category.set("position", position++);
+        (subcategories[category.get("id")] || []).forEach(subcategory => subcategory.set("position", position++));
+      }
+    }
+
+    this.send("commit");
+  },
+
   actions: {
     moveUp(cat) {
       this.moveDir(cat, -1);
@@ -68,15 +92,6 @@ export default Ember.Controller.extend(ModalFunctionality, Ember.Evented, {
       this.moveDir(cat, 1);
     },
 
-    fixIndices() {
-      const cats = this.get("categoriesOrdered");
-      const len = cats.get("length");
-      for (let i = 0; i < len; i++) {
-        cats.objectAt(i).set("position", i);
-      }
-      this.send("commit");
-    },
-
     commit() {
       this.get("categoriesBuffered").forEach(bc => {
         if (bc.get("hasBufferedChanges")) {
@@ -87,6 +102,8 @@ export default Ember.Controller.extend(ModalFunctionality, Ember.Evented, {
     },
 
     saveOrder() {
+      this.fixIndices();
+
       const data = {};
       this.get("categoriesBuffered").forEach(cat => {
         data[cat.get("id")] = cat.get("position");

app/assets/javascripts/discourse/templates/modal/reorder-categories.hbs

@@ -23,9 +23,6 @@
 {{/d-modal-body}}
 
 <div class="modal-footer">
-  {{#if showFixIndices}}
-    {{d-button action="fixIndices" icon="random" label="categories.reorder.fix_order" title="categories.reorder.fix_order_tooltip"}}
-  {{/if}}
   {{#if showApplyAll}}
     {{d-button action="commit" icon="check" label="categories.reorder.apply_all"}}
   {{/if}}

config/locales/client.en.yml

@@ -556,8 +556,6 @@ en:
       reorder:
         title: "Reorder Categories"
         title_long: "Reorganize the category list"
-        fix_order: "Fix Positions"
-        fix_order_tooltip: "Not all categories have a unique position number, which may cause unexpected results."
         save: "Save Order"
         apply_all: "Apply"
         position: "Position"

test/javascripts/controllers/reorder-categories-test.js.es6

@@ -0,0 +1,50 @@
+import { mapRoutes } from "discourse/mapping-router";
+import createStore from "helpers/create-store";
+
+moduleFor("controller:reorder-categories", "controller:reorder-categories", {
+  beforeEach() {
+    this.registry.register("router:main", mapRoutes());
+  },
+  needs: ["controller:modal"]
+});
+
+QUnit.test("fixIndices set unique position number", function(assert) {
+  const store = createStore();
+
+  const categories = [];
+  for (let i = 0; i < 3; ++i) {
+    categories.push(store.createRecord("category", { id: i, position: 0 }));
+  }
+
+  const site = Ember.Object.create({ categories: categories });
+  const reorderCategoriesController = this.subject({ site });
+
+  reorderCategoriesController.fixIndices();
+
+  reorderCategoriesController.get("categoriesOrdered").forEach((category, index) => {
+    assert.equal(category.get("position"), index);
+  });
+});
+
+QUnit.test(
+  "fixIndices places subcategories after their parent categories, while maintaining the relative order",
+  function(assert) {
+    const store = createStore();
+
+    const parent = store.createRecord("category", { id: 1, position: 1, slug: "parent" });
+    const child1 = store.createRecord("category", { id: 2, position: 3, slug: "child1", parent_category_id: 1 });
+    const child2 = store.createRecord("category", { id: 3, position: 0, slug: "child2", parent_category_id: 1 });
+    const other = store.createRecord("category", { id: 4, position: 2, slug: "other" });
+
+    const categories = [child2, parent, other, child1];
+    const expectedOrderSlugs = ["parent", "child2", "child1", "other"];
+
+    const site = Ember.Object.create({ categories: categories });
+    const reorderCategoriesController = this.subject({ site });
+
+    reorderCategoriesController.fixIndices();
+
+    assert.deepEqual(reorderCategoriesController.get("categoriesOrdered").mapBy("slug"), expectedOrderSlugs);
+  }
+);
+