diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index b978f7112..a94b6d6d0 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -80,6 +80,10 @@ class ApplicationController < ActionController::Base end end + def redirect_permanently_to(location) + redirect_to location + internal_nav_param, status: :moved_permanently + end + def customize_params params[:signed_in] = user_signed_in?.to_s end @@ -178,4 +182,10 @@ class ApplicationController < ActionController::Base def configure_permitted_parameters devise_parameter_sanitizer.permit(:sign_up, keys: %i[username name profile_image profile_image_url]) end + + def internal_nav_param + return "" unless params[:i] == "i" + + "?i=i" + end end diff --git a/app/controllers/stories_controller.rb b/app/controllers/stories_controller.rb index 99e4f8db6..858a1cf21 100644 --- a/app/controllers/stories_controller.rb +++ b/app/controllers/stories_controller.rb @@ -78,7 +78,7 @@ class StoriesController < ApplicationController user_or_org = User.find_by("old_username = ? OR old_old_username = ?", potential_username, potential_username) || Organization.find_by("old_slug = ? OR old_old_slug = ?", potential_username, potential_username) if user_or_org.present? && !user_or_org.decorate.fully_banished? - redirect_to user_or_org.path, status: :moved_permanently + redirect_permanently_to(user_or_org.path) else not_found end @@ -88,10 +88,10 @@ class StoriesController < ApplicationController potential_username = params[:username].tr("@", "").downcase @user = User.find_by("old_username = ? OR old_old_username = ?", potential_username, potential_username) if @user&.articles&.find_by(slug: params[:slug]) - redirect_to URI.parse("/#{@user.username}/#{params[:slug]}").path, status: :moved_permanently + redirect_permanently_to(URI.parse("/#{@user.username}/#{params[:slug]}").path) return elsif (@organization = @article.organization) - redirect_to URI.parse("/#{@organization.slug}/#{params[:slug]}").path, status: :moved_permanently + redirect_permanently_to(URI.parse("/#{@organization.slug}/#{params[:slug]}").path) return end not_found @@ -126,7 +126,7 @@ class StoriesController < ApplicationController @tag_model = Tag.find_by(name: @tag) || not_found @moderators = User.with_role(:tag_moderator, @tag_model).select(:username, :profile_image, :id) if @tag_model.alias_for.present? - redirect_to "/t/#{@tag_model.alias_for}", status: :moved_permanently + redirect_permanently_to("/t/#{@tag_model.alias_for}") return end @@ -369,7 +369,7 @@ class StoriesController < ApplicationController def redirect_to_lowercase_username return unless params[:username] && params[:username]&.match?(/[[:upper:]]/) - redirect_to "/#{params[:username].downcase}", status: :moved_permanently + redirect_permanently_to("/#{params[:username].downcase}") end def set_user_json_ld diff --git a/docs/technical-overview/architecture.md b/docs/technical-overview/architecture.md index 7ba6a5dcd..75e6b79c1 100644 --- a/docs/technical-overview/architecture.md +++ b/docs/technical-overview/architecture.md @@ -105,6 +105,20 @@ differently than expected. Abstracting and removing these caveats is a long term goal, and contribution on that front is welcome! +We use the parameter `i=i` (i for internal) to indicate to the backend that we only +want the "internal" version of the page (the one without the top nav and footer, etc.) + +## URLS and constraints + +Because we use the top directory for user-generated pages, we need to be aware of +some constraints. `some-forem.com/sophia` could be a user, a page, an organization, +or a previously banished user. We allow users to retain two redirects and should +use `:moved_permanently` when a user changes their username. + +Because we may silently insert `?i=i` on the frontend to indicate internal nav, we +need to maintain that parameter if we are redirecting. We use the method +`redirect_permanently_to(location)` to encompass all of this behavior. + # General app concepts ## Articles (or posts) diff --git a/spec/requests/stories_show_spec.rb b/spec/requests/stories_show_spec.rb index 8dbcb82f9..da1ecaf7c 100644 --- a/spec/requests/stories_show_spec.rb +++ b/spec/requests/stories_show_spec.rb @@ -19,6 +19,31 @@ RSpec.describe "StoriesShow", type: :request do expect(response).to have_http_status(:moved_permanently) end + it "preserves internal nav param (i=i) upon redirect" do + old_path = article.path + article.update(organization: org) + get old_path + "?i=i" + expect(response.body).to redirect_to "#{article.path}?i=i" + expect(response).to have_http_status(:moved_permanently) + end + + it "does not have ?i=i on redirects which did not originally include it" do + old_path = article.path + article.update(organization: org) + get old_path + expect(response.body).not_to redirect_to "#{article.path}?i=i" + expect(response).to have_http_status(:moved_permanently) + end + + it "does not have ?i=i on redirects without that precise param" do + old_path = article.path + article.update(organization: org) + get old_path + "?i=j" + expect(response.body).to redirect_to article.path + expect(response.body).not_to redirect_to "#{article.path}?i=j" + expect(response.body).not_to redirect_to "#{article.path}?i=j" + end + ## Title tag it "renders signed-in title tag for signed-in user" do allow(SiteConfig).to receive(:community_emoji).and_return("🌱")