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

1853 id verification consent sms #2094

Merged
merged 10 commits into from
Jan 3, 2023

Conversation

eastandwestwind
Copy link
Contributor

@eastandwestwind eastandwestwind commented Dec 20, 2022

Closes #1853

Code Changes

  • Adjust back-end to support both email and SMS id verification for consent
  • Adds SMS input field to privacy center
  • Adds unit tests

Steps to Confirm

Pre-Merge Checklist

Description Of Changes

Write some things here about the changes and any potential caveats

Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

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

the BE here generally looks good to me, pretty clean updates! just a couple of minor points that i don't think are necessary to address, but wanted to call attention to them.

i also ran through manual testing of the basic functionality here, and covered regression testing with email verification too. it may be worth noting the privacy center config flags used to determine whether email or phone_number are required identity inputs, since that did catch my attention for a sec. i'm not sure if we've got any spot to document privacy center configurability, but at least putting a note in the PR or issue may be helpful if people run into questions when they try to use this in the wild?

a couple of other asides that i noticed along the way, which we may want to create follow ups for:

  • i found the setup docs generally good, but lacking a crucial detail that indicates the actual URL/path that needs to be hit for the given steps. am i missing something obvious? there do seem to be other points in the docs where we've also omitted the URL/path, so perhaps this is a deliberate decision? from the POV of a client, i think i'd find this pretty confusing. what do you think?
  • i also noticed what seems to be a bug in our caching of verification codes - i don't think we're actually setting our verification codes to expire in the cache using the identity_verification_code_ttl_seconds setting, i think we're just relying on the default_ttl_seconds expiration time. i created a follow up ticket for this one (the redis.identity_verification_code_ttl_seconds config property is not being respected #2104)

@eastandwestwind
Copy link
Contributor Author

Thanks so much for the thorough testing and review @adamsachs ! A couple things:

  1. Based on your feedback, I've updated comments in the code to reflect he configurability of the PC:
Consent requests, unlike privacy requests, only accept 1 identity type- email or phone. 
    These identity types are configurable as optional/required within the privacy center config.json.
    If both identity types are provided, we'll use identity type if defined in
    CONFIG.notifications.notification_service_type, else default to email.
  1. As discussed in Slack, now that I know where the new docs live, I'll make the appropriate updates to Twilio configuration docs for clarity- Add clarity to Twilio Config docs #2107
  2. Follow-up ticket for exception handling- Add new Exception type to the message dispatcher for 422 type errors #2106
  3. Thanks so much for working on use verification ttl property when setting in cache #2105, I'll take a look today!

@adamsachs
Copy link
Contributor

nice, this all looks good to me on the backend pieces - thanks for addressing everything i brought up in a thoughtful way!

not sure if we want to go ahead and merge without further FE code review? things certainly seem functional, but can't comment much on the code itself :)

@eastandwestwind eastandwestwind merged commit 65dbcfc into main Jan 3, 2023
@eastandwestwind eastandwestwind deleted the 1853-id-verification-consent-sms branch January 3, 2023 19:44
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.

Support SMS-based ID verification for consent
2 participants