diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 7df225561..f93e062ad 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -214,15 +214,16 @@ def _update_notification_stats_query(notification_type, status): def _update_statistics(notification, notification_statistics_status): + if notification.job_id: + db.session.query(Job).filter_by(id=notification.job_id + ).update(_update_job_stats_query(notification_statistics_status)) + db.session.query(NotificationStatistics).filter_by( day=notification.created_at.date(), service_id=notification.service_id ).update( _update_notification_stats_query(notification.template.template_type, notification_statistics_status) ) - if notification.job_id: - db.session.query(Job).filter_by(id=notification.job_id - ).update(_update_job_stats_query(notification_statistics_status)) def _update_job_stats_query(status): @@ -243,16 +244,14 @@ def _decide_permanent_temporary_failure(current_status, status): def _update_notification_status(notification, status, notification_statistics_status): - if not notification: - return 0 status = _decide_permanent_temporary_failure(current_status=notification.status, status=status) - count = db.session.query(Notification).filter(Notification.id == notification.id - ).update({Notification.status: status}) - if count == 1 and notification_statistics_status: + if notification_statistics_status: _update_statistics(notification, notification_statistics_status) - return count + db.session.query(Notification).filter(Notification.id == notification.id + ).update({Notification.status: status}) + return True @transactional @@ -262,10 +261,14 @@ def update_notification_status_by_id(notification_id, status, notification_stati Notification.status == 'pending') ).first() - count = _update_notification_status(notification=notification, - status=status, - notification_statistics_status=notification_statistics_status) - return count + if not notification: + return False + + return _update_notification_status( + notification=notification, + status=status, + notification_statistics_status=notification_statistics_status + ) @transactional @@ -274,10 +277,14 @@ def update_notification_status_by_reference(reference, status, notification_stat or_(Notification.status == 'sending', Notification.status == 'pending') ).first() + if not notification: + return False - return _update_notification_status(notification=notification, - status=status, - notification_statistics_status=notification_statistics_status) + return _update_notification_status( + notification=notification, + status=status, + notification_statistics_status=notification_statistics_status + ) def dao_update_notification(notification): diff --git a/app/notifications/process_client_response.py b/app/notifications/process_client_response.py index 3e2e2e90e..2f537d3a1 100644 --- a/app/notifications/process_client_response.py +++ b/app/notifications/process_client_response.py @@ -54,10 +54,9 @@ def process_sms_client_response(status, reference, client_name): notification_success = response_dict['success'] # record stats - update_success = notifications_dao.update_notification_status_by_id(reference, - notification_status, - notification_statistics_status) - if update_success == 0: + if not notifications_dao.update_notification_status_by_id(reference, + notification_status, + notification_statistics_status): status_error = "{} callback failed: notification {} either not found or already updated " \ "from sending. Status {}".format(client_name, reference, diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 7d03a0d9a..8729b1058 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -90,11 +90,11 @@ def process_ses_response(): ), 200 reference = ses_message['mail']['messageId'] - if notifications_dao.update_notification_status_by_reference( + if not notifications_dao.update_notification_status_by_reference( reference, notification_status, notification_statistics_status - ) == 0: + ): message = "SES callback failed: notification either not found or already updated " \ "from sending. Status {}".format(notification_status) current_app.logger.info( diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index a69e62f71..1a55e3849 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -64,8 +64,7 @@ def test_should_by_able_to_update_status_by_id(sample_template, sample_job, mmg_ notification = Notification(**data) dao_create_notification(notification, sample_template.template_type, 'mmg') assert Notification.query.get(notification.id).status == 'sending' - count = update_notification_status_by_id(notification.id, 'delivered', 'delivered') - assert count == 1 + assert update_notification_status_by_id(notification.id, 'delivered', 'delivered') assert Notification.query.get(notification.id).status == 'delivered' _assert_notification_stats(notification.service_id, sms_delivered=1, sms_requested=1, sms_failed=0) _assert_job_stats(notification.job_id, sent=1, count=1, delivered=1, failed=0) @@ -75,8 +74,7 @@ def test_should_not_update_status_by_id_if_not_sending_and_does_not_update_job(n notification = sample_notification(notify_db, notify_db_session, status='delivered') job = Job.query.get(notification.job_id) assert Notification.query.get(notification.id).status == 'delivered' - count = update_notification_status_by_id(notification.id, 'failed', 'failure') - assert count == 0 + assert not update_notification_status_by_id(notification.id, 'failed', 'failure') assert Notification.query.get(notification.id).status == 'delivered' assert job == Job.query.get(notification.job_id) @@ -86,14 +84,12 @@ def test_should_by_able_to_update_status_by_id_from_pending_to_delivered(sample_ notification = Notification(**data) dao_create_notification(notification, sample_template.template_type, 'mmg') assert Notification.query.get(notification.id).status == 'sending' - count = update_notification_status_by_id(notification_id=notification.id, status='pending') - assert count == 1 + assert update_notification_status_by_id(notification_id=notification.id, status='pending') assert Notification.query.get(notification.id).status == 'pending' _assert_notification_stats(notification.service_id, sms_requested=1, sms_delivered=0, sms_failed=0) _assert_job_stats(sample_job.id, sent=1, count=1, delivered=0, failed=0) - count = update_notification_status_by_id(notification.id, 'delivered', 'delivered') - assert count == 1 + assert update_notification_status_by_id(notification.id, 'delivered', 'delivered') assert Notification.query.get(notification.id).status == 'delivered' _assert_notification_stats(notification.service_id, sms_requested=1, sms_delivered=1, sms_failed=0) _assert_job_stats(notification.job_id, sent=1, count=1, delivered=1, failed=0) @@ -104,15 +100,15 @@ def test_should_by_able_to_update_status_by_id_from_pending_to_temporary_failure notification = Notification(**data) dao_create_notification(notification, sample_template.template_type, 'mmg') assert Notification.query.get(notification.id).status == 'sending' - count = update_notification_status_by_id(notification_id=notification.id, status='pending') - assert count == 1 + assert update_notification_status_by_id(notification_id=notification.id, status='pending') assert Notification.query.get(notification.id).status == 'pending' _assert_notification_stats(notification.service_id, sms_requested=1, sms_delivered=0, sms_failed=0) _assert_job_stats(notification.job_id, sent=1, count=1, delivered=0, failed=0) - count = update_notification_status_by_id(notification.id, status='permanent-failure', - notification_statistics_status='failure') - assert count == 1 + assert update_notification_status_by_id( + notification.id, + status='permanent-failure', + notification_statistics_status='failure') assert Notification.query.get(notification.id).status == 'temporary-failure' _assert_notification_stats(notification.service_id, sms_delivered=0, sms_requested=1, sms_failed=1) _assert_job_stats(sample_job.id, sent=1, count=1, delivered=0, failed=1) @@ -124,9 +120,11 @@ def test_should_by_able_to_update_status_by_id_from_sending_to_permanent_failure dao_create_notification(notification, sample_template.template_type, 'firetext') assert Notification.query.get(notification.id).status == 'sending' - count = update_notification_status_by_id(notification.id, status='permanent-failure', - notification_statistics_status='failure') - assert count == 1 + assert update_notification_status_by_id( + notification.id, + status='permanent-failure', + notification_statistics_status='failure' + ) assert Notification.query.get(notification.id).status == 'permanent-failure' _assert_notification_stats(notification.service_id, sms_requested=1, sms_delivered=0, sms_failed=1) _assert_job_stats(sample_job.id, sent=1, count=1, delivered=0, failed=1) @@ -154,8 +152,7 @@ def test_should_not_update_status_one_notification_status_is_delivered(sample_em def test_should_be_able_to_record_statistics_failure_for_sms(sample_notification): assert Notification.query.get(sample_notification.id).status == 'sending' - count = update_notification_status_by_id(sample_notification.id, 'permanent-failure', 'failure') - assert count == 1 + assert update_notification_status_by_id(sample_notification.id, 'permanent-failure', 'failure') assert Notification.query.get(sample_notification.id).status == 'permanent-failure' _assert_notification_stats(sample_notification.service_id, sms_requested=1, sms_delivered=0, sms_failed=1) _assert_job_stats(sample_notification.job_id, sent=1, count=1, delivered=0, failed=1) @@ -175,11 +172,11 @@ def test_should_be_able_to_record_statistics_failure_for_email(sample_email_temp def test_should_return_zero_count_if_no_notification_with_id(): - assert update_notification_status_by_id(str(uuid.uuid4()), 'delivered', 'delivered') == 0 + assert not update_notification_status_by_id(str(uuid.uuid4()), 'delivered', 'delivered') def test_should_return_zero_count_if_no_notification_with_reference(): - assert update_notification_status_by_reference('something', 'delivered', 'delivered') == 0 + assert not update_notification_status_by_reference('something', 'delivered', 'delivered') def test_should_be_able_to_get_statistics_for_a_service(sample_template, mmg_provider):