From 35215e3e2d90bbceb90a0615ae9b42edc3baedbb Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Fri, 14 Jun 2019 22:41:44 +0200 Subject: [PATCH 1/7] Add an all_elements relation to page We have scopes on the page.elements relation that makes it impossible to get all elements from a page. --- app/models/alchemy/page/page_elements.rb | 3 ++ spec/models/alchemy/page_spec.rb | 53 ++++++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/app/models/alchemy/page/page_elements.rb b/app/models/alchemy/page/page_elements.rb index 6d1bf66c5f..f3bde9c9ad 100644 --- a/app/models/alchemy/page/page_elements.rb +++ b/app/models/alchemy/page/page_elements.rb @@ -7,6 +7,9 @@ module Page::PageElements included do attr_accessor :autogenerate_elements + has_many :all_elements, + -> { order(:position) }, + class_name: 'Alchemy::Element' has_many :elements, -> { order(:position).not_nested.unfixed.not_trashed }, class_name: 'Alchemy::Element' diff --git a/spec/models/alchemy/page_spec.rb b/spec/models/alchemy/page_spec.rb index fb695e7bd1..d0c31795c6 100644 --- a/spec/models/alchemy/page_spec.rb +++ b/spec/models/alchemy/page_spec.rb @@ -908,6 +908,59 @@ module Alchemy end end + describe "#all_elements" do + let(:page) { create(:alchemy_page) } + let!(:element_1) { create(:alchemy_element, page: page) } + let!(:element_2) { create(:alchemy_element, page: page) } + let!(:element_3) { create(:alchemy_element, page: page) } + + before do + element_3.move_to_top + end + + it 'returns a ordered active record collection of elements on that page' do + expect(page.all_elements).to eq([element_3, element_1, element_2]) + end + + context 'with nestable elements' do + let!(:nestable_element) do + create(:alchemy_element, page: page) + end + + let!(:nested_element) do + create(:alchemy_element, name: 'slide', parent_element: nestable_element, page: page) + end + + it 'contains nested elements of an element' do + expect(page.all_elements).to include(nested_element) + end + end + + context 'with trashed elements' do + let(:trashed_element) { create(:alchemy_element, page: page).tap(&:trash!) } + + it 'contains trashed elements' do + expect(page.all_elements).to include(trashed_element) + end + end + + context 'with hidden elements' do + let(:hidden_element) { create(:alchemy_element, page: page, public: false) } + + it 'contains hidden elements' do + expect(page.all_elements).to include(hidden_element) + end + end + + context 'with fixed elements' do + let(:fixed_element) { create(:alchemy_element, page: page, fixed: true) } + + it 'contains hidden elements' do + expect(page.all_elements).to include(fixed_element) + end + end + end + describe "#elements" do let(:page) { create(:alchemy_page) } let!(:element_1) { create(:alchemy_element, page: page) } From d2587b84e0f7ec7e8305878cf1b050dc56f320a9 Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Fri, 14 Jun 2019 23:03:17 +0200 Subject: [PATCH 2/7] Use explicit scopes in admin elements controller Instead of using the page.elements relation that is meant to be used by frontend devs and rely on that relation to never change we use explicit scopes to load exactly the elements we need in the admin. --- .../alchemy/admin/elements_controller.rb | 4 +- .../alchemy/admin/elements_controller_spec.rb | 39 +++++++++++-------- 2 files changed, 24 insertions(+), 19 deletions(-) diff --git a/app/controllers/alchemy/admin/elements_controller.rb b/app/controllers/alchemy/admin/elements_controller.rb index dbfef9610d..2e5ff1502d 100644 --- a/app/controllers/alchemy/admin/elements_controller.rb +++ b/app/controllers/alchemy/admin/elements_controller.rb @@ -8,8 +8,8 @@ class ElementsController < Alchemy::Admin::BaseController def index @page = Page.find(params[:page_id]) - @elements = @page.elements - @fixed_elements = @page.fixed_elements + @elements = @page.all_elements.not_nested.unfixed.not_trashed + @fixed_elements = @page.all_elements.fixed.not_trashed end def list diff --git a/spec/controllers/alchemy/admin/elements_controller_spec.rb b/spec/controllers/alchemy/admin/elements_controller_spec.rb index 5bb4930546..907284c48d 100644 --- a/spec/controllers/alchemy/admin/elements_controller_spec.rb +++ b/spec/controllers/alchemy/admin/elements_controller_spec.rb @@ -14,28 +14,39 @@ module Alchemy before { authorize_user(:as_author) } describe '#index' do - let(:alchemy_page) { build_stubbed(:alchemy_page) } - - before do - expect(Page).to receive(:find).and_return alchemy_page - end + let!(:alchemy_page) { create(:alchemy_page) } + let!(:element) { create(:alchemy_element, page: alchemy_page) } + let!(:trashed_element) { create(:alchemy_element, page: alchemy_page).tap(&:trash!) } + let!(:nested_element) { create(:alchemy_element, :nested, page: alchemy_page) } + let!(:hidden_element) { create(:alchemy_element, page: alchemy_page, public: false) } context 'with fixed elements' do - let(:fixed_element) { build_stubbed(:alchemy_element, :fixed, page: alchemy_page) } + let!(:fixed_element) do + create(:alchemy_element, :fixed, + page: alchemy_page) + end - before do - expect(alchemy_page).to receive(:fixed_elements).and_return [fixed_element] + let!(:fixed_hidden_element) do + create(:alchemy_element, :fixed, + public: false, + page: alchemy_page) + end + + let!(:fixed_trashed_element) do + create(:alchemy_element, :fixed, + public: false, + page: alchemy_page).tap(&:trash!) end it "assigns fixed elements" do get :index, params: {page_id: alchemy_page.id} - expect(assigns(:fixed_elements)).to eq([fixed_element]) + expect(assigns(:fixed_elements)).to eq([fixed_element, fixed_hidden_element]) end end it "assigns page elements" do - expect(alchemy_page).to receive(:elements).and_return(double(not_trashed: [])) get :index, params: {page_id: alchemy_page.id} + expect(assigns(:elements)).to eq([element, hidden_element]) end end @@ -111,13 +122,7 @@ module Alchemy end context "untrashing" do - let(:trashed_element) { create(:alchemy_element) } - - before do - # Because of a before_create filter it can not be created with a nil position - # and needs to be trashed here - trashed_element.trash! - end + let!(:trashed_element) { create(:alchemy_element).tap(&:trash!) } it "sets a list of trashed element ids" do post :order, params: {page_id: page.id, element_ids: [trashed_element.id]}, xhr: true From 0ee8f95e0517a62242716b9eb2499e8f578ceb2e Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Sat, 15 Jun 2019 00:17:00 +0200 Subject: [PATCH 3/7] Remove unnecessary page element relations These two relations were only used in one method. We can create the necessary query in that method instead. --- app/models/alchemy/element.rb | 2 +- app/models/alchemy/page/page_elements.rb | 11 +---- spec/models/alchemy/page_spec.rb | 54 ------------------------ 3 files changed, 3 insertions(+), 64 deletions(-) diff --git a/app/models/alchemy/element.rb b/app/models/alchemy/element.rb index 05be69dddb..7dbb689510 100644 --- a/app/models/alchemy/element.rb +++ b/app/models/alchemy/element.rb @@ -75,7 +75,7 @@ class Element < BaseRecord foreign_key: :parent_element_id, dependent: :destroy - belongs_to :page, touch: true, inverse_of: :descendent_elements + belongs_to :page, touch: true, inverse_of: :all_elements # A nested element belongs to a parent element. belongs_to :parent_element, diff --git a/app/models/alchemy/page/page_elements.rb b/app/models/alchemy/page/page_elements.rb index f3bde9c9ad..c0c18bf01c 100644 --- a/app/models/alchemy/page/page_elements.rb +++ b/app/models/alchemy/page/page_elements.rb @@ -22,14 +22,7 @@ module Page::PageElements has_many :fixed_elements, -> { order(:position).fixed.not_trashed }, class_name: 'Alchemy::Element' - has_many :descendent_elements, - -> { order(:position).unfixed.not_trashed }, - class_name: 'Alchemy::Element' has_many :contents, through: :elements - has_many :descendent_contents, - through: :descendent_elements, - class_name: 'Alchemy::Content', - source: :contents has_and_belongs_to_many :to_be_swept_elements, -> { distinct }, class_name: 'Alchemy::Element', join_table: ElementToPage.table_name @@ -186,8 +179,8 @@ def feed_elements # Returns an array of all EssenceRichtext contents ids from not folded elements # def richtext_contents_ids - descendent_contents - .where(Element.table_name => {folded: false}) + Alchemy::Content.joins(:element) + .where(Element.table_name => {page_id: id, folded: false}) .select(&:has_tinymce?) .collect(&:id) end diff --git a/spec/models/alchemy/page_spec.rb b/spec/models/alchemy/page_spec.rb index d0c31795c6..be6ef1701b 100644 --- a/spec/models/alchemy/page_spec.rb +++ b/spec/models/alchemy/page_spec.rb @@ -990,60 +990,6 @@ module Alchemy end end - describe "#descendent_elements" do - let!(:page) do - create(:alchemy_page) - end - - let!(:element_1) do - create(:alchemy_element, page: page) - end - - let!(:element_2) do - create(:alchemy_element, :with_nestable_elements, page: page, parent_element_id: element_1.id) - end - - let!(:element_3) do - create(:alchemy_element, page: page) - end - - it 'returns an active record collection of all elements including nested elements on that page' do - expect(page.descendent_elements.count).to eq(4) - end - end - - describe "#descendent_contents" do - let!(:page) do - create(:alchemy_page) - end - - let!(:element_1) do - create :alchemy_element, - :with_nestable_elements, - :with_contents, { - name: 'slider', - page: page - } - end - - let!(:element_2) do - create :alchemy_element, - :with_contents, { - name: 'slide', - page: page, - parent_element_id: element_1.id - } - end - - let!(:element_3) do - create(:alchemy_element, :with_contents, name: 'slide', page: page) - end - - it 'returns an active record collection of all content including nested elements on that page' do - expect(page.descendent_contents.count).to eq(6) - end - end - describe '#element_definitions' do let(:page) { build_stubbed(:alchemy_page) } subject { page.element_definitions } From 509d0e48c09816c409792185d7789c3f0ee405ad Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Mon, 17 Jun 2019 22:25:23 +0200 Subject: [PATCH 4/7] Remove elements_including_fixed relation from Page This relation was only used in two internal methods and was never meant to be public API. --- app/models/alchemy/page/page_elements.rb | 22 +++++------- .../alchemy/admin/trash_controller_spec.rb | 7 ++-- spec/models/alchemy/page_spec.rb | 36 +++++++------------ 3 files changed, 23 insertions(+), 42 deletions(-) diff --git a/app/models/alchemy/page/page_elements.rb b/app/models/alchemy/page/page_elements.rb index c0c18bf01c..f4e25e261c 100644 --- a/app/models/alchemy/page/page_elements.rb +++ b/app/models/alchemy/page/page_elements.rb @@ -13,9 +13,6 @@ module Page::PageElements has_many :elements, -> { order(:position).not_nested.unfixed.not_trashed }, class_name: 'Alchemy::Element' - has_many :elements_including_fixed, - -> { order(:position).not_nested.not_trashed }, - class_name: 'Alchemy::Element' has_many :trashed_elements, -> { Element.trashed.order(:position) }, class_name: 'Alchemy::Element' @@ -45,15 +42,12 @@ module ClassMethods # @return [Array] # def copy_elements(source, target) - new_elements = [] - source.elements_including_fixed.each do |source_element| - new_element = Element.copy(source_element, { + source_elements = source.all_elements.not_nested.not_trashed + source_elements.order(:position).map do |source_element| + Element.copy(source_element, { page_id: target.id - }) - new_element.move_to_bottom - new_elements << new_element + }).tap(&:move_to_bottom) end - new_elements end end @@ -87,7 +81,8 @@ def available_element_definitions(only_element_named = nil) return [] if @_element_definitions.blank? - @_existing_element_names = elements_including_fixed.pluck(:name) + existing_elements = all_elements.not_nested.not_trashed + @_existing_element_names = existing_elements.pluck(:name) delete_unique_element_definitions! delete_outnumbered_element_definitions! @@ -192,9 +187,10 @@ def richtext_contents_ids # And if so, it generates them. # def generate_elements - elements_already_on_page = elements_including_fixed.pluck(:name) + existing_elements = all_elements.not_nested.not_trashed + existing_element_names = existing_elements.pluck(:name).uniq definition.fetch('autogenerate', []).each do |element_name| - next if elements_already_on_page.include?(element_name) + next if existing_element_names.include?(element_name) Element.create(page: self, name: element_name) end end diff --git a/spec/controllers/alchemy/admin/trash_controller_spec.rb b/spec/controllers/alchemy/admin/trash_controller_spec.rb index ba66d4d747..cbccab043c 100644 --- a/spec/controllers/alchemy/admin/trash_controller_spec.rb +++ b/spec/controllers/alchemy/admin/trash_controller_spec.rb @@ -44,14 +44,11 @@ module Admin end context "and with an unique element on the page" do - let(:unique) { build_stubbed(:alchemy_element, :unique) } - let(:page) { build_stubbed(:alchemy_page, :public) } + let!(:page) { create(:alchemy_page, :public) } + let!(:unique) { create(:alchemy_element, :unique, page: page) } before do allow(Page).to receive(:find).and_return(page) - allow(page).to receive(:elements_including_fixed) do - double(pluck: [unique.name]) - end end it "unique elements should not be draggable" do diff --git a/spec/models/alchemy/page_spec.rb b/spec/models/alchemy/page_spec.rb index be6ef1701b..da647d65c7 100644 --- a/spec/models/alchemy/page_spec.rb +++ b/spec/models/alchemy/page_spec.rb @@ -558,8 +558,8 @@ module Alchemy before { page.elements << create(:alchemy_element, :fixed) } it "the copy should have source fixed elements" do - expect(subject.elements_including_fixed).not_to be_empty - expect(subject.elements_including_fixed.count).to eq(page.elements_including_fixed.count) + expect(subject.fixed_elements).not_to be_empty + expect(subject.fixed_elements.count).to eq(page.fixed_elements.count) end end @@ -778,7 +778,7 @@ module Alchemy describe '#available_element_definitions' do subject { page.available_element_definitions } - let(:page) { build_stubbed(:alchemy_page, :public) } + let(:page) { create(:alchemy_page, :public) } it "returns all element definitions of available elements" do expect(subject).to be_an(Array) @@ -786,13 +786,7 @@ module Alchemy end context "with unique elements already on page" do - let(:element) { build_stubbed(:alchemy_element, :unique) } - - before do - allow(page).to receive(:elements_including_fixed) do - double(pluck: [element.name]) - end - end + let!(:element) { create(:alchemy_element, :unique, page: page) } it "does not return unique element definitions" do expect(subject.collect { |e| e['name'] }).to include('article') @@ -801,13 +795,15 @@ module Alchemy end context 'limited amount' do - let(:page) { build_stubbed(:alchemy_page, page_layout: 'columns') } - let(:unique_element) do - build_stubbed(:alchemy_element, :unique, name: 'unique_headline') + let(:page) { create(:alchemy_page, page_layout: 'columns') } + + let!(:unique_element) do + create(:alchemy_element, :unique, name: 'unique_headline', page: page) end - let(:element_1) { build_stubbed(:alchemy_element, name: 'column_headline') } - let(:element_2) { build_stubbed(:alchemy_element, name: 'column_headline') } - let(:element_3) { build_stubbed(:alchemy_element, name: 'column_headline') } + + let!(:element_1) { create(:alchemy_element, name: 'column_headline', page: page) } + let!(:element_2) { create(:alchemy_element, name: 'column_headline', page: page) } + let!(:element_3) { create(:alchemy_element, name: 'column_headline', page: page) } before do allow(Element).to receive(:definitions).and_return([ @@ -828,14 +824,6 @@ module Alchemy 'elements' => ['column_headline', 'unique_headline'], 'autogenerate' => ['unique_headline', 'column_headline', 'column_headline', 'column_headline'] }) - allow(page).to receive(:elements_including_fixed) do - double(pluck: [ - unique_element.name, - element_1.name, - element_2.name, - element_3.name - ]) - end end it "should be readable" do From abe3b40cf6ab7c8d2b10378db7d2628c1f3606e3 Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Mon, 17 Jun 2019 21:08:09 +0200 Subject: [PATCH 5/7] Only return visible elements from page element relations We already only return untrashed elements from these relations. It does not make sense to include unvisible elements. Refs #1587 --- app/models/alchemy/page/page_elements.rb | 4 +- spec/models/alchemy/page_spec.rb | 55 ++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/app/models/alchemy/page/page_elements.rb b/app/models/alchemy/page/page_elements.rb index f4e25e261c..1326b9a103 100644 --- a/app/models/alchemy/page/page_elements.rb +++ b/app/models/alchemy/page/page_elements.rb @@ -11,13 +11,13 @@ module Page::PageElements -> { order(:position) }, class_name: 'Alchemy::Element' has_many :elements, - -> { order(:position).not_nested.unfixed.not_trashed }, + -> { order(:position).not_nested.unfixed.available }, class_name: 'Alchemy::Element' has_many :trashed_elements, -> { Element.trashed.order(:position) }, class_name: 'Alchemy::Element' has_many :fixed_elements, - -> { order(:position).fixed.not_trashed }, + -> { order(:position).fixed.available }, class_name: 'Alchemy::Element' has_many :contents, through: :elements has_and_belongs_to_many :to_be_swept_elements, -> { distinct }, diff --git a/spec/models/alchemy/page_spec.rb b/spec/models/alchemy/page_spec.rb index da647d65c7..f841292e6e 100644 --- a/spec/models/alchemy/page_spec.rb +++ b/spec/models/alchemy/page_spec.rb @@ -976,6 +976,61 @@ module Alchemy expect(page.elements).to_not include(nestable_element.nested_elements.first) end end + + context 'with trashed elements' do + let(:trashed_element) { create(:alchemy_element, page: page) } + + before do + trashed_element.trash! + end + + it 'does not contain trashed elements' do + expect(page.elements).to_not include(trashed_element) + end + end + + context 'with hidden elements' do + let(:hidden_element) { create(:alchemy_element, page: page, public: false) } + + it 'does not contain hidden elements' do + expect(page.elements).to_not include(hidden_element) + end + end + end + + describe "#fixed_elements" do + let(:page) { create(:alchemy_page) } + let!(:element_1) { create(:alchemy_element, fixed: true, page: page) } + let!(:element_2) { create(:alchemy_element, fixed: true, page: page) } + let!(:element_3) { create(:alchemy_element, fixed: true, page: page) } + + before do + element_3.move_to_top + end + + it 'returns a ordered active record collection of fixed elements on that page' do + expect(page.fixed_elements).to eq([element_3, element_1, element_2]) + end + + context 'with trashed fixed elements' do + let(:trashed_element) { create(:alchemy_element, page: page, fixed: true) } + + before do + trashed_element.trash! + end + + it 'does not contain trashed fixed elements' do + expect(page.fixed_elements).to_not include(trashed_element) + end + end + + context 'with hidden fixed elements' do + let(:hidden_element) { create(:alchemy_element, page: page, fixed: true, public: false) } + + it 'does not contain hidden fixed elements' do + expect(page.fixed_elements).to_not include(hidden_element) + end + end end describe '#element_definitions' do From 6ce2eb4c5528975302ed4557fd29ec3064c08f11 Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Mon, 17 Jun 2019 21:13:36 +0200 Subject: [PATCH 6/7] Remove redundant element scope from element helper The available scope is the default scope for Pages elements relation. --- app/helpers/alchemy/elements_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/helpers/alchemy/elements_helper.rb b/app/helpers/alchemy/elements_helper.rb index 09067ef4ad..971182237d 100644 --- a/app/helpers/alchemy/elements_helper.rb +++ b/app/helpers/alchemy/elements_helper.rb @@ -51,7 +51,7 @@ module ElementsHelper # # class MyCustomNewsArchive # def elements(page:) - # news_page.elements.available.named('news').order(created_at: :desc) + # news_page.elements.named('news').order(created_at: :desc) # end # # private From ec6a844edca99b72da07cab1cbb8c7658d09c65a Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Mon, 17 Jun 2019 21:14:44 +0200 Subject: [PATCH 7/7] Remove redundant element scope from Page#feed_elements The available scope is now the default scope for Pages elements relation. --- app/models/alchemy/page/page_elements.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/alchemy/page/page_elements.rb b/app/models/alchemy/page/page_elements.rb index 1326b9a103..e14fe047b7 100644 --- a/app/models/alchemy/page/page_elements.rb +++ b/app/models/alchemy/page/page_elements.rb @@ -168,7 +168,7 @@ def element_definitions_by_name(names) # feed_elements: [element_name, element_2_name] # def feed_elements - elements.available.named(definition['feed_elements']) + elements.named(definition['feed_elements']) end # Returns an array of all EssenceRichtext contents ids from not folded elements