From d40d520d2cc76c3e397f8017815e1b10210035d3 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Wed, 7 Feb 2018 14:14:02 +0000 Subject: [PATCH 1/4] Add Organisation model and migration Now that we have renamed the 'old' organisation model to email_branding, we can create a new organisation model. --- app/models.py | 9 +++++ migrations/versions/0163_add_new_org_model.py | 34 +++++++++++++++++++ 2 files changed, 43 insertions(+) create mode 100644 migrations/versions/0163_add_new_org_model.py diff --git a/app/models.py b/app/models.py index 580626b4b..6dbc64cd5 100644 --- a/app/models.py +++ b/app/models.py @@ -303,6 +303,15 @@ class Service(db.Model, Versioned): return permission in [p.permission for p in self.permissions] +class Organisation(db.Model): + __tablename__ = "organisation" + id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4, unique=False) + name = db.Column(db.String(255), nullable=False, unique=True, index=True) + active = db.Column(db.Boolean, nullable=False, default=True) + created_at = db.Column(db.DateTime, nullable=False, default=datetime.datetime.utcnow) + updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow) + + class AnnualBilling(db.Model): __tablename__ = "annual_billing" id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4, unique=False) diff --git a/migrations/versions/0163_add_new_org_model.py b/migrations/versions/0163_add_new_org_model.py new file mode 100644 index 000000000..da2232011 --- /dev/null +++ b/migrations/versions/0163_add_new_org_model.py @@ -0,0 +1,34 @@ +""" + +Revision ID: 0163_add_new_org_model +Revises: 0162_remove_org +Create Date: 2018-02-07 14:03:00.804849 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +revision = '0163_add_new_org_model' +down_revision = '0162_remove_org' + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.create_table('organisation', + sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('name', sa.String(length=255), nullable=False), + sa.Column('active', sa.Boolean(), nullable=False), + sa.Column('created_at', sa.DateTime(), nullable=False), + sa.Column('updated_at', sa.DateTime(), nullable=True), + sa.PrimaryKeyConstraint('id') + ) + op.create_index(op.f('ix_organisation_name'), 'organisation', ['name'], unique=True) + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_index(op.f('ix_organisation_name'), table_name='organisation') + op.drop_table('organisation') + # ### end Alembic commands ### From 6a79eedbcec5bf9a68bc1954c59ee47dbd79bc83 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Wed, 7 Feb 2018 14:43:09 +0000 Subject: [PATCH 2/4] Add Organisation DAO --- app/dao/organisation_dao.py | 25 ++++++++++++ tests/app/dao/test_organisation_dao.py | 53 ++++++++++++++++++++++++++ tests/app/db.py | 13 +++++++ 3 files changed, 91 insertions(+) create mode 100644 app/dao/organisation_dao.py create mode 100644 tests/app/dao/test_organisation_dao.py diff --git a/app/dao/organisation_dao.py b/app/dao/organisation_dao.py new file mode 100644 index 000000000..fd5b863b2 --- /dev/null +++ b/app/dao/organisation_dao.py @@ -0,0 +1,25 @@ +from app import db +from app.dao.dao_utils import transactional +from app.models import Organisation + + +def dao_get_organisations(): + return Organisation.query.order_by( + Organisation.active.desc(), Organisation.name.asc() + ).all() + + +def dao_get_organisation_by_id(organisation_id): + return Organisation.query.filter_by(id=organisation_id).one() + + +@transactional +def dao_create_organisation(organisation): + db.session.add(organisation) + + +@transactional +def dao_update_organisation(organisation, **kwargs): + for key, value in kwargs.items(): + setattr(organisation, key, value) + db.session.add(organisation) diff --git a/tests/app/dao/test_organisation_dao.py b/tests/app/dao/test_organisation_dao.py new file mode 100644 index 000000000..0ab662e31 --- /dev/null +++ b/tests/app/dao/test_organisation_dao.py @@ -0,0 +1,53 @@ +from app.dao.organisation_dao import ( + dao_get_organisations, + dao_get_organisation_by_id, + dao_update_organisation, +) +from app.models import Organisation + +from tests.app.db import create_organisation + + +def test_get_organisations_gets_all_organisations_alphabetically_with_active_organisations_first( + notify_db, + notify_db_session +): + m_active_org = create_organisation(name='m_active_organisation') + z_inactive_org = create_organisation(name='z_inactive_organisation', active=False) + a_inactive_org = create_organisation(name='a_inactive_organisation', active=False) + z_active_org = create_organisation(name='z_active_organisation') + a_active_org = create_organisation(name='a_active_organisation') + + organisations = dao_get_organisations() + + assert len(organisations) == 5 + assert organisations[0] == a_active_org + assert organisations[1] == m_active_org + assert organisations[2] == z_active_org + assert organisations[3] == a_inactive_org + assert organisations[4] == z_inactive_org + + +def test_get_organisation_by_id_gets_correct_organisation(notify_db, notify_db_session): + organisation = create_organisation() + + organisation_from_db = dao_get_organisation_by_id(organisation.id) + + assert organisation_from_db == organisation + + +def test_update_organisation(notify_db, notify_db_session): + updated_name = 'new name' + create_organisation() + + organisation = Organisation.query.all() + + assert len(organisation) == 1 + assert organisation[0].name != updated_name + + dao_update_organisation(organisation[0], name=updated_name) + + organisation = Organisation.query.all() + + assert len(organisation) == 1 + assert organisation[0].name == updated_name diff --git a/tests/app/db.py b/tests/app/db.py index d35d2b275..bd55e40a3 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -14,6 +14,7 @@ from app.models import ( MonthlyBilling, Notification, EmailBranding, + Organisation, Rate, Service, ServiceEmailReplyTo, @@ -41,6 +42,7 @@ from app.dao.services_dao import dao_create_service from app.dao.service_permissions_dao import dao_add_service_permission from app.dao.inbound_sms_dao import dao_create_inbound_sms from app.dao.email_branding_dao import dao_create_email_branding +from app.dao.organisation_dao import dao_create_organisation def create_user(mobile_number="+447700900986", email="notify@digital.cabinet-office.gov.uk", state='active'): @@ -479,3 +481,14 @@ def create_letter_rate( db.session.commit() return rate + + +def create_organisation(name='test_org_1', active=True): + data = { + 'name': name, + 'active': active + } + organisation = Organisation(**data) + dao_create_organisation(organisation) + + return organisation From 269923ba288d11ff81ee3efdb626909fc435f547 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Thu, 8 Feb 2018 14:54:08 +0000 Subject: [PATCH 3/4] Add Organisations endpoints As part of this we also needed to add: - schemas for validation - serialize method for Organisation model --- app/__init__.py | 4 ++ app/models.py | 9 +++ app/organisation/__init__.py | 0 app/organisation/organisation_schema.py | 21 ++++++ app/organisation/rest.py | 57 ++++++++++++++++ tests/app/organisation/test_rest.py | 90 +++++++++++++++++++++++++ 6 files changed, 181 insertions(+) create mode 100644 app/organisation/__init__.py create mode 100644 app/organisation/organisation_schema.py create mode 100644 app/organisation/rest.py create mode 100644 tests/app/organisation/test_rest.py diff --git a/app/__init__.py b/app/__init__.py index 51c98808d..79cc48cf3 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -107,6 +107,7 @@ def register_blueprint(application): from app.authentication.auth import requires_admin_auth, requires_auth, requires_no_auth from app.letters.rest import letter_job from app.billing.rest import billing_blueprint + from app.organisation.rest import organisation_blueprint service_blueprint.before_request(requires_admin_auth) application.register_blueprint(service_blueprint, url_prefix='/service') @@ -177,6 +178,9 @@ def register_blueprint(application): service_callback_blueprint.before_request(requires_admin_auth) application.register_blueprint(service_callback_blueprint) + organisation_blueprint.before_request(requires_admin_auth) + application.register_blueprint(organisation_blueprint, url_prefix='/organisations') + def register_v2_blueprints(application): from app.v2.inbound_sms.get_inbound_sms import v2_inbound_sms_blueprint as get_inbound_sms diff --git a/app/models.py b/app/models.py index 6dbc64cd5..61d402414 100644 --- a/app/models.py +++ b/app/models.py @@ -311,6 +311,15 @@ class Organisation(db.Model): created_at = db.Column(db.DateTime, nullable=False, default=datetime.datetime.utcnow) updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow) + def serialize(self): + serialized = { + "id": str(self.id), + "name": self.name, + "active": self.active, + } + + return serialized + class AnnualBilling(db.Model): __tablename__ = "annual_billing" diff --git a/app/organisation/__init__.py b/app/organisation/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/app/organisation/organisation_schema.py b/app/organisation/organisation_schema.py new file mode 100644 index 000000000..8640167b9 --- /dev/null +++ b/app/organisation/organisation_schema.py @@ -0,0 +1,21 @@ +post_create_organisation_schema = { + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "POST organisation schema", + "type": "object", + "properties": { + "name": {"type": "string"}, + "active": {"type": ["boolean", "null"]} + }, + "required": ["name"] +} + +post_update_organisation_schema = { + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "POST organisation schema", + "type": "object", + "properties": { + "name": {"type": ["string", "null"]}, + "active": {"type": ["boolean", "null"]} + }, + "required": [] +} diff --git a/app/organisation/rest.py b/app/organisation/rest.py new file mode 100644 index 000000000..659c073ae --- /dev/null +++ b/app/organisation/rest.py @@ -0,0 +1,57 @@ +from flask import Blueprint, jsonify, request + +from app.dao.organisation_dao import ( + dao_create_organisation, + dao_get_organisations, + dao_get_organisation_by_id, + dao_update_organisation, +) +from app.errors import register_errors +from app.models import Organisation +from app.organisation.organisation_schema import ( + post_create_organisation_schema, + post_update_organisation_schema, +) +from app.schema_validation import validate + +organisation_blueprint = Blueprint('organisation', __name__) +register_errors(organisation_blueprint) + + +@organisation_blueprint.route('', methods=['GET']) +def get_organisations(): + organisations = [ + org.serialize() for org in dao_get_organisations() + ] + + return jsonify(organisations) + + +@organisation_blueprint.route('/', methods=['GET']) +def get_organisation_by_id(organisation_id): + organisation = dao_get_organisation_by_id(organisation_id) + return jsonify(organisation.serialize()) + + +@organisation_blueprint.route('', methods=['POST']) +def create_organisation(): + data = request.get_json() + + validate(data, post_create_organisation_schema) + + organisation = Organisation(**data) + dao_create_organisation(organisation) + + return jsonify(organisation.serialize()), 201 + + +@organisation_blueprint.route('/', methods=['POST']) +def update_organisation(organisation_id): + data = request.get_json() + + validate(data, post_update_organisation_schema) + + fetched_organisation = dao_get_organisation_by_id(organisation_id) + dao_update_organisation(fetched_organisation, **data) + + return jsonify(fetched_organisation.serialize()), 200 diff --git a/tests/app/organisation/test_rest.py b/tests/app/organisation/test_rest.py new file mode 100644 index 000000000..949f828ef --- /dev/null +++ b/tests/app/organisation/test_rest.py @@ -0,0 +1,90 @@ +from app.models import Organisation +from tests.app.db import create_organisation + + +def test_get_all_organisations(admin_request, notify_db_session): + create_organisation(name='inactive org', active=False) + create_organisation(name='active org') + + response = admin_request.get( + 'organisation.get_organisations', + _expected_status=200 + ) + + assert len(response) == 2 + assert response[0]['name'] == 'active org' + assert response[0]['active'] is True + assert response[1]['name'] == 'inactive org' + assert response[1]['active'] is False + + +def test_get_organisation_by_id(admin_request, notify_db_session): + org = create_organisation() + + response = admin_request.get( + 'organisation.get_organisation_by_id', + _expected_status=200, + organisation_id=org.id + ) + + assert set(response.keys()) == {'id', 'name', 'active'} + assert response['id'] == str(org.id) + assert response['name'] == 'test_org_1' + assert response['active'] is True + + +def test_post_create_organisation(admin_request, notify_db_session): + data = { + 'name': 'test organisation', + 'active': True + } + + response = admin_request.post( + 'organisation.create_organisation', + _data=data, + _expected_status=201 + ) + + organisation = Organisation.query.all() + + assert data['name'] == response['name'] + assert data['active'] == response['active'] + assert len(organisation) == 1 + + +def test_post_create_organisation_with_missing_name_gives_validation_error(admin_request, notify_db_session): + data = { + 'active': False + } + + response = admin_request.post( + 'organisation.create_organisation', + _data=data, + _expected_status=400 + ) + + assert len(response['errors']) == 1 + assert response['errors'][0]['error'] == 'ValidationError' + assert response['errors'][0]['message'] == 'name is a required property' + + +def test_post_update_organisation_updates_fields(admin_request, notify_db_session): + org = create_organisation() + data = { + 'name': 'new organisation name', + 'active': False + } + + admin_request.post( + 'organisation.update_organisation', + _data=data, + organisation_id=org.id, + _expected_status=200 + ) + + organisation = Organisation.query.all() + + assert len(organisation) == 1 + assert organisation[0].id == org.id + assert organisation[0].name == data['name'] + assert organisation[0].active == data['active'] From 4a14225d04b985c80edb0c49c302035feb7c149a Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Fri, 9 Feb 2018 11:17:13 +0000 Subject: [PATCH 4/4] Change Organisation DAO update method - Changed the organisation DAO update method to only make 1 query - Updated the update rest endpoint to not return an organisation when the update is successful --- app/dao/organisation_dao.py | 8 ++++---- app/organisation/rest.py | 12 ++++++------ tests/app/dao/test_organisation_dao.py | 2 +- tests/app/organisation/test_rest.py | 17 ++++++++++++++++- 4 files changed, 27 insertions(+), 12 deletions(-) diff --git a/app/dao/organisation_dao.py b/app/dao/organisation_dao.py index fd5b863b2..80708f266 100644 --- a/app/dao/organisation_dao.py +++ b/app/dao/organisation_dao.py @@ -19,7 +19,7 @@ def dao_create_organisation(organisation): @transactional -def dao_update_organisation(organisation, **kwargs): - for key, value in kwargs.items(): - setattr(organisation, key, value) - db.session.add(organisation) +def dao_update_organisation(organisation_id, **kwargs): + return Organisation.query.filter_by(id=organisation_id).update( + kwargs + ) diff --git a/app/organisation/rest.py b/app/organisation/rest.py index 659c073ae..f27525b22 100644 --- a/app/organisation/rest.py +++ b/app/organisation/rest.py @@ -6,7 +6,7 @@ from app.dao.organisation_dao import ( dao_get_organisation_by_id, dao_update_organisation, ) -from app.errors import register_errors +from app.errors import register_errors, InvalidRequest from app.models import Organisation from app.organisation.organisation_schema import ( post_create_organisation_schema, @@ -48,10 +48,10 @@ def create_organisation(): @organisation_blueprint.route('/', methods=['POST']) def update_organisation(organisation_id): data = request.get_json() - validate(data, post_update_organisation_schema) + result = dao_update_organisation(organisation_id, **data) - fetched_organisation = dao_get_organisation_by_id(organisation_id) - dao_update_organisation(fetched_organisation, **data) - - return jsonify(fetched_organisation.serialize()), 200 + if result: + return '', 204 + else: + raise InvalidRequest("Organisation not found", 404) diff --git a/tests/app/dao/test_organisation_dao.py b/tests/app/dao/test_organisation_dao.py index 0ab662e31..a72670cb8 100644 --- a/tests/app/dao/test_organisation_dao.py +++ b/tests/app/dao/test_organisation_dao.py @@ -45,7 +45,7 @@ def test_update_organisation(notify_db, notify_db_session): assert len(organisation) == 1 assert organisation[0].name != updated_name - dao_update_organisation(organisation[0], name=updated_name) + dao_update_organisation(organisation[0].id, **{'name': updated_name}) organisation = Organisation.query.all() diff --git a/tests/app/organisation/test_rest.py b/tests/app/organisation/test_rest.py index 949f828ef..d9628016c 100644 --- a/tests/app/organisation/test_rest.py +++ b/tests/app/organisation/test_rest.py @@ -79,7 +79,7 @@ def test_post_update_organisation_updates_fields(admin_request, notify_db_sessio 'organisation.update_organisation', _data=data, organisation_id=org.id, - _expected_status=200 + _expected_status=204 ) organisation = Organisation.query.all() @@ -88,3 +88,18 @@ def test_post_update_organisation_updates_fields(admin_request, notify_db_sessio assert organisation[0].id == org.id assert organisation[0].name == data['name'] assert organisation[0].active == data['active'] + + +def test_post_update_organisation_gives_404_status_if_org_does_not_exist(admin_request, notify_db_session): + data = {'name': 'new organisation name'} + + admin_request.post( + 'organisation.update_organisation', + _data=data, + organisation_id='31d42ce6-3dac-45a7-95cb-94423d5ca03c', + _expected_status=404 + ) + + organisation = Organisation.query.all() + + assert not organisation