summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorValery Sizov <valery@gitlab.com>2016-07-08 00:42:16 +0300
committerValery Sizov <valery@gitlab.com>2016-07-09 15:41:10 +0300
commitb4d1fc945d2a038234b07cf3de54e2fda3361243 (patch)
treea7ea69010ab462e97020ef9d11822cdba296b512
parent440345bd977caac1b604085feca4b46b4c1eea01 (diff)
downloadgitlab-ce-lock_for_issuable.tar.gz
Optimistic locking for Issue and Merge Requestslock_for_issuable
-rw-r--r--CHANGELOG1
-rw-r--r--app/controllers/projects/issues_controller.rb6
-rw-r--r--app/controllers/projects/merge_requests_controller.rb7
-rw-r--r--app/models/concerns/issuable.rb6
-rw-r--r--app/views/shared/issuable/_form.html.haml9
-rw-r--r--db/migrate/20160707104333_add_lock_to_issuables.rb17
-rw-r--r--db/schema.rb24
-rw-r--r--features/project/merge_requests.feature2
-rw-r--r--spec/features/issues_spec.rb11
-rw-r--r--spec/features/merge_requests/edit_mr_spec.rb11
10 files changed, 79 insertions, 15 deletions
diff --git a/CHANGELOG b/CHANGELOG
index 3e4a10bb5a3..52fcc47f210 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -60,6 +60,7 @@ v 8.10.0 (unreleased)
- Fix 404 redirect after validation fails importing a GitLab project
- Added setting to set new users by default as external !4545 (Dravere)
- Add min value for project limit field on user's form !3622 (jastkand)
+ - Optimistic locking for Issues and Merge Requests (Title and description overriding prevention)
v 8.9.5
- Add more debug info to import/export and memory killer. !5108
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index b6e80762e3c..f7ada5cfee4 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -119,6 +119,10 @@ class Projects::IssuesController < Projects::ApplicationController
render json: @issue.to_json(include: { milestone: {}, assignee: { methods: :avatar_url }, labels: { methods: :text_color } })
end
end
+
+ rescue ActiveRecord::StaleObjectError
+ @conflict = true
+ render :edit
end
def referenced_merge_requests
@@ -216,7 +220,7 @@ class Projects::IssuesController < Projects::ApplicationController
def issue_params
params.require(:issue).permit(
:title, :assignee_id, :position, :description, :confidential,
- :milestone_id, :due_date, :state_event, :task_num, label_ids: []
+ :milestone_id, :due_date, :state_event, :task_num, :lock_version, label_ids: []
)
end
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 5678a4015b6..8c8c55a75e5 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -54,7 +54,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
def show
respond_to do |format|
format.html
-
+
format.json do
render json: @merge_request
end
@@ -187,6 +187,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
else
render "edit"
end
+ rescue ActiveRecord::StaleObjectError
+ @conflict = true
+ render :edit
end
def remove_wip
@@ -394,7 +397,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
:title, :assignee_id, :source_project_id, :source_branch,
:target_project_id, :target_branch, :milestone_id,
:state_event, :description, :task_num, :force_remove_source_branch,
- label_ids: []
+ :lock_version, label_ids: []
)
end
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index acb6f5a2998..fb49bd7dd64 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -87,6 +87,12 @@ module Issuable
User.find(assignee_id_was).update_cache_counts if assignee_id_was
assignee.update_cache_counts if assignee
end
+
+ # We want to use optimistic lock for cases when only title or description are involved
+ # http://api.rubyonrails.org/classes/ActiveRecord/Locking/Optimistic.html
+ def locking_enabled?
+ title_changed? || description_changed?
+ end
end
module ClassMethods
diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml
index c30bdb0ae91..98bbb12eaec 100644
--- a/app/views/shared/issuable/_form.html.haml
+++ b/app/views/shared/issuable/_form.html.haml
@@ -1,5 +1,12 @@
= form_errors(issuable)
+- if @conflict
+ .alert.alert-danger
+ Someone edited the #{issuable.class.model_name.human.downcase} the same time you did.
+ Please check out
+ = link_to "the #{issuable.class.model_name.human.downcase}", polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable]), target: "_blank"
+ and make sure your changes will not unintentionally remove theirs
+
.form-group
= f.label :title, class: 'control-label'
.col-sm-10
@@ -149,3 +156,5 @@
= link_to 'Delete', polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable]), data: { confirm: "#{issuable.class.name.titleize} will be removed! Are you sure?" },
method: :delete, class: 'btn btn-danger btn-grouped'
= link_to 'Cancel', polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable]), class: 'btn btn-grouped btn-cancel'
+
+= f.hidden_field :lock_version
diff --git a/db/migrate/20160707104333_add_lock_to_issuables.rb b/db/migrate/20160707104333_add_lock_to_issuables.rb
new file mode 100644
index 00000000000..cb516672800
--- /dev/null
+++ b/db/migrate/20160707104333_add_lock_to_issuables.rb
@@ -0,0 +1,17 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddLockToIssuables < ActiveRecord::Migration
+ include Gitlab::Database::MigrationHelpers
+ disable_ddl_transaction!
+
+ def up
+ add_column_with_default :issues, :lock_version, :integer, default: 0
+ add_column_with_default :merge_requests, :lock_version, :integer, default: 0
+ end
+
+ def down
+ remove_column :issues, :lock_version
+ remove_column :merge_requests, :lock_version
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index a5eea3a697c..9d31947d80f 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.
-ActiveRecord::Schema.define(version: 20160705163108) do
+ActiveRecord::Schema.define(version: 20160707104333) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
@@ -70,11 +70,11 @@ ActiveRecord::Schema.define(version: 20160705163108) do
t.string "recaptcha_site_key"
t.string "recaptcha_private_key"
t.integer "metrics_port", default: 8089
- t.boolean "akismet_enabled", default: false
- t.string "akismet_api_key"
t.integer "metrics_sample_interval", default: 15
t.boolean "sentry_enabled", default: false
t.string "sentry_dsn"
+ t.boolean "akismet_enabled", default: false
+ t.string "akismet_api_key"
t.boolean "email_author_in_body", default: false
t.integer "default_group_visibility"
t.boolean "repository_checks_enabled", default: false
@@ -84,10 +84,10 @@ ActiveRecord::Schema.define(version: 20160705163108) do
t.string "health_check_access_token"
t.boolean "send_user_confirmation_email", default: false
t.integer "container_registry_token_expire_delay", default: 5
- t.boolean "user_default_external", default: false, null: false
t.text "after_sign_up_text"
t.string "repository_storage", default: "default"
t.string "enabled_git_access_protocol"
+ t.boolean "user_default_external", default: false, null: false
end
create_table "audit_events", force: :cascade do |t|
@@ -165,8 +165,8 @@ ActiveRecord::Schema.define(version: 20160705163108) do
t.text "artifacts_metadata"
t.integer "erased_by_id"
t.datetime "erased_at"
- t.string "environment"
t.datetime "artifacts_expire_at"
+ t.string "environment"
t.integer "artifacts_size"
end
@@ -481,10 +481,11 @@ ActiveRecord::Schema.define(version: 20160705163108) do
t.string "state"
t.integer "iid"
t.integer "updated_by_id"
+ t.integer "moved_to_id"
t.boolean "confidential", default: false
t.datetime "deleted_at"
t.date "due_date"
- t.integer "moved_to_id"
+ t.integer "lock_version", default: 0, null: false
end
add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree
@@ -624,6 +625,7 @@ ActiveRecord::Schema.define(version: 20160705163108) do
t.integer "merge_user_id"
t.string "merge_commit_sha"
t.datetime "deleted_at"
+ t.integer "lock_version", default: 0, null: false
end
add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree
@@ -773,10 +775,10 @@ ActiveRecord::Schema.define(version: 20160705163108) do
t.integer "user_id", null: false
t.string "token", null: false
t.string "name", null: false
- t.datetime "created_at", null: false
- t.datetime "updated_at", null: false
t.boolean "revoked", default: false
t.datetime "expires_at"
+ t.datetime "created_at", null: false
+ t.datetime "updated_at", null: false
end
add_index "personal_access_tokens", ["token"], name: "index_personal_access_tokens_on_token", unique: true, using: :btree
@@ -896,9 +898,9 @@ ActiveRecord::Schema.define(version: 20160705163108) do
t.string "type"
t.string "title"
t.integer "project_id"
- t.datetime "created_at"
- t.datetime "updated_at"
- t.boolean "active", default: false, null: false
+ t.datetime "created_at", null: false
+ t.datetime "updated_at", null: false
+ t.boolean "active", null: false
t.text "properties"
t.boolean "template", default: false
t.boolean "push_events", default: true
diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature
index 21768c15c17..8176ec5ab45 100644
--- a/features/project/merge_requests.feature
+++ b/features/project/merge_requests.feature
@@ -89,7 +89,7 @@ Feature: Project Merge Requests
Then The list should be sorted by "Oldest updated"
@javascript
- Scenario: Visiting Merge Requests from a differente Project after sorting
+ Scenario: Visiting Merge Requests from a different Project after sorting
Given I visit project "Shop" merge requests page
And I sort the list by "Oldest updated"
And I visit dashboard merge requests page
diff --git a/spec/features/issues_spec.rb b/spec/features/issues_spec.rb
index d51c9abea19..cfe6349a1a1 100644
--- a/spec/features/issues_spec.rb
+++ b/spec/features/issues_spec.rb
@@ -121,6 +121,17 @@ describe 'Issues', feature: true do
expect(page).to have_content date.to_s(:medium)
end
end
+
+ it 'warns about version conflict' do
+ issue.update(title: "New title")
+
+ fill_in 'issue_title', with: 'bug 345'
+ fill_in 'issue_description', with: 'bug description'
+
+ click_button 'Save changes'
+
+ expect(page).to have_content 'Someone edited the issue the same time you did'
+ end
end
end
diff --git a/spec/features/merge_requests/edit_mr_spec.rb b/spec/features/merge_requests/edit_mr_spec.rb
index 9e007ab7635..8ad884492d1 100644
--- a/spec/features/merge_requests/edit_mr_spec.rb
+++ b/spec/features/merge_requests/edit_mr_spec.rb
@@ -17,5 +17,16 @@ feature 'Edit Merge Request', feature: true do
it 'form should have class js-quick-submit' do
expect(page).to have_selector('.js-quick-submit')
end
+
+ it 'warns about version conflict' do
+ merge_request.update(title: "New title")
+
+ fill_in 'merge_request_title', with: 'bug 345'
+ fill_in 'merge_request_description', with: 'bug description'
+
+ click_button 'Save changes'
+
+ expect(page).to have_content 'Someone edited the merge request the same time you did'
+ end
end
end