LG-15440 We couldnt find records matching your address screen #12066
Conversation
changelog: User-Facing Improvements, Doc Auth, add we couldnt find your address screen
theabrad
requested review from
amirbey,
solipet,
lmgeorge,
jmax-gsa and
AShukla-GSA
| def address_warning | ||
| rate_limiter = RateLimiter.new( | ||
| user: idv_session_user, | ||
| rate_limit_type: :idv_resolution, | ||
| ) | ||
|
|
||
| @step_indicator_steps = step_indicator_steps | ||
| @address_path = idv_address_url | ||
| @remaining_submit_attempts = rate_limiter.remaining_count | ||
| log_event(based_on_limiter: rate_limiter) | ||
| end | ||
|
|
There was a problem hiding this comment.
| def address_warning | |
| rate_limiter = RateLimiter.new( | |
| user: idv_session_user, | |
| rate_limit_type: :idv_resolution, | |
| ) | |
| @step_indicator_steps = step_indicator_steps | |
| @address_path = idv_address_url | |
| @remaining_submit_attempts = rate_limiter.remaining_count | |
| log_event(based_on_limiter: rate_limiter) | |
| end | |
| def address_warning | |
| @step_indicator_steps = step_indicator_steps | |
| @address_path = idv_address_url | |
| @remaining_submit_attempts = resolution_rate_limiter.remaining_count | |
| log_event(based_on_limiter: resolution_rate_limiter) | |
| end | |
| # private | |
| def resolution_rate_limter | |
| @resolution_rate_limter ||= RateLimiter.new( | |
| user: idv_session_user, | |
| rate_limit_type: :idv_resolution, | |
| ) | |
| end | |
seems like a refactor opportunity since this rate limiter is being instantiated in 3 places... and then can update failure and warning actions 🤔
There was a problem hiding this comment.
thanks for refactoring. While perhaps choosing a different route then the code suggestion, I'm concerned that the path chosen could be more confusing from a readability standpoint because other methods in this controller also using rate_limiter for varying rate_limiter_types. As suggested, I think it would be helpful to rename to distinguish the type of rate limiter being used here ... As opposed to the refactor path chosen, I'd prefer revert to your original implementation when we replicated the same code 3 times ... it's more readable 🙏🏿
| if !residential_resolution_result.success? && (same_address_as_id == 'false' || | ||
| ipp_enrollment_in_progress) |
There was a problem hiding this comment.
what is the intention for this change? it appears that same_address_as_id is defined when updating an address during IPP 🤔
There was a problem hiding this comment.
btw, I created an issue to refactor our usage of same_address_as_id
| def set_resolution_rate_limiter | ||
| @resolution_rate_limiter = RateLimiter.new( |
There was a problem hiding this comment.
| def set_resolution_rate_limiter | |
| @resolution_rate_limiter = RateLimiter.new( | |
| def resolution_rate_limiter | |
| @resolution_rate_limiter ||= RateLimiter.new( |
| @@ -8,36 +8,37 @@ class SessionErrorsController < ApplicationController | |||
|
|
|||
| before_action :confirm_two_factor_authenticated_or_user_id_in_session | |||
| before_action :confirm_idv_session_step_needed | |||
| before_action :set_resolution_rate_limiter, only: [:warning, :address_warning, :failure] | |||
There was a problem hiding this comment.
| before_action :set_resolution_rate_limiter, only: [:warning, :address_warning, :failure] |
i think it's simpler to just call the rate limiter when needed ... is there a benefit to using the filter here?
| attr_reader :resolution_rate_limiter | ||
|
|
There was a problem hiding this comment.
| attr_reader :resolution_rate_limiter |
| elsif has_exception && is_address_exception | ||
| idv_failure_log_address_warning | ||
| redirect_to address_warning_url |
There was a problem hiding this comment.
do we know that address is the sole error?
* add address errors page * add address proofing exception redirect * add changelog changelog: User-Facing Improvements, Doc Auth, add we couldnt find your address screen * linty mclinterson * refactor rate limiter code in session_errors_controller * rename rate limiter to resolution_rate_limiter * changed to resolution result instead of residential_resolution_result * add address_exception to resolution result * move address fail check to verify_info_concern * linty mclinterson * rate limiter fixups * redirect to address warning if there is ONLY an address exception * lint fix
🎫 Ticket
Link to the relevant ticket:
LG-15440
🛠 Summary of changes
Whenever a user fails InstantVerify, because address can't be verified we want to show them a screen to try to enter their address again.
We still have to wait until the mock proofer is up to test this on the idp.
📜 Testing Plan
Provide a checklist of steps to confirm the changes.
/verify/session/errors/address_warningand confirm changes are made.👀 Screenshots