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-9067: Account reset account deleted event fixes #7982

Merged
merged 4 commits into from
Mar 15, 2023

Conversation

olatifflexion
Copy link
Contributor

changelog: Internal, Attempts API, Update & fixes Account reset account deleted event

@@ -0,0 +1,5 @@
class AddSpIssuerFieldToAccountResetRequest < ActiveRecord::Migration[7.0]
def change
add_column :account_reset_requests, :sp_issuer, :string
Copy link
Contributor

Choose a reason for hiding this comment

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

typically we call this column service_provider or issuer, I have a slight preference for issuer and maybe if we wanted to be clear, we could call it requesting_issuer or something

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 like requesting_issuer, I added sp_issuer because it was in the ticket but I will change this to requesting_issuer.

@olatifflexion olatifflexion force-pushed the olatif/LG-9067-acct-reset-bug branch 2 times, most recently from 2154608 to bba817f Compare March 14, 2023 20:11
changelog: Internal, Attempts API, Update & fixes Account reset account deleted event
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM, some small comments but this is good to go!

app/services/account_reset/track_irs_event.rb Outdated Show resolved Hide resolved
sp: sp,
cookie_device_uuid: cookies[:device],
sp_request_uri: nil,
enabled_for_session: sp.irs_attempts_api_enabled?,
Copy link
Contributor

Choose a reason for hiding this comment

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

@n1zyy this just reminder me of this line in your PR:

https://github.com/18F/identity-idp/pull/7951/files#diff-0414a45947b697e51d740c5ba32eb6c6445f4c795807250ce68106de5ff7e414R133

We have the enabled_for_session so the tracker will no-op if it's not enabled, we can do that instead of having a. nillable tracker

app/validators/account_reset/granted_token_validator.rb Outdated Show resolved Hide resolved
spec/features/account_reset/delete_account_spec.rb Outdated Show resolved Hide resolved
spec/services/account_reset/create_request_spec.rb Outdated Show resolved Hide resolved
@olatifflexion olatifflexion merged commit 67e40bc into main Mar 15, 2023
@olatifflexion olatifflexion deleted the olatif/LG-9067-acct-reset-bug branch March 15, 2023 16:33
svalexander pushed a commit that referenced this pull request Mar 16, 2023
* LG-9067: Account reset account deleted event fixes

changelog: Internal, Attempts API, Update & fixes Account reset account deleted event
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.

2 participants