Merge pull request #327 from alphagov/add_job_row_number

Added job row number to the notification for csv jobs. All tests pass…

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,

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
     )

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)

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 ###

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

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

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)

tests/app/notifications/test_rest.py

@@ -130,20 +130,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)
 
@@ -155,9 +173,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