Preserve internal navigation indicator on redirect (#11476)

* Preserve internal navigation indicator on redirect

* Fix test

* Refactor for cleaner reusability

* Add docs

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

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

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)

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("🌱")