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

LG-14320 Add deactivate IPP cancelled profiles task #11180

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

shanechesnutt-ft
Copy link
Contributor

@shanechesnutt-ft shanechesnutt-ft commented Aug 30, 2024

🎫 Ticket

Link to the relevant ticket:
LG-14320

🛠 Summary of changes

Added a script to deactivate profiles that are considered to be IPP verification cancelled state.

📜 Testing Plan

Scenario: Running backfill script

  • Create data using the seed_users_with_profiles.rb seed file
bundle exec rake seed:broken_profiles
  • Check that number of profile in broken state is greater than 0 using rails console
    InPersonEnrollment.where(status: [:expired, :cancelled, :failed]).
      includes(:profile).
      where.not(profile: { in_person_verification_pending_at: nil }).
      count

Note: It might be good to setup the seeder to create a few pending profiles for testing to ensure those do not get updated.

  • Run the backfill script to backup data without updating profiles
bundle exec rake profiles:backfill_deactivated_ipp_verification_cancelled UPDATE_PROFILES=false
  • Verify that log output contains the expected profiles information (id, deactivation_reason, in_person_verification_pending_at) is contained in it
  • Copy the log output and save it to a file
  • Run the backfill script data updating profiles
bundle exec rake profiles:backfill_deactivated_ipp_verification_cancelled UPDATE_PROFILES=true
  • Check the number of profile in broken state is 0 using rails console
    InPersonEnrollment.where(status: [:expired, :cancelled, :failed]).
      includes(:profile).
      where.not(profile: { in_person_verification_pending_at: nil }).
      count

Scenario: Running rollback script

  • Run the rollback script
bundle exec rake profiles:rollback_backfill_deactivated_ipp_verification_cancelled BACKFILL_OUTPUT="$(cat output)"
  • Check the number of profile in broken state is back to the original count before the backfill script ran using rails console
    InPersonEnrollment.where(status: [:expired, :cancelled, :failed]).
      includes(:profile).
      where.not(profile: { in_person_verification_pending_at: nil }).
      count

enrollment.profile = profile
enrollment.save!
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

For Run the backfill script to backup data without updating profiles - we are just running that to 1. confirm batches are working and 2. ensure the print out is working as expected, correct? What do you think about adding a sentence on the testing plan to confirm both? (I am adding it here so it can be resolved.)

@@ -0,0 +1,86 @@
# frozen_string_literal: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we should create data that should not be updated to ensure/check that we are not updating establishing, pending, and passed enrollments and/or those enrollments that don't have a timestamp for the profile? What are your thoughts on this? Overkill?

Copy link
Member

Choose a reason for hiding this comment

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

I like this idea if it's not too much trouble!

Copy link
Member

Choose a reason for hiding this comment

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

If it doesn't exist already, can we make a follow-up ticket to remove these extra tasks once it's been run successfully in production?

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 don't think we have a ticket for this yet. I will make sure it is created.

@shanechesnutt-ft
Copy link
Contributor Author

I just wanted to note that I am going to delete lib/tasks/seed_users_with_profiles.rake before this is merged

@gina-yamada
Copy link
Contributor

I created LG-14391 Clean up Backfill In Person Pending At Rake Task to delete this script so it would not get forgotten.

@shanechesnutt-ft shanechesnutt-ft marked this pull request as ready for review September 4, 2024 15:48
@@ -75,4 +75,67 @@ namespace :profiles do

warn "backfill_in_person_verification_pending_at left #{profiles.count} rows"
end

##
# Usage:
Copy link
Contributor

@gina-yamada gina-yamada Sep 4, 2024

Choose a reason for hiding this comment

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

Checked prod on 09/04/24 at approx. 11:30ET

  • Expired (3786), Cancelled *813), Failed (587) - Total 5186 in prod
  • Establishing 1313

Copy link
Member

@jennyverdeyen jennyverdeyen left a comment

Choose a reason for hiding this comment

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

Ran through the testing plan and the tasks work as described. Looks great!

Copy link
Contributor

@eileen-nava eileen-nava left a comment

Choose a reason for hiding this comment

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

I didn't manually test this, but I reviewed it and everything looks good. Approved. ✅


warn "Updating #{profile_data.count} records"
profile_data.in_groups_of(1000, false) do |batch|
batch.each do |profile_datum|
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice use of datum. 👏🏻

@gina-yamada
Copy link
Contributor

I did locally testing- and it worked as expected. I also worked through this with Shane a bit so happy others approved.

@shanechesnutt-ft
Copy link
Contributor Author

I did a squash and removed commits from my branch. I removed the seed file that was added testing. Need a quick double check then I can merge.

@eileen-nava
Copy link
Contributor

@shanechesnutt-ft I did a once-over just now. Looks good. 👍🏻

@jennyverdeyen
Copy link
Member

Ran through the test plan again after your changes. Looks good, script works as described!

Copy link
Member

@matthinz matthinz left a comment

Choose a reason for hiding this comment

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

LGTM

timestamp = profile.in_person_verification_pending_at

warn "#{profile.id},#{profile.deactivation_reason},#{timestamp}"
if update_profiles
Copy link
Member

Choose a reason for hiding this comment

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

TBH I wouldn't change it at this point, but I feel like it would be a tiny bit more ideal if the warn on line 101 occurred inside this conditional, or there was otherwise some indication if update_profiles was false. It'd otherwise be easy to miss that this script didn't change anything if the environment variable wasn't set.

But, it looks like this has already been extensively tested, so it doesn't feel like it's worth changing. I'd just make sure someone confirms the script was successful, which was probably part of the plan all along anyway. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good callout! I did add checks in the roll plan for verifying the data.

changelog: Internal, Data Normalization, Add a backfill script to
deactivate all profiles consider to be in ipp verfication cancelled
state.

co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>
@shanechesnutt-ft shanechesnutt-ft merged commit dd28eba into main Sep 6, 2024
2 checks passed
@shanechesnutt-ft shanechesnutt-ft deleted the sc/LG-14320 branch September 6, 2024 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants