PR fixes

atst/domain/environment_roles.py

@@ -73,7 +73,6 @@ class EnvironmentRoles(object):
         if existing_env_role:
             # TODO: Implement suspension
             existing_env_role.deleted = True
-            cls._update_status(existing_env_role, EnvironmentRole.Status.DISABLED)
             db.session.add(existing_env_role)
             db.session.commit()
             return True
@@ -126,11 +125,3 @@ class EnvironmentRoles(object):
             .one_or_none()
         )
         return existing_env_role
-
-    @classmethod
-    def get_all_for_application_member(cls, application_role_id):
-        return (
-            db.session.query(EnvironmentRole)
-            .filter(EnvironmentRole.application_role_id == application_role_id)
-            .all()
-        )

atst/domain/environments.py

@@ -4,7 +4,14 @@ from typing import List
 from uuid import UUID
 
 from atst.database import db
-from atst.models import Environment, Application, Portfolio, TaskOrder, CLIN
+from atst.models import (
+    Environment,
+    Application,
+    Portfolio,
+    TaskOrder,
+    CLIN,
+    EnvironmentRole,
+)
 from atst.domain.environment_roles import EnvironmentRoles
 
 from .exceptions import NotFoundError
@@ -53,7 +60,11 @@ class Environments(object):
         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:
+        if (
+            env_role
+            and env_role.role != new_role
+            and env_role.status != EnvironmentRole.Status.DISABLED
+        ):
             env_role.role = new_role
             updated = True
         elif not env_role and new_role:
@@ -65,8 +76,8 @@ class Environments(object):
             updated = True
 
         if env_role and not new_role:
-            env_role.role = None
+            EnvironmentRoles.disable(env_role.id)
-            updated = EnvironmentRoles.delete(application_role.id, environment.id)
+            updated = True
 
         if updated:
             db.session.add(env_role)

atst/forms/application_member.py

@@ -18,7 +18,7 @@ class EnvironmentForm(Form):
         default=NO_ACCESS,
         filters=[lambda x: NO_ACCESS if x == "None" else x],
     )
-    deleted = BooleanField("Revoke Access", default=False)
+    disabled = BooleanField("Revoke Access", default=False)
 
     @property
     def data(self):

atst/models/application_role.py

@@ -1,5 +1,5 @@
 from enum import Enum
-from sqlalchemy import and_, Index, ForeignKey, Column, Enum as SQLAEnum, Table
+from sqlalchemy import Index, ForeignKey, Column, Enum as SQLAEnum, Table
 from sqlalchemy.dialects.postgresql import UUID
 from sqlalchemy.orm import relationship
 from sqlalchemy.event import listen
@@ -8,7 +8,6 @@ from atst.utils import first_or_none
 from atst.models.base import Base
 import atst.models.mixins as mixins
 import atst.models.types as types
-from atst.models.environment_role import EnvironmentRole
 from atst.models.mixins.auditable import record_permission_sets_updates
 
 
@@ -55,9 +54,7 @@ class ApplicationRole(
 
     environment_roles = relationship(
         "EnvironmentRole",
-        primaryjoin=and_(
+        primaryjoin="and_(EnvironmentRole.application_role_id == ApplicationRole.id, EnvironmentRole.deleted == False)",
-            EnvironmentRole.application_role_id == id, EnvironmentRole.deleted == False
-        ),
     )
 
     @property

atst/routes/applications/settings.py

@@ -14,6 +14,7 @@ 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
@@ -31,7 +32,14 @@ def get_environments_obj_for_app(application):
                 "edit_form": EditEnvironmentForm(obj=env),
                 "member_count": len(env.roles),
                 "members": sorted(
-                    [env_role.application_role.user_name for env_role in env.roles]
+                    [
+                        {
+                            "user_name": env_role.application_role.user_name,
+                            "status": env_role.status.value,
+                        }
+                        for env_role in env.roles
+                    ],
+                    key=lambda env_role: env_role["user_name"],
                 ),
             }
             for env in application.environments
@@ -77,16 +85,14 @@ def filter_env_roles_form_data(member, environments):
             "environment_id": str(env.id),
             "environment_name": env.name,
             "role": NO_ACCESS,
-            "deleted": False,
+            "disabled": False,
         }
-        env_roles_set = set(env.roles).intersection(
+        env_roles_set = set(env.roles).intersection(set(member.environment_roles))
-            set(EnvironmentRoles.get_all_for_application_member(member.id))
-        )
 
         if len(env_roles_set) == 1:
             (env_role,) = env_roles_set
             env_data["role"] = env_role.role
-            env_data["deleted"] = env_role.deleted
+            env_data["disabled"] = env_role.status == EnvironmentRole.Status.DISABLED
 
         env_roles_form_data.append(env_data)
 
@@ -391,7 +397,7 @@ def update_member(application_id, application_role_id):
 
         for env_role in form.environment_roles:
             environment = Environments.get(env_role.environment_id.data)
-            new_role = None if env_role.deleted.data else env_role.data["role"]
+            new_role = None if env_role.disabled.data else env_role.data["role"]
             Environments.update_env_role(environment, app_role, new_role)
 
         flash("application_member_updated", user_name=app_role.user_name)

templates/applications/fragments/environments.html

@@ -64,8 +64,9 @@
                   {% call ToggleSection(section_name="members") %}
                     <ul>
                       {% for member in env['members'] %}
+                      {% set status = ": Access Suspended" if member['status'] == 'disabled' %}
                       <li class="accordion-table__item-toggle-content__expanded">
-                        {{ member }}
+                        {{ member['user_name'] }}{{ status }}
                       </li>
                       {% endfor %}
                     </ul>

templates/applications/fragments/member_form_fields.html

@@ -3,30 +3,30 @@
 {% from "components/text_input.html" import TextInput %}
 {% from "components/phone_input.html" import PhoneInput %}
 
-{% macro EnvRoleInput(field, member_role_id=None) %}
+{% macro EnvRoleInput(sub_form, member_role_id=None) %}
-  {% set role = field.role.data if not field.deleted.data else "Access Suspended" %}
+  {% set role = sub_form.role.data if not sub_form.disabled.data else "Access Suspended" %}
-  {% if field.role.data != "No Access" and not field.deleted.data -%}
+  {% if sub_form.role.data != "No Access" and not sub_form.disabled.data -%}
     <checkboxinput
-      name="'{{ field.deleted.name | string }}-{% if member_role_id %}-{{ member_role_id }}{% endif %}'"
+      name="'{{ sub_form.disabled.name | string }}-{% if member_role_id %}-{{ member_role_id }}{% endif %}'"
       inline-template
-      key="'{{ field.deleted.name | string }}-{% if member_role_id %}-{{ member_role_id }}{% endif %}'"
+      key="'{{ sub_form.disabled.name | string }}-{% if member_role_id %}-{{ member_role_id }}{% endif %}'"
-      v-bind:initial-checked='{{ field.deleted.data|string|lower }}'
+      v-bind:initial-checked='{{ sub_form.disabled.data|string|lower }}'
       v-bind:optional="true"
       >
       <fieldset data-ally-disabled="true" v-on:change="onInput" class="usa-input__choices revoke-button">
-        {% set id = "{}-{}".format(field.deleted.name, member_role_id) %}
+        {% set id = "{}-{}".format(sub_form.disabled.name, member_role_id) %}
         <div class="form-row" v-if="!isChecked">
           <div class="form-col form-col--two-thirds">
             <div class="usa-input__title-inline">
-              {{ field.environment_name.data }}
+              {{ sub_form.environment_name.data }}
             </div>
-            <p class="usa-input__help">
+            <div class="usa-input__help">
               {{ role }}
-            </p>
+            </div>
           </div>
           <div class="form-col form-col--third">
-            {{ field.deleted(id=id, checked=True, **{"v-model": "isChecked"}) }}
+            {{ sub_form.disabled(id=id, checked=True, **{"v-model": "isChecked"}) }}
-            {{ field.deleted.label(for=id, class="usa-button button-danger-outline") | safe }}
+            {{ sub_form.disabled.label(for=id, class="usa-button button-danger-outline") | safe }}
           </div>
         </div>
         <div v-else>
@@ -34,14 +34,14 @@
             <div class="form-row">
               <div class="form-col form-col--two-thirds">
                 <div class="usa-input__title-inline">
-                  {{ field.environment_name.data }}
+                  {{ sub_form.environment_name.data }}
                 </div>
                 <p class="usa-input__help">
-                  Save changes to revoke access, <strong>this can not be undone.</strong>
+                  {{ "portfolios.applications.members.form.env_access.revoke_warning" | translate | safe }}
                 </p>
               </div>
               <div class="form-col form-col--third">
-                {{ field.deleted(id=id, checked=True, **{"v-model": "isChecked"}) }}
+                {{ sub_form.disabled(id=id, checked=True, **{"v-model": "isChecked"}) }}
                 <label for="{{ id }}" class="link">Undo</label>
               </div>
             </div>
@@ -51,11 +51,10 @@
     </checkboxinput>
   {% else %}
     <div class="form-row">
-      <!-- refactor this into its own macro? -->
       <div class="form-col form-col--two-thirds">
-        <div class="usa-input {% if field.deleted.data or field.role.data == 'No Access' %}env-role__no-access{% endif %}">
+        <div class="usa-input {% if sub_form.disabled.data or sub_form.role.data == 'No Access' %}env-role__no-access{% endif %}">
           <div class='usa-input__title-inline'>
-            {{ field.environment_name.data }}
+            {{ sub_form.environment_name.data }}
           </div>
           <p class="usa-input__help">
             {{ role }}
@@ -63,22 +62,22 @@
         </div>
       </div>
       <div class="form-col form-col--third">
-        {% if field.role.data == "No Access" and not field.deleted.data -%}
+        {% if sub_form.role.data == "No Access" and not sub_form.disabled.data -%}
           <optionsinput inline-template
-            v-bind:initial-value="'{{ field.role.data | string }}'"
+            v-bind:initial-value="'{{ sub_form.role.data | string }}'"
-            v-bind:name="'{{ field.name | string }}{% if member_role_id %}-{{ member_role_id }}{% endif %}'"
+            v-bind:name="'{{ sub_form.name | string }}{% if member_role_id %}-{{ member_role_id }}{% endif %}'"
             v-bind:optional="true"
             v-bind:watch="true">
             <fieldset data-ally-disabled="true" v-on:change="onInput" class="usa-input__choices">
-              {{ field.role(**{"v-model": "value", "id": "{}-{}".format(field.role.name, member_role_id)}) }}
+              {{ sub_form.role(**{"v-model": "value", "id": "{}-{}".format(sub_form.role.name, member_role_id)}) }}
             </fieldset>
           </optionsinput>
-        {% elif field.deleted.data -%}
+        {% elif sub_form.disabled.data -%}
           <p class="usa-input__help">
-            Suspended access cannot be modified.
+            {{ "portfolios.applications.members.form.env_access.suspended" | translate }}
           </p>
         {%- endif %}
-        {{ field.environment_id() }}
+        {{ sub_form.environment_id() }}
       </div>
     </div>
   {% endif %}
@@ -105,7 +104,12 @@
   <hr>
   <div class="environment_roles environment-roles-new">
     <h2>{{ "portfolios.applications.members.form.env_access.title" | translate }}</h2>
-    <p class='usa-input__help subtitle'>{{ "portfolios.applications.members.form.env_access.description" | translate | safe }}</p>
+    <p class='usa-input__help subtitle'>
+      {% if not new -%}
+        {{ "portfolios.applications.members.form.env_access.edit_description" | translate }}
+      {%- endif %}
+      {{ "portfolios.applications.members.form.env_access.description" | translate | safe }}
+    </p>
     <hr>
     {% for environment_data in form.environment_roles %}
       {{ EnvRoleInput(environment_data, member_role_id) }}

templates/applications/fragments/new_member_modal_content.html

@@ -45,6 +45,6 @@
   {% endset %}
 
   {% call MemberFormTemplate(next_button=next_button) %}
-    {{ member_fields.PermsFields(form=member_form) }}
+    {{ member_fields.PermsFields(form=member_form, new=True) }}
   {% endcall %}
 {% endmacro %}

tests/domain/test_environment_roles.py

@@ -54,8 +54,6 @@ def test_delete(application_role, environment, monkeypatch):
         application_role=application_role, environment=environment
     )
     assert EnvironmentRoles.delete(application_role.id, environment.id)
-    assert env_role.status == EnvironmentRole.Status.DISABLED
-    assert env_role.deleted
     assert not EnvironmentRoles.delete(application_role.id, environment.id)
 
 
@@ -102,12 +100,3 @@ def test_get_for_update(application_role, environment):
     assert role.application_role == application_role
     assert role.environment == environment
     assert role.deleted
-
-
-def test_get_all_for_application_member(application_role, environment):
-    EnvironmentRoleFactory.create(
-        application_role=application_role, environment=environment, deleted=True
-    )
-
-    roles = EnvironmentRoles.get_all_for_application_member(application_role.id)
-    assert len(roles) == 1

tests/domain/test_environments.py

@@ -46,7 +46,6 @@ 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.deleted
     assert env_role.status == EnvironmentRole.Status.DISABLED
 
 
@@ -66,7 +65,6 @@ def test_update_env_role_deleted_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
 
 

tests/forms/test_application_member.py

@@ -10,7 +10,7 @@ def test_environment_form():
         "environment_id": 123,
         "environment_name": "testing",
         "role": ENV_ROLES[0][0],
-        "deleted": True,
+        "disabled": True,
     }
     form = EnvironmentForm(data=form_data)
     assert form.validate()
@@ -25,7 +25,7 @@ def test_environment_form_default_no_access():
         "environment_id": 123,
         "environment_name": "testing",
         "role": None,
-        "deleted": False,
+        "disabled": False,
     }
 
 

tests/routes/applications/test_settings.py

@@ -129,10 +129,14 @@ def test_edit_application_environments_obj(app, client, user_session):
         assert env_obj["name"] == env.name
         assert env_obj["id"] == env.id
         assert isinstance(env_obj["edit_form"], EditEnvironmentForm)
-        assert (
+        assert {
-            env_obj["members"].sort()
+            "user_name": app_role1.user_name,
-            == [app_role1.user_name, app_role2.user_name].sort()
+            "status": env_role1.status.value,
-        )
+        } in env_obj["members"]
+        assert {
+            "user_name": app_role2.user_name,
+            "status": env_role2.status.value,
+        } in env_obj["members"]
         assert isinstance(context["audit_events"], Paginator)
 
 
@@ -503,7 +507,7 @@ def test_update_member(client, user_session, session):
     env_1 = EnvironmentFactory.create(application=application)
     env_2 = EnvironmentFactory.create(application=application)
     # add user to two of the environments: env and env_1
-    EnvironmentRoleFactory.create(
+    updated_role = EnvironmentRoleFactory.create(
         environment=env, application_role=app_role, role=CSPRole.BASIC_ACCESS.value
     )
     suspended_role = EnvironmentRoleFactory.create(
@@ -525,7 +529,7 @@ def test_update_member(client, user_session, session):
             "environment_roles-0-environment_name": env.name,
             "environment_roles-1-environment_id": env_1.id,
             "environment_roles-1-environment_name": env_1.name,
-            "environment_roles-1-deleted": "True",
+            "environment_roles-1-disabled": "True",
             "environment_roles-2-environment_id": env_2.id,
             "environment_roles-2-role": CSPRole.NETWORK_ADMIN.value,
             "environment_roles-2-environment_name": env_2.name,
@@ -556,13 +560,10 @@ def test_update_member(client, user_session, session):
     )
 
     environment_roles = application.roles[0].environment_roles
-    # make sure that old env role was deleted and there are only 2 env roles
-    assert len(environment_roles) == 2
     # check that the user has roles in the correct envs
-    assert environment_roles[0].environment in [env, env_2]
+    assert len(environment_roles) == 3
-    assert environment_roles[1].environment in [env, env_2]
+    assert updated_role.role == CSPRole.TECHNICAL_READ.value
     assert suspended_role.status == EnvironmentRole.Status.DISABLED
-    assert suspended_role.deleted
 
 
 def test_revoke_invite(client, user_session):

translations.yaml

@@ -404,7 +404,10 @@ portfolios:
         env_access:
           title: Environment Roles
           table_header: Environment Access
+          edit_description: Add or revoke Environment access.
           description: Additional role controls are available in the CSP console. <a href="#"> Learn More </a>
+          revoke_warning: Save changes to revoke access, <strong>this can not be undone.</strong>
+          suspended: Suspended access cannot be modified.
         next_button: "Next: Permissions"
         app_perms:
           title: Application Permissions