Merge pull request #1810 from GSA/code_coverage

more tests

.coveragerc

@@ -0,0 +1,5 @@
+[report]
+exclude_lines =
+    pragma: no cover
+    ^\s*logger\.
+    ^\s*current_app\.logger\.

Makefile

@@ -104,8 +104,8 @@ test: ## Run tests and create coverage report
 	poetry run isort ./app ./tests
 	poetry run coverage run --omit=*/migrations/*,*/tests/* -m pytest --maxfail=10
 
-    ## TODO set this back to 95 asap
-	poetry run coverage report -m --fail-under=94
+
+	poetry run coverage report -m --fail-under=95
 	poetry run coverage html -d .coverage_cache
 
 .PHONY: test-debug

app/clients/pinpoint/aws_pinpoint.py

@@ -37,6 +37,7 @@ class AwsPinpointClient(Client):
             # TODO right now this will only print with AWS simulated numbers,
             # but remove this when that changes
             current_app.logger.info(hilite(response))
+            return response
         except ClientError:
             current_app.logger.exception(
                 "#notify-debug-validate-phone-number Could not validate with pinpoint"

app/commands.py

@@ -752,12 +752,13 @@ def create_admin_jwt():
 @notify_command(name="create-user-jwt")
 @click.option("-t", "--token", required=True, prompt=False)
 def create_user_jwt(token):
-    if getenv("NOTIFY_ENVIRONMENT", "") != "development":
+    if getenv("NOTIFY_ENVIRONMENT", "") not in ["development", "test"]:
         current_app.logger.error("Can only be run in development")
         return
     service_id = token[-73:-37]
     api_key = token[-36:]
-    current_app.logger.info(create_jwt_token(api_key, service_id))
+    token = create_jwt_token(api_key, service_id)
+    current_app.logger.info(token)
 
 
 def _update_template(id, name, template_type, content, subject):

tests/app/aws/test_s3.py

@@ -2,7 +2,7 @@ import os
 import time
 from datetime import timedelta
 from os import getenv
-from unittest.mock import MagicMock, Mock, call
+from unittest.mock import MagicMock, Mock, call, patch
 
 import botocore
 import pytest
@@ -13,6 +13,7 @@ from app.aws import s3
 from app.aws.s3 import (
     cleanup_old_s3_objects,
     download_from_s3,
+    extract_phones,
     file_exists,
     get_job_and_metadata_from_s3,
     get_job_from_s3,
@@ -650,3 +651,33 @@ def test_read_s3_file_populates_cache(monkeypatch):
     assert job_cache.get("66")[0].startswith("Phone number")
     assert job_cache.get("66_phones")[0] == {"0", "15551234"}
     assert job_cache.get("66_personalisation")[0] == {0: {"Name": "Alice"}}
+
+
+@patch("app.aws.s3.current_app")
+def test_valid_csv(mock_app):
+    csv_data = "Name,Phone Number\nAlice,+1 (555) 555-5555\nBob,555.555.1111"
+    result = extract_phones(csv_data, "service1", "job1")
+    expected = {0: "15555555555", 1: "5555551111"}
+    assert result == expected
+    mock_app.logger.error.assert_not_called()
+
+
+@patch("app.aws.s3.current_app")
+def test_missing_phone_column(mock_app):
+
+    csv_data = "Name,Phone Number\nAlice,\nBob"
+    result = extract_phones(csv_data, "service1", "job1")
+    assert result == {0: "", 1: "Unavailable"}
+    mock_app.logger.error.assert_called_once()
+
+
+@patch("app.aws.s3.current_app")
+def test_test_with_bom_header(mock_app):
+    csv_data = "\ufeffName,Phone Number\nAlice,1-555-555-5555"
+    result = extract_phones(csv_data, "service2", "job2")
+    expected = {0: "15555555555"}
+    assert result == expected
+
+
+if __name__ == "__main__":
+    test_valid_csv()

tests/app/clients/test_aws_pinpoint.py

@@ -0,0 +1,37 @@
+from unittest.mock import MagicMock
+
+import pytest
+from aiohttp import ClientError
+
+from app.clients.pinpoint.aws_pinpoint import AwsPinpointClient
+
+
+def test_validate_phone_number_success():
+    phone = "+1234567890"
+    mock_response = {
+        "NumberValidateResponse": {
+            "PhoneType": "MOBILE",
+            "CleansedPhoneNumberE164": phone,
+        }
+    }
+    client_instance = AwsPinpointClient()
+    client_instance._client = MagicMock()
+    client_instance._client.phone_number_validate.return_value = mock_response
+
+    result = client_instance.validate_phone_number("US", phone)
+    assert result is not None
+    client_instance._client.phone_number_validate.assert_called_once_with(
+        NumberValidateRequest={"IsoCountryCode": "US", "PhoneNumber": phone}
+    )
+
+
+def test_validate_phone_number_client_error():
+    client_instance = AwsPinpointClient()
+    client_instance._client = MagicMock()
+    client_instance._client.phone_number_validate.side_effect = ClientError(
+        {"Error": {"Code": "BadRequest1", "MEssage": "Invalid phone"}},
+        "phone number validate",
+    )
+
+    with pytest.raises(ClientError):
+        client_instance.validate_phone_number("US", "bad-number")

tests/app/test_commands.py

@@ -1,6 +1,7 @@
+import json
 import os
 from datetime import datetime, timedelta
-from unittest.mock import MagicMock, mock_open
+from unittest.mock import MagicMock, mock_open, patch
 
 import pytest
 from sqlalchemy import func, select
@@ -11,13 +12,16 @@ from app.commands import (
     _update_template,
     associate_services_to_organizations,
     bulk_invite_user_to_service,
+    clear_redis_list,
     create_new_service,
     create_test_user,
+    create_user_jwt,
     download_csv_file_by_name,
     dump_sms_senders,
     dump_user_info,
     fix_billable_units,
     generate_salt,
+    get_service_sender_phones,
     insert_inbound_numbers_from_file,
     populate_annual_billing_with_defaults,
     populate_annual_billing_with_the_previous_years_allowance,
@@ -26,8 +30,10 @@ from app.commands import (
     populate_organizations_from_file,
     process_row_from_job,
     promote_user_to_platform_admin,
+    purge_csv_bucket,
     purge_functional_test_data,
     update_jobs_archived_flag,
+    update_templates,
 )
 from app.dao.inbound_numbers_dao import dao_get_available_inbound_numbers
 from app.dao.users_dao import get_user_by_email
@@ -732,3 +738,113 @@ def test_clear_templates_from_cache(mocker):
         [mocker.call(p) for p in expected_patterns], any_order=True
     )
     mock_logger.info.assert_called_once_with("Number of templates deleted from cache 9")
+
+
+@patch("app.commands.s3.purge_bucket")
+@patch("app.commands.current_app")
+def test_purge_csv_bucket(mock_current_app, mock_purge_bucket, notify_api):
+    mock_current_app.config = {
+        "CSV_UPLOAD_BUCKET": {
+            "bucket": "test-bucket",
+            "access_key_id": "FAKE_ACCESS_KEY",
+            "secret_access_key": "FAKE_SECRET_KEY",  # pragma: allowlist secret
+            "region": "us-north-1",
+        }
+    }
+    runner = notify_api.test_cli_runner()
+    runner.invoke(purge_csv_bucket)
+
+    mock_purge_bucket.assert_called_once_with(
+        "test-bucket", "FAKE_ACCESS_KEY", "FAKE_SECRET_KEY", "us-north-1"
+    )
+
+
+@patch("app.commands.db.session.execute")
+def test_get_service_sender_phones(mock_execute, notify_api):
+    runner = notify_api.test_cli_runner()
+    result = runner.invoke(get_service_sender_phones, ["-s", "service-id"])
+    assert result.exit_code == 0
+    mock_execute.assert_called_once()
+
+
+@patch("app.commands.current_app.logger.info")
+def test_create_user_jwt(mock_logger, notify_api, sample_api_key, sample_service):
+    token = f"{sample_service.id}{sample_api_key}"
+    runner = notify_api.test_cli_runner()
+    result = runner.invoke(create_user_jwt, ["-t", token])
+    assert result.exit_code == 0
+    mock_logger.assert_called_once()
+
+
+def test_update_templates_calls_update_and_clear(monkeypatch, notify_api):
+    templates_data = [
+        {
+            "id": 1,
+            "name": "Template1",
+            "type": "email",
+            "content": "Hello",
+            "subject": "Subject1",
+        },
+        {
+            "id": 2,
+            "name": "Template2",
+            "type": "sms",
+            "content": "Hi",
+            "subject": "Subject2",
+        },
+    ]
+    m = mock_open(read_data=json.dumps(templates_data))
+    monkeypatch.setattr(
+        "app.commands.current_app",
+        type("obj", (), {"config": {"CONFIG_FILES": "/fake/path"}}),
+    )
+    with patch("app.commands.open", m):
+        with patch("app.commands._update_template") as mock_update_template, patch(
+            "app.commands._clear_templates_from_cache"
+        ) as mock_clear_cache:
+
+            runner = notify_api.test_cli_runner()
+            result = runner.invoke(update_templates)
+            assert result.exit_code == 0
+
+            expected_calls = [
+                (1, "Template1", "email", "Hello", "Subject1"),
+                (2, "Template2", "sms", "Hi", "Subject2"),
+            ]
+            actual_calls = [call.args for call in mock_update_template.call_args_list]
+            assert actual_calls == expected_calls
+            mock_clear_cache.assert_called_once()
+
+
+def test_clear_redis_list(monkeypatch, notify_api):
+    mock_redis_store = type("RedisStoreMock", (), {})()
+    monkeypatch.setattr("app.commands.redis_store", mock_redis_store)
+    mock_redis_store.llen = lambda list_name: {"before_list": 5, "after_list": 0}[
+        list_name
+    ]
+    ltrim_calls = []
+
+    def fake_ltrim(name, start, end):
+        ltrim_calls.append((name, start, end))
+
+    mock_redis_store.ltrim = fake_ltrim
+    logger_info_calls = []
+    mock_logger = type(
+        "LoggerMock", (), {"info": lambda self, msg: logger_info_calls.append(msg)}
+    )()
+    mock_app = type("FlaskAppMock", (), {"logger": mock_logger})
+    monkeypatch.setattr("app.commands.current_app", mock_app)
+    call_count = {"count": 0}
+
+    def llen_side_effect(name):
+        if call_count["count"] == 0:
+            call_count["count"] += 1
+            return 5
+        return 0
+
+    mock_redis_store.llen = llen_side_effect
+    runner = notify_api.test_cli_runner()
+    result = runner.invoke(clear_redis_list, ["-n", "test_list"])
+    assert result.exit_code == 0
+    assert ltrim_calls == [("test_list", 1, 0)]
+    assert logger_info_calls == ["Cleared redis list test_list. Before: 5 after 0"]

tests/app/test_history_meta.py

@@ -0,0 +1,80 @@
+from unittest.mock import MagicMock
+
+from app.history_meta import (
+    _get_bases_for_versioned_class,
+    _handle_single_table_inheritance,
+)
+
+
+def test_get_bases_with_super_history_and_table():
+    mock_super_mapper = MagicMock()
+    mock_super_mapper.class_ = object
+    mock_super_mapper.attrs.changed.columns = ["col1", "col2"]
+    mock_table = MagicMock()
+    mock_table.c.changed = "changed_col"
+
+    properties = {}
+    bases = _get_bases_for_versioned_class(
+        super_history_mapper=mock_super_mapper,
+        table=mock_table,
+        properties=properties,
+        local_mapper=None,
+    )
+
+    assert bases == (object,)
+    assert properties["changed"] == ("changed_col", "col1", "col2")
+
+
+def test_get_bases_with_super_history_and_no_table():
+    mock_super_mapper = MagicMock()
+    mock_super_mapper.class_ = int
+    properties = {}
+    bases = _get_bases_for_versioned_class(
+        super_history_mapper=mock_super_mapper,
+        table=None,
+        properties=properties,
+        local_mapper=None,
+    )
+    assert bases == (int,)
+    assert "changed" not in properties
+
+
+def test_get_bases_without_super_history():
+    class Base:
+        pass
+
+    class_ = type("Dummy", (Base,), {})
+    local_mapper = MagicMock()
+    local_mapper.base_mapper.class_ = class_
+
+    properties = {}
+
+    bases = _get_bases_for_versioned_class(
+        super_history_mapper=None,
+        table=None,
+        properties=properties,
+        local_mapper=local_mapper,
+    )
+
+    assert bases == (Base,)
+    assert "changed" not in properties
+
+
+def test_handle_single_table_inheritance():
+    col1 = MagicMock()
+    col1.key = "id"
+    col1.name = "name_col1"
+    col2 = MagicMock()
+    col2.key = "new_column"
+    col2.name = "name_col2"
+
+    local_mapper = MagicMock()
+    local_mapper.local_table.c = [col1, col2]
+
+    super_history_mapper = MagicMock()
+    super_history_mapper.local_table.c = {"id": col1}
+    super_history_mapper.local_table.append_column = col2
+
+    _handle_single_table_inheritance(local_mapper, super_history_mapper)
+
+    super_history_mapper.local_table.append_column.assert_called_once()