diff --git a/CHANGELOG b/CHANGELOG index 2ad8460731e..a50edf1a797 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -82,6 +82,7 @@ v 8.10.0 (unreleased) - Remove duplicate `description` field in `MergeRequest` entities (Ben Boeckel) - Style of import project buttons were fixed in the new project page. !5183 (rdemirbay) - Fix GitHub client requests when rate limit is disabled + - Optimistic locking for Issues and Merge Requests (Title and description overriding prevention) v 8.9.6 (unreleased) - Fix importing of events under notes for GitLab projects 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 df659bb8c3b..2deb7959700 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -196,6 +196,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController else render "edit" end + rescue ActiveRecord::StaleObjectError + @conflict = true + render :edit end def remove_wip @@ -424,7 +427,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