Skip to content
Merged
9 changes: 6 additions & 3 deletions app/controllers/concerns/idv_step_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,12 @@ def check_for_mail_only_outage
end

def redirect_for_mail_only
return redirect_to vendor_outage_url unless FeatureManagement.gpo_verification_enabled?

redirect_to idv_mail_only_warning_url
policy = Idv::GpoVerifyByMailPolicy.new(current_user)
if policy.send_letter_available?
redirect_to idv_mail_only_warning_url
else
redirect_to vendor_outage_url
end
end

def pii_from_user
Expand Down
3 changes: 2 additions & 1 deletion app/controllers/concerns/rate_limit_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ def confirm_not_rate_limited_for_phone_address_verification
private

def confirm_not_rate_limited_for_phone_and_letter_address_verification
if idv_attempter_rate_limited?(:proof_address) && Idv::GpoMail.new(current_user).rate_limited?
gpo_policy = Idv::GpoVerifyByMailPolicy.new(current_user)
if idv_attempter_rate_limited?(:proof_address) && gpo_policy.rate_limited?
rate_limit_redirect!(:proof_address)
return true
end
Expand Down
7 changes: 2 additions & 5 deletions app/controllers/idv/by_mail/enter_code_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -148,11 +148,8 @@ def user_did_not_receive_letter?

def user_can_request_another_letter?
return @user_can_request_another_letter if defined?(@user_can_request_another_letter)
gpo_mail = Idv::GpoMail.new(current_user)
@user_can_request_another_letter =
FeatureManagement.gpo_verification_enabled? &&
!gpo_mail.rate_limited? &&
!gpo_mail.profile_too_old?
policy = Idv::GpoVerifyByMailPolicy.new(current_user)
@user_can_request_another_letter = policy.resend_letter_available?
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.

153-155 became the resend_letter_available? method.

I am not convinced this memoization ever saves us anything, but 🤷

end

def last_date_letter_was_sent
Expand Down
8 changes: 4 additions & 4 deletions app/controllers/idv/by_mail/request_letter_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ def create
end
end

def gpo_mail_service
@gpo_mail_service ||= Idv::GpoMail.new(current_user)
def gpo_mail_policy
@gpo_mail_policy ||= Idv::GpoVerifyByMailPolicy.new(current_user)
end

def self.step_info
Expand All @@ -59,7 +59,7 @@ def self.step_info
private

def confirm_profile_not_too_old
redirect_to idv_path if gpo_mail_service.profile_too_old?
redirect_to idv_path if gpo_mail_policy.profile_too_old?
end

def update_tracking
Expand Down Expand Up @@ -96,7 +96,7 @@ def hours_since_first_letter(first_letter_requested_at)
end

def confirm_mail_not_rate_limited
redirect_to idv_enter_password_url if gpo_mail_service.rate_limited?
redirect_to idv_enter_password_url if gpo_mail_policy.rate_limited?
end

def resend_letter
Expand Down
17 changes: 7 additions & 10 deletions app/controllers/idv/by_mail/resend_letter_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,7 @@ class ResendLetterController < ApplicationController

before_action :confirm_two_factor_authenticated
before_action :confirm_verification_needed
before_action :confirm_mail_not_rate_limited
before_action :confirm_profile_not_too_old
before_action :confirm_resend_letter_available

def new
analytics.idv_resend_letter_visited
Expand All @@ -28,8 +27,8 @@ def create
end
end

def gpo_mail_service
@gpo_mail_service ||= Idv::GpoMail.new(current_user)
def gpo_mail_policy
@gpo_mail_policy ||= Idv::GpoVerifyByMailPolicy.new(current_user)
end

private
Expand All @@ -39,12 +38,10 @@ def confirm_verification_needed
redirect_to account_url
end

def confirm_profile_not_too_old
redirect_to idv_verify_by_mail_enter_code_path if gpo_mail_service.profile_too_old?
end

def confirm_mail_not_rate_limited
redirect_to idv_verify_by_mail_enter_code_path if gpo_mail_service.rate_limited?
def confirm_resend_letter_available
unless gpo_mail_policy.resend_letter_available?
redirect_to idv_verify_by_mail_enter_code_path
end
end

def update_tracking
Expand Down
4 changes: 2 additions & 2 deletions app/controllers/idv/phone_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -225,8 +225,8 @@ def formatted_previous_phone_step_params_phone

def gpo_letter_available
return @gpo_letter_available if defined?(@gpo_letter_available)
@gpo_letter_available ||= FeatureManagement.gpo_verification_enabled? &&
!Idv::GpoMail.new(current_user).rate_limited?
policy = Idv::GpoVerifyByMailPolicy.new(current_user)
@gpo_letter_available = policy.send_letter_available?
end

# Migrated from otp_delivery_method_controller
Expand Down
4 changes: 2 additions & 2 deletions app/controllers/idv/phone_errors_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,8 @@ def track_event(type:)
# rubocop:disable Naming/MemoizedInstanceVariableName
def set_gpo_letter_available
return @gpo_letter_available if defined?(@gpo_letter_available)
@gpo_letter_available ||= FeatureManagement.gpo_verification_enabled? &&
!Idv::GpoMail.new(current_user).rate_limited?
policy = Idv::GpoVerifyByMailPolicy.new(current_user)
@gpo_letter_available = policy.send_letter_available?
end
# rubocop:enable Naming/MemoizedInstanceVariableName
end
Expand Down
6 changes: 3 additions & 3 deletions app/controllers/vendor_outage_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ def from_idv_phone?
end

def gpo_letter_available?
FeatureManagement.gpo_verification_enabled? &&
current_user &&
!Idv::GpoMail.new(current_user).rate_limited?
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.

And this became send_letter_available?.

return false unless current_user
policy = Idv::GpoVerifyByMailPolicy.new(current_user)
policy.send_letter_available?
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.

20 and 21 (and many instances of this across these files) could be inclined to Idv::GpoVerifyByMailPolicy.new(current_user).send_letter_available?, but I'm of the school of thought that it's cleaner to do it across two simple lines, and that the memory impact of optimizing for readability is negligible.

end
end
3 changes: 2 additions & 1 deletion app/forms/gpo_verify_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ def activate_profile
end

def user_can_request_another_letter?
!Idv::GpoMail.new(user).rate_limited?
policy = Idv::GpoVerifyByMailPolicy.new(user)
policy.resend_letter_available?
end
end
Original file line number Diff line number Diff line change
@@ -1,24 +1,35 @@
# frozen_string_literal: true

module Idv
class GpoMail
attr_reader :current_user
class GpoVerifyByMailPolicy
attr_reader :user
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.

This is fun: initially this was a new class, but then I moved over enough from GpoMail that GitHub has solved the Ship of Theseus problem for us.


def initialize(current_user)
@current_user = current_user
def initialize(user)
@user = user
end

def resend_letter_available?
FeatureManagement.gpo_verification_enabled? &&
!rate_limited? &&
!profile_too_old?
end

def send_letter_available?
FeatureManagement.gpo_verification_enabled? &&
!rate_limited?
end

def rate_limited?
too_many_letter_requests_within_window? || last_letter_request_too_recent?
end

def profile_too_old?
return false if !current_user.pending_profile
return false if !user.pending_profile

min_creation_date = IdentityConfig.store.
gpo_max_profile_age_to_send_letter_in_days.days.ago

current_user.pending_profile.created_at < min_creation_date
user.pending_profile.created_at < min_creation_date
end

private
Expand All @@ -34,16 +45,16 @@ def last_not_too_recent_enabled?

def too_many_letter_requests_within_window?
return false unless window_limit_enabled?
current_user.gpo_confirmation_codes.where(
user.gpo_confirmation_codes.where(
created_at: IdentityConfig.store.max_mail_events_window_in_days.days.ago..Time.zone.now,
).count >= IdentityConfig.store.max_mail_events
end

def last_letter_request_too_recent?
return false unless last_not_too_recent_enabled?
return false unless current_user.gpo_verification_pending_profile?
return false unless user.gpo_verification_pending_profile?

current_user.gpo_verification_pending_profile.gpo_confirmation_codes.exists?(
user.gpo_verification_pending_profile.gpo_confirmation_codes.exists?(
[
'created_at > ?',
IdentityConfig.store.minimum_wait_before_another_usps_letter_in_hours.hours.ago,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@
end

it 'redirects if the user has sent too much mail' do
allow(controller.gpo_mail_service).to receive(:rate_limited?).and_return(true)
allow(controller.gpo_mail_policy).to receive(:rate_limited?).and_return(true)
allow(subject.idv_session).to receive(:address_mechanism_chosen?).
and_return(true)
get :index
Expand All @@ -69,7 +69,7 @@
end

it 'allows a user to request another letter' do
allow(controller.gpo_mail_service).to receive(:rate_limited?).and_return(false)
allow(controller.gpo_mail_policy).to receive(:rate_limited?).and_return(false)
get :index

expect(response).to be_ok
Expand Down
Loading