Commit d4677353 authored by Robert Speicher's avatar Robert Speicher

Merge branch 'revert-lock-for-issuable' into 'master'

Revert "Optimistic locking for Issue and Merge Requests"

The migration to add `lock_version` in !5146 to every issue and merge request takes too long on GitLab.com since it has to add a default value of 0 to every row. Unfortunately, Rails 4.2 doesn't work properly if the value is left as `nil`; anything using optimistic locking cannot be edited. 

This bug was fixed in Rails 5.0 via this commit: https://github.com/rails/rails/commit/13772bfa49325a8dc26410d2dcb555665a320f92. I suggest we revert this change for now, and when we upgrade to Rails 5.0 we can reintroduce this feature.

See merge request !5245
parents dfea11e1 d7c59191
...@@ -119,10 +119,6 @@ class Projects::IssuesController < Projects::ApplicationController ...@@ -119,10 +119,6 @@ class Projects::IssuesController < Projects::ApplicationController
render json: @issue.to_json(include: { milestone: {}, assignee: { methods: :avatar_url }, labels: { methods: :text_color } }) render json: @issue.to_json(include: { milestone: {}, assignee: { methods: :avatar_url }, labels: { methods: :text_color } })
end end
end end
rescue ActiveRecord::StaleObjectError
@conflict = true
render :edit
end end
def referenced_merge_requests def referenced_merge_requests
...@@ -220,7 +216,7 @@ class Projects::IssuesController < Projects::ApplicationController ...@@ -220,7 +216,7 @@ class Projects::IssuesController < Projects::ApplicationController
def issue_params def issue_params
params.require(:issue).permit( params.require(:issue).permit(
:title, :assignee_id, :position, :description, :confidential, :title, :assignee_id, :position, :description, :confidential,
:milestone_id, :due_date, :state_event, :task_num, :lock_version, label_ids: [] :milestone_id, :due_date, :state_event, :task_num, label_ids: []
) )
end end
......
...@@ -196,9 +196,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -196,9 +196,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
else else
render "edit" render "edit"
end end
rescue ActiveRecord::StaleObjectError
@conflict = true
render :edit
end end
def remove_wip def remove_wip
...@@ -427,7 +424,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -427,7 +424,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
:title, :assignee_id, :source_project_id, :source_branch, :title, :assignee_id, :source_project_id, :source_branch,
:target_project_id, :target_branch, :milestone_id, :target_project_id, :target_branch, :milestone_id,
:state_event, :description, :task_num, :force_remove_source_branch, :state_event, :description, :task_num, :force_remove_source_branch,
:lock_version, label_ids: [] label_ids: []
) )
end end
......
...@@ -87,12 +87,6 @@ module Issuable ...@@ -87,12 +87,6 @@ module Issuable
User.find(assignee_id_was).update_cache_counts if assignee_id_was User.find(assignee_id_was).update_cache_counts if assignee_id_was
assignee.update_cache_counts if assignee assignee.update_cache_counts if assignee
end 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 end
module ClassMethods module ClassMethods
......
= form_errors(issuable) = 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 .form-group
= f.label :title, class: 'control-label' = f.label :title, class: 'control-label'
.col-sm-10 .col-sm-10
...@@ -142,5 +135,3 @@ ...@@ -142,5 +135,3 @@
= link_to 'Delete', polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable]), data: { confirm: "#{issuable.class.name.titleize} will be removed! Are you sure?" }, = 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' 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' = link_to 'Cancel', polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable]), class: 'btn btn-grouped btn-cancel'
= f.hidden_field :lock_version
# 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
...@@ -70,11 +70,11 @@ ActiveRecord::Schema.define(version: 20160712171823) do ...@@ -70,11 +70,11 @@ ActiveRecord::Schema.define(version: 20160712171823) do
t.string "recaptcha_site_key" t.string "recaptcha_site_key"
t.string "recaptcha_private_key" t.string "recaptcha_private_key"
t.integer "metrics_port", default: 8089 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.integer "metrics_sample_interval", default: 15
t.boolean "sentry_enabled", default: false t.boolean "sentry_enabled", default: false
t.string "sentry_dsn" t.string "sentry_dsn"
t.boolean "akismet_enabled", default: false
t.string "akismet_api_key"
t.boolean "email_author_in_body", default: false t.boolean "email_author_in_body", default: false
t.integer "default_group_visibility" t.integer "default_group_visibility"
t.boolean "repository_checks_enabled", default: false t.boolean "repository_checks_enabled", default: false
...@@ -84,10 +84,10 @@ ActiveRecord::Schema.define(version: 20160712171823) do ...@@ -84,10 +84,10 @@ ActiveRecord::Schema.define(version: 20160712171823) do
t.string "health_check_access_token" t.string "health_check_access_token"
t.boolean "send_user_confirmation_email", default: false t.boolean "send_user_confirmation_email", default: false
t.integer "container_registry_token_expire_delay", default: 5 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.text "after_sign_up_text"
t.string "repository_storage", default: "default" t.string "repository_storage", default: "default"
t.string "enabled_git_access_protocol" t.string "enabled_git_access_protocol"
t.boolean "user_default_external", default: false, null: false
end end
create_table "audit_events", force: :cascade do |t| create_table "audit_events", force: :cascade do |t|
...@@ -165,8 +165,8 @@ ActiveRecord::Schema.define(version: 20160712171823) do ...@@ -165,8 +165,8 @@ ActiveRecord::Schema.define(version: 20160712171823) do
t.text "artifacts_metadata" t.text "artifacts_metadata"
t.integer "erased_by_id" t.integer "erased_by_id"
t.datetime "erased_at" t.datetime "erased_at"
t.datetime "artifacts_expire_at"
t.string "environment" t.string "environment"
t.datetime "artifacts_expire_at"
t.integer "artifacts_size" t.integer "artifacts_size"
end end
...@@ -481,11 +481,10 @@ ActiveRecord::Schema.define(version: 20160712171823) do ...@@ -481,11 +481,10 @@ ActiveRecord::Schema.define(version: 20160712171823) do
t.string "state" t.string "state"
t.integer "iid" t.integer "iid"
t.integer "updated_by_id" t.integer "updated_by_id"
t.integer "moved_to_id"
t.boolean "confidential", default: false t.boolean "confidential", default: false
t.datetime "deleted_at" t.datetime "deleted_at"
t.date "due_date" t.date "due_date"
t.integer "lock_version", default: 0, null: false t.integer "moved_to_id"
end end
add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree
...@@ -625,7 +624,6 @@ ActiveRecord::Schema.define(version: 20160712171823) do ...@@ -625,7 +624,6 @@ ActiveRecord::Schema.define(version: 20160712171823) do
t.integer "merge_user_id" t.integer "merge_user_id"
t.string "merge_commit_sha" t.string "merge_commit_sha"
t.datetime "deleted_at" t.datetime "deleted_at"
t.integer "lock_version", default: 0, null: false
end end
add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree
...@@ -775,10 +773,10 @@ ActiveRecord::Schema.define(version: 20160712171823) do ...@@ -775,10 +773,10 @@ ActiveRecord::Schema.define(version: 20160712171823) do
t.integer "user_id", null: false t.integer "user_id", null: false
t.string "token", null: false t.string "token", null: false
t.string "name", null: false t.string "name", null: false
t.boolean "revoked", default: false
t.datetime "expires_at"
t.datetime "created_at", null: false t.datetime "created_at", null: false
t.datetime "updated_at", null: false t.datetime "updated_at", null: false
t.boolean "revoked", default: false
t.datetime "expires_at"
end end
add_index "personal_access_tokens", ["token"], name: "index_personal_access_tokens_on_token", unique: true, using: :btree add_index "personal_access_tokens", ["token"], name: "index_personal_access_tokens_on_token", unique: true, using: :btree
...@@ -898,9 +896,9 @@ ActiveRecord::Schema.define(version: 20160712171823) do ...@@ -898,9 +896,9 @@ ActiveRecord::Schema.define(version: 20160712171823) do
t.string "type" t.string "type"
t.string "title" t.string "title"
t.integer "project_id" t.integer "project_id"
t.datetime "created_at", null: false t.datetime "created_at"
t.datetime "updated_at", null: false t.datetime "updated_at"
t.boolean "active", null: false t.boolean "active", default: false, null: false
t.text "properties" t.text "properties"
t.boolean "template", default: false t.boolean "template", default: false
t.boolean "push_events", default: true t.boolean "push_events", default: true
......
...@@ -89,7 +89,7 @@ Feature: Project Merge Requests ...@@ -89,7 +89,7 @@ Feature: Project Merge Requests
Then The list should be sorted by "Oldest updated" Then The list should be sorted by "Oldest updated"
@javascript @javascript
Scenario: Visiting Merge Requests from a different Project after sorting Scenario: Visiting Merge Requests from a differente Project after sorting
Given I visit project "Shop" merge requests page Given I visit project "Shop" merge requests page
And I sort the list by "Oldest updated" And I sort the list by "Oldest updated"
And I visit dashboard merge requests page And I visit dashboard merge requests page
......
...@@ -120,17 +120,6 @@ describe 'Issues', feature: true do ...@@ -120,17 +120,6 @@ describe 'Issues', feature: true do
expect(page).to have_content date.to_s(:medium) expect(page).to have_content date.to_s(:medium)
end end
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
end end
......
...@@ -17,16 +17,5 @@ feature 'Edit Merge Request', feature: true do ...@@ -17,16 +17,5 @@ feature 'Edit Merge Request', feature: true do
it 'form should have class js-quick-submit' do it 'form should have class js-quick-submit' do
expect(page).to have_selector('.js-quick-submit') expect(page).to have_selector('.js-quick-submit')
end 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
end end
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