Merge pull request #851 from alphagov/feat-add-who-updated-providers

Store who updated providers

tests/app/celery/test_scheduled_tasks.py

@@ -316,8 +316,11 @@ def test_switch_providers_on_slow_delivery_runs_if_config_set(
 
 def test_switch_providers_triggers_on_slow_notification_delivery(
     notify_api,
-    prepare_current_provider
+    mocker,
+    prepare_current_provider,
+    sample_user
 ):
+    mocker.patch('app.provider_details.switch_providers.get_user_by_id', return_value=sample_user)
     starting_provider = get_current_provider('sms')
 
     with set_config_values(notify_api, {
@@ -335,8 +338,11 @@ def test_switch_providers_triggers_on_slow_notification_delivery(
 
 def test_switch_providers_on_slow_delivery_does_not_switch_if_already_switched(
     notify_api,
-    prepare_current_provider
+    mocker,
+    prepare_current_provider,
+    sample_user
 ):
+    mocker.patch('app.provider_details.switch_providers.get_user_by_id', return_value=sample_user)
     starting_provider = get_current_provider('sms')
 
     with set_config_values(notify_api, {
@@ -356,7 +362,10 @@ def test_switch_providers_on_slow_delivery_does_not_switch_if_already_switched(
 
 def test_switch_providers_on_slow_delivery_does_not_switch_based_on_older_notifications(
     notify_api,
-    prepare_current_provider
+    mocker,
+    prepare_current_provider,
+    sample_user,
+
 ):
     """
     Assume we have three slow delivery notifications for the current provider x. This triggers
@@ -367,7 +376,9 @@ def test_switch_providers_on_slow_delivery_does_not_switch_based_on_older_notifi
     based on these as they are old. We only want to look for slow notifications after the point at
     which we switched back to provider x.
     """
+    mocker.patch('app.provider_details.switch_providers.get_user_by_id', return_value=sample_user)
     starting_provider = get_current_provider('sms')
+
     with set_config_values(notify_api, {
         'FUNCTIONAL_TEST_PROVIDER_SERVICE_ID': '7954469d-8c6d-43dc-b8f7-86be2d69f5f3',
         'FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID': '331a63e6-f1aa-4588-ad3f-96c268788ae7'

tests/app/conftest.py

@@ -9,7 +9,6 @@ from flask import current_app
 
 from app import db
 from app.models import (
-    User,
     Service,
     Template,
     ApiKey,

tests/app/dao/test_provider_details_dao.py

@@ -1,9 +1,8 @@
 import pytest
 
 from datetime import datetime
-
 from freezegun import freeze_time
-from sqlalchemy import desc
+from sqlalchemy import asc, desc
 
 from app.models import ProviderDetails, ProviderDetailsHistory
 from app import clients
@@ -15,7 +14,9 @@ from app.dao.provider_details_dao import (
     get_provider_details_by_notification_type,
     dao_switch_sms_provider_to_provider_with_identifier,
     dao_toggle_sms_provider,
-    dao_update_provider_details
+    dao_update_provider_details,
+    dao_get_provider_versions,
+    dao_get_sms_provider_with_equal_priority
 )
 
 
@@ -148,9 +149,13 @@ def test_switch_sms_provider_to_inactive_provider_does_not_switch(
 
 
 def test_toggle_sms_provider_switches_provider(
+    mocker,
     restore_provider_details,
-    current_sms_provider
+    current_sms_provider,
+    sample_user
+
 ):
+    mocker.patch('app.provider_details.switch_providers.get_user_by_id', return_value=sample_user)
     dao_toggle_sms_provider(current_sms_provider.identifier)
     new_provider = get_current_provider('sms')
 
@@ -161,8 +166,11 @@ def test_toggle_sms_provider_switches_provider(
 
 
 def test_toggle_sms_provider_switches_when_provider_priorities_are_equal(
-    restore_provider_details
+    mocker,
+    restore_provider_details,
+    sample_user
 ):
+    mocker.patch('app.provider_details.switch_providers.get_user_by_id', return_value=sample_user)
     current_provider = get_current_provider('sms')
     new_provider = get_alternative_sms_provider(current_provider.identifier)
 
@@ -178,9 +186,12 @@ def test_toggle_sms_provider_switches_when_provider_priorities_are_equal(
 
 
 def test_toggle_sms_provider_updates_provider_history(
+    mocker,
     restore_provider_details,
-    current_sms_provider
+    current_sms_provider,
+    sample_user
 ):
+    mocker.patch('app.provider_details.switch_providers.get_user_by_id', return_value=sample_user)
     provider_history_rows = ProviderDetailsHistory.query.filter(
         ProviderDetailsHistory.id == current_sms_provider.id
     ).order_by(
@@ -197,3 +208,68 @@ def test_toggle_sms_provider_updates_provider_history(
 
     assert len(updated_provider_history_rows) - len(provider_history_rows) == 1
     assert updated_provider_history_rows[0].version - provider_history_rows[0].version == 1
+
+
+def test_toggle_sms_provider_switches_provider_stores_notify_user_id(
+    restore_provider_details,
+    sample_user,
+    mocker
+):
+    mocker.patch('app.provider_details.switch_providers.get_user_by_id', return_value=sample_user)
+
+    current_provider = get_current_provider('sms')
+    dao_toggle_sms_provider(current_provider.identifier)
+    new_provider = get_current_provider('sms')
+
+    assert current_provider.identifier != new_provider.identifier
+    assert new_provider.created_by.id == sample_user.id
+    assert new_provider.created_by_id == sample_user.id
+
+
+def test_toggle_sms_provider_switches_provider_stores_notify_user_id_in_history(
+    restore_provider_details,
+    sample_user,
+    mocker
+):
+    mocker.patch('app.provider_details.switch_providers.get_user_by_id', return_value=sample_user)
+
+    old_provider = get_current_provider('sms')
+    dao_toggle_sms_provider(old_provider.identifier)
+    new_provider = get_current_provider('sms')
+
+    old_provider_from_history = ProviderDetailsHistory.query.filter_by(
+        identifier=old_provider.identifier,
+        version=old_provider.version
+    ).order_by(
+        asc(ProviderDetailsHistory.priority)
+    ).first()
+    new_provider_from_history = ProviderDetailsHistory.query.filter_by(
+        identifier=new_provider.identifier,
+        version=new_provider.version
+    ).order_by(
+        asc(ProviderDetailsHistory.priority)
+    ).first()
+
+    assert old_provider.version == old_provider_from_history.version
+    assert new_provider.version == new_provider_from_history.version
+    assert new_provider_from_history.created_by_id == sample_user.id
+    assert old_provider_from_history.created_by_id == sample_user.id
+
+
+def test_can_get_all_provider_history(current_sms_provider):
+    assert len(dao_get_provider_versions(current_sms_provider.id)) == 1
+
+
+def test_get_sms_provider_with_equal_priority_returns_provider(
+    restore_provider_details
+):
+    current_provider = get_current_provider('sms')
+    new_provider = get_alternative_sms_provider(current_provider.identifier)
+
+    current_provider.priority = new_provider.priority
+    dao_update_provider_details(current_provider)
+
+    conflicting_provider = \
+        dao_get_sms_provider_with_equal_priority(current_provider.identifier, current_provider.priority)
+
+    assert conflicting_provider

tests/app/delivery/test_send_to_providers.py

@@ -23,7 +23,7 @@ from app.models import (
 from tests.app.db import create_service, create_template, create_notification
 
 
-def test_should_return_highest_priority_active_provider(notify_db_session):
+def test_should_return_highest_priority_active_provider(restore_provider_details):
     providers = provider_details_dao.get_provider_details_by_notification_type('sms')
 
     first = providers[0]

tests/app/provider_details/test_rest.py

@@ -1,7 +1,7 @@
 import pytest
 from flask import json
 
-from app.models import ProviderDetails
+from app.models import ProviderDetails, ProviderDetailsHistory
 
 from tests import create_authorization_header
 
@@ -45,7 +45,9 @@ def test_get_provider_details_contains_correct_fields(client, notify_db):
     )
     json_resp = json.loads(response.get_data(as_text=True))['provider_details']
     allowed_keys = {
-        "id", "display_name", "identifier", "priority", 'notification_type', "active", "version", "updated_at"
+        "id", "created_by", "display_name",
+        "identifier", "priority", 'notification_type',
+        "active", "version", "updated_at"
     }
     assert allowed_keys == set(json_resp[0].keys())
 
@@ -92,13 +94,46 @@ def test_should_be_able_to_update_status(client, restore_provider_details):
 def test_should_not_be_able_to_update_disallowed_fields(client, restore_provider_details, field, value):
     provider = ProviderDetails.query.first()
 
-    update_resp = client.post(
+    resp = client.post(
         '/provider-details/{}'.format(provider.id),
         headers=[('Content-Type', 'application/json'), create_authorization_header()],
         data=json.dumps({field: value})
     )
-    assert update_resp.status_code == 400
-    update_resp = json.loads(update_resp.get_data(as_text=True))
-    print(update_resp)
-    assert update_resp['message'][field][0] == 'Not permitted to be updated'
-    assert update_resp['result'] == 'error'
+    resp_json = json.loads(resp.get_data(as_text=True))
+
+    assert resp_json['message'][field][0] == 'Not permitted to be updated'
+    assert resp_json['result'] == 'error'
+    assert resp.status_code == 400
+
+
+def test_get_provider_versions_contains_correct_fields(client, notify_db):
+    provider = ProviderDetailsHistory.query.first()
+    response = client.get(
+        '/provider-details/{}/versions'.format(provider.id),
+        headers=[create_authorization_header()]
+    )
+    json_resp = json.loads(response.get_data(as_text=True))['data']
+    allowed_keys = {
+        "id", "created_by", "display_name",
+        "identifier", "priority", 'notification_type',
+        "active", "version", "updated_at"
+    }
+    assert allowed_keys == set(json_resp[0].keys())
+
+
+def test_update_provider_should_store_user_id(client, restore_provider_details, sample_user):
+    provider = ProviderDetails.query.first()
+
+    update_resp_1 = client.post(
+        '/provider-details/{}'.format(provider.id),
+        headers=[('Content-Type', 'application/json'), create_authorization_header()],
+        data=json.dumps({
+            'created_by': sample_user.id,
+            'active': False
+        })
+    )
+    assert update_resp_1.status_code == 200
+    update_resp_1 = json.loads(update_resp_1.get_data(as_text=True))['provider_details']
+    assert update_resp_1['identifier'] == provider.identifier
+    assert not update_resp_1['active']
+    assert not provider.active