From a4f21dc7e6ee36cd27f2bfb6a556c39680211d2f Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Thu, 14 Nov 2019 15:55:23 -0500 Subject: [PATCH] Prevent error from being raised when user is not trying to update a disabled env role We were only checking to see if a role was disabled or deleted before raising an error, so I added in a check to see if the user was trying to update the env role before raising an error. The error should only be raised if the role is disabled or deleted AND the user is trying to assign a new role to the env role. I also added in a disabled property to the EnvironmentRole model to make things more readable. --- atst/domain/environments.py | 14 ++++---------- atst/models/environment_role.py | 4 ++++ atst/routes/applications/settings.py | 3 +-- tests/domain/test_environment_roles.py | 2 +- tests/domain/test_environments.py | 9 +++++++-- tests/routes/applications/test_settings.py | 2 +- 6 files changed, 18 insertions(+), 16 deletions(-) diff --git a/atst/domain/environments.py b/atst/domain/environments.py index 2114ce21..a1056623 100644 --- a/atst/domain/environments.py +++ b/atst/domain/environments.py @@ -10,7 +10,6 @@ from atst.models import ( Portfolio, TaskOrder, CLIN, - EnvironmentRole, ) from atst.domain.environment_roles import EnvironmentRoles @@ -58,16 +57,11 @@ class Environments(object): @classmethod def update_env_role(cls, environment, application_role, new_role): env_role = EnvironmentRoles.get_for_update(application_role.id, environment.id) - if env_role and ( - env_role.status == EnvironmentRole.Status.DISABLED or env_role.deleted - ): + + if env_role and new_role and (env_role.disabled or env_role.deleted): raise DisabledError("environment_role", env_role.id) - if ( - env_role - and env_role.role != new_role - and env_role.status != EnvironmentRole.Status.DISABLED - ): + if env_role and env_role.role != new_role and not env_role.disabled: env_role.role = new_role db.session.add(env_role) elif not env_role and new_role: @@ -78,7 +72,7 @@ class Environments(object): ) db.session.add(env_role) - if env_role and not new_role: + if env_role and not new_role and not env_role.disabled: EnvironmentRoles.disable(env_role.id) db.session.commit() diff --git a/atst/models/environment_role.py b/atst/models/environment_role.py index 813d3d50..37d1c17c 100644 --- a/atst/models/environment_role.py +++ b/atst/models/environment_role.py @@ -66,6 +66,10 @@ class EnvironmentRole( def displayname(self): return self.role + @property + def disabled(self): + return self.status == EnvironmentRole.Status.DISABLED + @property def event_details(self): return { diff --git a/atst/routes/applications/settings.py b/atst/routes/applications/settings.py index 16d227ed..22863ac6 100644 --- a/atst/routes/applications/settings.py +++ b/atst/routes/applications/settings.py @@ -14,7 +14,6 @@ from atst.forms.application import NameAndDescriptionForm, EditEnvironmentForm from atst.forms.data import ENV_ROLE_NO_ACCESS as NO_ACCESS from atst.forms.member import NewForm as MemberForm from atst.domain.authz.decorator import user_can_access_decorator as user_can -from atst.models.environment_role import EnvironmentRole from atst.models.permissions import Permissions from atst.domain.permission_sets import PermissionSets from atst.utils.flash import formatted_flash as flash @@ -92,7 +91,7 @@ def filter_env_roles_form_data(member, environments): if len(env_roles_set) == 1: (env_role,) = env_roles_set env_data["role"] = env_role.role - env_data["disabled"] = env_role.status == EnvironmentRole.Status.DISABLED + env_data["disabled"] = env_role.disabled env_roles_form_data.append(env_data) diff --git a/tests/domain/test_environment_roles.py b/tests/domain/test_environment_roles.py index 6ac96488..486da70a 100644 --- a/tests/domain/test_environment_roles.py +++ b/tests/domain/test_environment_roles.py @@ -88,7 +88,7 @@ def test_disable_completed(application_role, environment): environment_role = EnvironmentRoles.disable(environment_role.id) - assert environment_role.status == EnvironmentRole.Status.DISABLED + assert environment_role.disabled def test_get_for_update(application_role, environment): diff --git a/tests/domain/test_environments.py b/tests/domain/test_environments.py index 55d54f44..3aa5f547 100644 --- a/tests/domain/test_environments.py +++ b/tests/domain/test_environments.py @@ -42,21 +42,26 @@ def test_update_env_role_no_access(): env_role.application_role.id, env_role.environment.id ) assert env_role.role is None - assert env_role.status == EnvironmentRole.Status.DISABLED + assert env_role.disabled def test_update_env_role_disabled_role(): env_role = EnvironmentRoleFactory.create(role=CSPRole.BASIC_ACCESS.value) Environments.update_env_role(env_role.environment, env_role.application_role, None) + # An exception should be raised when a new role is passed to Environments.update_env_role with pytest.raises(DisabledError): Environments.update_env_role( env_role.environment, env_role.application_role, CSPRole.TECHNICAL_READ.value, ) + assert env_role.role is None - assert env_role.status == EnvironmentRole.Status.DISABLED + assert env_role.disabled + + # An exception should not be raised when no new role is passed + Environments.update_env_role(env_role.environment, env_role.application_role, None) def test_get_excludes_deleted(): diff --git a/tests/routes/applications/test_settings.py b/tests/routes/applications/test_settings.py index 45522edb..8eb0079b 100644 --- a/tests/routes/applications/test_settings.py +++ b/tests/routes/applications/test_settings.py @@ -563,7 +563,7 @@ def test_update_member(client, user_session, session): # check that the user has roles in the correct envs assert len(environment_roles) == 3 assert updated_role.role == CSPRole.TECHNICAL_READ.value - assert suspended_role.status == EnvironmentRole.Status.DISABLED + assert suspended_role.disabled def test_revoke_invite(client, user_session):