Merge pull request #1450 from 18F/ab-add-errors-to-pkey-form

Add errors to FormResponse in PersonalKeyForm

app/forms/personal_key_form.rb

@@ -5,13 +5,15 @@ class PersonalKeyForm
   attr_accessor :personal_key
   attr_reader :user
 
+  validate :check_personal_key
+
   def initialize(user, personal_key = nil)
     @user = user
     @personal_key = normalize_personal_key(personal_key)
   end
 
   def submit
-    @success = valid_personal_key?
+    @success = valid?
 
     if success
       UpdateUser.new(user: user, attributes: { personal_key: nil }).call

app/forms/reactivate_account_form.rb

@@ -54,12 +54,12 @@ def validate_password_reset_profile
   end
 
   def validate_password
-    return if password.blank? || valid_password?
+    return if valid_password?
     errors.add :password, :password_incorrect
   end
 
   def validate_personal_key
-    return if personal_key.blank? || (valid_personal_key? && personal_key_decrypts?)
+    return check_personal_key if personal_key_decrypts?
     errors.add :personal_key, :personal_key_incorrect
   end
 

app/services/personal_key_formatter.rb

@@ -1,11 +1,15 @@
 class PersonalKeyFormatter
-  def regexp
-    char_count = RandomPhrase::WORD_LENGTH
-    word_count = Figaro.env.recovery_code_length.to_i
-    valid_char = '[a-zA-Z0-9]'
-    regexp =
-      "(?:#{valid_char}{#{char_count}}([\s-])?){#{word_count - 1}}#{valid_char}{#{char_count}}"
+  CHAR_COUNT = RandomPhrase::WORD_LENGTH
+  WORD_COUNT = Figaro.env.recovery_code_length.to_i
+  VALID_CHAR = '[a-zA-Z0-9]'.freeze
+  VALID_WORD = "#{VALID_CHAR}{#{CHAR_COUNT}}".freeze
+  REGEXP = "(?:#{VALID_WORD}([\s-])?){#{WORD_COUNT - 1}}#{VALID_WORD}".freeze
 
-    regexp
+  def self.regexp
+    /\A#{REGEXP}\Z/
+  end
+
+  def self.regexp_string
+    REGEXP
   end
 end

app/validators/personal_key_validator.rb

@@ -1,20 +1,20 @@
 module PersonalKeyValidator
   extend ActiveSupport::Concern
 
-  included do
-    validate :valid_personal_key?
-  end
-
   private
 
   def normalize_personal_key(personal_key = nil)
     return nil if personal_key.blank?
     personal_key_generator.normalize(personal_key)
   end
 
-  def valid_personal_key?
-    return false unless personal_key =~ /\A#{PersonalKeyFormatter.new.regexp}\Z/
-    personal_key_generator.verify(personal_key)
+  def check_personal_key
+    return if personal_key_format_matches? && personal_key_generator.verify(personal_key)
+    errors.add :personal_key, :personal_key_incorrect
+  end
+
+  def personal_key_format_matches?
+    personal_key =~ PersonalKeyFormatter.regexp
   end
 
   def personal_key_generator

app/views/shared/_personal_key_confirmation_modal.html.slim

@@ -14,7 +14,7 @@
           autocomplete='off'
           class='col-12 mb4 border-dashed field monospace personal-key caps'
           name="personal-key"
-          pattern="^#{PersonalKeyFormatter.new.regexp}$"
+          pattern="^#{PersonalKeyFormatter.regexp_string}$"
           required=true
           spellcheck='false'
           type='text')

spec/forms/personal_key_form_spec.rb

@@ -20,14 +20,15 @@
 
     context 'when the form is invalid' do
       it 'returns FormResponse with success: false' do
-        user = build_stubbed(:user, :signed_up, personal_key: 'code')
+        user = create(:user, :signed_up, personal_key: 'code')
+        errors = { personal_key: ['Incorrect personal key'] }
 
         form = PersonalKeyForm.new(user, 'foo')
         result = instance_double(FormResponse)
         extra = { multi_factor_auth_method: 'personal key' }
 
         expect(FormResponse).to receive(:new).
-          with(success: false, errors: {}, extra: extra).and_return(result)
+          with(success: false, errors: errors, extra: extra).and_return(result)
         expect(form.submit).to eq result
         expect(user.personal_key).to_not be_nil
         expect(form.personal_key).to be_nil

spec/forms/reactivate_account_form_spec.rb

@@ -29,7 +29,7 @@
 
       it 'is invalid' do
         expect(subject).to_not be_valid
-        expect(subject.errors[:personal_key]).to eq [t('errors.messages.blank')]
+        expect(subject.errors[:personal_key]).to include t('errors.messages.blank')
         expect(subject.errors[:password]).to eq [t('errors.messages.blank')]
       end
     end