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-14196 | idv_consent_given -> idv_consent_given_at #11168

Merged
merged 10 commits into from
Sep 6, 2024

Conversation

n1zyy
Copy link
Member

@n1zyy n1zyy commented Aug 29, 2024

🎫 Ticket

Link to the relevant ticket:
LG-14196

🛠 Summary of changes

<strikeDo not merge (at least, do not deploy) until after the previous PR for this story is released. (Done!)

This moves everything over to using idv_consent_given_at vs. the old boolean. Since most stuff only cares about the boolean value, I added a lil' helper method.

This is part 2, which moves everything over to using the new
timestamp. Most things only care about the boolean state, so I added
a method.
n1zyy added 3 commits August 29, 2024 17:24
This is part 2, which moves everything over to using the new
timestamp. Most things only care about the boolean state, so I added
a method.
changelog: Internal, Socure, Replace idv_consent_given with timestamp
@n1zyy n1zyy marked this pull request as ready for review September 4, 2024 14:13
Copy link
Member

@jmhooper jmhooper left a comment

Choose a reason for hiding this comment

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

One note because I don't think we can stop writing the old value quite yet

@@ -36,7 +36,6 @@ def update
)

if result.success?
idv_session.idv_consent_given = true
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to keep these around for 1 more release cycle. Old instances will still be inspecting idv_session.idv_consent_given. We need to stop reading from these everywhere before we can stop writing it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, good catch! The first PR started setting the timestamp, allowing this to begin reading it immediately, but I overlooked the inverse which will occur in the 50/50 state.

@@ -296,6 +295,10 @@ def desktop_selfie_test_mode_enabled?
IdentityConfig.store.doc_auth_selfie_desktop_test_mode
end

def idv_consent_given?
Copy link
Member

Choose a reason for hiding this comment

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

Smart to just make this a predicate method

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, thank you for addressing the 50/50 state issue

@n1zyy n1zyy merged commit 7753d43 into main Sep 6, 2024
2 checks passed
@n1zyy n1zyy deleted the mattw/LG-14196_consent_part2 branch September 6, 2024 19:18
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