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

Add message if send_sms fail with an Exception #316

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

yoandyshyno
Copy link

@yoandyshyno yoandyshyno commented Sep 30, 2019

Description

In function send_sms of Twilio gateway we handled the call to client.messages.create()

Motivation and Context

In the Twilio gateway when calling function send_sms an exception might be raised, in this case an internal error is produced. For some cases we have an application which use two_factor and we want instead to show a message to the users. Fixes #315

How Has This Been Tested?

I tested the changes locally with a working Django application.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@yoandyshyno yoandyshyno changed the title Patch 1 Add message if send_sms fail with an Exception Sep 30, 2019
Copy link
Collaborator

@moggers87 moggers87 left a comment

Choose a reason for hiding this comment

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

This is very twilio specific and I doubt this bug only affects this backend. See my comment: #315 (comment)

Also, tests!

…age is added to the messages instance with this text.
@codecov
Copy link

codecov bot commented Oct 1, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@9186a52). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #316   +/-   ##
=========================================
  Coverage          ?   96.61%           
=========================================
  Files             ?       39           
  Lines             ?     1713           
  Branches          ?      118           
=========================================
  Hits              ?     1655           
  Misses            ?       36           
  Partials          ?       22

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9186a52...f4bf1ba. Read the comment docs.

 Add correction in text.
@yoandyshyno
Copy link
Author

This is very twilio specific and I doubt this bug only affects this backend. See my comment: #315 (comment)

Also, tests!

The tests are in place, do you need something else at the moment?

@moggers87
Copy link
Collaborator

@yoandyshyno I did say that these changes are Twilio specific and I don't think the bug you found only affects Twilio. Have you looked into that?

@yoandyshyno
Copy link
Author

@moggers87 , I am sorry, I don't understand what else can be affecting the Bug, right now there is only one gateway that is able to send sms (Twilio).

@moggers87
Copy link
Collaborator

Your issue was with an uncaught exception causing an internal error response. Our view code should be catching errors from whatever backend you're using and as it's not I would assume this bug isn't specific to Twilio. This is why I asked for a stacktrace in #315

@yoandyshyno
Copy link
Author

Your issue was with an uncaught exception causing an internal error response. Our view code should be catching errors from whatever backend you're using and as it's not I would assume this bug isn't specific to Twilio. This is why I asked for a stacktrace in #315

Here is the Stacktrace:
Traceback:

File "/src/app/venv/CP/lib/python3.7/site-packages/django/core/handlers/exception.py" in inner
34. response = get_response(request)

File "/src/app/venv/CP/lib/python3.7/site-packages/django/core/handlers/base.py" in _get_response
126. response = self.process_exception_by_middleware(e, request)

File "/src/app/venv/CP/lib/python3.7/site-packages/django/core/handlers/base.py" in _get_response
124. response = wrapped_callback(request, *callback_args, **callback_kwargs)

File "/src/app/venv/CP/lib/python3.7/site-packages/django/views/generic/base.py" in view
68. return self.dispatch(request, *args, **kwargs)

File "/src/app/venv/CP/lib/python3.7/site-packages/django/utils/decorators.py" in _wrapper
45. return bound_method(*args, **kwargs)

File "/src/app/venv/CP/lib/python3.7/site-packages/django/views/decorators/debug.py" in sensitive_post_parameters_wrapper
76. return view(request, *args, **kwargs)

File "/src/app/venv/CP/lib/python3.7/site-packages/django/utils/decorators.py" in _wrapper
45. return bound_method(*args, **kwargs)

File "/src/app/venv/CP/lib/python3.7/site-packages/django/views/decorators/cache.py" in _wrapped_view_func
44. response = view_func(request, *args, **kwargs)

File "/src/app/venv/CP/lib/python3.7/site-packages/formtools/wizard/views.py" in dispatch
248. response = super(WizardView, self).dispatch(request, *args, **kwargs)

File "/src/app/venv/CP/lib/python3.7/site-packages/django/views/generic/base.py" in dispatch
88. return handler(request, *args, **kwargs)

File "/src/app/venv/CP/lib/python3.7/site-packages/two_factor/views/core.py" in post
100. return super(LoginView, self).post(*args, **kwargs)

File "/src/app/venv/CP/lib/python3.7/site-packages/two_factor/views/utils.py" in post
128. return super(IdempotentSessionWizardView, self).post(*args, **kwargs)

File "/src/app/venv/CP/lib/python3.7/site-packages/formtools/wizard/views.py" in post
312. return self.render_next_step(form)

File "/src/app/venv/CP/lib/python3.7/site-packages/formtools/wizard/views.py" in render_next_step
330. return self.render(new_form, **kwargs)

File "/src/app/venv/CP/lib/python3.7/site-packages/two_factor/views/core.py" in render
163. self.get_device().generate_challenge()

File "/src/app/venv/CP/lib/python3.7/site-packages/two_factor/models.py" in generate_challenge
116. send_sms(device=self, token=token)

File "/src/app/venv/CP/lib/python3.7/site-packages/two_factor/gateways/init.py" in send_sms
16. gateway.send_sms(device=device, token=token)

File "/src/app/venv/CP/lib/python3.7/site-packages/two_factor/gateways/twilio/gateway.py" in send_sms
65. body=body)

File "/src/app/venv/CP/lib/python3.7/site-packages/twilio/rest/api/v2010/account/message/init.py" in create
92. data=data,

File "/src/app/venv/CP/lib/python3.7/site-packages/twilio/base/version.py" in create
209. raise self.exception(method, uri, response, 'Unable to create record')

Exception Type: TwilioRestException at /account/login/
Exception Value: HTTP 400 error: Unable to create record: To number: +31343182121, is not a mobile number

@yoandyshyno
Copy link
Author

I noticed in core.py , generate_challenge() is missing a try - except in some places.

@patroqueeet
Copy link

is this issue stalled?

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 displaying a message in case of a Twilio exception
3 participants