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

coope93 · 2024-11-12T17:51:54Z

Summary

This work is behind a feature toggle (flipper): YES
Refactored the HCA::LogEmailDiffJob to not use a redis key that is never deleted to log if an email address is different between the VA Profile email and the 10-10EZ submission email address. Using a new table to store the user uuid and the in progress form id instead of expecting a redis key to always exist.
10-10 Health Apps
Flipper toggle named hca_log_email_diff_in_progress_form.

Related issue(s)

[BE] 10-10EZ - Improvement: Update HCA::LogEmailDiffJob to remove the need for a redis key with a ttl of -1 va.gov-team#94285
Address concerns from this slack thread.

Testing done

New code is covered by unit tests
Kept the same functionality and logic, just updated where we are using the redis key
Turn it on in staging to validate it manually

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?

github-actions · 2024-11-12T17:52:51Z

Error: A file (or its parent directories) was deleted but its reference still exists in CODEOWNERS. Please update the .github/CODEOWNERS file and delete the entry for the Offending file:

github-actions · 2024-11-12T17:52:53Z

Error: A file (or its parent directories) does not have a CODEOWNERS entry. Please update the .github/CODEOWNERS file and add the entry for the Offending file:

github-actions · 2024-11-12T18:06:30Z

Error: A file (or its parent directories) was deleted but its reference still exists in CODEOWNERS. Please update the .github/CODEOWNERS file and delete the entry for the Offending file:

github-actions · 2024-11-12T18:06:36Z

Error: A file (or its parent directories) does not have a CODEOWNERS entry. Please update the .github/CODEOWNERS file and add the entry for the Offending file:

dellerbie · 2024-11-13T17:27:07Z

app/sidekiq/hca/log_email_diff_job.rb

@@ -28,5 +36,29 @@ def perform(in_progress_form_id, user_uuid)
      )
      $redis.set(redis_key, 't')
    end
+
+    def log_different_email(in_progress_form_id, user_uuid)
+      return if InProgressEmailMatchLog.exists?(user_uuid:, in_progress_form_id:)


would race conditions be a concern in this method?

I don't think so, because of the unique constraint in this PR. If two concurrent jobs both returned true for exists?, one would fail when trying the create since one of them would successfully create the record and the index would throw the exception (which would just kill the job). I think that should handle it?

I'm not totally sure the job would be killed since you're using create and not create! but yes, the unique constraint should prevent dupes.

Yeah, you're right. Poor wording on my part. The create will fail, and since it's at the end of the job and there are no retries for this job, it wouldn't cause any other issues. But I think we should be okay as far as race conditions, though. I appreciate the call out on it! 👍

I don't know that we gain anything by the create!? Although I guess if it throws an exception I think we'd have some automatic logging that would catch it, so if we did find we were running into issues with this that could be a way to identify it 🤔

Yeah no biggie, it was good exercise to think through these things though

dellerbie · 2024-11-13T17:34:42Z

app/sidekiq/hca/log_email_diff_job.rb

+        "api.1010ez.in_progress_form_email.#{tag_text}"
+      )
+
+      InProgressEmailMatchLog.create(user_uuid:, in_progress_form_id:)


So this would create a record if the emails are the same and if they are different. The method is called log_different_email but it looks like it creates a record even if it's the same?

Yeah, that could probably be clearer. The names for this one are tricky to me. Would log_email_difference make more sense? Open to suggestions on the name 🙂

Yes, I don't like to knock names if I don't have a better suggestion but log_email_difference does seem closer to what's happening here

dellerbie

lgtm , just confirm if you want to use InProgressEmailMatchLog.create! over InProgressEmailMatchLog.create

github-actions · 2024-11-13T20:56:54Z

Error: A file (or its parent directories) was deleted but its reference still exists in CODEOWNERS. Please update the .github/CODEOWNERS file and delete the entry for the Offending file:

github-actions · 2024-11-13T20:56:55Z

Error: A file (or its parent directories) does not have a CODEOWNERS entry. Please update the .github/CODEOWNERS file and add the entry for the Offending file:

github-actions · 2024-11-15T18:23:26Z

Error: A file (or its parent directories) does not have a CODEOWNERS entry. Please update the .github/CODEOWNERS file and add the entry for the Offending file:

coope93 · 2024-11-19T14:12:38Z

app/sidekiq/hca/log_email_diff_job.rb

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


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.

Mitch-A6

LGTM. Interesting pattern with the specs - makes it nicely readable!

github-actions · 2024-11-20T18:37:10Z

Backend-review-group approval confirmed.

coope93 added 7 commits November 12, 2024 12:42

add new in_progress_email_match_logs table

d258fab

separate index and create table migrations

e39cb17

delete new class

961a015

add flipper toggle for log_email_diff_job

2de17ec

add class for new table

391c272

update log_email_diff_job to use new table

e93075a

use .exists? for efficiency

0b8d0a9

va-vfs-bot had a problem deploying to 94285-refactor-log-email-diff-job/main/main November 12, 2024 17:52 Error

github-actions bot added the codeowners-delete-failure label Nov 12, 2024

github-actions bot added codeowners-addition-failure test-passing labels Nov 12, 2024

add codeowners for new model

c77d7d9

github-actions bot removed the test-passing label Nov 12, 2024

coope93 mentioned this pull request Nov 12, 2024

[10-10CG] Create new in_progress_email_match_logs table #19397

Merged

8 tasks

github-actions bot added the test-passing label Nov 12, 2024

Merge branch 'master' into 94285-create-email-diff-table

2ce8ce4

va-vfs-bot temporarily deployed to 94285-refactor-log-email-diff-job/main/main November 12, 2024 22:38 Inactive

Merge branch 'master' into 94285-create-email-diff-table

83b2b36

dellerbie reviewed Nov 13, 2024

View reviewed changes

dellerbie previously approved these changes Nov 13, 2024

View reviewed changes

rename method to be clearer

783ee2e

github-actions bot added test-passing and removed test-passing labels Nov 13, 2024

github-actions bot added the codeowners-delete-failure label Nov 15, 2024

github-actions bot added the codeowners-addition-failure label Nov 15, 2024

Merge branch 'master' into 94285-refactor-log-email-diff-job

7a19608

github-actions bot added require-backend-approval test-passing and removed require-backend-approval codeowners-delete-failure codeowners-addition-failure test-passing labels Nov 15, 2024

coope93 requested a review from dellerbie November 15, 2024 18:42

github-actions bot added require-backend-approval ready-for-backend-review and removed require-backend-approval labels Nov 15, 2024

coope93 requested a review from Mitch-A6 November 15, 2024 19:48

github-actions bot added require-backend-approval and removed require-backend-approval labels Nov 15, 2024

va-vfs-bot deployed to 94285-refactor-log-email-diff-job/main/main November 15, 2024 22:40 View deployment

coope93 commented Nov 19, 2024

View reviewed changes

Mitch-A6 approved these changes Nov 19, 2024

View reviewed changes

platform-support-slack-integration bot mentioned this pull request Nov 19, 2024

Can I get a PR review on https://github.com/department-of-ve... department-of-veterans-affairs/va.gov-team#97386

Closed

coope93 added Please Review This PR had changes requested by reviewer AND IS NOW COMPLETE Ready For Review labels Nov 19, 2024

RachalCassity approved these changes Nov 20, 2024

View reviewed changes

RachalCassity self-assigned this Nov 20, 2024

github-actions bot removed the ready-for-backend-review label Nov 20, 2024

coope93 merged commit 950ae18 into master Nov 20, 2024
56 of 58 checks passed

coope93 deleted the 94285-refactor-log-email-diff-job branch November 20, 2024 19:06

coope93 changed the title ~~[10-10CG] Update LogEmailDiffJob to not use redis key~~ [10-10EZ] Update LogEmailDiffJob to not use redis key Nov 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

coope93 commented Nov 12, 2024

github-actions bot commented Nov 12, 2024

github-actions bot commented Nov 12, 2024

github-actions bot commented Nov 12, 2024

github-actions bot commented Nov 12, 2024

dellerbie Nov 13, 2024

coope93 Nov 13, 2024

dellerbie Nov 13, 2024

coope93 Nov 13, 2024 •

edited

Loading

coope93 Nov 13, 2024

dellerbie Nov 13, 2024

dellerbie Nov 13, 2024

coope93 Nov 13, 2024

dellerbie Nov 13, 2024

dellerbie left a comment

github-actions bot commented Nov 13, 2024

github-actions bot commented Nov 13, 2024

github-actions bot commented Nov 15, 2024

coope93 Nov 19, 2024

Mitch-A6 left a comment

github-actions bot commented Nov 20, 2024

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

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

Conversation

coope93 commented Nov 12, 2024

Summary

Related issue(s)

Testing done

What areas of the site does it impact?

Acceptance criteria

Requested Feedback

github-actions bot commented Nov 12, 2024

github-actions bot commented Nov 12, 2024

github-actions bot commented Nov 12, 2024

github-actions bot commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coope93 Nov 13, 2024 • edited Loading

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

dellerbie left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 13, 2024

github-actions bot commented Nov 13, 2024

github-actions bot commented Nov 15, 2024

Choose a reason for hiding this comment

Mitch-A6 left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 20, 2024

coope93 Nov 13, 2024 •

edited

Loading