5708 vet360 linking

[kpethtel]

Summary

This ticket fixes two issues.

1. it moves the vet360 linking out of the authenticate method. The authenticate method is called on every single request, which meant that when a vet is using the app and they don't have a vet360_id, sometimes multiple linking jobs would be spun up per session. The redis locking mechanism did not appear to reliably prevent this.
2. it avoids the poll with backoff functionality. That functionality (which is still used for synchronous contact update requests) uses the sleep method. Most of the time that's not a problem because the request usually works the first try, but when the upstream is down, a single job will occupy a sidekiq thread for a full 55 seconds.

Because of these two issues, when the upstream is down, we could see multiple jobs stack up per user and occupy sidekiq threads for nearly a minute each, which puts a lot of unnecessary strain on sidekiq.

Another notable side-effect of this change is that now the linking will happen on both IAM and SIS requests. Previously, these requests were only happening on IAM sessions because no one knew what the linking did so we didn't bother reproducing it for SIS. The fact that this will potentially happen more is a good thing because some mobile app features don't work without a VA Profile account, which the linking job attempts to create for users who don't have them. So this should result in a better mobile app experience for some users.

Related issue(s)

department-of-veterans-affairs/va-mobile-app#5708

Testing done

Specs

Screenshots

What areas of the site does it impact?

• No major impact. The linking job may occur more frequently but should also tie up threads less often.

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 or Grafana (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

Any and all.

[github-actions]

1 Warning

⚠️

This PR changes 223 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 config/redis.yml (+0/-3) modules/mobile/app/controllers/mobile/application_controller.rb (+0/-24) modules/mobile/app/controllers/mobile/v0/users_controller.rb (+5/-0) modules/mobile/app/controllers/mobile/v1/users_controller.rb (+5/-0) modules/mobile/app/services/mobile/v0/profile/sync_update_service.rb (+0/-18) modules/mobile/app/workers/mobile/v0/vet360_linking_job.rb (+18/-8) modules/mobile/spec/controllers/application_controller_spec.rb (+0/-16) modules/mobile/spec/request/user_request_spec.rb (+36/-0) modules/mobile/spec/request/v1/user_request_spec.rb (+40/-4) modules/mobile/spec/workers/vet360_linking_spec.rb (+12/-34) Note: We exclude files matching the following when considering PR size: *.csv, *.json, *.tsv, *.txt, 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/ Big PRs are difficult to review, often become stale, and cause delays.

Generated by 🚫 Danger

[kpethtel]

These are not related to the PR. They were mistakes that were made when creating the v1 of this spec.

[kpethtel]

I don't think there's any value in this transaction id. One thing that may not be very clear from the diff is that this transaction is different from the transaction that was previously being returned. Neither of them is valuable unless we want to dig deeply into logs with the VAProfile team.

I spent quite a bit of time trying to understand exactly why we were doing this linking and what impact it has. It's not clear from the code and I eventually had to ask Alastair for context (see comment above for Alastair's insight). If you follow the pre-existing code all the way through, what it actually does is make a request to create a VAProfile acount, which creates a response object that has a transaction id that we use to create a transaction record. Then we repeatedly poll the service asking if the transaction succeeded. But this is an async job so there's no reason to repeatedly poll for success. We didn't retry if it failed anyhow. And there's no reason to create the transaction record because it's only used for the purpose of checking back for success. All transaction records are deleted after a month and are not used anywhere else in the app.

[aherzberg]

LGTM.

[ryan-mcneil]

👌