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

Always use mhv_user_account cached response on User #20216

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ def mhv_correlation_id
end

def mhv_user_account
@mhv_user_account ||= MHV::UserAccount::Creator.new(user_verification:).perform
@mhv_user_account ||= MHV::UserAccount::Creator.new(user_verification:, from_cache_only: true).perform
rescue => e
log_mhv_user_account_error(e.message)
nil
Expand Down
16 changes: 11 additions & 5 deletions app/services/mhv/user_account/creator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,12 @@
module MHV
module UserAccount
class Creator
attr_reader :user_verification, :break_cache
attr_reader :user_verification, :break_cache, :from_cache_only

def initialize(user_verification:, break_cache: false)
def initialize(user_verification:, break_cache: false, from_cache_only: false)
@user_verification = user_verification
@break_cache = break_cache
@from_cache_only = from_cache_only
end

def perform
Expand All @@ -30,16 +31,17 @@ def perform
private

def create_mhv_user_account!
return nil if mhv_account_creation_response.nil? && from_cache_only

account = MHVUserAccount.new(mhv_account_creation_response)
account.validate!
MPIData.find(icn)&.destroy
account
end

def mhv_account_creation_response
tou_occurred_at = current_tou_agreement.created_at

mhv_client.create_account(icn:, email:, tou_occurred_at:, break_cache:)
@mhv_account_creation_response ||= mhv_client.create_account(icn:, email:, tou_occurred_at:, break_cache:,
from_cache_only:)
end

def icn
Expand All @@ -58,6 +60,10 @@ def user_account
@user_account ||= user_verification.user_account
end

def tou_occurred_at
current_tou_agreement.created_at
end

def mhv_client
MHV::AccountCreation::Service.new
end
Expand Down
15 changes: 14 additions & 1 deletion lib/mhv/account_creation/service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ module AccountCreation
class Service < Common::Client::Base
configuration Configuration

def create_account(icn:, email:, tou_occurred_at:, break_cache: false)
def create_account(icn:, email:, tou_occurred_at:, break_cache: false, from_cache_only: false)
return find_cached_response(icn) if from_cache_only

params = build_create_account_params(icn:, email:, tou_occurred_at:)

create_account_with_cache(icn:, force: break_cache, expires_in: 1.day) do
Expand All @@ -23,6 +25,17 @@ def create_account(icn:, email:, tou_occurred_at:, break_cache: false)

private

def find_cached_response(icn)
account = Rails.cache.read("#{config.service_name}_#{icn}")

if account.present?
log_payload = { icn:, account:, from_cache: true, from_cache_only: true }
Rails.logger.info("#{config.logging_prefix} create_account success", log_payload)
end

account
end

def create_account_with_cache(icn:, force:, expires_in:, &request)
cache_hit = true
start = nil
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@
expect(mhv_client).to have_received(:create_account).with(icn:,
email: user_credential_email.credential_email,
tou_occurred_at: terms_of_use_agreement.created_at,
break_cache: true)
break_cache: true,
from_cache_only: false)
end
end

Expand Down
62 changes: 52 additions & 10 deletions spec/lib/mhv/account_creation/service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

describe MHV::AccountCreation::Service do
describe '#create_account' do
subject { described_class.new.create_account(icn:, email:, tou_occurred_at:, break_cache:) }
subject { described_class.new.create_account(icn:, email:, tou_occurred_at:, break_cache:, from_cache_only:) }

let(:icn) { '10101V964144' }
let(:email) { '[email protected]' }
Expand All @@ -16,9 +16,20 @@
let(:account_creation_base_url) { 'https://apigw-intb.aws.myhealth.va.gov' }
let(:account_creation_path) { 'v1/usermgmt/account-service/account' }
let(:break_cache) { false }
let(:from_cache_only) { false }
let(:start_time) { Time.zone.now }
let(:end_time) { start_time + 10.seconds }

let(:user_profile_id) { '12345678' }
let(:premium) { true }
let(:champ_va) { true }
let(:patient) { true }
let(:sm_account_created) { true }
let(:message) { 'Existing MHV Account Found for ICN' }
let(:expected_response_body) do
{ user_profile_id:, premium:, champ_va:, patient:, sm_account_created:, message: }
end

before do
allow(Rails.logger).to receive(:info)
allow(Rails.logger).to receive(:error)
Expand Down Expand Up @@ -47,22 +58,53 @@
end
end

context 'when from_cache_only is true' do
let(:from_cache_only) { true }
let(:expected_cache_key) { "mhv_account_creation_#{icn}" }

context 'when the account is in the cache' do
before do
allow(Rails.cache).to receive(:read).with(expected_cache_key).and_return(expected_response_body)
end

it 'returns the cached response' do
expect(subject).to eq(expected_response_body)
end

it 'does not make a request to the account creation service' do
subject
expect(a_request(:post, "#{account_creation_base_url}/#{account_creation_path}")).not_to have_been_made
expect(Rails.logger).not_to have_received(:info).with("#{log_prefix} create_account request",
anything)
end
end

context 'when the account is not in the cache' do
before do
allow(Rails.cache).to receive(:read).with(expected_cache_key).and_return(nil)
end

it 'returns nil' do
expect(subject).to be_nil
end

it 'does not make a request to the account creation service' do
subject
expect(a_request(:post, "#{account_creation_base_url}/#{account_creation_path}")).not_to have_been_made
expect(Rails.logger).not_to have_received(:info).with("#{log_prefix} create_account request",
anything)
end
end
end

context 'when the response is successful' do
let(:successful_response_cassette) { 'mhv/account_creation/account_creation_service_200_found' }
let(:expected_log_message) { "#{log_prefix} create_account success" }
let(:expected_log_payload) do
{ icn:, account: expected_response_body, from_cache: expected_from_cache_log,
duration_ms: expected_duration }.compact
end
let(:user_profile_id) { '12345678' }
let(:premium) { true }
let(:champ_va) { true }
let(:patient) { true }
let(:sm_account_created) { true }
let(:message) { 'Existing MHV Account Found for ICN' }
let(:expected_response_body) do
{ user_profile_id:, premium:, champ_va:, patient:, sm_account_created:, message: }
end

let(:expected_duration) { 10_000.0 }

shared_examples 'a successful external request' do
Expand Down
77 changes: 44 additions & 33 deletions spec/models/user_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1399,6 +1399,7 @@
describe '#mhv_user_account' do
let(:user) { build(:user, :loa3) }
let(:icn) { user.icn }
let(:expected_cache_key) { "mhv_account_creation_#{icn}" }

let!(:user_verification) do
create(:idme_user_verification, idme_uuid: user.idme_uuid, user_credential_email:, user_account:)
Expand All @@ -1423,55 +1424,65 @@
before do
allow(Rails.logger).to receive(:info)
allow(MHV::AccountCreation::Service).to receive(:new).and_return(mhv_client)
allow(mhv_client).to receive(:create_account).and_return(mhv_response)
allow(Rails.cache).to receive(:read).with(expected_cache_key).and_return(mhv_response)
end

context 'when the user has all required attributes' do
it 'returns a MHVUserAccount with the expected attributes' do
mhv_user_account = user.mhv_user_account
context 'when the mhv response is cached' do
context 'when the user has all required attributes' do
it 'returns a MHVUserAccount with the expected attributes' do
mhv_user_account = user.mhv_user_account

expect(mhv_user_account).to be_a(MHVUserAccount)
expect(mhv_user_account.attributes).to eq(mhv_response.with_indifferent_access)
expect(mhv_user_account).to be_a(MHVUserAccount)
expect(mhv_user_account.attributes).to eq(mhv_response.with_indifferent_access)
end
end
end

context 'when there is an error creating the account' do
shared_examples 'mhv_user_account error' do
let(:expected_log_message) { '[User] mhv_user_account error' }
let(:expected_log_payload) { { error_message: /#{expected_error_message}/, icn: user.icn } }
context 'when there is an error creating the account' do
shared_examples 'mhv_user_account error' do
let(:expected_log_message) { '[User] mhv_user_account error' }
let(:expected_log_payload) { { error_message: /#{expected_error_message}/, icn: user.icn } }

it 'logs and returns nil' do
expect(user.mhv_user_account).to be_nil
expect(Rails.logger).to have_received(:info).with(expected_log_message, expected_log_payload)
it 'logs and returns nil' do
expect(user.mhv_user_account).to be_nil
expect(Rails.logger).to have_received(:info).with(expected_log_message, expected_log_payload)
end
end
end

context 'when the user does not have a terms_of_use_agreement' do
let(:terms_of_use_agreement) { nil }
let(:expected_error_message) { 'Current terms of use agreement must be present' }
context 'when the user does not have a terms_of_use_agreement' do
let(:terms_of_use_agreement) { nil }
let(:expected_error_message) { 'Current terms of use agreement must be present' }

it_behaves_like 'mhv_user_account error'
end
it_behaves_like 'mhv_user_account error'
end

context 'when the user has not accepted the terms of use' do
let(:terms_of_use_response) { 'declined' }
let(:expected_error_message) { "Current terms of use agreement must be 'accepted'" }
context 'when the user has not accepted the terms of use' do
let(:terms_of_use_response) { 'declined' }
let(:expected_error_message) { "Current terms of use agreement must be 'accepted'" }

it_behaves_like 'mhv_user_account error'
end
it_behaves_like 'mhv_user_account error'
end

context 'when the user does not have a user_credential_email' do
let(:user_credential_email) { nil }
let(:expected_error_message) { 'Email must be present' }

it_behaves_like 'mhv_user_account error'
end

context 'when the user does not have a user_credential_email' do
let(:user_credential_email) { nil }
let(:expected_error_message) { 'Email must be present' }
context 'when the user does not have an icn' do
let(:icn) { nil }
let(:expected_error_message) { 'ICN must be present' }

it_behaves_like 'mhv_user_account error'
it_behaves_like 'mhv_user_account error'
end
end
end

context 'when the user does not have an icn' do
let(:icn) { nil }
let(:expected_error_message) { 'ICN must be present' }
context 'when the mhv response is not cached' do
let(:mhv_response) { nil }

it_behaves_like 'mhv_user_account error'
it 'returns nil' do
expect(user.mhv_user_account).to be_nil
end
end
end
Expand Down
42 changes: 38 additions & 4 deletions spec/services/mhv/user_account/creator_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
require 'mhv/account_creation/service'

RSpec.describe MHV::UserAccount::Creator do
subject { described_class.new(user_verification:, break_cache:) }
subject { described_class.new(user_verification:, break_cache:, from_cache_only:) }

let(:user_account) { create(:user_account, icn:) }
let(:user_verification) { create(:user_verification, user_account:, user_credential_email:) }
Expand All @@ -14,6 +14,7 @@
let(:email) { '[email protected]' }
let(:tou_occurred_at) { terms_of_use_agreement&.created_at }
let(:break_cache) { false }
let(:from_cache_only) { false }
let(:mhv_client) { instance_double(MHV::AccountCreation::Service) }
let(:mhv_response_body) do
{
Expand All @@ -30,7 +31,7 @@

allow(MHV::AccountCreation::Service).to receive(:new).and_return(mhv_client)
allow(mhv_client).to receive(:create_account)
.with(icn:, email:, tou_occurred_at:, break_cache:)
.with(icn:, email:, tou_occurred_at:, break_cache:, from_cache_only:)
.and_return(mhv_response_body)
end

Expand Down Expand Up @@ -80,7 +81,8 @@
context 'when break_cache is false' do
it 'calls MHV::AccountCreation::Service#create_account with break_cache: false' do
subject.perform
expect(mhv_client).to have_received(:create_account).with(icn:, email:, tou_occurred_at:, break_cache: false)
expect(mhv_client).to have_received(:create_account).with(icn:, email:, tou_occurred_at:, break_cache: false,
from_cache_only:)
end
end

Expand All @@ -89,7 +91,39 @@

it 'calls MHV::AccountCreation::Service#create_account with break_cache: true' do
subject.perform
expect(mhv_client).to have_received(:create_account).with(icn:, email:, tou_occurred_at:, break_cache: true)
expect(mhv_client).to have_received(:create_account).with(icn:, email:, tou_occurred_at:, break_cache: true,
from_cache_only:)
end
end

context 'when from_cache_only is true' do
let(:from_cache_only) { true }
let(:expected_cache_key) { "mhv_account_creation_#{icn}" }

before do
allow(Rails.cache).to receive(:read).with(expected_cache_key).and_return(mhv_response_body)
end

it 'calls MHV::AccountCreation::Service#create_account with from_cache_only: true' do
subject.perform
expect(mhv_client).to have_received(:create_account).with(icn:, email:, tou_occurred_at:, break_cache:,
from_cache_only: true)
end

context 'when the response is cached' do
it 'returns the cached response' do
mhv_user_account = subject.perform
expect(mhv_user_account).to be_a(MHVUserAccount)
expect(mhv_user_account).to be_valid
end
end

context 'when the response is not cached' do
let(:mhv_response_body) { nil }

it 'returns nil' do
expect(subject.perform).to be_nil
end
end
end
end
Expand Down
Loading