Merge pull request swapmyvote#954 from stevebaxter/feature/issue-952-…

…error-reports

app/controllers/user/constituencies_controller.rb

@@ -18,7 +18,7 @@ def update
 
     else
 
-      flash.now[:errors] = ["You must tell us your constituency. Without it, the swaps we offer may not make sense"]
+      flash.now[:errors] = ["You must tell us your constituency. Without it, the swaps we offer may not make sense."]
       edit
       render "edit"
 

app/controllers/user/swaps_controller.rb

@@ -6,6 +6,7 @@ class User::SwapsController < ApplicationController
   before_action :assert_parties_exist, only: [:show]
   before_action :assert_has_email, only: [:new, :create, :update]
   before_action :assert_has_constituency, only: [:new, :create, :update]
+  before_action :assert_mobile_phone_present, only: [:new, :create, :update]
   before_action :assert_mobile_phone_verified, only: [:new, :create, :update]
   before_action :hide_polls?, only: [:show, :new]
 
@@ -57,6 +58,13 @@ def destroy
 
   private
 
+  def assert_mobile_phone_present
+    return unless @user.mobile_phone.blank?
+
+    flash[:errors] = ["Please enter your mobile phone number before you swap"]
+    redirect_to edit_user_path
+  end
+
   def assert_mobile_phone_verified
     return unless @user.mobile_verification_missing?
 
@@ -66,14 +74,14 @@ def assert_mobile_phone_verified
   def assert_has_email
     return unless @user.email.blank?
 
-    flash[:errors] = ["Please enter your email address before you swap!"]
+    flash[:errors] = ["Please enter your email address before you swap"]
     redirect_to edit_user_path
   end
 
   def assert_has_constituency
     return unless @user.constituency_ons_id.blank?
 
-    flash[:errors] = ["Please enter your postcode or constituency before you swap!"]
+    flash[:errors] = ["Please enter your postcode or constituency before you swap"]
     redirect_to edit_user_path
   end
 

app/models/user.rb

@@ -33,9 +33,14 @@ class User < ApplicationRecord
   # Additional validation for emails, :validatable has already added basic validation
   validate :email_uniqueness, on: :create
 
+  # Required fields
   validates :name, presence: true
+  validates :email, presence: true
   validates :consent_to_data_processing, acceptance: true
 
+  # Name cannot be the same as email
+  validate :check_email_and_name
+
   include UsersHelper
   include ::UserErrorsConcern
 
@@ -391,6 +396,10 @@ def email_uniqueness
     email_uniqueness_errors(existing_user)
   end
 
+  def check_email_and_name
+    errors.add(:name, "can't be the same as email") if email == name
+  end
+
   def find_existing_email(email)
     if id
       # Ignore self in the uniqueness check

app/views/devise/passwords/new.html.erb

@@ -9,7 +9,7 @@
         <%= render "devise/shared/error_messages", resource: resource %>
 
         <%= form_for(resource, as: resource_name, url: password_path(resource_name), html: { method: :post }) do |f| %>
-          <div class="form-group">
+          <div class="form-group required">
             <%= f.label :email, class: "mb-0" %>
             <%= f.email_field :email, autofocus: true, autocomplete: "email", class: "form-control" %>
           </div>

app/views/mobile_phone/_form.html.haml

@@ -1,4 +1,4 @@
-.form-group.form-group-short
+.form-group.form-group-short.required
   %label.mb-0 My mobile number is
 
   = telephone_field_tag "mobile_phone[full]", mobile_number, size: 14, class: "form-control"

app/views/mobile_phone/verify_token.html.haml

@@ -3,14 +3,15 @@
     - if @new_verification
       .card
         .card-header
-          %h1.h4.mb-0 Verification successful!
+          %h1.h4.mb-0 Verification successful
 
         .card-body
           %p
             Thank you for verifying your mobile phone number.
 
-          = link_to user_path do
-            = button_tag "Continue", class: 'btn btn-primary'
+          .d-flex.justify-content-center
+            = link_to user_path do
+              = button_tag "Continue", class: 'btn btn-primary'
 
     - else
       .card
@@ -21,5 +22,6 @@
           %p
             Your mobile phone number was already verified.
 
-          = link_to user_path do
-            = button_tag "Continue", class: 'btn btn-primary'
+          .d-flex.justify-content-center
+            = link_to user_path do
+              = button_tag "Continue", class: 'btn btn-primary'

app/views/users/edit.html.haml

@@ -10,23 +10,23 @@
           Edit profile
       .card-body
         = form_for @user, url: user_path do |f|
-          .form-group
+          .form-group.required
             %label.mb-0 My preferred party is
             = f.collection_select :preferred_party_id, @parties,
                                   :id, :name,
                                   { prompt: "...",
                                     selected: @user.preferred_party_id },
                                   { class: "form-control" }
 
-          .form-group
+          .form-group.required
             %label.mb-0 but I'm willing to vote for
             = f.collection_select :willing_party_id, @parties,
                                   :id, :name,
                                   { prompt: "...",
                                     selected: @user.willing_party_id },
                                   { class: "form-control" }
 
-          .form-group
+          .form-group.required
             %label.mb-0 My constituency is
             %br
             = f.collection_select :constituency_ons_id, @constituencies,
@@ -37,7 +37,7 @@
 
           = render partial: "postcode_field"
 
-          .form-group
+          .form-group.required
             = render partial: "users/email_field", locals: { f: f }
 
           = render partial: 'mobile_phone/form',

spec/controllers/user/swaps_controller_spec.rb

@@ -5,7 +5,8 @@
 
   context "when user has a potential swap" do
     let(:new_user) do
-      build(:user, id: 121, email: "foo@bar.com")
+      build(:user, id: 121, email: "foo@bar.com",
+      mobile_phone: build(:mobile_phone, user_id: 122, number: "07400 123456", verified: false))
     end
 
     let(:swap_user) do
@@ -270,11 +271,8 @@
     let(:new_user) do
       build(:user, id: 122, name: "the new user",
             constituency: build(:ons_constituency, ons_id: "E121"),
-            email: "foo@bar.com")
-    end
-
-    let(:mobile_phone) do
-      build(:mobile_phone, user_id: 122, number: "07400 123456", verified: false)
+            email: "foo@bar.com",
+            mobile_phone: build(:mobile_phone, user_id: 122, number: "07400 123456", verified: false))
     end
 
     let(:swap_user) do
@@ -329,8 +327,57 @@
             email: "")
     end
 
-    let(:mobile_phone) do
-      build(:mobile_phone, user_id: 122, number: "07400 123456", verified: true)
+    let(:swap_user) do
+      build(:user, id: 132,
+            constituency: build(:ons_constituency, name: "Fareham", ons_id: "E131"),
+            email: "match@foo.com")
+    end
+
+    let(:an_email) { double(:an_email) }
+
+    before do
+      allow(request.env["warden"]).to receive(:authenticate!).and_return(new_user)
+      allow(controller).to receive(:current_user).and_return(new_user)
+
+      allow(User).to receive(:find).with(swap_user.id.to_s)
+                       .and_return(swap_user)
+      allow(new_user).to receive(:mobile_phone_verified?).and_return(true)
+    end
+
+    describe "POST #create" do
+      it "redirects to user page" do
+        expect(new_user.swap).to be_nil
+
+        post :create, params: { user_id: swap_user.id }
+
+        expect(response).to redirect_to :edit_user
+        expect(flash[:errors].first).to eq "Please enter your email address before you swap"
+        expect(new_user.swap).to be_nil
+      end
+    end
+
+    describe "PUT #update" do
+      it "redirects to user page" do
+        swap = Swap.create(chosen_user_id: swap_user.id)
+        new_user.incoming_swap = swap
+        swap_user.outgoing_swap = swap
+
+        expect(swap_user.swap.confirmed).to be nil
+
+        put :update, params: { swap: { confirmed: true } }
+
+        expect(response).to redirect_to :edit_user
+        expect(flash[:errors].first).to eq "Please enter your email address before you swap"
+        expect(swap_user.swap.confirmed).to be nil
+      end
+    end
+  end
+
+  context "when users don't have an mobile number" do
+    let(:new_user) do
+      build(:user, id: 122,
+            constituency: build(:ons_constituency, ons_id: "E121"),
+            email: "foo@bar.com")
     end
 
     let(:swap_user) do
@@ -357,13 +404,13 @@
         post :create, params: { user_id: swap_user.id }
 
         expect(response).to redirect_to :edit_user
-        expect(flash[:errors].first).to eq "Please enter your email address before you swap!"
+        expect(flash[:errors].first).to eq "Please enter your mobile phone number before you swap"
         expect(new_user.swap).to be_nil
       end
     end
 
     describe "PUT #update" do
-      it "confirms the swap if all ducks are lined up" do
+      it "redirects to user page" do
         swap = Swap.create(chosen_user_id: swap_user.id)
         new_user.incoming_swap = swap
         swap_user.outgoing_swap = swap
@@ -373,7 +420,7 @@
         put :update, params: { swap: { confirmed: true } }
 
         expect(response).to redirect_to :edit_user
-        expect(flash[:errors].first).to eq "Please enter your email address before you swap!"
+        expect(flash[:errors].first).to eq "Please enter your mobile phone number before you swap"
         expect(swap_user.swap.confirmed).to be nil
       end
     end