Skip to content

LG-16298 bug fix for Create Job to Track Deletion Workflow Drop-offs#12277

Merged
kevinsmaster5 merged 10 commits intomainfrom
LG-16298-kmas-bug-fix-track-deletion-drop-off
Jun 30, 2025
Merged

LG-16298 bug fix for Create Job to Track Deletion Workflow Drop-offs#12277
kevinsmaster5 merged 10 commits intomainfrom
LG-16298-kmas-bug-fix-track-deletion-drop-off

Conversation

@kevinsmaster5
Copy link
Contributor

@kevinsmaster5 kevinsmaster5 commented Jun 20, 2025

🎫 Ticket

Link to the relevant ticket:
LG-16298

🛠 Summary of changes

Resolves the timing logic behind the job created with #12187
Adds more comprehensive testing.

📜 Testing Plan

  • In local environment create 2 or more accounts.
  • Sign in with the accounts and create a deletion request by selecting another method at the MFA phase then commencing with deletion
  • Wait for the token to be awarded. You can hasten that by setting the local config account_reset_wait_period_days: 0 Keep the 2 browser tabs with the simulated email open
  • Go into the rails console and pick one of the 2 AccountResetRequest objects and update the granted_at to 2.days.ago
  • Wait for the job to run and check the console for that account to have been updated with a canceled_at date and request_token & granted_token to both be null.
  • Attempt to use the Deletion button in the simulated email tabs.
  • The account with the altered reset request should fail and flash the expired token message but the other one should work.

@kevinsmaster5 kevinsmaster5 marked this pull request as ready for review June 20, 2025 19:00
@kevinsmaster5 kevinsmaster5 requested a review from a team June 20, 2025 19:00
Copy link
Contributor

@vrajmohan vrajmohan left a comment

Choose a reason for hiding this comment

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

Also, why is the changelog under OneAccount?

Copy link
Contributor

Choose a reason for hiding this comment

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

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 referred to prior art with this worker job https://github.com/18F/identity-idp/blob/main/app/jobs/grant_account_reset_requests_and_send_emails_job.rb#L31-L39 using SQL. I assumed it was more efficient to use a SQL query.

Copy link
Contributor

Choose a reason for hiding this comment

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

The ticket calls for a separate field in the model called expired_at? Why are we using cancelled_at, which would denote a cancellation by user as opposed to the system?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed in Slack. Determined that the *_at database change is not needed to accomplish the intent of the story.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this required? Ditto for lines 20, 29 and 30.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed those with ecc86e1

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be a very weak statement of the expected behavior. Wouldn't "it expires requests with expired grant tokens and ignores valid grant tokens" be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct, that's a much better statement. Thank you!
ecc86e1

@kevinsmaster5 kevinsmaster5 force-pushed the LG-16298-kmas-bug-fix-track-deletion-drop-off branch 2 times, most recently from ecc86e1 to 3ea8f36 Compare June 24, 2025 15:59
@kevinsmaster5 kevinsmaster5 marked this pull request as draft June 24, 2025 16:56
@kevinsmaster5 kevinsmaster5 force-pushed the LG-16298-kmas-bug-fix-track-deletion-drop-off branch from 5cea3b1 to bc6b569 Compare June 27, 2025 16:14
@kevinsmaster5 kevinsmaster5 marked this pull request as ready for review June 27, 2025 17:25
@kevinsmaster5 kevinsmaster5 requested a review from a team June 27, 2025 17:27
@kevinsmaster5 kevinsmaster5 merged commit 53d9044 into main Jun 30, 2025
1 check passed
@kevinsmaster5 kevinsmaster5 deleted the LG-16298-kmas-bug-fix-track-deletion-drop-off branch June 30, 2025 13: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.

3 participants