Commit 64131f04 authored by Robert Speicher's avatar Robert Speicher

Merge branch '1827-prevent-concurrent-editing-wiki' into 'master'

Prevent concurrent editing wiki

Closes #1827

See merge request !9707
parents fba50b4a 35259a4f
...@@ -55,6 +55,9 @@ class Projects::WikisController < Projects::ApplicationController ...@@ -55,6 +55,9 @@ class Projects::WikisController < Projects::ApplicationController
else else
render 'edit' render 'edit'
end end
rescue WikiPage::PageChangedError
@conflict = true
render 'edit'
end end
def create def create
...@@ -119,6 +122,6 @@ class Projects::WikisController < Projects::ApplicationController ...@@ -119,6 +122,6 @@ class Projects::WikisController < Projects::ApplicationController
end end
def wiki_params def wiki_params
params.require(:wiki).permit(:title, :content, :format, :message) params.require(:wiki).permit(:title, :content, :format, :message, :last_commit_sha)
end end
end end
class WikiPage class WikiPage
PageChangedError = Class.new(StandardError)
include ActiveModel::Validations include ActiveModel::Validations
include ActiveModel::Conversion include ActiveModel::Conversion
include StaticModel include StaticModel
...@@ -136,6 +138,10 @@ class WikiPage ...@@ -136,6 +138,10 @@ class WikiPage
versions.first versions.first
end end
def last_commit_sha
commit&.sha
end
# Returns the Date that this latest version was # Returns the Date that this latest version was
# created on. # created on.
def created_at def created_at
...@@ -186,13 +192,18 @@ class WikiPage ...@@ -186,13 +192,18 @@ class WikiPage
# format - Optional symbol representing the content format. # format - Optional symbol representing the content format.
# See ProjectWiki::MARKUPS Hash for available formats. # See ProjectWiki::MARKUPS Hash for available formats.
# message - Optional commit message to set on the new version. # 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 # Returns the String SHA1 of the newly created page
# or False if the save was unsuccessful. # 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[:content] = new_content
@attributes[:format] = format @attributes[:format] = format
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.")
end
save :update_page, @page, content, format, message save :update_page, @page, content, format, message
end end
......
module WikiPages module WikiPages
class UpdateService < WikiPages::BaseService class UpdateService < WikiPages::BaseService
def execute(page) def execute(page)
if page.update(@params[:content], @params[:format], @params[:message]) if page.update(@params[:content], format: @params[:format], message: @params[:message], last_commit_sha: @params[:last_commit_sha])
execute_hooks(page, 'update') execute_hooks(page, 'update')
end end
......
...@@ -4,6 +4,8 @@ ...@@ -4,6 +4,8 @@
= form_errors(@page) = form_errors(@page)
= f.hidden_field :title, value: @page.title = f.hidden_field :title, value: @page.title
- if @page.persisted?
= f.hidden_field :last_commit_sha, value: @page.last_commit_sha
.form-group .form-group
.col-sm-12= f.label :format, class: 'control-label-full-width' .col-sm-12= f.label :format, class: 'control-label-full-width'
.col-sm-12 .col-sm-12
......
- @content_class = "limit-container-width limit-container-width-sm" unless fluid_layout - @content_class = "limit-container-width limit-container-width-sm" unless fluid_layout
- page_title "Edit", @page.title.capitalize, "Wiki" - page_title "Edit", @page.title.capitalize, "Wiki"
- if @conflict
.alert.alert-danger
Someone edited the page the same time you did. Please check out
= link_to "the page", project_wiki_path(@project, @page), target: "_blank"
and make sure your changes will not unintentionally remove theirs.
.wiki-page-header.has-sidebar-toggle .wiki-page-header.has-sidebar-toggle
%button.btn.btn-default.sidebar-toggle.js-sidebar-wiki-toggle{ role: "button", type: "button" } %button.btn.btn-default.sidebar-toggle.js-sidebar-wiki-toggle{ role: "button", type: "button" }
= icon('angle-double-left') = icon('angle-double-left')
......
---
title: Alert the user if a Wiki page changed while they were editing it in order to prevent overwriting changes.
merge_request: 9707
author: Hiroyuki Sato
...@@ -63,7 +63,7 @@ class Spinach::Features::ProjectWiki < Spinach::FeatureSteps ...@@ -63,7 +63,7 @@ class Spinach::Features::ProjectWiki < Spinach::FeatureSteps
end end
step 'That page has two revisions' do step 'That page has two revisions' do
@page.update("new content", :markdown, "second commit") @page.update("new content", message: "second commit")
end end
step 'I click the History button' do step 'I click the History button' do
......
...@@ -2,10 +2,10 @@ require 'spec_helper' ...@@ -2,10 +2,10 @@ require 'spec_helper'
feature 'Projects > Wiki > User updates wiki page' do feature 'Projects > Wiki > User updates wiki page' do
let(:user) { create(:user) } let(:user) { create(:user) }
let!(:wiki_page) { WikiPages::CreateService.new(project, user, title: 'home', content: 'Home page').execute }
background do background do
project.team << [user, :master] project.team << [user, :master]
WikiPages::CreateService.new(project, user, title: 'home', content: 'Home page').execute
sign_in(user) sign_in(user)
visit project_wikis_path(project) visit project_wikis_path(project)
...@@ -51,6 +51,16 @@ feature 'Projects > Wiki > User updates wiki page' do ...@@ -51,6 +51,16 @@ feature 'Projects > Wiki > User updates wiki page' do
expect(page).to have_selector('.atwho-view') expect(page).to have_selector('.atwho-view')
end end
end 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 end
context 'in a group namespace' do context 'in a group namespace' do
......
...@@ -208,6 +208,18 @@ describe WikiPage do ...@@ -208,6 +208,18 @@ describe WikiPage do
expect(@page.update("more content")).to be_truthy expect(@page.update("more content")).to be_truthy
end end
end end
context 'with same last commit sha' do
it 'returns true' do
expect(@page.update('more content', last_commit_sha: @page.last_commit_sha)).to be_truthy
end
end
context 'with different last commit sha' do
it 'raises exception' do
expect { @page.update('more content', last_commit_sha: 'xxx') }.to raise_error(WikiPage::PageChangedError)
end
end
end end
describe "#destroy" do describe "#destroy" do
...@@ -331,6 +343,30 @@ describe WikiPage do ...@@ -331,6 +343,30 @@ describe WikiPage do
end end
end end
describe '#last_commit_sha' do
before do
create_page("Update", "content")
@page = wiki.find_page("Update")
end
after do
destroy_page("Update")
end
it 'returns commit sha' do
expect(@page.last_commit_sha).to eq @page.commit.sha
end
it 'is changed after page updated' do
last_commit_sha_before_update = @page.last_commit_sha
@page.update("new content")
@page = wiki.find_page("Update")
expect(@page.last_commit_sha).not_to eq last_commit_sha_before_update
end
end
private private
def remove_temp_repo(path) def remove_temp_repo(path)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment