diff --git a/app/controllers/projects/wikis_controller.rb b/app/controllers/projects/wikis_controller.rb index ac98470c2b1..968d880886c 100644 --- a/app/controllers/projects/wikis_controller.rb +++ b/app/controllers/projects/wikis_controller.rb @@ -55,6 +55,9 @@ class Projects::WikisController < Projects::ApplicationController else render 'edit' end + rescue WikiPage::PageChangedError + @conflict = true + render 'edit' end def create @@ -119,6 +122,6 @@ class Projects::WikisController < Projects::ApplicationController end def wiki_params - params.require(:wiki).permit(:title, :content, :format, :message) + params.require(:wiki).permit(:title, :content, :format, :message, :last_commit_sha) end end diff --git a/app/models/wiki_page.rb b/app/models/wiki_page.rb index 224eb3cd4d0..148998bc9be 100644 --- a/app/models/wiki_page.rb +++ b/app/models/wiki_page.rb @@ -1,4 +1,6 @@ class WikiPage + PageChangedError = Class.new(StandardError) + include ActiveModel::Validations include ActiveModel::Conversion include StaticModel @@ -136,6 +138,10 @@ class WikiPage versions.first end + def last_commit_sha + commit&.sha + end + # Returns the Date that this latest version was # created on. def created_at @@ -182,17 +188,22 @@ class WikiPage # Updates an existing Wiki Page, creating a new version. # - # new_content - The raw markup content to replace the existing. - # format - Optional symbol representing the content format. - # See ProjectWiki::MARKUPS Hash for available formats. - # message - Optional commit message to set on the new version. + # new_content - The raw markup content to replace the existing. + # 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 != 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 end diff --git a/app/services/wiki_pages/update_service.rb b/app/services/wiki_pages/update_service.rb index 8f6a50da838..c628e6781af 100644 --- a/app/services/wiki_pages/update_service.rb +++ b/app/services/wiki_pages/update_service.rb @@ -1,7 +1,7 @@ module WikiPages class UpdateService < WikiPages::BaseService 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') end diff --git a/app/views/projects/wikis/_form.html.haml b/app/views/projects/wikis/_form.html.haml index fc6b7a33943..adb8d5aaecb 100644 --- a/app/views/projects/wikis/_form.html.haml +++ b/app/views/projects/wikis/_form.html.haml @@ -4,6 +4,8 @@ = form_errors(@page) = f.hidden_field :title, value: @page.title + - if @page.persisted? + = f.hidden_field :last_commit_sha, value: @page.last_commit_sha .form-group .col-sm-12= f.label :format, class: 'control-label-full-width' .col-sm-12 diff --git a/app/views/projects/wikis/edit.html.haml b/app/views/projects/wikis/edit.html.haml index df0ec14eb3b..8fd60216536 100644 --- a/app/views/projects/wikis/edit.html.haml +++ b/app/views/projects/wikis/edit.html.haml @@ -1,6 +1,12 @@ - @content_class = "limit-container-width limit-container-width-sm" unless fluid_layout - 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 %button.btn.btn-default.sidebar-toggle.js-sidebar-wiki-toggle{ role: "button", type: "button" } = icon('angle-double-left') diff --git a/changelogs/unreleased/1827-prevent-concurrent-editing-wiki.yml b/changelogs/unreleased/1827-prevent-concurrent-editing-wiki.yml new file mode 100644 index 00000000000..c8c2bb3eb4c --- /dev/null +++ b/changelogs/unreleased/1827-prevent-concurrent-editing-wiki.yml @@ -0,0 +1,4 @@ +--- +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 diff --git a/features/steps/project/wiki.rb b/features/steps/project/wiki.rb index 9d38939378d..6a478c50e5e 100644 --- a/features/steps/project/wiki.rb +++ b/features/steps/project/wiki.rb @@ -63,7 +63,7 @@ class Spinach::Features::ProjectWiki < Spinach::FeatureSteps end step 'That page has two revisions' do - @page.update("new content", :markdown, "second commit") + @page.update("new content", message: "second commit") end step 'I click the History button' do diff --git a/spec/features/projects/wiki/user_updates_wiki_page_spec.rb b/spec/features/projects/wiki/user_updates_wiki_page_spec.rb index 6ebf59cba98..e3739a705bf 100644 --- a/spec/features/projects/wiki/user_updates_wiki_page_spec.rb +++ b/spec/features/projects/wiki/user_updates_wiki_page_spec.rb @@ -2,10 +2,10 @@ require 'spec_helper' feature 'Projects > Wiki > User updates wiki page' do let(:user) { create(:user) } + let!(:wiki_page) { WikiPages::CreateService.new(project, user, title: 'home', content: 'Home page').execute } background do project.team << [user, :master] - WikiPages::CreateService.new(project, user, title: 'home', content: 'Home page').execute sign_in(user) visit project_wikis_path(project) @@ -51,6 +51,16 @@ feature 'Projects > Wiki > User updates wiki page' do expect(page).to have_selector('.atwho-view') 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 context 'in a group namespace' do diff --git a/spec/models/wiki_page_spec.rb b/spec/models/wiki_page_spec.rb index 90ad3cdeb93..55d9c4ddd7b 100644 --- a/spec/models/wiki_page_spec.rb +++ b/spec/models/wiki_page_spec.rb @@ -208,6 +208,18 @@ describe WikiPage do expect(@page.update("more content")).to be_truthy 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 describe "#destroy" do @@ -331,6 +343,30 @@ describe WikiPage do 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 def remove_temp_repo(path)