Skip to content
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

Update veteran status determination, VIC eligibility #1536

Merged
merged 6 commits into from
Nov 16, 2017
Merged
Show file tree
Hide file tree
Changes from 4 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
2 changes: 1 addition & 1 deletion app/controllers/v0/id_card_attributes_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ def authorize
raise VIC::IDCardAttributeError, status: 403, code: 'VIC002', detail: 'Unable to verify EDIPI' unless
current_user.edipi.present?
begin
unless current_user.veteran?
unless current_user.can_access_id_card?
raise VIC::IDCardAttributeError, status: 403, code: 'VIC003',
detail: 'Not eligible for a Veteran ID Card'
end
Expand Down
14 changes: 5 additions & 9 deletions app/models/emis_redis/veteran_status.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,20 @@ class VeteranStatus < Model
CLASS_NAME = 'VeteranStatusService'

def veteran?
title38_status == 'V1'
end

def title38_status
raise VeteranStatus::NotAuthorized unless @user.loa3?
response = emis_response('get_veteran_status')
raise VeteranStatus::RecordNotFound if response.empty?
any_veteran_indicator?(response.items.first)
response.items.first&.title38_status_code
end

class NotAuthorized < StandardError
end

class RecordNotFound < StandardError
end

private

def any_veteran_indicator?(item)
item.post911_deployment_indicator == 'Y' ||
item.post911_combat_indicator == 'Y' ||
item.pre911_deployment_indicator == 'Y'
end
end
end
8 changes: 8 additions & 0 deletions app/models/id_card_attributes.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# frozen_string_literal: true

class IdCardAttributes
attr_accessor :user

Expand All @@ -20,6 +21,7 @@ def traits
'zip' => @user.va_profile&.address&.postal_code || '',
'email' => @user.email,
'phone' => @user.va_profile&.home_phone || '',
'title38status' => title38_status_code,
'branchofservice' => branches_of_service,
'dischargetype' => discharge_types
}
Expand All @@ -38,6 +40,12 @@ def traits
'H' => 'PHS' # USPHS
}.freeze

def title38_status_code
@user.veteran_status.title38_status || 'UNKNOWN'
rescue StandardError
'UNKNOWN'
end

def branches_of_service
branches = @user.military_information.service_episodes_by_date.map do |ep|
SERVICE_KEYS[ep.branch_of_service_code]
Expand Down
9 changes: 7 additions & 2 deletions app/models/user.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# frozen_string_literal: true

require 'common/models/base'
require 'common/models/redis_store'
require 'mvi/messages/find_profile_message'
Expand All @@ -12,6 +13,9 @@ class User < Common::RedisStore

UNALLOCATED_SSN_PREFIX = '796' # most test accounts use this

# Defined per issue #6042
ID_CARD_ALLOWED_STATUSES = %w(V1 V3 V6).freeze

redis_store REDIS_CONFIG['user_store']['namespace']
redis_ttl REDIS_CONFIG['user_store']['each_ttl']
redis_key :uuid
Expand Down Expand Up @@ -111,8 +115,9 @@ def can_prefill_emis?
end

def can_access_id_card?
Copy link
Contributor

Choose a reason for hiding this comment

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

Not specifically aimed at this PR 😄 but the User model's getting pretty hefty. Once we have policy files for authorization all the can_x? logic can move there. Maybe move ID_CARD_ALLOWED_STATUSES so all the constants are at the top of the class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed about the can_xyz methods getting crufty.

Will move the constant.

beta_enabled?(uuid, 'veteran_id_card') && loa3? && edipi.present? && veteran?
rescue # Default to false for any veteran_status error
beta_enabled?(uuid, 'veteran_id_card') && loa3? && edipi.present? &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Performance NITPICK, possibly do the loa3? check before beta_enabled? check to avoid making a db call if not necessary. Maybe even edipi since MVI is going to get fetched one way or the other if loa3.

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure why my comment got suppressed.

Performance NITPICK:

loa3? && edipi.present? && beta_enabled?(uuid, 'veteran_id_card') &&

will avoid invoking a database call unless necessary. LOA3 users will always fetch EDIPI one way or the other as part of users profile, and future hits will be a redis cache hit. Whereas for LOA1 you'd effectively be hitting the database first, every time.

ID_CARD_ALLOWED_STATUSES.include?(veteran_status.title38_status)
rescue StandardError # Default to false for any veteran_status error
false
end

Expand Down
31 changes: 26 additions & 5 deletions spec/lib/emis/veteran_status_service_spec.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# frozen_string_literal: true

require 'rails_helper'
require 'emis/veteran_status_service'
require 'emis/responses/get_veteran_status_response'
Expand All @@ -8,26 +9,37 @@
require 'emis/veteran_status_configuration'

describe EMIS::VeteranStatusService do
let(:edipi) { '1607472595' }
let(:edipi_veteran) { '1068619536' }
let(:edipi_non_veteran) { '1140840595' }
let(:bad_edipi) { '595' }
let(:missing_edipi) { '1111111111' }
let(:no_status) { '1005079361' }

describe 'get_veteran_status' do
context 'with a valid request' do
it 'calls the get_veteran_status endpoint with a proper emis message' do
VCR.use_cassette('emis/get_veteran_status/valid') do
response = subject.get_veteran_status(edipi: edipi)
response = subject.get_veteran_status(edipi: edipi_veteran)
expect(response).to be_ok
end
end

it 'gives me the right values back' do
VCR.use_cassette('emis/get_veteran_status/valid') do
response = subject.get_veteran_status(edipi: edipi)
expect(response.items.first.title38_status_code).to eq('V4')
response = subject.get_veteran_status(edipi: edipi_veteran)
expect(response.items.first.title38_status_code).to eq('V1')
expect(response.items.first.post911_deployment_indicator).to eq('Y')
expect(response.items.first.post911_combat_indicator).to eq('N')
expect(response.items.first.pre911_deployment_indicator).to eq('Y')
expect(response.items.first.pre911_deployment_indicator).to eq('N')
end
end
end

context 'with a valid request for a non-veteran' do
it 'gives me the right values back' do
VCR.use_cassette('emis/get_veteran_status/valid_non_veteran') do
response = subject.get_veteran_status(edipi: edipi_non_veteran)
expect(response.items.first.title38_status_code).to eq('V4')
end
end
end
Expand All @@ -50,6 +62,15 @@
end
end
end

context 'with an empty response element' do
it 'returns nil' do
VCR.use_cassette('emis/get_veteran_status/empty_title38') do
response = subject.get_veteran_status(edipi: no_status)
expect(response.items.first).to be_nil
end
end
end
end
end

Expand Down
18 changes: 18 additions & 0 deletions spec/models/emis_redis/veteran_status_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,22 @@
end
end
end

describe 'title38_status' do
context 'with a valid response for a veteran' do
it 'returns true' do
VCR.use_cassette('emis/get_veteran_status/valid') do
expect(subject.title38_status).to eq('V1')
end
end
end

context 'with a valid response for a non-veteran' do
it 'returns false' do
VCR.use_cassette('emis/get_veteran_status/valid_non_veteran') do
expect(subject.title38_status).to eq('V4')
end
end
end
end
end
15 changes: 8 additions & 7 deletions spec/request/id_card_attributes_request_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,15 @@
before do
Settings.vic.signing_key_path = "#{::Rails.root}/spec/support/certificates/vic-signing-key.pem"
use_authenticated_current_user(current_user: current_user)
end

def url_param_map(url)
params = URI.decode_www_form(url.query)
params.each_with_object({}) { |a, h| h[a.first] = a.last }
expect(current_user).to receive('beta_enabled?').with(current_user.uuid, 'veteran_id_card').and_return(true)
end

describe '#show /v0/id_card/attributes' do
it 'should return a signed redirect URL' do
expect_any_instance_of(EMISRedis::MilitaryInformation)
.to receive(:service_episodes_by_date).at_least(:once).and_return(service_episodes)
expect_any_instance_of(EMISRedis::VeteranStatus)
.to receive(:title38_status).at_least(:once).and_return('V1')
get '/v0/id_card/attributes', headers: auth_header
expect(response).to have_http_status(:ok)
json = JSON.parse(response.body)
Expand All @@ -30,13 +28,16 @@ def url_param_map(url)
expect(traits.key?('edipi')).to be_truthy
expect(traits.key?('firstname')).to be_truthy
expect(traits.key?('lastname')).to be_truthy
expect(traits.key?('title38status')).to be_truthy
expect(traits.key?('branchofservice')).to be_truthy
expect(traits.key?('dischargetype')).to be_truthy
expect(traits.key?('timestamp')).to be_truthy
expect(traits.key?('signature')).to be_truthy
end

it 'should return Bad Gateway if military information not retrievable' do
expect_any_instance_of(EMISRedis::VeteranStatus)
.to receive(:title38_status).at_least(:once).and_return('V1')
expect_any_instance_of(EMISRedis::MilitaryInformation)
.to receive(:service_episodes_by_date).and_raise(StandardError)
get '/v0/id_card/attributes', headers: auth_header
Expand All @@ -45,14 +46,14 @@ def url_param_map(url)

it 'should return Forbidden for non-veteran user' do
expect_any_instance_of(EMISRedis::VeteranStatus)
.to receive(:veteran?).and_return(false)
.to receive(:title38_status).and_return('V2')
get '/v0/id_card/attributes', headers: auth_header
expect(response).to have_http_status(:forbidden)
end

it 'should return Forbidden when veteran status not retrievable' do
expect_any_instance_of(EMISRedis::VeteranStatus)
.to receive(:veteran?).and_raise(StandardError)
.to receive(:title38_status).and_raise(StandardError)
get '/v0/id_card/attributes', headers: auth_header
expect(response).to have_http_status(:forbidden)
end
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 10 additions & 10 deletions spec/support/vcr_cassettes/emis/get_veteran_status/valid.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading