From 6b90ccb9fd57401912e1978cbad28cc693a2e0a1 Mon Sep 17 00:00:00 2001
From: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
Date: Thu, 6 Oct 2016 15:14:24 +0300
Subject: [PATCH] Change user & group landing page routing from /u/:name &
 /groups/:name to /:name

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
---
 CHANGELOG                                     |  1 +
 app/assets/javascripts/user_tabs.js.es6       | 11 ++----
 app/assets/javascripts/users_select.js        |  4 +--
 .../boards/components/_card.html.haml         |  2 +-
 app/views/users/show.html.haml                |  2 +-
 config/routes/group.rb                        |  8 +++++
 config/routes/user.rb                         | 35 ++++++++++++-------
 lib/constraints/group_url_constrainer.rb      |  7 ++++
 lib/constraints/namespace_url_constrainer.rb  | 13 +++++++
 lib/constraints/user_url_constrainer.rb       |  7 ++++
 spec/features/users_spec.rb                   | 22 ++++++++++++
 spec/helpers/projects_helper_spec.rb          |  2 +-
 .../namespace_url_constrainer_spec.rb         | 26 ++++++++++++++
 spec/routing/routing_spec.rb                  |  4 ++-
 spec/services/system_note_service_spec.rb     |  2 +-
 15 files changed, 119 insertions(+), 27 deletions(-)
 create mode 100644 lib/constraints/group_url_constrainer.rb
 create mode 100644 lib/constraints/namespace_url_constrainer.rb
 create mode 100644 lib/constraints/user_url_constrainer.rb
 create mode 100644 spec/lib/constraints/namespace_url_constrainer_spec.rb

diff --git a/CHANGELOG b/CHANGELOG
index 30c8e801ea1..43713d9c9e2 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -82,6 +82,7 @@ v 8.12.4
   - Fix failed project deletion when feature visibility set to private. !6688
   - Prevent claiming associated model IDs via import.
   - Set GitLab project exported file permissions to owner only
+  - Change user & group landing page routing from /u/:username to /:username
 
 v 8.12.3
   - Update Gitlab Shell to support low IO priority for storage moves
diff --git a/app/assets/javascripts/user_tabs.js.es6 b/app/assets/javascripts/user_tabs.js.es6
index 63bce0a6f6f..dfdfa1e7f75 100644
--- a/app/assets/javascripts/user_tabs.js.es6
+++ b/app/assets/javascripts/user_tabs.js.es6
@@ -89,7 +89,7 @@ content on the Users#show page.
       const action = $target.data('action');
       const source = $target.attr('href');
       this.setTab(source, action);
-      return this.setCurrentAction(action);
+      return this.setCurrentAction(source, action);
     }
 
     activateTab(action) {
@@ -142,14 +142,9 @@ content on the Users#show page.
         .toggle(status);
     }
 
-    setCurrentAction(action) {
-      const regExp = new RegExp(`\/(${this.actions.join('|')})(\.html)?\/?$`);
-      let new_state = this._location.pathname;
+    setCurrentAction(source, action) {
+      let new_state = source
       new_state = new_state.replace(/\/+$/, '');
-      new_state = new_state.replace(regExp, '');
-      if (action !== this.defaultAction) {
-        new_state += `/${action}`;
-      }
       new_state += this._location.search + this._location.hash;
       history.replaceState({
         turbolinks: true,
diff --git a/app/assets/javascripts/users_select.js b/app/assets/javascripts/users_select.js
index 05056a73aaf..bcabda3ceb2 100644
--- a/app/assets/javascripts/users_select.js
+++ b/app/assets/javascripts/users_select.js
@@ -71,8 +71,8 @@
               return $collapsedSidebar.html(collapsedAssigneeTemplate(user));
             });
           };
-          collapsedAssigneeTemplate = _.template('<% if( avatar ) { %> <a class="author_link" href="/u/<%- username %>"> <img width="24" class="avatar avatar-inline s24" alt="" src="<%- avatar %>"> </a> <% } else { %> <i class="fa fa-user"></i> <% } %>');
-          assigneeTemplate = _.template('<% if (username) { %> <a class="author_link bold" href="/u/<%- username %>"> <% if( avatar ) { %> <img width="32" class="avatar avatar-inline s32" alt="" src="<%- avatar %>"> <% } %> <span class="author"><%- name %></span> <span class="username"> @<%- username %> </span> </a> <% } else { %> <span class="no-value assign-yourself"> No assignee - <a href="#" class="js-assign-yourself"> assign yourself </a> </span> <% } %>');
+          collapsedAssigneeTemplate = _.template('<% if( avatar ) { %> <a class="author_link" href="/<%- username %>"> <img width="24" class="avatar avatar-inline s24" alt="" src="<%- avatar %>"> </a> <% } else { %> <i class="fa fa-user"></i> <% } %>');
+          assigneeTemplate = _.template('<% if (username) { %> <a class="author_link bold" href="/<%- username %>"> <% if( avatar ) { %> <img width="32" class="avatar avatar-inline s32" alt="" src="<%- avatar %>"> <% } %> <span class="author"><%- name %></span> <span class="username"> @<%- username %> </span> </a> <% } else { %> <span class="no-value assign-yourself"> No assignee - <a href="#" class="js-assign-yourself"> assign yourself </a> </span> <% } %>');
           return $dropdown.glDropdown({
             showMenuAbove: showMenuAbove,
             data: function(term, callback) {
diff --git a/app/views/projects/boards/components/_card.html.haml b/app/views/projects/boards/components/_card.html.haml
index f15c87c8185..d8f16022407 100644
--- a/app/views/projects/boards/components/_card.html.haml
+++ b/app/views/projects/boards/components/_card.html.haml
@@ -26,7 +26,7 @@
         ":title" => "label.description",
         data: { container: 'body' } }
         {{ label.title }}
-      %a.has-tooltip{ ":href" => "'/u/' + issue.assignee.username",
+      %a.has-tooltip{ ":href" => "'/' + issue.assignee.username",
         ":title" => "'Assigned to ' + issue.assignee.name",
         "v-if" => "issue.assignee",
         data: { container: 'body' } }
diff --git a/app/views/users/show.html.haml b/app/views/users/show.html.haml
index 60fc0c0daf6..19759a33add 100644
--- a/app/views/users/show.html.haml
+++ b/app/views/users/show.html.haml
@@ -82,7 +82,7 @@
 
     %ul.nav-links.center.user-profile-nav
       %li.js-activity-tab
-        = link_to user_calendar_activities_path, data: {target: 'div#activity', action: 'activity', toggle: 'tab'} do
+        = link_to user_path, data: {target: 'div#activity', action: 'activity', toggle: 'tab'} do
           Activity
       %li.js-groups-tab
         = link_to user_groups_path, data: {target: 'div#groups', action: 'groups', toggle: 'tab'} do
diff --git a/config/routes/group.rb b/config/routes/group.rb
index 5b3e25d5e3d..47a8a0a53d4 100644
--- a/config/routes/group.rb
+++ b/config/routes/group.rb
@@ -1,3 +1,11 @@
+require 'constraints/group_url_constrainer'
+
+constraints(GroupUrlConstrainer.new) do
+  scope(path: ':id', as: :group, controller: :groups) do
+    get '/', action: :show
+  end
+end
+
 resources :groups, constraints: { id: /[a-zA-Z.0-9_\-]+(?<!\.atom)/ }  do
   member do
     get :issues
diff --git a/config/routes/user.rb b/config/routes/user.rb
index bbb30cedd4d..c287039ba26 100644
--- a/config/routes/user.rb
+++ b/config/routes/user.rb
@@ -1,15 +1,7 @@
-scope(path: 'u/:username',
-      as: :user,
-      constraints: { username: /[a-zA-Z.0-9_\-]+(?<!\.atom)/ },
-      controller: :users) do
-  get :calendar
-  get :calendar_activities
-  get :groups
-  get :projects
-  get :contributed, as: :contributed_projects
-  get :snippets
-  get '/', action: :show
-end
+require 'constraints/user_url_constrainer'
+
+get '/u/:username', to: redirect('/%{username}'),
+                    constraints: { username: /[a-zA-Z.0-9_\-]+(?<!\.atom)/ }
 
 devise_for :users, controllers: { omniauth_callbacks: :omniauth_callbacks,
                                   registrations: :registrations,
@@ -21,3 +13,22 @@ devise_scope :user do
   get '/users/auth/:provider/omniauth_error' => 'omniauth_callbacks#omniauth_error', as: :omniauth_error
   get '/users/almost_there' => 'confirmations#almost_there'
 end
+
+constraints(UserUrlConstrainer.new) do
+  scope(path: ':username', as: :user, controller: :users) do
+    get '/', action: :show
+  end
+end
+
+scope(path: 'u/:username',
+      as: :user,
+      constraints: { username: /[a-zA-Z.0-9_\-]+(?<!\.atom)/ },
+      controller: :users) do
+  get :calendar
+  get :calendar_activities
+  get :groups
+  get :projects
+  get :contributed, as: :contributed_projects
+  get :snippets
+  get '/', to: redirect('/%{username}')
+end
diff --git a/lib/constraints/group_url_constrainer.rb b/lib/constraints/group_url_constrainer.rb
new file mode 100644
index 00000000000..ca39b1961ae
--- /dev/null
+++ b/lib/constraints/group_url_constrainer.rb
@@ -0,0 +1,7 @@
+require 'constraints/namespace_url_constrainer'
+
+class GroupUrlConstrainer < NamespaceUrlConstrainer
+  def find_resource(id)
+    Group.find_by_path(id)
+  end
+end
diff --git a/lib/constraints/namespace_url_constrainer.rb b/lib/constraints/namespace_url_constrainer.rb
new file mode 100644
index 00000000000..23920193743
--- /dev/null
+++ b/lib/constraints/namespace_url_constrainer.rb
@@ -0,0 +1,13 @@
+class NamespaceUrlConstrainer
+  def matches?(request)
+    id = request.path.sub(/\A\/+/, '').split('/').first.sub(/.atom\z/, '')
+
+    if id =~ Gitlab::Regex.namespace_regex
+      find_resource(id)
+    end
+  end
+
+  def find_resource(id)
+    Namespace.find_by_path(id)
+  end
+end
diff --git a/lib/constraints/user_url_constrainer.rb b/lib/constraints/user_url_constrainer.rb
new file mode 100644
index 00000000000..ef38d47d5c6
--- /dev/null
+++ b/lib/constraints/user_url_constrainer.rb
@@ -0,0 +1,7 @@
+require 'constraints/namespace_url_constrainer'
+
+class UserUrlConstrainer < NamespaceUrlConstrainer
+  def find_resource(id)
+    User.find_by_username(id)
+  end
+end
diff --git a/spec/features/users_spec.rb b/spec/features/users_spec.rb
index b5a94fe0383..f2c0aa7784a 100644
--- a/spec/features/users_spec.rb
+++ b/spec/features/users_spec.rb
@@ -40,6 +40,23 @@ feature 'Users', feature: true do
     expect(number_of_errors_on_page(page)).to be(1), 'errors on page:\n #{errors_on_page page}'
   end
 
+  describe 'redirect alias routes' do
+    before { user }
+
+    scenario '/u/user1 redirects to user page' do
+      visit '/u/user1'
+
+      expect_user_show_page
+    end
+
+
+    scenario '/users/user1 redirects to user page' do
+      visit '/users/user1'
+
+      expect_user_show_page
+    end
+  end
+
   def errors_on_page(page)
     page.find('#error_explanation').find('ul').all('li').map{ |item| item.text }.join("\n")
   end
@@ -47,4 +64,9 @@ feature 'Users', feature: true do
   def number_of_errors_on_page(page)
     page.find('#error_explanation').find('ul').all('li').count
   end
+
+  def expect_user_show_page
+    expect(current_path).to eq user_path(user)
+    expect(page).to have_text(user.name)
+  end
 end
diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb
index bcd53440cb4..8113742923b 100644
--- a/spec/helpers/projects_helper_spec.rb
+++ b/spec/helpers/projects_helper_spec.rb
@@ -72,7 +72,7 @@ describe ProjectsHelper do
       it 'returns an HTML link to the user' do
         link = helper.link_to_member(project, user)
 
-        expect(link).to match(%r{/u/#{user.username}})
+        expect(link).to match(%r{/#{user.username}})
       end
     end
   end
diff --git a/spec/lib/constraints/namespace_url_constrainer_spec.rb b/spec/lib/constraints/namespace_url_constrainer_spec.rb
new file mode 100644
index 00000000000..8940fd6b94e
--- /dev/null
+++ b/spec/lib/constraints/namespace_url_constrainer_spec.rb
@@ -0,0 +1,26 @@
+require 'spec_helper'
+
+describe NamespaceUrlConstrainer, lib: true do
+  let!(:group) { create(:group, path: 'gitlab') }
+  subject { NamespaceUrlConstrainer.new }
+
+  describe '#matches?' do
+    context 'existing namespace' do
+      it { expect(subject.matches?(request '/gitlab')).to be_truthy }
+      it { expect(subject.matches?(request '/gitlab.atom')).to be_truthy }
+      it { expect(subject.matches?(request '/gitlab/')).to be_truthy }
+      it { expect(subject.matches?(request '//gitlab/')).to be_truthy }
+    end
+
+    context 'non-existing namespace' do
+      it { expect(subject.matches?(request '/gitlab-ce')).to be_falsey }
+      it { expect(subject.matches?(request '/gitlab.ce')).to be_falsey }
+      it { expect(subject.matches?(request '/g/gitlab')).to be_falsey }
+      it { expect(subject.matches?(request '/.gitlab')).to be_falsey }
+    end
+  end
+
+  def request(path)
+    OpenStruct.new(path: path)
+  end
+end
diff --git a/spec/routing/routing_spec.rb b/spec/routing/routing_spec.rb
index 4bc3cddd9c2..3608aa70f65 100644
--- a/spec/routing/routing_spec.rb
+++ b/spec/routing/routing_spec.rb
@@ -9,7 +9,9 @@ require 'spec_helper'
 # user_calendar_activities   GET    /u/:username/calendar_activities(.:format)
 describe UsersController, "routing" do
   it "to #show" do
-    expect(get("/u/User")).to route_to('users#show', username: 'User')
+    allow(User).to receive(:find_by_username).and_return(true)
+
+    expect(get("/User")).to route_to('users#show', username: 'User')
   end
 
   it "to #groups" do
diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb
index c22dd9ab77a..d1a47ea9b6f 100644
--- a/spec/services/system_note_service_spec.rb
+++ b/spec/services/system_note_service_spec.rb
@@ -561,7 +561,7 @@ describe SystemNoteService, services: true do
 
       describe "existing reference" do
         before do
-          message = %Q{[#{author.name}|http://localhost/u/#{author.username}] mentioned this issue in [a commit of #{project.path_with_namespace}|http://localhost/#{project.path_with_namespace}/commit/#{commit.id}]:\\n'#{commit.title}'}
+          message = %Q{[#{author.name}|http://localhost/#{author.username}] mentioned this issue in [a commit of #{project.path_with_namespace}|http://localhost/#{project.path_with_namespace}/commit/#{commit.id}]:\\n'#{commit.title}'}
           WebMock.stub_request(:get, jira_api_comment_url).to_return(body: %Q({"comments":[{"body":"#{message}"}]}))
         end