From c07e991c0aec09b7ce00b7c65fdfdcc139282482 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Wed, 21 Apr 2021 14:00:02 +0100 Subject: [PATCH 1/6] Update sqlalchemy from 1.3.23 to 1.4.10 --- requirements-app.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements-app.txt b/requirements-app.txt index a3cd69af9..2dedf42ce 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -19,7 +19,7 @@ marshmallow-sqlalchemy==0.23.1 # pyup: <0.24.0 # marshmallow v3 throws errors marshmallow==2.21.0 # pyup: <3 # v3 throws errors psycopg2-binary==2.8.6 PyJWT==2.0.1 -SQLAlchemy==1.3.23 +SQLAlchemy==1.4.10 strict-rfc3339==0.7 rfc3987==1.3.8 cachetools==4.2.1 From 66127f3800c6ce3428b50c426232c299ffb7294f Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Thu, 22 Apr 2021 16:59:15 +0100 Subject: [PATCH 2/6] Apply requirements update --- requirements.txt | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/requirements.txt b/requirements.txt index 42f1bf29e..d243c10b7 100644 --- a/requirements.txt +++ b/requirements.txt @@ -21,7 +21,7 @@ marshmallow-sqlalchemy==0.23.1 # pyup: <0.24.0 # marshmallow v3 throws errors marshmallow==2.21.0 # pyup: <3 # v3 throws errors psycopg2-binary==2.8.6 PyJWT==2.0.1 -SQLAlchemy==1.3.23 +SQLAlchemy==1.4.10 strict-rfc3339==0.7 rfc3987==1.3.8 cachetools==4.2.1 @@ -47,14 +47,14 @@ alembic==1.5.8 amqp==1.4.9 anyjson==0.3.3 attrs==20.3.0 -awscli==1.19.53 +awscli==1.19.55 bcrypt==3.2.0 billiard==3.3.0.23 bleach==3.3.0 blinker==1.4 boto==2.49.0 -boto3==1.17.53 -botocore==1.20.53 +boto3==1.17.55 +botocore==1.20.55 certifi==2020.12.5 chardet==4.0.0 click==7.1.2 @@ -88,7 +88,7 @@ PyYAML==5.4.1 redis==3.5.3 requests==2.25.1 rsa==4.7.2 -s3transfer==0.3.7 +s3transfer==0.4.1 Shapely==1.7.1 six==1.15.0 smartypants==2.0.1 From e1a626855ddba9694d26615a0b409c3ff7cedb6c Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 23 Apr 2021 16:48:36 +0100 Subject: [PATCH 3/6] Update error messages after SQLAlchemy version update --- tests/app/dao/test_invited_user_dao.py | 2 +- tests/app/dao/test_services_dao.py | 2 +- tests/app/dao/test_templates_dao.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/app/dao/test_invited_user_dao.py b/tests/app/dao/test_invited_user_dao.py index 1711f4595..a2b0d3b28 100644 --- a/tests/app/dao/test_invited_user_dao.py +++ b/tests/app/dao/test_invited_user_dao.py @@ -79,7 +79,7 @@ def test_get_unknown_invited_user_returns_none(notify_db, notify_db_session, sam with pytest.raises(NoResultFound) as e: get_invited_user_by_service_and_id(sample_service.id, unknown_id) - assert 'No row was found for one()' in str(e.value) + assert 'No row was found when one was required' in str(e.value) def test_get_invited_users_for_service(notify_db, notify_db_session, sample_service): diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 3522fc0f0..9671e7896 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -509,7 +509,7 @@ def test_dao_fetch_live_services_data(sample_user): def test_get_service_by_id_returns_none_if_no_service(notify_db): with pytest.raises(NoResultFound) as e: dao_fetch_service_by_id(str(uuid.uuid4())) - assert 'No row was found for one()' in str(e.value) + assert 'No row was found when one was required' in str(e.value) def test_get_service_by_id_returns_service(notify_db_session): diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index 3c7dae016..f903bb602 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -372,7 +372,7 @@ def test_get_template_version_returns_none_for_hidden_templates(sample_service): def test_get_template_by_id_and_service_returns_none_if_no_template(sample_service, fake_uuid): with pytest.raises(NoResultFound) as e: dao_get_template_by_id_and_service_id(template_id=fake_uuid, service_id=sample_service.id) - assert 'No row was found for one' in str(e.value) + assert 'No row was found when one was required' in str(e.value) def test_create_template_creates_a_history_record_with_current_data(sample_service, sample_user): From 1b070d69a12111b1d12cfe048578cc91859a15d7 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 26 Apr 2021 11:50:17 +0100 Subject: [PATCH 4/6] The update of SQLAlchemy 1.4.10 has caused some conflicts in our code. This PR fixes most of those conflicts. - sqlalchemy.sql.expression.case must include an else statement. - clearly define list of columns for inbound_sms_history insert, getting the list from InboundSmsHistory.__table__.c was causing data type errors. - remove relationships when not needed, the foreign key relationship is established in the creation of the column. This will get rid of the warnings referenced here: http://sqlalche.me/e/14/qzyx. - update queries now that he user relationship in ServiceUser db model has been removed. - move the check that a template is archived to the view instead of the dao method. The check was clearing the session before the version history could be done. Deleting notifications in the night tasks still needs to be investigated. The raw sql is causing an error. --- app/dao/fact_billing_dao.py | 9 +++++--- app/dao/inbound_sms_dao.py | 8 ++++++- app/dao/service_user_dao.py | 10 +++++--- app/dao/templates_dao.py | 3 --- app/models.py | 3 --- app/template/rest.py | 3 ++- app/user/rest.py | 2 +- tests/app/dao/test_templates_dao.py | 36 +---------------------------- 8 files changed, 24 insertions(+), 50 deletions(-) diff --git a/app/dao/fact_billing_dao.py b/app/dao/fact_billing_dao.py index 97c40bf87..d88ef8b12 100644 --- a/app/dao/fact_billing_dao.py +++ b/app/dao/fact_billing_dao.py @@ -150,9 +150,12 @@ def fetch_letter_line_items_for_all_services(start_date, end_date): [(FactBilling.postage.in_(INTERNATIONAL_POSTAGE_TYPES), "international")], else_=FactBilling.postage ).label("postage") - postage_order = case(((formatted_postage == "second", 1), - (formatted_postage == "first", 2), - (formatted_postage == "international", 3))) + postage_order = case( + (formatted_postage == "second", 1), + (formatted_postage == "first", 2), + (formatted_postage == "international", 3), + else_=0 # assumes never get 0 as a result + ) query = db.session.query( Organisation.name.label("organisation_name"), diff --git a/app/dao/inbound_sms_dao.py b/app/dao/inbound_sms_dao.py index 303027e4c..a8fe7852c 100644 --- a/app/dao/inbound_sms_dao.py +++ b/app/dao/inbound_sms_dao.py @@ -71,7 +71,13 @@ def dao_count_inbound_sms_for_service(service_id, limit_days): def _insert_inbound_sms_history(subquery, query_limit=10000): offset = 0 inbound_sms_query = db.session.query( - *[x.name for x in InboundSmsHistory.__table__.c] + InboundSms.id, + InboundSms.created_at, + InboundSms.service_id, + InboundSms.notify_number, + InboundSms.provider_date, + InboundSms.provider_reference, + InboundSms.provider ).filter(InboundSms.id.in_(subquery)) inbound_sms_count = inbound_sms_query.count() diff --git a/app/dao/service_user_dao.py b/app/dao/service_user_dao.py index e6685322e..1c7b2af42 100644 --- a/app/dao/service_user_dao.py +++ b/app/dao/service_user_dao.py @@ -9,9 +9,13 @@ def dao_get_service_user(user_id, service_id): def dao_get_active_service_users(service_id): - query = ServiceUser.query.join(ServiceUser.user).filter( - ServiceUser.service_id == service_id, - User.state == 'active' + query = db.session.query( + ServiceUser + ).join( + User, User.id == ServiceUser.user_id + ).filter( + User.state == 'active', + ServiceUser.service_id == service_id ) return query.all() diff --git a/app/dao/templates_dao.py b/app/dao/templates_dao.py index e28ffa3bf..19361fce1 100644 --- a/app/dao/templates_dao.py +++ b/app/dao/templates_dao.py @@ -43,9 +43,6 @@ def dao_create_template(template): VersionOptions(Template, history_class=TemplateHistory) ) def dao_update_template(template): - if template.archived: - template.folder = None - db.session.add(template) diff --git a/app/models.py b/app/models.py index 9d0db0563..249b33626 100644 --- a/app/models.py +++ b/app/models.py @@ -199,8 +199,6 @@ class ServiceUser(db.Model): UniqueConstraint('user_id', 'service_id', name='uix_user_to_service'), ) - user = db.relationship('User') - user_to_organisation = db.Table( 'user_to_organisation', @@ -671,7 +669,6 @@ class ServicePermission(db.Model): primary_key=True, index=True, nullable=False) permission = db.Column(db.String(255), db.ForeignKey('service_permission_types.name'), index=True, primary_key=True, nullable=False) - service = db.relationship("Service") created_at = db.Column(db.DateTime, default=datetime.datetime.utcnow, nullable=False) service_permission_types = db.relationship( diff --git a/app/template/rest.py b/app/template/rest.py index 0ff792660..740e649a6 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -145,7 +145,8 @@ def update_template(service_id, template_id): raise InvalidRequest(errors, status_code=400) update_dict = template_schema.load(updated_template).data - + if update_dict.archived: + update_dict.folder = None dao_update_template(update_dict) return jsonify(data=template_schema.dump(update_dict).data), 200 diff --git a/app/user/rest.py b/app/user/rest.py index f02e2c527..4c7a943e1 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -405,7 +405,7 @@ def set_permissions(user_id, service_id): # TODO fix security hole, how do we verify that the user # who is making this request has permission to make the request. service_user = dao_get_service_user(user_id, service_id) - user = service_user.user + user = get_user_by_id(user_id) service = dao_fetch_service_by_id(service_id=service_id) data = request.get_json() diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index f903bb602..56a93c455 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -13,12 +13,7 @@ from app.dao.templates_dao import ( dao_update_template, dao_update_template_reply_to, ) -from app.models import ( - Template, - TemplateFolder, - TemplateHistory, - TemplateRedacted, -) +from app.models import Template, TemplateHistory, TemplateRedacted from tests.app.db import create_letter_contact, create_template @@ -95,35 +90,6 @@ def test_update_template(sample_service, sample_user): assert dao_get_all_templates_for_service(sample_service.id)[0].name == 'new name' -def test_update_template_in_a_folder_to_archived(sample_service, sample_user): - template_data = { - 'name': 'Sample Template', - 'template_type': "sms", - 'content': "Template content", - 'service': sample_service, - 'created_by': sample_user - } - template = Template(**template_data) - - template_folder_data = { - 'name': 'My Folder', - 'service_id': sample_service.id, - } - template_folder = TemplateFolder(**template_folder_data) - - template.folder = template_folder - dao_create_template(template) - - template.archived = True - dao_update_template(template) - - template_folder = TemplateFolder.query.one() - archived_template = Template.query.one() - - assert template_folder - assert not archived_template.folder - - def test_dao_update_template_reply_to_none_to_some(sample_service, sample_user): letter_contact = create_letter_contact(sample_service, 'Edinburgh, ED1 1AA') From f941768d8cf094678a1c3dfa0b8919196026c03e Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 27 Apr 2021 08:36:34 +0100 Subject: [PATCH 5/6] Change the query to get the services to purge to use query on the db.Model rather than db.session.query. `service_ids_to_purge` is a list of `row` object rather than a list of `UUID`. NOTE: db.session.query(Service).filter(Service.id.notin_(services_with_data_retention)).all() would have also worked. It seems that only selecting attributes from the db.Model has caused the change. --- app/dao/notifications_dao.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index f858ab6c4..3caf04627 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -328,11 +328,11 @@ def delete_notifications_older_than_retention_by_type(notification_type, qry_lim seven_days_ago = get_london_midnight_in_utc(convert_utc_to_bst(datetime.utcnow()).date()) - timedelta(days=7) services_with_data_retention = [x.service_id for x in flexible_data_retention] - service_ids_to_purge = db.session.query(Service.id).filter(Service.id.notin_(services_with_data_retention)).all() + service_ids_to_purge = Service.query.filter(Service.id.notin_(services_with_data_retention)).all() - for service_id in service_ids_to_purge: + for service in service_ids_to_purge: deleted += _move_notifications_to_notification_history( - notification_type, service_id, seven_days_ago, qry_limit) + notification_type, service.id, seven_days_ago, qry_limit) current_app.logger.info('Finished deleting {} notifications'.format(notification_type)) From dcd08a0e458d8fe0bcfdf015d06e50fa9d582e61 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 27 Apr 2021 12:30:50 +0100 Subject: [PATCH 6/6] Add unit test for archiving a template with a template folder --- tests/app/template/test_rest.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 79610d09c..759afd215 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -411,6 +411,27 @@ def test_should_be_able_to_archive_template(client, sample_template): assert Template.query.first().archived +def test_should_be_able_to_archive_template_should_remove_template_folders( + client, sample_service +): + template_folder = create_template_folder(service=sample_service) + template = create_template(service=sample_service, folder=template_folder) + + data = { + 'archived': True, + } + + client.post( + f'/service/{sample_service.id}/template/{template.id}', + headers=[('Content-Type', 'application/json'), create_authorization_header()], + data=json.dumps(data) + ) + + updated_template = Template.query.get(template.id) + assert updated_template.archived + assert not updated_template.folder + + def test_get_precompiled_template_for_service( client, notify_user,