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

RachalCassity · 2024-09-19T14:38:27Z

Note: Delete the description statements, complete each step. None are optional, but can be justified as to why they cannot be completed as written. Provide known gaps to testing that may raise the risk of merging to production.

Summary

The new ContactInformationV2 service modified the addressPou value from RESIDENCE/CHOICE to RESIDENCE and VAProfile::Models::V2::Address was created to build the address records with the correct addressPou. Created PersonV2 Service and VAProfile::Models::V2::Person to utilize the new VAProfile::Models::V2::Address.

With the new PersonV2 Service, the ContactInformationV2 Service can now be enabled for McpNotificationEmailJob and StatementIdentifierService.

Related issue(s)

issue

Testing done

New code is covered by unit tests
New ContactInformationV2 Service can be tested throughout the vets-api, besides mobile endpoints. The latency and requests will be monitored for the new service in Datadog.

Screenshots

Note: Optional

What areas of the site does it impact?

(Describe what parts of the site are impacted andifcode touched other areas)

Acceptance criteria

I fixed|updated|added unit tests and integration tests for each feature (if applicable).
No error nor warning in the console.
Events are being sent to the appropriate logging solution
Documentation has been updated (link to documentation)
No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs
Feature/bug has a monitor built into Datadog (if applicable)
If app impacted requires authentication, did you login to a local build and verify all authenticated routes work as expected
I added a screenshot of the developed feature

Requested Feedback

(OPTIONAL)What should the reviewers know in addition to the above. Is there anything specific you wish the reviewer to assist with. Do you have any concerns with this PR, why?

…f-veterans-affairs/vets-api into rcassity-new-v2-uri

lib/va_profile/v2/contact_information/service.rb

bosawt · 2024-09-20T20:42:47Z

lib/va_profile/v2/person/service.rb

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


There aren't ICN

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.

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.

ah ok, that makes more sense

bosawt

I'll have to look at the solution more closely, it looks like things have already been merged that are trying to send ICN to another service but what it's sending is not ICN, so just 'requesting changes' until I can confirm this is intentional or not and whether the changes that have already been merged need to be reconsidered

rmtolmach · 2024-09-23T14:23:23Z

Gemfile.lock

@@ -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)


Are the changes in this file intentional?

bosawt · 2024-09-23T16:29:36Z

spec/factories/users.rb

@@ -165,8 +165,8 @@
      loa do
        { current: LOA::THREE, highest: LOA::THREE }
      end
-
-      idme_uuid { '6767671' }
+      vet360_id { '6767671' }


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

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

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

rmtolmach · 2024-09-23T21:15:48Z

lib/va_profile/v2/contact_information/service.rb

          return "#{@user.idme_uuid}^PN^200VIDM^USDVA" if @user.idme_uuid
          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?


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

rmtolmach · 2024-09-23T21:22:14Z

lib/va_profile/v2/person/service.rb

+          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?


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

rmtolmach · 2024-09-23T21:31:38Z

spec/lib/debt_management_center/statement_identifier_service_spec.rb

@@ -7,6 +7,194 @@
               type: :service do
  describe '#get_mpi_data' do
    Flipper.disable(:va_v3_contact_information_service)
+
+    context 'given edipi statement' do
+      edipi = '492031291'


Where'd you get this edipi from? The edipi I'm familiar with (from here) all start with a 1.

oh, never mind... I see it used in fixtures. I just didn't want to to be some actual Veteran edipi 😅

rmtolmach · 2024-09-23T21:43:20Z

spec/models/va_profile_redis/contact_information_spec.rb

-    end
-  end
-
+  Flipper.disable(:va_v3_contact_information_service)


Should this be wrapped in a before do block so it doesn't interfere with other tests?

rmtolmach · 2024-09-23T21:47:13Z

spec/models/va_profile_redis/contact_information_v2_spec.rb

+          residence = address_for VAProfile::Models::V2::Address::CORRESPONDENCE
+          VCR.use_cassette('va_profile/v2/contact_information/person', VCR::MATCH_EVERYTHING) do
+            expect(contact_info.mailing_address).to eq residence


Maybe call this correspondence instead?

Suggested change

residence = address_for VAProfile::Models::V2::Address::CORRESPONDENCE

VCR.use_cassette('va_profile/v2/contact_information/person', VCR::MATCH_EVERYTHING) do

expect(contact_info.mailing_address).to eq residence

correspondence = address_for VAProfile::Models::V2::Address::CORRESPONDENCE

VCR.use_cassette('va_profile/v2/contact_information/person', VCR::MATCH_EVERYTHING) do

expect(contact_info.mailing_address).to eq correspondence

rmtolmach · 2024-09-23T21:48:35Z

spec/models/va_profile_redis/contact_information_v2_spec.rb

+          phone = phone_for VAProfile::Models::Telephone::FAX
+          VCR.use_cassette('va_profile/v2/contact_information/person', VCR::MATCH_EVERYTHING) do
+            expect(contact_info.fax_number).to eq phone


This could be called fax to distinguish it from the other phone numbers.

Suggested change

phone = phone_for VAProfile::Models::Telephone::FAX

VCR.use_cassette('va_profile/v2/contact_information/person', VCR::MATCH_EVERYTHING) do

expect(contact_info.fax_number).to eq phone

fax = phone_for VAProfile::Models::Telephone::FAX

VCR.use_cassette('va_profile/v2/contact_information/person', VCR::MATCH_EVERYTHING) do

expect(contact_info.fax_number).to eq fax

RachalCassity added 30 commits August 28, 2024 16:31

first commit

ddc43d9

Update V2::ContactInformation VCR cassettes

a3ad085

merge from master

78044af

lint

3b964f6

Updated V2::ContactInformation Async Transaction Tests

a7230d6

lint

92e073b

removed t

60536f7

Attempting v2 specs

f47c2b5

Made changes from review

4184779

second commit

924af4e

another commit

8042477

remove gha

a61cc00

address spec

102afaa

transaction fix

9bcf8ae

transactions done

62b06dc

test

b9f7e8a

Merge branch 'master' into rcassity-new-v2-uri

bc2ba23

test

b121369

disable

3056c63

test

9be612b

Merge branch 'rcassity-new-v2-uri' of https://github.com/department-o…

4fa31bc

…f-veterans-affairs/vets-api into rcassity-new-v2-uri

test

3eb52d7

test

c8e7aae

test

910a34d

test

11eae4c

test

05a2783

disable

3a2287c

disable

7f5ef03

disable

b177faf

test

74ffca9

github-actions bot added the ready-for-backend-review label Sep 20, 2024

va-vfs-bot temporarily deployed to rcassity-swagger-v2/main/main September 20, 2024 20:15 Inactive

fixed bundle audit

c13c08a

github-actions bot added omit-backend-approval and removed omit-backend-approval ready-for-backend-review test-passing lint-failure labels Sep 20, 2024

bosawt reviewed Sep 20, 2024

View reviewed changes

lib/va_profile/v2/contact_information/service.rb Show resolved Hide resolved

github-actions bot added the test-passing label Sep 20, 2024

bosawt reviewed Sep 20, 2024

View reviewed changes

bosawt self-requested a review September 20, 2024 20:44

github-actions bot added require-backend-approval and removed omit-backend-approval labels Sep 20, 2024

bosawt requested changes Sep 20, 2024

View reviewed changes

Changed icn_with_aaid methods to uuid_with_aaid

d1b2ce4

github-actions bot added require-backend-approval ready-for-backend-review and removed require-backend-approval test-passing labels Sep 20, 2024

va-vfs-bot deployed to rcassity-swagger-v2/main/main September 20, 2024 21:18 View deployment

github-actions bot added the test-passing label Sep 20, 2024

rmtolmach reviewed Sep 23, 2024

View reviewed changes

bosawt reviewed Sep 23, 2024

View reviewed changes

rmtolmach reviewed Sep 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

RachalCassity commented Sep 19, 2024 •

edited

Loading

bosawt Sep 20, 2024

RachalCassity Sep 20, 2024

RachalCassity Sep 20, 2024

bosawt Sep 23, 2024

bosawt left a comment

rmtolmach Sep 23, 2024

bosawt Sep 23, 2024

RachalCassity Sep 23, 2024

bosawt Sep 23, 2024

rmtolmach Sep 23, 2024

rmtolmach Sep 23, 2024

rmtolmach Sep 23, 2024

rmtolmach Sep 23, 2024

rmtolmach Sep 23, 2024

rmtolmach Sep 23, 2024

rmtolmach Sep 23, 2024

		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?

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

Are you sure you want to change the base?

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

Conversation

RachalCassity commented Sep 19, 2024 • edited Loading

Summary

Related issue(s)

Testing done

Screenshots

What areas of the site does it impact?

Acceptance criteria

Requested Feedback

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bosawt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RachalCassity commented Sep 19, 2024 •

edited

Loading