Resolve discussions

8 files changed, 31 insertions, 25 deletions

diff --git a/app/controllers/concerns/routable_actions.rb b/app/controllers/concerns/routable_actions.rb
index ba236fa5459..d4ab6782444 100644
--- a/app/controllers/concerns/routable_actions.rb
+++ b/app/controllers/concerns/routable_actions.rb
@@ -1,10 +1,10 @@
 module RoutableActions
   extend ActiveSupport::Concern
 
-  def find_routable!(routable_klass, requested_full_path, extra_authorization_method: nil)
+  def find_routable!(routable_klass, requested_full_path, extra_authorization_proc: nil)
     routable = routable_klass.find_by_full_path(requested_full_path, follow_redirects: request.get?)
 
-    if authorized?(routable_klass, routable, extra_authorization_method)
+    if routable_authorized?(routable_klass, routable, extra_authorization_proc)
       ensure_canonical_path(routable, requested_full_path)
       routable
     else
@@ -13,12 +13,12 @@ module RoutableActions
     end
   end
 
-  def authorized?(routable_klass, routable, extra_authorization_method)
+  def routable_authorized?(routable_klass, routable, extra_authorization_proc)
     action = :"read_#{routable_klass.to_s.underscore}"
     return false unless can?(current_user, action, routable)
 
-    if extra_authorization_method
-      send(extra_authorization_method, routable)
+    if extra_authorization_proc
+      extra_authorization_proc.call(routable)
     else
       true
     end
@@ -27,9 +27,11 @@ module RoutableActions
   def ensure_canonical_path(routable, requested_path)
     return unless request.get?
 
-    canonical_path = routable.try(:full_path) || routable.namespace.full_path
+    canonical_path = routable.full_path
     if canonical_path != requested_path
-      flash[:notice] = 'This project has moved to this location. Please update your links and bookmarks.'
+      if canonical_path.casecmp(requested_path) != 0
+        flash[:notice] = "Project '#{requested_path}' was moved to '#{canonical_path}'. Please update any links and bookmarks that may still have the old path."
+      end
       redirect_to request.original_url.sub(requested_path, canonical_path)
     end
   end
diff --git a/app/controllers/groups/application_controller.rb b/app/controllers/groups/application_controller.rb
index 2157a56dea2..afffb813b44 100644
--- a/app/controllers/groups/application_controller.rb
+++ b/app/controllers/groups/application_controller.rb
@@ -9,11 +9,7 @@ class Groups::ApplicationController < ApplicationController
   private
 
   def group
-    @group ||= find_routable!(Group, requested_full_path)
-  end
-
-  def requested_full_path
-    params[:group_id] || params[:id]
+    @group ||= find_routable!(Group, params[:group_id] || params[:id])
   end
 
   def group_projects
diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb
index 2301e1cca77..25232fc9457 100644
--- a/app/controllers/projects/application_controller.rb
+++ b/app/controllers/projects/application_controller.rb
@@ -17,24 +17,17 @@ class Projects::ApplicationController < ApplicationController
     # to
     #   localhost/group/project
     #
-    if params[:format] == 'git'
-      redirect_to request.original_url.gsub(/\.git\/?\Z/, '')
-      return
-    end
+    redirect_to url_for(params.merge(format: nil)) if params[:format] == 'git'
   end
 
   def project
-    @project ||= find_routable!(Project, requested_full_path, extra_authorization_method: :project_not_being_deleted?)
-  end
-
-  def requested_full_path
-    namespace = params[:namespace_id]
-    id = params[:project_id] || params[:id]
-    "#{namespace}/#{id}"
+    @project ||= find_routable!(Project,
+      File.join(params[:namespace_id], params[:project_id] || params[:id]),
+      extra_authorization_proc: project_not_being_deleted?)
   end
 
-  def project_not_being_deleted?(project)
-    !project.pending_delete?
+  def project_not_being_deleted?
+    ->(project) { !project.pending_delete? }
   end
 
   def repository
diff --git a/app/models/user.rb b/app/models/user.rb
index 7da92d03427..dd2c8f1b6ef 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -360,6 +360,10 @@ class User < ActiveRecord::Base
     end
   end
 
+  def full_path
+    username
+  end
+
   def self.internal_attributes
     [:ghost]
   end
diff --git a/changelogs/unreleased/17361-redirect-renamed-paths.yml b/changelogs/unreleased/17361-redirect-renamed-paths.yml
new file mode 100644
index 00000000000..7a33c9fb3ec
--- /dev/null
+++ b/changelogs/unreleased/17361-redirect-renamed-paths.yml
@@ -0,0 +1,4 @@
+---
+title: Redirect old links after renaming a user/group/project.
+merge_request: 10370
+author:
diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb
index b5fd611747a..073b87a1cb4 100644
--- a/spec/controllers/groups_controller_spec.rb
+++ b/spec/controllers/groups_controller_spec.rb
@@ -55,6 +55,7 @@ describe GroupsController do
         get :issues, id: group.to_param.upcase
 
         expect(response).to redirect_to(issues_group_path(group.to_param))
+        expect(controller).not_to set_flash[:notice]
       end
     end
 
@@ -99,6 +100,7 @@ describe GroupsController do
         get :merge_requests, id: group.to_param.upcase
 
         expect(response).to redirect_to(merge_requests_group_path(group.to_param))
+        expect(controller).not_to set_flash[:notice]
       end
     end
 
diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb
index 5e3e943c124..e46ef447df2 100644
--- a/spec/controllers/projects_controller_spec.rb
+++ b/spec/controllers/projects_controller_spec.rb
@@ -185,6 +185,7 @@ describe ProjectsController do
 
           expect(assigns(:project)).to eq(public_project)
           expect(response).to redirect_to("/#{public_project.full_path}")
+          expect(controller).not_to set_flash[:notice]
         end
       end
     end
diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb
index 9b6b9358a40..74c5aa44ba9 100644
--- a/spec/controllers/users_controller_spec.rb
+++ b/spec/controllers/users_controller_spec.rb
@@ -71,6 +71,7 @@ describe UsersController do
           get :show, username: user.username.downcase
 
           expect(response).to redirect_to(user)
+          expect(controller).not_to set_flash[:notice]
         end
       end
     end
@@ -149,6 +150,7 @@ describe UsersController do
           get :calendar, username: user.username.downcase
 
           expect(response).to redirect_to(user_calendar_path(user))
+          expect(controller).not_to set_flash[:notice]
         end
       end
     end
@@ -202,6 +204,7 @@ describe UsersController do
           get :calendar_activities, username: user.username.downcase
 
           expect(response).to redirect_to(user_calendar_activities_path(user))
+          expect(controller).not_to set_flash[:notice]
         end
       end
     end
@@ -255,6 +258,7 @@ describe UsersController do
           get :snippets, username: user.username.downcase
 
           expect(response).to redirect_to(user_snippets_path(user))
+          expect(controller).not_to set_flash[:notice]
         end
       end
     end