From 31793816305c8d357dc82df52ff71572af37496f Mon Sep 17 00:00:00 2001 From: ansonK Date: Thu, 30 Jul 2015 14:50:42 +0100 Subject: [PATCH 1/2] Add a redirect back to the petition show page for users that reuse the verify link in the sign petition confirmation email. If the signer shares the link to others then everyone else will also be redirected to the petition show page --- app/controllers/signatures_controller.rb | 9 ++- app/models/signature.rb | 4 ++ ..._signed_confirmation_page_to_signatures.rb | 5 ++ db/structure.sql | 5 +- features/step_definitions/common_steps.rb | 5 ++ features/step_definitions/petition_steps.rb | 2 +- features/step_definitions/signature_steps.rb | 9 +++ features/suzie_signs_a_petition.feature | 6 ++ .../controllers/signatures_controller_spec.rb | 58 ++++++++++++++----- spec/factories.rb | 10 ++-- 10 files changed, 90 insertions(+), 23 deletions(-) create mode 100644 db/migrate/20150730110838_add_seen_signed_confirmation_page_to_signatures.rb diff --git a/app/controllers/signatures_controller.rb b/app/controllers/signatures_controller.rb index df66dbdde..063ec4033 100644 --- a/app/controllers/signatures_controller.rb +++ b/app/controllers/signatures_controller.rb @@ -26,7 +26,14 @@ def signed verify_token if @signature.validated? - @petition = @signature.petition + + if @signature.seen_signed_confirmation_page? + redirect_to petition_url @signature.petition + else + @signature.mark_seen_signed_confirmation_page! + @petition = @signature.petition + end + else redirect_to(verify_signature_url(@signature, @signature.perishable_token)) end diff --git a/app/models/signature.rb b/app/models/signature.rb index 49e01d82d..731e90ca1 100644 --- a/app/models/signature.rb +++ b/app/models/signature.rb @@ -111,6 +111,10 @@ def validate! end end + def mark_seen_signed_confirmation_page! + update seen_signed_confirmation_page: true + end + def unsubscribe!(token) if unsubscribed? errors.add(:base, "Already Unsubscribed") diff --git a/db/migrate/20150730110838_add_seen_signed_confirmation_page_to_signatures.rb b/db/migrate/20150730110838_add_seen_signed_confirmation_page_to_signatures.rb new file mode 100644 index 000000000..a41b9817a --- /dev/null +++ b/db/migrate/20150730110838_add_seen_signed_confirmation_page_to_signatures.rb @@ -0,0 +1,5 @@ +class AddSeenSignedConfirmationPageToSignatures < ActiveRecord::Migration + def change + add_column :signatures, :seen_signed_confirmation_page, :boolean, null: false, default: false + end +end diff --git a/db/structure.sql b/db/structure.sql index b1fc75cd0..a240af9f7 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -462,7 +462,8 @@ CREATE TABLE signatures ( unsubscribe_token character varying, constituency_id character varying, validated_at timestamp without time zone, - number integer + number integer, + seen_signed_confirmation_page boolean DEFAULT false NOT NULL ); @@ -1154,3 +1155,5 @@ INSERT INTO schema_migrations (version) VALUES ('20150709152530'); INSERT INTO schema_migrations (version) VALUES ('20150714140659'); +INSERT INTO schema_migrations (version) VALUES ('20150730110838'); + diff --git a/features/step_definitions/common_steps.rb b/features/step_definitions/common_steps.rb index 5f042fb39..d134a8bdf 100644 --- a/features/step_definitions/common_steps.rb +++ b/features/step_definitions/common_steps.rb @@ -46,3 +46,8 @@ binding.pry :debugger end + +When(/^I click the shared link$/) do + expect(@shared_link).not_to be_blank + visit @shared_link +end diff --git a/features/step_definitions/petition_steps.rb b/features/step_definitions/petition_steps.rb index c994d3383..09d5ab339 100644 --- a/features/step_definitions/petition_steps.rb +++ b/features/step_definitions/petition_steps.rb @@ -121,7 +121,7 @@ @petition = FactoryGirl.create(:archived_petition, :rejected, title: title, reason_for_rejection: reason_for_rejection) end -When /^I view the petition$/ do +When(/^[I|they] view the petition$/) do if @petition.is_a?(ArchivedPetition) visit archived_petition_url(@petition) else diff --git a/features/step_definitions/signature_steps.rb b/features/step_definitions/signature_steps.rb index 9ddafa801..bb00a4cf4 100644 --- a/features/step_definitions/signature_steps.rb +++ b/features/step_definitions/signature_steps.rb @@ -127,6 +127,15 @@ def should_be_signature_count_of(count) :postcode => "SW14 9RQ", :name => "Sam Wibbledon") end +Given(/^Suzie has already signed the petition and validated her email$/) do + @suzies_signature = FactoryGirl.create(:validated_signature, :petition => @petition, :email => "womboid@wimbledon.com", + :postcode => "SW14 9RQ", :name => "Womboid Wibbledon") +end + +When(/^Suzie shares the signatory confirmation link with Eric$/) do + @shared_link = signed_signature_url(@suzies_signature, token: @suzies_signature.perishable_token) +end + When /^I try to sign the petition with the same email address and a different name$/ do steps %Q{ When I decide to sign the petition diff --git a/features/suzie_signs_a_petition.feature b/features/suzie_signs_a_petition.feature index 5aed69848..edf587983 100644 --- a/features/suzie_signs_a_petition.feature +++ b/features/suzie_signs_a_petition.feature @@ -103,3 +103,9 @@ Feature: Suzie signs a petition And I should see "2 signatures" 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 + And Suzie shares the signatory confirmation link with Eric + And I click the shared link + Then I view the petition diff --git a/spec/controllers/signatures_controller_spec.rb b/spec/controllers/signatures_controller_spec.rb index c7db18e21..3bdaec204 100644 --- a/spec/controllers/signatures_controller_spec.rb +++ b/spec/controllers/signatures_controller_spec.rb @@ -109,25 +109,53 @@ describe "signed" do let(:petition) { FactoryGirl.create(:petition) } + def make_signed_request(token = nil) + get :signed, id: signature.to_param, token: (token || signature.perishable_token) + end + context 'for validated signatures' do let(:signature) { FactoryGirl.create(:validated_signature, petition: petition) } - it 'renders the signed template' do - get :signed, id: signature.to_param, token: signature.perishable_token - expect(response).to be_success - expect(response).to render_template('signatures/signed') + it "raises exception if token does not match the signature" do + expect { make_signed_request "not_a_valid_token" }.to raise_error(ActiveRecord::RecordNotFound) end - it 'exposes the signature and its petition' do - get :signed, id: signature.to_param, token: signature.perishable_token - expect(assigns['signature']).to eq signature - expect(assigns['petition']).to eq petition + context 'signer has not seen the signed page before' do + before do + signature.update! seen_signed_confirmation_page: false + end + + it 'exposes the signature and its petition' do + make_signed_request + expect(assigns['signature']).to eq signature + expect(assigns['petition']).to eq petition + end + + it 'marks the signature as the signer having seen the confirmation page' do + make_signed_request + expect(signature.reload.seen_signed_confirmation_page).to be_truthy + end + + it 'renders the signed template' do + make_signed_request + expect(response).to be_success + expect(response).to render_template('signatures/signed') + end end - it "raises exception if token does not match the signature" do - expect do - get :signed, id: signature.to_param, token: "#{signature.perishable_token}a" - end.to raise_error(ActiveRecord::RecordNotFound) + context 'signer has already seen the signed page (clicking link in the email again)' do + before do + signature.mark_seen_signed_confirmation_page! + end + + it 'keeps the signature as the signer having seen the confirmation page' do + expect { make_signed_request }.not_to change(signature.reload, :seen_signed_confirmation_page) + end + + it 'redirects to the petition show page' do + make_signed_request + expect(response).to redirect_to "https://petition.parliament.uk/petitions/#{petition.id}" + end end end @@ -135,14 +163,12 @@ let(:signature) { FactoryGirl.create(:pending_signature, petition: petition) } it "redirects to the signature verify page" do - get :signed, id: signature.to_param, token: signature.perishable_token + make_signed_request expect(response).to redirect_to("https://petition.parliament.uk/signatures/#{signature.id}/verify/#{signature.perishable_token}") end it "raises exception if token does not match" do - expect do - get :signed, id: signature.to_param, token: "#{signature.perishable_token}a" - end.to raise_error(ActiveRecord::RecordNotFound) + expect { make_signed_request "not_a_valid_token" }.to raise_error(ActiveRecord::RecordNotFound) end end end diff --git a/spec/factories.rb b/spec/factories.rb index 2975a64b6..d14be830d 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -193,8 +193,9 @@ state Signature::VALIDATED_STATE after(:create) do |signature, evaluator| - if signature.petition - signature.petition.increment!(:signature_count) if signature.validated? + if signature.petition && signature.validated? + signature.petition.increment!(:signature_count) + signature.increment!(:number) end end end @@ -204,8 +205,9 @@ end factory :validated_signature, :parent => :signature do - state Signature::VALIDATED_STATE - validated_at { Time.current } + state Signature::VALIDATED_STATE + validated_at { Time.current } + seen_signed_confirmation_page true end sequence(:sponsor_email) { |n| "sponsor#{n}@example.com" } From be4cb3b6cadf5e2b3be79d0051aa3a8dba6d00e8 Mon Sep 17 00:00:00 2001 From: ansonK Date: Thu, 30 Jul 2015 15:21:12 +0100 Subject: [PATCH 2/2] remove used version of petition step def --- features/step_definitions/petition_steps.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/features/step_definitions/petition_steps.rb b/features/step_definitions/petition_steps.rb index 09d5ab339..cb4c3fcd4 100644 --- a/features/step_definitions/petition_steps.rb +++ b/features/step_definitions/petition_steps.rb @@ -121,7 +121,7 @@ @petition = FactoryGirl.create(:archived_petition, :rejected, title: title, reason_for_rejection: reason_for_rejection) end -When(/^[I|they] view the petition$/) do +When(/^I view the petition$/) do if @petition.is_a?(ArchivedPetition) visit archived_petition_url(@petition) else