From daea09dd3aacfeebe2d965e33ccae8ccdc2ce7e7 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 7 Nov 2016 11:03:21 +0000 Subject: [PATCH 1/6] refactor service id and api key id in tests --- tests/conftest.py | 16 ++--- .../test_base_api_client.py | 63 +++++-------------- 2 files changed, 24 insertions(+), 55 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 2548f3c..19a6e0a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,13 +1,15 @@ +from unittest import mock + import requests_mock import pytest + from notifications_python_client.base import BaseAPIClient from notifications_python_client.notifications import NotificationsAPIClient -import mock TEST_HOST = 'http://test-host' -TEST_CLIENT_ID = "tests" -TEST_SECRET = 'very' +SERVICE_ID = 'c745a8d8-b48a-4b0d-96e5-dbea0165ebd1' +API_KEY_ID = '8b3aa916-ec82-434e-b0c5-d5d9b371d6a3' @pytest.yield_fixture @@ -25,12 +27,12 @@ def rmock_patch(): @pytest.yield_fixture def base_client(): yield BaseAPIClient(base_url=TEST_HOST, - service_id=TEST_CLIENT_ID, - api_key=TEST_SECRET) + service_id=SERVICE_ID, + api_key=API_KEY_ID) @pytest.yield_fixture def notifications_client(): yield NotificationsAPIClient(base_url=TEST_HOST, - service_id=TEST_CLIENT_ID, - api_key=TEST_SECRET) + service_id=SERVICE_ID, + api_key=API_KEY_ID) diff --git a/tests/notifications_python_client/test_base_api_client.py b/tests/notifications_python_client/test_base_api_client.py index 6837a01..050becb 100644 --- a/tests/notifications_python_client/test_base_api_client.py +++ b/tests/notifications_python_client/test_base_api_client.py @@ -1,66 +1,33 @@ +import requests +from unittest import mock + import pytest -import mock from notifications_python_client.errors import HTTPError, InvalidResponse from notifications_python_client.base import BaseAPIClient -import requests +from tests.conftest import API_KEY_ID, SERVICE_ID @pytest.mark.parametrize('client', [ - BaseAPIClient( - service_id='c745a8d8-b48a-4b0d-96e5-dbea0165ebd1', - api_key='8b3aa916-ec82-434e-b0c5-d5d9b371d6a3' - ), - BaseAPIClient( - api_key=( - 'name_of_key' - '-' - 'c745a8d8-b48a-4b0d-96e5-dbea0165ebd1' # service ID - '-' - '8b3aa916-ec82-434e-b0c5-d5d9b371d6a3' # secret - ) - ), - BaseAPIClient( - api_key=( - 'name_of_key' - '😬' - 'c745a8d8-b48a-4b0d-96e5-dbea0165ebd1' # service ID - '😬' - '8b3aa916-ec82-434e-b0c5-d5d9b371d6a3' # secret - ) - ), - BaseAPIClient( - service_id='c745a8d8-b48a-4b0d-96e5-dbea0165ebd1', - api_key=( - 'name_of_key' - '-' - 'c745a8d8-b48a-4b0d-96e5-dbea0165ebd1' # service ID - '-' - '8b3aa916-ec82-434e-b0c5-d5d9b371d6a3' # secret - ) - ) + BaseAPIClient(service_id=SERVICE_ID, api_key=API_KEY_ID), + BaseAPIClient(api_key='-'.join(['name_of_key', SERVICE_ID, API_KEY_ID])), + BaseAPIClient(api_key='😬'.join(['name_of_key', SERVICE_ID, API_KEY_ID])), + BaseAPIClient(service_id=SERVICE_ID, api_key='-'.join(['name_of_key', SERVICE_ID, API_KEY_ID])) ]) -@mock.patch('notifications_python_client.base.create_jwt_token') -def test_passes_through_service_id_and_key(mock_create_token, rmock, client): - rmock.request("GET", "/", status_code=204) - client.request("GET", '/') - mock_create_token.assert_called_once_with( - '8b3aa916-ec82-434e-b0c5-d5d9b371d6a3', - 'c745a8d8-b48a-4b0d-96e5-dbea0165ebd1' - ) +def test_passes_through_service_id_and_key(rmock, client): + with mock.patch('notifications_python_client.base.create_jwt_token') as mock_create_token: + rmock.request("GET", "/", status_code=204) + client.request("GET", '/') + mock_create_token.assert_called_once_with(API_KEY_ID, SERVICE_ID) def test_fails_if_client_id_missing(): with pytest.raises(AssertionError) as err: - BaseAPIClient( - api_key='8b3aa916-ec82-434e-b0c5-d5d9b371d6a3' - ) + BaseAPIClient(api_key=API_KEY_ID) assert str(err.value) == "Missing service ID" def test_connection_error_raises_api_error(base_client, rmock_patch): - rmock_patch.side_effect = requests.exceptions.ConnectionError( - None - ) + rmock_patch.side_effect = requests.exceptions.ConnectionError(None) with pytest.raises(HTTPError) as e: base_client.request("GET", '/') From 756026e4bb2eccc3d387d1d34b99a19dfaeebb30 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 7 Nov 2016 11:27:02 +0000 Subject: [PATCH 2/6] allow api_key as a positional argument it doesn't make sense that if someone creates a client without any kwargs, it writes their first positional parameter as the base_url, which users will rarely, if ever, change. by making api_key a positional argument, the user can create the ApiClient object without kwargs. eg NotificationsAPIClient(my_api_key). this is a breaking change if users were originally calling without keywords, as api_key now preceeds base_url. --- notifications_python_client/base.py | 9 +++++---- .../test_base_api_client.py | 18 +++++++++++++++--- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/notifications_python_client/base.py b/notifications_python_client/base.py index 1ae5a16..1e2b77c 100644 --- a/notifications_python_client/base.py +++ b/notifications_python_client/base.py @@ -1,10 +1,11 @@ from __future__ import absolute_import +import logging +import json from time import monotonic + +from notifications_python_client.version import __version__ from notifications_python_client.errors import HTTPError, InvalidResponse from notifications_python_client.authentication import create_jwt_token -from notifications_python_client.version import __version__ -import logging -import json try: import urlparse @@ -20,9 +21,9 @@ class BaseAPIClient(object): def __init__( self, + api_key, base_url='https://api.notifications.service.gov.uk', service_id=None, - api_key=None ): """ Initialise the client diff --git a/tests/notifications_python_client/test_base_api_client.py b/tests/notifications_python_client/test_base_api_client.py index 050becb..3cf0569 100644 --- a/tests/notifications_python_client/test_base_api_client.py +++ b/tests/notifications_python_client/test_base_api_client.py @@ -7,17 +7,29 @@ from tests.conftest import API_KEY_ID, SERVICE_ID +COMBINED_API_KEY = '-'.join(['name_of_key', SERVICE_ID, API_KEY_ID]) +EMOJI_API_KEY = '😬'.join(['name_of_key', SERVICE_ID, API_KEY_ID]) + + @pytest.mark.parametrize('client', [ BaseAPIClient(service_id=SERVICE_ID, api_key=API_KEY_ID), - BaseAPIClient(api_key='-'.join(['name_of_key', SERVICE_ID, API_KEY_ID])), - BaseAPIClient(api_key='😬'.join(['name_of_key', SERVICE_ID, API_KEY_ID])), - BaseAPIClient(service_id=SERVICE_ID, api_key='-'.join(['name_of_key', SERVICE_ID, API_KEY_ID])) + BaseAPIClient(api_key=COMBINED_API_KEY), + BaseAPIClient(api_key=EMOJI_API_KEY), + BaseAPIClient(service_id=SERVICE_ID, api_key=COMBINED_API_KEY), + BaseAPIClient(COMBINED_API_KEY), +], ids=[ + 'service and api as kwargs', + 'combined api key', + 'key with emoji', + 'service id and combined api key', + 'positional api key' ]) def test_passes_through_service_id_and_key(rmock, client): with mock.patch('notifications_python_client.base.create_jwt_token') as mock_create_token: rmock.request("GET", "/", status_code=204) client.request("GET", '/') mock_create_token.assert_called_once_with(API_KEY_ID, SERVICE_ID) + assert client.base_url == 'https://api.notifications.service.gov.uk' def test_fails_if_client_id_missing(): From 6946728aa56a01b7974af8220205bc1cbcaedd1a Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 8 Nov 2016 11:45:55 +0000 Subject: [PATCH 3/6] update client integration tests to use kwargs --- integration_test/integration_tests.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/integration_test/integration_tests.py b/integration_test/integration_tests.py index 9225cea..d96bf6e 100644 --- a/integration_test/integration_tests.py +++ b/integration_test/integration_tests.py @@ -47,9 +47,9 @@ def get_all_notifications(client): if __name__ == "__main__": client = NotificationsAPIClient( - os.environ['NOTIFY_API_URL'], - os.environ['SERVICE_ID'], - os.environ['API_KEY'] + base_url=os.environ['NOTIFY_API_URL'], + service_id=os.environ['SERVICE_ID'], + api_key=os.environ['API_KEY'] ) sms_id = send_sms_notification_test_response(client) From 84c3ee4dc4379e3f4f40a60199b4b19f59dedae2 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 14 Nov 2016 14:28:57 +0000 Subject: [PATCH 4/6] bump version from 2.0.0 to 3.0.0 --- notifications_python_client/version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/notifications_python_client/version.py b/notifications_python_client/version.py index afced14..4eb28e3 100644 --- a/notifications_python_client/version.py +++ b/notifications_python_client/version.py @@ -1 +1 @@ -__version__ = '2.0.0' +__version__ = '3.0.0' From 927c0fec241f83d2664301f54426074bc0a7fcc0 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 14 Nov 2016 14:44:57 +0000 Subject: [PATCH 5/6] ensure we use kwargs to avoid confusion/bugs --- tests/notifications_python_client/test_base_api_client.py | 5 +++++ utils/make_api_call.py | 6 +++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/tests/notifications_python_client/test_base_api_client.py b/tests/notifications_python_client/test_base_api_client.py index 3cf0569..4a44466 100644 --- a/tests/notifications_python_client/test_base_api_client.py +++ b/tests/notifications_python_client/test_base_api_client.py @@ -32,6 +32,11 @@ def test_passes_through_service_id_and_key(rmock, client): assert client.base_url == 'https://api.notifications.service.gov.uk' +def test_can_set_base_url(): + client = BaseAPIClient(base_url='foo', service_id=SERVICE_ID, api_key=COMBINED_API_KEY) + assert client.base_url == 'foo' + + def test_fails_if_client_id_missing(): with pytest.raises(AssertionError) as err: BaseAPIClient(api_key=API_KEY_ID) diff --git a/utils/make_api_call.py b/utils/make_api_call.py index 6d28db6..17658ec 100755 --- a/utils/make_api_call.py +++ b/utils/make_api_call.py @@ -89,9 +89,9 @@ def get_template_version(notifications_client): arguments = docopt(__doc__) client = NotificationsAPIClient( - arguments[''], - arguments[''], - arguments[''] + base_url=arguments[''], + service_id=arguments[''], + api_key=arguments[''] ) if arguments[''] == 'create': From 947b1c90270cfa82b62544f81fbdd1801697c10f Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 14 Nov 2016 15:40:31 +0000 Subject: [PATCH 6/6] add changelog --- CHANGELOG.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 CHANGELOG.md diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..f110723 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,14 @@ +## 3.0.0 (2016-11-14) + +### Changed +* `BaseClient` method signature: `base_url` is now optional. See [#41](https://github.com/alphagov/notifications-python-client/pull/41) +* `BaseClient` method signature: `api_key` is now a positional argument. See [#41](https://github.com/alphagov/notifications-python-client/pull/41) + +## 2.0.0 (2016-11-09) + +### Changed +* Replace asserts with proper exceptions in jwt token code. See [#40](https://github.com/alphagov/notifications-python-client/pull/40) + +# Prior versions + +Changelog not recorded - please see pull requests on github.