diff --git a/app/dao/fact_billing_dao.py b/app/dao/fact_billing_dao.py index 5904fe9a0..97f346cf5 100644 --- a/app/dao/fact_billing_dao.py +++ b/app/dao/fact_billing_dao.py @@ -779,45 +779,37 @@ def fetch_daily_volumes_for_platform(start_date, end_date): FactBilling.local_date, func.sum( case( - [ - ( - FactBilling.notification_type == NotificationType.SMS, - FactBilling.notifications_sent, - ) - ], + ( + FactBilling.notification_type == NotificationType.SMS, + FactBilling.notifications_sent, + ), else_=0, ) ).label("sms_totals"), func.sum( case( - [ - ( - FactBilling.notification_type == NotificationType.SMS, - FactBilling.billable_units, - ) - ], + ( + FactBilling.notification_type == NotificationType.SMS, + FactBilling.billable_units, + ), else_=0, ) ).label("sms_fragment_totals"), func.sum( case( - [ - ( - FactBilling.notification_type == NotificationType.SMS, - FactBilling.billable_units * FactBilling.rate_multiplier, - ) - ], + ( + FactBilling.notification_type == NotificationType.SMS, + FactBilling.billable_units * FactBilling.rate_multiplier, + ), else_=0, ) ).label("sms_fragments_times_multiplier"), func.sum( case( - [ - ( - FactBilling.notification_type == NotificationType.EMAIL, - FactBilling.notifications_sent, - ) - ], + ( + FactBilling.notification_type == NotificationType.EMAIL, + FactBilling.notifications_sent, + ), else_=0, ) ).label("email_totals"), @@ -897,34 +889,28 @@ def fetch_volumes_by_service(start_date, end_date): FactBilling.service_id, func.sum( case( - [ - ( - FactBilling.notification_type == NotificationType.SMS, - FactBilling.notifications_sent, - ) - ], + ( + FactBilling.notification_type == NotificationType.SMS, + FactBilling.notifications_sent, + ), else_=0, ) ).label("sms_totals"), func.sum( case( - [ - ( - FactBilling.notification_type == NotificationType.SMS, - FactBilling.billable_units * FactBilling.rate_multiplier, - ) - ], + ( + FactBilling.notification_type == NotificationType.SMS, + FactBilling.billable_units * FactBilling.rate_multiplier, + ), else_=0, ) ).label("sms_fragments_times_multiplier"), func.sum( case( - [ - ( - FactBilling.notification_type == NotificationType.EMAIL, - FactBilling.notifications_sent, - ) - ], + ( + FactBilling.notification_type == NotificationType.EMAIL, + FactBilling.notifications_sent, + ), else_=0, ) ).label("email_totals"), diff --git a/app/dao/fact_notification_status_dao.py b/app/dao/fact_notification_status_dao.py index 286beebc2..5eee7d8c2 100644 --- a/app/dao/fact_notification_status_dao.py +++ b/app/dao/fact_notification_status_dao.py @@ -459,25 +459,21 @@ def get_total_notifications_for_date_range(start_date, end_date): FactNotificationStatus.local_date.label("local_date"), func.sum( case( - [ - ( - FactNotificationStatus.notification_type - == NotificationType.EMAIL, - FactNotificationStatus.notification_count, - ) - ], + ( + FactNotificationStatus.notification_type + == NotificationType.EMAIL, + FactNotificationStatus.notification_count, + ), else_=0, ) ).label("emails"), func.sum( case( - [ - ( - FactNotificationStatus.notification_type - == NotificationType.SMS, - FactNotificationStatus.notification_count, - ) - ], + ( + FactNotificationStatus.notification_type + == NotificationType.SMS, + FactNotificationStatus.notification_count, + ), else_=0, ) ).label("sms"), @@ -507,78 +503,66 @@ def fetch_monthly_notification_statuses_per_service(start_date, end_date): FactNotificationStatus.notification_type, func.sum( case( - [ - ( - FactNotificationStatus.notification_status.in_( - [NotificationStatus.SENDING, NotificationStatus.PENDING] - ), - FactNotificationStatus.notification_count, - ) - ], + ( + FactNotificationStatus.notification_status.in_( + [NotificationStatus.SENDING, NotificationStatus.PENDING] + ), + FactNotificationStatus.notification_count, + ), else_=0, ) ).label("count_sending"), func.sum( case( - [ - ( - FactNotificationStatus.notification_status - == NotificationStatus.DELIVERED, - FactNotificationStatus.notification_count, - ) - ], + ( + FactNotificationStatus.notification_status + == NotificationStatus.DELIVERED, + FactNotificationStatus.notification_count, + ), else_=0, ) ).label("count_delivered"), func.sum( case( - [ - ( - FactNotificationStatus.notification_status.in_( - [ - NotificationStatus.TECHNICAL_FAILURE, - NotificationStatus.FAILED, - ] - ), - FactNotificationStatus.notification_count, - ) - ], + ( + FactNotificationStatus.notification_status.in_( + [ + NotificationStatus.TECHNICAL_FAILURE, + NotificationStatus.FAILED, + ] + ), + FactNotificationStatus.notification_count, + ), else_=0, ) ).label("count_technical_failure"), func.sum( case( - [ - ( - FactNotificationStatus.notification_status - == NotificationStatus.TEMPORARY_FAILURE, - FactNotificationStatus.notification_count, - ) - ], + ( + FactNotificationStatus.notification_status + == NotificationStatus.TEMPORARY_FAILURE, + FactNotificationStatus.notification_count, + ), else_=0, ) ).label("count_temporary_failure"), func.sum( case( - [ - ( - FactNotificationStatus.notification_status - == NotificationStatus.PERMANENT_FAILURE, - FactNotificationStatus.notification_count, - ) - ], + ( + FactNotificationStatus.notification_status + == NotificationStatus.PERMANENT_FAILURE, + FactNotificationStatus.notification_count, + ), else_=0, ) ).label("count_permanent_failure"), func.sum( case( - [ - ( - FactNotificationStatus.notification_status - == NotificationStatus.SENT, - FactNotificationStatus.notification_count, - ) - ], + ( + FactNotificationStatus.notification_status + == NotificationStatus.SENT, + FactNotificationStatus.notification_count, + ), else_=0, ) ).label("count_sent"), diff --git a/app/dao/fact_processing_time_dao.py b/app/dao/fact_processing_time_dao.py index 1a9de6da6..350de270a 100644 --- a/app/dao/fact_processing_time_dao.py +++ b/app/dao/fact_processing_time_dao.py @@ -40,19 +40,17 @@ def get_processing_time_percentage_for_date_range(start_date, end_date): FactProcessingTime.messages_total, FactProcessingTime.messages_within_10_secs, case( - [ + ( + FactProcessingTime.messages_total > 0, ( - FactProcessingTime.messages_total > 0, ( - ( - FactProcessingTime.messages_within_10_secs - / FactProcessingTime.messages_total.cast(db.Float) - ) - * 100 - ), + FactProcessingTime.messages_within_10_secs + / FactProcessingTime.messages_total.cast(db.Float) + ) + * 100 ), - (FactProcessingTime.messages_total == 0, 100.0), - ] + ), + (FactProcessingTime.messages_total == 0, 100.0), ).label("percentage"), ) .filter( diff --git a/app/dao/inbound_sms_dao.py b/app/dao/inbound_sms_dao.py index 9b33ff0c8..272ae5e1c 100644 --- a/app/dao/inbound_sms_dao.py +++ b/app/dao/inbound_sms_dao.py @@ -6,7 +6,7 @@ from sqlalchemy.orm import aliased from app import db from app.dao.dao_utils import autocommit from app.enums import NotificationType -from app.models import InboundSms, InboundSmsHistory, Service, ServiceDataRetention +from app.models import InboundSms, InboundSmsHistory, ServiceDataRetention from app.utils import midnight_n_days_ago @@ -122,9 +122,7 @@ def delete_inbound_sms_older_than_retention(): ) flexible_data_retention = ( - ServiceDataRetention.query.join( - ServiceDataRetention.service, Service.inbound_number - ) + ServiceDataRetention.query.join(ServiceDataRetention.service) .filter(ServiceDataRetention.notification_type == NotificationType.SMS) .all() ) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index fe201dd8c..2928046b6 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -7,7 +7,7 @@ from notifications_utils.recipients import ( try_validate_and_format_phone_number, validate_and_format_email_address, ) -from sqlalchemy import asc, desc, or_, select, union +from sqlalchemy import asc, desc, or_, select, text, union from sqlalchemy.orm import joinedload from sqlalchemy.orm.exc import NoResultFound from sqlalchemy.sql import functions @@ -386,15 +386,15 @@ def insert_notification_history_delete_notifications( "qry_limit": qry_limit, } - db.session.execute(select_into_temp_table, input_params) + db.session.execute(text(select_into_temp_table), input_params) - result = db.session.execute("select count(*) from NOTIFICATION_ARCHIVE").fetchone()[ - 0 - ] + result = db.session.execute( + text("select count(*) from NOTIFICATION_ARCHIVE") + ).fetchone()[0] - db.session.execute(insert_query) + db.session.execute(text(insert_query)) - db.session.execute(delete_query) + db.session.execute(text(delete_query)) return result @@ -585,7 +585,7 @@ def dao_get_notifications_processing_time_stats(start_date, end_date): ) result = db.session.execute(stmt) - return result.scalar_one() + return result.one() def dao_get_last_notification_added_for_job_id(job_id): diff --git a/app/dao/permissions_dao.py b/app/dao/permissions_dao.py index fb00172e0..92e8fc291 100644 --- a/app/dao/permissions_dao.py +++ b/app/dao/permissions_dao.py @@ -26,7 +26,9 @@ class PermissionDAO(DAOClass): ): try: if replace: - query = self.Meta.model.query.filter_by(user=user, service=service) + query = self.Meta.model.query.filter( + self.Meta.model.user == user, self.Meta.model.service == service + ) query.delete() for p in permissions: p.user = user diff --git a/app/dao/service_user_dao.py b/app/dao/service_user_dao.py index f75b17873..0b991a4fc 100644 --- a/app/dao/service_user_dao.py +++ b/app/dao/service_user_dao.py @@ -4,7 +4,12 @@ from app.models import ServiceUser, User def dao_get_service_user(user_id, service_id): - return ServiceUser.query.filter_by(user_id=user_id, service_id=service_id).one() + # TODO: This has been changed to account for the test case failure + # that used this method but have any service user to return. Somehow, this + # started to throw an error with one() method in sqlalchemy 2.0 unlike 1.4 + return ServiceUser.query.filter_by( + user_id=user_id, service_id=service_id + ).one_or_none() def dao_get_active_service_users(service_id): diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 690317ef4..df8e59287 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -102,23 +102,17 @@ def dao_fetch_live_services_data(): Service.volume_sms.label("sms_volume_intent"), Service.volume_email.label("email_volume_intent"), case( - [ - ( - this_year_ft_billing.c.notification_type - == NotificationType.EMAIL, - func.sum(this_year_ft_billing.c.notifications_sent), - ) - ], + ( + this_year_ft_billing.c.notification_type == NotificationType.EMAIL, + func.sum(this_year_ft_billing.c.notifications_sent), + ), else_=0, ).label("email_totals"), case( - [ - ( - this_year_ft_billing.c.notification_type - == NotificationType.SMS, - func.sum(this_year_ft_billing.c.notifications_sent), - ) - ], + ( + this_year_ft_billing.c.notification_type == NotificationType.SMS, + func.sum(this_year_ft_billing.c.notifications_sent), + ), else_=0, ).label("sms_totals"), AnnualBilling.free_sms_fragment_limit, @@ -178,15 +172,15 @@ def dao_fetch_live_services_data(): def dao_fetch_service_by_id(service_id, only_active=False): stmt = ( select(Service) - .options(joinedload(Service.users)) .where(Service.id == service_id) + .options(joinedload(Service.users)) ) if only_active: stmt = stmt.where(Service.active) result = db.session.execute(stmt) - return result.unique().scalars().first() + return result.unique().scalars().one() def dao_fetch_service_by_inbound_number(number): @@ -241,8 +235,7 @@ def dao_archive_service(service_id): # to ensure that db.session still contains the models when it comes to creating history objects service = ( Service.query.options( - joinedload(Service.templates), - joinedload(Service.templates.template_redacted), + joinedload(Service.templates).subqueryload(Template.template_redacted), joinedload(Service.api_keys), ) .filter(Service.id == service_id) @@ -329,7 +322,12 @@ def dao_add_user_to_service(service, user, permissions=None, folder_permissions= try: from app.dao.permissions_dao import permission_dao - service.users.append(user) + # As per SQLAlchemy 2.0, we need to add the user to the service only if the user is not already added; + # otherwise it throws sqlalchemy.exc.IntegrityError: + # (psycopg2.errors.UniqueViolation) duplicate key value violates unique constraint "uix_user_to_service" + service_user = dao_get_service_user(user.id, service.id) + if service_user is None: + service.users.append(user) permission_dao.set_user_service_permission( user, service, permissions, _commit=False ) diff --git a/app/dao/users_dao.py b/app/dao/users_dao.py index fc3dd5612..ab958f2e5 100644 --- a/app/dao/users_dao.py +++ b/app/dao/users_dao.py @@ -2,6 +2,8 @@ import uuid from datetime import datetime, timedelta from secrets import randbelow +import sqlalchemy +from flask import current_app from sqlalchemy import func from sqlalchemy.orm import joinedload @@ -11,7 +13,7 @@ from app.dao.permissions_dao import permission_dao from app.dao.service_user_dao import dao_get_service_users_by_user_id from app.enums import AuthType, PermissionType from app.errors import InvalidRequest -from app.models import User, VerifyCode +from app.models import Organization, Service, User, VerifyCode from app.utils import escape_special_characters, get_archived_db_column_value @@ -39,10 +41,21 @@ def get_login_gov_user(login_uuid, email_address): user = User.query.filter_by(login_uuid=login_uuid).first() if user: if user.email_address != email_address: - save_user_attribute(user, {"email_address": email_address}) + try: + save_user_attribute(user, {"email_address": email_address}) + except sqlalchemy.exc.IntegrityError as ie: + # We are trying to change the email address as a courtesy, + # based on the assumption that the user has somehow changed their + # address in login.gov. + # But if we cannot change the email address, at least we don't + # want to fail here, otherwise the user will be locked out. + current_app.logger.error(ie) + db.session.rollback() + return user # Remove this 1 July 2025, all users should have login.gov uuids by now - user = User.query.filter_by(email_address=email_address).first() + user = User.query.filter(User.email_address.ilike(email_address)).first() + if user: save_user_attribute(user, {"login_uuid": login_uuid}) return user @@ -172,15 +185,14 @@ def update_user_password(user, password): def get_user_and_accounts(user_id): + # TODO: With sqlalchemy 2.0 change as below because of the breaking change + # at User.organizations.services, we need to verify that the below subqueryload + # that we have put is functionally doing the same thing as before return ( User.query.filter(User.id == user_id) .options( - # eagerly load the user's services and organizations, and also the service's org and vice versa - # (so we can see if the user knows about it) - joinedload(User.services), - joinedload(User.organizations), - joinedload(User.organizations.services), - joinedload(User.services.organization), + joinedload(User.services).joinedload(Service.organization), + joinedload(User.organizations).subqueryload(Organization.services), ) .one() ) diff --git a/app/status/healthcheck.py b/app/status/healthcheck.py index eda4396a1..16fd65a09 100644 --- a/app/status/healthcheck.py +++ b/app/status/healthcheck.py @@ -1,4 +1,5 @@ from flask import Blueprint, jsonify, request +from sqlalchemy import text from app import db, version from app.dao.organization_dao import dao_count_organizations_with_live_services @@ -37,5 +38,5 @@ def live_service_and_organization_counts(): def get_db_version(): query = "SELECT version_num FROM alembic_version" - full_name = db.session.execute(query).fetchone()[0] + full_name = db.session.execute(text(query)).fetchone()[0] return full_name diff --git a/app/user/rest.py b/app/user/rest.py index 3ebca90ed..88236216b 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -6,6 +6,7 @@ from urllib.parse import urlencode from flask import Blueprint, abort, current_app, jsonify, request from notifications_utils.recipients import is_us_phone_number, use_numeric_sender from sqlalchemy.exc import IntegrityError +from sqlalchemy.orm.exc import NoResultFound from app import redis_store from app.config import QueueNames @@ -533,6 +534,12 @@ def set_permissions(user_id, service_id): # TODO fix security hole, how do we verify that the user # who is making this request has permission to make the request. service_user = dao_get_service_user(user_id, service_id) + # TODO: Below exception is raised to account for the test case failure that got handled + # on its own in 1.4 when dao_get_service_user() returned an excpetion in case no result was found + if service_user is None: + raise NoResultFound( + "No ServiceUser found with the provided user_id and service_id" + ) user = get_user_by_id(user_id) service = dao_fetch_service_by_id(service_id=service_id) diff --git a/tests/app/dao/test_fact_billing_dao.py b/tests/app/dao/test_fact_billing_dao.py index cebed26d8..0282d6983 100644 --- a/tests/app/dao/test_fact_billing_dao.py +++ b/tests/app/dao/test_fact_billing_dao.py @@ -838,7 +838,7 @@ def test_fetch_sms_billing_for_all_services_with_remainder(notify_db_session): }, ] - assert [dict(result) for result in results] == expected_results + assert [result._asdict() for result in results] == expected_results def test_fetch_sms_billing_for_all_services_without_an_organization_appears( @@ -893,7 +893,7 @@ def test_fetch_sms_billing_for_all_services_without_an_organization_appears( }, ] - assert [dict(result) for result in results] == expected_results + assert [result._asdict() for result in results] == expected_results @freeze_time("2019-06-01 13:30") @@ -1228,8 +1228,8 @@ def test_query_organization_sms_usage_for_year_handles_multiple_services( result = query_organization_sms_usage_for_year(org.id, 2022).all() - service_1_rows = [row for row in result if row.service_id == service_1.id] - service_2_rows = [row for row in result if row.service_id == service_2.id] + service_1_rows = [row._asdict() for row in result if row.service_id == service_1.id] + service_2_rows = [row._asdict() for row in result if row.service_id == service_2.id] assert len(service_1_rows) == 2 assert len(service_2_rows) == 2 @@ -1256,10 +1256,10 @@ def test_query_organization_sms_usage_for_year_handles_multiple_services( # assert total costs are accurate assert ( - float(sum(row.cost for row in service_1_rows)) == 1 + float(sum(row["cost"] for row in service_1_rows)) == 1 ) # rows with 2 and 4, allowance of 5 assert ( - float(sum(row.cost for row in service_2_rows)) == 14 + float(sum(row["cost"] for row in service_2_rows)) == 14 ) # rows with 8 and 16, allowance of 10 @@ -1296,7 +1296,10 @@ def test_query_organization_sms_usage_for_year_handles_multiple_rates( financial_year_start=current_year, ) - result = query_organization_sms_usage_for_year(org.id, 2022).all() + result = [ + row._asdict() + for row in query_organization_sms_usage_for_year(org.id, 2022).all() + ] # al lthe free allowance is used on the first day assert result[0]["local_date"] == date(2022, 4, 29) diff --git a/tests/app/template_folder/test_template_folder_rest.py b/tests/app/template_folder/test_template_folder_rest.py index dae559ada..6461ad3df 100644 --- a/tests/app/template_folder/test_template_folder_rest.py +++ b/tests/app/template_folder/test_template_folder_rest.py @@ -237,7 +237,7 @@ def test_update_template_folder_users(admin_request, sample_service): [ ({}, "name is a required property"), ({"name": None}, "name None is not of type string"), - ({"name": ""}, "name is too short"), + ({"name": ""}, "name should be non-empty"), ], ) def test_update_template_folder_fails_if_missing_name( diff --git a/tests/app/webauthn/test_rest.py b/tests/app/webauthn/test_rest.py index 5c0e21f7a..b663600a8 100644 --- a/tests/app/webauthn/test_rest.py +++ b/tests/app/webauthn/test_rest.py @@ -89,7 +89,7 @@ def test_create_webauthn_credential_returns_201(admin_request, sample_user): "name None is not of type string", ), # name is empty - ({"name": "", "credential_data": "ABC123"}, "name is too short"), + ({"name": "", "credential_data": "ABC123"}, "name should be non-empty"), ], ) def test_create_webauthn_credential_errors_if_schema_violation( @@ -131,7 +131,7 @@ def test_update_webauthn_credential_returns_200(admin_request, sample_user): # name is null ({"name": None}, "name None is not of type string"), # name is empty - ({"name": ""}, "name is too short"), + ({"name": ""}, "name should be non-empty"), ], ) def test_update_webauthn_credential_errors_if_schema_violation(