Skip to content

Commit

Permalink
Fix race condition when locking a petition
Browse files Browse the repository at this point in the history
When two moderators go to edit a petition at the same time they can
both try to obtain locks thinking the petition is unlocked. Rather
than one of them blowing up with a 500 error just return false and
the JSON returned will trigger the JS locking script to automatically
popup the petition locked by someone else screen.
  • Loading branch information
pixeltrix committed Sep 18, 2017
1 parent 489c587 commit 2365908
Show file tree
Hide file tree
Showing 6 changed files with 18 additions and 14 deletions.
2 changes: 1 addition & 1 deletion app/models/archived/petition.rb
Expand Up @@ -313,7 +313,7 @@ def update_lock!(user, now = Time.current)
def checkout!(user, now = Time.current)
with_lock do
if locked_by.present? && locked_by != user
raise RuntimeError, "Petition already being edited by #{locked_by.pretty_name}"
false
else
update!(locked_by: user, locked_at: now)
end
Expand Down
2 changes: 1 addition & 1 deletion app/models/petition.rb
Expand Up @@ -678,7 +678,7 @@ def update_lock!(user, now = Time.current)
def checkout!(user, now = Time.current)
with_lock do
if locked_by.present? && locked_by != user
raise RuntimeError, "Petition already being edited by #{locked_by.pretty_name}"
false
else
update!(locked_by: user, locked_at: now)
end
Expand Down
8 changes: 6 additions & 2 deletions spec/controllers/admin/archived/locks_controller_spec.rb
Expand Up @@ -143,13 +143,17 @@
it "doesn't update the locked_by association" do
expect {
get :create, petition_id: petition.to_param, format: :json
}.to raise_error(RuntimeError, /Petition already being edited/)
}.not_to change {
petition.reload.locked_by
}
end

it "doesn't update the locked_at timestamp" do
expect {
get :create, petition_id: petition.to_param, format: :json
}.to raise_error(RuntimeError, /Petition already being edited/)
}.not_to change {
petition.reload.locked_at
}
end
end
end
Expand Down
8 changes: 6 additions & 2 deletions spec/controllers/admin/locks_controller_spec.rb
Expand Up @@ -143,13 +143,17 @@
it "doesn't update the locked_by association" do
expect {
get :create, petition_id: petition.to_param, format: :json
}.to raise_error(RuntimeError, /Petition already being edited/)
}.not_to change {
petition.reload.locked_by
}
end

it "doesn't update the locked_at timestamp" do
expect {
get :create, petition_id: petition.to_param, format: :json
}.to raise_error(RuntimeError, /Petition already being edited/)
}.not_to change {
petition.reload.locked_at
}
end
end
end
Expand Down
6 changes: 2 additions & 4 deletions spec/models/archived/petition_spec.rb
Expand Up @@ -941,10 +941,8 @@
let(:other_user) { FactoryGirl.create(:moderator_user) }
let(:petition) { FactoryGirl.create(:petition, locked_by: other_user, locked_at: 1.hour.ago) }

it "raises an error" do
expect {
petition.checkout!(current_user)
}.to raise_error(RuntimeError, /Petition already being edited/)
it "returns false" do
expect(petition.checkout!(current_user)).to eq(false)
end
end

Expand Down
6 changes: 2 additions & 4 deletions spec/models/petition_spec.rb
Expand Up @@ -2649,10 +2649,8 @@
let(:other_user) { FactoryGirl.create(:moderator_user) }
let(:petition) { FactoryGirl.create(:petition, locked_by: other_user, locked_at: 1.hour.ago) }

it "raises an error" do
expect {
petition.checkout!(current_user)
}.to raise_error(RuntimeError, /Petition already being edited/)
it "returns false" do
expect(petition.checkout!(current_user)).to eq(false)
end
end

Expand Down

0 comments on commit 2365908

Please sign in to comment.