diff --git a/app/assets/javascripts/discourse/app/controllers/reorder-categories.js b/app/assets/javascripts/discourse/app/controllers/reorder-categories.js index 47da67b284..36b2d9d9eb 100644 --- a/app/assets/javascripts/discourse/app/controllers/reorder-categories.js +++ b/app/assets/javascripts/discourse/app/controllers/reorder-categories.js @@ -60,56 +60,97 @@ export default Controller.extend(ModalFunctionality, Evented, { this.notifyPropertyChange("categoriesBuffered"); }, + countDescendants(category) { + return category.get("subcategories") + ? category + .get("subcategories") + .reduce( + (count, subcategory) => count + this.countDescendants(subcategory), + category.get("subcategories").length + ) + : 0; + }, + move(category, direction) { - let otherCategory; + let targetPosition = category.get("position") + direction; - if (direction === -1) { - // First category above current one - const categoriesOrderedDesc = this.categoriesOrdered.reverse(); - otherCategory = categoriesOrderedDesc.find( - (c) => - category.get("parent_category_id") === c.get("parent_category_id") && - c.get("position") < category.get("position") - ); - } else if (direction === 1) { - // First category under current one - otherCategory = this.categoriesOrdered.find( - (c) => - category.get("parent_category_id") === c.get("parent_category_id") && - c.get("position") > category.get("position") - ); - } else { - // Find category occupying target position - otherCategory = this.categoriesOrdered.find( - (c) => c.get("position") === category.get("position") + direction - ); - } - - if (otherCategory) { - // Try to swap positions of the two categories - if (category.get("id") !== otherCategory.get("id")) { - const currentPosition = category.get("position"); - category.set("position", otherCategory.get("position")); - otherCategory.set("position", currentPosition); + // Adjust target position for sub-categories + if (direction > 0) { + // Moving down (position gets larger) + if (category.get("isParent")) { + // This category has subcategories, adjust targetPosition to account for them + let offset = this.countDescendants(category); + if (direction <= offset) { + // Only apply offset if target position is occupied by a subcategory + // Seems weird but fixes a UX quirk + targetPosition += offset; + } + } + } else { + // Moving up (position gets smaller) + const otherCategory = this.categoriesOrdered.find( + (c) => + // find category currently at targetPosition + c.get("position") === targetPosition + ); + if (otherCategory && otherCategory.get("ancestors")) { + // Target category is a subcategory, adjust targetPosition to account for ancestors + const highestAncestor = otherCategory + .get("ancestors") + .reduce((current, min) => + current.get("position") < min.get("position") ? current : min + ); + targetPosition = highestAncestor.get("position"); } - } else if (direction < 0) { - category.set("position", -1); - } else if (direction > 0) { - category.set("position", this.categoriesOrdered.length); } + // Adjust target position for range bounds + if (targetPosition >= this.categoriesOrdered.length) { + // Set to max + targetPosition = this.categoriesOrdered.length - 1; + } else if (targetPosition < 0) { + // Set to min + targetPosition = 0; + } + + // Update other categories between current and target position + this.categoriesOrdered.map((c) => { + if (direction < 0) { + // Moving up (position gets smaller) + if ( + c.get("position") < category.get("position") && + c.get("position") >= targetPosition + ) { + const newPosition = c.get("position") + 1; + c.set("position", newPosition); + } + } else { + // Moving down (position gets larger) + if ( + c.get("position") > category.get("position") && + c.get("position") <= targetPosition + ) { + const newPosition = c.get("position") - 1; + c.set("position", newPosition); + } + } + }); + + // Update this category's position to target position + category.set("position", targetPosition); + this.reorder(); }, actions: { change(category, event) { - let newPosition = parseInt(event.target.value, 10); - newPosition = Math.min( - Math.max(newPosition, 0), - this.categoriesOrdered.length - 1 - ); - - this.move(category, newPosition - category.get("position")); + let newPosition = parseFloat(event.target.value); + newPosition = + newPosition < category.get("position") + ? Math.ceil(newPosition) + : Math.floor(newPosition); + const direction = newPosition - category.get("position"); + this.move(category, direction); }, moveUp(category) { diff --git a/app/assets/javascripts/discourse/tests/unit/controllers/reorder-categories-test.js b/app/assets/javascripts/discourse/tests/unit/controllers/reorder-categories-test.js index 2feb422d95..821b2cbb85 100644 --- a/app/assets/javascripts/discourse/tests/unit/controllers/reorder-categories-test.js +++ b/app/assets/javascripts/discourse/tests/unit/controllers/reorder-categories-test.js @@ -11,17 +11,14 @@ discourseModule("Unit | Controller | reorder-categories", function () { categories.push(store.createRecord("category", { id: i, position: 0 })); } - const reorderCategoriesController = this.getController( - "reorder-categories", - { site: { categories } } - ); - reorderCategoriesController.reorder(); + const controller = this.getController("reorder-categories", { + site: { categories }, + }); + controller.reorder(); - reorderCategoriesController - .get("categoriesOrdered") - .forEach((category, index) => { - assert.equal(category.get("position"), index); - }); + controller.get("categoriesOrdered").forEach((category, index) => { + assert.equal(category.get("position"), index); + }); }); test("reorder places subcategories after their parent categories, while maintaining the relative order", function (assert) { @@ -51,14 +48,13 @@ discourseModule("Unit | Controller | reorder-categories", function () { }); const expectedOrderSlugs = ["parent", "child2", "child1", "other"]; - const reorderCategoriesController = this.getController( - "reorder-categories", - { site: { categories: [child2, parent, other, child1] } } - ); - reorderCategoriesController.reorder(); + const controller = this.getController("reorder-categories", { + site: { categories: [child2, parent, other, child1] }, + }); + controller.reorder(); assert.deepEqual( - reorderCategoriesController.get("categoriesOrdered").mapBy("slug"), + controller.get("categoriesOrdered").mapBy("slug"), expectedOrderSlugs ); }); @@ -84,21 +80,18 @@ discourseModule("Unit | Controller | reorder-categories", function () { slug: "test", }); - const reorderCategoriesController = this.getController( - "reorder-categories", - { site: { categories: [elem1, elem2, elem3] } } - ); + const controller = this.getController("reorder-categories", { + site: { categories: [elem1, elem2, elem3] }, + }); - reorderCategoriesController.actions.change.call( - reorderCategoriesController, - elem1, - { target: { value: "2" } } - ); + // Move category 'foo' from position 0 to position 2 + controller.send("change", elem1, { target: { value: "2" } }); - assert.deepEqual( - reorderCategoriesController.get("categoriesOrdered").mapBy("slug"), - ["test", "bar", "foo"] - ); + assert.deepEqual(controller.get("categoriesOrdered").mapBy("slug"), [ + "bar", + "test", + "foo", + ]); }); test("changing the position number of a category should place it at given position and respect children", function (assert) { @@ -129,72 +122,71 @@ discourseModule("Unit | Controller | reorder-categories", function () { slug: "test", }); - const reorderCategoriesController = this.getController( - "reorder-categories", - { site: { categories: [elem1, child1, elem2, elem3] } } - ); + const controller = this.getController("reorder-categories", { + site: { categories: [elem1, child1, elem2, elem3] }, + }); - reorderCategoriesController.actions.change.call( - reorderCategoriesController, - elem1, - { target: { value: 3 } } - ); + controller.send("change", elem1, { target: { value: 3 } }); - assert.deepEqual( - reorderCategoriesController.get("categoriesOrdered").mapBy("slug"), - ["test", "bar", "foo", "foochild"] - ); + assert.deepEqual(controller.get("categoriesOrdered").mapBy("slug"), [ + "bar", + "test", + "foo", + "foochild", + ]); }); test("changing the position through click on arrow of a category should place it at given position and respect children", function (assert) { const store = createStore(); - const elem1 = store.createRecord("category", { - id: 1, - position: 0, - slug: "foo", + const child2 = store.createRecord("category", { + id: 105, + position: 2, + slug: "foochildchild", + parent_category_id: 104, }); const child1 = store.createRecord("category", { - id: 4, + id: 104, position: 1, slug: "foochild", - parent_category_id: 1, + parent_category_id: 101, + subcategories: [child2], }); - const child2 = store.createRecord("category", { - id: 5, - position: 2, - slug: "foochildchild", - parent_category_id: 4, + const elem1 = store.createRecord("category", { + id: 101, + position: 0, + slug: "foo", + subcategories: [child1], }); const elem2 = store.createRecord("category", { - id: 2, + id: 102, position: 3, slug: "bar", }); const elem3 = store.createRecord("category", { - id: 3, + id: 103, position: 4, slug: "test", }); - const reorderCategoriesController = this.getController( - "reorder-categories", - { site: { categories: [elem1, child1, child2, elem2, elem3] } } - ); - reorderCategoriesController.reorder(); + const controller = this.getController("reorder-categories", { + site: { categories: [elem1, child1, child2, elem2, elem3] }, + }); - reorderCategoriesController.actions.moveDown.call( - reorderCategoriesController, - elem1 - ); + controller.reorder(); - assert.deepEqual( - reorderCategoriesController.get("categoriesOrdered").mapBy("slug"), - ["bar", "foo", "foochild", "foochildchild", "test"] - ); + controller.send("moveDown", elem1); + + assert.deepEqual(controller.get("categoriesOrdered").mapBy("slug"), [ + "bar", + "foo", + "foochild", + "foochildchild", + "test", + ]); }); });