Merge pull request #624 from alphagov/fix-release-lock

Fix a couple of bugs with admin petition locking

app/assets/javascripts/edit_lock.js

@@ -1,7 +1,7 @@
 (function ($) {
   'use strict';
 
-  $.fn.editLock = function(id, user_id, path) {
+  $.fn.editLock = function(id, user_id, path, moderated) {
     var $html = this;
     var $message = $html.find('.edit-lock-message');
     var $override = $html.find('#edit-lock-override');
@@ -12,6 +12,7 @@
     var PATH = path;
     var INTERVAL = 10000;
     var LOCK_URL = PATH + '/' + ID + '/lock.json';
+    var MODERATED = moderated;
 
     var EditLock = {
       processStatus: function(data) {
@@ -59,7 +60,11 @@
       },
 
       cancelClicked: function(e) {
-        window.location = PATH + '/' + ID;
+        if (MODERATED) {
+          window.location = PATH + '/' + ID;
+        } else {
+          window.history.back();
+        }
       }
     };
 

app/models/archived/petition.rb

@@ -326,15 +326,12 @@ def force_checkout!(user, now = Time.current)
 
     def release!(user)
       with_lock do
-        if locked_by.present? && locked_by != user
-          raise RuntimeError, "Petition already being edited by #{locked_by.pretty_name}"
-        else
+        if locked_by.present? && locked_by == user
           update!(locked_by: nil, locked_at: nil)
         end
       end
     end
 
-
     private
 
     def evaluate_debate_state

app/models/petition.rb

@@ -691,9 +691,7 @@ def force_checkout!(user, now = Time.current)
 
   def release!(user)
     with_lock do
-      if locked_by.present? && locked_by != user
-        raise RuntimeError, "Petition already being edited by #{locked_by.pretty_name}"
-      else
+      if locked_by.present? && locked_by == user
         update!(locked_by: nil, locked_at: nil)
       end
     end

app/views/admin/admin/_edit_lock.html.erb

@@ -13,9 +13,9 @@
 <script>
   $(document).ready(function() {
     <% if @petition.is_a?(::Archived::Petition) %>
-      $("#edit-lock").editLock(<%= @petition.id %>, <%= current_user.id %>, '/admin/archived/petitions');
+      $("#edit-lock").editLock(<%= @petition.id %>, <%= current_user.id %>, '/admin/archived/petitions', true);
     <% else %>
-      $("#edit-lock").editLock(<%= @petition.id %>, <%= current_user.id %>, '/admin/petitions');
+      $("#edit-lock").editLock(<%= @petition.id %>, <%= current_user.id %>, '/admin/petitions', <%= @petition.moderated? %>);
     <% end %>
   });
 </script>

spec/controllers/admin/archived/locks_controller_spec.rb

@@ -285,13 +285,17 @@
         it "doesn't update the locked_by association" do
           expect {
             get :destroy, 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 :destroy, 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

spec/controllers/admin/locks_controller_spec.rb

@@ -285,13 +285,17 @@
         it "doesn't update the locked_by association" do
           expect {
             get :destroy, 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 :destroy, 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

spec/models/archived/petition_spec.rb

@@ -848,4 +848,254 @@
       end
     end
   end
+
+  describe "#update_lock!" do
+    let(:current_user) { FactoryGirl.create(:moderator_user) }
+
+    context "when the petition is not locked" do
+      let(:petition) { FactoryGirl.create(:petition, locked_by: nil, locked_at: nil) }
+
+      it "doesn't update the locked_by association" do
+        expect {
+          petition.update_lock!(current_user)
+        }.not_to change {
+          petition.reload.locked_by
+        }
+      end
+
+      it "doesn't update the locked_at timestamp" do
+        expect {
+          petition.update_lock!(current_user)
+        }.not_to change {
+          petition.reload.locked_at
+        }
+      end
+    end
+
+    context "when the petition is locked by someone else" do
+      let(:other_user) { FactoryGirl.create(:moderator_user) }
+      let(:petition) { FactoryGirl.create(:petition, locked_by: other_user, locked_at: 1.hour.ago) }
+
+      it "doesn't update the locked_by association" do
+        expect {
+          petition.update_lock!(current_user)
+        }.not_to change {
+          petition.reload.locked_by
+        }
+      end
+
+      it "doesn't update the locked_at timestamp" do
+        expect {
+          petition.update_lock!(current_user)
+        }.not_to change {
+          petition.reload.locked_at
+        }
+      end
+    end
+
+    context "when the petition is locked by the current user" do
+      let(:petition) { FactoryGirl.create(:petition, locked_by: current_user, locked_at: 1.hour.ago) }
+
+      it "doesn't update the locked_by association" do
+        expect {
+          petition.update_lock!(current_user)
+        }.not_to change {
+          petition.reload.locked_by
+        }
+      end
+
+      it "updates the locked_at timestamp" do
+        expect {
+          petition.update_lock!(current_user)
+        }.to change {
+          petition.reload.locked_at
+        }.to be_within(1.second).of(Time.current)
+      end
+    end
+  end
+
+  describe "#checkout!" do
+    let(:current_user) { FactoryGirl.create(:moderator_user) }
+
+    context "when the petition is not locked" do
+      let(:petition) { FactoryGirl.create(:petition, locked_by: nil, locked_at: nil) }
+
+      it "updates the locked_by association" do
+        expect {
+          petition.checkout!(current_user)
+        }.to change {
+          petition.reload.locked_by
+        }.from(nil).to(current_user)
+      end
+
+      it "updates the locked_at timestamp" do
+        expect {
+          petition.checkout!(current_user)
+        }.to change {
+          petition.reload.locked_at
+        }.from(nil).to(be_within(1.second).of(Time.current))
+      end
+    end
+
+    context "when the petition is locked by someone else" do
+      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/)
+      end
+    end
+
+    context "when the petition is locked by the current user" do
+      let(:petition) { FactoryGirl.create(:petition, locked_by: current_user, locked_at: 1.hour.ago) }
+
+      it "doesn't update the locked_by association" do
+        expect {
+          petition.checkout!(current_user)
+        }.not_to change {
+          petition.reload.locked_by
+        }
+      end
+
+      it "updates the locked_at timestamp" do
+        expect {
+          petition.checkout!(current_user)
+        }.to change {
+          petition.reload.locked_at
+        }.to be_within(1.second).of(Time.current)
+      end
+    end
+  end
+
+  describe "#force_checkout!" do
+    let(:current_user) { FactoryGirl.create(:moderator_user) }
+
+    context "when the petition is not locked" do
+      let(:petition) { FactoryGirl.create(:petition, locked_by: nil, locked_at: nil) }
+
+      it "updates the locked_by association" do
+        expect {
+          petition.force_checkout!(current_user)
+        }.to change {
+          petition.reload.locked_by
+        }.from(nil).to(current_user)
+      end
+
+      it "updates the locked_at timestamp" do
+        expect {
+          petition.force_checkout!(current_user)
+        }.to change {
+          petition.reload.locked_at
+        }.from(nil).to(be_within(1.second).of(Time.current))
+      end
+    end
+
+    context "when the petition is locked by someone else" do
+      let(:other_user) { FactoryGirl.create(:moderator_user) }
+      let(:petition) { FactoryGirl.create(:petition, locked_by: other_user, locked_at: 1.hour.ago) }
+
+      it "updates the locked_by association" do
+        expect {
+          petition.force_checkout!(current_user)
+        }.to change {
+          petition.reload.locked_by
+        }.from(other_user).to(current_user)
+      end
+
+      it "updates the locked_at timestamp" do
+        expect {
+          petition.force_checkout!(current_user)
+        }.to change {
+          petition.reload.locked_at
+        }.to(be_within(1.second).of(Time.current))
+      end
+    end
+
+    context "when the petition is locked by the current user" do
+      let(:petition) { FactoryGirl.create(:petition, locked_by: current_user, locked_at: 1.hour.ago) }
+
+      it "doesn't update the locked_by association" do
+        expect {
+          petition.force_checkout!(current_user)
+        }.not_to change {
+          petition.reload.locked_by
+        }
+      end
+
+      it "updates the locked_at timestamp" do
+        expect {
+          petition.force_checkout!(current_user)
+        }.to change {
+          petition.reload.locked_at
+          }.to be_within(1.second).of(Time.current)
+        end
+      end
+    end
+
+  describe "#release!" do
+    let(:current_user) { FactoryGirl.create(:moderator_user) }
+
+    context "when the petition is not locked" do
+      let(:petition) { FactoryGirl.create(:petition, locked_by: nil, locked_at: nil) }
+
+      it "doesn't update the locked_by association" do
+        expect {
+          petition.release!(current_user)
+        }.not_to change {
+          petition.reload.locked_by
+        }
+      end
+
+      it "doesn't update the locked_at timestamp" do
+        expect {
+          petition.release!(current_user)
+        }.not_to change {
+          petition.reload.locked_at
+        }
+      end
+    end
+
+    context "when the petition is locked by someone else" do
+      let(:other_user) { FactoryGirl.create(:moderator_user) }
+      let(:petition) { FactoryGirl.create(:petition, locked_by: other_user, locked_at: 1.hour.ago) }
+
+      it "doesn't update the locked_by association" do
+        expect {
+          petition.release!(current_user)
+        }.not_to change {
+          petition.reload.locked_by
+        }
+      end
+
+      it "doesn't update the locked_at timestamp" do
+        expect {
+          petition.release!(current_user)
+        }.not_to change {
+          petition.reload.locked_at
+        }
+      end
+    end
+
+    context "when the petition is locked by the current user" do
+      let(:petition) { FactoryGirl.create(:petition, locked_by: current_user, locked_at: 1.hour.ago) }
+
+      it "updates the locked_by association" do
+        expect {
+          petition.release!(current_user)
+        }.to change {
+          petition.reload.locked_by
+        }.from(current_user).to(nil)
+      end
+
+      it "updates the locked_at timestamp" do
+        expect {
+          petition.release!(current_user)
+        }.to change {
+          petition.reload.locked_at
+        }.to be_nil
+      end
+    end
+  end
 end