From 0fe0c1d2b439a39030f7539e1aa36468f3803063 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Thu, 19 May 2016 10:46:03 +0100 Subject: [PATCH] Added job row number to the notification for csv jobs. All tests passing. --- app/celery/tasks.py | 7 ++-- app/dao/notifications_dao.py | 4 +-- app/models.py | 1 + .../versions/0019_add_job_row_number.py | 26 +++++++++++++++ requirements.txt | 2 +- tests/app/celery/test_tasks.py | 20 ++++++++--- tests/app/conftest.py | 3 ++ tests/app/notifications/test_rest.py | 33 +++++++++++++++---- 8 files changed, 81 insertions(+), 15 deletions(-) create mode 100644 migrations/versions/0019_add_job_row_number.py diff --git a/app/celery/tasks.py b/app/celery/tasks.py index c796a2960..cf6c9dcdc 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -148,17 +148,18 @@ def process_job(job_id): dao_get_template_by_id(job.template_id, job.template_version).__dict__ ) - for recipient, personalisation in RecipientCSV( + for row_number, recipient, personalisation in RecipientCSV( s3.get_job_from_s3(str(service.id), str(job_id)), template_type=template.template_type, placeholders=template.placeholders - ).recipients_and_personalisation: + ).enumerated_recipients_and_personalisation: encrypted = encryption.encrypt({ 'template': str(template.id), 'template_version': job.template_version, 'job': str(job.id), 'to': recipient, + 'row_number': row_number, 'personalisation': { key: personalisation.get(key) for key in template.placeholders @@ -242,6 +243,7 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): to=notification['to'], service_id=service_id, job_id=notification.get('job', None), + job_row_number=notification.get('row_number', None), status='failed' if restricted else 'sending', created_at=datetime.strptime(created_at, DATETIME_FORMAT), sent_at=sent_at, @@ -307,6 +309,7 @@ def send_email(service_id, notification_id, from_address, encrypted_notification to=notification['to'], service_id=service_id, job_id=notification.get('job', None), + job_row_number=notification.get('row_number', None), status='failed' if restricted else 'sending', created_at=datetime.strptime(created_at, DATETIME_FORMAT), sent_at=sent_at, diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 43a8b015c..48c393c26 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -1,4 +1,4 @@ -from sqlalchemy import (desc, func, Integer, and_) +from sqlalchemy import (desc, func, Integer, and_, asc) from sqlalchemy.sql.expression import cast from datetime import ( @@ -227,7 +227,7 @@ def get_notifications_for_job(service_id, job_id, filter_dict=None, page=1, page page_size = current_app.config['PAGE_SIZE'] query = Notification.query.filter_by(service_id=service_id, job_id=job_id) query = filter_query(query, filter_dict) - return query.order_by(desc(Notification.created_at)).paginate( + return query.order_by(asc(Notification.job_row_number)).paginate( page=page, per_page=page_size ) diff --git a/app/models.py b/app/models.py index 33a525b7c..120f7ef40 100644 --- a/app/models.py +++ b/app/models.py @@ -317,6 +317,7 @@ class Notification(db.Model): to = db.Column(db.String, nullable=False) job_id = db.Column(UUID(as_uuid=True), db.ForeignKey('jobs.id'), index=True, unique=False) job = db.relationship('Job', backref=db.backref('notifications', lazy='dynamic')) + job_row_number = db.Column(db.Integer, nullable=True) service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, unique=False) service = db.relationship('Service') template_id = db.Column(UUID(as_uuid=True), db.ForeignKey('templates.id'), index=True, unique=False) diff --git a/migrations/versions/0019_add_job_row_number.py b/migrations/versions/0019_add_job_row_number.py new file mode 100644 index 000000000..631773d59 --- /dev/null +++ b/migrations/versions/0019_add_job_row_number.py @@ -0,0 +1,26 @@ +"""empty message + +Revision ID: 0019_add_job_row_number +Revises: 0018_remove_subject_uniqueness +Create Date: 2016-05-18 15:04:24.513071 + +""" + +# revision identifiers, used by Alembic. +revision = '0019_add_job_row_number' +down_revision = '0018_remove_subject_uniqueness' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.add_column('notifications', sa.Column('job_row_number', sa.Integer(), nullable=True)) + ### end Alembic commands ### + + +def downgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.drop_column('notifications', 'job_row_number') + ### end Alembic commands ### diff --git a/requirements.txt b/requirements.txt index 3e55a74b1..7526f9afc 100644 --- a/requirements.txt +++ b/requirements.txt @@ -22,4 +22,4 @@ statsd==3.2.1 git+https://github.com/alphagov/notifications-python-client.git@1.0.0#egg=notifications-python-client==1.0.0 -git+https://github.com/alphagov/notifications-utils.git@5.2.6#egg=notifications-utils==5.2.6 +git+https://github.com/alphagov/notifications-utils.git@5.2.8#egg=notifications-utils==5.2.8 diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 481976b88..d6aa09d9a 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -119,6 +119,7 @@ def test_should_process_sms_job(sample_job, mocker, mock_celery_remove_job): assert encryption.encrypt.call_args[0][0]['template'] == str(sample_job.template.id) assert encryption.encrypt.call_args[0][0]['template_version'] == sample_job.template.version assert encryption.encrypt.call_args[0][0]['personalisation'] == {} + assert encryption.encrypt.call_args[0][0]['row_number'] == 0 tasks.send_sms.apply_async.assert_called_once_with( (str(sample_job.service_id), "uuid", @@ -559,7 +560,11 @@ def test_should_not_send_email_if_restricted_service_and_invalid_email_address(n def test_should_send_template_to_correct_sms_provider_and_persist_with_job_id(sample_job, mocker): - notification = _notification_json(sample_job.template, to="+447234123123", job_id=sample_job.id) + notification = _notification_json( + sample_job.template, + to="+447234123123", + job_id=sample_job.id, + row_number=2) mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.mmg_client.send_sms') mocker.patch('app.mmg_client.get_name', return_value="mmg") @@ -586,10 +591,15 @@ def test_should_send_template_to_correct_sms_provider_and_persist_with_job_id(sa assert persisted_notification.sent_at > now assert persisted_notification.created_at == now assert persisted_notification.sent_by == 'mmg' + assert persisted_notification.job_row_number == 2 def test_should_use_email_template_and_persist(sample_email_template_with_placeholders, mocker): - notification = _notification_json(sample_email_template_with_placeholders, "my_email@my_email.com", {"name": "Jo"}) + notification = _notification_json( + sample_email_template_with_placeholders, + "my_email@my_email.com", + {"name": "Jo"}, + row_number=1) mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.timing_with_dates') @@ -644,6 +654,7 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh assert persisted_notification.sent_at > now assert persisted_notification.status == 'sending' assert persisted_notification.sent_by == 'ses' + assert persisted_notification.job_row_number == 1 def test_send_email_should_use_template_version_from_job_not_latest(sample_email_template, mocker): @@ -952,7 +963,6 @@ def test_email_reset_password_should_send_email(notify_db, notify_db_session, no aws_ses_client.send_email(current_app.config['VERIFY_CODE_FROM_EMAIL_ADDRESS'], reset_password_message['to'], "Reset password for GOV.UK Notify", - message) @@ -983,7 +993,7 @@ def test_process_email_job_should_use_reply_to_email_if_present(sample_email_job ) -def _notification_json(template, to, personalisation=None, job_id=None): +def _notification_json(template, to, personalisation=None, job_id=None, row_number=None): notification = { "template": template.id, "template_version": template.version, @@ -993,4 +1003,6 @@ def _notification_json(template, to, personalisation=None, job_id=None): notification.update({"personalisation": personalisation}) if job_id: notification.update({"job": job_id}) + if row_number: + notification['row_number'] = row_number return notification diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 0d9aeb8ff..b9bcf91a3 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -310,6 +310,7 @@ def sample_notification(notify_db, service=None, template=None, job=None, + job_row_number=None, to_field=None, status='sending', reference=None, @@ -347,6 +348,8 @@ def sample_notification(notify_db, 'created_at': created_at, 'content_char_count': content_char_count } + if job_row_number: + data['job_row_number'] = job_row_number notification = Notification(**data) if create: dao_create_notification(notification, template.template_type, provider.identifier) diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index d96881ea3..c060766df 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -129,20 +129,38 @@ def test_get_all_notifications_for_service_in_order(notify_api, notify_db, notif assert response.status_code == 200 -def test_get_all_notifications_for_job_in_order(notify_api, notify_db, notify_db_session, sample_service): +def test_get_all_notifications_for_job_in_order_of_job_number(notify_api, + notify_db, + notify_db_session, + sample_service): with notify_api.test_request_context(): with notify_api.test_client() as client: main_job = create_sample_job(notify_db, notify_db_session, service=sample_service) another_job = create_sample_job(notify_db, notify_db_session, service=sample_service) notification_1 = create_sample_notification( - notify_db, notify_db_session, job=main_job, to_field="1", created_at=datetime.utcnow() + notify_db, + notify_db_session, + job=main_job, + to_field="1", + created_at=datetime.utcnow(), + job_row_number=1 ) notification_2 = create_sample_notification( - notify_db, notify_db_session, job=main_job, to_field="2", created_at=datetime.utcnow() + notify_db, + notify_db_session, + job=main_job, + to_field="2", + created_at=datetime.utcnow(), + job_row_number=2 ) notification_3 = create_sample_notification( - notify_db, notify_db_session, job=main_job, to_field="3", created_at=datetime.utcnow() + notify_db, + notify_db_session, + job=main_job, + to_field="3", + created_at=datetime.utcnow(), + job_row_number=3 ) create_sample_notification(notify_db, notify_db_session, job=another_job) @@ -154,9 +172,12 @@ def test_get_all_notifications_for_job_in_order(notify_api, notify_db, notify_db resp = json.loads(response.get_data(as_text=True)) assert len(resp['notifications']) == 3 - assert resp['notifications'][0]['to'] == notification_3.to + assert resp['notifications'][0]['to'] == notification_1.to + assert resp['notifications'][0]['job_row_number'] == notification_1.job_row_number assert resp['notifications'][1]['to'] == notification_2.to - assert resp['notifications'][2]['to'] == notification_1.to + assert resp['notifications'][1]['job_row_number'] == notification_2.job_row_number + assert resp['notifications'][2]['to'] == notification_3.to + assert resp['notifications'][2]['job_row_number'] == notification_3.job_row_number assert response.status_code == 200