Prevent concurrent editing wiki

app/controllers/projects/wikis_controller.rb

@@ -56,6 +56,9 @@ class Projects::WikisController < Projects::ApplicationController
     else
       render 'edit'
     end
+  rescue WikiPage::PageChangedError
+    @conflict = true
+    render 'edit'
   end
 
   def create
@@ -125,6 +128,6 @@ class Projects::WikisController < Projects::ApplicationController
   end
 
   def wiki_params
-    params[:wiki].slice(:title, :content, :format, :message)
+    params[:wiki].slice(:title, :content, :format, :message, :last_commit_sha)
   end
 end

app/models/wiki_page.rb

 class WikiPage
+  PageChangedError = Class.new(StandardError)
+
   include ActiveModel::Validations
   include ActiveModel::Conversion
   include StaticModel
@@ -180,13 +182,18 @@ class WikiPage
   # format          - Optional symbol representing the content format.
   #                   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.
   #
   # Returns the String SHA1 of the newly created page
   # or False if the save was unsuccessful.
-  def update(new_content = "", format = :markdown, message = nil)
+  def update(new_content = "", format = :markdown, message = nil, last_commit_sha = nil)
     @attributes[:content] = new_content
     @attributes[:format] = format
 
+    if last_commit_sha && last_commit_sha != commit.sha
+      raise PageChangedError.new("You are attempting to update a page that has changed since you started editing it.")
+    end
+
     save :update_page, @page, content, format, message
   end
 

app/services/wiki_pages/update_service.rb

 module WikiPages
   class UpdateService < WikiPages::BaseService
     def execute(page)
-      if page.update(@params[:content], @params[:format], @params[:message])
+      if page.update(@params[:content], @params[:format], @params[:message], @params[:last_commit_sha])
         execute_hooks(page, 'update')
       end
 

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

@@ -2,6 +2,8 @@
   = form_errors(@page)
 
   = f.hidden_field :title, value: @page.title
+  - if @page.persisted?
+    = f.hidden_field :last_commit_sha, value: @page.commit.sha
   .form-group
     = f.label :format, class: 'control-label'
     .col-sm-10

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

@@ -2,6 +2,12 @@
 - page_title "Edit", @page.title.capitalize, "Wiki"
 
 %div{ class: container_class }
+  - if @conflict
+    .alert.alert-danger
+      Someone edited the page the same time you did. Please check out
+      = link_to "the page", namespace_project_wiki_path(@project.namespace, @project, @page), target: "_blank"
+      and make sure your changes will not unintentionally remove theirs.
+
   .wiki-page-header.has-sidebar-toggle
     %button.btn.btn-default.sidebar-toggle.js-sidebar-wiki-toggle{ role: "button", type: "button" }
       = icon('angle-double-left')

changelogs/unreleased/1827-prevent-concurrent-editing-wiki.yml

+---
+title: Prevent concurrent editing wiki
+merge_request: 9707
+author: Hiroyuki Sato

spec/features/projects/wiki/user_updates_wiki_page_spec.rb

@@ -8,7 +8,7 @@ feature 'Projects > Wiki > User updates wiki page', feature: true do
     login_as(user)
 
     visit namespace_project_path(project.namespace, project)
-    WikiPages::CreateService.new(project, user, title: 'home', content: 'Home page').execute
+    @wiki_page = WikiPages::CreateService.new(project, user, title: 'home', content: 'Home page').execute
     click_link 'Wiki'
   end
 
@@ -25,6 +25,16 @@ feature 'Projects > Wiki > User updates wiki page', feature: true do
       expect(page).to have_content("Last edited by #{user.name}")
       expect(page).to have_content('My awesome wiki!')
     end
+
+    scenario 'page has been updated since the user opened the edit page' do
+      click_link 'Edit'
+
+      @wiki_page.update("Update")
+
+      click_button 'Save changes'
+
+      expect(page).to have_content 'Someone edited the page the same time you did.'
+    end
   end
 
   context 'in a group namespace' do

spec/models/wiki_page_spec.rb

@@ -208,6 +208,20 @@ describe WikiPage, models: true do
         expect(@page.update("more content")).to be_truthy
       end
     end
+
+    context "with same last commit sha" do
+      it "returns true" do
+        last_commit_sha = @page.commit.sha
+        expect(@page.update("more content", :markdown, nil, last_commit_sha)).to be_truthy
+      end
+    end
+
+    context "with different last commit sha" do
+      it "raises exception" do
+        last_commit_sha = "xxx"
+        expect { @page.update("more content", :markdown, nil, last_commit_sha) }.to raise_error(WikiPage::PageChangedError)
+      end
+    end
   end
 
   describe "#destroy" do