From a6cd490942e9a6ae22adf261d035f83f39bc45c0 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 4 Aug 2016 12:35:47 +0100 Subject: [PATCH 1/4] add organisation and branding models a service now has branding and organisation_id columns, and two new tables have been aded to reflect these: * branding is a static types table referring to how a service wants their emails to be branded: * 'govuk' for GOV UK branding (default) * 'org' for organisational branding only * 'both' for co-branded output with both * organisation is a table defining an organisation's branding. this contains three entries, all of which are nullable * colour - a hex code for a coloured bar on the logo's left * logo - relative path for that org's logo image * name - the name to display on the right of the logo --- README.md | 2 +- app/models.py | 29 +++++++++- app/schemas.py | 8 ++- .../0046_organisations_and_branding.py | 57 +++++++++++++++++++ tests/conftest.py | 2 +- 5 files changed, 92 insertions(+), 6 deletions(-) create mode 100644 migrations/versions/0046_organisations_and_branding.py diff --git a/README.md b/README.md index d2dbc9892..8d31a6be3 100644 --- a/README.md +++ b/README.md @@ -22,7 +22,7 @@ Create a local environment.sh file containing the following: ``` echo " -export NOTIFY_API_ENVIRONMENT='config.Development' +export NOTIFY_ENVIRONMENT='development' export ADMIN_BASE_URL='http://localhost:6012' export ADMIN_CLIENT_SECRET='dev-notify-secret-key' export ADMIN_CLIENT_USER_NAME='dev-notify-admin' diff --git a/app/models.py b/app/models.py index f236841ac..e3aeb3d13 100644 --- a/app/models.py +++ b/app/models.py @@ -5,7 +5,7 @@ from sqlalchemy.dialects.postgresql import ( UUID, JSON ) -from sqlalchemy import UniqueConstraint +from sqlalchemy import UniqueConstraint, text from app.encryption import ( hashpw, @@ -74,6 +74,24 @@ user_to_service = db.Table( ) +BRANDING_GOVUK = 'govuk' +BRANDING_ORG = 'org' +BRANDING_BOTH = 'both' + + +class BrandingTypes(db.Model): + __tablename__ = 'branding_type' + name = db.Column(db.String(255), primary_key=True) + + +class Organisation(db.Model): + __tablename__ = 'organisation' + id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) + colour = db.Column(db.String(7), nullable=True) + logo = db.Column(db.String(255), nullable=True) + name = db.Column(db.String(255), nullable=True) + + class Service(db.Model, Versioned): __tablename__ = 'services' @@ -104,6 +122,15 @@ class Service(db.Model, Versioned): created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False) reply_to_email_address = db.Column(db.Text, index=False, unique=False, nullable=True) sms_sender = db.Column(db.String(11), nullable=True) + organisation_id = db.Column(UUID(as_uuid=True), db.ForeignKey('organisation.id'), index=True, nullable=True) + organisation = db.relationship('Organisation') + branding = db.Column( + db.String(255), + db.ForeignKey('branding_type.name'), + index=True, + nullable=False, + default=BRANDING_GOVUK + ) class ApiKey(db.Model, Versioned): diff --git a/app/schemas.py b/app/schemas.py index 659ea996f..ea71ac341 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -114,12 +114,13 @@ class ServiceSchema(BaseSchema): 'old_id', 'template_statistics', 'service_provider_stats', - 'service_notification_stats') + 'service_notification_stats', + 'organisation') strict = True @validates('sms_sender') def validate_sms_sender(self, value): - if value and not re.match('^[a-zA-Z0-9\s]+$', value): + if value and not re.match(r'^[a-zA-Z0-9\s]+$', value): raise ValidationError('Only alphanumeric characters allowed') @@ -136,7 +137,8 @@ class DetailedServiceSchema(BaseSchema): 'jobs', 'template_statistics', 'service_provider_stats', - 'service_notification_stats' + 'service_notification_stats', + 'organisation' ) diff --git a/migrations/versions/0046_organisations_and_branding.py b/migrations/versions/0046_organisations_and_branding.py new file mode 100644 index 000000000..e36d2ded6 --- /dev/null +++ b/migrations/versions/0046_organisations_and_branding.py @@ -0,0 +1,57 @@ +"""empty message + +Revision ID: 0046_organisations_and_branding +Revises: 0045_billable_units +Create Date: 2016-08-04 12:00:43.682610 + +""" + +# revision identifiers, used by Alembic. +revision = '0046_organisations_and_branding' +down_revision = '0045_billable_units' + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +def upgrade(): + op.create_table('branding_type', + sa.Column('name', sa.String(length=255), nullable=False), + sa.PrimaryKeyConstraint('name') + ) + op.create_table('organisation', + sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('colour', sa.String(length=7), nullable=True), + sa.Column('logo', sa.String(length=255), nullable=True), + sa.Column('name', sa.String(length=255), nullable=True), + sa.PrimaryKeyConstraint('id') + ) + + op.add_column('services', sa.Column('branding', sa.String(length=255))) + op.add_column('services', sa.Column('organisation_id', postgresql.UUID(as_uuid=True))) + op.add_column('services_history', sa.Column('branding', sa.String(length=255))) + op.add_column('services_history', sa.Column('organisation_id', postgresql.UUID(as_uuid=True))) + + op.execute("INSERT INTO branding_type VALUES ('govuk'), ('org'), ('both')") + op.execute("UPDATE services SET branding='govuk'") + op.execute("UPDATE services_history SET branding='govuk'") + + op.alter_column('services', 'branding', nullable=False) + op.alter_column('services_history', 'branding', nullable=False) + + op.create_index(op.f('ix_services_branding'), 'services', ['branding'], unique=False) + op.create_index(op.f('ix_services_organisation_id'), 'services', ['organisation_id'], unique=False) + op.create_index(op.f('ix_services_history_branding'), 'services_history', ['branding'], unique=False) + op.create_index(op.f('ix_services_history_organisation_id'), 'services_history', ['organisation_id'], unique=False) + + op.create_foreign_key(None, 'services', 'branding_type', ['branding'], ['name']) + op.create_foreign_key(None, 'services', 'organisation', ['organisation_id'], ['id']) + + +def downgrade(): + op.drop_column('services_history', 'organisation_id') + op.drop_column('services_history', 'branding') + op.drop_column('services', 'organisation_id') + op.drop_column('services', 'branding') + op.drop_table('organisation') + op.drop_table('branding_type') diff --git a/tests/conftest.py b/tests/conftest.py index 0f88fe667..cf1789d6e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -49,7 +49,7 @@ def notify_db_session(request, notify_db): def teardown(): notify_db.session.remove() for tbl in reversed(notify_db.metadata.sorted_tables): - if tbl.name not in ["provider_details", "key_types"]: + if tbl.name not in ["provider_details", "key_types", "branding_type"]: notify_db.engine.execute(tbl.delete()) notify_db.session.commit() From 8e46cd44fde85b19a228a71c53723e455c97775a Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 4 Aug 2016 15:57:41 +0100 Subject: [PATCH 2/4] make history meta handle nullable and default better * can now handle nullable foreign keys - would previously raise AttributeError * can now handle default values - we would previously not insert default values that hadn't been generated yet, which if the field is not nullable will result in IntegrityErrors. We were deliberately removing 'default' attributes from columns. Only remove them if nullable is set to true now. --- app/history_meta.py | 11 +++++++++-- tests/app/dao/test_services_dao.py | 16 +++++++++++----- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/app/history_meta.py b/app/history_meta.py index be0c184b5..9cbbe07f1 100644 --- a/app/history_meta.py +++ b/app/history_meta.py @@ -53,7 +53,12 @@ def _history_mapper(local_mapper): col = col.copy() orig.info['history_copy'] = col col.unique = False - col.default = col.server_default = None + + # if the column is nullable, we could end up overwriting an on-purpose null value with a default. + # if it's not nullable, however, the default may be relied upon to correctly set values within the database, + # so we should preserve it + if col.nullable: + col.default = col.server_default = None return col properties = util.OrderedDict() @@ -201,7 +206,9 @@ def create_history(obj): elif isinstance(prop, RelationshipProperty): if hasattr(history, prop.key+'_id'): - data[prop.key+'_id'] = getattr(obj, prop.key).id + foreign_obj = getattr(obj, prop.key) + # if it's a nullable relationship, foreign_obj will be None, and we actually want to record that + data[prop.key+'_id'] = getattr(foreign_obj, 'id', None) if not obj.version: obj.version = 1 diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 943343b61..1af238b4d 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -35,7 +35,8 @@ from app.models import ( Permission, User, InvitedUser, - Service + Service, + BRANDING_GOVUK ) from tests.app.conftest import ( @@ -60,10 +61,13 @@ def test_create_service(sample_user): created_by=sample_user) dao_create_service(service, sample_user) assert Service.query.count() == 1 - assert Service.query.first().name == "service_name" - assert Service.query.first().id == service.id - assert not Service.query.first().research_mode - assert sample_user in Service.query.first().users + + service_db = Service.query.first() + assert service_db.name == "service_name" + assert service_db.id == service.id + assert service_db.branding == BRANDING_GOVUK + assert not service_db.research_mode + assert sample_user in service_db.users def test_cannot_create_two_services_with_same_name(sample_user): @@ -254,6 +258,8 @@ def test_create_service_creates_a_history_record_with_current_data(sample_user): assert service_from_db.version == service_history.version assert sample_user.id == service_history.created_by_id assert service_from_db.created_by.id == service_history.created_by_id + assert service_from_db.branding == BRANDING_GOVUK + assert service_history.branding == BRANDING_GOVUK def test_update_service_creates_a_history_record_with_current_data(sample_user): From e631333a34e518b2b7e6fa4cad310ba0fa4335fe Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 5 Aug 2016 15:48:06 +0100 Subject: [PATCH 3/4] add GET /organisation and GET /organisation/id endpoints --- app/__init__.py | 2 ++ app/dao/organisation_dao.py | 9 +++++++ app/organisation/__init__.py | 0 app/organisation/rest.py | 20 +++++++++++++++ app/schemas.py | 7 ++++++ tests/app/organisation/__init__.py | 0 tests/app/organisation/test_rest.py | 39 +++++++++++++++++++++++++++++ 7 files changed, 77 insertions(+) create mode 100644 app/dao/organisation_dao.py create mode 100644 app/organisation/__init__.py create mode 100644 app/organisation/rest.py create mode 100644 tests/app/organisation/__init__.py create mode 100644 tests/app/organisation/test_rest.py diff --git a/app/__init__.py b/app/__init__.py index be9f8aa95..af3f15c74 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -71,6 +71,7 @@ def create_app(app_name=None): from app.events.rest import events as events_blueprint from app.provider_details.rest import provider_details as provider_details_blueprint from app.spec.rest import spec as spec_blueprint + from app.organisation.rest import organisation_blueprint application.register_blueprint(service_blueprint, url_prefix='/service') application.register_blueprint(user_blueprint, url_prefix='/user') @@ -85,6 +86,7 @@ def create_app(app_name=None): application.register_blueprint(events_blueprint) application.register_blueprint(provider_details_blueprint, url_prefix='/provider-details') application.register_blueprint(spec_blueprint, url_prefix='/spec') + application.register_blueprint(organisation_blueprint, url_prefix='/organisation') return application diff --git a/app/dao/organisation_dao.py b/app/dao/organisation_dao.py new file mode 100644 index 000000000..804fd8a1d --- /dev/null +++ b/app/dao/organisation_dao.py @@ -0,0 +1,9 @@ +from app.models import Organisation + + +def dao_get_organisations(): + return Organisation.query.all() + + +def dao_get_organisation_by_id(org_id): + return Organisation.query.filter_by(id=org_id).one() diff --git a/app/organisation/__init__.py b/app/organisation/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/app/organisation/rest.py b/app/organisation/rest.py new file mode 100644 index 000000000..1bdfbff74 --- /dev/null +++ b/app/organisation/rest.py @@ -0,0 +1,20 @@ +from flask import Blueprint, jsonify + +from app.dao.organisation_dao import dao_get_organisations, dao_get_organisation_by_id +from app.schemas import organisation_schema +from app.errors import register_errors + +organisation_blueprint = Blueprint('organisation', __name__) +register_errors(organisation_blueprint) + + +@organisation_blueprint.route('', methods=['GET']) +def get_organisations(): + data = organisation_schema.dump(dao_get_organisations(), many=True).data + return jsonify(organisations=data) + + +@organisation_blueprint.route('/', methods=['GET']) +def get_organisation_by_id(org_id): + data = organisation_schema.dump(dao_get_organisation_by_id(org_id)).data + return jsonify(organisation=data) diff --git a/app/schemas.py b/app/schemas.py index ea71ac341..168faa02c 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -440,6 +440,12 @@ class EventSchema(BaseSchema): strict = True +class OrganisationSchema(BaseSchema): + class Meta: + model = models.Organisation + strict = True + + class FromToDateSchema(ma.Schema): class Meta: @@ -533,6 +539,7 @@ service_history_schema = ServiceHistorySchema() api_key_history_schema = ApiKeyHistorySchema() template_history_schema = TemplateHistorySchema() event_schema = EventSchema() +organisation_schema = OrganisationSchema() from_to_date_schema = FromToDateSchema() provider_details_schema = ProviderDetailsSchema() week_aggregate_notification_statistics_schema = WeekAggregateNotificationStatisticsSchema() diff --git a/tests/app/organisation/__init__.py b/tests/app/organisation/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/app/organisation/test_rest.py b/tests/app/organisation/test_rest.py new file mode 100644 index 000000000..f024edd31 --- /dev/null +++ b/tests/app/organisation/test_rest.py @@ -0,0 +1,39 @@ +from flask import json + +from app.models import Organisation + +from tests import create_authorization_header + + +def test_get_organisations(notify_api, notify_db, notify_db_session): + org1 = Organisation(colour='#FFFFFF', logo='/path/image.png', name='Org1') + org2 = Organisation(colour='#000000', logo='/path/other.png', name='Org2') + notify_db.session.add_all([org1, org2]) + notify_db.session.commit() + + with notify_api.test_request_context(), notify_api.test_client() as client: + auth_header = create_authorization_header() + response = client.get('/organisation', headers=[auth_header]) + + assert response.status_code == 200 + organisations = json.loads(response.get_data(as_text=True))['organisations'] + assert len(organisations) == 2 + assert {org['id'] for org in organisations} == {str(org1.id), str(org2.id)} + + +def test_get_organisation_by_id(notify_api, notify_db, notify_db_session): + org = Organisation(colour='#FFFFFF', logo='/path/image.png', name='My Org') + notify_db.session.add(org) + notify_db.session.commit() + + with notify_api.test_request_context(), notify_api.test_client() as client: + auth_header = create_authorization_header() + response = client.get('/organisation/{}'.format(org.id), headers=[auth_header]) + + assert response.status_code == 200 + organisation = json.loads(response.get_data(as_text=True))['organisation'] + assert set(organisation.keys()) == {'colour', 'logo', 'name', 'id'} + assert organisation['colour'] == '#FFFFFF' + assert organisation['logo'] == '/path/image.png' + assert organisation['name'] == 'My Org' + assert organisation['id'] == str(org.id) From c7d909be03947376cd713d17a273e016ff217128 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 5 Aug 2016 16:32:55 +0100 Subject: [PATCH 4/4] add UKVI as first organisation branding --- migrations/versions/0046_organisations_and_branding.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/migrations/versions/0046_organisations_and_branding.py b/migrations/versions/0046_organisations_and_branding.py index e36d2ded6..c03faf2ce 100644 --- a/migrations/versions/0046_organisations_and_branding.py +++ b/migrations/versions/0046_organisations_and_branding.py @@ -33,6 +33,13 @@ def upgrade(): op.add_column('services_history', sa.Column('organisation_id', postgresql.UUID(as_uuid=True))) op.execute("INSERT INTO branding_type VALUES ('govuk'), ('org'), ('both')") + # insert UKVI data as initial test data. hex and crest pulled from alphagov/whitehall + op.execute("""INSERT INTO organisation VALUES ( + '9d25d02d-2915-4e98-874b-974e123e8536', + '#9325b2', + 'ho_crest_27px_x2.png', + 'UK Visas and Immigration' + )""") op.execute("UPDATE services SET branding='govuk'") op.execute("UPDATE services_history SET branding='govuk'")