diff --git a/app/aws/s3.py b/app/aws/s3.py index fd316cf1e..2aa7aac39 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -1,16 +1,23 @@ from boto3 import resource +from flask import current_app + +FILE_LOCATION_STRUCTURE = 'service-{}-notify/{}.csv' -def get_s3_job_object(bucket_name, job_id): +def get_s3_job_object(bucket_name, file_location): s3 = resource('s3') - return s3.Object(bucket_name, '{}.csv'.format(job_id)) + return s3.Object(bucket_name, file_location) -def get_job_from_s3(bucket_name, job_id): - obj = get_s3_job_object(bucket_name, job_id) +def get_job_from_s3(service_id, job_id): + bucket_name = current_app.config['CSV_UPLOAD_BUCKET_NAME'] + file_location = FILE_LOCATION_STRUCTURE.format(service_id, job_id) + obj = get_s3_job_object(bucket_name, file_location) return obj.get()['Body'].read().decode('utf-8') -def remove_job_from_s3(bucket_name, job_id): - obj = get_s3_job_object(bucket_name, job_id) +def remove_job_from_s3(service_id, job_id): + bucket_name = current_app.config['CSV_UPLOAD_BUCKET_NAME'] + file_location = FILE_LOCATION_STRUCTURE.format(service_id, job_id) + obj = get_s3_job_object(bucket_name, file_location) return obj.delete() diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 8ee59a00a..57868a070 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -146,7 +146,7 @@ def process_job(job_id): ) for recipient, personalisation in RecipientCSV( - s3.get_job_from_s3(job.bucket_name, job_id), + s3.get_job_from_s3(str(service.id), str(job_id)), template_type=template.template_type, placeholders=template.placeholders ).recipients_and_personalisation: @@ -191,7 +191,7 @@ def process_job(job_id): @notify_celery.task(name="remove-job") def remove_job(job_id): job = dao_get_job_by_id(job_id) - s3.remove_job_from_s3(job.bucket_name, job_id) + s3.remove_job_from_s3(job.service.id, str(job_id)) current_app.logger.info("Job {} has been removed from s3.".format(job_id)) diff --git a/app/models.py b/app/models.py index 721bb4a08..c41cbac97 100644 --- a/app/models.py +++ b/app/models.py @@ -171,8 +171,6 @@ class Job(db.Model): id = db.Column(UUID(as_uuid=True), primary_key=True) original_file_name = db.Column(db.String, nullable=False) - bucket_name = db.Column(db.String, nullable=False) - file_name = db.Column(db.String, nullable=False) service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, unique=False, nullable=False) service = db.relationship('Service', backref=db.backref('jobs', lazy='dynamic')) template_id = db.Column(db.BigInteger, db.ForeignKey('templates.id'), index=True, unique=False) diff --git a/config.py b/config.py index 01d61ce5a..43feb3325 100644 --- a/config.py +++ b/config.py @@ -80,17 +80,26 @@ class Config(object): TWILIO_NUMBER = os.getenv('TWILIO_NUMBER') FIRETEXT_NUMBER = os.getenv('FIRETEXT_NUMBER') FIRETEXT_API_KEY = os.getenv("FIRETEXT_API_KEY") + CSV_UPLOAD_BUCKET_NAME = '{}-notifications-csv-upload'.format( + os.environ['NOTIFY_API_ENVIRONMENT'].split('config.')[1].lower()) class Development(Config): DEBUG = True +class Preview(Config): + pass + + class Test(Development): pass + configs = { + 'development': 'config.Development', + 'test': 'config.Test', 'live': 'config_live.Live', 'staging': 'config_staging.Staging', - 'preview': 'config.Config' + 'preview': 'config.Preview' } diff --git a/migrations/versions/0046_remove_bucketname.py b/migrations/versions/0046_remove_bucketname.py new file mode 100644 index 000000000..d5cfd2377 --- /dev/null +++ b/migrations/versions/0046_remove_bucketname.py @@ -0,0 +1,28 @@ +"""empty message + +Revision ID: 0046_remove_bucketname +Revises: 0045_template_stats_update_time +Create Date: 2016-04-07 12:23:55.050714 + +""" + +# revision identifiers, used by Alembic. +revision = '0046_remove_bucketname' +down_revision = '0045_template_stats_update_time' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.drop_column('jobs', 'file_name') + op.drop_column('jobs', 'bucket_name') + ### end Alembic commands ### + + +def downgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.add_column('jobs', sa.Column('bucket_name', sa.VARCHAR(), autoincrement=False, nullable=False)) + op.add_column('jobs', sa.Column('file_name', sa.VARCHAR(), autoincrement=False, nullable=False)) + ### end Alembic commands ### diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 5fe212664..2f8fe5da0 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -76,8 +76,10 @@ def test_should_process_sms_job(sample_job, mocker, mock_celery_remove_job): mocker.patch('app.celery.tasks.create_uuid', return_value="uuid") process_job(sample_job.id) - - s3.get_job_from_s3.assert_called_once_with(sample_job.bucket_name, sample_job.id) + s3.get_job_from_s3.assert_called_once_with( + str(sample_job.service.id), + str(sample_job.id) + ) assert encryption.encrypt.call_args[0][0]['to'] == '+441234123123' assert encryption.encrypt.call_args[0][0]['personalisation'] == {} tasks.send_sms.apply_async.assert_called_once_with( @@ -191,7 +193,10 @@ def test_should_process_sms_job_if_exactly_on_send_limits(notify_db, process_job(job.id) - s3.get_job_from_s3.assert_called_once_with(job.bucket_name, job.id) + s3.get_job_from_s3.assert_called_once_with( + str(job.service.id), + str(job.id) + ) job = jobs_dao.dao_get_job_by_id(job.id) assert job.status == 'finished' tasks.send_email.apply_async.assert_called_with( @@ -212,7 +217,10 @@ def test_should_not_create_send_task_for_empty_file(sample_job, mocker, mock_cel process_job(sample_job.id) - s3.get_job_from_s3.assert_called_once_with(sample_job.bucket_name, sample_job.id) + s3.get_job_from_s3.assert_called_once_with( + str(sample_job.service.id), + str(sample_job.id) + ) job = jobs_dao.dao_get_job_by_id(sample_job.id) assert job.status == 'finished' tasks.send_sms.apply_async.assert_not_called @@ -227,7 +235,10 @@ def test_should_process_email_job(sample_email_job, mocker, mock_celery_remove_j process_job(sample_email_job.id) - s3.get_job_from_s3.assert_called_once_with(sample_email_job.bucket_name, sample_email_job.id) + s3.get_job_from_s3.assert_called_once_with( + str(sample_email_job.service.id), + str(sample_email_job.id) + ) assert encryption.encrypt.call_args[0][0]['to'] == 'test@test.com' assert encryption.encrypt.call_args[0][0]['personalisation'] == {} tasks.send_email.apply_async.assert_called_once_with( @@ -256,8 +267,8 @@ def test_should_process_all_sms_job(sample_job, process_job(sample_job_with_placeholdered_template.id) s3.get_job_from_s3.assert_called_once_with( - sample_job_with_placeholdered_template.bucket_name, - sample_job_with_placeholdered_template.id + str(sample_job_with_placeholdered_template.service.id), + str(sample_job_with_placeholdered_template.id) ) assert encryption.encrypt.call_args[0][0]['to'] == '+441234123120' assert encryption.encrypt.call_args[0][0]['personalisation'] == {'name': 'chris'} diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 73d645139..fa67cb573 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -44,17 +44,21 @@ def sample_user(notify_db, notify_db_session, mobile_numnber="+447700900986", email="notify@digital.cabinet-office.gov.uk"): - data = { - 'name': 'Test User', - 'email_address': email, - 'password': 'password', - 'mobile_number': mobile_numnber, - 'state': 'active' - } - usr = User.query.filter_by(email_address=email).first() - if not usr: - usr = User(**data) - save_model_user(usr) + try: + data = { + 'name': 'Test User', + 'email_address': email, + 'password': 'password', + 'mobile_number': mobile_numnber, + 'state': 'active' + } + usr = User.query.filter_by(email_address=email).first() + if not usr: + usr = User(**data) + save_model_user(usr) + except Exception: + import traceback + traceback.print_exc() return usr @@ -211,14 +215,11 @@ def sample_job(notify_db, template = sample_template(notify_db, notify_db_session, service=service) job_id = uuid.uuid4() - bucket_name = 'service-{}-notify'.format(service.id) - file_name = '{}.csv'.format(job_id) data = { 'id': uuid.uuid4(), 'service_id': service.id, + 'service': service, 'template_id': template.id, - 'bucket_name': bucket_name, - 'file_name': file_name, 'original_file_name': 'some.csv', 'notification_count': notification_count, 'created_at': created_at @@ -255,14 +256,11 @@ def sample_email_job(notify_db, notify_db_session, service=service) job_id = uuid.uuid4() - bucket_name = 'service-{}-notify'.format(service.id) - file_name = '{}.csv'.format(job_id) data = { 'id': uuid.uuid4(), 'service_id': service.id, + 'service': service, 'template_id': template.id, - 'bucket_name': bucket_name, - 'file_name': file_name, 'original_file_name': 'some.csv', 'notification_count': 1 } diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index de5894387..636bc163f 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -15,14 +15,10 @@ def test_create_job(sample_template): assert Job.query.count() == 0 job_id = uuid.uuid4() - bucket_name = 'service-{}-notify'.format(sample_template.service.id) - file_name = '{}.csv'.format(job_id) data = { 'id': job_id, 'service_id': sample_template.service.id, 'template_id': sample_template.id, - 'bucket_name': bucket_name, - 'file_name': file_name, 'original_file_name': 'some.csv', 'notification_count': 1 } diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index 636abc401..c2468fa8a 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -99,8 +99,6 @@ def test_create_job(notify_api, sample_template, mocker): 'service': str(sample_template.service.id), 'template': sample_template.id, 'original_file_name': 'thisisatest.csv', - 'bucket_name': 'service-{}-notify'.format(sample_template.service.id), - 'file_name': '{}.csv'.format(job_id), 'notification_count': 1 } path = '/service/{}/job'.format(sample_template.service.id) @@ -153,10 +151,8 @@ def test_create_job_returns_400_if_missing_data(notify_api, sample_template, moc app.celery.tasks.process_job.apply_async.assert_not_called() assert resp_json['result'] == 'error' assert 'Missing data for required field.' in resp_json['message']['original_file_name'] - assert 'Missing data for required field.' in resp_json['message']['file_name'] assert 'Missing data for required field.' in resp_json['message']['notification_count'] assert 'Missing data for required field.' in resp_json['message']['id'] - assert 'Missing data for required field.' in resp_json['message']['bucket_name'] def test_create_job_returns_404_if_missing_service(notify_api, sample_template, mocker):