Simplify and add tests

Makes these assumptions:
- we don’t care about history before 29th November 2019 at 11am (this
  is when priority started to mean a proportion rather than a ranking)
- the priority of the second provider will always be the inverse of
  the first provider

Which means the code is a lot simpler/actually does what you’d expect.

app/main/views/providers.py

@@ -10,7 +10,7 @@ from app.main import main
 from app.main.forms import ProviderForm, ProviderRatioForm
 from app.utils import user_is_platform_admin
 
-PROVIDER_PRIORITY_MEANING_SWITCHOVER = datetime(2019, 11, 29, 11, 0)
+PROVIDER_PRIORITY_MEANING_SWITCHOVER = datetime(2019, 11, 29, 11, 0).isoformat()
 
 
 @main.route("/providers")
@@ -69,65 +69,39 @@ def edit_sms_provider_ratio():
 
     form = ProviderRatioForm(ratio=providers[0]['priority'])
 
-    if form.validate_on_submit():
-        provider_client.update_provider(providers[0]['id'], form.percentage_left)
-        provider_client.update_provider(providers[1]['id'], form.percentage_right)
-
     if len(providers) < 2:
         abort(400)
 
+    primary_provider, secondary_provider = providers[0:2]
+
+    if form.validate_on_submit():
+        provider_client.update_provider(primary_provider['id'], form.percentage_left)
+        provider_client.update_provider(secondary_provider['id'], form.percentage_right)
+        return redirect(url_for('.edit_sms_provider_ratio'))
+
     return render_template(
         'views/providers/edit-sms-provider-ratio.html',
-        versions=_chunk_versions_by_day(_get_versions(providers[0], providers[1])),
+        versions=_chunk_versions_by_day(_get_versions_since_switchover(primary_provider['id'])),
         form=form,
+        primary_provider=providers[0]['display_name'],
+        secondary_provider=providers[1]['display_name'],
     )
 
 
-def _get_versions(provider0, provider1):
+def _get_versions_since_switchover(provider_id):
 
-    versions = sorted((
-        provider_client.get_provider_versions(provider0['id'])['data'] +
-        provider_client.get_provider_versions(provider1['id'])['data']
-    ), key=lambda version: version['updated_at'] or '', reverse=True)
+    for version in sorted(
+        provider_client.get_provider_versions(provider_id)['data'],
+        key=lambda version: version['updated_at'] or ''
+    ):
 
-    for index, version in enumerate(versions):
-
-        previous_version = get_previous_version(
-            versions,
-            index,
-            version['identifier'],
-        )
-
-        if (
-            (version['updated_at'] or '0') < str(PROVIDER_PRIORITY_MEANING_SWITCHOVER)
-        ):
-            version['priority'], previous_version['priority'] = (
-                int(version['priority'] > previous_version['priority']),
-                int(version['priority'] <= previous_version['priority'])
-            )
-
-        if version['identifier'] == provider0['identifier']:
-            fresh_version = version.copy()
-            fresh_version['other_provider'] = previous_version.copy()
-            yield fresh_version
-        elif previous_version['identifier'] in {provider0['identifier'], None}:
-            fresh_version = previous_version.copy()
-            fresh_version['other_provider'] = version.copy()
-            yield fresh_version
-
-
-def get_previous_version(versions, start_index, current_provider_identifier):
-    for index, version in enumerate(versions):
-        if index < start_index:
+        if not version['updated_at']:
             continue
-        if current_provider_identifier == version['identifier']:
+
+        if version['updated_at'] < PROVIDER_PRIORITY_MEANING_SWITCHOVER:
             continue
-        return version
-    return {
-        'identifier': None,
-        'priority': 999,
-        'updated_at': None,
-    }
+
+        yield version
 
 
 def _chunk_versions_by_day(versions):
@@ -136,7 +110,7 @@ def _chunk_versions_by_day(versions):
 
     for version in sorted(versions, key=lambda version: version['updated_at'] or '', reverse=True):
         days[
-            format_date_numeric(version['updated_at']) if version['updated_at'] else ''
+            format_date_numeric(version['updated_at'])
         ].append(version)
 
     return sorted(days.items(), reverse=True)

app/templates/views/providers/edit-sms-provider-ratio.html

@@ -24,10 +24,10 @@
         <div class="column-two-thirds">
           <div class="history-list-percentage-without-border">
             <div class="history-list-percentage-left-label">
-              MMG
+              {{ primary_provider }}
             </div>
             <div class="history-list-percentage-right-label">
-              Firetext
+              {{ secondary_provider }}
             </div>
             {% call form_wrapper() %}
               <div class="radio-slider" data-module="radio-slider">
@@ -71,17 +71,16 @@
               {% endif %}
             </div>
             <div class="column-two-thirds">
-              {% set percentage = (version.priority / (version.priority + version.other_provider.priority) * 100) %}
               <div class="history-list-percentage">
                 <div class="history-list-percentage-left-label">
-                  {{ version.display_name }}<br><br>
-                  {{ percentage|format_thousands }}%
+                  {{ primary_provider }}<br><br>
+                  {{ version.priority|format_thousands }}%
                 </div>
                 <div class="history-list-percentage-right-label">
-                  {{ version.other_provider.display_name }}<br><br>
-                  {{ (100 - percentage)|format_thousands }}%
+                  {{ secondary_provider }}<br><br>
+                  {{ (100 - version.priority)|format_thousands }}%
                 </div>
-                <div class="history-list-percentage-marker" style="left: {{ (100 -percentage)|format_thousands }}%"></div>
+                <div class="history-list-percentage-marker" style="left: {{ (100 -version.priority)|format_thousands }}%"></div>
               </div>
             </div>
           </div>

tests/app/main/views/test_providers.py

@@ -1,12 +1,15 @@
 import copy
 import re
 from datetime import datetime
+from unittest.mock import call
 
+import pytest
 from bs4 import BeautifulSoup
 from flask import url_for
 
 import app
 from app.main.views.providers import add_monthly_traffic
+from tests.conftest import normalize_spaces
 
 stub_providers = {
     'provider_details': [
@@ -79,12 +82,13 @@ stub_providers = {
             'id': '67c770f5-918e-4afa-a5ff-880b9beb161d',
             'active': False,
             'priority': 10,
-            'display_name': 'International SMS Provider (no flag)',
+            'display_name': 'Second International SMS Provider',
             'identifier': 'second_sms_international',
             'notification_type': 'sms',
             'updated_at': None,
             'version': 1,
             'created_by': None,
+            'supports_international': True,
             'current_month_billable_sms': 0,
         }
     ]
@@ -406,3 +410,143 @@ def test_should_show_provider_version_history(
     assert second_row[2].text.strip() == "None"
     assert second_row[3].text.strip() == "10"
     assert second_row[4].text.strip() == "True"
+
+
+def test_should_show_version_history_for_first_two_sms_providers(
+    client_request,
+    platform_admin_user,
+    mocker
+):
+    mocker.patch(
+        'app.provider_client.get_all_providers',
+        return_value=copy.deepcopy(stub_providers)
+    )
+    # second_sms_international will be the primary provider because it’s
+    # the first in the list when reverse sorting the SMS providers
+    second_sms_international = stub_providers['provider_details'][5]
+    mocker.patch(
+        'app.provider_client.get_provider_versions',
+        return_value={'data': [
+            {
+                'id': id,
+                'priority': priority,
+                'display_name': second_sms_international['display_name'],
+                'identifier': second_sms_international['identifier'],
+                'updated_at': updated_at,
+                'created_by': {
+                    'email_address': 'test@foo.bar',
+                    'name': 'Test User',
+                    'id': '7cc1dddb-bcbc-4739-8fc1-61bedde3332a'
+                },
+                'supports_international': False,
+            }
+            for updated_at, priority in [
+                (datetime(2022, 2, 22, 14).isoformat(), 100),
+                (datetime(2020, 1, 1, 5).isoformat(), 80),
+                (datetime(2020, 1, 1, 3).isoformat(), 10),
+                # Anything older than 11am on 29 November 2019
+                # should be ignored because the priority numbers
+                # didn’t mean the same thing before then
+                (datetime(2019, 11, 29, 10, 59).isoformat(), 123),
+                (datetime(2000, 1, 1, 0).isoformat(), 1999),
+                (None, 30),
+            ]
+        ]}
+    )
+
+    client_request.login(platform_admin_user)
+    page = client_request.get('main.edit_sms_provider_ratio')
+
+    assert [
+        radio['value']
+        for radio in page.select('input[name=ratio]')
+    ] == [
+        '100', '90', '80', '70', '60', '50', '40', '30', '20', '10', '0',
+    ]
+
+    assert [
+        radio['value']
+        for radio in page.select('input[checked]')
+    ] == [
+        str(second_sms_international['priority'])
+    ]
+
+    assert [
+        normalize_spaces(heading.text)
+        for heading in page.select('main h2')
+    ] == [
+        'Now',
+        '21 February 2022',
+        '31 December',
+    ]
+
+    assert [
+        normalize_spaces(version.text)
+        for version in page.select('li.history-list-item')
+    ] == [
+        (
+            'Test User 2:00pm '
+            'Second International SMS Provider 100% '
+            'Second Domestic SMS Provider 0%'
+        ),
+        (
+            'Test User 5:00am '
+            'Second International SMS Provider 80% '
+            'Second Domestic SMS Provider 20%'
+        ),
+        (
+            'Test User 3:00am '
+            'Second International SMS Provider 10% '
+            'Second Domestic SMS Provider 90%'
+        ),
+    ]
+
+
+@pytest.mark.parametrize('posted_number, expected_calls', [
+    (
+        '10',
+        [
+            call(stub_providers['provider_details'][5]['id'], 10),
+            call(stub_providers['provider_details'][1]['id'], 90),
+        ],
+    ),
+    (
+        '80',
+        [
+            call(stub_providers['provider_details'][5]['id'], 80),
+            call(stub_providers['provider_details'][1]['id'], 20),
+        ],
+    ),
+])
+def test_should_update_priority_of_first_two_sms_providers(
+    client_request,
+    platform_admin_user,
+    mocker,
+    posted_number,
+    expected_calls,
+):
+    mocker.patch(
+        'app.provider_client.get_all_providers',
+        return_value=copy.deepcopy(stub_providers)
+    )
+    mocker.patch(
+        'app.provider_client.get_provider_versions',
+        return_value={'data': []}
+    )
+    mock_update_provider = mocker.patch(
+        'app.provider_client.update_provider'
+    )
+
+    client_request.login(platform_admin_user)
+    client_request.post(
+        '.edit_sms_provider_ratio',
+        _data={
+            'ratio': posted_number,
+        },
+        _expected_redirect=url_for(
+            '.edit_sms_provider_ratio',
+            _external=True,
+        ),
+    )
+
+    assert mock_update_provider.call_args_list == expected_calls