From 69e5ddae4f601ff3c95fa9103651c68cba839c1f Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 6 Apr 2021 13:42:18 +0100 Subject: [PATCH 1/5] When a service is associated with a organisation set the free allowance to the default free allowance for the organisation type. The update/insert for the default free allowance is done in a separate transaction. Updates to services need to happen in a transaction to trigger the insert into the ServicesHistory table. For that reason the call to set_default_free_allowance_for_service is done after the service is updated. I've added a try/except around the set_default_free_allowance_for_service call to ensure we still get the update to the service but get an exception log if the update to annual_billing fails. I believe it's important to preserve the update to the service in the unlikely event that the annual_billing upsert fails. --- app/dao/annual_billing_dao.py | 3 +- app/organisation/rest.py | 13 ++++++- tests/app/dao/test_annual_billing_dao.py | 18 ++++++++++ tests/app/organisation/test_rest.py | 43 ++++++++++++++++++++---- 4 files changed, 68 insertions(+), 9 deletions(-) diff --git a/app/dao/annual_billing_dao.py b/app/dao/annual_billing_dao.py index 36d12e540..5caa46011 100644 --- a/app/dao/annual_billing_dao.py +++ b/app/dao/annual_billing_dao.py @@ -53,7 +53,7 @@ def dao_get_all_free_sms_fragment_limit(service_id): ).order_by(AnnualBilling.financial_year_start).all() -def set_default_free_allowance_for_service(service, year_start=None): +def set_default_free_allowance_for_service(service, year_start=None, commit=True): default_free_sms_fragment_limits = { 'central': { 2020: 250_000, @@ -90,6 +90,7 @@ def set_default_free_allowance_for_service(service, year_start=None): } if not year_start: year_start = get_current_financial_year_start_year() + # handle cases where the year is less than 2020 or greater than 2021 if year_start < 2020: year_start = 2020 if year_start > 2021: diff --git a/app/organisation/rest.py b/app/organisation/rest.py index 77761fcf6..d745a7adb 100644 --- a/app/organisation/rest.py +++ b/app/organisation/rest.py @@ -1,8 +1,9 @@ from flask import Blueprint, abort, current_app, jsonify, request -from sqlalchemy.exc import IntegrityError +from sqlalchemy.exc import IntegrityError, SQLAlchemyError from app.config import QueueNames +from app.dao.annual_billing_dao import set_default_free_allowance_for_service from app.dao.fact_billing_dao import fetch_usage_year_for_organisation from app.dao.organisation_dao import ( dao_add_service_to_organisation, @@ -119,6 +120,16 @@ def link_service_to_organisation(organisation_id): service.organisation = None dao_add_service_to_organisation(service, organisation_id) + # Need to do the annual billing update in a separate transaction because the both the + # dao_add_service_to_organisation and set_default_free_allowance_for_service are wrapped in a transaction. + # Catch and report an error if the annual billing doesn't happen - but don't rollback the service update. + try: + set_default_free_allowance_for_service(service, year_start=None) + except SQLAlchemyError: + # No need to worry about key errors because service.organisation_type has a foreign key to organisation_types + current_app.logger.exception( + f"Exception caught when trying to update annual billing when the organisation " + f"changed for service: {service.id} to organisation: {organisation_id}") return '', 204 diff --git a/tests/app/dao/test_annual_billing_dao.py b/tests/app/dao/test_annual_billing_dao.py index 37e8e4fb2..4fa0954c6 100644 --- a/tests/app/dao/test_annual_billing_dao.py +++ b/tests/app/dao/test_annual_billing_dao.py @@ -91,3 +91,21 @@ def test_set_default_free_allowance_for_service_using_correct_year(sample_servic 25000, 2020 ) + + +@freeze_time('2021-04-01 14:02:00') +def test_set_default_free_allowance_for_service_updates_existing_year(sample_service): + set_default_free_allowance_for_service(service=sample_service, year_start=None) + annual_billing = AnnualBilling.query.all() + assert not sample_service.organisation_type + assert len(annual_billing) == 1 + assert annual_billing[0].service_id == sample_service.id + assert annual_billing[0].free_sms_fragment_limit == 10000 + + sample_service.organisation_type = 'central' + + set_default_free_allowance_for_service(service=sample_service, year_start=None) + annual_billing = AnnualBilling.query.all() + assert len(annual_billing) == 1 + assert annual_billing[0].service_id == sample_service.id + assert annual_billing[0].free_sms_fragment_limit == 150000 diff --git a/tests/app/organisation/test_rest.py b/tests/app/organisation/test_rest.py index a3734083a..02889e574 100644 --- a/tests/app/organisation/test_rest.py +++ b/tests/app/organisation/test_rest.py @@ -3,13 +3,14 @@ from datetime import datetime import pytest from freezegun import freeze_time +from sqlalchemy.exc import SQLAlchemyError from app.dao.organisation_dao import ( dao_add_service_to_organisation, dao_add_user_to_organisation, ) from app.dao.services_dao import dao_archive_service -from app.models import Organisation +from app.models import AnnualBilling, Organisation from tests.app.db import ( create_annual_billing, create_domain, @@ -491,19 +492,25 @@ def test_post_update_organisation_set_mou_emails_signed_by( } -def test_post_link_service_to_organisation(admin_request, sample_service, sample_organisation): +def test_post_link_service_to_organisation(admin_request, sample_service): data = { 'service_id': str(sample_service.id) } - + organisation = create_organisation(organisation_type='central') + assert len(organisation.services) == 0 + assert len(AnnualBilling.query.all()) == 0 admin_request.post( 'organisation.link_service_to_organisation', _data=data, - organisation_id=sample_organisation.id, + organisation_id=organisation.id, _expected_status=204 ) - assert len(sample_organisation.services) == 1 + assert len(organisation.services) == 1 + assert sample_service.organisation_type == 'central' + annual_billing = AnnualBilling.query.all() + assert len(annual_billing) == 1 + assert annual_billing[0].free_sms_fragment_limit == 150000 def test_post_link_service_to_another_org( @@ -511,7 +518,8 @@ def test_post_link_service_to_another_org( data = { 'service_id': str(sample_service.id) } - + assert len(sample_organisation.services) == 0 + assert not sample_service.organisation_type admin_request.post( 'organisation.link_service_to_organisation', _data=data, @@ -520,8 +528,9 @@ def test_post_link_service_to_another_org( ) assert len(sample_organisation.services) == 1 + assert not sample_service.organisation_type - new_org = create_organisation() + new_org = create_organisation(organisation_type='central') admin_request.post( 'organisation.link_service_to_organisation', _data=data, @@ -530,6 +539,10 @@ def test_post_link_service_to_another_org( ) assert not sample_organisation.services assert len(new_org.services) == 1 + assert sample_service.organisation_type == 'central' + annual_billing = AnnualBilling.query.all() + assert len(annual_billing) == 1 + assert annual_billing[0].free_sms_fragment_limit == 150000 def test_post_link_service_to_organisation_nonexistent_organisation( @@ -569,6 +582,22 @@ def test_post_link_service_to_organisation_missing_payload( ) +def test_link_service_to_organisation_updates_service_if_annual_billing_update_fails( + mocker, admin_request, sample_service, sample_organisation +): + mocker.patch('app.dao.annual_billing_dao.set_default_free_allowance_for_service', raises=SQLAlchemyError) + data = { + 'service_id': str(sample_service.id) + } + admin_request.post( + 'organisation.link_service_to_organisation', + organisation_id=str(sample_organisation.id), + _data=data, + _expected_status=204 + ) + assert sample_service.organisation_id == sample_organisation.id + + def test_rest_get_organisation_services( admin_request, sample_organisation, sample_service): dao_add_service_to_organisation(sample_service, sample_organisation.id) From 9a03e579d60d20821df4e69a2450b9edaf8eee3b Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 7 Apr 2021 09:58:12 +0100 Subject: [PATCH 2/5] When a service is created add the default annual billing for the service. This will need to be merged before https://github.com/alphagov/notifications-admin/pull/3855, it will be that until the admin PR is merged the annual billing will be set twice, but that's not an issue. --- app/service/rest.py | 14 +++++++++- tests/app/organisation/test_rest.py | 3 ++- tests/app/service/test_rest.py | 42 +++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+), 2 deletions(-) diff --git a/app/service/rest.py b/app/service/rest.py index 09ef7f27e..03d111be9 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -4,12 +4,13 @@ from datetime import datetime from flask import Blueprint, current_app, jsonify, request from notifications_utils.letter_timings import letter_can_be_cancelled from notifications_utils.timezones import convert_utc_to_bst -from sqlalchemy.exc import IntegrityError +from sqlalchemy.exc import IntegrityError, SQLAlchemyError from sqlalchemy.orm.exc import NoResultFound from app.aws import s3 from app.config import QueueNames from app.dao import fact_notification_status_dao, notifications_dao +from app.dao.annual_billing_dao import set_default_free_allowance_for_service from app.dao.api_key_dao import ( expire_api_key, get_model_api_keys, @@ -255,6 +256,17 @@ def create_service(): dao_create_service(valid_service, user) + # Need to do the annual billing update in a separate transaction because the both the + # dao_add_service_to_organisation and set_default_free_allowance_for_service are wrapped in a transaction. + # Catch and report an error if the annual billing doesn't happen - but don't rollback the service update. + try: + set_default_free_allowance_for_service(valid_service, year_start=None) + except SQLAlchemyError: + # No need to worry about key errors because service.organisation_type has a foreign key to organisation_types + current_app.logger.exception( + f"Exception caught when trying to insert annual billing creating a service {valid_service.id} " + f"for organisation_type {valid_service.organisation_type}") + return jsonify(data=service_schema.dump(valid_service).data), 201 diff --git a/tests/app/organisation/test_rest.py b/tests/app/organisation/test_rest.py index 02889e574..5b2def4e4 100644 --- a/tests/app/organisation/test_rest.py +++ b/tests/app/organisation/test_rest.py @@ -585,7 +585,7 @@ def test_post_link_service_to_organisation_missing_payload( def test_link_service_to_organisation_updates_service_if_annual_billing_update_fails( mocker, admin_request, sample_service, sample_organisation ): - mocker.patch('app.dao.annual_billing_dao.set_default_free_allowance_for_service', raises=SQLAlchemyError) + mocker.patch('app.organisation.rest.set_default_free_allowance_for_service', raises=SQLAlchemyError) data = { 'service_id': str(sample_service.id) } @@ -596,6 +596,7 @@ def test_link_service_to_organisation_updates_service_if_annual_billing_update_f _expected_status=204 ) assert sample_service.organisation_id == sample_organisation.id + assert len(AnnualBilling.query.all()) == 0 def test_rest_get_organisation_services( diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 1878befb0..6e2599a45 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -6,6 +6,7 @@ from unittest.mock import ANY import pytest from flask import current_app, url_for from freezegun import freeze_time +from sqlalchemy.exc import SQLAlchemyError from app.dao.organisation_dao import dao_add_service_to_organisation from app.dao.service_sms_sender_dao import dao_get_sms_senders_by_service_id @@ -31,6 +32,7 @@ from app.models import ( SERVICE_PERMISSION_TYPES, SMS_TYPE, UPLOAD_LETTERS, + AnnualBilling, EmailBranding, InboundNumber, Notification, @@ -482,6 +484,46 @@ def test_create_service_with_domain_sets_organisation( assert json_resp['data']['organisation'] is None +def test_create_service_should_create_annual_billing_for_service( + admin_request, sample_user +): + data = { + 'name': 'created service', + 'user_id': str(sample_user.id), + 'message_limit': 1000, + 'restricted': False, + 'active': False, + 'email_from': 'created.service', + 'created_by': str(sample_user.id) + } + assert len(AnnualBilling.query.all()) == 0 + admin_request.post('service.create_service', _data=data, _expected_status=201) + + annual_billing = AnnualBilling.query.all() + assert len(annual_billing) == 1 + + +def test_create_service_should_create_service_if_annual_billing_query_fails( + admin_request, sample_user, mocker +): + mocker.patch('app.service.rest.set_default_free_allowance_for_service', raises=SQLAlchemyError) + data = { + 'name': 'created service', + 'user_id': str(sample_user.id), + 'message_limit': 1000, + 'restricted': False, + 'active': False, + 'email_from': 'created.service', + 'created_by': str(sample_user.id) + } + assert len(AnnualBilling.query.all()) == 0 + admin_request.post('service.create_service', _data=data, _expected_status=201) + + annual_billing = AnnualBilling.query.all() + assert len(annual_billing) == 0 + assert len(Service.query.filter(Service.name == 'created service').all()) == 1 + + def test_create_service_inherits_branding_from_organisation( admin_request, sample_user, From cf351356052c78432faf1a53f0b53d4cae5501ae Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 12 Apr 2021 13:52:40 +0100 Subject: [PATCH 3/5] Adding @nested_transactional for transactions that require more than one db update/insert. Using a savepoint for the multiple transactions allows us to rollback if there is an error when executing the second db transaction. However, this does add a bit of complexity. Developers need to manage the db session when calling multiple nested tranactions. Unit tests have been added to test this functionality and some end to end tests have been done to make sure all transactions are rollback if there is an exception while executing the transaction. --- app/billing/rest.py | 3 ++ app/commands.py | 2 + app/dao/annual_billing_dao.py | 6 +-- app/dao/dao_utils.py | 17 ++++++++ app/dao/organisation_dao.py | 9 +++- app/dao/services_dao.py | 9 +++- app/organisation/rest.py | 15 +++---- app/service/rest.py | 18 +++----- tests/app/dao/test_organisation_dao.py | 2 +- tests/app/organisation/test_rest.py | 59 ++++++++++++++++++-------- tests/app/service/test_rest.py | 9 ++-- 11 files changed, 99 insertions(+), 50 deletions(-) diff --git a/app/billing/rest.py b/app/billing/rest.py index 3f5f6e08c..95c79798f 100644 --- a/app/billing/rest.py +++ b/app/billing/rest.py @@ -1,5 +1,6 @@ from flask import Blueprint, jsonify, request +from app import db from app.billing.billing_schemas import ( create_or_update_free_sms_fragment_limit_schema, serialize_ft_billing_remove_emails, @@ -83,6 +84,7 @@ def get_free_sms_fragment_limit(service_id): annual_billing = dao_create_or_update_annual_billing_for_year(service_id, annual_billing.free_sms_fragment_limit, financial_year_start) + db.session.commit() return jsonify(annual_billing.serialize_free_sms_items()), 200 @@ -118,3 +120,4 @@ def update_free_sms_fragment_limit_data(service_id, free_sms_fragment_limit, fin free_sms_fragment_limit, financial_year_start ) + db.session.commit() diff --git a/app/commands.py b/app/commands.py index c98ce9bf3..12801b4a8 100644 --- a/app/commands.py +++ b/app/commands.py @@ -880,6 +880,7 @@ def populate_annual_billing_with_the_previous_years_allowance(year): dao_create_or_update_annual_billing_for_year(service_id=row.id, free_sms_fragment_limit=free_allowance[0], financial_year_start=int(year)) + db.session.commit() @notify_command(name='populate-annual-billing-with-defaults') @@ -914,3 +915,4 @@ def populate_annual_billing_with_defaults(year, missing_services_only): for service in active_services: set_default_free_allowance_for_service(service, year) + db.session.commit() diff --git a/app/dao/annual_billing_dao.py b/app/dao/annual_billing_dao.py index 5caa46011..0ce4d0357 100644 --- a/app/dao/annual_billing_dao.py +++ b/app/dao/annual_billing_dao.py @@ -1,12 +1,12 @@ from flask import current_app from app import db -from app.dao.dao_utils import transactional +from app.dao.dao_utils import nested_transactional, transactional from app.dao.date_util import get_current_financial_year_start_year from app.models import AnnualBilling -@transactional +@nested_transactional def dao_create_or_update_annual_billing_for_year(service_id, free_sms_fragment_limit, financial_year_start): result = dao_get_free_sms_fragment_limit_for_year(service_id, financial_year_start) @@ -53,7 +53,7 @@ def dao_get_all_free_sms_fragment_limit(service_id): ).order_by(AnnualBilling.financial_year_start).all() -def set_default_free_allowance_for_service(service, year_start=None, commit=True): +def set_default_free_allowance_for_service(service, year_start=None): default_free_sms_fragment_limits = { 'central': { 2020: 250_000, diff --git a/app/dao/dao_utils.py b/app/dao/dao_utils.py index f5515d8b6..71c881bb3 100644 --- a/app/dao/dao_utils.py +++ b/app/dao/dao_utils.py @@ -18,6 +18,23 @@ def transactional(func): return commit_or_rollback +def nested_transactional(func): + # This creates a save point for the nested transaction. + # You must manage the commit or rollback from outer most call of the nested of the transactions. + @wraps(func) + def commit_or_rollback(*args, **kwargs): + try: + db.session.begin_nested() + res = func(*args, **kwargs) + db.session.commit() + return res + except Exception: + db.session.rollback() + raise + + return commit_or_rollback + + class VersionOptions(): def __init__(self, model_class, history_class=None, must_write_history=True): diff --git a/app/dao/organisation_dao.py b/app/dao/organisation_dao.py index 877f8af30..e4b95d92f 100644 --- a/app/dao/organisation_dao.py +++ b/app/dao/organisation_dao.py @@ -1,7 +1,12 @@ from sqlalchemy.sql.expression import func from app import db -from app.dao.dao_utils import VersionOptions, transactional, version_class +from app.dao.dao_utils import ( + VersionOptions, + nested_transactional, + transactional, + version_class, +) from app.models import Domain, Organisation, Service, User @@ -105,7 +110,7 @@ def _update_organisation_services(organisation, attribute, only_where_none=True) db.session.add(service) -@transactional +@nested_transactional @version_class(Service) def dao_add_service_to_organisation(service, organisation_id): organisation = Organisation.query.filter_by( diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index c68092c64..18071f5fa 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -7,7 +7,12 @@ from sqlalchemy.orm import joinedload from sqlalchemy.sql.expression import and_, asc, case, func from app import db -from app.dao.dao_utils import VersionOptions, transactional, version_class +from app.dao.dao_utils import ( + VersionOptions, + nested_transactional, + transactional, + version_class, +) from app.dao.date_util import get_current_financial_year from app.dao.email_branding_dao import dao_get_email_branding_by_name from app.dao.letter_branding_dao import dao_get_letter_branding_by_name @@ -284,7 +289,7 @@ def dao_fetch_service_by_id_and_user(service_id, user_id): ).one() -@transactional +@nested_transactional @version_class(Service) def dao_create_service( service, diff --git a/app/organisation/rest.py b/app/organisation/rest.py index d745a7adb..06b4ea83c 100644 --- a/app/organisation/rest.py +++ b/app/organisation/rest.py @@ -2,6 +2,7 @@ from flask import Blueprint, abort, current_app, jsonify, request from sqlalchemy.exc import IntegrityError, SQLAlchemyError +from app import db from app.config import QueueNames from app.dao.annual_billing_dao import set_default_free_allowance_for_service from app.dao.fact_billing_dao import fetch_usage_year_for_organisation @@ -119,17 +120,13 @@ def link_service_to_organisation(organisation_id): service = dao_fetch_service_by_id(data['service_id']) service.organisation = None - dao_add_service_to_organisation(service, organisation_id) - # Need to do the annual billing update in a separate transaction because the both the - # dao_add_service_to_organisation and set_default_free_allowance_for_service are wrapped in a transaction. - # Catch and report an error if the annual billing doesn't happen - but don't rollback the service update. try: + dao_add_service_to_organisation(service, organisation_id) set_default_free_allowance_for_service(service, year_start=None) - except SQLAlchemyError: - # No need to worry about key errors because service.organisation_type has a foreign key to organisation_types - current_app.logger.exception( - f"Exception caught when trying to update annual billing when the organisation " - f"changed for service: {service.id} to organisation: {organisation_id}") + db.session.commit() + except SQLAlchemyError as e: + db.session.rollback() + raise e return '', 204 diff --git a/app/service/rest.py b/app/service/rest.py index 03d111be9..9a6996a22 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -7,6 +7,7 @@ from notifications_utils.timezones import convert_utc_to_bst from sqlalchemy.exc import IntegrityError, SQLAlchemyError from sqlalchemy.orm.exc import NoResultFound +from app import db from app.aws import s3 from app.config import QueueNames from app.dao import fact_notification_status_dao, notifications_dao @@ -254,18 +255,13 @@ def create_service(): # unpack valid json into service object valid_service = Service.from_json(data) - dao_create_service(valid_service, user) - - # Need to do the annual billing update in a separate transaction because the both the - # dao_add_service_to_organisation and set_default_free_allowance_for_service are wrapped in a transaction. - # Catch and report an error if the annual billing doesn't happen - but don't rollback the service update. try: - set_default_free_allowance_for_service(valid_service, year_start=None) - except SQLAlchemyError: - # No need to worry about key errors because service.organisation_type has a foreign key to organisation_types - current_app.logger.exception( - f"Exception caught when trying to insert annual billing creating a service {valid_service.id} " - f"for organisation_type {valid_service.organisation_type}") + dao_create_service(valid_service, user) + set_default_free_allowance_for_service(service=valid_service, year_start=None) + db.session.commit() + except SQLAlchemyError as e: + db.session.rollback() + raise e return jsonify(data=service_schema.dump(valid_service).data), 201 diff --git a/tests/app/dao/test_organisation_dao.py b/tests/app/dao/test_organisation_dao.py index 91136de8a..ed938ea61 100644 --- a/tests/app/dao/test_organisation_dao.py +++ b/tests/app/dao/test_organisation_dao.py @@ -233,7 +233,7 @@ def test_add_service_to_organisation(sample_service, sample_organisation): sample_organisation.crown = False dao_add_service_to_organisation(sample_service, sample_organisation.id) - + db.session.commit() assert len(sample_organisation.services) == 1 assert sample_organisation.services[0].id == sample_service.id diff --git a/tests/app/organisation/test_rest.py b/tests/app/organisation/test_rest.py index 5b2def4e4..2c3e3d0bb 100644 --- a/tests/app/organisation/test_rest.py +++ b/tests/app/organisation/test_rest.py @@ -505,14 +505,54 @@ def test_post_link_service_to_organisation(admin_request, sample_service): organisation_id=organisation.id, _expected_status=204 ) - assert len(organisation.services) == 1 assert sample_service.organisation_type == 'central' + + +def test_post_link_service_to_organisation_inserts_annual_billing(admin_request, sample_service): + data = { + 'service_id': str(sample_service.id) + } + organisation = create_organisation(organisation_type='central') + assert len(organisation.services) == 0 + assert len(AnnualBilling.query.all()) == 0 + admin_request.post( + 'organisation.link_service_to_organisation', + _data=data, + organisation_id=organisation.id, + _expected_status=204 + ) + annual_billing = AnnualBilling.query.all() assert len(annual_billing) == 1 assert annual_billing[0].free_sms_fragment_limit == 150000 +def test_post_link_service_to_organisation_rollback_service_if_annual_billing_update_fails( + admin_request, sample_service, mocker +): + mocker.patch('app.dao.annual_billing_dao.dao_create_or_update_annual_billing_for_year', + side_effect=SQLAlchemyError) + data = { + 'service_id': str(sample_service.id) + } + assert not sample_service.organisation_type + + organisation = create_organisation(organisation_type='central') + assert len(organisation.services) == 0 + assert len(AnnualBilling.query.all()) == 0 + with pytest.raises(expected_exception=SQLAlchemyError): + admin_request.post( + 'organisation.link_service_to_organisation', + _data=data, + organisation_id=organisation.id, + _expected_status=404 + ) + assert not sample_service.organisation_type + assert len(organisation.services) == 0 + assert len(AnnualBilling.query.all()) == 0 + + def test_post_link_service_to_another_org( admin_request, sample_service, sample_organisation): data = { @@ -582,23 +622,6 @@ def test_post_link_service_to_organisation_missing_payload( ) -def test_link_service_to_organisation_updates_service_if_annual_billing_update_fails( - mocker, admin_request, sample_service, sample_organisation -): - mocker.patch('app.organisation.rest.set_default_free_allowance_for_service', raises=SQLAlchemyError) - data = { - 'service_id': str(sample_service.id) - } - admin_request.post( - 'organisation.link_service_to_organisation', - organisation_id=str(sample_organisation.id), - _data=data, - _expected_status=204 - ) - assert sample_service.organisation_id == sample_organisation.id - assert len(AnnualBilling.query.all()) == 0 - - def test_rest_get_organisation_services( admin_request, sample_organisation, sample_service): dao_add_service_to_organisation(sample_service, sample_organisation.id) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 6e2599a45..7484d6203 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -503,10 +503,10 @@ def test_create_service_should_create_annual_billing_for_service( assert len(annual_billing) == 1 -def test_create_service_should_create_service_if_annual_billing_query_fails( +def test_create_service_should_raise_exception_and_not_create_service_if_annual_billing_query_fails( admin_request, sample_user, mocker ): - mocker.patch('app.service.rest.set_default_free_allowance_for_service', raises=SQLAlchemyError) + mocker.patch('app.service.rest.set_default_free_allowance_for_service', side_effect=SQLAlchemyError) data = { 'name': 'created service', 'user_id': str(sample_user.id), @@ -517,11 +517,12 @@ def test_create_service_should_create_service_if_annual_billing_query_fails( 'created_by': str(sample_user.id) } assert len(AnnualBilling.query.all()) == 0 - admin_request.post('service.create_service', _data=data, _expected_status=201) + with pytest.raises(expected_exception=SQLAlchemyError): + admin_request.post('service.create_service', _data=data) annual_billing = AnnualBilling.query.all() assert len(annual_billing) == 0 - assert len(Service.query.filter(Service.name == 'created service').all()) == 1 + assert len(Service.query.filter(Service.name == 'created service').all()) == 0 def test_create_service_inherits_branding_from_organisation( From 93908bacda1a3948e1a03e30038991d434ee31c5 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 13 Apr 2021 15:02:46 +0100 Subject: [PATCH 4/5] New strategy for transaction management. Introduce a contextmanger function to handle exceptions and nested transactions. Using the nested_transaction will start a nested transaction with `db.session.begin_nested`, once the nested transaction is complete the commit will happen. `@transactional` has been updated to commit unless in a nested transaction. --- app/billing/rest.py | 3 --- app/commands.py | 2 -- app/dao/annual_billing_dao.py | 4 ++-- app/dao/dao_utils.py | 31 +++++++++++++------------- app/dao/organisation_dao.py | 9 ++------ app/dao/services_dao.py | 9 ++------ app/organisation/rest.py | 10 +++------ app/service/rest.py | 11 +++------ tests/app/dao/test_organisation_dao.py | 2 +- 9 files changed, 29 insertions(+), 52 deletions(-) diff --git a/app/billing/rest.py b/app/billing/rest.py index 95c79798f..3f5f6e08c 100644 --- a/app/billing/rest.py +++ b/app/billing/rest.py @@ -1,6 +1,5 @@ from flask import Blueprint, jsonify, request -from app import db from app.billing.billing_schemas import ( create_or_update_free_sms_fragment_limit_schema, serialize_ft_billing_remove_emails, @@ -84,7 +83,6 @@ def get_free_sms_fragment_limit(service_id): annual_billing = dao_create_or_update_annual_billing_for_year(service_id, annual_billing.free_sms_fragment_limit, financial_year_start) - db.session.commit() return jsonify(annual_billing.serialize_free_sms_items()), 200 @@ -120,4 +118,3 @@ def update_free_sms_fragment_limit_data(service_id, free_sms_fragment_limit, fin free_sms_fragment_limit, financial_year_start ) - db.session.commit() diff --git a/app/commands.py b/app/commands.py index 12801b4a8..c98ce9bf3 100644 --- a/app/commands.py +++ b/app/commands.py @@ -880,7 +880,6 @@ def populate_annual_billing_with_the_previous_years_allowance(year): dao_create_or_update_annual_billing_for_year(service_id=row.id, free_sms_fragment_limit=free_allowance[0], financial_year_start=int(year)) - db.session.commit() @notify_command(name='populate-annual-billing-with-defaults') @@ -915,4 +914,3 @@ def populate_annual_billing_with_defaults(year, missing_services_only): for service in active_services: set_default_free_allowance_for_service(service, year) - db.session.commit() diff --git a/app/dao/annual_billing_dao.py b/app/dao/annual_billing_dao.py index 0ce4d0357..e1309b0c4 100644 --- a/app/dao/annual_billing_dao.py +++ b/app/dao/annual_billing_dao.py @@ -1,12 +1,12 @@ from flask import current_app from app import db -from app.dao.dao_utils import nested_transactional, transactional +from app.dao.dao_utils import transactional from app.dao.date_util import get_current_financial_year_start_year from app.models import AnnualBilling -@nested_transactional +@transactional def dao_create_or_update_annual_billing_for_year(service_id, free_sms_fragment_limit, financial_year_start): result = dao_get_free_sms_fragment_limit_for_year(service_id, financial_year_start) diff --git a/app/dao/dao_utils.py b/app/dao/dao_utils.py index 71c881bb3..0e9470b03 100644 --- a/app/dao/dao_utils.py +++ b/app/dao/dao_utils.py @@ -1,4 +1,5 @@ import itertools +from contextlib import contextmanager from functools import wraps from app import db @@ -10,7 +11,10 @@ def transactional(func): def commit_or_rollback(*args, **kwargs): try: res = func(*args, **kwargs) - db.session.commit() + + if not db.session.registry().transaction.nested: + db.session.commit() + return res except Exception: db.session.rollback() @@ -18,21 +22,18 @@ def transactional(func): return commit_or_rollback -def nested_transactional(func): - # This creates a save point for the nested transaction. - # You must manage the commit or rollback from outer most call of the nested of the transactions. - @wraps(func) - def commit_or_rollback(*args, **kwargs): - try: - db.session.begin_nested() - res = func(*args, **kwargs) - db.session.commit() - return res - except Exception: - db.session.rollback() - raise +@contextmanager +def nested_transaction(): + try: + db.session.begin_nested() + yield + db.session.commit() - return commit_or_rollback + if not db.session.registry().transaction.nested: + db.session.commit() + except Exception: + db.session.rollback() + raise class VersionOptions(): diff --git a/app/dao/organisation_dao.py b/app/dao/organisation_dao.py index e4b95d92f..877f8af30 100644 --- a/app/dao/organisation_dao.py +++ b/app/dao/organisation_dao.py @@ -1,12 +1,7 @@ from sqlalchemy.sql.expression import func from app import db -from app.dao.dao_utils import ( - VersionOptions, - nested_transactional, - transactional, - version_class, -) +from app.dao.dao_utils import VersionOptions, transactional, version_class from app.models import Domain, Organisation, Service, User @@ -110,7 +105,7 @@ def _update_organisation_services(organisation, attribute, only_where_none=True) db.session.add(service) -@nested_transactional +@transactional @version_class(Service) def dao_add_service_to_organisation(service, organisation_id): organisation = Organisation.query.filter_by( diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 18071f5fa..c68092c64 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -7,12 +7,7 @@ from sqlalchemy.orm import joinedload from sqlalchemy.sql.expression import and_, asc, case, func from app import db -from app.dao.dao_utils import ( - VersionOptions, - nested_transactional, - transactional, - version_class, -) +from app.dao.dao_utils import VersionOptions, transactional, version_class from app.dao.date_util import get_current_financial_year from app.dao.email_branding_dao import dao_get_email_branding_by_name from app.dao.letter_branding_dao import dao_get_letter_branding_by_name @@ -289,7 +284,7 @@ def dao_fetch_service_by_id_and_user(service_id, user_id): ).one() -@nested_transactional +@transactional @version_class(Service) def dao_create_service( service, diff --git a/app/organisation/rest.py b/app/organisation/rest.py index 06b4ea83c..ec8d6aa25 100644 --- a/app/organisation/rest.py +++ b/app/organisation/rest.py @@ -1,10 +1,10 @@ from flask import Blueprint, abort, current_app, jsonify, request -from sqlalchemy.exc import IntegrityError, SQLAlchemyError +from sqlalchemy.exc import IntegrityError -from app import db from app.config import QueueNames from app.dao.annual_billing_dao import set_default_free_allowance_for_service +from app.dao.dao_utils import nested_transaction from app.dao.fact_billing_dao import fetch_usage_year_for_organisation from app.dao.organisation_dao import ( dao_add_service_to_organisation, @@ -120,13 +120,9 @@ def link_service_to_organisation(organisation_id): service = dao_fetch_service_by_id(data['service_id']) service.organisation = None - try: + with nested_transaction(): dao_add_service_to_organisation(service, organisation_id) set_default_free_allowance_for_service(service, year_start=None) - db.session.commit() - except SQLAlchemyError as e: - db.session.rollback() - raise e return '', 204 diff --git a/app/service/rest.py b/app/service/rest.py index 9a6996a22..226ab3cc4 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -4,10 +4,9 @@ from datetime import datetime from flask import Blueprint, current_app, jsonify, request from notifications_utils.letter_timings import letter_can_be_cancelled from notifications_utils.timezones import convert_utc_to_bst -from sqlalchemy.exc import IntegrityError, SQLAlchemyError +from sqlalchemy.exc import IntegrityError from sqlalchemy.orm.exc import NoResultFound -from app import db from app.aws import s3 from app.config import QueueNames from app.dao import fact_notification_status_dao, notifications_dao @@ -19,7 +18,7 @@ from app.dao.api_key_dao import ( save_model_api_key, ) from app.dao.broadcast_service_dao import set_broadcast_service_type -from app.dao.dao_utils import dao_rollback +from app.dao.dao_utils import dao_rollback, nested_transaction from app.dao.date_util import get_financial_year from app.dao.fact_notification_status_dao import ( fetch_monthly_template_usage_for_service, @@ -255,13 +254,9 @@ def create_service(): # unpack valid json into service object valid_service = Service.from_json(data) - try: + with nested_transaction(): dao_create_service(valid_service, user) set_default_free_allowance_for_service(service=valid_service, year_start=None) - db.session.commit() - except SQLAlchemyError as e: - db.session.rollback() - raise e return jsonify(data=service_schema.dump(valid_service).data), 201 diff --git a/tests/app/dao/test_organisation_dao.py b/tests/app/dao/test_organisation_dao.py index ed938ea61..91136de8a 100644 --- a/tests/app/dao/test_organisation_dao.py +++ b/tests/app/dao/test_organisation_dao.py @@ -233,7 +233,7 @@ def test_add_service_to_organisation(sample_service, sample_organisation): sample_organisation.crown = False dao_add_service_to_organisation(sample_service, sample_organisation.id) - db.session.commit() + assert len(sample_organisation.services) == 1 assert sample_organisation.services[0].id == sample_service.id From d4009ffc52355bad20bd64c6d803024ac646c811 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 14 Apr 2021 07:11:01 +0100 Subject: [PATCH 5/5] Rename database management functions. Rename @transactional to @autocommit. Rename nested_transaction to tranaction. --- app/dao/annual_billing_dao.py | 6 +++--- app/dao/api_key_dao.py | 6 +++--- app/dao/broadcast_message_dao.py | 6 +++--- app/dao/broadcast_service_dao.py | 4 ++-- app/dao/complaint_dao.py | 4 ++-- app/dao/daily_sorted_letter_dao.py | 4 ++-- app/dao/dao_utils.py | 6 +++--- app/dao/email_branding_dao.py | 6 +++--- app/dao/fact_notification_status_dao.py | 4 ++-- app/dao/fact_processing_time_dao.py | 4 ++-- app/dao/inbound_numbers_dao.py | 8 ++++---- app/dao/inbound_sms_dao.py | 6 +++--- app/dao/jobs_dao.py | 4 ++-- app/dao/letter_branding_dao.py | 6 +++--- app/dao/notifications_dao.py | 16 ++++++++-------- app/dao/organisation_dao.py | 10 +++++----- app/dao/provider_details_dao.py | 8 ++++---- app/dao/provider_rates_dao.py | 4 ++-- app/dao/returned_letters_dao.py | 4 ++-- app/dao/service_callback_api_dao.py | 8 ++++---- app/dao/service_data_retention_dao.py | 6 +++--- app/dao/service_email_reply_to_dao.py | 8 ++++---- app/dao/service_inbound_api_dao.py | 8 ++++---- app/dao/service_letter_contact_dao.py | 8 ++++---- app/dao/service_permissions_dao.py | 4 ++-- app/dao/service_sms_sender_dao.py | 10 +++++----- app/dao/service_user_dao.py | 4 ++-- app/dao/services_dao.py | 12 ++++++------ app/dao/template_folder_dao.py | 8 ++++---- app/dao/templates_dao.py | 10 +++++----- app/dao/users_dao.py | 4 ++-- app/organisation/rest.py | 4 ++-- app/service/rest.py | 4 ++-- app/template_folder/rest.py | 4 ++-- tests/app/organisation/test_rest.py | 6 ++---- 35 files changed, 111 insertions(+), 113 deletions(-) diff --git a/app/dao/annual_billing_dao.py b/app/dao/annual_billing_dao.py index e1309b0c4..9497b0d75 100644 --- a/app/dao/annual_billing_dao.py +++ b/app/dao/annual_billing_dao.py @@ -1,12 +1,12 @@ from flask import current_app from app import db -from app.dao.dao_utils import transactional +from app.dao.dao_utils import autocommit from app.dao.date_util import get_current_financial_year_start_year from app.models import AnnualBilling -@transactional +@autocommit def dao_create_or_update_annual_billing_for_year(service_id, free_sms_fragment_limit, financial_year_start): result = dao_get_free_sms_fragment_limit_for_year(service_id, financial_year_start) @@ -25,7 +25,7 @@ def dao_get_annual_billing(service_id): ).order_by(AnnualBilling.financial_year_start).all() -@transactional +@autocommit def dao_update_annual_billing_for_future_years(service_id, free_sms_fragment_limit, financial_year_start): AnnualBilling.query.filter( AnnualBilling.service_id == service_id, diff --git a/app/dao/api_key_dao.py b/app/dao/api_key_dao.py index 9f82e2555..0830b4218 100644 --- a/app/dao/api_key_dao.py +++ b/app/dao/api_key_dao.py @@ -4,11 +4,11 @@ from datetime import datetime, timedelta from sqlalchemy import func, or_ from app import db -from app.dao.dao_utils import transactional, version_class +from app.dao.dao_utils import autocommit, version_class from app.models import ApiKey -@transactional +@autocommit @version_class(ApiKey) def save_model_api_key(api_key): if not api_key.id: @@ -17,7 +17,7 @@ def save_model_api_key(api_key): db.session.add(api_key) -@transactional +@autocommit @version_class(ApiKey) def expire_api_key(service_id, api_key_id): api_key = ApiKey.query.filter_by(id=api_key_id, service_id=service_id).one() diff --git a/app/dao/broadcast_message_dao.py b/app/dao/broadcast_message_dao.py index 9715c93d5..8adb9b681 100644 --- a/app/dao/broadcast_message_dao.py +++ b/app/dao/broadcast_message_dao.py @@ -1,7 +1,7 @@ import uuid from app import db -from app.dao.dao_utils import transactional +from app.dao.dao_utils import autocommit from app.models import ( BroadcastEvent, BroadcastMessage, @@ -43,7 +43,7 @@ def get_earlier_events_for_broadcast_event(broadcast_event_id): ).all() -@transactional +@autocommit def create_broadcast_provider_message(broadcast_event, provider): broadcast_provider_message_id = uuid.uuid4() provider_message = BroadcastProviderMessage( @@ -63,6 +63,6 @@ def create_broadcast_provider_message(broadcast_event, provider): return provider_message -@transactional +@autocommit def update_broadcast_provider_message_status(broadcast_provider_message, *, status): broadcast_provider_message.status = status diff --git a/app/dao/broadcast_service_dao.py b/app/dao/broadcast_service_dao.py index 27ac31273..6eb143feb 100644 --- a/app/dao/broadcast_service_dao.py +++ b/app/dao/broadcast_service_dao.py @@ -3,7 +3,7 @@ from datetime import datetime from flask import current_app from app import db -from app.dao.dao_utils import transactional, version_class +from app.dao.dao_utils import autocommit, version_class from app.models import ( BROADCAST_TYPE, EMAIL_AUTH_TYPE, @@ -14,7 +14,7 @@ from app.models import ( ) -@transactional +@autocommit @version_class(Service) def set_broadcast_service_type(service, service_mode, broadcast_channel, provider_restriction): insert_or_update_service_broadcast_settings( diff --git a/app/dao/complaint_dao.py b/app/dao/complaint_dao.py index e7840fe70..f6d5ac3bb 100644 --- a/app/dao/complaint_dao.py +++ b/app/dao/complaint_dao.py @@ -4,12 +4,12 @@ from flask import current_app from sqlalchemy import desc from app import db -from app.dao.dao_utils import transactional +from app.dao.dao_utils import autocommit from app.models import Complaint from app.utils import get_london_midnight_in_utc -@transactional +@autocommit def save_complaint(complaint): db.session.add(complaint) diff --git a/app/dao/daily_sorted_letter_dao.py b/app/dao/daily_sorted_letter_dao.py index eca7b3350..720eddf38 100644 --- a/app/dao/daily_sorted_letter_dao.py +++ b/app/dao/daily_sorted_letter_dao.py @@ -3,7 +3,7 @@ from datetime import datetime from sqlalchemy.dialects.postgresql import insert from app import db -from app.dao.dao_utils import transactional +from app.dao.dao_utils import autocommit from app.models import DailySortedLetter @@ -13,7 +13,7 @@ def dao_get_daily_sorted_letter_by_billing_day(billing_day): ).first() -@transactional +@autocommit def dao_create_or_update_daily_sorted_letter(new_daily_sorted_letter): ''' This uses the Postgres upsert to avoid race conditions when two threads try and insert diff --git a/app/dao/dao_utils.py b/app/dao/dao_utils.py index 0e9470b03..456f6eacc 100644 --- a/app/dao/dao_utils.py +++ b/app/dao/dao_utils.py @@ -6,7 +6,7 @@ from app import db from app.history_meta import create_history -def transactional(func): +def autocommit(func): @wraps(func) def commit_or_rollback(*args, **kwargs): try: @@ -23,7 +23,7 @@ def transactional(func): @contextmanager -def nested_transaction(): +def transaction(): try: db.session.begin_nested() yield @@ -93,7 +93,7 @@ def dao_rollback(): db.session.rollback() -@transactional +@autocommit def dao_save_object(obj): # add/update object in db db.session.add(obj) diff --git a/app/dao/email_branding_dao.py b/app/dao/email_branding_dao.py index d8738e920..1dedd78a8 100644 --- a/app/dao/email_branding_dao.py +++ b/app/dao/email_branding_dao.py @@ -1,5 +1,5 @@ from app import db -from app.dao.dao_utils import transactional +from app.dao.dao_utils import autocommit from app.models import EmailBranding @@ -15,12 +15,12 @@ def dao_get_email_branding_by_name(email_branding_name): return EmailBranding.query.filter_by(name=email_branding_name).first() -@transactional +@autocommit def dao_create_email_branding(email_branding): db.session.add(email_branding) -@transactional +@autocommit def dao_update_email_branding(email_branding, **kwargs): for key, value in kwargs.items(): setattr(email_branding, key, value or None) diff --git a/app/dao/fact_notification_status_dao.py b/app/dao/fact_notification_status_dao.py index fdbaa8e63..ae7d816a3 100644 --- a/app/dao/fact_notification_status_dao.py +++ b/app/dao/fact_notification_status_dao.py @@ -8,7 +8,7 @@ from sqlalchemy.sql.expression import extract, literal from sqlalchemy.types import DateTime, Integer from app import db -from app.dao.dao_utils import transactional +from app.dao.dao_utils import autocommit from app.models import ( KEY_TYPE_TEST, NOTIFICATION_CANCELLED, @@ -83,7 +83,7 @@ def query_for_fact_status_data(table, start_date, end_date, notification_type, s return query.all() -@transactional +@autocommit def update_fact_notification_status(data, process_day, notification_type): table = FactNotificationStatus.__table__ FactNotificationStatus.query.filter( diff --git a/app/dao/fact_processing_time_dao.py b/app/dao/fact_processing_time_dao.py index 6b879186c..c2559ac9b 100644 --- a/app/dao/fact_processing_time_dao.py +++ b/app/dao/fact_processing_time_dao.py @@ -3,11 +3,11 @@ from datetime import datetime from sqlalchemy.dialects.postgresql import insert from app import db -from app.dao.dao_utils import transactional +from app.dao.dao_utils import autocommit from app.models import FactProcessingTime -@transactional +@autocommit def insert_update_processing_time(processing_time): ''' This uses the Postgres upsert to avoid race conditions when two threads try and insert diff --git a/app/dao/inbound_numbers_dao.py b/app/dao/inbound_numbers_dao.py index afe24017f..b3d1a8cc2 100644 --- a/app/dao/inbound_numbers_dao.py +++ b/app/dao/inbound_numbers_dao.py @@ -1,5 +1,5 @@ from app import db -from app.dao.dao_utils import transactional +from app.dao.dao_utils import autocommit from app.models import InboundNumber @@ -19,13 +19,13 @@ def dao_get_inbound_number(inbound_number_id): return InboundNumber.query.filter(InboundNumber.id == inbound_number_id).first() -@transactional +@autocommit def dao_set_inbound_number_to_service(service_id, inbound_number): inbound_number.service_id = service_id db.session.add(inbound_number) -@transactional +@autocommit def dao_set_inbound_number_active_flag(service_id, active): inbound_number = InboundNumber.query.filter(InboundNumber.service_id == service_id).first() inbound_number.active = active @@ -33,7 +33,7 @@ def dao_set_inbound_number_active_flag(service_id, active): db.session.add(inbound_number) -@transactional +@autocommit def dao_allocate_number_for_service(service_id, inbound_number_id): updated = InboundNumber.query.filter_by( id=inbound_number_id, diff --git a/app/dao/inbound_sms_dao.py b/app/dao/inbound_sms_dao.py index 0add36249..303027e4c 100644 --- a/app/dao/inbound_sms_dao.py +++ b/app/dao/inbound_sms_dao.py @@ -4,7 +4,7 @@ from sqlalchemy.dialects.postgresql import insert from sqlalchemy.orm import aliased from app import db -from app.dao.dao_utils import transactional +from app.dao.dao_utils import autocommit from app.models import ( SMS_TYPE, InboundSms, @@ -15,7 +15,7 @@ from app.models import ( from app.utils import midnight_n_days_ago -@transactional +@autocommit def dao_create_inbound_sms(inbound_sms): db.session.add(inbound_sms) @@ -113,7 +113,7 @@ def _delete_inbound_sms(datetime_to_delete_from, query_filter): return deleted -@transactional +@autocommit def delete_inbound_sms_older_than_retention(): current_app.logger.info('Deleting inbound sms for services with flexible data retention') diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index ca549d21b..575f8a65b 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -9,7 +9,7 @@ from notifications_utils.letter_timings import ( from sqlalchemy import and_, asc, desc, func from app import db -from app.dao.dao_utils import transactional +from app.dao.dao_utils import autocommit from app.dao.templates_dao import dao_get_template_by_id from app.models import ( JOB_STATUS_CANCELLED, @@ -183,7 +183,7 @@ def dao_get_jobs_older_than_data_retention(notification_types): return jobs -@transactional +@autocommit def dao_cancel_letter_job(job): number_of_notifications_cancelled = Notification.query.filter( Notification.job_id == job.id diff --git a/app/dao/letter_branding_dao.py b/app/dao/letter_branding_dao.py index 4a140412c..b07106596 100644 --- a/app/dao/letter_branding_dao.py +++ b/app/dao/letter_branding_dao.py @@ -1,5 +1,5 @@ from app import db -from app.dao.dao_utils import transactional +from app.dao.dao_utils import autocommit from app.models import LetterBranding @@ -15,12 +15,12 @@ def dao_get_all_letter_branding(): return LetterBranding.query.order_by(LetterBranding.name).all() -@transactional +@autocommit def dao_create_letter_branding(letter_branding): db.session.add(letter_branding) -@transactional +@autocommit def dao_update_letter_branding(letter_branding_id, **kwargs): letter_branding = LetterBranding.query.get(letter_branding_id) for key, value in kwargs.items(): diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 6388eddca..f858ab6c4 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -25,7 +25,7 @@ from app import create_uuid, db from app.clients.sms.firetext import ( get_message_status_and_reason_from_firetext_code, ) -from app.dao.dao_utils import transactional +from app.dao.dao_utils import autocommit from app.letters.utils import LetterPDFNotFound, find_letter_pdf_in_s3 from app.models import ( EMAIL_TYPE, @@ -79,7 +79,7 @@ def dao_get_last_date_template_was_used(template_id, service_id): return last_date -@transactional +@autocommit def dao_create_notification(notification): if not notification.id: # need to populate defaulted fields before we create the notification history object @@ -123,7 +123,7 @@ def _update_notification_status(notification, status, detailed_status_code=None) return notification -@transactional +@autocommit def update_notification_status_by_id(notification_id, status, sent_by=None, detailed_status_code=None): notification = Notification.query.with_for_update().filter(Notification.id == notification_id).first() @@ -159,7 +159,7 @@ def update_notification_status_by_id(notification_id, status, sent_by=None, deta ) -@transactional +@autocommit def update_notification_status_by_reference(reference, status): # this is used to update letters and emails notification = Notification.query.filter(Notification.reference == reference).first() @@ -181,7 +181,7 @@ def update_notification_status_by_reference(reference, status): ) -@transactional +@autocommit def dao_update_notification(notification): notification.updated_at = datetime.utcnow() db.session.add(notification) @@ -339,7 +339,7 @@ def delete_notifications_older_than_retention_by_type(notification_type, qry_lim return deleted -@transactional +@autocommit def insert_notification_history_delete_notifications( notification_type, service_id, timestamp_to_delete_backwards_from, qry_limit=50000 ): @@ -459,7 +459,7 @@ def _delete_letters_from_s3( "Could not delete S3 object for letter: {}".format(letter.id)) -@transactional +@autocommit def dao_delete_notifications_by_id(notification_id): db.session.query(Notification).filter( Notification.id == notification_id @@ -569,7 +569,7 @@ def is_delivery_slow_for_providers( return slow_providers -@transactional +@autocommit def dao_update_notifications_by_reference(references, update_dict): updated_count = Notification.query.filter( Notification.reference.in_(references) diff --git a/app/dao/organisation_dao.py b/app/dao/organisation_dao.py index 877f8af30..33cc8d1d8 100644 --- a/app/dao/organisation_dao.py +++ b/app/dao/organisation_dao.py @@ -1,7 +1,7 @@ from sqlalchemy.sql.expression import func from app import db -from app.dao.dao_utils import VersionOptions, transactional, version_class +from app.dao.dao_utils import VersionOptions, autocommit, version_class from app.models import Domain, Organisation, Service, User @@ -55,12 +55,12 @@ def dao_get_organisation_by_service_id(service_id): return Organisation.query.join(Organisation.services).filter_by(id=service_id).first() -@transactional +@autocommit def dao_create_organisation(organisation): db.session.add(organisation) -@transactional +@autocommit def dao_update_organisation(organisation_id, **kwargs): domains = kwargs.pop('domains', None) @@ -105,7 +105,7 @@ def _update_organisation_services(organisation, attribute, only_where_none=True) db.session.add(service) -@transactional +@autocommit @version_class(Service) def dao_add_service_to_organisation(service, organisation_id): organisation = Organisation.query.filter_by( @@ -130,7 +130,7 @@ def dao_get_users_for_organisation(organisation_id): ).order_by(User.created_at).all() -@transactional +@autocommit def dao_add_user_to_organisation(organisation_id, user_id): organisation = dao_get_organisation_by_id(organisation_id) user = User.query.filter_by(id=user_id).one() diff --git a/app/dao/provider_details_dao.py b/app/dao/provider_details_dao.py index 697dfbd58..a3e3cf24c 100644 --- a/app/dao/provider_details_dao.py +++ b/app/dao/provider_details_dao.py @@ -5,7 +5,7 @@ from notifications_utils.timezones import convert_utc_to_bst from sqlalchemy import asc, desc, func from app import db -from app.dao.dao_utils import transactional +from app.dao.dao_utils import autocommit from app.models import ( SMS_TYPE, FactBilling, @@ -75,7 +75,7 @@ def _get_sms_providers_for_update(time_threshold): return q -@transactional +@autocommit def dao_reduce_sms_provider_priority(identifier, *, time_threshold): """ Will reduce a chosen sms provider's priority, and increase the other provider's priority by 10 points each. @@ -101,7 +101,7 @@ def dao_reduce_sms_provider_priority(identifier, *, time_threshold): _adjust_provider_priority(increased_provider, increased_provider_priority) -@transactional +@autocommit def dao_adjust_provider_priority_back_to_resting_points(): """ Provided that neither SMS provider has been modified in the last hour, move both providers by 10 percentage points @@ -135,7 +135,7 @@ def get_provider_details_by_notification_type(notification_type, supports_intern return ProviderDetails.query.filter(*filters).order_by(asc(ProviderDetails.priority)).all() -@transactional +@autocommit def dao_update_provider_details(provider_details): _update_provider_details_without_commit(provider_details) diff --git a/app/dao/provider_rates_dao.py b/app/dao/provider_rates_dao.py index d75b0e15b..e30097ce3 100644 --- a/app/dao/provider_rates_dao.py +++ b/app/dao/provider_rates_dao.py @@ -1,9 +1,9 @@ from app import db -from app.dao.dao_utils import transactional +from app.dao.dao_utils import autocommit from app.models import ProviderDetails, ProviderRates -@transactional +@autocommit def create_provider_rates(provider_identifier, valid_from, rate): provider = ProviderDetails.query.filter_by(identifier=provider_identifier).one() diff --git a/app/dao/returned_letters_dao.py b/app/dao/returned_letters_dao.py index a0353d0eb..d118aa3c1 100644 --- a/app/dao/returned_letters_dao.py +++ b/app/dao/returned_letters_dao.py @@ -4,7 +4,7 @@ from sqlalchemy import desc, func from sqlalchemy.dialects.postgresql import insert from app import db -from app.dao.dao_utils import transactional +from app.dao.dao_utils import autocommit from app.models import ( Job, Notification, @@ -28,7 +28,7 @@ def _get_notification_ids_for_references(references): return notification_ids + notification_history_ids -@transactional +@autocommit def insert_or_update_returned_letters(references): data = _get_notification_ids_for_references(references) for row in data: diff --git a/app/dao/service_callback_api_dao.py b/app/dao/service_callback_api_dao.py index d07934190..665d4a12a 100644 --- a/app/dao/service_callback_api_dao.py +++ b/app/dao/service_callback_api_dao.py @@ -1,7 +1,7 @@ from datetime import datetime from app import create_uuid, db -from app.dao.dao_utils import transactional, version_class +from app.dao.dao_utils import autocommit, version_class from app.models import ( COMPLAINT_CALLBACK_TYPE, DELIVERY_STATUS_CALLBACK_TYPE, @@ -9,7 +9,7 @@ from app.models import ( ) -@transactional +@autocommit @version_class(ServiceCallbackApi) def save_service_callback_api(service_callback_api): service_callback_api.id = create_uuid() @@ -17,7 +17,7 @@ def save_service_callback_api(service_callback_api): db.session.add(service_callback_api) -@transactional +@autocommit @version_class(ServiceCallbackApi) def reset_service_callback_api(service_callback_api, updated_by_id, url=None, bearer_token=None): if url: @@ -48,6 +48,6 @@ def get_service_complaint_callback_api_for_service(service_id): ).first() -@transactional +@autocommit def delete_service_callback_api(service_callback_api): db.session.delete(service_callback_api) diff --git a/app/dao/service_data_retention_dao.py b/app/dao/service_data_retention_dao.py index 2e2859302..cf6e7116d 100644 --- a/app/dao/service_data_retention_dao.py +++ b/app/dao/service_data_retention_dao.py @@ -1,7 +1,7 @@ from datetime import datetime from app import db -from app.dao.dao_utils import transactional +from app.dao.dao_utils import autocommit from app.models import ServiceDataRetention @@ -28,7 +28,7 @@ def fetch_service_data_retention_by_notification_type(service_id, notification_t return data_retention_list -@transactional +@autocommit def insert_service_data_retention(service_id, notification_type, days_of_retention): new_data_retention = ServiceDataRetention(service_id=service_id, notification_type=notification_type, @@ -38,7 +38,7 @@ def insert_service_data_retention(service_id, notification_type, days_of_retenti return new_data_retention -@transactional +@autocommit def update_service_data_retention(service_data_retention_id, service_id, days_of_retention): updated_count = ServiceDataRetention.query.filter( ServiceDataRetention.id == service_data_retention_id, diff --git a/app/dao/service_email_reply_to_dao.py b/app/dao/service_email_reply_to_dao.py index 86169e3c7..eb932fbf8 100644 --- a/app/dao/service_email_reply_to_dao.py +++ b/app/dao/service_email_reply_to_dao.py @@ -1,7 +1,7 @@ from sqlalchemy import desc from app import db -from app.dao.dao_utils import transactional +from app.dao.dao_utils import autocommit from app.errors import InvalidRequest from app.exceptions import ArchiveValidationError from app.models import ServiceEmailReplyTo @@ -28,7 +28,7 @@ def dao_get_reply_to_by_id(service_id, reply_to_id): return reply_to -@transactional +@autocommit def add_reply_to_email_address_for_service(service_id, email_address, is_default): old_default = _get_existing_default(service_id) if is_default: @@ -41,7 +41,7 @@ def add_reply_to_email_address_for_service(service_id, email_address, is_default return new_reply_to -@transactional +@autocommit def update_reply_to_email_address(service_id, reply_to_id, email_address, is_default): old_default = _get_existing_default(service_id) if is_default: @@ -57,7 +57,7 @@ def update_reply_to_email_address(service_id, reply_to_id, email_address, is_def return reply_to_update -@transactional +@autocommit def archive_reply_to_email_address(service_id, reply_to_id): reply_to_archive = ServiceEmailReplyTo.query.filter_by( id=reply_to_id, diff --git a/app/dao/service_inbound_api_dao.py b/app/dao/service_inbound_api_dao.py index 611eb214b..a099e3801 100644 --- a/app/dao/service_inbound_api_dao.py +++ b/app/dao/service_inbound_api_dao.py @@ -1,11 +1,11 @@ from datetime import datetime from app import create_uuid, db -from app.dao.dao_utils import transactional, version_class +from app.dao.dao_utils import autocommit, version_class from app.models import ServiceInboundApi -@transactional +@autocommit @version_class(ServiceInboundApi) def save_service_inbound_api(service_inbound_api): service_inbound_api.id = create_uuid() @@ -13,7 +13,7 @@ def save_service_inbound_api(service_inbound_api): db.session.add(service_inbound_api) -@transactional +@autocommit @version_class(ServiceInboundApi) def reset_service_inbound_api(service_inbound_api, updated_by_id, url=None, bearer_token=None): if url: @@ -35,6 +35,6 @@ def get_service_inbound_api_for_service(service_id): return ServiceInboundApi.query.filter_by(service_id=service_id).first() -@transactional +@autocommit def delete_service_inbound_api(service_inbound_api): db.session.delete(service_inbound_api) diff --git a/app/dao/service_letter_contact_dao.py b/app/dao/service_letter_contact_dao.py index d2c53bd7d..7ee409142 100644 --- a/app/dao/service_letter_contact_dao.py +++ b/app/dao/service_letter_contact_dao.py @@ -1,7 +1,7 @@ from sqlalchemy import desc from app import db -from app.dao.dao_utils import transactional +from app.dao.dao_utils import autocommit from app.models import ServiceLetterContact, Template @@ -30,7 +30,7 @@ def dao_get_letter_contact_by_id(service_id, letter_contact_id): return letter_contact -@transactional +@autocommit def add_letter_contact_for_service(service_id, contact_block, is_default): old_default = _get_existing_default(service_id) if is_default: @@ -45,7 +45,7 @@ def add_letter_contact_for_service(service_id, contact_block, is_default): return new_letter_contact -@transactional +@autocommit def update_letter_contact(service_id, letter_contact_id, contact_block, is_default): old_default = _get_existing_default(service_id) # if we want to make this the default, ensure there are no other existing defaults @@ -59,7 +59,7 @@ def update_letter_contact(service_id, letter_contact_id, contact_block, is_defau return letter_contact_update -@transactional +@autocommit def archive_letter_contact(service_id, letter_contact_id): letter_contact_to_archive = ServiceLetterContact.query.filter_by( id=letter_contact_id, diff --git a/app/dao/service_permissions_dao.py b/app/dao/service_permissions_dao.py index 4f0c5de22..bea2cec98 100644 --- a/app/dao/service_permissions_dao.py +++ b/app/dao/service_permissions_dao.py @@ -1,5 +1,5 @@ from app import db -from app.dao.dao_utils import transactional +from app.dao.dao_utils import autocommit from app.models import ServicePermission @@ -8,7 +8,7 @@ def dao_fetch_service_permissions(service_id): ServicePermission.service_id == service_id).all() -@transactional +@autocommit def dao_add_service_permission(service_id, permission): service_permission = ServicePermission(service_id=service_id, permission=permission) db.session.add(service_permission) diff --git a/app/dao/service_sms_sender_dao.py b/app/dao/service_sms_sender_dao.py index 30f9afa93..cf4a5f6fe 100644 --- a/app/dao/service_sms_sender_dao.py +++ b/app/dao/service_sms_sender_dao.py @@ -1,7 +1,7 @@ from sqlalchemy import desc from app import db -from app.dao.dao_utils import transactional +from app.dao.dao_utils import autocommit from app.exceptions import ArchiveValidationError from app.models import ServiceSmsSender @@ -32,7 +32,7 @@ def dao_get_sms_senders_by_service_id(service_id): ).order_by(desc(ServiceSmsSender.is_default)).all() -@transactional +@autocommit def dao_add_sms_sender_for_service(service_id, sms_sender, is_default, inbound_number_id=None): old_default = _get_existing_default(service_id=service_id) if is_default: @@ -51,7 +51,7 @@ def dao_add_sms_sender_for_service(service_id, sms_sender, is_default, inbound_n return new_sms_sender -@transactional +@autocommit def dao_update_service_sms_sender(service_id, service_sms_sender_id, is_default, sms_sender=None): old_default = _get_existing_default(service_id) if is_default: @@ -68,7 +68,7 @@ def dao_update_service_sms_sender(service_id, service_sms_sender_id, is_default, return sms_sender_to_update -@transactional +@autocommit def update_existing_sms_sender_with_inbound_number(service_sms_sender, sms_sender, inbound_number_id): service_sms_sender.sms_sender = sms_sender service_sms_sender.inbound_number_id = inbound_number_id @@ -76,7 +76,7 @@ def update_existing_sms_sender_with_inbound_number(service_sms_sender, sms_sende return service_sms_sender -@transactional +@autocommit def archive_sms_sender(service_id, sms_sender_id): sms_sender_to_archive = ServiceSmsSender.query.filter_by( id=sms_sender_id, diff --git a/app/dao/service_user_dao.py b/app/dao/service_user_dao.py index fafd8d7fb..e6685322e 100644 --- a/app/dao/service_user_dao.py +++ b/app/dao/service_user_dao.py @@ -1,6 +1,6 @@ from app import db -from app.dao.dao_utils import transactional +from app.dao.dao_utils import autocommit from app.models import ServiceUser, User @@ -21,6 +21,6 @@ def dao_get_service_users_by_user_id(user_id): return ServiceUser.query.filter_by(user_id=user_id).all() -@transactional +@autocommit def dao_update_service_user(service_user): db.session.add(service_user) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index c68092c64..3c7d42722 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -7,7 +7,7 @@ from sqlalchemy.orm import joinedload from sqlalchemy.sql.expression import and_, asc, case, func from app import db -from app.dao.dao_utils import VersionOptions, transactional, version_class +from app.dao.dao_utils import VersionOptions, autocommit, version_class from app.dao.date_util import get_current_financial_year from app.dao.email_branding_dao import dao_get_email_branding_by_name from app.dao.letter_branding_dao import dao_get_letter_branding_by_name @@ -247,7 +247,7 @@ def dao_fetch_all_services_created_by_user(user_id): return query.all() -@transactional +@autocommit @version_class( VersionOptions(ApiKey, must_write_history=False), VersionOptions(Service), @@ -284,7 +284,7 @@ def dao_fetch_service_by_id_and_user(service_id, user_id): ).one() -@transactional +@autocommit @version_class(Service) def dao_create_service( service, @@ -338,7 +338,7 @@ def dao_create_service( db.session.add(service) -@transactional +@autocommit @version_class(Service) def dao_update_service(service): db.session.add(service) @@ -507,7 +507,7 @@ def dao_fetch_todays_stats_for_all_services(include_from_test_key=True, only_act return query.all() -@transactional +@autocommit @version_class( VersionOptions(ApiKey, must_write_history=False), VersionOptions(Service), @@ -526,7 +526,7 @@ def dao_suspend_service(service_id): service.active = False -@transactional +@autocommit @version_class(Service) def dao_resume_service(service_id): service = Service.query.get(service_id) diff --git a/app/dao/template_folder_dao.py b/app/dao/template_folder_dao.py index 6264f9bcf..daa6e19ad 100644 --- a/app/dao/template_folder_dao.py +++ b/app/dao/template_folder_dao.py @@ -1,5 +1,5 @@ from app import db -from app.dao.dao_utils import transactional +from app.dao.dao_utils import autocommit from app.models import TemplateFolder @@ -14,16 +14,16 @@ def dao_get_valid_template_folders_by_id(folder_ids): return TemplateFolder.query.filter(TemplateFolder.id.in_(folder_ids)).all() -@transactional +@autocommit def dao_create_template_folder(template_folder): db.session.add(template_folder) -@transactional +@autocommit def dao_update_template_folder(template_folder): db.session.add(template_folder) -@transactional +@autocommit def dao_delete_template_folder(template_folder): db.session.delete(template_folder) diff --git a/app/dao/templates_dao.py b/app/dao/templates_dao.py index 45c030a1a..e28ffa3bf 100644 --- a/app/dao/templates_dao.py +++ b/app/dao/templates_dao.py @@ -5,7 +5,7 @@ from flask import current_app from sqlalchemy import asc, desc from app import db -from app.dao.dao_utils import VersionOptions, transactional, version_class +from app.dao.dao_utils import VersionOptions, autocommit, version_class from app.dao.users_dao import get_user_by_id from app.models import ( LETTER_TYPE, @@ -16,7 +16,7 @@ from app.models import ( ) -@transactional +@autocommit @version_class( VersionOptions(Template, history_class=TemplateHistory) ) @@ -38,7 +38,7 @@ def dao_create_template(template): db.session.add(template) -@transactional +@autocommit @version_class( VersionOptions(Template, history_class=TemplateHistory) ) @@ -49,7 +49,7 @@ def dao_update_template(template): db.session.add(template) -@transactional +@autocommit def dao_update_template_reply_to(template_id, reply_to): Template.query.filter_by(id=template_id).update( {"service_letter_contact_id": reply_to, @@ -81,7 +81,7 @@ def dao_update_template_reply_to(template_id, reply_to): return template -@transactional +@autocommit def dao_redact_template(template, user_id): template.template_redacted.redact_personalisation = True template.template_redacted.updated_at = datetime.utcnow() diff --git a/app/dao/users_dao.py b/app/dao/users_dao.py index 9734337b2..453c29ce4 100644 --- a/app/dao/users_dao.py +++ b/app/dao/users_dao.py @@ -6,7 +6,7 @@ from sqlalchemy import func from sqlalchemy.orm import joinedload from app import db -from app.dao.dao_utils import transactional +from app.dao.dao_utils import autocommit from app.dao.permissions_dao import permission_dao from app.dao.service_user_dao import dao_get_service_users_by_user_id from app.errors import InvalidRequest @@ -146,7 +146,7 @@ def get_user_and_accounts(user_id): ).one() -@transactional +@autocommit def dao_archive_user(user): if not user_can_be_archived(user): msg = "User can’t be removed from a service - check all services have another team member with manage_settings" diff --git a/app/organisation/rest.py b/app/organisation/rest.py index ec8d6aa25..697ec9825 100644 --- a/app/organisation/rest.py +++ b/app/organisation/rest.py @@ -4,7 +4,7 @@ from sqlalchemy.exc import IntegrityError from app.config import QueueNames from app.dao.annual_billing_dao import set_default_free_allowance_for_service -from app.dao.dao_utils import nested_transaction +from app.dao.dao_utils import transaction from app.dao.fact_billing_dao import fetch_usage_year_for_organisation from app.dao.organisation_dao import ( dao_add_service_to_organisation, @@ -120,7 +120,7 @@ def link_service_to_organisation(organisation_id): service = dao_fetch_service_by_id(data['service_id']) service.organisation = None - with nested_transaction(): + with transaction(): dao_add_service_to_organisation(service, organisation_id) set_default_free_allowance_for_service(service, year_start=None) diff --git a/app/service/rest.py b/app/service/rest.py index 226ab3cc4..303bc0715 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -18,7 +18,7 @@ from app.dao.api_key_dao import ( save_model_api_key, ) from app.dao.broadcast_service_dao import set_broadcast_service_type -from app.dao.dao_utils import dao_rollback, nested_transaction +from app.dao.dao_utils import dao_rollback, transaction from app.dao.date_util import get_financial_year from app.dao.fact_notification_status_dao import ( fetch_monthly_template_usage_for_service, @@ -254,7 +254,7 @@ def create_service(): # unpack valid json into service object valid_service = Service.from_json(data) - with nested_transaction(): + with transaction(): dao_create_service(valid_service, user) set_default_free_allowance_for_service(service=valid_service, year_start=None) diff --git a/app/template_folder/rest.py b/app/template_folder/rest.py index f25cd98a0..479e3a831 100644 --- a/app/template_folder/rest.py +++ b/app/template_folder/rest.py @@ -2,7 +2,7 @@ from flask import Blueprint, current_app, jsonify, request from sqlalchemy.exc import IntegrityError from sqlalchemy.orm.exc import NoResultFound -from app.dao.dao_utils import transactional +from app.dao.dao_utils import autocommit from app.dao.service_user_dao import ( dao_get_active_service_users, dao_get_service_user, @@ -104,7 +104,7 @@ def delete_template_folder(service_id, template_folder_id): @template_folder_blueprint.route('/contents', methods=['POST']) @template_folder_blueprint.route('//contents', methods=['POST']) -@transactional +@autocommit def move_to_template_folder(service_id, target_template_folder_id=None): data = request.get_json() diff --git a/tests/app/organisation/test_rest.py b/tests/app/organisation/test_rest.py index 2c3e3d0bb..81fe9c8b0 100644 --- a/tests/app/organisation/test_rest.py +++ b/tests/app/organisation/test_rest.py @@ -497,8 +497,7 @@ def test_post_link_service_to_organisation(admin_request, sample_service): 'service_id': str(sample_service.id) } organisation = create_organisation(organisation_type='central') - assert len(organisation.services) == 0 - assert len(AnnualBilling.query.all()) == 0 + admin_request.post( 'organisation.link_service_to_organisation', _data=data, @@ -545,8 +544,7 @@ def test_post_link_service_to_organisation_rollback_service_if_annual_billing_up admin_request.post( 'organisation.link_service_to_organisation', _data=data, - organisation_id=organisation.id, - _expected_status=404 + organisation_id=organisation.id ) assert not sample_service.organisation_type assert len(organisation.services) == 0