From a9c133887387889175f4a2809f34d73134b9a271 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 6 Jul 2017 14:20:24 +0100 Subject: [PATCH] Remove Notification, NotificationHistory status labels: Replace labels by adding a key kwarg in the model for status. We still need this as sqlalchemy attmempts to look for `notification_status` on the model (Notification/NotificationHistory). To achieve true ORM mapping (map status -> notification_status) we need the key kwarg. More here: http://docs.sqlalchemy.org/en/latest/core/metadata.html#sqlalchemy.schema.Column#key --- app/dao/jobs_dao.py | 2 +- app/dao/services_dao.py | 18 ++++++----------- app/dao/statistics_dao.py | 2 +- app/models.py | 28 ++++++-------------------- tests/app/dao/test_notification_dao.py | 4 ++-- tests/app/job/test_rest.py | 2 -- 6 files changed, 16 insertions(+), 40 deletions(-) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index 748b7cbaa..ec7bbe2a6 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -17,7 +17,7 @@ from app.statsd_decorators import statsd def dao_get_notification_outcomes_for_job(service_id, job_id): query = db.session.query( func.count(NotificationHistory.status).label('count'), - NotificationHistory.status.label('status') + NotificationHistory.status ) return query \ diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index b48f94f92..a8d546810 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -262,8 +262,7 @@ def fetch_todays_total_message_count(service_id): def _stats_for_service_query(service_id): return db.session.query( Notification.notification_type, - # see dao_fetch_todays_stats_for_all_services for why we have this label - Notification.status.label('status'), + Notification.status, func.count(Notification.id).label('count') ).filter( Notification.service_id == service_id, @@ -281,8 +280,7 @@ def dao_fetch_monthly_historical_stats_by_template_for_service(service_id, year) start_date, end_date = get_financial_year(year) sq = db.session.query( NotificationHistory.template_id, - # see dao_fetch_todays_stats_for_all_services for why we have this label - NotificationHistory.status.label('status'), + NotificationHistory.status, month.label('month'), func.count().label('count') ).filter( @@ -298,7 +296,7 @@ def dao_fetch_monthly_historical_stats_by_template_for_service(service_id, year) Template.id.label('template_id'), Template.name, Template.template_type, - sq.c.status.label('status'), + sq.c.status, sq.c.count.label('count'), sq.c.month ).join( @@ -316,8 +314,7 @@ def dao_fetch_monthly_historical_stats_for_service(service_id, year): start_date, end_date = get_financial_year(year) rows = db.session.query( NotificationHistory.notification_type, - # see dao_fetch_todays_stats_for_all_services for why we have this label - NotificationHistory.status.label('status'), + NotificationHistory.status, month, func.count(NotificationHistory.id).label('count') ).filter( @@ -356,9 +353,7 @@ def dao_fetch_monthly_historical_stats_for_service(service_id, year): def dao_fetch_todays_stats_for_all_services(include_from_test_key=True): query = db.session.query( Notification.notification_type, - # this label is necessary as the column has a different name under the hood (_status_enum / _status_fkey), - # if we query the Notification object there is a hybrid property to translate, but here there isn't anything. - Notification.status.label('status'), + Notification.status, Notification.service_id, func.count(Notification.id).label('count') ).filter( @@ -388,8 +383,7 @@ def fetch_stats_by_date_range_for_all_services(start_date, end_date, include_fro query = db.session.query( table.notification_type, - # see dao_fetch_todays_stats_for_all_services for why we have this label - table.status.label('status'), + table.status, table.service_id, func.count(table.id).label('count') ).filter( diff --git a/app/dao/statistics_dao.py b/app/dao/statistics_dao.py index 48d62e71c..d1e1cc661 100644 --- a/app/dao/statistics_dao.py +++ b/app/dao/statistics_dao.py @@ -31,7 +31,7 @@ def timeout_job_counts(notifications_type, timeout_start): results = db.session.query( JobStatistics.job_id.label('job_id'), func.count(Notification.status).label('count'), - Notification.status.label('status') + Notification.status ).filter( Notification.notification_type == notifications_type, JobStatistics.job_id == Notification.job_id, diff --git a/app/models.py b/app/models.py index 445b9f99e..6518b222e 100644 --- a/app/models.py +++ b/app/models.py @@ -788,14 +788,14 @@ class Notification(db.Model): unique=False, nullable=True, onupdate=datetime.datetime.utcnow) - _status_enum = db.Column('status', NOTIFICATION_STATUS_TYPES_ENUM, index=True, nullable=True, default='created') - _status_fkey = db.Column( + status = db.Column( 'notification_status', db.String, db.ForeignKey('notification_status_types.name'), index=True, nullable=True, - default='created' + default='created', + key='status' # http://docs.sqlalchemy.org/en/latest/core/metadata.html#sqlalchemy.schema.Column ) reference = db.Column(db.String, nullable=True, index=True) client_reference = db.Column(db.String, index=True, nullable=True) @@ -817,14 +817,6 @@ class Notification(db.Model): created_by = db.relationship('User') created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), nullable=True) - @hybrid_property - def status(self): - return self._status_fkey - - @status.setter - def status(self, status): - self._status_fkey = status - @property def personalisation(self): if self._personalisation: @@ -998,14 +990,14 @@ class NotificationHistory(db.Model, HistoryModel): sent_at = db.Column(db.DateTime, index=False, unique=False, nullable=True) sent_by = db.Column(db.String, nullable=True) updated_at = db.Column(db.DateTime, index=False, unique=False, nullable=True) - _status_enum = db.Column('status', NOTIFICATION_STATUS_TYPES_ENUM, index=True, nullable=True, default='created') - _status_fkey = db.Column( + status = db.Column( 'notification_status', db.String, db.ForeignKey('notification_status_types.name'), index=True, nullable=True, - default='created' + default='created', + key='status' # http://docs.sqlalchemy.org/en/latest/core/metadata.html#sqlalchemy.schema.Column ) reference = db.Column(db.String, nullable=True, index=True) client_reference = db.Column(db.String, nullable=True) @@ -1027,14 +1019,6 @@ class NotificationHistory(db.Model, HistoryModel): super().update_from_original(original) self.status = original.status - @hybrid_property - def status(self): - return self._status_fkey - - @status.setter - def status(self, status): - self._status_fkey = status - INVITED_USER_STATUS_TYPES = ['pending', 'accepted', 'cancelled'] diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 4c26025a5..ca5d8cb99 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -410,7 +410,7 @@ def test_should_by_able_to_update_status_by_id(sample_template, sample_job, mmg_ data = _notification_json(sample_template, job_id=sample_job.id, status='sending') notification = Notification(**data) dao_create_notification(notification) - assert notification._status_fkey == 'sending' + assert notification.status == 'sending' assert Notification.query.get(notification.id).status == 'sending' @@ -421,7 +421,7 @@ def test_should_by_able_to_update_status_by_id(sample_template, sample_job, mmg_ assert updated.updated_at == datetime(2000, 1, 2, 12, 0, 0) assert Notification.query.get(notification.id).status == 'delivered' assert notification.updated_at == datetime(2000, 1, 2, 12, 0, 0) - assert notification._status_fkey == 'delivered' + assert notification.status == 'delivered' def test_should_not_update_status_by_id_if_not_sending_and_does_not_update_job(notify_db, notify_db_session): diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index b41dfe065..ca25e0769 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -463,8 +463,6 @@ def test_get_all_notifications_for_job_returns_correct_format( assert len(resp['notifications']) == 1 assert resp['notifications'][0]['id'] == str(sample_notification_with_job.id) assert resp['notifications'][0]['status'] == sample_notification_with_job.status - assert '_status_fkey' not in resp['notifications'][0] - assert '_status_enum' not in resp['notifications'][0] def test_get_job_by_id(notify_api, sample_job):