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

Created PersonV2 Service and enabled ContactInformationV2 for StatementIdentifierService and McpNotificationEmailJob #18514

Open
wants to merge 50 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
ddc43d9
first commit
RachalCassity Aug 28, 2024
a3ad085
Update V2::ContactInformation VCR cassettes
RachalCassity Sep 11, 2024
78044af
merge from master
RachalCassity Sep 11, 2024
3b964f6
lint
RachalCassity Sep 11, 2024
a7230d6
Updated V2::ContactInformation Async Transaction Tests
RachalCassity Sep 11, 2024
92e073b
lint
RachalCassity Sep 11, 2024
60536f7
removed t
RachalCassity Sep 11, 2024
f47c2b5
Attempting v2 specs
RachalCassity Sep 11, 2024
4184779
Made changes from review
RachalCassity Sep 11, 2024
924af4e
second commit
RachalCassity Sep 16, 2024
8042477
another commit
RachalCassity Sep 16, 2024
a61cc00
remove gha
RachalCassity Sep 16, 2024
102afaa
address spec
RachalCassity Sep 16, 2024
9bcf8ae
transaction fix
RachalCassity Sep 16, 2024
62b06dc
transactions done
RachalCassity Sep 16, 2024
b9f7e8a
test
RachalCassity Sep 17, 2024
bc2ba23
Merge branch 'master' into rcassity-new-v2-uri
RachalCassity Sep 17, 2024
b121369
test
RachalCassity Sep 17, 2024
3056c63
disable
RachalCassity Sep 17, 2024
9be612b
test
RachalCassity Sep 17, 2024
4fa31bc
Merge branch 'rcassity-new-v2-uri' of https://github.com/department-o…
RachalCassity Sep 17, 2024
3eb52d7
test
RachalCassity Sep 17, 2024
c8e7aae
test
RachalCassity Sep 17, 2024
910a34d
test
RachalCassity Sep 17, 2024
11eae4c
test
RachalCassity Sep 17, 2024
05a2783
test
RachalCassity Sep 17, 2024
3a2287c
disable
RachalCassity Sep 17, 2024
7f5ef03
disable
RachalCassity Sep 17, 2024
b177faf
disable
RachalCassity Sep 17, 2024
74ffca9
test
RachalCassity Sep 17, 2024
cebb103
Merge branch 'rcassity-new-v2-uri' of https://github.com/department-o…
RachalCassity Sep 17, 2024
2c07fb0
test
RachalCassity Sep 17, 2024
3d7b5c3
fixed contact info v2 spec
RachalCassity Sep 17, 2024
5baec05
test
RachalCassity Sep 18, 2024
18b6907
test
RachalCassity Sep 18, 2024
20db378
test
RachalCassity Sep 18, 2024
493f030
merge from master
RachalCassity Sep 18, 2024
33cfeaf
Created PersonV2 Service
RachalCassity Sep 19, 2024
aed60e2
merge from master
RachalCassity Sep 19, 2024
b2d1a21
test
RachalCassity Sep 19, 2024
eaa0f0d
Completed v2 contact information redis spec
RachalCassity Sep 19, 2024
bda2142
Fixed person service
RachalCassity Sep 19, 2024
f82794b
test
RachalCassity Sep 19, 2024
fe68e62
test
RachalCassity Sep 19, 2024
bfe7175
test
RachalCassity Sep 20, 2024
7083569
person error fix
RachalCassity Sep 20, 2024
3f55056
removed extra civ2 enable flipper in spec
RachalCassity Sep 20, 2024
3fcda93
Swagger specs
RachalCassity Sep 20, 2024
c13c08a
fixed bundle audit
RachalCassity Sep 20, 2024
d1b2ce4
Changed icn_with_aaid methods to uuid_with_aaid
RachalCassity Sep 20, 2024
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
23 changes: 14 additions & 9 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ GEM
attr_extras (7.1.0)
awesome_print (1.9.2)
aws-eventstream (1.3.0)
aws-partitions (1.976.0)
aws-partitions (1.977.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the changes in this file intentional?

Copy link
Contributor

Choose a reason for hiding this comment

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

aws-sdk-core (3.206.0)
aws-eventstream (~> 1, >= 1.3.0)
aws-partitions (~> 1, >= 1.651.0)
Expand All @@ -256,8 +256,8 @@ GEM
aws-sdk-core (~> 3, >= 3.205.0)
aws-sdk-kms (~> 1)
aws-sigv4 (~> 1.5)
aws-sdk-sns (1.82.0)
aws-sdk-core (~> 3, >= 3.201.0)
aws-sdk-sns (1.85.0)
aws-sdk-core (~> 3, >= 3.205.0)
aws-sigv4 (~> 1.5)
aws-sigv4 (1.10.0)
aws-eventstream (~> 1, >= 1.0.2)
Expand Down Expand Up @@ -512,8 +512,13 @@ GEM
thor (>= 0.20, < 2.a)
google-cloud-env (2.1.1)
faraday (>= 1.0, < 3.a)
google-protobuf (4.28.1)
google-protobuf (4.28.2)
bigdecimal
rake (>= 13)
google-protobuf (4.28.2-java)
bigdecimal
ffi (~> 1)
ffi-compiler (~> 1)
rake (>= 13)
googleauth (1.11.0)
faraday (>= 1.0, < 3.a)
Expand Down Expand Up @@ -647,7 +652,7 @@ GEM
mini_mime (1.1.5)
mini_portile2 (2.8.7)
minitest (5.25.1)
mock_redis (0.44.0)
mock_redis (0.45.0)
msgpack (1.7.2)
msgpack (1.7.2-java)
multi_json (1.15.0)
Expand Down Expand Up @@ -733,7 +738,7 @@ GEM
hashery (~> 2.0)
ruby-rc4
ttfunk
pg (1.5.7)
pg (1.5.8)
pg_query (5.1.0)
google-protobuf (>= 3.22.3)
pg_search (2.3.7)
Expand Down Expand Up @@ -764,9 +769,9 @@ GEM
psych (5.1.2-java)
jar-dependencies (>= 0.1.7)
public_suffix (6.0.1)
puma (6.4.2)
puma (6.4.3)
nio4r (~> 2.0)
puma (6.4.2-java)
puma (6.4.3-java)
nio4r (~> 2.0)
pundit (2.4.0)
activesupport (>= 3.0.0)
Expand Down Expand Up @@ -935,7 +940,7 @@ GEM
rack (>= 1.1)
rubocop (>= 1.52.0, < 2.0)
rubocop-ast (>= 1.31.1, < 2.0)
rubocop-rspec (3.0.4)
rubocop-rspec (3.0.5)
rubocop (~> 1.61)
rubocop-rspec_rails (2.30.0)
rubocop (~> 1.61)
Expand Down
7 changes: 6 additions & 1 deletion app/controllers/v0/profile/persons_controller.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# frozen_string_literal: true

require 'va_profile/person/service'
require 'va_profile/v2/person/service'

module V0
module Profile
Expand All @@ -11,7 +12,11 @@ class PersonsController < ApplicationController
after_action :invalidate_mpi_cache

def initialize_vet360_id
response = VAProfile::Person::Service.new(@current_user).init_vet360_id
response = if Flipper.enabled?(:va_v3_contact_information_service, @current_user)
VAProfile::V2::Person::Service.new(@current_user).init_vet360_id
else
VAProfile::Person::Service.new(@current_user).init_vet360_id
end
transaction = AsyncTransaction::VAProfile::InitializePersonTransaction.start(@current_user, response)

render json: AsyncTransaction::BaseSerializer.new(transaction).serializable_hash
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,11 @@ def send_email(email, template_id, personalisation)
private

def person_response(vet360_id)
VAProfile::ContactInformation::Service.get_person(vet360_id)
if Flipper.enabled?(:va_v3_contact_information_service)
VAProfile::V2::ContactInformation::Service.get_person(vet360_id)
else
VAProfile::ContactInformation::Service.get_person(vet360_id)
end
end
end
end
9 changes: 5 additions & 4 deletions lib/va_profile/v2/contact_information/service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@ class Service < VAProfile::Service
def get_person
with_monitoring do
vet360_id_present!
raw_response = perform(:get, "#{MPI::Constants::VA_ROOT_OID}/#{ERB::Util.url_encode(icn_with_aaid)}")
raw_response = perform(:get, "#{MPI::Constants::VA_ROOT_OID}/#{ERB::Util.url_encode(uuid_with_aaid)}")
PersonResponse.from(raw_response)
end
rescue Common::Client::Errors::ClientError => e
if e.status == 400
if e.status == 404
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to still cover the 400 status code here?

log_exception_to_sentry(
e,
{ vet360_id: },
Expand Down Expand Up @@ -213,9 +213,10 @@ def get_person_transaction_status(transaction_id)

private

def icn_with_aaid
def uuid_with_aaid
return "#{@user.idme_uuid}^PN^200VIDM^USDVA" if @user.idme_uuid
bosawt marked this conversation as resolved.
Show resolved Hide resolved
return "#{@user.logingov_uuid}^PN^200VLGN^USDVA" if @user.logingov_uuid
return "#{vet360_id}^PI^200VETS^USDVA" if @user.idme_uuid.blank? && @user.logingov_uuid.blank?
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use VA_PROFILE_ID_POSTFIX from line 16 instead of ^PI^200VETS^USDVA?


nil
end
Expand Down Expand Up @@ -289,7 +290,7 @@ def vet360_id_present!
def post_or_put_data(method, model, path, response_class)
with_monitoring do
vet360_id_present!
request_path = "#{MPI::Constants::VA_ROOT_OID}/#{ERB::Util.url_encode(icn_with_aaid)}" + "/#{path}"
request_path = "#{MPI::Constants::VA_ROOT_OID}/#{ERB::Util.url_encode(uuid_with_aaid)}" + "/#{path}"
raw_response = perform(method, request_path, model.in_json)
response_class.from(raw_response)
end
Expand Down
60 changes: 60 additions & 0 deletions lib/va_profile/v2/person/service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
# frozen_string_literal: true

require 'common/client/base'
require 'common/client/concerns/monitoring'
require 'va_profile/v2/contact_information/configuration'
require 'va_profile/v2/contact_information/transaction_response'
require 'va_profile/service'
require 'va_profile/stats'
require 'identity/parsers/gc_ids_constants'

module VAProfile
module V2
module Person
class Service < VAProfile::Service
include Common::Client::Concerns::Monitoring
include ERB::Util

STATSD_KEY_PREFIX = "#{VAProfile::Service::STATSD_KEY_PREFIX}.person".freeze
configuration VAProfile::V2::ContactInformation::Configuration

# Initializes a vet360_id for a user that does not have one. Can be used when a current user
# is present, or through a rake task when no user is present (through passing in their ICN).
# This is an asynchronous process for VAProfile, so it returns VAProfile transaction information.
#
# @return [VAProfile::V2::ContactInformation::PersonTransactionResponse]
# response wrapper around a transaction object
#
def init_vet360_id
with_monitoring do
raw_response = perform(:post, "#{MPI::Constants::VA_ROOT_OID}/#{ERB::Util.url_encode(uuid_with_aaid)}",
empty_body)
VAProfile::V2::ContactInformation::PersonTransactionResponse.from(raw_response, @user)
end
rescue => e
handle_error(e)
end

private

# @see https://ruby-doc.org/stdlib-2.3.0/libdoc/erb/rdoc/ERB/Util.html
#
def uuid_with_aaid
return "#{@user.idme_uuid}^PN^200VIDM^USDVA" if @user.idme_uuid
Copy link
Contributor

Choose a reason for hiding this comment

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

There aren't ICN

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I know. I kept the icn_with_aaid method for the moment. We will be refactoring a lot of code after we make sure the upgrade is successful. I can update the method name now though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Contact Service is not using the ICN to make requests, I changed the method from icn_with_aaid with uuid_with_aaid to avoid confusion.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok, that makes more sense

return "#{@user.logingov_uuid}^PN^200VLGN^USDVA" if @user.logingov_uuid
return "#{@user.vet360_id}^PI^200VETS^USDVA" if @user.idme_uuid.blank? && @user.logingov_uuid.blank?
Comment on lines +44 to +45
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious, where do these ^PN^... values come from?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think these are given to us from VA Profile


nil
end

def empty_body
{
bio: {
sourceDate: Time.zone.now.iso8601
}
}.to_json
end
end
end
end
end
4 changes: 2 additions & 2 deletions spec/factories/users.rb
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,8 @@
loa do
{ current: LOA::THREE, highest: LOA::THREE }
end

idme_uuid { '6767671' }
vet360_id { '6767671' }
Copy link
Contributor

Choose a reason for hiding this comment

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

I think instead of having this trait :error in the user factory, it probably just makes sense to define the attribute combination to cause the error for your specific service in the spec file itself. This isn't generally a user error in any other situation so it's difficult for any other spec to leverage this, which is what would be the reason to put a trait in the factory

Copy link
Member Author

Choose a reason for hiding this comment

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

Idme uuid also has to be null. The :user, :error factory was used to validate invalid user requests.

Copy link
Contributor

Choose a reason for hiding this comment

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

But the user model is generally fine without an idme_uuid or a vet360_id, so this doesn't actually describe an error state for the User model, it only describes an error when interacting with this specific service

idme_uuid { nil }
end

trait :accountable do
Expand Down
6 changes: 5 additions & 1 deletion spec/lib/common/models/concerns/cache_aside_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,11 @@
end

before do
allow(VAProfile::Models::Person).to receive(:build_from).and_return(person)
if Flipper.enabled?(:va_v3_contact_information_service)
allow(VAProfile::Models::V2::Person).to receive(:build_from).and_return(person)
else
allow(VAProfile::Models::Person).to receive(:build_from).and_return(person)
end
end

describe '#do_cached_with' do
Expand Down
Loading
Loading