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

feat: log ip address on callback from twilio #3937

Merged
merged 7 commits into from
Jun 7, 2022

Conversation

tshuli
Copy link
Contributor

@tshuli tshuli commented Jun 2, 2022

Problem

  • Currently, we are unable to match the sender IP address to the twilio sms delivery report

Solution

  • pass sender IP address to twilio as part of query param for callback

Tests

  • Create form. Test that email and mobile field OTP verification and submission works
  • Check that twilio webhooks contain sender IP address after OTP has been sent
  • Check that admin contact number adding works normally

hanstirtaputra
hanstirtaputra previously approved these changes Jun 2, 2022
Copy link
Contributor

@hanstirtaputra hanstirtaputra left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

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

as discussed, would be better to pass the IP as a param to Twilio

@tshuli
Copy link
Contributor Author

tshuli commented Jun 2, 2022

as discussed, would be better to pass the IP as a param to Twilio

Thanks! I changed the implementation to pass it as a query param in the callback

@tshuli tshuli dismissed hanstirtaputra’s stale review June 2, 2022 09:04

implementation changed

@tshuli tshuli temporarily deployed to staging-al2 June 2, 2022 13:59 Inactive
@tshuli tshuli temporarily deployed to staging-al2 June 2, 2022 14:11 Inactive
@tshuli tshuli changed the title feat: log mobile number when otp generated feat: log ip address on callback from twilio Jun 3, 2022
@tshuli tshuli force-pushed the feat/log-mobileno-verification branch from c16ed53 to cedbcbd Compare June 6, 2022 05:30
@tshuli tshuli force-pushed the feat/log-mobileno-verification branch from cedbcbd to 293dc77 Compare June 6, 2022 05:30
@karrui
Copy link
Contributor

karrui commented Jun 6, 2022

will need to fix tests

@tshuli tshuli force-pushed the feat/log-mobileno-verification branch from 69c9f1f to bac944d Compare June 6, 2022 14:47
@tshuli
Copy link
Contributor Author

tshuli commented Jun 6, 2022

Tests were failing because req.url contains only the relative path, and new URL() requires the absolute path (which has to be constructed using req.protocol + '://' + req.get('host') + req.originalUrl

Also added try catch block in case the URL constructor fails

@tshuli tshuli closed this Jun 6, 2022
@tshuli tshuli reopened this Jun 6, 2022
@tshuli tshuli force-pushed the feat/log-mobileno-verification branch from b165249 to 81ae615 Compare June 7, 2022 00:42
@tshuli tshuli merged commit 0493774 into develop Jun 7, 2022
@tshuli tshuli deleted the feat/log-mobileno-verification branch June 7, 2022 01:08
@tshuli tshuli mentioned this pull request Jun 7, 2022
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