Merge pull request #667 from alphagov/make-verify-idempotent

Make signature verification idempotent

Author: pixeltrix

app/controllers/signatures_controller.rb

@@ -48,21 +48,17 @@ def create
   end
 
   def signed
-    if @signature.seen_signed_confirmation_page?
-      redirect_to petition_url(@petition)
-    else
+    unless @signature.seen_signed_confirmation_page?
       @signature.mark_seen_signed_confirmation_page!
+    end
 
-      respond_to do |format|
-        format.html
-      end
+    respond_to do |format|
+      format.html
     end
   end
 
   def verify
-    if @signature.validated?
-      flash[:notice] = "You’ve already signed this petition"
-    else
+    unless @signature.validated?
       @signature.validate!
     end
 

app/controllers/sponsors_controller.rb

@@ -8,25 +8,13 @@ class SponsorsController < SignaturesController
   before_action :validate_creator, only: [:new]
 
   def verify
-    if @signature.validated?
-      flash[:notice] = "You’ve already supported this petition"
-    else
+    unless @signature.validated?
       @signature.validate!
     end
 
     redirect_to signed_sponsor_url(@signature, token: @signature.perishable_token)
   end
 
-  def signed
-    unless @signature.seen_signed_confirmation_page?
-      @signature.mark_seen_signed_confirmation_page!
-    end
-
-    respond_to do |format|
-      format.html
-    end
-  end
-
   private
 
   def retrieve_petition

features/laura_signs_charlies_petition_as_a_sponsor.feature

@@ -80,8 +80,9 @@ Feature: As Laura, a sponsor of my friend Charlie's petition
     When I have sponsored a petition
     When I confirm my email address
     Then I should see a heading called "Thanks"
+    And I should see "Your signature has been added to this petition as a supporter"
     And I should have fully signed the petition as a sponsor
-    When I confirm my email address
+    When I confirm my email address again
     Then I should see a heading called "Thanks"
-    And I should see "You’ve already supported this petition"
+    And I should see "Your signature has been added to this petition as a supporter"
     And I should see /This petition needs [0-9]+ supporters to go live/

features/step_definitions/signature_steps.rb

@@ -20,16 +20,15 @@
   expect(find_email(address, with_subject: "Please confirm your email address")).to be_present
 end
 
-When(/^I confirm my email address$/) do
+When(/^I confirm my email address(?: again)?$/) do
   steps %Q(
     And I open the email with subject "Please confirm your email address"
     When I click the first link in the email
   )
 end
 
 def should_be_signature_count_of(count)
-  visit petition_url(@petition)
-  expect(page).to have_css("p.signature-count-number", :text => count.to_s + " signatures")
+  expect(Petition.find(@petition.id).signature_count).to eq(count)
 end
 
 Then /^I should have signed the petition$/ do

features/suzie_signs_a_petition.feature

@@ -104,12 +104,13 @@ Feature: Suzie signs a petition
     When I fill in my details and sign a petition
     And I confirm my email address
     And I should see "2 signatures"
+    And I should see "We've added your signature to the petition"
     And I can click on a link to return to the petition
     And I should have signed the petition
-    When I confirm my email address
+    When I confirm my email address again
     And I should see "2 signatures"
+    And I should see "We've added your signature to the petition"
     And I can click on a link to return to the petition
-    Then I should see that I have already signed the petition
 
   Scenario: Eric clicks the link shared to him by Suzie
     When Suzie has already signed the petition and validated her email

spec/controllers/signatures_controller_spec.rb

@@ -595,8 +595,8 @@
       context "and the signature has already been validated" do
         let(:signature) { FactoryBot.create(:validated_signature, petition: petition) }
 
-        it "sets the flash :notice message" do
-          expect(flash[:notice]).to eq("You’ve already signed this petition")
+        it "doesn't set the flash :notice message" do
+          expect(flash[:notice]).to be_nil
         end
       end
     end
@@ -767,8 +767,8 @@
       context "and the signature has already seen the confirmation page" do
         let(:signature) { FactoryBot.create(:validated_signature, petition: petition) }
 
-        it "redirects to the petition page" do
-          expect(response).to redirect_to("/petitions/#{petition.id}")
+        it "doesn't redirect to the petition page" do
+          expect(response).not_to redirect_to("/petitions/#{petition.id}")
         end
       end
     end

spec/controllers/sponsors_controller_spec.rb

@@ -645,8 +645,8 @@
         context "and the signature has already been validated" do
           let(:signature) { FactoryBot.create(:validated_signature, petition: petition, sponsor: true) }
 
-          it "sets the flash :notice message" do
-            expect(flash[:notice]).to eq("You’ve already supported this petition")
+          it "doesn't set the flash :notice message" do
+            expect(flash[:notice]).to be_nil
           end
         end