From 8e46cd44fde85b19a228a71c53723e455c97775a Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 4 Aug 2016 15:57:41 +0100 Subject: [PATCH] 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):