Merge pull request #1186 from dod-ccpo/fix-revoke-env-access-bug

Fix revoke env role bug

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()

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 {

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)
 

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):

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():

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):