From 64f13a006412112830e1d7cc2ff71ea32cb4c1b6 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 24 Nov 2017 16:25:57 +0000 Subject: [PATCH 1/9] Add a new organisation for letter branding --- .../versions/0150_another_letter_org.py | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 migrations/versions/0150_another_letter_org.py diff --git a/migrations/versions/0150_another_letter_org.py b/migrations/versions/0150_another_letter_org.py new file mode 100644 index 000000000..ffbb91d9a --- /dev/null +++ b/migrations/versions/0150_another_letter_org.py @@ -0,0 +1,38 @@ +"""empty message + +Revision ID: 0150_another_letter_org +Revises: 0149_add_crown_to_services +Create Date: 2017-06-29 12:44:16.815039 + +""" + +# revision identifiers, used by Alembic. +revision = '0150_another_letter_org' +down_revision = '0149_add_crown_to_services' + +from alembic import op + + +NEW_ORGANISATIONS = [ + ('006', 'DWP (Welsh)'), + ('007', 'Department for Communities'), + ('008', 'Marine Management Organisation'), +] + + +def upgrade(): + for numeric_id, name in NEW_ORGANISATIONS: + op.execute(""" + INSERT + INTO dvla_organisation + VALUES ('{}', '{}') + """.format(numeric_id, name)) + + +def downgrade(): + for numeric_id, _ in NEW_ORGANISATIONS: + op.execute(""" + DELETE + FROM dvla_organisation + WHERE id = '{}' + """.format(numeric_id)) From 0d12b0666764ca37435a520300788544d61b9f52 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Tue, 5 Dec 2017 23:50:00 +0000 Subject: [PATCH 2/9] Update awscli from 1.14.2 to 1.14.4 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index d9699e50a..dab6dcc1f 100644 --- a/requirements.txt +++ b/requirements.txt @@ -23,7 +23,7 @@ statsd==3.2.1 notifications-python-client==4.6.0 # PaaS -awscli==1.14.2 +awscli==1.14.4 awscli-cwlogs>=1.4,<1.5 git+https://github.com/alphagov/notifications-utils.git@23.2.1#egg=notifications-utils==23.2.1 From b0d4044ff542c76d231ca3a668fe06372dbe3905 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 1 Dec 2017 16:31:21 +0000 Subject: [PATCH 3/9] remove free_sms_fragment_limit from service * remove from model * still required when calling POST /service - we just call through from dao_create_service to add a new annual billing entry. * removed from POST /service/ update_service - if you want to update/add a new one, use POST /service//free-sms-fragment-limit * made sure tests create services with default 250k limit. --- app/dao/annual_billing_dao.py | 4 +- app/dao/services_dao.py | 9 +-- app/models.py | 1 - app/schemas.py | 1 - app/service/rest.py | 18 ++--- tests/app/conftest.py | 12 ++- tests/app/db.py | 7 +- tests/app/service/test_rest.py | 131 ++++++++------------------------- 8 files changed, 58 insertions(+), 125 deletions(-) diff --git a/app/dao/annual_billing_dao.py b/app/dao/annual_billing_dao.py index 76a4e2f46..c3522ecbd 100644 --- a/app/dao/annual_billing_dao.py +++ b/app/dao/annual_billing_dao.py @@ -61,12 +61,12 @@ def dao_get_all_free_sms_fragment_limit(service_id): ).order_by(AnnualBilling.financial_year_start).all() -def dao_insert_annual_billing(service): +def dao_insert_annual_billing_for_this_year(service, free_sms_fragment_limit): """ This method is called from create_service which is wrapped in a transaction. """ annual_billing = AnnualBilling( - free_sms_fragment_limit=service.free_sms_fragment_limit, + free_sms_fragment_limit=free_sms_fragment_limit, financial_year_start=get_current_financial_year_start_year(), service=service, ) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 0b5f3e245..ebf0153c3 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -41,7 +41,7 @@ from app.models import ( ) from app.statsd_decorators import statsd from app.utils import get_london_month_from_utc_column, get_london_midnight_in_utc -from app.dao.annual_billing_dao import dao_insert_annual_billing +from app.dao.annual_billing_dao import dao_insert_annual_billing_for_this_year DEFAULT_SERVICE_PERMISSIONS = [ SMS_TYPE, @@ -155,7 +155,7 @@ def dao_fetch_service_by_id_and_user(service_id, user_id): @transactional @version_class(Service) -def dao_create_service(service, user, service_id=None, service_permissions=None): +def dao_create_service(service, user, free_sms_fragment_limit, service_id=None, service_permissions=None): # the default property does not appear to work when there is a difference between the sqlalchemy schema and the # db schema (ie: during a migration), so we have to set sms_sender manually here. After the GOVUK sms_sender # migration is completed, this code should be able to be removed. @@ -163,9 +163,6 @@ def dao_create_service(service, user, service_id=None, service_permissions=None) if service_permissions is None: service_permissions = DEFAULT_SERVICE_PERMISSIONS - if service.free_sms_fragment_limit is None: - service.free_sms_fragment_limit = current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] - from app.dao.permissions_dao import permission_dao service.users.append(user) permission_dao.add_default_service_permissions_for_user(user, service) @@ -180,7 +177,7 @@ def dao_create_service(service, user, service_id=None, service_permissions=None) # do we just add the default - or will we get a value from FE? insert_service_sms_sender(service, current_app.config['FROM_NUMBER']) - dao_insert_annual_billing(service) + dao_insert_annual_billing_for_this_year(service, free_sms_fragment_limit) db.session.add(service) diff --git a/app/models.py b/app/models.py index fdd00fa9a..498240b2c 100644 --- a/app/models.py +++ b/app/models.py @@ -228,7 +228,6 @@ class Service(db.Model, Versioned): created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False) prefix_sms = db.Column(db.Boolean, nullable=True, default=True) organisation_id = db.Column(UUID(as_uuid=True), db.ForeignKey('organisation.id'), index=True, nullable=True) - free_sms_fragment_limit = db.Column(db.BigInteger, index=False, unique=False, nullable=True) organisation = db.relationship('Organisation') dvla_organisation_id = db.Column( db.String, diff --git a/app/schemas.py b/app/schemas.py index 419c9b438..12ffa4da1 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -203,7 +203,6 @@ class ServiceSchema(BaseSchema): organisation_type = field_for(models.Service, 'organisation_type') branding = field_for(models.Service, 'branding') dvla_organisation = field_for(models.Service, 'dvla_organisation') - free_sms_fragment_limit = field_for(models.Service, 'free_sms_fragment_limit') permissions = fields.Method("service_permissions") override_flag = False reply_to_email_address = fields.Method(method_name="get_reply_to_email_address") diff --git a/app/service/rest.py b/app/service/rest.py index c4d0e2bb9..f9ea00e38 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -87,7 +87,6 @@ from app.schemas import ( detailed_service_schema ) from app.utils import pagination_links -from app.billing.rest import update_free_sms_fragment_limit_data service_blueprint = Blueprint('service', __name__) @@ -154,19 +153,24 @@ def get_service_by_id(service_id): @service_blueprint.route('', methods=['POST']) def create_service(): data = request.get_json() - if not data.get('user_id', None): - errors = {'user_id': ['Missing data for required field.']} + errors = { + required_field: ['Missing data for required field.'] + for required_field in ['user_id', 'free_sms_fragment_limit'] + if not data.get(required_field, None) + } + if errors: raise InvalidRequest(errors, status_code=400) # validate json with marshmallow - service_schema.load(request.get_json()) + service_schema.load(data) user = get_user_by_id(data.pop('user_id', None)) + free_sms_fragment_limit = data.pop('free_sms_fragment_limit') # unpack valid json into service object valid_service = Service.from_json(data) - dao_create_service(valid_service, user) + dao_create_service(valid_service, user, free_sms_fragment_limit) return jsonify(data=service_schema.dump(valid_service).data), 201 @@ -185,10 +189,6 @@ def update_service(service_id): update_dict.crown = org_type == 'central' dao_update_service(update_dict) - # bridging code between frontend is deployed and data has not been migrated yet. Can only update current year - if 'free_sms_fragment_limit' in req_json: - update_free_sms_fragment_limit_data(fetched_service.id, req_json['free_sms_fragment_limit']) - if service_going_live: send_notification_to_service_users( service_id=service_id, diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 79a56ed10..6b678c16f 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -162,13 +162,12 @@ def sample_service( 'message_limit': limit, 'restricted': restricted, 'email_from': email_from, - 'created_by': user, - 'free_sms_fragment_limit': free_sms_fragment_limit + 'created_by': user } service = Service.query.filter_by(name=service_name).first() if not service: service = Service(**data) - dao_create_service(service, user, service_permissions=permissions) + dao_create_service(service, user, free_sms_fragment_limit, service_permissions=permissions) if research_mode: service.research_mode = research_mode @@ -1002,7 +1001,12 @@ def notify_service(notify_db, notify_db_session): created_by=user, prefix_sms=False, ) - dao_create_service(service=service, service_id=current_app.config['NOTIFY_SERVICE_ID'], user=user) + dao_create_service( + service=service, + service_id=current_app.config['NOTIFY_SERVICE_ID'], + user=user, + free_sms_fragment_limit=250000 # live central gov service + ) data = { 'service': service, diff --git a/tests/app/db.py b/tests/app/db.py index 717b1aa2b..16471d555 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -79,7 +79,12 @@ def create_service( prefix_sms=prefix_sms, ) - dao_create_service(service, service.created_by, service_id, service_permissions=service_permissions) + dao_create_service( + service, + service.created_by, + free_sms_fragment_limit=250000, + service_id=service_id, + service_permissions=service_permissions) service.active = active service.research_mode = research_mode diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index cc8f13eaa..90f82da5d 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -38,7 +38,6 @@ from tests.app.db import ( create_service_with_defined_sms_sender ) from tests.app.db import create_user -from app.dao.date_util import get_current_financial_year_start_year def test_get_service_list(client, service_factory): @@ -133,16 +132,8 @@ def test_get_service_by_id(admin_request, sample_service): assert json_resp['data']['dvla_organisation'] == '001' assert json_resp['data']['sms_sender'] == current_app.config['FROM_NUMBER'] assert json_resp['data']['prefix_sms'] is True - - -def test_get_service_by_id_returns_free_sms_limit(admin_request, sample_service): - json_resp = admin_request.get('service.get_service_by_id', service_id=sample_service.id) - assert json_resp['data']['free_sms_fragment_limit'] == current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] - - -def test_get_detailed_service_by_id_returns_free_sms_limit(admin_request, sample_service): - json_resp = admin_request.get('service.get_service_by_id', service_id=sample_service.id, detailed=True) - assert json_resp['data']['free_sms_fragment_limit'] == current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] + # deprecated field, no longer exists + assert 'free_sms_fragment_limit' not in json_resp['data'] @pytest.mark.parametrize('detailed', [True, False]) @@ -228,7 +219,9 @@ def test_create_service(client, sample_user): 'restricted': False, 'active': False, 'email_from': 'created.service', - 'created_by': str(sample_user.id)} + 'created_by': str(sample_user.id), + 'free_sms_fragment_limit': 250000 + } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] resp = client.post( @@ -243,8 +236,6 @@ def test_create_service(client, sample_user): assert not json_resp['data']['research_mode'] assert json_resp['data']['dvla_organisation'] == '001' assert json_resp['data']['sms_sender'] == current_app.config['FROM_NUMBER'] - # TODO: Remove this after the new data is used - assert json_resp['data']['free_sms_fragment_limit'] == current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] service_db = Service.query.get(json_resp['data']['id']) assert service_db.name == 'created service' @@ -274,7 +265,8 @@ def test_should_not_create_service_with_missing_user_id_field(notify_api, fake_u 'message_limit': 1000, 'restricted': False, 'active': False, - 'created_by': str(fake_uuid) + 'created_by': str(fake_uuid), + 'free_sms_fragment_limit': 250000 } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] @@ -288,7 +280,7 @@ def test_should_not_create_service_with_missing_user_id_field(notify_api, fake_u assert 'Missing data for required field.' in json_resp['message']['user_id'] -def test_create_service_free_sms_fragment_limit_is_optional(client, sample_user): +def test_create_service_free_sms_fragment_limit_is_not_optional(admin_request, sample_user): data1 = { 'name': 'service 1', 'user_id': str(sample_user.id), @@ -296,56 +288,16 @@ def test_create_service_free_sms_fragment_limit_is_optional(client, sample_user) 'restricted': False, 'active': False, 'email_from': 'sample_user.email1', - 'created_by': str(sample_user.id), - 'free_sms_fragment_limit': 9999 + 'created_by': str(sample_user.id) } - auth_header = create_authorization_header() - headers = [('Content-Type', 'application/json'), auth_header] - resp = client.post( - '/service', - data=json.dumps(data1), - headers=headers) - json_resp = json.loads(resp.get_data(as_text=True)) - assert resp.status_code == 201 - - # Test data from the new annual billing table - service_id = json_resp['data']['id'] - annual_billing = client.get('service/{}/billing/free-sms-fragment-limit?financial_year_start={}' - .format(service_id, get_current_financial_year_start_year()), - headers=[('Content-Type', 'application/json'), create_authorization_header()]) - json_resp = json.loads(annual_billing.get_data(as_text=True)) - assert json_resp['free_sms_fragment_limit'] == 9999 - # TODO: Remove this after the new data is used - assert json_resp['free_sms_fragment_limit'] == 9999 - - data2 = { - 'name': 'service 2', - 'user_id': str(sample_user.id), - 'message_limit': 1000, - 'restricted': False, - 'active': False, - 'email_from': 'sample_user.email2', - 'created_by': str(sample_user.id), - } - - auth_header = create_authorization_header() - headers = [('Content-Type', 'application/json'), auth_header] - resp = client.post( - '/service', - data=json.dumps(data2), - headers=headers) - json_resp = json.loads(resp.get_data(as_text=True)) - assert resp.status_code == 201 - # Test data from the new annual billing table - service_id = json_resp['data']['id'] - annual_billing = client.get('service/{}/billing/free-sms-fragment-limit?financial_year_start={}' - .format(service_id, get_current_financial_year_start_year()), - headers=[('Content-Type', 'application/json'), create_authorization_header()]) - json_resp = json.loads(annual_billing.get_data(as_text=True)) - assert json_resp['free_sms_fragment_limit'] == current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] - # TODO: Remove this after the new data is used - assert json_resp['free_sms_fragment_limit'] == current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] + json_resp = admin_request.post( + 'service.create_service', + _data=data1, + _expected_status=400 + ) + assert json_resp['result'] == 'error' + assert 'Missing data for required field.' in json_resp['message']['free_sms_fragment_limit'] def test_should_error_if_created_by_missing(notify_api, sample_user): @@ -357,7 +309,8 @@ def test_should_error_if_created_by_missing(notify_api, sample_user): 'message_limit': 1000, 'restricted': False, 'active': False, - 'user_id': str(sample_user.id) + 'user_id': str(sample_user.id), + 'free_sms_fragment_limit': 250000 } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] @@ -384,7 +337,8 @@ def test_should_not_create_service_with_missing_if_user_id_is_not_in_database(no 'message_limit': 1000, 'restricted': False, 'active': False, - 'created_by': str(fake_uuid) + 'created_by': str(fake_uuid), + 'free_sms_fragment_limit': 250000 } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] @@ -402,7 +356,8 @@ def test_should_not_create_service_if_missing_data(notify_api, sample_user): with notify_api.test_request_context(): with notify_api.test_client() as client: data = { - 'user_id': str(sample_user.id) + 'user_id': str(sample_user.id), + 'free_sms_fragment_limit': 250000 } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] @@ -430,7 +385,9 @@ def test_should_not_create_service_with_duplicate_name(notify_api, 'restricted': False, 'active': False, 'email_from': 'sample.service2', - 'created_by': str(sample_user.id)} + 'created_by': str(sample_user.id), + 'free_sms_fragment_limit': 250000 + } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] resp = client.post( @@ -456,7 +413,9 @@ def test_create_service_should_throw_duplicate_key_constraint_for_existing_email 'restricted': False, 'active': False, 'email_from': 'first.service', - 'created_by': str(sample_user.id)} + 'created_by': str(sample_user.id), + 'free_sms_fragment_limit': 250000 + } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] resp = client.post( @@ -603,37 +562,6 @@ def test_update_service_flags_will_remove_service_permissions(client, notify_db, assert set([p.permission for p in permissions]) == set([SMS_TYPE, EMAIL_TYPE]) -# TODO: Remove after new table is created and verified -def test_update_service_free_sms_fragment_limit(client, notify_db, sample_service): - org = Organisation(colour='#000000', logo='justice-league.png', name='Justice League') - notify_db.session.add(org) - notify_db.session.commit() - - auth_header = create_authorization_header() - resp = client.get( - '/service/{}'.format(sample_service.id), - headers=[auth_header] - ) - json_resp = json.loads(resp.get_data(as_text=True)) - assert resp.status_code == 200 - assert json_resp['data']['free_sms_fragment_limit'] == current_app.config['FREE_SMS_TIER_FRAGMENT_COUNT'] - - data = { - 'free_sms_fragment_limit': 9999 - } - - auth_header = create_authorization_header() - - resp = client.post( - '/service/{}'.format(sample_service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header] - ) - result = json.loads(resp.get_data(as_text=True)) - assert resp.status_code == 200 - assert result['data']['free_sms_fragment_limit'] == 9999 - - def test_update_permissions_will_override_permission_flags(client, service_with_no_permissions): auth_header = create_authorization_header() @@ -916,7 +844,8 @@ def test_default_permissions_are_added_for_user_service(notify_api, 'restricted': False, 'active': False, 'email_from': 'created.service', - 'created_by': str(sample_user.id)} + 'created_by': str(sample_user.id), + 'free_sms_fragment_limit': 250000} auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] resp = client.post( From 664187f878267ca5e1097d0de8d501e502c59f9d Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 1 Dec 2017 16:51:09 +0000 Subject: [PATCH 4/9] update model to reflect service.prefix_sms not being nullable --- app/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models.py b/app/models.py index 498240b2c..6eeda9a77 100644 --- a/app/models.py +++ b/app/models.py @@ -226,7 +226,7 @@ class Service(db.Model, Versioned): email_from = db.Column(db.Text, index=False, unique=True, nullable=False) created_by = db.relationship('User') created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False) - prefix_sms = db.Column(db.Boolean, nullable=True, default=True) + prefix_sms = db.Column(db.Boolean, nullable=False, default=True) organisation_id = db.Column(UUID(as_uuid=True), db.ForeignKey('organisation.id'), index=True, nullable=True) organisation = db.relationship('Organisation') dvla_organisation_id = db.Column( From a4e2a40134c721f75a1de02b0228a19c3382d008 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 1 Dec 2017 17:10:25 +0000 Subject: [PATCH 5/9] update dao tests to pass in free sms fragment limit, remove command --- app/commands.py | 26 --------------------- app/schemas.py | 1 - tests/app/dao/test_services_dao.py | 36 +++++++++++++++++------------- tests/app/service/test_rest.py | 5 +++-- 4 files changed, 23 insertions(+), 45 deletions(-) diff --git a/app/commands.py b/app/commands.py index 4b8f2b37f..ee92f4c2b 100644 --- a/app/commands.py +++ b/app/commands.py @@ -335,32 +335,6 @@ def populate_service_letter_contact(): print("Populated letter contacts for {} services".format(result.rowcount)) -@notify_command() -def populate_service_and_service_history_free_sms_fragment_limit(): - """ - DEPRECATED. Set services to have 250k sms limit. - """ - services_to_update = """ - UPDATE services - SET free_sms_fragment_limit = 250000 - WHERE free_sms_fragment_limit IS NULL - """ - - services_history_to_update = """ - UPDATE services_history - SET free_sms_fragment_limit = 250000 - WHERE free_sms_fragment_limit IS NULL - """ - - services_result = db.session.execute(services_to_update) - services_history_result = db.session.execute(services_history_to_update) - - db.session.commit() - - print("Populated free sms fragment limits for {} services".format(services_result.rowcount)) - print("Populated free sms fragment limits for {} services history".format(services_history_result.rowcount)) - - @notify_command() def populate_annual_billing(): """ diff --git a/app/schemas.py b/app/schemas.py index 12ffa4da1..2afa6270d 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -290,7 +290,6 @@ class DetailedServiceSchema(BaseSchema): 'letter_contact_block', # new exclude from here 'message_limit', 'email_from', - # 'free_sms_fragment_limit', 'inbound_api', 'dvla_organisation', 'whitelist', diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 2caf092fe..abeb06c32 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -51,6 +51,7 @@ from app.models import ( Service, ServicePermission, ServicePermissionTypes, + AnnualBilling, BRANDING_GOVUK, DVLA_ORG_HM_GOVERNMENT, KEY_TYPE_NORMAL, @@ -89,10 +90,10 @@ def test_create_service(sample_user): restricted=False, organisation_type='central', created_by=sample_user) - dao_create_service(service, sample_user) + dao_create_service(service, sample_user, 12345) assert Service.query.count() == 1 - service_db = Service.query.first() + service_db = Service.query.one() assert service_db.name == "service_name" assert service_db.id == service.id assert service_db.branding == BRANDING_GOVUK @@ -105,6 +106,9 @@ def test_create_service(sample_user): assert service_db.free_sms_fragment_limit == 250000 assert service_db.organisation_type == 'central' assert service_db.crown is True + annual_billing = AnnualBilling.query.one() + assert annual_billing.service == service_db + assert annual_billing.free_sms_fragment_limit == 12345 def test_cannot_create_two_services_with_same_name(sample_user): @@ -121,8 +125,8 @@ def test_cannot_create_two_services_with_same_name(sample_user): restricted=False, created_by=sample_user) with pytest.raises(IntegrityError) as excinfo: - dao_create_service(service1, sample_user) - dao_create_service(service2, sample_user) + dao_create_service(service1, sample_user, 12345) + dao_create_service(service2, sample_user, 12345) assert 'duplicate key value violates unique constraint "services_name_key"' in str(excinfo.value) @@ -139,8 +143,8 @@ def test_cannot_create_two_services_with_same_email_from(sample_user): restricted=False, created_by=sample_user) with pytest.raises(IntegrityError) as excinfo: - dao_create_service(service1, sample_user) - dao_create_service(service2, sample_user) + dao_create_service(service1, sample_user, 12345) + dao_create_service(service2, sample_user, 12345) assert 'duplicate key value violates unique constraint "services_email_from_key"' in str(excinfo.value) @@ -152,7 +156,7 @@ def test_cannot_create_service_with_no_user(notify_db_session, sample_user): restricted=False, created_by=sample_user) with pytest.raises(FlushError) as excinfo: - dao_create_service(service, None) + dao_create_service(service, None, 12345) assert "Can't flush None value found in collection Service.users" in str(excinfo.value) @@ -162,7 +166,7 @@ def test_should_add_user_to_service(sample_user): message_limit=1000, restricted=False, created_by=sample_user) - dao_create_service(service, sample_user) + dao_create_service(service, sample_user, 12345) assert sample_user in Service.query.first().users new_user = User( name='Test User', @@ -181,7 +185,7 @@ def test_should_remove_user_from_service(sample_user): message_limit=1000, restricted=False, created_by=sample_user) - dao_create_service(service, sample_user) + dao_create_service(service, sample_user, 12345) new_user = User( name='Test User', email_address='new_user@digital.cabinet-office.gov.uk', @@ -333,7 +337,7 @@ def test_create_service_creates_a_history_record_with_current_data(sample_user): message_limit=1000, restricted=False, created_by=sample_user) - dao_create_service(service, sample_user) + dao_create_service(service, sample_user, 12345) assert Service.query.count() == 1 assert Service.get_history_model().query.count() == 1 @@ -360,7 +364,7 @@ def test_update_service_creates_a_history_record_with_current_data(sample_user): message_limit=1000, restricted=False, created_by=sample_user) - dao_create_service(service, sample_user) + dao_create_service(service, sample_user, 12345) assert Service.query.count() == 1 assert Service.query.first().version == 1 @@ -388,7 +392,7 @@ def test_update_service_permission_creates_a_history_record_with_current_data(sa message_limit=1000, restricted=False, created_by=sample_user) - dao_create_service(service, sample_user) + dao_create_service(service, sample_user, 12345) service.permissions.append(ServicePermission(service_id=service.id, permission='letter')) dao_update_service(service) @@ -431,7 +435,7 @@ def test_create_service_and_history_is_transactional(sample_user): created_by=sample_user) with pytest.raises(IntegrityError) as excinfo: - dao_create_service(service, sample_user) + dao_create_service(service, sample_user, 12345) assert 'column "name" violates not-null constraint' in str(excinfo.value) assert Service.query.count() == 0 @@ -480,7 +484,7 @@ def test_add_existing_user_to_another_service_doesnot_change_old_permissions(sam restricted=False, created_by=sample_user) - dao_create_service(service_one, sample_user) + dao_create_service(service_one, sample_user, 12345) assert sample_user.id == service_one.users[0].id test_user_permissions = Permission.query.filter_by(service=service_one, user=sample_user).all() assert len(test_user_permissions) == 8 @@ -497,7 +501,7 @@ def test_add_existing_user_to_another_service_doesnot_change_old_permissions(sam message_limit=1000, restricted=False, created_by=other_user) - dao_create_service(service_two, other_user) + dao_create_service(service_two, other_user, 12345) assert other_user.id == service_two.users[0].id other_user_permissions = Permission.query.filter_by(service=service_two, user=other_user).all() @@ -526,7 +530,7 @@ def test_fetch_stats_filters_on_service(sample_notification): email_from="hello", restricted=False, message_limit=1000) - dao_create_service(service_two, sample_notification.service.created_by) + dao_create_service(service_two, sample_notification.service.created_by, 12345) stats = dao_fetch_stats_for_service(service_two.id) assert len(stats) == 0 diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 90f82da5d..765e64e2f 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1432,12 +1432,13 @@ def test_set_sms_prefixing_for_service_cant_be_none( admin_request, sample_service, ): - admin_request.post( + resp = admin_request.post( 'service.update_service', service_id=sample_service.id, _data={'prefix_sms': None}, - _expected_status=500, + _expected_status=400, ) + assert resp['message'] == {'prefix_sms': ['Field may not be null.']} @pytest.mark.parametrize('today_only,stats', [ From 8cd422ebeade247d2674486d0d9b9e88e603c01c Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 4 Dec 2017 11:17:23 +0000 Subject: [PATCH 6/9] remove unnecessary None from .get --- app/service/rest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/service/rest.py b/app/service/rest.py index f9ea00e38..a5cd8742a 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -156,7 +156,7 @@ def create_service(): errors = { required_field: ['Missing data for required field.'] for required_field in ['user_id', 'free_sms_fragment_limit'] - if not data.get(required_field, None) + if not data.get(required_field) } if errors: raise InvalidRequest(errors, status_code=400) From 4cdcc4e0355d760a4d787b2eea3a0273e2fb14e6 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 6 Dec 2017 11:01:18 +0000 Subject: [PATCH 7/9] no longer add an annual billing entry creating a service this is now handled by a separate call from the admin app --- app/dao/services_dao.py | 4 +-- app/service/rest.py | 14 ++++----- tests/app/conftest.py | 6 ++-- tests/app/dao/test_services_dao.py | 35 ++++++++++------------ tests/app/db.py | 7 +---- tests/app/service/test_rest.py | 47 ++++++------------------------ 6 files changed, 33 insertions(+), 80 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index ebf0153c3..6985a26e8 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -41,7 +41,6 @@ from app.models import ( ) from app.statsd_decorators import statsd from app.utils import get_london_month_from_utc_column, get_london_midnight_in_utc -from app.dao.annual_billing_dao import dao_insert_annual_billing_for_this_year DEFAULT_SERVICE_PERMISSIONS = [ SMS_TYPE, @@ -155,7 +154,7 @@ def dao_fetch_service_by_id_and_user(service_id, user_id): @transactional @version_class(Service) -def dao_create_service(service, user, free_sms_fragment_limit, service_id=None, service_permissions=None): +def dao_create_service(service, user, service_id=None, service_permissions=None): # the default property does not appear to work when there is a difference between the sqlalchemy schema and the # db schema (ie: during a migration), so we have to set sms_sender manually here. After the GOVUK sms_sender # migration is completed, this code should be able to be removed. @@ -177,7 +176,6 @@ def dao_create_service(service, user, free_sms_fragment_limit, service_id=None, # do we just add the default - or will we get a value from FE? insert_service_sms_sender(service, current_app.config['FROM_NUMBER']) - dao_insert_annual_billing_for_this_year(service, free_sms_fragment_limit) db.session.add(service) diff --git a/app/service/rest.py b/app/service/rest.py index a5cd8742a..d71cd2b90 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -153,24 +153,20 @@ def get_service_by_id(service_id): @service_blueprint.route('', methods=['POST']) def create_service(): data = request.get_json() - errors = { - required_field: ['Missing data for required field.'] - for required_field in ['user_id', 'free_sms_fragment_limit'] - if not data.get(required_field) - } - if errors: + + if not data.get('user_id'): + errors = {'user_id': ['Missing data for required field.']} raise InvalidRequest(errors, status_code=400) # validate json with marshmallow service_schema.load(data) - user = get_user_by_id(data.pop('user_id', None)) - free_sms_fragment_limit = data.pop('free_sms_fragment_limit') + user = get_user_by_id(data.pop('user_id')) # unpack valid json into service object valid_service = Service.from_json(data) - dao_create_service(valid_service, user, free_sms_fragment_limit) + dao_create_service(valid_service, user) return jsonify(data=service_schema.dump(valid_service).data), 201 diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 6b678c16f..71b510767 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -150,7 +150,6 @@ def sample_service( email_from=None, permissions=None, research_mode=None, - free_sms_fragment_limit=250000 ): if user is None: user = create_user() @@ -167,7 +166,7 @@ def sample_service( service = Service.query.filter_by(name=service_name).first() if not service: service = Service(**data) - dao_create_service(service, user, free_sms_fragment_limit, service_permissions=permissions) + dao_create_service(service, user, service_permissions=permissions) if research_mode: service.research_mode = research_mode @@ -1004,8 +1003,7 @@ def notify_service(notify_db, notify_db_session): dao_create_service( service=service, service_id=current_app.config['NOTIFY_SERVICE_ID'], - user=user, - free_sms_fragment_limit=250000 # live central gov service + user=user ) data = { diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index abeb06c32..32f16f27a 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -51,7 +51,6 @@ from app.models import ( Service, ServicePermission, ServicePermissionTypes, - AnnualBilling, BRANDING_GOVUK, DVLA_ORG_HM_GOVERNMENT, KEY_TYPE_NORMAL, @@ -90,7 +89,7 @@ def test_create_service(sample_user): restricted=False, organisation_type='central', created_by=sample_user) - dao_create_service(service, sample_user, 12345) + dao_create_service(service, sample_user) assert Service.query.count() == 1 service_db = Service.query.one() @@ -103,12 +102,8 @@ def test_create_service(sample_user): assert service_db.prefix_sms is True assert service.active is True assert sample_user in service_db.users - assert service_db.free_sms_fragment_limit == 250000 assert service_db.organisation_type == 'central' assert service_db.crown is True - annual_billing = AnnualBilling.query.one() - assert annual_billing.service == service_db - assert annual_billing.free_sms_fragment_limit == 12345 def test_cannot_create_two_services_with_same_name(sample_user): @@ -125,8 +120,8 @@ def test_cannot_create_two_services_with_same_name(sample_user): restricted=False, created_by=sample_user) with pytest.raises(IntegrityError) as excinfo: - dao_create_service(service1, sample_user, 12345) - dao_create_service(service2, sample_user, 12345) + dao_create_service(service1, sample_user) + dao_create_service(service2, sample_user) assert 'duplicate key value violates unique constraint "services_name_key"' in str(excinfo.value) @@ -143,8 +138,8 @@ def test_cannot_create_two_services_with_same_email_from(sample_user): restricted=False, created_by=sample_user) with pytest.raises(IntegrityError) as excinfo: - dao_create_service(service1, sample_user, 12345) - dao_create_service(service2, sample_user, 12345) + dao_create_service(service1, sample_user) + dao_create_service(service2, sample_user) assert 'duplicate key value violates unique constraint "services_email_from_key"' in str(excinfo.value) @@ -156,7 +151,7 @@ def test_cannot_create_service_with_no_user(notify_db_session, sample_user): restricted=False, created_by=sample_user) with pytest.raises(FlushError) as excinfo: - dao_create_service(service, None, 12345) + dao_create_service(service, None) assert "Can't flush None value found in collection Service.users" in str(excinfo.value) @@ -166,7 +161,7 @@ def test_should_add_user_to_service(sample_user): message_limit=1000, restricted=False, created_by=sample_user) - dao_create_service(service, sample_user, 12345) + dao_create_service(service, sample_user) assert sample_user in Service.query.first().users new_user = User( name='Test User', @@ -185,7 +180,7 @@ def test_should_remove_user_from_service(sample_user): message_limit=1000, restricted=False, created_by=sample_user) - dao_create_service(service, sample_user, 12345) + dao_create_service(service, sample_user) new_user = User( name='Test User', email_address='new_user@digital.cabinet-office.gov.uk', @@ -337,7 +332,7 @@ def test_create_service_creates_a_history_record_with_current_data(sample_user): message_limit=1000, restricted=False, created_by=sample_user) - dao_create_service(service, sample_user, 12345) + dao_create_service(service, sample_user) assert Service.query.count() == 1 assert Service.get_history_model().query.count() == 1 @@ -364,7 +359,7 @@ def test_update_service_creates_a_history_record_with_current_data(sample_user): message_limit=1000, restricted=False, created_by=sample_user) - dao_create_service(service, sample_user, 12345) + dao_create_service(service, sample_user) assert Service.query.count() == 1 assert Service.query.first().version == 1 @@ -392,7 +387,7 @@ def test_update_service_permission_creates_a_history_record_with_current_data(sa message_limit=1000, restricted=False, created_by=sample_user) - dao_create_service(service, sample_user, 12345) + dao_create_service(service, sample_user) service.permissions.append(ServicePermission(service_id=service.id, permission='letter')) dao_update_service(service) @@ -435,7 +430,7 @@ def test_create_service_and_history_is_transactional(sample_user): created_by=sample_user) with pytest.raises(IntegrityError) as excinfo: - dao_create_service(service, sample_user, 12345) + dao_create_service(service, sample_user) assert 'column "name" violates not-null constraint' in str(excinfo.value) assert Service.query.count() == 0 @@ -484,7 +479,7 @@ def test_add_existing_user_to_another_service_doesnot_change_old_permissions(sam restricted=False, created_by=sample_user) - dao_create_service(service_one, sample_user, 12345) + dao_create_service(service_one, sample_user) assert sample_user.id == service_one.users[0].id test_user_permissions = Permission.query.filter_by(service=service_one, user=sample_user).all() assert len(test_user_permissions) == 8 @@ -501,7 +496,7 @@ def test_add_existing_user_to_another_service_doesnot_change_old_permissions(sam message_limit=1000, restricted=False, created_by=other_user) - dao_create_service(service_two, other_user, 12345) + dao_create_service(service_two, other_user) assert other_user.id == service_two.users[0].id other_user_permissions = Permission.query.filter_by(service=service_two, user=other_user).all() @@ -530,7 +525,7 @@ def test_fetch_stats_filters_on_service(sample_notification): email_from="hello", restricted=False, message_limit=1000) - dao_create_service(service_two, sample_notification.service.created_by, 12345) + dao_create_service(service_two, sample_notification.service.created_by) stats = dao_fetch_stats_for_service(service_two.id) assert len(stats) == 0 diff --git a/tests/app/db.py b/tests/app/db.py index 16471d555..717b1aa2b 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -79,12 +79,7 @@ def create_service( prefix_sms=prefix_sms, ) - dao_create_service( - service, - service.created_by, - free_sms_fragment_limit=250000, - service_id=service_id, - service_permissions=service_permissions) + dao_create_service(service, service.created_by, service_id, service_permissions=service_permissions) service.active = active service.research_mode = research_mode diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 765e64e2f..707bffc40 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -132,8 +132,6 @@ def test_get_service_by_id(admin_request, sample_service): assert json_resp['data']['dvla_organisation'] == '001' assert json_resp['data']['sms_sender'] == current_app.config['FROM_NUMBER'] assert json_resp['data']['prefix_sms'] is True - # deprecated field, no longer exists - assert 'free_sms_fragment_limit' not in json_resp['data'] @pytest.mark.parametrize('detailed', [True, False]) @@ -219,8 +217,7 @@ def test_create_service(client, sample_user): 'restricted': False, 'active': False, 'email_from': 'created.service', - 'created_by': str(sample_user.id), - 'free_sms_fragment_limit': 250000 + 'created_by': str(sample_user.id) } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] @@ -265,8 +262,7 @@ def test_should_not_create_service_with_missing_user_id_field(notify_api, fake_u 'message_limit': 1000, 'restricted': False, 'active': False, - 'created_by': str(fake_uuid), - 'free_sms_fragment_limit': 250000 + 'created_by': str(fake_uuid) } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] @@ -280,26 +276,6 @@ def test_should_not_create_service_with_missing_user_id_field(notify_api, fake_u assert 'Missing data for required field.' in json_resp['message']['user_id'] -def test_create_service_free_sms_fragment_limit_is_not_optional(admin_request, sample_user): - data1 = { - 'name': 'service 1', - 'user_id': str(sample_user.id), - 'message_limit': 1000, - 'restricted': False, - 'active': False, - 'email_from': 'sample_user.email1', - 'created_by': str(sample_user.id) - } - - json_resp = admin_request.post( - 'service.create_service', - _data=data1, - _expected_status=400 - ) - assert json_resp['result'] == 'error' - assert 'Missing data for required field.' in json_resp['message']['free_sms_fragment_limit'] - - def test_should_error_if_created_by_missing(notify_api, sample_user): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -309,8 +285,7 @@ def test_should_error_if_created_by_missing(notify_api, sample_user): 'message_limit': 1000, 'restricted': False, 'active': False, - 'user_id': str(sample_user.id), - 'free_sms_fragment_limit': 250000 + 'user_id': str(sample_user.id) } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] @@ -337,8 +312,7 @@ def test_should_not_create_service_with_missing_if_user_id_is_not_in_database(no 'message_limit': 1000, 'restricted': False, 'active': False, - 'created_by': str(fake_uuid), - 'free_sms_fragment_limit': 250000 + 'created_by': str(fake_uuid) } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] @@ -356,8 +330,7 @@ def test_should_not_create_service_if_missing_data(notify_api, sample_user): with notify_api.test_request_context(): with notify_api.test_client() as client: data = { - 'user_id': str(sample_user.id), - 'free_sms_fragment_limit': 250000 + 'user_id': str(sample_user.id) } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] @@ -385,8 +358,7 @@ def test_should_not_create_service_with_duplicate_name(notify_api, 'restricted': False, 'active': False, 'email_from': 'sample.service2', - 'created_by': str(sample_user.id), - 'free_sms_fragment_limit': 250000 + 'created_by': str(sample_user.id) } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] @@ -413,8 +385,7 @@ def test_create_service_should_throw_duplicate_key_constraint_for_existing_email 'restricted': False, 'active': False, 'email_from': 'first.service', - 'created_by': str(sample_user.id), - 'free_sms_fragment_limit': 250000 + 'created_by': str(sample_user.id) } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] @@ -844,8 +815,8 @@ def test_default_permissions_are_added_for_user_service(notify_api, 'restricted': False, 'active': False, 'email_from': 'created.service', - 'created_by': str(sample_user.id), - 'free_sms_fragment_limit': 250000} + 'created_by': str(sample_user.id) + } auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] resp = client.post( From 78099de7763e1bd1b57825e150fa5fc924117e3d Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 6 Dec 2017 11:03:42 +0000 Subject: [PATCH 8/9] make sure POST /free-sms-fragment-limit always creates refactored billing/rest.py and annual_billing_dao.py to remove logic from the dao, and simplify the process around creating new rows. Make sure that the POST always creates (it previously wouldn't create rows for years that don't already exist). Clean up some tests that were doing too much set-up/data verification via rest calls rather than directly inserting test data in to the DB. --- app/billing/rest.py | 24 +++-- app/dao/annual_billing_dao.py | 12 +-- tests/app/billing/test_billing.py | 117 +++++++++++------------ tests/app/dao/test_annual_billing_dao.py | 34 ++----- 4 files changed, 81 insertions(+), 106 deletions(-) diff --git a/app/billing/rest.py b/app/billing/rest.py index 1bdeaf6a9..73e742229 100644 --- a/app/billing/rest.py +++ b/app/billing/rest.py @@ -117,6 +117,7 @@ def get_free_sms_fragment_limit(service_id): financial_year_start = get_current_financial_year_start_year() if int(financial_year_start) < get_current_financial_year_start_year(): + # return the earliest historical entry annual_billing = sms_list[0] # The oldest entry else: annual_billing = sms_list[-1] # The newest entry @@ -141,10 +142,21 @@ def create_or_update_free_sms_fragment_limit(service_id): return jsonify(form), 201 -def update_free_sms_fragment_limit_data(service_id, free_sms_fragment_limit, financial_year_start=None): +def update_free_sms_fragment_limit_data(service_id, free_sms_fragment_limit, financial_year_start): current_year = get_current_financial_year_start_year() - if financial_year_start is None or financial_year_start >= current_year: - dao_update_annual_billing_for_current_and_future_years(service_id, free_sms_fragment_limit) - else: - dao_create_or_update_annual_billing_for_year(service_id, - free_sms_fragment_limit, financial_year_start) + if not financial_year_start: + financial_year_start = current_year + + dao_create_or_update_annual_billing_for_year( + service_id, + free_sms_fragment_limit, + financial_year_start + ) + # if we're trying to update historical data, don't touch other rows. + # Otherwise, make sure that future years will get the new updated value. + if financial_year_start >= current_year: + dao_update_annual_billing_for_current_and_future_years( + service_id, + free_sms_fragment_limit, + financial_year_start + ) diff --git a/app/dao/annual_billing_dao.py b/app/dao/annual_billing_dao.py index c3522ecbd..6e43be2d6 100644 --- a/app/dao/annual_billing_dao.py +++ b/app/dao/annual_billing_dao.py @@ -7,11 +7,7 @@ from app.dao.date_util import get_current_financial_year_start_year @transactional -def dao_create_or_update_annual_billing_for_year(service_id, free_sms_fragment_limit, financial_year_start=None): - - if not financial_year_start: - financial_year_start = get_current_financial_year_start_year() - +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) if result: @@ -30,11 +26,7 @@ def dao_get_annual_billing(service_id): @transactional -def dao_update_annual_billing_for_current_and_future_years(service_id, free_sms_fragment_limit, - financial_year_start=None): - if not financial_year_start: - financial_year_start = get_current_financial_year_start_year() - +def dao_update_annual_billing_for_current_and_future_years(service_id, free_sms_fragment_limit, financial_year_start): AnnualBilling.query.filter( AnnualBilling.service_id == service_id, AnnualBilling.financial_year_start >= financial_year_start diff --git a/tests/app/billing/test_billing.py b/tests/app/billing/test_billing.py index 6a440aaa7..9714af153 100644 --- a/tests/app/billing/test_billing.py +++ b/tests/app/billing/test_billing.py @@ -1,25 +1,25 @@ from datetime import datetime, timedelta import json +import pytest + from app.billing.rest import _transform_billing_for_month from app.dao.monthly_billing_dao import ( create_or_update_monthly_billing, get_monthly_billing_by_notification_type, ) from app.models import SMS_TYPE, EMAIL_TYPE +from app.dao.date_util import get_current_financial_year_start_year +from app.dao.annual_billing_dao import dao_get_free_sms_fragment_limit_for_year from tests.app.db import ( create_notification, create_rate, - create_monthly_billing_entry + create_monthly_billing_entry, + create_annual_billing, ) - -from tests import create_authorization_header - -from app.dao.date_util import get_current_financial_year_start_year -from app.dao.annual_billing_dao import dao_get_free_sms_fragment_limit_for_year -from tests.app.db import create_annual_billing from app.billing.rest import update_free_sms_fragment_limit_data +from tests import create_authorization_header APR_2016_MONTH_START = datetime(2016, 3, 31, 23, 00, 00) APR_2016_MONTH_END = datetime(2016, 4, 30, 22, 59, 59, 99999) @@ -270,70 +270,62 @@ def test_create_update_free_sms_fragment_limit_invalid_schema(client, sample_ser assert 'JSON' in json_resp['message'] -def test_create_free_sms_fragment_limit_current_year(client, sample_service): +def test_create_free_sms_fragment_limit_current_year_updates_future_years(admin_request, sample_service): current_year = get_current_financial_year_start_year() - data = {'free_sms_fragment_limit': 9999} - response = client.post('service/{}/billing/free-sms-fragment-limit'.format(sample_service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), create_authorization_header()]) + future_billing = create_annual_billing(sample_service.id, 1, current_year + 1) - response_get = client.get( - 'service/{}/billing/free-sms-fragment-limit?financial_year_start={}'.format(sample_service.id, current_year), - headers=[('Content-Type', 'application/json'), create_authorization_header()]) + admin_request.post( + 'billing.create_or_update_free_sms_fragment_limit', + service_id=sample_service.id, + _data={'free_sms_fragment_limit': 9999}, + _expected_status=201 + ) - json_resp = json.loads(response_get.get_data(as_text=True)) - assert response.status_code == 201 - assert response_get.status_code == 200 - assert json_resp['financial_year_start'] == current_year - assert json_resp['free_sms_fragment_limit'] == 9999 + current_billing = dao_get_free_sms_fragment_limit_for_year(sample_service.id, current_year) + assert future_billing.free_sms_fragment_limit == 9999 + assert current_billing.financial_year_start == current_year + assert current_billing.free_sms_fragment_limit == 9999 -def test_create_free_sms_fragment_limit_past_year(client, sample_service): - - data = {'financial_year_start': 2016, 'free_sms_fragment_limit': 9999} - response = client.post('service/{}/billing/free-sms-fragment-limit'.format(sample_service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), create_authorization_header()]) - - response_get = client.get( - 'service/{}/billing/free-sms-fragment-limit?financial_year_start=2016'.format(sample_service.id), - headers=[('Content-Type', 'application/json'), create_authorization_header()]) - - json_resp = json.loads(response_get.get_data(as_text=True)) - assert response.status_code == 201 - assert response_get.status_code == 200 - assert json_resp['financial_year_start'] == 2016 - assert json_resp['free_sms_fragment_limit'] == 9999 - - -def test_update_free_sms_fragment_limit(client, sample_service): +@pytest.mark.parametrize('update_existing', [True, False]) +def test_create_or_update_free_sms_fragment_limit_past_year_doenst_update_other_years( + admin_request, + sample_service, + update_existing +): current_year = get_current_financial_year_start_year() + create_annual_billing(sample_service.id, 1, current_year) + if update_existing: + create_annual_billing(sample_service.id, 1, current_year - 1) - annual_billing = dao_get_free_sms_fragment_limit_for_year(sample_service.id, current_year) - assert annual_billing.free_sms_fragment_limit == 250000 + data = {'financial_year_start': current_year - 1, 'free_sms_fragment_limit': 9999} + admin_request.post( + 'billing.create_or_update_free_sms_fragment_limit', + service_id=sample_service.id, + _data=data, + _expected_status=201) - data_new = {'financial_year_start': current_year, 'free_sms_fragment_limit': 9999} - response = client.post('service/{}/billing/free-sms-fragment-limit'.format(sample_service.id), - data=json.dumps(data_new), - headers=[('Content-Type', 'application/json'), create_authorization_header()]) - - response_get = client.get( - 'service/{}/billing/free-sms-fragment-limit?financial_year_start={}' - .format(sample_service.id, current_year), - headers=[('Content-Type', 'application/json'), create_authorization_header()]) - - json_resp = json.loads(response_get.get_data(as_text=True)) - - assert response.status_code == 201 - assert response_get.status_code == 200 - assert json_resp['financial_year_start'] == current_year - assert json_resp['free_sms_fragment_limit'] == 9999 + assert dao_get_free_sms_fragment_limit_for_year(sample_service.id, current_year - 1).free_sms_fragment_limit == 9999 + assert dao_get_free_sms_fragment_limit_for_year(sample_service.id, current_year).free_sms_fragment_limit == 1 -def test_get_free_sms_fragment_limit_current_year(client, sample_service): +def test_create_free_sms_fragment_limit_updates_existing_year(admin_request, sample_service): + current_year = get_current_financial_year_start_year() + annual_billing = create_annual_billing(sample_service.id, 1, current_year) + + admin_request.post( + 'billing.create_or_update_free_sms_fragment_limit', + service_id=sample_service.id, + _data={'financial_year_start': current_year, 'free_sms_fragment_limit': 2}, + _expected_status=201) + + assert annual_billing.free_sms_fragment_limit == 2 + + +def test_get_free_sms_fragment_limit_current_year_creates_new_row(client, sample_service): current_year = get_current_financial_year_start_year() - create_annual_billing(sample_service.id, free_sms_fragment_limit=9999, financial_year_start=current_year - 1) + create_annual_billing(sample_service.id, 9999, current_year - 1) response_get = client.get( 'service/{}/billing/free-sms-fragment-limit'.format(sample_service.id), @@ -342,7 +334,7 @@ def test_get_free_sms_fragment_limit_current_year(client, sample_service): json_resp = json.loads(response_get.get_data(as_text=True)) assert response_get.status_code == 200 assert json_resp['financial_year_start'] == get_current_financial_year_start_year() - assert json_resp['free_sms_fragment_limit'] == 250000 + assert json_resp['free_sms_fragment_limit'] == 9999 def test_get_free_sms_fragment_limit_past_year_not_exist(client, sample_service): @@ -385,10 +377,9 @@ def test_get_free_sms_fragment_limit_future_year_not_exist(client, sample_servic def test_update_free_sms_fragment_limit_data(client, sample_service): current_year = get_current_financial_year_start_year() - annual_billing = dao_get_free_sms_fragment_limit_for_year(sample_service.id, current_year) - assert annual_billing.free_sms_fragment_limit == 250000 + create_annual_billing(sample_service.id, free_sms_fragment_limit=250000, financial_year_start=current_year - 1) - update_free_sms_fragment_limit_data(sample_service.id, 9999) + update_free_sms_fragment_limit_data(sample_service.id, 9999, current_year) annual_billing = dao_get_free_sms_fragment_limit_for_year(sample_service.id, current_year) assert annual_billing.free_sms_fragment_limit == 9999 diff --git a/tests/app/dao/test_annual_billing_dao.py b/tests/app/dao/test_annual_billing_dao.py index 3e475ef5c..559e1c8f4 100644 --- a/tests/app/dao/test_annual_billing_dao.py +++ b/tests/app/dao/test_annual_billing_dao.py @@ -8,16 +8,6 @@ from app.dao.annual_billing_dao import ( from tests.app.db import create_annual_billing -def test_get_sample_service_has_default_free_sms_fragment_limit(notify_db_session, sample_service): - - # when sample_service was created, it automatically create an entry in the annual_billing table - free_limit = dao_get_free_sms_fragment_limit_for_year(sample_service.id, get_current_financial_year_start_year()) - - assert free_limit.free_sms_fragment_limit == 250000 - assert free_limit.financial_year_start == get_current_financial_year_start_year() - assert free_limit.service_id == sample_service.id - - def test_dao_update_free_sms_fragment_limit(notify_db_session, sample_service): new_limit = 9999 year = get_current_financial_year_start_year() @@ -27,16 +17,7 @@ def test_dao_update_free_sms_fragment_limit(notify_db_session, sample_service): assert new_free_limit.free_sms_fragment_limit == new_limit -def test_create_annual_billing_not_specify_year(notify_db_session, sample_service): - - dao_create_or_update_annual_billing_for_year(sample_service.id, 9999) - - free_limit = dao_get_free_sms_fragment_limit_for_year(sample_service.id) - - assert free_limit.free_sms_fragment_limit == 9999 - - -def test_create_annual_billing_specify_year(notify_db_session, sample_service): +def test_create_annual_billing(sample_service): dao_create_or_update_annual_billing_for_year(sample_service.id, 9999, 2016) @@ -47,16 +28,15 @@ def test_create_annual_billing_specify_year(notify_db_session, sample_service): def test_dao_update_annual_billing_for_current_and_future_years(notify_db_session, sample_service): current_year = get_current_financial_year_start_year() - limits = [240000, 250000, 260000, 270000] + limits = [1, 2, 3, 4] create_annual_billing(sample_service.id, limits[0], current_year - 1) create_annual_billing(sample_service.id, limits[2], current_year + 1) create_annual_billing(sample_service.id, limits[3], current_year + 2) dao_update_annual_billing_for_current_and_future_years(sample_service.id, 9999, current_year) - free_limit = dao_get_free_sms_fragment_limit_for_year(sample_service.id, current_year - 1) - assert free_limit.free_sms_fragment_limit == 240000 - - for year in range(current_year, current_year + 3): - free_limit = dao_get_free_sms_fragment_limit_for_year(sample_service.id, year) - assert free_limit.free_sms_fragment_limit == 9999 + assert dao_get_free_sms_fragment_limit_for_year(sample_service.id, current_year - 1).free_sms_fragment_limit == 1 + # current year is not created + assert dao_get_free_sms_fragment_limit_for_year(sample_service.id, current_year) is None + assert dao_get_free_sms_fragment_limit_for_year(sample_service.id, current_year + 1).free_sms_fragment_limit == 9999 + assert dao_get_free_sms_fragment_limit_for_year(sample_service.id, current_year + 2).free_sms_fragment_limit == 9999 From f29e08c7781be702abc6946421e6e55e6fcd9fed Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 6 Dec 2017 14:41:32 +0000 Subject: [PATCH 9/9] don't update current year twice --- app/billing/rest.py | 4 ++-- app/dao/annual_billing_dao.py | 4 ++-- tests/app/dao/test_annual_billing_dao.py | 7 ++++--- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/app/billing/rest.py b/app/billing/rest.py index 73e742229..66726bb5a 100644 --- a/app/billing/rest.py +++ b/app/billing/rest.py @@ -14,7 +14,7 @@ from app.utils import convert_utc_to_bst from app.dao.annual_billing_dao import (dao_get_free_sms_fragment_limit_for_year, dao_get_all_free_sms_fragment_limit, dao_create_or_update_annual_billing_for_year, - dao_update_annual_billing_for_current_and_future_years) + dao_update_annual_billing_for_future_years) from app.billing.billing_schemas import create_or_update_free_sms_fragment_limit_schema from app.errors import InvalidRequest from app.schema_validation import validate @@ -155,7 +155,7 @@ def update_free_sms_fragment_limit_data(service_id, free_sms_fragment_limit, fin # if we're trying to update historical data, don't touch other rows. # Otherwise, make sure that future years will get the new updated value. if financial_year_start >= current_year: - dao_update_annual_billing_for_current_and_future_years( + dao_update_annual_billing_for_future_years( service_id, free_sms_fragment_limit, financial_year_start diff --git a/app/dao/annual_billing_dao.py b/app/dao/annual_billing_dao.py index 6e43be2d6..744fc6106 100644 --- a/app/dao/annual_billing_dao.py +++ b/app/dao/annual_billing_dao.py @@ -26,10 +26,10 @@ def dao_get_annual_billing(service_id): @transactional -def dao_update_annual_billing_for_current_and_future_years(service_id, free_sms_fragment_limit, financial_year_start): +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, - AnnualBilling.financial_year_start >= financial_year_start + AnnualBilling.financial_year_start > financial_year_start ).update( {'free_sms_fragment_limit': free_sms_fragment_limit} ) diff --git a/tests/app/dao/test_annual_billing_dao.py b/tests/app/dao/test_annual_billing_dao.py index 559e1c8f4..ac0c4df2f 100644 --- a/tests/app/dao/test_annual_billing_dao.py +++ b/tests/app/dao/test_annual_billing_dao.py @@ -1,9 +1,10 @@ + from app.dao.date_util import get_current_financial_year_start_year from app.dao.annual_billing_dao import ( dao_create_or_update_annual_billing_for_year, dao_get_free_sms_fragment_limit_for_year, - dao_update_annual_billing_for_current_and_future_years, + dao_update_annual_billing_for_future_years, ) from tests.app.db import create_annual_billing @@ -26,14 +27,14 @@ def test_create_annual_billing(sample_service): assert free_limit.free_sms_fragment_limit == 9999 -def test_dao_update_annual_billing_for_current_and_future_years(notify_db_session, sample_service): +def test_dao_update_annual_billing_for_future_years(notify_db_session, sample_service): current_year = get_current_financial_year_start_year() limits = [1, 2, 3, 4] create_annual_billing(sample_service.id, limits[0], current_year - 1) create_annual_billing(sample_service.id, limits[2], current_year + 1) create_annual_billing(sample_service.id, limits[3], current_year + 2) - dao_update_annual_billing_for_current_and_future_years(sample_service.id, 9999, current_year) + dao_update_annual_billing_for_future_years(sample_service.id, 9999, current_year) assert dao_get_free_sms_fragment_limit_for_year(sample_service.id, current_year - 1).free_sms_fragment_limit == 1 # current year is not created