From e8f21acf5bd117dfd02ffe63bca1b14c99a01ab6 Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Thu, 31 Oct 2019 10:12:41 -0400 Subject: [PATCH] PR fixes --- atst/domain/environment_roles.py | 9 --- atst/domain/environments.py | 19 +++++-- atst/forms/application_member.py | 2 +- atst/models/application_role.py | 7 +-- atst/routes/applications/settings.py | 20 ++++--- .../applications/fragments/environments.html | 3 +- .../fragments/member_form_fields.html | 56 ++++++++++--------- .../fragments/new_member_modal_content.html | 2 +- tests/domain/test_environment_roles.py | 11 ---- tests/domain/test_environments.py | 2 - tests/forms/test_application_member.py | 4 +- tests/routes/applications/test_settings.py | 23 ++++---- translations.yaml | 3 + 13 files changed, 81 insertions(+), 80 deletions(-) diff --git a/atst/domain/environment_roles.py b/atst/domain/environment_roles.py index 4dfb29c5..add92091 100644 --- a/atst/domain/environment_roles.py +++ b/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() - ) diff --git a/atst/domain/environments.py b/atst/domain/environments.py index 39d6214c..f36c1a3b 100644 --- a/atst/domain/environments.py +++ b/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 - updated = EnvironmentRoles.delete(application_role.id, environment.id) + EnvironmentRoles.disable(env_role.id) + updated = True if updated: db.session.add(env_role) diff --git a/atst/forms/application_member.py b/atst/forms/application_member.py index e44d2ad9..ec873f77 100644 --- a/atst/forms/application_member.py +++ b/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): diff --git a/atst/models/application_role.py b/atst/models/application_role.py index f0e20be1..9468896c 100644 --- a/atst/models/application_role.py +++ b/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_( - EnvironmentRole.application_role_id == id, EnvironmentRole.deleted == False - ), + primaryjoin="and_(EnvironmentRole.application_role_id == ApplicationRole.id, EnvironmentRole.deleted == False)", ) @property diff --git a/atst/routes/applications/settings.py b/atst/routes/applications/settings.py index 65f6d00e..16d227ed 100644 --- a/atst/routes/applications/settings.py +++ b/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( - set(EnvironmentRoles.get_all_for_application_member(member.id)) - ) + env_roles_set = set(env.roles).intersection(set(member.environment_roles)) 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) diff --git a/templates/applications/fragments/environments.html b/templates/applications/fragments/environments.html index 923d41a9..6ff63642 100644 --- a/templates/applications/fragments/environments.html +++ b/templates/applications/fragments/environments.html @@ -64,8 +64,9 @@ {% call ToggleSection(section_name="members") %} diff --git a/templates/applications/fragments/member_form_fields.html b/templates/applications/fragments/member_form_fields.html index 7b57cac1..97ab3832 100644 --- a/templates/applications/fragments/member_form_fields.html +++ b/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) %} - {% set role = field.role.data if not field.deleted.data else "Access Suspended" %} - {% if field.role.data != "No Access" and not field.deleted.data -%} +{% macro EnvRoleInput(sub_form, member_role_id=None) %} + {% set role = sub_form.role.data if not sub_form.disabled.data else "Access Suspended" %} + {% if sub_form.role.data != "No Access" and not sub_form.disabled.data -%}
- {% set id = "{}-{}".format(field.deleted.name, member_role_id) %} + {% set id = "{}-{}".format(sub_form.disabled.name, member_role_id) %}
- {{ field.environment_name.data }} + {{ sub_form.environment_name.data }}
-

+

{{ role }} -

+
- {{ field.deleted(id=id, checked=True, **{"v-model": "isChecked"}) }} - {{ field.deleted.label(for=id, class="usa-button button-danger-outline") | safe }} + {{ sub_form.disabled(id=id, checked=True, **{"v-model": "isChecked"}) }} + {{ sub_form.disabled.label(for=id, class="usa-button button-danger-outline") | safe }}
@@ -34,14 +34,14 @@
- {{ field.environment_name.data }} + {{ sub_form.environment_name.data }}

- Save changes to revoke access, this can not be undone. + {{ "portfolios.applications.members.form.env_access.revoke_warning" | translate | safe }}

- {{ field.deleted(id=id, checked=True, **{"v-model": "isChecked"}) }} + {{ sub_form.disabled(id=id, checked=True, **{"v-model": "isChecked"}) }}
@@ -51,11 +51,10 @@ {% else %}
-
-
+
- {{ field.environment_name.data }} + {{ sub_form.environment_name.data }}

{{ role }} @@ -63,22 +62,22 @@

- {% if field.role.data == "No Access" and not field.deleted.data -%} + {% if sub_form.role.data == "No Access" and not sub_form.disabled.data -%}
- {{ 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)}) }}
- {% elif field.deleted.data -%} + {% elif sub_form.disabled.data -%}

- Suspended access cannot be modified. + {{ "portfolios.applications.members.form.env_access.suspended" | translate }}

{%- endif %} - {{ field.environment_id() }} + {{ sub_form.environment_id() }}
{% endif %} @@ -105,7 +104,12 @@

{{ "portfolios.applications.members.form.env_access.title" | translate }}

-

{{ "portfolios.applications.members.form.env_access.description" | translate | safe }}

+

+ {% if not new -%} + {{ "portfolios.applications.members.form.env_access.edit_description" | translate }} + {%- endif %} + {{ "portfolios.applications.members.form.env_access.description" | translate | safe }} +

{% for environment_data in form.environment_roles %} {{ EnvRoleInput(environment_data, member_role_id) }} diff --git a/templates/applications/fragments/new_member_modal_content.html b/templates/applications/fragments/new_member_modal_content.html index d59585f0..bee6f303 100644 --- a/templates/applications/fragments/new_member_modal_content.html +++ b/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 %} diff --git a/tests/domain/test_environment_roles.py b/tests/domain/test_environment_roles.py index 8f68207f..6ac96488 100644 --- a/tests/domain/test_environment_roles.py +++ b/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 diff --git a/tests/domain/test_environments.py b/tests/domain/test_environments.py index a3562741..277c47ea 100644 --- a/tests/domain/test_environments.py +++ b/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 diff --git a/tests/forms/test_application_member.py b/tests/forms/test_application_member.py index 95ed6e91..fc056223 100644 --- a/tests/forms/test_application_member.py +++ b/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, } diff --git a/tests/routes/applications/test_settings.py b/tests/routes/applications/test_settings.py index 18a89014..45522edb 100644 --- a/tests/routes/applications/test_settings.py +++ b/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 ( - env_obj["members"].sort() - == [app_role1.user_name, app_role2.user_name].sort() - ) + assert { + "user_name": app_role1.user_name, + "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 environment_roles[1].environment in [env, env_2] + assert len(environment_roles) == 3 + 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): diff --git a/translations.yaml b/translations.yaml index 5ddfb9c7..b0705132 100644 --- a/translations.yaml +++ b/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. Learn More + revoke_warning: Save changes to revoke access, this can not be undone. + suspended: Suspended access cannot be modified. next_button: "Next: Permissions" app_perms: title: Application Permissions