-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
API-41395-rep-validation-suffix-II #19071
base: master
Are you sure you want to change the base?
Conversation
…all_for_user in poa_verification and target_veteran. Removes get_suffixes method in representative, and adds suffix lookup using the identity suffix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking good, some questions below.
modules/claims_api/app/controllers/concerns/claims_api/target_veteran.rb
Outdated
Show resolved
Hide resolved
modules/claims_api/app/controllers/concerns/claims_api/user_identifier.rb
Outdated
Show resolved
Hide resolved
modules/claims_api/app/controllers/concerns/claims_api/poa_verification.rb
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we will return nil if we find a representative with a suffix in their last name.
representatives = if suffix.present? | ||
last_and_suffix = "#{last_name} #{suffix.downcase}" | ||
|
||
representatives = where('lower(first_name) = ? AND lower(last_name) = ?', first_name&.downcase, | ||
last_name&.downcase) | ||
where('lower(first_name) = ? AND lower(last_name) = ?', first_name&.downcase, | ||
last_and_suffix&.downcase) | ||
end | ||
if representatives.blank? | ||
where('lower(first_name) = ? AND lower(last_name) = ?', first_name&.downcase, | ||
last_name&.downcase) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We aren't returning anything if we find representatives with a suffix (i.e., the function will return nil if representatives
is not blank). Should we add a return statement either after line 121 or 124 5?
it 'can find a user with a suffix' do | ||
expect(Veteran::Service::Representative.all_for_user( | ||
first_name: identity.first_name, | ||
last_name: "#{identity.last_name} III", | ||
last_name: identity.last_name, | ||
suffix: 'Jr', | ||
poa_code: 'A1Q' | ||
).first.poa_codes).to include('A1Q') | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we create a test user with a suffix in their last name, so we can exercise the case where we find a user by their suffix? This test will strip the suffix (Jr
) because there is no lincoln jr
in the database, which conceals the error in our return logic.
last_name: @current_user.last_name, | ||
middle_initial:) | ||
middle_initial:, suffix:) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could keep the logic here and not modify the all_for_user
method at all. we can just do the existing lookup
first_name: @current_user.first_name,
last_name: @current_user.last_name
and if we don't find anything we can do something like
last_with_suffix = [@current_user.last_name, @current_user.suffix].compact_blank.join(' ')
first_name: @current_user.first_name,
last_name: last_with_suffix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually seeing undefined local variable or method 'suffix'
in local
Summary
Related issue(s)
Testing done
What areas of the site does it impact?
Acceptance criteria
Requested Feedback
Ideas on how to better test the suffix lookup.