mirror of
https://github.com/GSA/notifications-api.git
synced 2026-02-03 09:51:11 -05:00
Remove per-client SMS exception classes
In response to: [^1].
The stacktrace conveys the same and more information. We don't do
anything different for each exception class, so there's no value
in having three of them over one exception.
I did think about DRYing-up the duplicate exception behaviour into
the base class one. This isn't ideal because the base class would
be making assumptions about how inheriting classes make requests,
which might change with future providers. Although it might be nice
to have more info in the top-level message, we'll still get it in
the stacktrace e.g.
ValueError: Expected 'code' to be '0'
During handling of the above exception, another exception occurred:
app.clients.sms.SmsClientResponseException: SMS client error (Invalid response JSON)
requests.exceptions.ReadTimeout
During handling of the above exception, another exception occurred:
app.clients.sms.SmsClientResponseException: SMS client error (Request failed)
[^1]: https://github.com/alphagov/notifications-api/pull/3493#discussion_r837363717
This commit is contained in:
@@ -2,11 +2,9 @@ from urllib.parse import parse_qs
|
||||
|
||||
import pytest
|
||||
import requests_mock
|
||||
from requests import HTTPError
|
||||
from requests.exceptions import ConnectTimeout, ReadTimeout
|
||||
|
||||
from app.clients.sms.firetext import (
|
||||
FiretextClientResponseException,
|
||||
SmsClientResponseException,
|
||||
get_firetext_responses,
|
||||
)
|
||||
@@ -116,9 +114,7 @@ def test_try_send_sms_raises_if_firetext_rejects(mocker, mock_firetext_client):
|
||||
request_mock.post('https://example.com/firetext', json=response_dict, status_code=200)
|
||||
mock_firetext_client.try_send_sms(to, content, reference, False, 'sender')
|
||||
|
||||
assert exc.value.status_code == 200
|
||||
assert '"description": "Some kind of error"' in exc.value.text
|
||||
assert '"code": 1' in exc.value.text
|
||||
assert "Invalid response JSON" in str(exc.value)
|
||||
|
||||
|
||||
def test_try_send_sms_raises_if_firetext_rejects_with_unexpected_data(mocker, mock_firetext_client):
|
||||
@@ -129,9 +125,7 @@ def test_try_send_sms_raises_if_firetext_rejects_with_unexpected_data(mocker, mo
|
||||
request_mock.post('https://example.com/firetext', json=response_dict, status_code=400)
|
||||
mock_firetext_client.try_send_sms(to, content, reference, False, 'sender')
|
||||
|
||||
assert exc.value.status_code == 400
|
||||
assert exc.value.text == '{"something": "gone bad"}'
|
||||
assert type(exc.value.exception) == HTTPError
|
||||
assert "Request failed" in str(exc.value)
|
||||
|
||||
|
||||
def test_try_send_sms_raises_if_firetext_fails_to_return_json(notify_api, mock_firetext_client):
|
||||
@@ -142,28 +136,24 @@ def test_try_send_sms_raises_if_firetext_fails_to_return_json(notify_api, mock_f
|
||||
request_mock.post('https://example.com/firetext', text=response_dict, status_code=200)
|
||||
mock_firetext_client.try_send_sms(to, content, reference, False, 'sender')
|
||||
|
||||
assert 'Code 200 text NOT AT ALL VALID JSON {"key" : "value"}} exception Expecting value: line 1 column 1 (char 0)' in str(exc.value) # noqa
|
||||
assert exc.value.status_code == 200
|
||||
assert exc.value.text == 'NOT AT ALL VALID JSON {"key" : "value"}}'
|
||||
assert "Invalid response JSON" in str(exc.value)
|
||||
|
||||
|
||||
def test_try_send_sms_raises_if_firetext_rejects_with_connect_timeout(rmock, mock_firetext_client):
|
||||
to = content = reference = 'foo'
|
||||
|
||||
with pytest.raises(FiretextClientResponseException) as exc:
|
||||
with pytest.raises(SmsClientResponseException) as exc:
|
||||
rmock.register_uri('POST', 'https://example.com/firetext', exc=ConnectTimeout)
|
||||
mock_firetext_client.try_send_sms(to, content, reference, False, 'sender')
|
||||
|
||||
assert exc.value.status_code == 504
|
||||
assert exc.value.text == 'Gateway Time-out'
|
||||
assert "Request failed" in str(exc.value)
|
||||
|
||||
|
||||
def test_try_send_sms_raises_if_firetext_rejects_with_read_timeout(rmock, mock_firetext_client):
|
||||
to = content = reference = 'foo'
|
||||
|
||||
with pytest.raises(FiretextClientResponseException) as exc:
|
||||
with pytest.raises(SmsClientResponseException) as exc:
|
||||
rmock.register_uri('POST', 'https://example.com/firetext', exc=ReadTimeout)
|
||||
mock_firetext_client.try_send_sms(to, content, reference, False, 'sender')
|
||||
|
||||
assert exc.value.status_code == 504
|
||||
assert exc.value.text == 'Gateway Time-out'
|
||||
assert "Request failed" in str(exc.value)
|
||||
|
||||
@@ -1,11 +1,9 @@
|
||||
import pytest
|
||||
import requests_mock
|
||||
from requests import HTTPError
|
||||
from requests.exceptions import ConnectTimeout, ReadTimeout
|
||||
|
||||
from app import mmg_client
|
||||
from app.clients.sms import SmsClientResponseException
|
||||
from app.clients.sms.mmg import MMGClientResponseException, get_mmg_responses
|
||||
from app.clients.sms.mmg import SmsClientResponseException, get_mmg_responses
|
||||
|
||||
|
||||
@pytest.mark.parametrize('detailed_status_code, result', [
|
||||
@@ -85,10 +83,7 @@ def test_try_send_sms_raises_if_mmg_rejects(notify_api, mocker):
|
||||
request_mock.post('https://example.com/mmg', json=response_dict, status_code=400)
|
||||
mmg_client.try_send_sms(to, content, reference, False, 'sender')
|
||||
|
||||
assert exc.value.status_code == 400
|
||||
assert '"Error": 206' in exc.value.text
|
||||
assert '"Description": "Some kind of error"' in exc.value.text
|
||||
assert type(exc.value.exception) == HTTPError
|
||||
assert "Request failed" in str(exc.value)
|
||||
|
||||
|
||||
def test_try_send_sms_raises_if_mmg_fails_to_return_json(notify_api, mocker):
|
||||
@@ -99,28 +94,24 @@ def test_try_send_sms_raises_if_mmg_fails_to_return_json(notify_api, mocker):
|
||||
request_mock.post('https://example.com/mmg', text=response_dict, status_code=200)
|
||||
mmg_client.try_send_sms(to, content, reference, False, 'sender')
|
||||
|
||||
assert 'Code 200 text NOT AT ALL VALID JSON {"key" : "value"}} exception Expecting value: line 1 column 1 (char 0)' in str(exc.value) # noqa
|
||||
assert exc.value.status_code == 200
|
||||
assert exc.value.text == 'NOT AT ALL VALID JSON {"key" : "value"}}'
|
||||
assert "Invalid response JSON" in str(exc.value)
|
||||
|
||||
|
||||
def test_try_send_sms_raises_if_mmg_rejects_with_connect_timeout(rmock):
|
||||
to = content = reference = 'foo'
|
||||
|
||||
with pytest.raises(MMGClientResponseException) as exc:
|
||||
with pytest.raises(SmsClientResponseException) as exc:
|
||||
rmock.register_uri('POST', 'https://example.com/mmg', exc=ConnectTimeout)
|
||||
mmg_client.try_send_sms(to, content, reference, False, 'sender')
|
||||
|
||||
assert exc.value.status_code == 504
|
||||
assert exc.value.text == 'Gateway Time-out'
|
||||
assert "Request failed" in str(exc.value)
|
||||
|
||||
|
||||
def test_try_send_sms_raises_if_mmg_rejects_with_read_timeout(rmock):
|
||||
to = content = reference = 'foo'
|
||||
|
||||
with pytest.raises(MMGClientResponseException) as exc:
|
||||
with pytest.raises(SmsClientResponseException) as exc:
|
||||
rmock.register_uri('POST', 'https://example.com/mmg', exc=ReadTimeout)
|
||||
mmg_client.try_send_sms(to, content, reference, False, 'sender')
|
||||
|
||||
assert exc.value.status_code == 504
|
||||
assert exc.value.text == 'Gateway Time-out'
|
||||
assert "Request failed" in str(exc.value)
|
||||
|
||||
@@ -1,11 +1,9 @@
|
||||
import pytest
|
||||
import requests_mock
|
||||
from requests import HTTPError
|
||||
from requests.exceptions import ConnectTimeout, ReadTimeout
|
||||
|
||||
from app import reach_client
|
||||
from app.clients.sms import SmsClientResponseException
|
||||
from app.clients.sms.reach import ReachClientResponseException
|
||||
|
||||
# TODO: tests for get_reach_responses
|
||||
|
||||
@@ -52,10 +50,7 @@ def test_try_send_sms_raises_if_reach_rejects(notify_api, mocker):
|
||||
request_mock.post('https://example.com/reach', json=response_dict, status_code=400)
|
||||
reach_client.try_send_sms(to, content, reference, False, 'sender')
|
||||
|
||||
assert exc.value.status_code == 400
|
||||
assert '"Error": 206' in exc.value.text
|
||||
assert '"Description": "Some kind of error"' in exc.value.text
|
||||
assert type(exc.value.exception) == HTTPError
|
||||
assert "Request failed" in str(exc)
|
||||
|
||||
|
||||
def test_try_send_sms_raises_if_reach_fails_to_return_json(notify_api, mocker):
|
||||
@@ -66,28 +61,24 @@ def test_try_send_sms_raises_if_reach_fails_to_return_json(notify_api, mocker):
|
||||
request_mock.post('https://example.com/reach', text=response_dict, status_code=200)
|
||||
reach_client.try_send_sms(to, content, reference, False, 'sender')
|
||||
|
||||
assert 'Code 200 text NOT AT ALL VALID JSON {"key" : "value"}} exception Expecting value: line 1 column 1 (char 0)' in str(exc.value) # noqa
|
||||
assert exc.value.status_code == 200
|
||||
assert exc.value.text == 'NOT AT ALL VALID JSON {"key" : "value"}}'
|
||||
assert 'Invalid response JSON' in str(exc.value)
|
||||
|
||||
|
||||
def test_try_send_sms_raises_if_reach_rejects_with_connect_timeout(rmock):
|
||||
to = content = reference = 'foo'
|
||||
|
||||
with pytest.raises(ReachClientResponseException) as exc:
|
||||
with pytest.raises(SmsClientResponseException) as exc:
|
||||
rmock.register_uri('POST', 'https://example.com/reach', exc=ConnectTimeout)
|
||||
reach_client.try_send_sms(to, content, reference, False, 'sender')
|
||||
|
||||
assert exc.value.status_code == 504
|
||||
assert exc.value.text == 'Gateway Time-out'
|
||||
assert 'Request failed' in str(exc.value)
|
||||
|
||||
|
||||
def test_try_send_sms_raises_if_reach_rejects_with_read_timeout(rmock):
|
||||
to = content = reference = 'foo'
|
||||
|
||||
with pytest.raises(ReachClientResponseException) as exc:
|
||||
with pytest.raises(SmsClientResponseException) as exc:
|
||||
rmock.register_uri('POST', 'https://example.com/reach', exc=ReadTimeout)
|
||||
reach_client.try_send_sms(to, content, reference, False, 'sender')
|
||||
|
||||
assert exc.value.status_code == 504
|
||||
assert exc.value.text == 'Gateway Time-out'
|
||||
assert 'Request failed' in str(exc.value)
|
||||
|
||||
Reference in New Issue
Block a user