From d40c11a8f65ceeb41bbe26e2a7fbd669f71dd944 Mon Sep 17 00:00:00 2001
From: leigh-mil <leigh-ctr@friends.dds.mil>
Date: Fri, 25 Oct 2019 10:40:38 -0400
Subject: [PATCH] Change how env_roles are updated

This change makes it so that when an env_role is updated to be None, the
role property on the env_role is changed to be None in addition to being
marked as deleted. This also adds in a check so that previously deleted
env_roles cannot be reassigned a role.
---
 atst/domain/environment_roles.py       | 12 +++++++++++
 atst/domain/environments.py            | 28 +++++++++++++-------------
 tests/domain/test_environment_roles.py | 11 ++++++++++
 tests/domain/test_environments.py      | 16 +++++++++++++++
 4 files changed, 53 insertions(+), 14 deletions(-)

diff --git a/atst/domain/environment_roles.py b/atst/domain/environment_roles.py
index 57fe2e36..6e69ac96 100644
--- a/atst/domain/environment_roles.py
+++ b/atst/domain/environment_roles.py
@@ -114,3 +114,15 @@ class EnvironmentRoles(object):
         db.session.commit()
 
         return environment_role
+
+    @classmethod
+    def get_for_update(cls, application_role_id, environment_id):
+        existing_env_role = (
+            db.session.query(EnvironmentRole)
+            .filter(
+                EnvironmentRole.application_role_id == application_role_id,
+                EnvironmentRole.environment_id == environment_id,
+            )
+            .one_or_none()
+        )
+        return existing_env_role
diff --git a/atst/domain/environments.py b/atst/domain/environments.py
index 1526f6dc..2a9c5f61 100644
--- a/atst/domain/environments.py
+++ b/atst/domain/environments.py
@@ -52,22 +52,22 @@ class Environments(object):
     def update_env_role(cls, environment, application_role, new_role):
         updated = False
 
+        env_role = EnvironmentRoles.get_for_update(application_role.id, environment.id)
+        if env_role and env_role.role != new_role and not env_role.deleted:
+            env_role.role = new_role
+            updated = True
+            db.session.add(env_role)
+        elif not env_role:
+            env_role = EnvironmentRoles.create(
+                application_role=application_role,
+                environment=environment,
+                role=new_role,
+            )
+            updated = True
+            db.session.add(env_role)
+
         if new_role is None:
             updated = EnvironmentRoles.delete(application_role.id, environment.id)
-        else:
-            env_role = EnvironmentRoles.get(application_role.id, environment.id)
-            if env_role and env_role.role != new_role:
-                env_role.role = new_role
-                updated = True
-                db.session.add(env_role)
-            elif not env_role:
-                env_role = EnvironmentRoles.create(
-                    application_role=application_role,
-                    environment=environment,
-                    role=new_role,
-                )
-                updated = True
-                db.session.add(env_role)
 
         if updated:
             db.session.commit()
diff --git a/tests/domain/test_environment_roles.py b/tests/domain/test_environment_roles.py
index f5459811..71bbbdf2 100644
--- a/tests/domain/test_environment_roles.py
+++ b/tests/domain/test_environment_roles.py
@@ -91,3 +91,14 @@ def test_disable_completed(application_role, environment):
     environment_role = EnvironmentRoles.disable(environment_role.id)
 
     assert environment_role.status == EnvironmentRole.Status.DISABLED
+
+
+def test_get_for_update():
+    app_role = ApplicationRoleFactory.create()
+    env = EnvironmentFactory.create(application=app_role.application)
+    EnvironmentRoleFactory.create(application_role=app_role, environment=env, deleted=True)
+    role = EnvironmentRoles.get_for_update(app_role.id, env.id)
+    assert role
+    assert role.application_role == app_role
+    assert role.environment == env
+    assert role.deleted
diff --git a/tests/domain/test_environments.py b/tests/domain/test_environments.py
index 4264154c..112c33af 100644
--- a/tests/domain/test_environments.py
+++ b/tests/domain/test_environments.py
@@ -45,6 +45,9 @@ def test_update_env_role_no_access():
     assert not EnvironmentRoles.get(
         env_role.application_role.id, env_role.environment.id
     )
+    assert env_role.role is None
+    assert env_role.deleted
+    assert env_role.status == EnvironmentRole.Status.DISABLED
 
 
 def test_update_env_role_no_change():
@@ -56,6 +59,19 @@ def test_update_env_role_no_change():
     )
 
 
+def test_update_env_role_deleted_role():
+    env_role = EnvironmentRoleFactory.create(role=CSPRole.BASIC_ACCESS.value)
+    Environments.update_env_role(
+        env_role.environment, env_role.application_role, None
+    )
+    assert not Environments.update_env_role(
+        env_role.environment, env_role.application_role, CSPRole.TECHNICAL_READ.value
+    )
+    assert env_role.role is None
+    assert env_role.deleted
+    assert env_role.status == EnvironmentRole.Status.DISABLED
+
+
 def test_get_excludes_deleted():
     env = EnvironmentFactory.create(
         deleted=True, application=ApplicationFactory.create()