Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions app/controllers/idv/address_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,14 @@ class AddressController < ApplicationController
def new
analytics.idv_address_visit

@presenter = AddressPresenter.new(pii: idv_session.pii_from_doc)
@address_form = Idv::AddressForm.new(idv_session.pii_from_doc)
@presenter = AddressPresenter.new
end

def update
clear_future_steps!
form_result = idv_form.submit(profile_params)
@address_form = Idv::AddressForm.new(idv_session.pii_from_doc)
form_result = @address_form.submit(profile_params)
analytics.idv_address_submitted(**form_result.to_h)
capture_address_edited(form_result)
if form_result.success?
Expand Down Expand Up @@ -44,14 +46,19 @@ def idv_form
end

def success
profile_params.each do |key, value|
idv_session.pii_from_doc[key] = value
end
idv_session.pii_from_doc = idv_session.pii_from_doc.merge(
address1: @address_form.address1,
address2: @address_form.address2,
city: @address_form.city,
state: @address_form.state,
zipcode: @address_form.zipcode,
)
redirect_to idv_verify_info_url
end

def failure
redirect_to idv_address_url
@presenter = AddressPresenter.new
render :new
end

def profile_params
Expand Down
20 changes: 14 additions & 6 deletions app/forms/idv/address_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@ class AddressForm
attr_accessor(*ATTRIBUTES)

def self.model_name
ActiveModel::Name.new(self, nil, 'Address')
ActiveModel::Name.new(self, nil, 'IdvForm')
end

def initialize(pii)
@pii = pii
set_ivars_with_pii(pii)
@address_edited = false
end

Expand All @@ -33,13 +33,21 @@ def submit(params)

private

def set_ivars_with_pii(pii)
pii = pii.symbolize_keys
@address1 = pii[:address1]
@address2 = pii[:address2]
@city = pii[:city]
@state = pii[:state]
@zipcode = pii[:zipcode]
end

def consume_params(params)
params.each do |key, value|
raise_invalid_address_parameter_error(key) unless ATTRIBUTES.include?(key.to_sym)
send(:"#{key}=", value)
if send(key) != @pii[key] && (send(key).present? || @pii[key].present?)
ATTRIBUTES.each do |attribute_name|
if send(attribute_name).to_s != params[attribute_name].to_s
@address_edited = true
end
send(:"#{attribute_name}=", params[attribute_name].to_s)
end
Comment on lines +46 to 51
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is very similar to set_ivars_with_pii except this sets @address_edited = true, do you think it would be worth trying to combine? or nah

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made an effort to combine and struggled with a good way to do it well. I am going to be working on this again in #10385 and I'm hoping that it will be easier when we have a struct to represent an address because then we can just compare them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'll be working on this in a follow-up to #10385 since #10385 is just for reads.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zachmargolis: I just opened #10390 which includes these changes to the AddressForm: https://github.com/18F/identity-idp/pull/10390/files#diff-1b0c80e533f13f34a865f61b08ff023a3efbd21ba6bfa03aa84e0c3d0c108128

I got it out of the business of tracking address edits. My thinking is that once we stop overwriting the address in pii_from_doc we will get a fix for this bug: https://github.com/18F/identity-idp/pull/10385/files#diff-ee8ce27b8fbb65556458f217dd0017a6418194fd31087457ac79c219f750d337R130

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

end

Expand Down
8 changes: 0 additions & 8 deletions app/presenters/idv/address_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,6 @@

module Idv
class AddressPresenter
def initialize(pii:)
@pii = pii
end

def pii
@pii
end

def address_line1_hint
"#{I18n.t('forms.example')} 150 Calle A Apt 3"
end
Expand Down
8 changes: 2 additions & 6 deletions app/views/idv/address/new.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
%>

<%= simple_form_for(
:idv_form,
@address_form,
url: idv_address_path,
method: 'POST',
html: { autocomplete: 'off', class: 'margin-top-5' },
Expand All @@ -35,7 +35,6 @@
hint_html: { class: @presenter.hint_class },
required: true,
maxlength: 255,
input_html: { value: @presenter.pii['address1'] },
) %>
<%= render ValidatedFieldComponent.new(
form: f,
Expand All @@ -45,7 +44,6 @@
hint_html: { class: @presenter.hint_class },
required: false,
maxlength: 255,
input_html: { value: @presenter.pii['address2'] },
) %>
<%= render ValidatedFieldComponent.new(
form: f,
Expand All @@ -55,15 +53,13 @@
hint_html: { class: @presenter.hint_class },
required: true,
maxlength: 255,
input_html: { value: @presenter.pii['city'] },
) %>
<%= render ValidatedFieldComponent.new(
form: f,
name: :state,
collection: us_states_territories,
label: t('idv.form.state'),
required: true,
selected: @presenter.pii['state'],
) %>
<div class="tablet:grid-col-6">
<%# using :tel for mobile numeric keypad %>
Expand All @@ -76,7 +72,7 @@
hint_html: { class: @presenter.hint_class },
required: true,
pattern: '(\d{5}([\-]\d{4})?)',
input_html: { value: @presenter.pii['zipcode'], class: 'zipcode' },
input_html: { class: 'zipcode' },
error_messages: {
patternMismatch: t('idv.errors.pattern_mismatch.zipcode'),
},
Expand Down
13 changes: 13 additions & 0 deletions spec/controllers/idv/address_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -97,5 +97,18 @@
error_details: nil },
)
end

context 'with invalid params' do
render_views

it 'renders errors if they occur' do
params[:idv_form][:zipcode] = 'this is invalid'

put :update, params: params

expect(response).to render_template(:new)
expect(response.body).to include(t('idv.errors.pattern_mismatch.zipcode'))
end
end
end
end
78 changes: 78 additions & 0 deletions spec/forms/idv/address_form_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
require 'rails_helper'

RSpec.describe Idv::AddressForm do
let(:pii) do
{
first_name: 'Test',
last_name: 'McTesterson',
address1: '123 Main St',
address2: nil,
city: 'Testertown',
state: 'TX',
zipcode: '11111',
}
end

let(:params) do
{
address1: '456 Other St',
address2: 'Apt 1',
city: 'McTestville',
state: 'IL',
zipcode: '22222',
}
end

it 'is initialized with values from the hash in the initializer' do
address_form = Idv::AddressForm.new(pii)
expect(address_form.address1).to eq('123 Main St')
expect(address_form.address2).to eq(nil)
expect(address_form.city).to eq('Testertown')
expect(address_form.state).to eq('TX')
expect(address_form.zipcode).to eq('11111')
end

describe '#submit' do
context 'with valid params' do
it 'returns a successful result' do
result = Idv::AddressForm.new(pii).submit(params)

expect(result.success?).to eq(true)
expect(result.extra[:address_edited]).to eq(true)
end
end

context 'with a malformed param' do
it 'returns an error result' do
params[:zipcode] = 'this is not a valid zipcde'

result = Idv::AddressForm.new(pii).submit(params)

expect(result.success?).to eq(false)
expect(result.errors[:zipcode]).to be_present
end
end

context 'with a missing params' do
it 'returns an error result' do
params.delete(:zipcode)

result = Idv::AddressForm.new(pii).submit(params)

expect(result.success?).to eq(false)
expect(result.errors[:zipcode]).to be_present
end
end

context 'the user submits the same address that is in the pii' do
it 'does not set `address_edited` to true' do
params = pii.slice(:address1, :address2, :city, :state, :zipcode)

result = Idv::AddressForm.new(pii).submit(params)

expect(result.success?).to eq(true)
expect(result.extra[:address_edited]).to eq(false)
end
end
end
end
3 changes: 2 additions & 1 deletion spec/views/idv/address/new.html.erb_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
let(:parsed_page) { Nokogiri::HTML.parse(rendered) }

before do
assign(:presenter, Idv::AddressPresenter.new(pii: {}))
assign(:presenter, Idv::AddressPresenter.new)
assign(:address_form, Idv::AddressForm.new({}))
render
end

Expand Down