Merge branch 'fix-wiki-page-history' into 'master'

Fix broken Wiki Page History

This MR fixes the broken Page History on the Wiki pages. It turns out `WikiHelper` did not allow users to view different versions due to its omitting of query string parameters, which was necessary to specify different `version_id` parameters. Instead of this hacky approach, use manually-specified wildcard routes that match the ID field properly for slashes.

Closes #2104

Closes #1751

Closes #1592

Closes https://github.com/gitlabhq/gitlabhq/issues/9399

See merge request !1232

9 files changed, 39 insertions, 40 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 8154d4333d9..cd3a7b24945 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.0.0 (unreleased)
   - Omit filename in Content-Disposition header in raw file download to avoid RFC 6266 encoding issues (Stan HU)
+  - Fix broken Wiki Page History (Stan Hu)
   - Prevent anchors from being hidden by header (Stan Hu)
   - Fix bug where only the first 15 Bitbucket issues would be imported (Stan Hu)
   - Sort issues by creation date in Bitbucket importer (Stan Hu)
diff --git a/app/controllers/projects/wikis_controller.rb b/app/controllers/projects/wikis_controller.rb
index 50512cb6dc3..51c26a6a465 100644
--- a/app/controllers/projects/wikis_controller.rb
+++ b/app/controllers/projects/wikis_controller.rb
@@ -5,7 +5,6 @@ class Projects::WikisController < Projects::ApplicationController
   before_action :authorize_create_wiki!, only: [:edit, :create, :history]
   before_action :authorize_admin_wiki!, only: :destroy
   before_action :load_project_wiki
-  include WikiHelper
 
   def pages
     @wiki_pages = Kaminari.paginate_array(@project_wiki.pages).page(params[:page]).per(PER_PAGE)
diff --git a/app/helpers/wiki_helper.rb b/app/helpers/wiki_helper.rb
deleted file mode 100644
index f8a96516e61..00000000000
--- a/app/helpers/wiki_helper.rb
+++ /dev/null
@@ -1,24 +0,0 @@
-module WikiHelper
-  # Rails v4.1.9+ escapes all model IDs, converting slashes into %2F. The
-  # only way around this is to implement our own path generators.
-  def namespace_project_wiki_path(namespace, project, wiki_page, *args)
-    slug =
-        case wiki_page
-        when Symbol
-          wiki_page
-        when String
-          wiki_page
-        else
-          wiki_page.slug
-        end
-    namespace_project_path(namespace, project) + "/wikis/#{slug}"
-  end
-
-  def edit_namespace_project_wiki_path(namespace, project, wiki_page, *args)
-    namespace_project_wiki_path(namespace, project, wiki_page) + '/edit'
-  end
-
-  def history_namespace_project_wiki_path(namespace, project, wiki_page, *args)
-    namespace_project_wiki_path(namespace, project, wiki_page) + '/history'
-  end
-end
diff --git a/app/views/projects/wikis/_main_links.html.haml b/app/views/projects/wikis/_main_links.html.haml
index 788bb8cf1e2..acc2c8b2f7f 100644
--- a/app/views/projects/wikis/_main_links.html.haml
+++ b/app/views/projects/wikis/_main_links.html.haml
@@ -1,8 +1,8 @@
 %span.pull-right
   - if (@page && @page.persisted?)
-    = link_to history_namespace_project_wiki_path(@project.namespace, @project, @page), class: "btn btn-grouped" do
+    = link_to namespace_project_wiki_history_path(@project.namespace, @project, @page), class: "btn btn-grouped" do
       Page History
     - if can?(current_user, :create_wiki, @project)
-      = link_to edit_namespace_project_wiki_path(@project.namespace, @project, @page), class: "btn btn-grouped" do
+      = link_to namespace_project_wiki_edit_path(@project.namespace, @project, @page), class: "btn btn-grouped" do
         %i.fa.fa-pencil-square-o
         Edit
diff --git a/app/views/projects/wikis/_nav.html.haml b/app/views/projects/wikis/_nav.html.haml
index 804a1b52dbe..ec87c3c765a 100644
--- a/app/views/projects/wikis/_nav.html.haml
+++ b/app/views/projects/wikis/_nav.html.haml
@@ -3,10 +3,10 @@
     = link_to 'Home', namespace_project_wiki_path(@project.namespace, @project, :home)
 
   = nav_link(path: 'wikis#pages') do
-    = link_to 'Pages', pages_namespace_project_wikis_path(@project.namespace, @project)
+    = link_to 'Pages', namespace_project_wiki_pages_path(@project.namespace, @project)
 
   = nav_link(path: 'wikis#git_access') do
-    = link_to git_access_namespace_project_wikis_path(@project.namespace, @project) do
+    = link_to namespace_project_wikis_git_access_path(@project.namespace, @project) do
       %i.fa.fa-download
       Git Access
 
diff --git a/app/views/projects/wikis/show.html.haml b/app/views/projects/wikis/show.html.haml
index 5c4dd7f91ae..90c4b83cf37 100644
--- a/app/views/projects/wikis/show.html.haml
+++ b/app/views/projects/wikis/show.html.haml
@@ -10,7 +10,7 @@
 - if @page.historical?
   .warning_message
     This is an old version of this page.
-    You can view the #{link_to "most recent version", namespace_project_wiki_path(@project.namespace, @project, @page)} or browse the #{link_to "history", history_namespace_project_wiki_path(@project.namespace, @project, @page)}.
+    You can view the #{link_to "most recent version", namespace_project_wiki_path(@project.namespace, @project, @page)} or browse the #{link_to "history", namespace_project_wiki_history_path(@project.namespace, @project, @page)}.
 
 %hr
 
diff --git a/config/routes.rb b/config/routes.rb
index a98c0f8aa77..25c286b3083 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -406,16 +406,20 @@ Gitlab::Application.routes.draw do
           end
         end
 
-        resources :wikis, only: [:show, :edit, :destroy, :create], constraints: { id: /[a-zA-Z.0-9_\-\/]+/ } do
-          collection do
-            get :pages
-            put ':id' => 'wikis#update'
-            get :git_access
-          end
+        WIKI_SLUG_ID = { id: /[a-zA-Z.0-9_\-\/]+/ } unless defined? WIKI_SLUG_ID
 
-          member do
-            get 'history'
-          end
+        scope do
+          # Order matters to give priority to these matches
+          get '/wikis/git_access', to: 'wikis#git_access'
+          get '/wikis/pages', to: 'wikis#pages', as: 'wiki_pages'
+          post '/wikis', to: 'wikis#create'
+
+          get '/wikis/*id/history', to: 'wikis#history', as: 'wiki_history', constraints: WIKI_SLUG_ID
+          get '/wikis/*id/edit', to: 'wikis#edit', as: 'wiki_edit', constraints: WIKI_SLUG_ID
+
+          get '/wikis/*id', to: 'wikis#show', as: 'wiki', constraints: WIKI_SLUG_ID
+          delete '/wikis/*id', to: 'wikis#destroy', constraints: WIKI_SLUG_ID
+          put '/wikis/*id', to: 'wikis#update', constraints: WIKI_SLUG_ID
         end
 
         resource :repository, only: [:show, :create] do
diff --git a/features/project/wiki.feature b/features/project/wiki.feature
index 2ebfa3c1660..af970ecf2d0 100644
--- a/features/project/wiki.feature
+++ b/features/project/wiki.feature
@@ -91,3 +91,15 @@ Feature: Project Wiki
     And I view the page history of a Wiki page that has a path
     Then I should see a non-escaped path
     And I should see the page history
+
+  @javascript
+  Scenario: View an old page version of a Wiki page
+    Given I create a New page with paths
+    And I click on the "Pages" button
+    And I edit the Wiki page with a path
+    Then I should see a non-escaped path
+    And I should see the Editing page
+    And I change the content
+    Then I click on Page History
+    And I should see the page history
+    And I should see a link with a version ID
diff --git a/features/steps/project/wiki.rb b/features/steps/project/wiki.rb
index eebfaee1ede..02207dbffa6 100644
--- a/features/steps/project/wiki.rb
+++ b/features/steps/project/wiki.rb
@@ -3,7 +3,6 @@ class Spinach::Features::ProjectWiki < Spinach::FeatureSteps
   include SharedProject
   include SharedNote
   include SharedPaths
-  include WikiHelper
 
   step 'I click on the Cancel button' do
     page.within(:css, ".form-actions") do
@@ -165,6 +164,10 @@ class Spinach::Features::ProjectWiki < Spinach::FeatureSteps
     click_on 'Page History'
   end
 
+  step 'I click on Page History' do
+    click_on 'Page History'
+  end
+
   step 'I should see the page history' do
     expect(page).to have_content('History for')
   end
@@ -174,6 +177,10 @@ class Spinach::Features::ProjectWiki < Spinach::FeatureSteps
     click_button "Search"
   end
 
+  step 'I should see a link with a version ID' do
+    find('a[href*="?version_id"]')
+  end
+
   def wiki
     @project_wiki = ProjectWiki.new(project, current_user)
   end