API-40053-store-id-from-vnp-requests #20103

rockwellwindsor-va · 2025-01-06T18:09:44Z

Summary

Gets the responses from the related calls in order to acquire the IDs returned
Adds those IDs into the metadata if they are present and saves them on the PowerOfAttorneyRequest record when saved
Adds a few tests for this scenario as well as updating some related tests this addition broke

Related issue(s)

API-43737

Testing done

New code is covered by unit tests

Testing Notes

Easiest way to manually test this is hit services/claims/v2/veterans/1012829932V238054/power-of-attorney-request (or whatever ICN) and then look the record up in your DB. You should see metadata have some ID values inside the saved record (this is of course dependent on those IDs being present, I always saw them but not sure if any test users would no)
- rec = ClaimsApi::PowerOfAttorneyRequest.find(YOUR_RECORD_ID)

rec.metadata
=> {"veteran"=>{"vnp_mail_id"=>"148859", "vnp_email_id"=>"148858", "vnp_phone_id"=>"106156"}}

Screenshots

Note: Optional

What areas of the site does it impact?

modified:   modules/claims_api/app/controllers/claims_api/v2/veterans/power_of_attorney/request_controller.rb
modified:   modules/claims_api/app/services/claims_api/power_of_attorney_request_service/create_request.rb
modified:   modules/claims_api/spec/services/power_of_attorney_request_service/create_request_spec.rb
modified:   modules/claims_api/spec/services/power_of_attorney_request_service/orchestrator_spec.rb

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?

* Gets the responses from the related calls in order to aquire the IDs returned * Adds those IDs into the metadata if they are present and saves them on the PowerOfAttorneyRequest record when saved modified: modules/claims_api/app/controllers/claims_api/v2/veterans/power_of_attorney/request_controller.rb modified: modules/claims_api/app/services/claims_api/power_of_attorney_request_service/create_request.rb modified: modules/claims_api/spec/services/power_of_attorney_request_service/create_request_spec.rb modified: modules/claims_api/spec/services/power_of_attorney_request_service/orchestrator_spec.rb

github-actions · 2025-01-06T22:08:22Z

	1 Warning
⚠️	This PR changes `241` LoC (not counting whitespace/newlines). In order to ensure each PR receives the proper attention it deserves, we recommend not exceeding `200`. Expect some delays getting reviews. File Summary Files modules/claims_api/app/controllers/claims_api/v2/veterans/power_of_attorney/request_controller.rb (+2/-1) modules/claims_api/app/services/claims_api/power_of_attorney_request_service/create_request.rb (+29/-10) modules/claims_api/spec/services/power_of_attorney_request_service/create_request_spec.rb (+180/-5) modules/claims_api/spec/services/power_of_attorney_request_service/orchestrator_spec.rb (+13/-1) Note: We exclude files matching the following when considering PR size: `.csv, .json, .tsv, .txt, .md, Gemfile.lock, app/swagger, modules/mobile/docs, spec/fixtures/, spec/support/vcr_cassettes/, modules/mobile/spec/support/vcr_cassettes/, db/seeds, modules/vaos/app/docs, modules/meb_api/app/docs, modules/appeals_api/app/swagger/, .bru, *.pdf` Big PRs are difficult to review, often become stale, and cause delays.

Generated by 🚫 Danger

tycol7

The code looks good to me, but I was unable to test due to the BGS errors discussed elsewhere.

tycol7 · 2025-01-07T17:25:32Z

...es/claims_api/app/controllers/claims_api/v2/veterans/power_of_attorney/request_controller.rb

@@ -97,7 +97,8 @@ def create # rubocop:disable Metrics/MethodLength
            claimant_icn = form_attributes.dig('claimant', 'claimantId')
            poa_request = ClaimsApi::PowerOfAttorneyRequest.create!(proc_id: res['procId'],
                                                                    veteran_icn: params[:veteranId],
-                                                                    claimant_icn:, poa_code:)
+                                                                    claimant_icn:, poa_code:,
+                                                                    metadata: res['meta'] || nil)


nit: || nil is redundant here.

tycol7 · 2025-01-07T17:30:12Z

modules/claims_api/app/services/claims_api/power_of_attorney_request_service/create_request.rb

@@ -72,22 +72,25 @@ def create_vonapp_data(person, vnp_ptcpnt_id, trace_digest) # rubocop:disable Me

        promises << Concurrent::Promise.execute do
          Datadog::Tracing.continue_trace!(trace_digest) do
-            create_vnp_mailing_address(person[:address], vnp_ptcpnt_id)
+            res = create_vnp_mailing_address(person[:address], vnp_ptcpnt_id)
+            @vnp_res_object['meta'][type.to_s]['vnp_mail_id'] = res[:vnp_ptcpnt_addrs_id] if res


Why type.to_s here and below but not on line 65? Arenʼt we only passing strings 'veteran' or 'claimant' as the type?

This is a Rubocop change, I just had the variable in there

tycol7 · 2025-01-07T17:38:32Z

modules/claims_api/app/services/claims_api/power_of_attorney_request_service/create_request.rb

+      def add_meta_ids(vet_obj)
+        return vet_obj if @vnp_res_object['meta'].empty?
+
+        vet_obj['meta'] ||= {}


Do we need this line if we have the guard above?

tycol7 · 2025-01-07T17:55:48Z

modules/claims_api/app/services/claims_api/power_of_attorney_request_service/create_request.rb

@@ -320,6 +328,22 @@ def limitation?(condition)
      def format_phone(phone)
        "#{phone[:areaCode]}#{phone[:phoneNumber]}"
      end
+
+      def add_meta_ids(vet_obj)
+        return vet_obj if @vnp_res_object['meta'].empty?


Would .blank? be safer here in case @vnp_res_object['meta'] is nil?

totally, good catch

rockwellwindsor-va added the claimsApi modules/claims_api label Jan 6, 2025

va-vfs-bot temporarily deployed to API-40053-store-id-from-vnp-requests/main/main January 6, 2025 18:15 Inactive

github-actions bot added the test-failure label Jan 6, 2025

Merge branch 'master' into API-40053-store-id-from-vnp-requests

2671638

github-actions bot added test-passing and removed test-failure labels Jan 6, 2025

va-vfs-bot temporarily deployed to API-40053-store-id-from-vnp-requests/main/main January 6, 2025 19:09 Inactive

rockwellwindsor-va added 2 commits January 6, 2025 14:29

Merge branch 'master' into API-40053-store-id-from-vnp-requests

c51bdbb

Reworks to accomodate the inclusion of claimant data

12edd3f

rockwellwindsor-va marked this pull request as ready for review January 6, 2025 22:11

rockwellwindsor-va requested a review from a team as a code owner January 6, 2025 22:11

rockwellwindsor-va marked this pull request as draft January 6, 2025 22:15

rockwellwindsor-va marked this pull request as ready for review January 6, 2025 22:18

va-vfs-bot temporarily deployed to API-40053-store-id-from-vnp-requests/main/main January 6, 2025 22:25 Inactive

tycol7 previously approved these changes Jan 7, 2025

View reviewed changes

Merge branch 'master' into API-40053-store-id-from-vnp-requests

c144d70

va-vfs-bot temporarily deployed to API-40053-store-id-from-vnp-requests/main/main January 8, 2025 19:30 Inactive

Few small adjustments based on PR feedback, one test update

1c0dee3

rockwellwindsor-va dismissed tycol7’s stale review via 1c0dee3 January 8, 2025 19:37

rockwellwindsor-va requested review from a team and tycol7 January 8, 2025 19:38

va-vfs-bot deployed to API-40053-store-id-from-vnp-requests/main/main January 8, 2025 19:51 View deployment

tycol7 approved these changes Jan 8, 2025

View reviewed changes

rockwellwindsor-va merged commit 50b23de into master Jan 8, 2025
32 checks passed

rockwellwindsor-va deleted the API-40053-store-id-from-vnp-requests branch January 8, 2025 21:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API-40053-store-id-from-vnp-requests #20103

API-40053-store-id-from-vnp-requests #20103

rockwellwindsor-va commented Jan 6, 2025 •

edited

Loading

github-actions bot commented Jan 6, 2025

Files

tycol7 left a comment

tycol7 Jan 7, 2025

rockwellwindsor-va Jan 8, 2025

tycol7 Jan 7, 2025

rockwellwindsor-va Jan 7, 2025 •

edited

Loading

tycol7 Jan 7, 2025

tycol7 Jan 7, 2025

rockwellwindsor-va Jan 7, 2025

rockwellwindsor-va Jan 8, 2025

API-40053-store-id-from-vnp-requests #20103

API-40053-store-id-from-vnp-requests #20103

Conversation

rockwellwindsor-va commented Jan 6, 2025 • edited Loading

Summary

Related issue(s)

Testing done

Testing Notes

Screenshots

What areas of the site does it impact?

Acceptance criteria

Requested Feedback

github-actions bot commented Jan 6, 2025

Files

tycol7 left a comment

Choose a reason for hiding this comment

tycol7 Jan 7, 2025

Choose a reason for hiding this comment

rockwellwindsor-va Jan 8, 2025

Choose a reason for hiding this comment

tycol7 Jan 7, 2025

Choose a reason for hiding this comment

rockwellwindsor-va Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

tycol7 Jan 7, 2025

Choose a reason for hiding this comment

tycol7 Jan 7, 2025

Choose a reason for hiding this comment

rockwellwindsor-va Jan 7, 2025

Choose a reason for hiding this comment

rockwellwindsor-va Jan 8, 2025

Choose a reason for hiding this comment

rockwellwindsor-va commented Jan 6, 2025 •

edited

Loading

rockwellwindsor-va Jan 7, 2025 •

edited

Loading