merge from main

.github/workflows/checks.yml

@@ -63,7 +63,7 @@ jobs:
         NOTIFY_E2E_TEST_PASSWORD: ${{ secrets.NOTIFY_E2E_TEST_PASSWORD }}
     - name: Check coverage threshold
       # TODO get this back up to 95
-      run: poetry run coverage report -m --fail-under=91
+      run: poetry run coverage report -m --fail-under=93
 
   validate-new-relic-config:
     runs-on: ubuntu-latest

.github/workflows/deploy-demo.yml

@@ -97,6 +97,10 @@ jobs:
     - name: Deploy egress proxy
       if: steps.changed-egress-config.outputs.any_changed == 'true'
       uses: ./.github/actions/deploy-proxy
+      env:
+          CF_USERNAME: ${{ secrets.CLOUDGOV_USERNAME }}
+          CF_PASSWORD: ${{ secrets.CLOUDGOV_PASSWORD }}
       with:
+        cf_org: gsa-tts-benefits-studio
         cf_space: notify-demo
         app: notify-api-demo

.github/workflows/deploy-prod.yml

@@ -101,6 +101,10 @@ jobs:
     - name: Deploy egress proxy
       if: steps.changed-egress-config.outputs.any_changed == 'true'
       uses: ./.github/actions/deploy-proxy
+      env:
+          CF_USERNAME: ${{ secrets.CLOUDGOV_USERNAME }}
+          CF_PASSWORD: ${{ secrets.CLOUDGOV_PASSWORD }}
       with:
+        cf_org: gsa-tts-benefits-studio
         cf_space: notify-production
         app: notify-api-production

.github/workflows/deploy.yml

@@ -101,7 +101,7 @@ jobs:
           .github/actions/deploy-proxy/action.yml
           .github/workflows/deploy.yml
     - name: Deploy egress proxy
-      #if: steps.changed-egress-config.outputs.any_changed == 'true'
+      if: steps.changed-egress-config.outputs.any_changed == 'true'
       uses: ./.github/actions/deploy-proxy
       env:
           CF_USERNAME: ${{ secrets.CLOUDGOV_USERNAME }}

Makefile

@@ -84,7 +84,7 @@ test: ## Run tests and create coverage report
 	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=91
+	poetry run coverage report -m --fail-under=93
 	poetry run coverage html -d .coverage_cache
 
 .PHONY: py-lock

app/aws/s3.py

@@ -295,7 +295,11 @@ def get_old_job_location(service_id, job_id):
 
 
 def get_job_and_metadata_from_s3(service_id, job_id):
-    obj = get_s3_object(*get_job_location(service_id, job_id))
+    try:
+        obj = get_s3_object(*get_job_location(service_id, job_id))
+    except botocore.exceptions.ClientError:
+        obj = get_s3_object(*get_old_job_location(service_id, job_id))
+
     return obj.get()["Body"].read().decode("utf-8"), obj.get()["Metadata"]
 
 
@@ -472,23 +476,7 @@ def get_personalisation_from_s3(service_id, job_id, job_row_number):
 
     set_job_cache(job_cache, f"{job_id}_personalisation", extract_personalisation(job))
 
-    # If we can find the quick dictionary, use it
+    return job_cache.get(f"{job_id}_personalisation")[0].get(job_row_number)
-    if job_cache.get(f"{job_id}_personalisation") is not None:
-        personalisation_to_return = job_cache.get(f"{job_id}_personalisation")[0].get(
-            job_row_number
-        )
-        if personalisation_to_return:
-            return personalisation_to_return
-        else:
-            current_app.logger.warning(
-                f"Was unable to retrieve personalisation from lookup dictionary for job {job_id}"
-            )
-            return {}
-    else:
-        current_app.logger.error(
-            f"Was unable to construct lookup dictionary for job {job_id}"
-        )
-        return {}
 
 
 def get_job_metadata_from_s3(service_id, job_id):

app/commands.py

@@ -24,12 +24,6 @@ from app.dao.annual_billing_dao import (
     dao_create_or_update_annual_billing_for_year,
     set_default_free_allowance_for_service,
 )
-from app.dao.fact_billing_dao import (
-    delete_billing_data_for_service_for_day,
-    fetch_billing_data_for_day,
-    get_service_ids_that_need_billing_populated,
-    update_fact_billing,
-)
 from app.dao.jobs_dao import dao_get_job_by_id
 from app.dao.organization_dao import (
     dao_add_service_to_organization,
@@ -63,7 +57,7 @@ from app.models import (
     TemplateHistory,
     User,
 )
-from app.utils import get_midnight_in_utc, utc_now
+from app.utils import utc_now
 from notifications_utils.recipients import RecipientCSV
 from notifications_utils.template import SMSMessageTemplate
 from tests.app.db import (
@@ -167,6 +161,7 @@ def purge_functional_test_data(user_email_prefix):
                 delete_model_user(usr)
 
 
+# TODO maintainability what is the purpose of this command?  Who would use it and why?
 @notify_command(name="insert-inbound-numbers")
 @click.option(
     "-f",
@@ -175,7 +170,6 @@ def purge_functional_test_data(user_email_prefix):
     help="""Full path of the file to upload, file is a contains inbound numbers, one number per line.""",
 )
 def insert_inbound_numbers_from_file(file_name):
-    # TODO maintainability what is the purpose of this command?  Who would use it and why?
 
     current_app.logger.info(f"Inserting inbound numbers from {file_name}")
     with open(file_name) as file:
@@ -195,50 +189,6 @@ def setup_commands(application):
     application.cli.add_command(command_group)
 
 
-@notify_command(name="rebuild-ft-billing-for-day")
-@click.option("-s", "--service_id", required=False, type=click.UUID)
-@click.option(
-    "-d",
-    "--day",
-    help="The date to recalculate, as YYYY-MM-DD",
-    required=True,
-    type=click_dt(format="%Y-%m-%d"),
-)
-def rebuild_ft_billing_for_day(service_id, day):
-    # TODO maintainability what is the purpose of this command?  Who would use it and why?
-
-    """
-    Rebuild the data in ft_billing for the given service_id and date
-    """
-
-    def rebuild_ft_data(process_day, service):
-        deleted_rows = delete_billing_data_for_service_for_day(process_day, service)
-        current_app.logger.info(
-            f"deleted {deleted_rows} existing billing rows for {service} on {process_day}"
-        )
-        transit_data = fetch_billing_data_for_day(
-            process_day=process_day, service_id=service
-        )
-        # transit_data = every row that should exist
-        for data in transit_data:
-            # upsert existing rows
-            update_fact_billing(data, process_day)
-        current_app.logger.info(
-            f"added/updated {len(transit_data)} billing rows for {service} on {process_day}"
-        )
-
-    if service_id:
-        # confirm the service exists
-        dao_fetch_service_by_id(service_id)
-        rebuild_ft_data(day, service_id)
-    else:
-        services = get_service_ids_that_need_billing_populated(
-            get_midnight_in_utc(day), get_midnight_in_utc(day + timedelta(days=1))
-        )
-        for row in services:
-            rebuild_ft_data(day, row.service_id)
-
-
 @notify_command(name="bulk-invite-user-to-service")
 @click.option(
     "-f",
@@ -472,31 +422,6 @@ def associate_services_to_organizations():
     current_app.logger.info("finished associating services to organizations")
 
 
-@notify_command(name="populate-service-volume-intentions")
-@click.option(
-    "-f",
-    "--file_name",
-    required=True,
-    help="Pipe delimited file containing service_id, SMS, email",
-)
-def populate_service_volume_intentions(file_name):
-    # [0] service_id
-    # [1] SMS:: volume intentions for service
-    # [2] Email:: volume intentions for service
-
-    # TODO maintainability what is the purpose of this command? Who would use it and why?
-
-    with open(file_name, "r") as f:
-        for line in itertools.islice(f, 1, None):
-            columns = line.split(",")
-            current_app.logger.info(columns)
-            service = dao_fetch_service_by_id(columns[0])
-            service.volume_sms = columns[1]
-            service.volume_email = columns[2]
-            dao_update_service(service)
-    current_app.logger.info("populate-service-volume-intentions complete")
-
-
 @notify_command(name="populate-go-live")
 @click.option(
     "-f", "--file_name", required=True, help="CSV file containing live service data"

app/delivery/send_to_providers.py

@@ -98,17 +98,7 @@ def send_sms_to_provider(notification):
 
                 # TODO This is temporary to test the capability of validating phone numbers
                 # The future home of the validation is TBD
-                if "+" not in recipient:
+                _experimentally_validate_phone_numbers(recipient)
-                    recipient_lookup = f"+{recipient}"
-                else:
-                    recipient_lookup = recipient
-                if recipient_lookup in current_app.config[
-                    "SIMULATED_SMS_NUMBERS"
-                ] and os.getenv("NOTIFY_ENVIRONMENT") in ["development", "test"]:
-                    current_app.logger.info(hilite("#validate-phone-number fired"))
-                    aws_pinpoint_client.validate_phone_number("01", recipient)
-                else:
-                    current_app.logger.info(hilite("#validate-phone-number not fired"))
 
                 sender_numbers = get_sender_numbers(notification)
                 if notification.reply_to_text not in sender_numbers:
@@ -145,6 +135,18 @@ def send_sms_to_provider(notification):
     return message_id
 
 
+def _experimentally_validate_phone_numbers(recipient):
+    if "+" not in recipient:
+        recipient_lookup = f"+{recipient}"
+    else:
+        recipient_lookup = recipient
+    if recipient_lookup in current_app.config["SIMULATED_SMS_NUMBERS"] and os.getenv(
+        "NOTIFY_ENVIRONMENT"
+    ) in ["development", "test"]:
+        current_app.logger.info(hilite("#validate-phone-number fired"))
+        aws_pinpoint_client.validate_phone_number("01", recipient)
+
+
 def _get_verify_code(notification):
     key = f"2facode-{notification.id}".replace(" ", "")
     recipient = redis_store.get(key)

app/service/rest.py

@@ -453,16 +453,6 @@ def get_all_notifications_for_service(service_id):
         data = notifications_filter_schema.load(MultiDict(request.get_json()))
         current_app.logger.debug(f"use POST, request {request.get_json()} data {data}")
 
-    if data.get("to"):
-        notification_type = (
-            data.get("template_type")[0] if data.get("template_type") else None
-        )
-        return search_for_notification_by_to_field(
-            service_id=service_id,
-            search_term=data["to"],
-            statuses=data.get("status"),
-            notification_type=notification_type,
-        )
     page = data["page"] if "page" in data else 1
     page_size = (
         data["page_size"]
@@ -583,53 +573,6 @@ def get_notification_for_service(service_id, notification_id):
     )
 
 
-def search_for_notification_by_to_field(
-    service_id, search_term, statuses, notification_type
-):
-    results = notifications_dao.dao_get_notifications_by_recipient_or_reference(
-        service_id=service_id,
-        search_term=search_term,
-        statuses=statuses,
-        notification_type=notification_type,
-        page=1,
-        page_size=current_app.config["PAGE_SIZE"],
-    )
-
-    # We try and get the next page of results to work out if we need provide a pagination link to the next page
-    # in our response. Note, this was previously be done by having
-    # notifications_dao.dao_get_notifications_by_recipient_or_reference use count=False when calling
-    # Flask-Sqlalchemys `paginate'. But instead we now use this way because it is much more performant for
-    # services with many results (unlike using Flask SqlAlchemy `paginate` with `count=True`, this approach
-    # doesn't do an additional query to count all the results of which there could be millions but instead only
-    # asks for a single extra page of results).
-    next_page_of_pagination = notifications_dao.dao_get_notifications_by_recipient_or_reference(
-        service_id=service_id,
-        search_term=search_term,
-        statuses=statuses,
-        notification_type=notification_type,
-        page=2,
-        page_size=current_app.config["PAGE_SIZE"],
-        error_out=False,  # False so that if there are no results, it doesn't end in aborting with a 404
-    )
-
-    return (
-        jsonify(
-            notifications=notification_with_template_schema.dump(
-                results.items, many=True
-            ),
-            links=get_prev_next_pagination_links(
-                1,
-                len(next_page_of_pagination.items),
-                ".get_all_notifications_for_service",
-                statuses=statuses,
-                notification_type=notification_type,
-                service_id=service_id,
-            ),
-        ),
-        200,
-    )
-
-
 @service_blueprint.route("/<uuid:service_id>/notifications/monthly", methods=["GET"])
 def get_monthly_notification_stats(service_id):
     # check service_id validity

app/service_invite/rest.py

@@ -32,7 +32,7 @@ service_invite = Blueprint("service_invite", __name__)
 register_errors(service_invite)
 
 
-def _create_service_invite(invited_user, invite_link_host):
+def _create_service_invite(invited_user, nonce):
 
     template_id = current_app.config["INVITATION_EMAIL_TEMPLATE_ID"]
 
@@ -40,12 +40,6 @@ def _create_service_invite(invited_user, invite_link_host):
 
     service = Service.query.get(current_app.config["NOTIFY_SERVICE_ID"])
 
-    token = generate_token(
-        str(invited_user.email_address),
-        current_app.config["SECRET_KEY"],
-        current_app.config["DANGEROUS_SALT"],
-    )
-
     # The raw permissions are in the form "a,b,c,d"
     # but need to be in the form ["a", "b", "c", "d"]
     data = {}
@@ -59,7 +53,8 @@ def _create_service_invite(invited_user, invite_link_host):
     data["invited_user_email"] = invited_user.email_address
 
     url = os.environ["LOGIN_DOT_GOV_REGISTRATION_URL"]
-    url = url.replace("NONCE", token)
+
+    url = url.replace("NONCE", nonce)  # handed from data sent from admin.
 
     user_data_url_safe = get_user_data_url_safe(data)
 
@@ -94,10 +89,16 @@ def _create_service_invite(invited_user, invite_link_host):
 @service_invite.route("/service/<service_id>/invite", methods=["POST"])
 def create_invited_user(service_id):
     request_json = request.get_json()
+    try:
+        nonce = request_json.pop("nonce")
+    except KeyError:
+        current_app.logger.exception("nonce not found in submitted data.")
+        raise
+
     invited_user = invited_user_schema.load(request_json)
     save_invited_user(invited_user)
 
-    _create_service_invite(invited_user, request_json.get("invite_link_host"))
+    _create_service_invite(invited_user, nonce)
 
     return jsonify(data=invited_user_schema.dump(invited_user)), 201
 

migrations/versions/0044_jobs_to_notification_hist.py

@@ -31,10 +31,10 @@ def upgrade():
     #
     # go_live = datetime.datetime.strptime('2016-05-18', '%Y-%m-%d')
     # notifications_history_start_date = datetime.datetime.strptime('2016-06-26 23:21:55', '%Y-%m-%d %H:%M:%S')
-    # jobs = session.query(Job).join(Template).filter(Job.service_id == '95316ff0-e555-462d-a6e7-95d26fbfd091',
+    # stmt = select(Job).join(Template).filter(Job.service_id == '95316ff0-e555-462d-a6e7-95d26fbfd091',
     #                                                 Job.created_at >= go_live,
     #                                                 Job.created_at < notifications_history_start_date).all()
-    #
+    # jobs = db.session.execute(stmt).scalars().all()
     # for job in jobs:
     #     for i in range(0, job.notifications_delivered):
     #         notification = NotificationHistory(id=uuid.uuid4(),
@@ -76,12 +76,11 @@ def downgrade():
     #
     # go_live = datetime.datetime.strptime('2016-05-18', '%Y-%m-%d')
     # notifications_history_start_date = datetime.datetime.strptime('2016-06-26 23:21:55', '%Y-%m-%d %H:%M:%S')
-    #
+    # stmt = delete(NotificationHistory).where(
-    # session.query(NotificationHistory).filter(
     #     NotificationHistory.created_at >= go_live,
     #     NotificationHistory.service_id == '95316ff0-e555-462d-a6e7-95d26fbfd091',
-    #     NotificationHistory.created_at < notifications_history_start_date).delete()
+    #     NotificationHistory.created_at < notifications_history_start_date)
-    #
+    # session.execute(stmt)
     # session.commit()
     # ### end Alembic commands ###
     pass

notifications_utils/sanitise_text.py

@@ -122,19 +122,15 @@ class SanitiseText:
     def is_punjabi(cls, value):
         # Gukmukhi script or Shahmukhi script
 
-        if regex.search(r"[\u0A00-\u0A7F]+", value):
+        if (
-            return True
+            regex.search(r"[\u0A00-\u0A7F]+", value)
-        elif regex.search(r"[\u0600-\u06FF]+", value):
+            or regex.search(r"[\u0600-\u06FF]+", value)
-            return True
+            or regex.search(r"[\u0750-\u077F]+", value)
-        elif regex.search(r"[\u0750-\u077F]+", value):
+            or regex.search(r"[\u08A0-\u08FF]+", value)
-            return True
+            or regex.search(r"[\uFB50-\uFDFF]+", value)
-        elif regex.search(r"[\u08A0-\u08FF]+", value):
+            or regex.search(r"[\uFE70-\uFEFF]+", value)
-            return True
+            or regex.search(r"[\u0900-\u097F]+", value)
-        elif regex.search(r"[\uFB50-\uFDFF]+", value):
+        ):
-            return True
-        elif regex.search(r"[\uFE70-\uFEFF]+", value):
-            return True
-        elif regex.search(r"[\u0900-\u097F]+", value):
             return True
         return False
 
@@ -156,33 +152,27 @@ class SanitiseText:
 
     @classmethod
     def _is_extended_language_group_two(cls, value):
-        if regex.search(r"\p{IsBuhid}", value):
+        if (
-            return True
+            regex.search(r"\p{IsBuhid}", value)
-        if regex.search(r"\p{IsCanadian_Aboriginal}", value):
+            or regex.search(r"\p{IsCanadian_Aboriginal}", value)
-            return True
+            or regex.search(r"\p{IsCherokee}", value)
-        if regex.search(r"\p{IsCherokee}", value):
+            or regex.search(r"\p{IsDevanagari}", value)
-            return True
+            or regex.search(r"\p{IsEthiopic}", value)
-        if regex.search(r"\p{IsDevanagari}", value):
+            or regex.search(r"\p{IsGeorgian}", value)
-            return True
+        ):
-        if regex.search(r"\p{IsEthiopic}", value):
-            return True
-        if regex.search(r"\p{IsGeorgian}", value):
             return True
         return False
 
     @classmethod
     def _is_extended_language_group_three(cls, value):
-        if regex.search(r"\p{IsGreek}", value):
+        if (
-            return True
+            regex.search(r"\p{IsGreek}", value)
-        if regex.search(r"\p{IsGujarati}", value):
+            or regex.search(r"\p{IsGujarati}", value)
-            return True
+            or regex.search(r"\p{IsHanunoo}", value)
-        if regex.search(r"\p{IsHanunoo}", value):
+            or regex.search(r"\p{IsHebrew}", value)
-            return True
+            or regex.search(r"\p{IsLimbu}", value)
-        if regex.search(r"\p{IsHebrew}", value):
+            or regex.search(r"\p{IsKannada}", value)
-            return True
+        ):
-        if regex.search(r"\p{IsLimbu}", value):
-            return True
-        if regex.search(r"\p{IsKannada}", value):
             return True
         return False
 

tests/app/service/test_rest.py

@@ -1959,6 +1959,84 @@ def test_get_all_notifications_for_service_including_ones_made_by_jobs(
     assert response.status_code == 200
 
 
+def test_get_monthly_notification_stats_by_user(
+    client,
+    sample_service,
+    sample_user,
+    mocker,
+):
+    mock_s3 = mocker.patch("app.service.rest.get_phone_number_from_s3")
+    mock_s3.return_value = ""
+
+    mock_s3 = mocker.patch("app.service.rest.get_personalisation_from_s3")
+    mock_s3.return_value = {}
+
+    auth_header = create_admin_authorization_header()
+
+    response = client.get(
+        path=(
+            f"/service/{sample_service.id}/notifications/{sample_user.id}/monthly?year=2024"
+        ),
+        headers=[auth_header],
+    )
+
+    resp = json.loads(response.get_data(as_text=True))
+    print(f"RESP is {resp}")
+    # TODO This test could be a little more complete
+    assert response.status_code == 200
+
+
+def test_get_single_month_notification_stats_by_user(
+    client,
+    sample_service,
+    sample_user,
+    mocker,
+):
+    mock_s3 = mocker.patch("app.service.rest.get_phone_number_from_s3")
+    mock_s3.return_value = ""
+
+    mock_s3 = mocker.patch("app.service.rest.get_personalisation_from_s3")
+    mock_s3.return_value = {}
+
+    auth_header = create_admin_authorization_header()
+
+    response = client.get(
+        path=(
+            f"/service/{sample_service.id}/notifications/{sample_user.id}/month?year=2024&month=07"
+        ),
+        headers=[auth_header],
+    )
+
+    resp = json.loads(response.get_data(as_text=True))
+    print(f"RESP is {resp}")
+    # TODO This test could be a little more complete
+    assert response.status_code == 200
+
+
+def test_get_single_month_notification_stats_for_service(
+    client,
+    sample_service,
+    mocker,
+):
+    mock_s3 = mocker.patch("app.service.rest.get_phone_number_from_s3")
+    mock_s3.return_value = ""
+
+    mock_s3 = mocker.patch("app.service.rest.get_personalisation_from_s3")
+    mock_s3.return_value = {}
+
+    auth_header = create_admin_authorization_header()
+
+    response = client.get(
+        path=(f"/service/{sample_service.id}/notifications/month?year=2024&month=07"),
+        headers=[auth_header],
+    )
+
+    resp = json.loads(response.get_data(as_text=True))
+    print(f"RESP is {resp}")
+    # TODO This test could be a little more complete
+    assert response.status_code == 200
+
+
 def test_get_only_api_created_notifications_for_service(
     admin_request,
     sample_job,

tests/app/service_invite/test_service_invite_rest.py

@@ -45,6 +45,7 @@ def test_create_invited_user(
         permissions="send_messages,manage_service,manage_api_keys",
         auth_type=AuthType.EMAIL,
         folder_permissions=["folder_1", "folder_2", "folder_3"],
+        nonce="FakeNonce",
         **extra_args,
     )
 
@@ -108,6 +109,7 @@ def test_create_invited_user_without_auth_type(
         "from_user": str(invite_from.id),
         "permissions": "send_messages,manage_service,manage_api_keys",
         "folder_permissions": [],
+        "nonce": "FakeNonce",
     }
 
     json_resp = admin_request.post(
@@ -131,6 +133,7 @@ def test_create_invited_user_invalid_email(client, sample_service, mocker, fake_
         "from_user": str(invite_from.id),
         "permissions": "send_messages,manage_service,manage_api_keys",
         "folder_permissions": [fake_uuid, fake_uuid],
+        "nonce": "FakeNonce",
     }
 
     data = json.dumps(data)