diff --git a/app/controllers/alchemy/pages_controller.rb b/app/controllers/alchemy/pages_controller.rb index 674ff91ed9..a0d0bedf24 100644 --- a/app/controllers/alchemy/pages_controller.rb +++ b/app/controllers/alchemy/pages_controller.rb @@ -13,7 +13,10 @@ class PagesController < Alchemy::BaseController # Redirecting concerns. Order is important here! include SiteRedirects - include LocaleRedirects + + before_action :enforce_no_locale, + if: :locale_prefix_not_allowed?, + only: [:index, :show] before_action :load_index_page, only: [:index] before_action :load_page, only: [:show] @@ -21,11 +24,13 @@ class PagesController < Alchemy::BaseController # Legacy page redirects need to run after the page was loaded and before we render 404. include LegacyPageRedirects - # From here on, we need a +@page+ to work with! - before_action :page_not_found!, if: -> { @page.blank? }, only: [:index, :show] + # From here on, we need a published +@page+ to work with! + before_action :page_not_found!, unless: -> { @page&.public? }, only: [:index, :show] - # Page redirects need to run after the page was loaded and we're sure to have a +@page+ set. - include PageRedirects + # Page redirects need to run after the page was loaded and we're sure to have a public +@page+ set. + before_action :enforce_locale, + if: :locale_prefix_missing?, + only: [:index, :show] # We only need to set the +@root_page+ if we are sure that no more redirects happen. before_action :set_root_page, only: [:index, :show] @@ -66,12 +71,8 @@ def index # descendant it finds. If no public page can be found it renders a 404 error. # def show - if redirect_url.present? - redirect_permanently_to redirect_url - else - authorize! :show, @page - render_page if render_fresh_page? - end + authorize! :show, @page + render_page if render_fresh_page? end # Renders a search engine compatible xml sitemap. @@ -84,13 +85,25 @@ def sitemap private + # Redirects to requested action without locale prefixed + def enforce_no_locale + redirect_permanently_to additional_params.merge(locale: nil) + end + + # Is the requested locale allowed? + # + # If Alchemy is not in multi language mode or the requested locale is the default locale, + # then we want to redirect to a non prefixed url. + # + def locale_prefix_not_allowed? + params[:locale].present? && !multi_language? || + params[:locale].presence == ::I18n.default_locale.to_s + end + # == Loads index page # # Loads the current public language root page. # - # If the root page is not public it redirects to the first published child. - # This can be configured via +redirect_to_public_child+ [default: true] - # # If no index page and no admin users are present we show the "Welcome to Alchemy" page. # def load_index_page @@ -116,6 +129,28 @@ def load_page ) end + def enforce_locale + redirect_permanently_to page_locale_redirect_url(locale: Language.current.code) + end + + def locale_prefix_missing? + multi_language? && params[:locale].blank? && !default_locale? + end + + def default_locale? + Language.current.code.to_sym == ::I18n.default_locale.to_sym + end + + # Page url with or without locale while keeping all additional params + def page_locale_redirect_url(options = {}) + options = { + locale: prefix_locale? ? @page.language_code : nil, + urlname: @page.urlname, + }.merge(options) + + alchemy.show_page_path additional_params.merge(options) + end + # Redirects to given url with 301 status def redirect_permanently_to(url) redirect_to url, status: :moved_permanently diff --git a/app/controllers/concerns/alchemy/locale_redirects.rb b/app/controllers/concerns/alchemy/locale_redirects.rb deleted file mode 100644 index df8eb23419..0000000000 --- a/app/controllers/concerns/alchemy/locale_redirects.rb +++ /dev/null @@ -1,40 +0,0 @@ -# frozen_string_literal: true - -module Alchemy - # Handles locale redirects - # - # If the current URL has a locale prefix, but should not have one it redirects - # to url without locale prefix. - # - # Situations we don't want a locale prefix: - # - # 1. If only one language is published - # 2. If the requested locale is the current default locale - # - module LocaleRedirects - extend ActiveSupport::Concern - - included do - before_action :enforce_no_locale, - if: :locale_prefix_not_allowed?, - only: [:index, :show] - end - - private - - # Redirects to requested action without locale prefixed - def enforce_no_locale - redirect_permanently_to additional_params.merge(locale: nil) - end - - # Is the requested locale allowed? - # - # If Alchemy is not in multi language mode or the requested locale is the default locale, - # then we want to redirect to a non prefixed url. - # - def locale_prefix_not_allowed? - params[:locale].present? && !multi_language? || - params[:locale].presence == ::I18n.default_locale.to_s - end - end -end diff --git a/app/controllers/concerns/alchemy/page_redirects.rb b/app/controllers/concerns/alchemy/page_redirects.rb deleted file mode 100644 index b39d766670..0000000000 --- a/app/controllers/concerns/alchemy/page_redirects.rb +++ /dev/null @@ -1,68 +0,0 @@ -# frozen_string_literal: true - -module Alchemy - # Handles page redirect urls - # - # Lots of reasons exist to redirect to another URL than the requested one. - # These module holds the logic behind these needs. - # - module PageRedirects - extend ActiveSupport::Concern - - private - - # Returns an URL to redirect the request to. - # - # == Lookup: - # - # 1. If the page is not published and we have a published child, - # we return the url top that page. (Configurable through +redirect_to_public_child+). - # 2. If the page layout of the page found has a controller and action configured, - # we return the url to that route. (Configure controller and action in `page_layouts.yml`). - # 3. If the current page URL has no locale prefixed, but we should have one, - # we return the prefixed URL. - # 4. If no redirection is needed returns nil. - # - # @return String - # @return NilClass - # - def redirect_url - @_redirect_url ||= public_child_redirect_url || locale_prefixed_url || nil - end - - def locale_prefixed_url - return unless locale_prefix_missing? - - page_redirect_url(locale: Language.current.code) - end - - def public_child_redirect_url - return if @page.public? - - if configuration(:redirect_to_public_child) - @page = @page.descendants.published.not_restricted.first - @page ? page_redirect_url : page_not_found! - else - page_not_found! - end - end - - # Page url with or without locale while keeping all additional params - def page_redirect_url(options = {}) - options = { - locale: prefix_locale? ? @page.language_code : nil, - urlname: @page.urlname, - }.merge(options) - - alchemy.show_page_path additional_params.merge(options) - end - - def default_locale? - Language.current.code.to_sym == ::I18n.default_locale.to_sym - end - - def locale_prefix_missing? - multi_language? && params[:locale].blank? && !default_locale? - end - end -end diff --git a/config/alchemy/config.yml b/config/alchemy/config.yml index 5c063790bf..a62397ebcd 100644 --- a/config/alchemy/config.yml +++ b/config/alchemy/config.yml @@ -9,12 +9,6 @@ # auto_logout_time: 30 -# === Redirect Options -# -# redirect_to_public_child [Boolean] # Alchemy redirects to the first public child page found, if a page is not public. -# -redirect_to_public_child: true - # === Page caching # # Enable/Disable page caching globally. diff --git a/lib/alchemy/config.rb b/lib/alchemy/config.rb index 1046937c50..242807b995 100644 --- a/lib/alchemy/config.rb +++ b/lib/alchemy/config.rb @@ -31,9 +31,7 @@ def show # a value of nil means there is no new default # any not nil value is the new default def deprecated_configs - { - redirect_to_public_child: nil, - } + {} end private diff --git a/spec/controllers/alchemy/pages_controller_spec.rb b/spec/controllers/alchemy/pages_controller_spec.rb index 1a01297168..c17bccf1ed 100644 --- a/spec/controllers/alchemy/pages_controller_spec.rb +++ b/spec/controllers/alchemy/pages_controller_spec.rb @@ -62,76 +62,24 @@ module Alchemy default_language_root.update!(public_on: nil) end - context "and redirect_to_public_child is set to false" do - before do - stub_alchemy_config(:redirect_to_public_child, false) - end - - it "raises routing error (404)" do - expect { - get :index - }.to raise_error(ActionController::RoutingError) - end - - context "when a page layout callback is set" do - before do - ApplicationController.extend Alchemy::OnPageLayout - ApplicationController.class_eval do - on_page_layout("index") { "do something" } - end - end - - it 'raises routing error (404) and no "undefined method for nil" error' do - expect { - get :index - }.to raise_error(ActionController::RoutingError) - end - end + it "raises routing error (404)" do + expect { + get :index + }.to raise_error(ActionController::RoutingError) end - context "and redirect_to_public_child is set to true" do + context "when a page layout callback is set" do before do - stub_alchemy_config(:redirect_to_public_child, true) - end - - context "that has a public child" do - let!(:public_child) do - create(:alchemy_page, :public, parent: default_language_root) - end - - it "loads this page" do - get :index - expect(assigns(:page)).to eq(public_child) + ApplicationController.extend Alchemy::OnPageLayout + ApplicationController.class_eval do + on_page_layout("index") { "do something" } end end - context "that has a non public child" do - let!(:non_public_child) do - create(:alchemy_page, parent: default_language_root) - end - - context "that has a public child" do - let!(:public_child) do - create(:alchemy_page, :public, parent: non_public_child) - end - - it "loads this page" do - get :index - expect(assigns(:page)).to eq(public_child) - end - end - - context "that has a non public child" do - before do - create(:alchemy_page, parent: non_public_child) - end - - it "raises routing error (404)" do - expect { - get :index - }.to raise_error(ActionController::RoutingError) - end - end + it 'raises routing error (404) and no "undefined method for nil" error' do + expect { + get :index + }.to raise_error(ActionController::RoutingError) end end end diff --git a/spec/features/page_redirects_spec.rb b/spec/features/page_redirects_spec.rb index 4f679332e3..fd7c1bdaf8 100644 --- a/spec/features/page_redirects_spec.rb +++ b/spec/features/page_redirects_spec.rb @@ -13,10 +13,6 @@ create(:alchemy_page, :public, name: "Page 1") end - let(:public_child) do - create(:alchemy_page, :public, name: "Public Child", parent_id: public_page.id) - end - context "in multi language mode" do let(:second_page) { create(:alchemy_page, :public, name: "Second Page") } @@ -92,74 +88,12 @@ name: "Not Public", urlname: "", ) - public_child - end - - it "redirects to public child" do - visit "/not-public" - expect(page.current_path).to eq("/not-public/public-child") - end - - context "with only unpublished pages in page tree" do - before do - public_child.update(public_on: nil) - end - - it "should raise not found error" do - expect { - visit "/not-public" - }.to raise_error(ActionController::RoutingError) - end end - context "if page locale is the default locale" do - it "redirects to public child with prefixed locale" do - allow(::I18n).to receive(:default_locale).and_return(:de) + it "should raise not found error" do + expect { visit "/not-public" - expect(page.current_path).to eq("/en/not-public/public-child") - end - end - end - - context "if requested url is the index url" do - context "and redirect_to_public_child is enabled" do - before do - allow(Alchemy::Config).to receive(:get) do |arg| - arg == :redirect_to_public_child ? true : Alchemy::Config.parameter(arg) - end - end - - context "if index page is unpublished" do - let!(:public_child) do - create(:alchemy_page, :public, name: "Public Child", parent_id: default_language_root.id) - end - - before do - default_language_root.update( - public_on: nil, - name: "Not Public", - urlname: "", - ) - end - - context "and index page locale is default locale" do - it "redirects to public child without prefixed locale" do - visit "/" - expect(page.current_path).to eq("/public-child") - end - end - - context "and page locale is not default locale" do - before do - allow(::I18n).to receive(:default_locale).and_return(:de) - end - - it "redirects to public child with prefixed locale" do - visit "/" - expect(page.current_path).to eq("/en/public-child") - end - end - end + }.to raise_error(ActionController::RoutingError) end end @@ -240,27 +174,6 @@ expect(page.current_path).to eq("/#{public_page.urlname}") end - context "redirects to public child" do - before do - public_page.update( - public_on: nil, - name: "Not Public", - urlname: "", - ) - public_child - end - - it "if requested page is unpublished" do - visit "/not-public" - expect(page.current_path).to eq("/not-public/public-child") - end - - it "with normal url, if requested url has nested language code and is not public" do - visit "/en/not-public" - expect(page.current_path).to eq("/not-public/public-child") - end - end - context "if requested url is index url" do context "when locale is prefixed" do it "redirects to normal url" do