Skip to content

LG-9536 stop writing to "deactivation_reason" for gpo_verification_pending#8405

Merged
theabrad merged 16 commits intomainfrom
abrad-lg-9536-deactivation-gpo
May 22, 2023
Merged

LG-9536 stop writing to "deactivation_reason" for gpo_verification_pending#8405
theabrad merged 16 commits intomainfrom
abrad-lg-9536-deactivation-gpo

Conversation

@theabrad
Copy link
Contributor

🎫 Ticket

LG-9536Link to the relevant ticket.

🛠 Summary of changes

Removes the old gpo_verification_pending deactivation_reason. We now read from the gpo_verification_pending_at timestamp. A roll plan has already occured to move users from the old deactivation reason to the new timestamps.

theabrad added 3 commits May 16, 2023 13:39
changelog: Internal, IdV GPO, remove gpo_verification_pending
deactivation_reason
@theabrad theabrad requested a review from a team May 16, 2023 19:22
@zachmargolis
Copy link
Contributor

fysa @olatifflexion about this column potentially going away

password_reset: 1,
encryption_error: 2,
gpo_verification_pending: 3,
# gpo_verification_pending used to be here. Do not map anything to 3 for deactivation_reason
Copy link
Contributor

Choose a reason for hiding this comment

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

one idea would be to rename it, instead of commenting out?

Suggested change
# gpo_verification_pending used to be here. Do not map anything to 3 for deactivation_reason
gpo_verification_pending__NO_LONGER_USED: 3, # deprecated

@theabrad theabrad requested a review from a team May 18, 2023 20:15
Copy link
Contributor

@eric-gade eric-gade left a comment

Choose a reason for hiding this comment

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

On small inline comment/question, but otherwise LGTM

success: true,
fraud_review_pending: idv_session.profile.fraud_review_pending?,
fraud_rejection: idv_session.profile.fraud_rejection?,
gpo_verification_pending: idv_session.profile.gpo_verification_pending?,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we checking idv_session.profile.gpo_verification_pending? in some places and current_user.gpo_verification_pending_profile? in others because the information hasn't yet been put into the session (for the latter case)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we check idv_session because the profile was just created a few lines up. In other places we check current user because they may be in a new session where the data isn't persisted.

@theabrad theabrad merged commit fd055d5 into main May 22, 2023
@theabrad theabrad deleted the abrad-lg-9536-deactivation-gpo branch May 22, 2023 18:08
amirbey added a commit that referenced this pull request Jun 14, 2023
amirbey added a commit that referenced this pull request Jun 14, 2023
…ctivated (#8549)

* refactor ProfileMaker#save_profile

* resolve profile_maker specs

* remove active param from save_profile call

[skip changelog]

* handle exception if user fails to activate

* happy linting

* define verified_at right before save

* rename exception to error

* restore verified_at setting changed in PR #8405

* only activate unless there are reasons not to

* typo fix

* rename to reason_not_activate

* verified_at should not set until active

* update tests for verified_at to not be set before activaton

* remove uneeded return

use implicit return

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

* remove uneeded return

use implicit return

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

* refactor confirm_that_profile_can_be_activated

---------

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
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.

4 participants