Don’t do multiple get API calls when revoking

It’s redundant to make two API calls here, one to get all keys and one
to get a single key. Since the API calls are sequential we can speed
things up by getting the one key from the list of all keys.

app/main/views/api_keys.py

@@ -128,7 +128,7 @@ def create_api_key(service_id):
 @login_required
 @user_has_permissions('manage_api_keys')
 def revoke_api_key(service_id, key_id):
-    key_name = api_key_api_client.get_api_keys(service_id=service_id, key_id=key_id)['apiKeys'][0]['name']
+    key_name = current_service.get_api_key(key_id)['name']
     if request.method == 'GET':
         return render_template(
             'views/api/keys.html',

app/models/service.py

@@ -1,3 +1,4 @@
+from flask import abort
 from notifications_utils.field import Field
 from werkzeug.utils import cached_property
 
@@ -344,3 +345,9 @@ class Service():
     @property
     def api_key_names(self):
         return [key['name'] for key in self.api_keys]
+
+    def get_api_key(self, id):
+        try:
+            return next(key for key in self.api_keys if key['id'] == id)
+        except StopIteration:
+            abort(404)

app/notify_client/api_key_api_client.py

@@ -10,11 +10,8 @@ class ApiKeyApiClient(NotifyAdminAPIClient):
     def __init__(self):
         super().__init__("a" * 73, "b")
 
-    def get_api_keys(self, service_id, key_id=None):
-        if key_id:
-            return self.get(url='/service/{}/api-keys/{}'.format(service_id, key_id))
-        else:
-            return self.get(url='/service/{}/api-keys'.format(service_id))
+    def get_api_keys(self, service_id):
+        return self.get(url='/service/{}/api-keys'.format(service_id))
 
     def create_api_key(self, service_id, key_name, key_type):
         data = {

tests/app/main/views/test_api_integration.py

@@ -318,16 +318,22 @@ def test_should_show_confirm_revoke_api_key(
         'Confirm'
     )
     assert mock_get_api_keys.call_args_list == [
-        call(
-            key_id=fake_uuid,
-            service_id='596364a0-858e-42c8-9062-a8fe822260eb',
-        ),
         call(
             '596364a0-858e-42c8-9062-a8fe822260eb'
         ),
     ]
 
 
+def test_should_404_for_api_key_that_doesnt_exist(
+    client_request,
+    mock_get_api_keys,
+):
+    client_request.get(
+        'main.revoke_api_key', service_id=SERVICE_ONE_ID, key_id='key-doesn’t-exist',
+        _expected_status=404,
+    )
+
+
 def test_should_redirect_after_revoking_api_key(
     logged_in_client,
     api_user_active,
@@ -338,12 +344,12 @@ def test_should_redirect_after_revoking_api_key(
     mock_has_permissions,
     fake_uuid,
 ):
-    response = logged_in_client.post(url_for('main.revoke_api_key', service_id=fake_uuid, key_id=fake_uuid))
+    response = logged_in_client.post(url_for('main.revoke_api_key', service_id=SERVICE_ONE_ID, key_id=fake_uuid))
 
     assert response.status_code == 302
-    assert response.location == url_for('.api_keys', service_id=fake_uuid, _external=True)
-    mock_revoke_api_key.assert_called_once_with(service_id=fake_uuid, key_id=fake_uuid)
-    mock_get_api_keys.assert_called_once_with(service_id=fake_uuid, key_id=fake_uuid)
+    assert response.location == url_for('.api_keys', service_id=SERVICE_ONE_ID, _external=True)
+    mock_revoke_api_key.assert_called_once_with(service_id=SERVICE_ONE_ID, key_id=fake_uuid)
+    mock_get_api_keys.assert_called_once_with(SERVICE_ONE_ID,)
 
 
 @pytest.mark.parametrize('route', [
@@ -354,6 +360,7 @@ def test_should_redirect_after_revoking_api_key(
 def test_route_permissions(
     mocker,
     app_,
+    fake_uuid,
     api_user_active,
     service_one,
     mock_get_api_keys,
@@ -365,7 +372,7 @@ def test_route_permissions(
             app_,
             "GET",
             200,
-            url_for(route, service_id=service_one['id'], key_id=123),
+            url_for(route, service_id=service_one['id'], key_id=fake_uuid),
             ['manage_api_keys'],
             api_user_active,
             service_one)
@@ -379,6 +386,7 @@ def test_route_permissions(
 def test_route_invalid_permissions(
     mocker,
     app_,
+    fake_uuid,
     api_user_active,
     service_one,
     mock_get_api_keys,
@@ -390,7 +398,7 @@ def test_route_invalid_permissions(
             app_,
             "GET",
             403,
-            url_for(route, service_id=service_one['id'], key_id=123),
+            url_for(route, service_id=service_one['id'], key_id=fake_uuid),
             ['view_activity'],
             api_user_active,
             service_one)

tests/conftest.py

@@ -1639,10 +1639,12 @@ def mock_revoke_api_key(mocker):
 
 
 @pytest.fixture(scope='function')
-def mock_get_api_keys(mocker):
+def mock_get_api_keys(mocker, fake_uuid):
     def _get_keys(service_id, key_id=None):
-        keys = {'apiKeys': [api_key_json(service_id, 'some key name'),
-                            api_key_json(service_id, 'another key name', expiry_date=str(date.fromtimestamp(0)))]}
+        keys = {'apiKeys': [
+            api_key_json(id_=fake_uuid, name='some key name',),
+            api_key_json(id_='1234567', name='another key name', expiry_date=str(date.fromtimestamp(0)))
+        ]}
         return keys
 
     return mocker.patch('app.api_key_api_client.get_api_keys', side_effect=_get_keys)