From 68d28aa83b38a0110ade2d9432d0745880a4cfbf Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 26 Apr 2021 11:50:17 +0100 Subject: [PATCH] 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 2b91d9395..959a6beb7 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', @@ -674,7 +672,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 0933ac919..521a1bef2 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -157,7 +157,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')