Merge branch 'ee-fj-37273-moving-wiki-pages-from-the-ui' into 'master'

Port from CE https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/16313

See merge request gitlab-org/gitlab-ee!4377

Gemfile

@@ -72,6 +72,10 @@ gem 'net-ldap'
 
 # Git Wiki
 # Required manually in config/initializers/gollum.rb to control load order
+# Before updating this gem, check if
+# https://github.com/gollum/gollum-lib/pull/292 has been merged.
+# If it has, then remove the monkey patch for update_page, rename_page and raw_data_in_committer
+# in config/initializers/gollum.rb
 gem 'gollum-lib', '~> 4.2', require: false
 
 # Before updating this gem, check if

app/assets/stylesheets/pages/wiki.scss

@@ -6,6 +6,14 @@
   }
 }
 
+.wiki-form {
+  .edit-wiki-page-slug-tip {
+    display: inline-block;
+    max-width: 100%;
+    margin-top: 5px;
+  }
+}
+
 .title .edit-wiki-header {
   width: 780px;
   margin-left: auto;

app/controllers/projects/wikis_controller.rb

@@ -54,8 +54,8 @@ class Projects::WikisController < Projects::ApplicationController
     else
       render 'edit'
     end
-  rescue WikiPage::PageChangedError
-    @conflict = true
+  rescue WikiPage::PageChangedError, WikiPage::PageRenameError => e
+    @error = e
     render 'edit'
   end
 

app/helpers/wiki_helper.rb

@@ -21,4 +21,22 @@ module WikiHelper
         add_to_breadcrumb_dropdown link_to(WikiPage.unhyphenize(dir_or_page).capitalize, project_wiki_path(@project, current_slug)), location: :after
       end
   end
+
+  def wiki_page_errors(error)
+    return unless error
+
+    content_tag(:div, class: 'alert alert-danger') do
+      case error
+      when WikiPage::PageChangedError
+        page_link = link_to s_("WikiPageConflictMessage|the page"), project_wiki_path(@project, @page), target: "_blank"
+        concat(
+          (s_("WikiPageConflictMessage|Someone edited the page the same time you did. Please check out %{page_link} and make sure your changes will not unintentionally remove theirs.") % { page_link: page_link }).html_safe
+        )
+      when WikiPage::PageRenameError
+        s_("WikiEdit|There is already a page with the same title in that path.")
+      else
+        error.message
+      end
+    end
+  end
 end

app/models/project_wiki.rb

@@ -131,6 +131,8 @@ class ProjectWiki
   end
 
   def delete_page(page, message = nil)
+    return unless page
+
     wiki.delete_page(page.path, commit_details(:deleted, message, page.title))
 
     update_elastic_index
@@ -145,6 +147,8 @@ class ProjectWiki
   end
 
   def page_title_and_dir(title)
+    return unless title
+
     title_array = title.split("/")
     title = title_array.pop
     [title, title_array.join("/")]

app/models/wiki_page.rb

 class WikiPage
   PageChangedError = Class.new(StandardError)
+  PageRenameError = Class.new(StandardError)
 
   include ActiveModel::Validations
   include ActiveModel::Conversion
@@ -102,7 +103,7 @@ class WikiPage
 
   # The hierarchy of the directory this page is contained in.
   def directory
-    wiki.page_title_and_dir(slug).last
+    wiki.page_title_and_dir(slug)&.last.to_s
   end
 
   # The processed/formatted content of this page.
@@ -177,7 +178,7 @@ class WikiPage
   # Creates a new Wiki Page.
   #
   # attr - Hash of attributes to set on the new page.
-  #       :title   - The title for the new page.
+  #       :title   - The title (optionally including dir) for the new page.
   #       :content - The raw markup content.
   #       :format  - Optional symbol representing the
   #                  content format. Can be any type
@@ -189,7 +190,7 @@ class WikiPage
   # Returns the String SHA1 of the newly created page
   # or False if the save was unsuccessful.
   def create(attrs = {})
-    @attributes.merge!(attrs)
+    update_attributes(attrs)
 
     save(page_details: title) do
       wiki.create_page(title, content, format, message)
@@ -204,24 +205,29 @@ class WikiPage
   #                           See ProjectWiki::MARKUPS Hash for available formats.
   #        :message         - Optional commit message to set on the new version.
   #        :last_commit_sha - Optional last commit sha to validate the page unchanged.
-  #        :title           - The Title to replace existing title
+  #        :title           - The Title (optionally including dir) to replace existing title
   #
   # Returns the String SHA1 of the newly created page
   # or False if the save was unsuccessful.
   def update(attrs = {})
     last_commit_sha = attrs.delete(:last_commit_sha)
+
     if last_commit_sha && last_commit_sha != self.last_commit_sha
-      raise PageChangedError.new("You are attempting to update a page that has changed since you started editing it.")
+      raise PageChangedError.new("WikiEdit|You are attempting to update a page that has changed since you started editing it.")
     end
 
-    attrs.slice!(:content, :format, :message, :title)
-    @attributes.merge!(attrs)
-    page_details =
-      if title.present? && @page.title != title
-        title
-      else
-        @page.url_path
+    update_attributes(attrs)
+
+    if title_changed?
+      page_details = title
+
+      if wiki.find_page(page_details).present?
+        @attributes[:title] = @page.url_path
+        raise PageRenameError.new("WikiEdit|There is already a page with the same title in that path.")
       end
+    else
+      page_details = @page.url_path
+    end
 
     save(page_details: page_details) do
       wiki.update_page(
@@ -255,8 +261,44 @@ class WikiPage
     page.version.to_s
   end
 
+  def title_changed?
+    title.present? && self.class.unhyphenize(@page.url_path) != title
+  end
+
   private
 
+  # Process and format the title based on the user input.
+  def process_title(title)
+    return if title.blank?
+
+    title = deep_title_squish(title)
+    current_dirname = File.dirname(title)
+
+    if @page.present?
+      return title[1..-1] if current_dirname == '/'
+      return File.join([directory.presence, title].compact) if current_dirname == '.'
+    end
+
+    title
+  end
+
+  # This method squishes all the filename
+  # i.e: '   foo   /  bar  / page_name' => 'foo/bar/page_name'
+  def deep_title_squish(title)
+    components = title.split(File::SEPARATOR).map(&:squish)
+
+    File.join(components)
+  end
+
+  # Updates the current @attributes hash by merging a hash of params
+  def update_attributes(attrs)
+    attrs[:title] = process_title(attrs[:title]) if attrs[:title].present?
+
+    attrs.slice!(:content, :format, :message, :title)
+
+    @attributes.merge!(attrs)
+  end
+
   def set_attributes
     attributes[:slug] = @page.url_path
     attributes[:title] = @page.title

app/views/projects/wikis/_form.html.haml

@@ -9,7 +9,13 @@
 
   .form-group
     .col-sm-12= f.label :title, class: 'control-label-full-width'
-    .col-sm-12= f.text_field :title, class: 'form-control', value: @page.title
+    .col-sm-12
+      = f.text_field :title, class: 'form-control', value: @page.title
+      - if @page.persisted?
+        %span.edit-wiki-page-slug-tip
+          = icon('lightbulb-o')
+          = s_("WikiEditPageTip|Tip: You can move this page by adding the path to the beginning of the title.")
+          = link_to icon('question-circle'), help_page_path('user/project/wiki/index', anchor: 'moving-a-wiki-page'), target: '_blank'
   .form-group
     .col-sm-12= f.label :format, class: 'control-label-full-width'
     .col-sm-12

app/views/projects/wikis/edit.html.haml

 - @content_class = "limit-container-width limit-container-width-sm" unless fluid_layout
 - page_title _("Edit"), @page.title.capitalize, _("Wiki")
 
-- if @conflict
-  .alert.alert-danger
-    - page_link = link_to s_("WikiPageConflictMessage|the page"), project_wiki_path(@project, @page), target: "_blank"
-    = (s_("WikiPageConflictMessage|Someone edited the page the same time you did. Please check out %{page_link} and make sure your changes will not unintentionally remove theirs.") % { page_link: page_link }).html_safe
+= wiki_page_errors(@error)
 
 .wiki-page-header.has-sidebar-toggle
   %button.btn.btn-default.sidebar-toggle.js-sidebar-wiki-toggle{ role: "button", type: "button" }

changelogs/unreleased/fj-37273-moving-wiki-pages-from-the-ui.yml

+---
+title: Allow moving wiki pages from the UI
+merge_request: 16313
+author:
+type: fixed

config/initializers/gollum.rb

@@ -35,6 +35,88 @@ module Gollum
         []
       end
     end
+
+    # Remove if https://github.com/gollum/gollum-lib/pull/292 has been merged
+    def update_page(page, name, format, data, commit = {})
+      name = name ? ::File.basename(name) : page.name
+      format ||= page.format
+      dir      = ::File.dirname(page.path)
+      dir      = '' if dir == '.'
+      filename = (rename = page.name != name) ? Gollum::Page.cname(name) : page.filename_stripped
+
+      multi_commit = !!commit[:committer]
+      committer    = multi_commit ? commit[:committer] : Committer.new(self, commit)
+
+      if !rename && page.format == format
+        committer.add(page.path, normalize(data))
+      else
+        committer.delete(page.path)
+        committer.add_to_index(dir, filename, format, data)
+      end
+
+      committer.after_commit do |index, _sha|
+        @access.refresh
+        index.update_working_dir(dir, page.filename_stripped, page.format)
+        index.update_working_dir(dir, filename, format)
+      end
+
+      multi_commit ? committer : committer.commit
+    end
+
+    # Remove if https://github.com/gollum/gollum-lib/pull/292 has been merged
+    def rename_page(page, rename, commit = {})
+      return false if page.nil?
+      return false if rename.nil? || rename.empty?
+
+      (target_dir, target_name) = ::File.split(rename)
+      (source_dir, source_name) = ::File.split(page.path)
+      source_name               = page.filename_stripped
+
+      # File.split gives us relative paths with ".", commiter.add_to_index doesn't like that.
+      target_dir                = '' if target_dir == '.'
+      source_dir                = '' if source_dir == '.'
+      target_dir                = target_dir.gsub(/^\//, '') # rubocop:disable Style/RegexpLiteral
+
+      # if the rename is a NOOP, abort
+      if source_dir == target_dir && source_name == target_name
+        return false
+      end
+
+      multi_commit = !!commit[:committer]
+      committer    = multi_commit ? commit[:committer] : Committer.new(self, commit)
+
+      # This piece only works for multi_commit
+      # If we are in a commit batch and one of the previous operations
+      # has updated the page, any information we ask to the page can be outdated.
+      # Therefore, we should ask first to the current committer tree to see if
+      # there is any updated change.
+      raw_data = raw_data_in_committer(committer, source_dir, page.filename) ||
+        raw_data_in_committer(committer, source_dir, "#{target_name}.#{Page.format_to_ext(page.format)}") ||
+        page.raw_data
+
+      committer.delete(page.path)
+      committer.add_to_index(target_dir, target_name, page.format, raw_data)
+
+      committer.after_commit do |index, _sha|
+        @access.refresh
+        index.update_working_dir(source_dir, source_name, page.format)
+        index.update_working_dir(target_dir, target_name, page.format)
+      end
+
+      multi_commit ? committer : committer.commit
+    end
+
+    # Remove if https://github.com/gollum/gollum-lib/pull/292 has been merged
+    def raw_data_in_committer(committer, dir, filename)
+      data = nil
+
+      [*dir.split(::File::SEPARATOR), filename].each do |key|
+        data = data ? data[key] : committer.tree[key]
+        break unless data
+      end
+
+      data
+    end
   end
 
   module Git

doc/user/project/wiki/index.md

@@ -64,6 +64,18 @@ effect.
 You can find the **Delete** button only when editing a page. Click on it and
 confirm you want the page to be deleted.
 
+## Moving a wiki page
+
+You can move a wiki page from one directory to another by specifying the full
+path in the wiki page title in the [edit](#editing-a-wiki-page) form.
+
+![Moving a page](img/wiki_move_page_1.png)
+
+![After moving a page](img/wiki_move_page_2.png)
+
+In order to move a wiki page to the root directory, the wiki page title must
+be preceded by the slash (`/`) character.
+
 ## Viewing a list of all created wiki pages
 
 Every wiki has a sidebar from which a short list of the created pages can be

lib/gitlab/git/wiki.rb

@@ -25,8 +25,9 @@ module Gitlab
         @repository.exists?
       end
 
+      # Disabled because of https://gitlab.com/gitlab-org/gitaly/merge_requests/539
       def write_page(name, format, content, commit_details)
-        @repository.gitaly_migrate(:wiki_write_page) do |is_enabled|
+        @repository.gitaly_migrate(:wiki_write_page, status: Gitlab::GitalyClient::MigrationStatus::DISABLED) do |is_enabled|
           if is_enabled
             gitaly_write_page(name, format, content, commit_details)
             gollum_wiki.clear_cache
@@ -47,8 +48,9 @@ module Gitlab
         end
       end
 
+      # Disable because of https://gitlab.com/gitlab-org/gitlab-ce/issues/42094
       def update_page(page_path, title, format, content, commit_details)
-        @repository.gitaly_migrate(:wiki_update_page) do |is_enabled|
+        @repository.gitaly_migrate(:wiki_update_page, status: Gitlab::GitalyClient::MigrationStatus::DISABLED) do |is_enabled|
           if is_enabled
             gitaly_update_page(page_path, title, format, content, commit_details)
             gollum_wiki.clear_cache
@@ -68,8 +70,9 @@ module Gitlab
         end
       end
 
+      # Disable because of https://gitlab.com/gitlab-org/gitlab-ce/issues/42039
       def page(title:, version: nil, dir: nil)
-        @repository.gitaly_migrate(:wiki_find_page) do |is_enabled|
+        @repository.gitaly_migrate(:wiki_find_page, status: Gitlab::GitalyClient::MigrationStatus::DISABLED) do |is_enabled|
           if is_enabled
             gitaly_find_page(title: title, version: version, dir: dir)
           else
@@ -192,7 +195,10 @@ module Gitlab
         assert_type!(format, Symbol)
         assert_type!(commit_details, CommitDetails)
 
-        gollum_wiki.write_page(name, format, content, commit_details.to_h)
+        filename = File.basename(name)
+        dir = (tmp_dir = File.dirname(name)) == '.' ? '' : tmp_dir
+
+        gollum_wiki.write_page(filename, format, content, commit_details.to_h, dir)
 
         nil
       rescue Gollum::DuplicatePageError => e
@@ -210,7 +216,15 @@ module Gitlab
         assert_type!(format, Symbol)
         assert_type!(commit_details, CommitDetails)
 
-        gollum_wiki.update_page(gollum_page_by_path(page_path), title, format, content, commit_details.to_h)
+        page = gollum_page_by_path(page_path)
+        committer = Gollum::Committer.new(page.wiki, commit_details.to_h)
+
+        # Instead of performing two renames if the title has changed,
+        # the update_page will only update the format and content and
+        # the rename_page will do anything related to moving/renaming
+        gollum_wiki.update_page(page, page.name, format, content, committer: committer)
+        gollum_wiki.rename_page(page, title, committer: committer)
+        committer.commit
         nil
       end
 

spec/features/projects/wiki/user_updates_wiki_page_spec.rb

 require 'spec_helper'
 
-describe 'User updates wiki page' do
+# Remove skip_gitaly_mock flag when gitaly_update_page implements moving pages
+describe 'User updates wiki page', :skip_gitaly_mock do
   let(:user) { create(:user) }
 
   before do
@@ -143,6 +144,7 @@ describe 'User updates wiki page' do
         expect(page).to have_field('wiki[message]', with: 'Update home')
 
         fill_in(:wiki_content, with: 'My awesome wiki!')
+
         click_button('Save changes')
 
         expect(page).to have_content('Home')
@@ -151,4 +153,74 @@ describe 'User updates wiki page' do
       end
     end
   end
+
+  context 'when the page is in a subdir' do
+    let!(:project) { create(:project, namespace: user.namespace) }
+    let(:project_wiki) { create(:project_wiki, project: project, user: project.creator) }
+    let(:page_name) { 'page_name' }
+    let(:page_dir) { "foo/bar/#{page_name}" }
+    let!(:wiki_page) { create(:wiki_page, wiki: project_wiki, attrs: { title: page_dir, content: 'Home page' }) }
+
+    before do
+      visit(project_wiki_edit_path(project, wiki_page))
+    end
+
+    it 'moves the page to the root folder' do
+      fill_in(:wiki_title, with: "/#{page_name}")
+
+      click_button('Save changes')
+
+      expect(current_path).to eq(project_wiki_path(project, page_name))
+    end
+
+    it 'moves the page to other dir' do
+      new_page_dir = "foo1/bar1/#{page_name}"
+
+      fill_in(:wiki_title, with: new_page_dir)
+
+      click_button('Save changes')
+
+      expect(current_path).to eq(project_wiki_path(project, new_page_dir))
+    end
+
+    it 'remains in the same place if title has not changed' do
+      original_path = project_wiki_path(project, wiki_page)
+
+      fill_in(:wiki_title, with: page_name)
+
+      click_button('Save changes')
+
+      expect(current_path).to eq(original_path)
+    end
+
+    it 'can be moved to a different dir with a different name' do
+      new_page_dir = "foo1/bar1/new_page_name"
+
+      fill_in(:wiki_title, with: new_page_dir)
+
+      click_button('Save changes')
+
+      expect(current_path).to eq(project_wiki_path(project, new_page_dir))
+    end
+
+    it 'can be renamed and moved to the root folder' do
+      new_name = 'new_page_name'
+
+      fill_in(:wiki_title, with: "/#{new_name}")
+
+      click_button('Save changes')
+
+      expect(current_path).to eq(project_wiki_path(project, new_name))
+    end
+
+    it 'squishes the title before creating the page' do
+      new_page_dir = "  foo1 /  bar1  /  #{page_name}  "
+
+      fill_in(:wiki_title, with: new_page_dir)
+
+      click_button('Save changes')
+
+      expect(current_path).to eq(project_wiki_path(project, "foo1/bar1/#{page_name}"))
+    end
+  end
 end

spec/features/projects/wiki/user_views_wiki_page_spec.rb

 require 'spec_helper'
 
-describe 'User views a wiki page' do
+# Remove skip_gitaly_mock flag when gitaly_update_page implements moving pages
+describe 'User views a wiki page', :skip_gitaly_mock do
   let(:user) { create(:user) }
   let(:project) { create(:project, namespace: user.namespace) }
   let(:wiki_page) do

spec/lib/gitlab/git/wiki_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Git::Wiki do
+  let(:project) { create(:project) }
+  let(:user) { project.owner }
+  let(:wiki) { ProjectWiki.new(project, user) }
+  let(:gollum_wiki) { wiki.wiki }
+
+  # Remove skip_gitaly_mock flag when gitaly_find_page when
+  # https://gitlab.com/gitlab-org/gitaly/merge_requests/539 gets merged
+  describe '#page', :skip_gitaly_mock do
+    it 'returns the right page' do
+      create_page('page1', 'content')
+      create_page('foo/page1', 'content')
+
+      expect(gollum_wiki.page(title: 'page1', dir: '').url_path).to eq 'page1'
+      expect(gollum_wiki.page(title: 'page1', dir: 'foo').url_path).to eq 'foo/page1'
+
+      destroy_page('page1')
+      destroy_page('page1', 'foo')
+    end
+  end
+
+  def create_page(name, content)
+    gollum_wiki.write_page(name, :markdown, content, commit_details)
+  end
+
+  def commit_details
+    Gitlab::Git::Wiki::CommitDetails.new(user.name, user.email, "test commit")
+  end
+
+  def destroy_page(title, dir = '')
+    page = gollum_wiki.page(title: title, dir: dir)
+    wiki.delete_page(page, "test commit")
+  end
+end

spec/models/wiki_page_spec.rb

@@ -188,14 +188,37 @@ describe WikiPage do
     end
   end
 
-  describe "#update" do
+  describe '#create', :skip_gitaly_mock do
+    context 'with valid attributes' do
+      it 'raises an error if a page with the same path already exists' do
+        create_page('New Page', 'content')
+        create_page('foo/bar', 'content')
+        expect { create_page('New Page', 'other content') }.to raise_error Gitlab::Git::Wiki::DuplicatePageError
+        expect { create_page('foo/bar', 'other content') }.to raise_error Gitlab::Git::Wiki::DuplicatePageError
+
+        destroy_page('New Page')
+        destroy_page('bar', 'foo')
+      end
+
+      it 'if the title is preceded by a / it is removed' do
+        create_page('/New Page', 'content')
+
+        expect(wiki.find_page('New Page')).not_to be_nil
+
+        destroy_page('New Page')
+      end
+    end
+  end
+
+  # Remove skip_gitaly_mock flag when gitaly_update_page implements moving pages
+  describe "#update", :skip_gitaly_mock do
     before do
       create_page("Update", "content")
       @page = wiki.find_page("Update")
     end
 
     after do
-      destroy_page(@page.title)
+      destroy_page(@page.title, @page.directory)
     end
 
     context "with valid attributes" do
@@ -233,6 +256,95 @@ describe WikiPage do
         expect { @page.update(content: 'more content', last_commit_sha: 'xxx') }.to raise_error(WikiPage::PageChangedError)
       end
     end
+
+    context 'when renaming a page' do
+      it 'raises an error if the page already exists' do
+        create_page('Existing Page', 'content')
+
+        expect { @page.update(title: 'Existing Page', content: 'new_content') }.to raise_error(WikiPage::PageRenameError)
+        expect(@page.title).to eq 'Update'
+        expect(@page.content).to eq 'new_content'
+
+        destroy_page('Existing Page')
+      end
+
+      it 'updates the content and rename the file' do
+        new_title = 'Renamed Page'
+        new_content = 'updated content'
+
+        expect(@page.update(title: new_title, content: new_content)).to be_truthy
+
+        @page = wiki.find_page(new_title)
+
+        expect(@page).not_to be_nil
+        expect(@page.content).to eq new_content
+      end
+    end
+
+    context 'when moving a page' do
+      it 'raises an error if the page already exists' do
+        create_page('foo/Existing Page', 'content')
+
+        expect { @page.update(title: 'foo/Existing Page', content: 'new_content') }.to raise_error(WikiPage::PageRenameError)
+        expect(@page.title).to eq 'Update'
+        expect(@page.content).to eq 'new_content'
+
+        destroy_page('Existing Page', 'foo')
+      end
+
+      it 'updates the content and moves the file' do
+        new_title = 'foo/Other Page'
+        new_content = 'new_content'
+
+        expect(@page.update(title: new_title, content: new_content)).to be_truthy
+
+        page = wiki.find_page(new_title)
+
+        expect(page).not_to be_nil
+        expect(page.content).to eq new_content
+      end
+
+      context 'in subdir' do
+        before do
+          create_page('foo/Existing Page', 'content')
+          @page = wiki.find_page('foo/Existing Page')
+        end
+
+        it 'moves the page to the root folder if the title is preceded by /' do
+          expect(@page.slug).to eq 'foo/Existing-Page'
+          expect(@page.update(title: '/Existing Page', content: 'new_content')).to be_truthy
+          expect(@page.slug).to eq 'Existing-Page'
+        end
+
+        it 'does nothing if it has the same title' do
+          original_path = @page.slug
+
+          expect(@page.update(title: 'Existing Page', content: 'new_content')).to be_truthy
+          expect(@page.slug).to eq original_path
+        end
+      end
+
+      context 'in root dir' do
+        it 'does nothing if the title is preceded by /' do
+          original_path = @page.slug
+
+          expect(@page.update(title: '/Update', content: 'new_content')).to be_truthy
+          expect(@page.slug).to eq original_path
+        end
+      end
+    end
+
+    context "with invalid attributes" do
+      it 'aborts update if title blank' do
+        expect(@page.update(title: '', content: 'new_content')).to be_falsey
+        expect(@page.content).to eq 'new_content'
+
+        page = wiki.find_page('Update')
+        expect(page.content).to eq 'content'
+
+        @page.title = 'Update'
+      end
+    end
   end
 
   describe "#destroy" do
@@ -421,8 +533,8 @@ describe WikiPage do
     wiki.wiki.write_page(name, :markdown, content, commit_details)
   end
 
-  def destroy_page(title)
-    page = wiki.wiki.page(title: title)
+  def destroy_page(title, dir = '')
+    page = wiki.wiki.page(title: title, dir: dir)
     wiki.delete_page(page, "test commit")
   end