From e28ef237e432fed376b26d93c8819f8f23fc8451 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Tue, 7 Jun 2016 16:31:10 +0100 Subject: [PATCH] When adding a user new with permissions to a service, the permissions dao was deleting all permissions for that user (regardless of service id) as the last filter on the permissions dao get_query method won. I've added a replace flag to the set_user_service_permission method so that it can handle adding new users + permissions and editing of existing users' permissions. Also by pass the get_query method until it can be refactored to work correctly. For now execute the filter query directly on the model. --- app/dao/permissions_dao.py | 11 ++++--- app/service/rest.py | 21 ++++++------- app/user/rest.py | 2 +- tests/app/dao/test_services_dao.py | 50 ++++++++++++++++++++++++++++++ 4 files changed, 67 insertions(+), 17 deletions(-) diff --git a/app/dao/permissions_dao.py b/app/dao/permissions_dao.py index 7ff9bb447..525b39a0f 100644 --- a/app/dao/permissions_dao.py +++ b/app/dao/permissions_dao.py @@ -32,6 +32,8 @@ class PermissionDAO(DAOClass): class Meta: model = Permission + # TODO rework this as last filter wins, whereas what is needed is + # append to filter so that semantics are 'and' def get_query(self, filter_by_dict=None): if filter_by_dict is None: filter_by_dict = MultiDict() @@ -60,13 +62,14 @@ class PermissionDAO(DAOClass): self.create_instance(permission, _commit=False) def remove_user_service_permissions(self, user, service): - query = self.get_query(filter_by_dict={'user': user.id, 'service': service.id}) + query = self.Meta.model.query.filter_by(user=user, service=service) query.delete() - def set_user_service_permission(self, user, service, permissions, _commit=False): + def set_user_service_permission(self, user, service, permissions, _commit=False, replace=False): try: - query = self.get_query(filter_by_dict={'user': user.id, 'service': service.id}) - query.delete() + if replace: + query = self.Meta.model.query.filter_by(user=user, service=service) + query.delete() for p in permissions: p.user = user p.service = service diff --git a/app/service/rest.py b/app/service/rest.py index 80b82fe67..0d2da9cb0 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -1,9 +1,13 @@ -from datetime import (datetime, date) +from datetime import ( + datetime, + date +) -from flask import Blueprint from flask import ( jsonify, - request + request, + abort, + Blueprint ) from sqlalchemy.orm.exc import NoResultFound @@ -152,6 +156,8 @@ def add_user_to_service(service_id, user_id): message='User id: {} already part of service id: {}'.format(user_id, service_id)), 400 permissions, errors = permission_schema.load(request.get_json(), many=True) + if errors: + abort(400, errors) dao_add_user_to_service(service, user, permissions) data, errors = service_schema.dump(service) @@ -174,15 +180,6 @@ def remove_user_from_service(service_id, user_id): return jsonify({}), 204 -def _process_permissions(user, service, permission_groups): - from app.permissions_utils import get_permissions_by_group - permissions = get_permissions_by_group(permission_groups) - for permission in permissions: - permission.user = user - permission.service = service - return permissions - - @service.route('//fragment/aggregate_statistics') def get_service_provider_aggregate_statistics(service_id): service = dao_fetch_service_by_id(service_id) diff --git a/app/user/rest.py b/app/user/rest.py index 1850c1c04..12fde343a 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -188,7 +188,7 @@ def set_permissions(user_id, service_id): for p in permissions: p.user = user p.service = service - permission_dao.set_user_service_permission(user, service, permissions, _commit=True) + permission_dao.set_user_service_permission(user, service, permissions, _commit=True, replace=True) return jsonify({}), 204 diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 199535d27..435d7aa35 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -334,3 +334,53 @@ def test_delete_service_and_associated_objects(notify_db, assert InvitedUser.query.count() == 0 assert Service.query.count() == 0 assert Service.get_history_model().query.count() == 0 + + +def test_add_existing_user_to_another_service_doesnot_change_old_permissions(sample_user): + + service_one = Service(name="service_one", + email_from="service_one", + message_limit=1000, + active=True, + restricted=False, + created_by=sample_user) + + dao_create_service(service_one, sample_user) + assert sample_user.id == service_one.users[0].id + test_user_permissions = Permission.query.filter_by(service=service_one, user=sample_user).all() + assert len(test_user_permissions) == 8 + + other_user = User( + name='Other Test User', + email_address='other_user@digital.cabinet-office.gov.uk', + password='password', + mobile_number='+447700900987' + ) + save_model_user(other_user) + service_two = Service(name="service_two", + email_from="service_two", + message_limit=1000, + active=True, + restricted=False, + created_by=other_user) + dao_create_service(service_two, other_user) + + assert other_user.id == service_two.users[0].id + other_user_permissions = Permission.query.filter_by(service=service_two, user=other_user).all() + assert len(other_user_permissions) == 8 + + other_user_service_one_permissions = Permission.query.filter_by(service=service_one, user=other_user).all() + assert len(other_user_service_one_permissions) == 0 + + # adding the other_user to service_one should leave all other_user permissions on service_two intact + permissions = [] + for p in ['send_emails', 'send_texts', 'send_letters']: + permissions.append(Permission(permission=p)) + + dao_add_user_to_service(service_one, other_user, permissions=permissions) + + other_user_service_one_permissions = Permission.query.filter_by(service=service_one, user=other_user).all() + assert len(other_user_service_one_permissions) == 3 + + other_user_service_two_permissions = Permission.query.filter_by(service=service_two, user=other_user).all() + assert len(other_user_service_two_permissions) == 8