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

[10-10EZ] Update LogEmailDiffJob to not use redis key #19398

Merged
merged 18 commits into from
Nov 20, 2024
Merged
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
1 change: 1 addition & 0 deletions .github/CODEOWNERS
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ app/models/form526_job_status.rb @department-of-veterans-affairs/Disability-Expe
app/models/form526_submission.rb @department-of-veterans-affairs/Disability-Experience @department-of-veterans-affairs/va-api-engineers @department-of-veterans-affairs/backend-review-group
app/models/form526_submission_remediation.rb @department-of-veterans-affairs/Disability-Experience @department-of-veterans-affairs/va-api-engineers @department-of-veterans-affairs/backend-review-group
app/models/form_attachment.rb @department-of-veterans-affairs/benefits-decision-reviews-be @department-of-veterans-affairs/va-api-engineers @department-of-veterans-affairs/backend-review-group
app/models/form_email_matches_profile_log.rb @department-of-veterans-affairs/vfs-10-10 @department-of-veterans-affairs/va-api-engineers @department-of-veterans-affairs/backend-review-group
app/models/form_profile.rb @department-of-veterans-affairs/vfs-authenticated-experience-backend @department-of-veterans-affairs/va-api-engineers @department-of-veterans-affairs/backend-review-group
app/models/form_profiles @department-of-veterans-affairs/my-education-benefits @department-of-veterans-affairs/va-api-engineers @department-of-veterans-affairs/backend-review-group
app/models/form_profiles/va_21p527ez.rb @department-of-veterans-affairs/pension-and-burials @department-of-veterans-affairs/backend-review-group
Expand Down
4 changes: 4 additions & 0 deletions app/models/form_email_matches_profile_log.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# frozen_string_literal: true

class FormEmailMatchesProfileLog < ApplicationRecord
end
32 changes: 32 additions & 0 deletions app/sidekiq/hca/log_email_diff_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,14 @@ class LogEmailDiffJob
sidekiq_options retry: false

def perform(in_progress_form_id, user_uuid)
if Flipper.enabled?(:hca_log_email_diff_in_progress_form)
log_email_difference(in_progress_form_id, user_uuid)
else
log_email_difference_redis(in_progress_form_id, user_uuid)
end
end

def log_email_difference_redis(in_progress_form_id, user_uuid)
redis_key = "HCA::LogEmailDiffJob:#{user_uuid}"
return if $redis.get(redis_key).present?

Expand All @@ -28,5 +36,29 @@ def perform(in_progress_form_id, user_uuid)
)
$redis.set(redis_key, 't')
end

def log_email_difference(in_progress_form_id, user_uuid)
return if FormEmailMatchesProfileLog.exists?(user_uuid:, in_progress_form_id:)

in_progress_form = InProgressForm.find_by(id: in_progress_form_id)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept the same logic as the redis version here. If we do any refactors I plan to do those later just to keep the changes here scope to the database vs redis usage and not the logic in this method.

return if in_progress_form.nil?

parsed_form = JSON.parse(in_progress_form.form_data)
form_email = parsed_form['email']
email_confirmation = parsed_form['view:email_confirmation']

return if form_email.blank? || form_email != email_confirmation

user = User.find(user_uuid)
va_profile_email = user.va_profile_email

tag_text = va_profile_email&.downcase == form_email.downcase ? 'same' : 'different'

StatsD.increment(
"api.1010ez.in_progress_form_email.#{tag_text}"
)

FormEmailMatchesProfileLog.create(user_uuid:, in_progress_form_id:)
end
end
end
3 changes: 3 additions & 0 deletions config/features.yml
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,9 @@ features:
hca_log_form_attachment_create:
actor_type: user
description: Enable logging all successful-looking attachment creation calls to Sentry at info-level
hca_log_email_diff_in_progress_form:
actor_type: user
description: Enable using database instead of redis to log email differences between va_profile and in progress forms.
hca_retrieve_facilities_without_repopulating:
actor_type: user
description: Constrain facilities endpoint to only return existing facilities values - even if the table is empty, do not rerun the Job to populate it.
Expand Down
189 changes: 142 additions & 47 deletions spec/sidekiq/hca/log_email_diff_job_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,89 +6,184 @@
let!(:in_progress_form) { create(:in_progress_1010ez_form_with_email) }
let!(:user) { create(:user, :loa3) }

before do
in_progress_form.update!(user_uuid: user.uuid)
allow(User).to receive(:find).with(user.uuid).and_return(user)
end
describe 'hca_log_email_diff_in_progress_form enabled' do
before do
allow(Flipper).to receive(:enabled?).with(:hca_log_email_diff_in_progress_form).and_return(true)
in_progress_form.update!(user_uuid: user.uuid)
allow(User).to receive(:find).with(user.uuid).and_return(user)
end

def self.expect_does_nothing
it 'does nothing' do
expect(StatsD).not_to receive(:increment)
expect($redis).not_to receive(:set)
def self.expect_does_nothing
it 'does nothing' do
expect(StatsD).not_to receive(:increment)

subject
subject

expect(FormEmailMatchesProfileLog).not_to receive(:create).with(
user_uuid: user.uuid, in_progress_form_id: in_progress_form.id
)
end
end
end

def self.expect_email_tag(tag)
it "logs that email is #{tag}" do
expect do
subject
end.to trigger_statsd_increment("api.1010ez.in_progress_form_email.#{tag}")
def self.expect_email_tag(tag)
it "logs that email is #{tag}" do
expect do
subject
end.to trigger_statsd_increment("api.1010ez.in_progress_form_email.#{tag}")

expect($redis.get("HCA::LogEmailDiffJob:#{user.uuid}")).to eq('t')
expect(InProgressForm.where(user_uuid: user.uuid, id: in_progress_form.id)).to exist
end
end
end

describe '#perform' do
subject { described_class.new.perform(in_progress_form.id, user.uuid) }
describe '#perform' do
subject { described_class.new.perform(in_progress_form.id, user.uuid) }

context 'when the form has been deleted' do
before do
in_progress_form.destroy!
context 'when the form has been deleted' do
before do
in_progress_form.destroy!
end

expect_does_nothing
end

expect_does_nothing
end
context 'when form email is present' do
context 'when email confirmation is different' do
before do
in_progress_form.update!(
form_data: JSON.parse(in_progress_form.form_data).except('view:email_confirmation').to_json
)
end

context 'when form email is present' do
context 'when email confirmation is different' do
expect_does_nothing
end

context 'when va profile email is different' do
expect_email_tag('different')
end

context 'when va profile is the same' do
before do
allow(user).to receive(:va_profile_email).and_return('Email@email.com')
end

expect_email_tag('same')

context 'when FormEmailMatchesProfileLog already exists' do
before do
FormEmailMatchesProfileLog.create(user_uuid: user.uuid, in_progress_form_id: in_progress_form.id)
end

expect_does_nothing
end
end

context 'when va profile email is blank' do
before do
expect(user).to receive(:va_profile_email).and_return(nil)
end

expect_email_tag('different')
end
end

context 'when form email is blank' do
before do
in_progress_form.update!(
form_data: JSON.parse(in_progress_form.form_data).except('view:email_confirmation').to_json
form_data: JSON.parse(in_progress_form.form_data).except('email').to_json
)
end

expect_does_nothing
end
end
end

describe 'hca_log_email_diff_in_progress_form disabled' do
before do
allow(Flipper).to receive(:enabled?).with(:hca_log_email_diff_in_progress_form).and_return(false)
in_progress_form.update!(user_uuid: user.uuid)
allow(User).to receive(:find).with(user.uuid).and_return(user)
end

def self.expect_does_nothing
it 'does nothing' do
expect(StatsD).not_to receive(:increment)
expect($redis).not_to receive(:set)

subject
end
end

def self.expect_email_tag(tag)
it "logs that email is #{tag}" do
expect do
subject
end.to trigger_statsd_increment("api.1010ez.in_progress_form_email.#{tag}")

context 'when va profile email is different' do
expect_email_tag('different')
expect($redis.get("HCA::LogEmailDiffJob:#{user.uuid}")).to eq('t')
end
end

describe '#perform' do
subject { described_class.new.perform(in_progress_form.id, user.uuid) }

context 'when va profile is the same' do
context 'when the form has been deleted' do
before do
allow(user).to receive(:va_profile_email).and_return('Email@email.com')
in_progress_form.destroy!
end

expect_email_tag('same')
expect_does_nothing
end

context 'when redis key for the user is already set' do
context 'when form email is present' do
context 'when email confirmation is different' do
before do
$redis.set("HCA::LogEmailDiffJob:#{user.uuid}", 't')
in_progress_form.update!(
form_data: JSON.parse(in_progress_form.form_data).except('view:email_confirmation').to_json
)
end

expect_does_nothing
end
end

context 'when va profile email is blank' do
before do
expect(user).to receive(:va_profile_email).and_return(nil)
context 'when va profile email is different' do
expect_email_tag('different')
end

expect_email_tag('different')
end
end
context 'when va profile is the same' do
before do
allow(user).to receive(:va_profile_email).and_return('Email@email.com')
end

context 'when form email is blank' do
before do
in_progress_form.update!(
form_data: JSON.parse(in_progress_form.form_data).except('email').to_json
)
expect_email_tag('same')

context 'when redis key for the user is already set' do
before do
$redis.set("HCA::LogEmailDiffJob:#{user.uuid}", 't')
end

expect_does_nothing
end
end

context 'when va profile email is blank' do
before do
expect(user).to receive(:va_profile_email).and_return(nil)
end

expect_email_tag('different')
end
end

expect_does_nothing
context 'when form email is blank' do
before do
in_progress_form.update!(
form_data: JSON.parse(in_progress_form.form_data).except('email').to_json
)
end

expect_does_nothing
end
end
end
end
Loading