From 2c451c8339be22e565823e77947b344c8987ef87 Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Wed, 9 Oct 2019 23:39:01 +0200 Subject: [PATCH 1/7] Fix typo in essence mixin --- lib/alchemy/essence.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/alchemy/essence.rb b/lib/alchemy/essence.rb index 370198965d..96b2d916d2 100644 --- a/lib/alchemy/essence.rb +++ b/lib/alchemy/essence.rb @@ -179,7 +179,7 @@ def ingredient end end - # Returns the value stored from the database column that is configured as ingredient column. + # Sets the value stored in the database column that is configured as ingredient column. def ingredient=(value) if respond_to?(ingredient_setter_method) send(ingredient_setter_method, value) From 18e49bd524c2db99b1f9584663f7bdacd9da0eaa Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Wed, 9 Oct 2019 23:27:35 +0200 Subject: [PATCH 2/7] Ensure a freshly build content has an essence And the default value set correctly on the newly build essence object. --- app/models/alchemy/content.rb | 2 +- app/models/alchemy/content/factory.rb | 31 +++++++++-------- spec/dummy/config/locales/en.yml | 2 ++ spec/models/alchemy/content_spec.rb | 50 ++++++++++++++++++++++----- 4 files changed, 61 insertions(+), 24 deletions(-) diff --git a/app/models/alchemy/content.rb b/app/models/alchemy/content.rb index 0ab8dfd34e..49b0588ce0 100644 --- a/app/models/alchemy/content.rb +++ b/app/models/alchemy/content.rb @@ -255,7 +255,7 @@ def tinymce_class_name # # If the value is a symbol it gets passed through i18n # inside the +alchemy.default_content_texts+ scope - def default_text(default) + def default_value(default = definition[:default]) case default when Symbol Alchemy.t(default, scope: :default_content_texts) diff --git a/app/models/alchemy/content/factory.rb b/app/models/alchemy/content/factory.rb index 9e366035da..f0d6bd151c 100644 --- a/app/models/alchemy/content/factory.rb +++ b/app/models/alchemy/content/factory.rb @@ -22,7 +22,11 @@ def new(attributes = {}) if definition.blank? raise ContentDefinitionError, "No definition found in elements.yml for #{attributes.inspect} and #{element.inspect}" end - super(name: definition['name'], element_id: element.id) + super( + name: definition[:name], + essence_type: normalize_essence_type(definition[:type]), + element_id: element.id + ).tap(&:build_essence) end alias_method :build, :new deprecate build: :new, deprecator: Alchemy::Deprecation @@ -43,7 +47,7 @@ def create(*args) element = attributes[:element] end new(attributes.merge(element: element)).tap do |content| - content.create_essence!(attributes[:essence_type]) + content.essence.save && content.save end end alias_method :create_from_scratch, :create @@ -119,12 +123,22 @@ def definition element.content_definition_for(name) || {} end + # Build essence from definition. + # + # If an optional type is passed, this type of essence gets created. + # + def build_essence(type = essence_type) + self.essence = essence_class(type).new({ + ingredient: default_value + }) + end + # Creates essence from definition. # # If an optional type is passed, this type of essence gets created. # def create_essence!(type = nil) - self.essence = essence_class(type).create!(prepared_attributes_for_essence) + build_essence(type).save! save! end @@ -137,16 +151,5 @@ def create_essence!(type = nil) def essence_class(type = nil) Content.normalize_essence_type(type || definition['type']).constantize end - - # Prepares the attributes for creating the essence. - # - # 1. It sets a default text if given in +elements.yml+ - # - def prepared_attributes_for_essence - attributes = { - ingredient: default_text(definition['default']) - } - attributes - end end end diff --git a/spec/dummy/config/locales/en.yml b/spec/dummy/config/locales/en.yml index 2f449f717c..7da86bea6d 100644 --- a/spec/dummy/config/locales/en.yml +++ b/spec/dummy/config/locales/en.yml @@ -24,6 +24,8 @@ en: essence_richtext: This content type (Essence) represents richtext essence_select: This content type (Essence) represents values the editor can choose from essence_text: This content type (Essence) represents a simple line of text + default_content_texts: + welcome: Welcome to my site activerecord: models: diff --git a/spec/models/alchemy/content_spec.rb b/spec/models/alchemy/content_spec.rb index adc190a7a2..783869a49d 100644 --- a/spec/models/alchemy/content_spec.rb +++ b/spec/models/alchemy/content_spec.rb @@ -155,32 +155,60 @@ module Alchemy end describe '.new' do - let(:element) { build_stubbed(:alchemy_element) } + let(:element) { build(:alchemy_element) } - it "builds a new instance from elements.yml definition" do - expect(Content.new({element: element, name: 'headline'})).to be_instance_of(Content) + subject { Content.new({element: element, name: 'headline'}) } + + it "builds a new content instance from elements.yml definition" do + is_expected.to be_instance_of(Content) + is_expected.to_not be_persisted + end + + it "builds a new essence instance from elements.yml definition" do + expect(subject.essence).to be_instance_of(EssenceText) + expect(subject.essence).to_not be_persisted end end describe '.create' do let(:element) { create(:alchemy_element, name: 'article') } - it "builds the content" do - expect(Content.create(element: element, name: 'headline')).to be_instance_of(Alchemy::Content) + subject(:content) { Content.create(element: element, name: 'headline') } + + it "creates the content" do + is_expected.to be_instance_of(Alchemy::Content) + is_expected.to be_persisted end it "creates the essence" do - expect(Content.create(element: element, name: 'headline').essence).to_not be_nil + expect(subject.essence).to be_instance_of(Alchemy::EssenceText) + expect(subject.essence).to be_persisted end context "with default value present" do it "should have the ingredient column filled with default value." do - allow_any_instance_of(Element).to receive(:content_definition_for) do - {'name' => 'headline', 'type' => 'EssenceText', 'default' => 'Welcome'} + allow_any_instance_of(Content).to receive(:definition) do + { + 'name' => 'headline', + 'type' => 'EssenceText', + 'default' => 'Welcome' + }.with_indifferent_access end - content = Content.create(element: element, name: 'headline') expect(content.ingredient).to eq("Welcome") end + + context 'with default value being a symbol' do + it "passes default value through I18n." do + allow_any_instance_of(Content).to receive(:definition) do + { + 'name' => 'headline', + 'type' => 'EssenceText', + 'default' => :welcome + }.with_indifferent_access + end + expect(content.ingredient).to eq("Welcome to my site") + end + end end end @@ -197,6 +225,10 @@ module Alchemy let(:element) { create(:alchemy_element, name: 'headline') } let(:content) { Alchemy::Content.new(element: element, name: 'headline').tap(&:save) } + before do + expect(content).to receive(:essence) { nil } + end + it "should raise error" do expect { content.ingredient = "Welcome" }.to raise_error(EssenceMissingError) end From 3a048491e92fbfbf21cfc7ce1a81f4dc25c7a98b Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Thu, 10 Oct 2019 12:30:45 +0200 Subject: [PATCH 3/7] Do not read crop settings from params Read the crop and size settings from one canonical source the content settings defined in the elements.yml This reduces complexity and prevents hard to track down errors. --- .../admin/essence_pictures_controller.rb | 18 +++--- .../admin/essence_pictures/crop.html.erb | 2 +- .../admin/essence_pictures_controller_spec.rb | 63 ++++++++++++++----- 3 files changed, 56 insertions(+), 27 deletions(-) diff --git a/app/controllers/alchemy/admin/essence_pictures_controller.rb b/app/controllers/alchemy/admin/essence_pictures_controller.rb index 1f7d47c485..561f77fdf5 100644 --- a/app/controllers/alchemy/admin/essence_pictures_controller.rb +++ b/app/controllers/alchemy/admin/essence_pictures_controller.rb @@ -19,10 +19,8 @@ def edit def crop if @picture = @essence_picture.picture @content = @essence_picture.content - options_from_params[:format] ||= (configuration(:image_store_format) || 'png') - @min_size = sizes_from_essence_or_params - @ratio = ratio_from_size_or_params + @ratio = ratio_from_size_or_settings infer_width_or_height_from_ratio @default_box = @essence_picture.default_mask(@min_size) @@ -70,24 +68,24 @@ def load_content # Gets the minimum size of the image to be rendered. # - # The +render_size+ attribute has preference over the +size+ parameter. + # The +render_size+ attribute has preference over the contents +size+ setting. # def sizes_from_essence_or_params if @essence_picture.render_size? @essence_picture.sizes_from_string(@essence_picture.render_size) - elsif options_from_params[:size] - @essence_picture.sizes_from_string(options_from_params[:size]) + elsif @essence_picture.content.settings[:size] + @essence_picture.sizes_from_string(@essence_picture.content.settings[:size]) else { width: 0, height: 0 } end end - # Infers the aspect ratio from size or parameters. If you don't want a fixed + # Infers the aspect ratio from size or contents settings. If you don't want a fixed # aspect ratio, don't specify a size or only width or height. # - def ratio_from_size_or_params - if @min_size.value?(0) && options_from_params[:fixed_ratio].to_s =~ FLOAT_REGEX - options_from_params[:fixed_ratio].to_f + def ratio_from_size_or_settings + if @min_size.value?(0) && @essence_picture.content.settings[:fixed_ratio].to_s =~ FLOAT_REGEX + @essence_picture.content.settings[:fixed_ratio].to_f elsif !@min_size[:width].zero? && !@min_size[:height].zero? @min_size[:width].to_f / @min_size[:height].to_f else diff --git a/app/views/alchemy/admin/essence_pictures/crop.html.erb b/app/views/alchemy/admin/essence_pictures/crop.html.erb index 3bd8e0cd9a..52bf388974 100644 --- a/app/views/alchemy/admin/essence_pictures/crop.html.erb +++ b/app/views/alchemy/admin/essence_pictures/crop.html.erb @@ -12,7 +12,7 @@ <%= form_for( @essence_picture, - url: alchemy.admin_essence_picture_path(@essence_picture, options: options_from_params), + url: alchemy.admin_essence_picture_path(@essence_picture), id: 'image_cropper_form', remote: true ) do |f| %> diff --git a/spec/controllers/alchemy/admin/essence_pictures_controller_spec.rb b/spec/controllers/alchemy/admin/essence_pictures_controller_spec.rb index c0f7a2e9d6..d6724f41d9 100644 --- a/spec/controllers/alchemy/admin/essence_pictures_controller_spec.rb +++ b/spec/controllers/alchemy/admin/essence_pictures_controller_spec.rb @@ -8,7 +8,7 @@ module Alchemy before { authorize_user(:as_admin) } - let(:essence) { EssencePicture.new } + let(:essence) { EssencePicture.new(content: content, picture: picture) } let(:content) { Content.new } let(:picture) { Picture.new } @@ -51,10 +51,13 @@ module Alchemy } end + let(:settings) { {} } + before do picture.image_file_width = 300 picture.image_file_height = 250 expect(essence).to receive(:picture).at_least(:once).and_return(picture) + allow(content).to receive(:settings) { settings } end context 'with no render_size present in essence' do @@ -62,14 +65,18 @@ module Alchemy expect(essence).to receive(:render_size).at_least(:once).and_return(nil) end - context 'with sizes in params' do + context 'with sizes in content settings' do + let(:settings) do + { size: '300x250' } + end + it "sets sizes to given values" do - get :crop, params: {id: 1, options: {size: '300x250'}} + get :crop, params: {id: 1} expect(assigns(:min_size)).to eq({ width: 300, height: 250 }) end end - context 'with no sizes in params' do + context 'with no sizes in content settngs' do it "sets sizes to zero" do get :crop, params: {id: 1} expect(assigns(:min_size)).to eq({ width: 0, height: 0 }) @@ -93,20 +100,32 @@ module Alchemy expect(assigns(:min_size)).to eq({ width: 30, height: 0}) end - it 'does not infer the height from the image file preserving the aspect ratio' do - expect(essence).to receive(:render_size).at_least(:once).and_return('x25') + context "and aspect ratio set on the contents settings" do + let(:settings) do + { fixed_ratio: "2" } + end - get :crop, params: {id: 1, options: { fixed_ratio: "2"}} - expect(assigns(:min_size)).to eq({ width: 50, height: 25 }) + it 'does not infer the height from the image file preserving the aspect ratio' do + expect(essence).to receive(:render_size).at_least(:once).and_return('x25') + + get :crop, params: {id: 1} + expect(assigns(:min_size)).to eq({ width: 50, height: 25 }) + end end end context 'when width or height is not fixed and an aspect ratio is given' do - it 'width is given, it infers the height from width and ratio' do - expect(essence).to receive(:render_size).at_least(:once).and_return('30x') + context "and aspect ratio set on the contents setting" do + let(:settings) do + { fixed_ratio: "0.5" } + end - get :crop, params: {id: 1, options: { fixed_ratio: "0.5" }} - expect(assigns(:min_size)).to eq({ width: 30, height: 60 }) + it 'width is given, it infers the height from width and ratio' do + expect(essence).to receive(:render_size).at_least(:once).and_return('30x') + + get :crop, params: {id: 1} + expect(assigns(:min_size)).to eq({ width: 30, height: 60 }) + end end it 'infers the height from the image file preserving the aspect ratio' do @@ -148,22 +167,34 @@ module Alchemy end context 'with fixed_ratio set to false' do + let(:settings) do + { fixed_ratio: false } + end + it "sets ratio to false" do - get :crop, params: {id: 1, options: {fixed_ratio: false}} + get :crop, params: {id: 1} expect(assigns(:ratio)).to eq(false) end end context 'with fixed_ratio set to a non float string' do + let(:settings) do + { fixed_ratio: '123,45' } + end + it "doesn't set a fixed ratio" do - get :crop, params: {id: 1, options: {fixed_ratio: '123,45'}} + get :crop, params: {id: 1} expect(assigns(:ratio)).to eq(false) end end - context 'with no fixed_ratio set in params' do + context 'with no fixed_ratio set' do + let(:settings) do + { size: '80x60' } + end + it "sets a fixed ratio from sizes" do - get :crop, params: {id: 1, options: {size: '80x60'}} + get :crop, params: {id: 1} expect(assigns(:ratio)).to eq(80.0 / 60.0) end end From f385992d76f456ee0bc2e0c1b5f360519cdd75fe Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Thu, 10 Oct 2019 17:05:27 +0200 Subject: [PATCH 4/7] Do not accept options in essence_picture_thumbnail anymore Rely only on the content settings for sizing and cropping settings. --- app/helpers/alchemy/admin/essences_helper.rb | 12 +++++----- app/models/alchemy/essence_picture.rb | 16 ++++++------- .../admin/essence_pictures/update.js.erb | 2 +- spec/models/alchemy/essence_picture_spec.rb | 23 ++++++++----------- 4 files changed, 24 insertions(+), 29 deletions(-) diff --git a/app/helpers/alchemy/admin/essences_helper.rb b/app/helpers/alchemy/admin/essences_helper.rb index e3bfa63885..d67c15c7f6 100644 --- a/app/helpers/alchemy/admin/essences_helper.rb +++ b/app/helpers/alchemy/admin/essences_helper.rb @@ -75,14 +75,14 @@ def render_missing_content(element, name, options) deprecate :render_missing_content, deprecator: Alchemy::Deprecation # Renders a thumbnail for given EssencePicture content with correct cropping and size - def essence_picture_thumbnail(content, options = {}) + def essence_picture_thumbnail(content) picture = content.ingredient essence = content.essence return if picture.nil? image_tag( - essence.thumbnail_url(options), + essence.thumbnail_url, alt: picture.name, class: 'img_paddingtop', title: Alchemy.t(:image_name) + ": #{picture.name}" @@ -90,11 +90,11 @@ def essence_picture_thumbnail(content, options = {}) end # Size value for edit picture dialog - def edit_picture_dialog_size(content, options = {}) - if content.settings_value(:caption_as_textarea, options) - content.settings_value(:sizes, options) ? '380x320' : '380x300' + def edit_picture_dialog_size(content) + if content.settings[:caption_as_textarea] + content.settings[:sizes] ? '380x320' : '380x300' else - content.settings_value(:sizes, options) ? '380x290' : '380x255' + content.settings[:sizes] ? '380x290' : '380x255' end end diff --git a/app/models/alchemy/essence_picture.rb b/app/models/alchemy/essence_picture.rb index 611c4fe6c7..098645f18d 100644 --- a/app/models/alchemy/essence_picture.rb +++ b/app/models/alchemy/essence_picture.rb @@ -85,11 +85,11 @@ def picture_url_options # image displayed in the frontend. # # @return [String] - def thumbnail_url(options = {}) + def thumbnail_url return if picture.nil? - crop = crop_values_present? || content.settings_value(:crop, options) - size = render_size || content.settings_value(:size, options) + crop = crop_values_present? || content.settings[:crop] + size = render_size || content.settings[:size] options = { size: thumbnail_size(size, crop), @@ -132,12 +132,12 @@ def serialized_ingredient picture_url(content.settings) end - # Show image cropping link for content and options? - def allow_image_cropping?(options = {}) - content && content.settings_value(:crop, options) && picture && + # Show image cropping link for content + def allow_image_cropping? + content && content.settings[:crop] && picture && picture.can_be_cropped_to( - content.settings_value(:size, options), - content.settings_value(:upsample, options) + content.settings[:size], + content.settings[:upsample] ) end diff --git a/app/views/alchemy/admin/essence_pictures/update.js.erb b/app/views/alchemy/admin/essence_pictures/update.js.erb index 36d5a86d31..ff3081b723 100644 --- a/app/views/alchemy/admin/essence_pictures/update.js.erb +++ b/app/views/alchemy/admin/essence_pictures/update.js.erb @@ -3,7 +3,7 @@ Alchemy.closeCurrentDialog(); Alchemy.setElementDirty('#element_<%= @content.element.id %>'); <% if @content.ingredient %> - $('.thumbnail_background img', '#<%= @content.dom_id %>').replaceWith('<%= essence_picture_thumbnail(@content, options_from_params) %>'); + $('.thumbnail_background img', '#<%= @content.dom_id %>').replaceWith('<%= essence_picture_thumbnail(@content) %>'); Alchemy.ImageLoader('#<%= @content.dom_id %> .thumbnail_background'); <% end %> })(jQuery); diff --git a/spec/models/alchemy/essence_picture_spec.rb b/spec/models/alchemy/essence_picture_spec.rb index cec5801fc5..df557e3684 100644 --- a/spec/models/alchemy/essence_picture_spec.rb +++ b/spec/models/alchemy/essence_picture_spec.rb @@ -149,9 +149,11 @@ module Alchemy end describe '#thumbnail_url' do - subject(:thumbnail_url) { essence.thumbnail_url(options) } + subject(:thumbnail_url) { essence.thumbnail_url } - let(:options) { {} } + let(:settings) do + {} + end let(:picture) do build_stubbed(:alchemy_picture) @@ -166,6 +168,7 @@ module Alchemy end before do + allow(content).to receive(:settings) { settings } allow(essence).to receive(:content) { content } end @@ -200,9 +203,9 @@ module Alchemy thumbnail_url end - context 'when crop is explicitely enabled in the options' do - let(:options) do - {crop: true} + context 'when crop is explicitely enabled in the settings' do + let(:settings) do + { crop: true } end it "it enables cropping." do @@ -214,14 +217,6 @@ module Alchemy end end - context 'with other options' do - let(:options) { {foo: 'baz'} } - - it 'drops them' do - expect(thumbnail_url).to_not match /\?foo=baz/ - end - end - context 'without picture assigned' do let(:picture) { nil } @@ -336,7 +331,7 @@ module Alchemy context "with crop set to true" do before do - allow(content).to receive(:settings_value) { true } + allow(content).to receive(:settings) { {crop: true} } end it { is_expected.to be(true) } From 38ee152992d81a83d3484e4eba0293d6e1e0ca97 Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Thu, 10 Oct 2019 17:22:02 +0200 Subject: [PATCH 5/7] Do not pass options in create missing content request All settings a content might have are set on the contents definition settings key. --- app/controllers/alchemy/admin/contents_controller.rb | 1 - app/views/alchemy/admin/contents/_missing.html.erb | 4 +--- app/views/alchemy/admin/contents/create.js.erb | 4 +--- 3 files changed, 2 insertions(+), 7 deletions(-) diff --git a/app/controllers/alchemy/admin/contents_controller.rb b/app/controllers/alchemy/admin/contents_controller.rb index 2d9ac0246d..d4685c266e 100644 --- a/app/controllers/alchemy/admin/contents_controller.rb +++ b/app/controllers/alchemy/admin/contents_controller.rb @@ -9,7 +9,6 @@ class ContentsController < Alchemy::Admin::BaseController def create @content = Content.create(content_params) - @html_options = params[:html_options] || {} end private diff --git a/app/views/alchemy/admin/contents/_missing.html.erb b/app/views/alchemy/admin/contents/_missing.html.erb index e2ed2ee1a4..37cdad8ab6 100644 --- a/app/views/alchemy/admin/contents/_missing.html.erb +++ b/app/views/alchemy/admin/contents/_missing.html.erb @@ -7,9 +7,7 @@ content: { element_id: element.id, name: name - }, - was_missing: true, - options: options + } ), 'data-create-missing-content' => true, class: 'small button' %> diff --git a/app/views/alchemy/admin/contents/create.js.erb b/app/views/alchemy/admin/contents/create.js.erb index 594cec04f4..47f694f91e 100644 --- a/app/views/alchemy/admin/contents/create.js.erb +++ b/app/views/alchemy/admin/contents/create.js.erb @@ -1,6 +1,4 @@ -var editor_html = '<%= j(render "alchemy/essences/#{@content.essence_partial_name}_editor", { - content: @content, options: options_from_params, html_options: @html_options -}) %>'; +var editor_html = '<%= j render("alchemy/essences/#{@content.essence_partial_name}_editor", content: @content) %>'; $("[data-element-<%= @content.element_id %>-missing-content=\"<%= @content.name %>\"]").replaceWith(editor_html); From 46d968231c9a802371d094be83bc66ed66e48e16 Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Mon, 14 Oct 2019 09:38:49 +0200 Subject: [PATCH 6/7] Do not pass options params around anymore This was only done because we supported to have local options in the element editor and essence editor partials. We do not support this anymore and expect to use static content.settings instead. --- .../alchemy/admin/base_controller.rb | 15 +--------- .../alchemy/admin/resources_controller.rb | 2 -- .../attachments/_archive_overlay.html.erb | 3 +- .../attachments/_file_to_assign.html.erb | 3 +- .../alchemy/admin/essence_files/assign.js.erb | 3 +- .../admin/essence_pictures/assign.js.erb | 3 +- .../admin/essence_pictures/edit.html.erb | 14 ++++----- .../partials/_remote_search_form.html.erb | 2 -- .../pictures/_filter_and_size_bar.html.erb | 6 +--- .../pictures/_overlay_picture_list.html.erb | 2 +- .../pictures/_picture_to_assign.html.erb | 3 +- config/brakeman.ignore | 2 +- .../alchemy/admin/base_controller_spec.rb | 29 ------------------- 13 files changed, 16 insertions(+), 71 deletions(-) diff --git a/app/controllers/alchemy/admin/base_controller.rb b/app/controllers/alchemy/admin/base_controller.rb index 33799f53a4..ccd22f085c 100644 --- a/app/controllers/alchemy/admin/base_controller.rb +++ b/app/controllers/alchemy/admin/base_controller.rb @@ -9,8 +9,7 @@ class BaseController < Alchemy::BaseController before_action { enforce_ssl if ssl_required? && !request.ssl? } before_action :load_locked_pages - helper_method :clipboard_empty?, :trash_empty?, :get_clipboard, :is_admin?, - :options_from_params + helper_method :clipboard_empty?, :trash_empty?, :get_clipboard, :is_admin? check_authorization @@ -128,18 +127,6 @@ def do_redirect_to(url_or_path) end end - # Extracts options from params and permits all keys - # - # If no options are present it returns an empty parameters hash. - # - # @returns [ActionController::Parameters] - def options_from_params - @_options_from_params ||= begin - (params[:options] || ActionController::Parameters.new).permit! - end - end - deprecate :options_from_params, deprecator: Alchemy::Deprecation - # This method decides if we want to raise an exception or not. # # I.e. in test environment. diff --git a/app/controllers/alchemy/admin/resources_controller.rb b/app/controllers/alchemy/admin/resources_controller.rb index 4d886399c8..49a0925ea1 100644 --- a/app/controllers/alchemy/admin/resources_controller.rb +++ b/app/controllers/alchemy/admin/resources_controller.rb @@ -143,8 +143,6 @@ def search_filter_params def common_search_filter_includes [ - # contrary to Rails' documentation passing an empty hash to permit all keys does not work - {options: options_from_params.keys}, {q: [ resource_handler.search_field_name, :s diff --git a/app/views/alchemy/admin/attachments/_archive_overlay.html.erb b/app/views/alchemy/admin/attachments/_archive_overlay.html.erb index f8efa8f3d4..990adc4760 100644 --- a/app/views/alchemy/admin/attachments/_archive_overlay.html.erb +++ b/app/views/alchemy/admin/attachments/_archive_overlay.html.erb @@ -7,8 +7,7 @@ in_dialog: true, redirect_url: admin_attachments_path( element_id: @element.try(:id), - content_id: @content.try(:id), - options: options_from_params + content_id: @content.try(:id) ) %> <% end %> <%= render 'alchemy/admin/partials/remote_search_form' %> diff --git a/app/views/alchemy/admin/attachments/_file_to_assign.html.erb b/app/views/alchemy/admin/attachments/_file_to_assign.html.erb index cbda4a2fc8..3b1d3e501f 100644 --- a/app/views/alchemy/admin/attachments/_file_to_assign.html.erb +++ b/app/views/alchemy/admin/attachments/_file_to_assign.html.erb @@ -1,8 +1,7 @@
  • <%= link_to alchemy.assign_admin_essence_files_path( attachment_id: file_to_assign.id, - content_id: @content.id, - options: options_from_params + content_id: @content.id ), remote: true, method: 'put' do %> diff --git a/app/views/alchemy/admin/essence_files/assign.js.erb b/app/views/alchemy/admin/essence_files/assign.js.erb index fb680e5edc..0498ee39e0 100644 --- a/app/views/alchemy/admin/essence_files/assign.js.erb +++ b/app/views/alchemy/admin/essence_files/assign.js.erb @@ -1,8 +1,7 @@ $('#<%= @content.dom_id %>').replaceWith('<%= j( render "alchemy/essences/essence_file_editor", formats: [:html], - content: @content, - options: options_from_params + content: @content ) %>'); Alchemy.closeCurrentDialog(); Alchemy.setElementDirty($('#element_<%= @content.element.id %>')); diff --git a/app/views/alchemy/admin/essence_pictures/assign.js.erb b/app/views/alchemy/admin/essence_pictures/assign.js.erb index b7634a5056..6ffe1bf4a3 100644 --- a/app/views/alchemy/admin/essence_pictures/assign.js.erb +++ b/app/views/alchemy/admin/essence_pictures/assign.js.erb @@ -2,8 +2,7 @@ $('#picture_to_assign_<%= @picture.id %> a').attr('href', '#').off('click'); $('#<%= @content.dom_id -%>').replaceWith('<%= j( render( "alchemy/essences/essence_picture_editor", - content: @content, - options: options_from_params + content: @content ) ) %>'); Alchemy.closeCurrentDialog(); diff --git a/app/views/alchemy/admin/essence_pictures/edit.html.erb b/app/views/alchemy/admin/essence_pictures/edit.html.erb index 187a89e2d9..c37cde6c5e 100644 --- a/app/views/alchemy/admin/essence_pictures/edit.html.erb +++ b/app/views/alchemy/admin/essence_pictures/edit.html.erb @@ -1,20 +1,20 @@ <%= alchemy_form_for @essence_picture, - url: alchemy.admin_essence_picture_path(@essence_picture, options: options_from_params) do |f| %> + url: alchemy.admin_essence_picture_path(@essence_picture) do |f| %> <%= hidden_field_tag 'content_id', @content.id %> - <%= f.input :caption, as: options_from_params[:caption_as_textarea] ? 'text' : 'string' %> + <%= f.input :caption, as: @content.settings[:caption_as_textarea] ? 'text' : 'string' %> <%= f.input :title %> <%= f.input :alt_tag %> - <%- if options_from_params[:sizes].present? -%> + <%- if @content.settings[:sizes].present? -%> <%= f.input :render_size, collection: [ - [Alchemy.t('Layout default'), options_from_params[:size]] - ] + options_from_params[:sizes].to_a, + [Alchemy.t('Layout default'), @content.settings[:size]] + ] + @content.settings[:sizes].to_a, include_blank: false, input_html: {class: 'alchemy_selectbox'} %> <%- end -%> - <%- if options_from_params[:css_classes].present? -%> + <%- if @content.settings[:css_classes].present? -%> <%= f.input :css_class, - collection: options_from_params[:css_classes], + collection: @content.settings[:css_classes], include_blank: Alchemy.t('None'), input_html: {class: 'alchemy_selectbox'} %> <%- else -%> diff --git a/app/views/alchemy/admin/partials/_remote_search_form.html.erb b/app/views/alchemy/admin/partials/_remote_search_form.html.erb index 2aed629b6c..4df6521bf8 100644 --- a/app/views/alchemy/admin/partials/_remote_search_form.html.erb +++ b/app/views/alchemy/admin/partials/_remote_search_form.html.erb @@ -1,6 +1,5 @@ <%= search_form_for @query, url: url_for( action: 'index', - options: options_from_params, size: @size ), remote: true, html: {class: 'search_form', id: nil} do |f| %> <%= hidden_field_tag("element_id", @element.blank? ? "" : @element.id) %> @@ -17,7 +16,6 @@ action: 'index', element_id: @element.blank? ? '' : @element.id, content_id: @content.blank? ? '' : @content.id, - options: options_from_params, size: @size, overlay: true ), diff --git a/app/views/alchemy/admin/pictures/_filter_and_size_bar.html.erb b/app/views/alchemy/admin/pictures/_filter_and_size_bar.html.erb index 074bbea84e..68a4bc7900 100644 --- a/app/views/alchemy/admin/pictures/_filter_and_size_bar.html.erb +++ b/app/views/alchemy/admin/pictures/_filter_and_size_bar.html.erb @@ -9,8 +9,7 @@ size: search_filter_params[:size], filter: 'last_upload', content_id: @content.try(:id), - element_id: @element.try(:id), - options: options_from_params + element_id: @element.try(:id) ) %>
    <% end %> @@ -23,7 +22,6 @@ content_id: @content, element_id: @element, q: search_filter_params[:q], - options: options_from_params, filter: search_filter_params[:filter], tagged_with: search_filter_params[:tagged_with] }), @@ -40,7 +38,6 @@ content_id: @content, element_id: @element, q: search_filter_params[:q], - options: options_from_params, filter: search_filter_params[:filter], tagged_with: search_filter_params[:tagged_with] }), @@ -57,7 +54,6 @@ content_id: @content, element_id: @element, q: search_filter_params[:q], - options: options_from_params, filter: search_filter_params[:filter], tagged_with: search_filter_params[:tagged_with] }), diff --git a/app/views/alchemy/admin/pictures/_overlay_picture_list.html.erb b/app/views/alchemy/admin/pictures/_overlay_picture_list.html.erb index 0bb2f487c8..9847e059a4 100644 --- a/app/views/alchemy/admin/pictures/_overlay_picture_list.html.erb +++ b/app/views/alchemy/admin/pictures/_overlay_picture_list.html.erb @@ -5,6 +5,6 @@ <% else %> <%= render partial: 'picture_to_assign', collection: @pictures, - locals: {options: options_from_params, size: @size} %> + locals: {size: @size} %> <%= paginate @pictures, theme: 'alchemy', remote: true, hide_per_page_select: true %> <% end %> diff --git a/app/views/alchemy/admin/pictures/_picture_to_assign.html.erb b/app/views/alchemy/admin/pictures/_picture_to_assign.html.erb index 3d6b7c5146..e929ff2dad 100644 --- a/app/views/alchemy/admin/pictures/_picture_to_assign.html.erb +++ b/app/views/alchemy/admin/pictures/_picture_to_assign.html.erb @@ -6,8 +6,7 @@ ), alchemy.assign_admin_essence_pictures_path( picture_id: picture_to_assign.id, - content_id: @content, - options: options + content_id: @content ), remote: true, onclick: '$(self).attr("href", "#").off("click"); return false', diff --git a/config/brakeman.ignore b/config/brakeman.ignore index 3bc13cb74e..52fb12990c 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -29,7 +29,7 @@ "file": "app/views/alchemy/admin/contents/create.js.erb", "line": 1, "link": "http://brakemanscanner.org/docs/warning_types/dynamic_render_path/", - "code": "render(action => \"alchemy/essences/#{Content.create(Element.find(params[:content][:element_id]), content_params).essence_partial_name}_editor\", { :content => Content.create(Element.find(params[:content][:element_id]), content_params), :options => options_from_params, :html_options => ((params[:html_options] or {})) })", + "code": "render(action => \"alchemy/essences/#{Content.create(Element.find(params[:content][:element_id]), content_params).essence_partial_name}_editor\", { :content => Content.create(Element.find(params[:content][:element_id]), content_params) })", "render_path": [{"type":"controller","class":"Alchemy::Admin::ContentsController","method":"create","line":21,"file":"app/controllers/alchemy/admin/contents_controller.rb"}], "location": { "type": "template", diff --git a/spec/controllers/alchemy/admin/base_controller_spec.rb b/spec/controllers/alchemy/admin/base_controller_spec.rb index d7a820cc9a..861319d2e9 100644 --- a/spec/controllers/alchemy/admin/base_controller_spec.rb +++ b/spec/controllers/alchemy/admin/base_controller_spec.rb @@ -3,35 +3,6 @@ require 'rails_helper' describe Alchemy::Admin::BaseController do - describe '#options_from_params' do - subject { controller.send(:options_from_params) } - - before do - expect(controller).to receive(:params).at_least(:once) do - ActionController::Parameters.new(options: options) - end - end - - context "params[:options] are Rails parameters" do - let(:options) do - ActionController::Parameters.new('hello' => 'world') - end - - it "returns the options as permitted parameters with indifferent access" do - expect(subject).to be_permitted - expect(subject[:hello]).to eq('world') - end - end - - context "params[:options] is nil" do - let(:options) { nil } - - it "returns an empty permitted parameters hash" do - is_expected.to eq(ActionController::Parameters.new.permit!) - end - end - end - describe '#raise_exception?' do subject { controller.send(:raise_exception?) } From f0ed7a0ad1b0dc8674d035ecc530929c09bb56a2 Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Thu, 10 Oct 2019 11:17:06 +0200 Subject: [PATCH 7/7] Remove local options from essence editors Passing options as locals to essence editors is not possible anymore. Static values should be set on the content settings on the contents defintion in elements.yml instead. For dynamic values please use a custom essence class. --- app/helpers/alchemy/admin/essences_helper.rb | 32 ++++----------- app/helpers/alchemy/elements_block_helper.rb | 4 +- .../alchemy/admin/essence_files/edit.html.erb | 2 +- .../essences/_essence_boolean_editor.html.erb | 7 +--- .../essences/_essence_date_editor.html.erb | 2 +- .../essences/_essence_file_editor.html.erb | 10 ++--- .../essences/_essence_picture_editor.html.erb | 5 +-- .../essences/_essence_select_editor.html.erb | 13 +++--- .../essences/_essence_text_editor.html.erb | 10 ++--- .../shared/_essence_picture_tools.html.erb | 17 +++----- .../alchemy/essence/templates/editor.html.erb | 1 - .../alchemy/elements_block_helper_spec.rb | 4 +- .../essences/essence_boolean_editor_spec.rb | 30 +++++++++----- .../essences/essence_date_editor_spec.rb | 18 ++++++++ .../essences/essence_picture_editor_spec.rb | 11 +++-- .../essences/essence_select_editor_spec.rb | 35 ++++++++++++++++ .../essences/essence_text_editor_spec.rb | 41 ++++++++++++++----- 17 files changed, 145 insertions(+), 97 deletions(-) create mode 100644 spec/views/essences/essence_date_editor_spec.rb create mode 100644 spec/views/essences/essence_select_editor_spec.rb diff --git a/app/helpers/alchemy/admin/essences_helper.rb b/app/helpers/alchemy/admin/essences_helper.rb index d67c15c7f6..941c05023a 100644 --- a/app/helpers/alchemy/admin/essences_helper.rb +++ b/app/helpers/alchemy/admin/essences_helper.rb @@ -7,22 +7,8 @@ module EssencesHelper include Alchemy::Admin::ContentsHelper # Renders the Content editor partial from the given Content. - # For options see -> render_essence - # @deprecated - def render_essence_editor(content, options = {}, html_options = {}) - if !options.empty? - Alchemy::Deprecation.warn <<~WARN - Passing options to `render_essence_editor` is deprecated and will be removed from Alchemy 5.0. - Add your static `#{options.keys}` options to the `#{content.name}` content definitions `settings` of `#{content.element&.name}` element. - For dynamic options consider adding your own essence class. - WARN - end - if !html_options.empty? - Alchemy::Deprecation.warn <<~WARN - Passing html_options to `render_essence_editor` is deprecated and will be removed in Alchemy 5.0 without replacement. - WARN - end - render_essence(content, :editor, {for_editor: options}, html_options) + def render_essence_editor(content) + render_essence(content, :editor) end deprecate :render_essence_editor, deprecator: Alchemy::Deprecation @@ -32,16 +18,16 @@ def render_essence_editor(content, options = {}, html_options = {}) # Content creation on the fly: # # If you update the elements.yml file after creating an element this helper displays a error message with an option to create the content. - # @deprecated - def render_essence_editor_by_name(element, name, options = {}, html_options = {}) + # + def render_essence_editor_by_name(element, name) if element.blank? return warning('Element is nil', Alchemy.t(:no_element_given)) end content = element.content_by_name(name) if content.nil? - render_missing_content(element, name, options) + render_missing_content(element, name) else - render_essence_editor(content, options, html_options) + render_essence_editor(content) end end deprecate :render_essence_editor_by_name, deprecator: Alchemy::Deprecation @@ -68,9 +54,9 @@ def pages_for_select(pages = nil, selected = nil, prompt = "Choose page", page_a end # Renders the missing content partial - # @deprecated - def render_missing_content(element, name, options) - render 'alchemy/admin/contents/missing', {element: element, name: name, options: options} + # + def render_missing_content(element, name) + render 'alchemy/admin/contents/missing', {element: element, name: name} end deprecate :render_missing_content, deprecator: Alchemy::Deprecation diff --git a/app/helpers/alchemy/elements_block_helper.rb b/app/helpers/alchemy/elements_block_helper.rb index b0df5dde98..eb7636f307 100644 --- a/app/helpers/alchemy/elements_block_helper.rb +++ b/app/helpers/alchemy/elements_block_helper.rb @@ -65,8 +65,8 @@ def essence(name) # Block-level helper class for element editors. # @deprecated class ElementEditorHelper < BlockHelper - def edit(name, *args) - helpers.render_essence_editor_by_name(element, name.to_s, *args) + def edit(name) + helpers.render_essence_editor_by_name(element, name.to_s) end deprecate :edit, deprecator: Alchemy::Deprecation end diff --git a/app/views/alchemy/admin/essence_files/edit.html.erb b/app/views/alchemy/admin/essence_files/edit.html.erb index cd96b70516..25fab7e877 100644 --- a/app/views/alchemy/admin/essence_files/edit.html.erb +++ b/app/views/alchemy/admin/essence_files/edit.html.erb @@ -1,4 +1,4 @@ -<% css_classes = @content.settings_value(:css_classes, local_assigns.fetch(:options, {})) %> +<% css_classes = @content.settings[:css_classes] %> <%= alchemy_form_for [:admin, @essence_file] do |f| %> <%= f.input :link_text %> diff --git a/app/views/alchemy/essences/_essence_boolean_editor.html.erb b/app/views/alchemy/essences/_essence_boolean_editor.html.erb index 0c61ffe25a..e767e6c84f 100644 --- a/app/views/alchemy/essences/_essence_boolean_editor.html.erb +++ b/app/views/alchemy/essences/_essence_boolean_editor.html.erb @@ -1,11 +1,6 @@
    - <%= check_box_tag content.form_field_name, 1, - content.ingredient.presence || content.settings_value( - :default_value, local_assigns.fetch(:options, {}) - ), - class: local_assigns.fetch(:html_options, {})[:class], - style: local_assigns.fetch(:html_options, {})[:style] %> + <%= check_box_tag content.form_field_name, 1, content.ingredient %> diff --git a/app/views/alchemy/essences/_essence_date_editor.html.erb b/app/views/alchemy/essences/_essence_date_editor.html.erb index b32394161f..bb933a3123 100644 --- a/app/views/alchemy/essences/_essence_date_editor.html.erb +++ b/app/views/alchemy/essences/_essence_date_editor.html.erb @@ -4,7 +4,7 @@ content.essence, :date, { name: content.form_field_name, id: content.form_field_id, - value: content.settings_value(:default_value, local_assigns.fetch(:options, {})) + value: content.ingredient } ) %>