From daa2a8d1b91a5e7115df82b6c90677a42feb5f77 Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Tue, 16 Feb 2021 16:40:43 +0100 Subject: [PATCH 1/2] Revert "Do not change page_id on reorder elements" This reverts commit c9b1c6857ad08016d0115e817df8fa324ad1e140. --- app/controllers/alchemy/admin/elements_controller.rb | 11 +++++++---- .../alchemy/admin/elements_controller_spec.rb | 8 ++++++-- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/app/controllers/alchemy/admin/elements_controller.rb b/app/controllers/alchemy/admin/elements_controller.rb index 558e946f73..bb1ac1c473 100644 --- a/app/controllers/alchemy/admin/elements_controller.rb +++ b/app/controllers/alchemy/admin/elements_controller.rb @@ -73,13 +73,16 @@ def publish def order @parent_element = Element.find_by(id: params[:parent_element_id]) Element.transaction do - Element.where(id: params.fetch(:element_ids, [])).each.with_index(1) do |element, position| - element.update_columns( + params.fetch(:element_ids, []).each_with_index do |element_id, idx| + # Ensure to set page_id and parent_element_id to the current + # because of trashed elements could still have old values + Element.where(id: element_id).update_all( + page_id: params[:page_id], parent_element_id: params[:parent_element_id], - position: position + position: idx + 1, ) end - @parent_element&.touch + @parent_element.try!(:touch) end end diff --git a/spec/controllers/alchemy/admin/elements_controller_spec.rb b/spec/controllers/alchemy/admin/elements_controller_spec.rb index 3b699bff76..373e7c36f5 100644 --- a/spec/controllers/alchemy/admin/elements_controller_spec.rb +++ b/spec/controllers/alchemy/admin/elements_controller_spec.rb @@ -51,13 +51,15 @@ module Alchemy let(:page) { element_1.page } it "sets new position for given element ids" do - post :order, params: { element_ids: element_ids }, xhr: true + post :order, params: {page_id: page.id, element_ids: element_ids}, xhr: true expect(Element.all.pluck(:id)).to eq(element_ids) end context "with missing [:element_ids] param" do it "does not raise any error and silently rejects to order" do - expect { post :order, xhr: true }.to_not raise_error + expect { + post :order, params: {page_id: page.id}, xhr: true + }.to_not raise_error end end @@ -68,6 +70,7 @@ module Alchemy expect(Element).to receive(:find_by) { parent } expect(parent).to receive(:touch) { true } post :order, params: { + page_id: page.id, element_ids: element_ids, parent_element_id: parent.id, }, xhr: true @@ -75,6 +78,7 @@ module Alchemy it "assigns parent element id to each element" do post :order, params: { + page_id: page.id, element_ids: element_ids, parent_element_id: parent.id, }, xhr: true From 45b667eee19f20a1720ca2f75db09a2e3429711e Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Tue, 16 Feb 2021 17:07:41 +0100 Subject: [PATCH 2/2] Do not change page_id on reorder elements Reordering elements does not need to change the elements page id anymore since we removed the trash. This time we do not change the order of the element ids passed via the params. To verify I updated the test so that the elements get created upfront and the order is explicitely assured. --- .../alchemy/admin/elements_controller.rb | 15 +++++---- .../alchemy/admin/elements_controller_spec.rb | 33 ++++++++++--------- 2 files changed, 25 insertions(+), 23 deletions(-) diff --git a/app/controllers/alchemy/admin/elements_controller.rb b/app/controllers/alchemy/admin/elements_controller.rb index bb1ac1c473..2c714611eb 100644 --- a/app/controllers/alchemy/admin/elements_controller.rb +++ b/app/controllers/alchemy/admin/elements_controller.rb @@ -73,16 +73,17 @@ def publish def order @parent_element = Element.find_by(id: params[:parent_element_id]) Element.transaction do - params.fetch(:element_ids, []).each_with_index do |element_id, idx| - # Ensure to set page_id and parent_element_id to the current - # because of trashed elements could still have old values - Element.where(id: element_id).update_all( - page_id: params[:page_id], + params.fetch(:element_ids, []).each.with_index(1) do |element_id, position| + # We need to set the parent_element_id, because we might have dragged the + # element over from another nestable element + Element.find_by(id: element_id).update_columns( parent_element_id: params[:parent_element_id], - position: idx + 1, + position: position, ) end - @parent_element.try!(:touch) + # Need to manually touch the parent because Rails does not do it + # with the update_columns above + @parent_element&.touch end end diff --git a/spec/controllers/alchemy/admin/elements_controller_spec.rb b/spec/controllers/alchemy/admin/elements_controller_spec.rb index 373e7c36f5..e7870b34d8 100644 --- a/spec/controllers/alchemy/admin/elements_controller_spec.rb +++ b/spec/controllers/alchemy/admin/elements_controller_spec.rb @@ -44,22 +44,24 @@ module Alchemy end describe "#order" do - let(:element_1) { create(:alchemy_element) } - let(:element_2) { create(:alchemy_element, page: page) } - let(:element_3) { create(:alchemy_element, page: page) } + let!(:element_1) { create(:alchemy_element) } + let!(:element_2) { create(:alchemy_element, page: page) } + let!(:element_3) { create(:alchemy_element, page: page) } let(:element_ids) { [element_1.id, element_3.id, element_2.id] } let(:page) { element_1.page } it "sets new position for given element ids" do - post :order, params: {page_id: page.id, element_ids: element_ids}, xhr: true - expect(Element.all.pluck(:id)).to eq(element_ids) + post :order, params: { element_ids: element_ids }, xhr: true + expect(Element.all.pluck(:id, :position)).to eq([ + [element_1.id, 1], + [element_3.id, 2], + [element_2.id, 3], + ]) end context "with missing [:element_ids] param" do it "does not raise any error and silently rejects to order" do - expect { - post :order, params: {page_id: page.id}, xhr: true - }.to_not raise_error + expect { post :order, xhr: true }.to_not raise_error end end @@ -67,18 +69,17 @@ module Alchemy let(:parent) { create(:alchemy_element) } it "touches the cache key of parent element" do - expect(Element).to receive(:find_by) { parent } - expect(parent).to receive(:touch) { true } - post :order, params: { - page_id: page.id, - element_ids: element_ids, - parent_element_id: parent.id, - }, xhr: true + parent.update_column(:updated_at, 3.days.ago) + expect { + post :order, params: { + element_ids: element_ids, + parent_element_id: parent.id, + }, xhr: true + }.to change { parent.reload.updated_at } end it "assigns parent element id to each element" do post :order, params: { - page_id: page.id, element_ids: element_ids, parent_element_id: parent.id, }, xhr: true