From 54f3c2f8ba6bc12b98a398caa14a878ea163db17 Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Wed, 23 Oct 2019 11:26:04 -0400 Subject: [PATCH 1/9] Update text and icon in modal Update env_role status when it is deleted --- atst/domain/environment_roles.py | 9 +++++++++ static/icons/avatar.svg | 2 +- templates/applications/fragments/members.html | 2 +- tests/domain/test_environment_roles.py | 5 ++++- translations.yaml | 3 +++ 5 files changed, 18 insertions(+), 3 deletions(-) diff --git a/atst/domain/environment_roles.py b/atst/domain/environment_roles.py index f2ed2988..02f97a41 100644 --- a/atst/domain/environment_roles.py +++ b/atst/domain/environment_roles.py @@ -57,6 +57,14 @@ class EnvironmentRoles(object): ) return existing_env_role + @classmethod + def _update_status(cls, environment_role, new_status): + environment_role.status = new_status + db.session.add(environment_role) + db.session.commit() + + return environment_role + @classmethod def delete(cls, application_role_id, environment_id): existing_env_role = EnvironmentRoles.get(application_role_id, environment_id) @@ -64,6 +72,7 @@ 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 diff --git a/static/icons/avatar.svg b/static/icons/avatar.svg index a3b68837..23737bfd 100644 --- a/static/icons/avatar.svg +++ b/static/icons/avatar.svg @@ -1 +1 @@ - + \ No newline at end of file diff --git a/templates/applications/fragments/members.html b/templates/applications/fragments/members.html index ac4aaffb..c252f452 100644 --- a/templates/applications/fragments/members.html +++ b/templates/applications/fragments/members.html @@ -36,7 +36,7 @@ {% set modal_name = "edit_member-{}".format(loop.index) %} {% call Modal(modal_name, classes="form-content--app-mem") %}
-

{{ Icon('avatar') }} {{ member.user_name }}

+

{{ Icon('avatar') }} {{ "portfolios.applications.members.form.edit_access_header" | translate({ "user": member.user_name }) }}

diff --git a/tests/domain/test_environment_roles.py b/tests/domain/test_environment_roles.py index 30f7a32b..f5459811 100644 --- a/tests/domain/test_environment_roles.py +++ b/tests/domain/test_environment_roles.py @@ -1,6 +1,7 @@ import pytest from atst.domain.environment_roles import EnvironmentRoles +from atst.models.environment_role import EnvironmentRole from tests.factories import * @@ -49,10 +50,12 @@ def test_get_by_user_and_environment(application_role, environment): def test_delete(application_role, environment, monkeypatch): - EnvironmentRoleFactory.create( + env_role = EnvironmentRoleFactory.create( 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) diff --git a/translations.yaml b/translations.yaml index 360d1daa..5ddfb9c7 100644 --- a/translations.yaml +++ b/translations.yaml @@ -409,6 +409,9 @@ portfolios: app_perms: title: Application Permissions description: Application permissions allow users to provision and modify applications and teams. Learn More + edit_access_header: "Manage {user}'s Access" + env_access_text: "Add or revoke Environment access. Additional controls are available in the CSP console." + step_2_title: Set Permissions and Roles add_member: Add Member menu: edit: Edit Roles and Permissions From d324ec57ec0ead3fd06601ba30708e6e2caabe0b Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Thu, 24 Oct 2019 08:31:17 -0400 Subject: [PATCH 2/9] Add field for deleted in the app members environment form --- atst/domain/environment_roles.py | 1 + atst/forms/application_member.py | 1 + atst/routes/applications/settings.py | 1 + tests/forms/test_application_member.py | 2 ++ tests/routes/applications/test_settings.py | 6 ++++-- 5 files changed, 9 insertions(+), 2 deletions(-) diff --git a/atst/domain/environment_roles.py b/atst/domain/environment_roles.py index 02f97a41..57fe2e36 100644 --- a/atst/domain/environment_roles.py +++ b/atst/domain/environment_roles.py @@ -29,6 +29,7 @@ class EnvironmentRoles(object): EnvironmentRole.application_role_id == application_role_id, EnvironmentRole.environment_id == environment_id, EnvironmentRole.deleted == False, + EnvironmentRole.status != EnvironmentRole.Status.DISABLED, ) .one_or_none() ) diff --git a/atst/forms/application_member.py b/atst/forms/application_member.py index ce2b6edd..152e0ddb 100644 --- a/atst/forms/application_member.py +++ b/atst/forms/application_member.py @@ -18,6 +18,7 @@ class EnvironmentForm(Form): default=NO_ACCESS, filters=[lambda x: None if x == "None" else x], ) + deleted = BooleanField("Revoke Access", default=False) @property def data(self): diff --git a/atst/routes/applications/settings.py b/atst/routes/applications/settings.py index 05d906ab..c1081a1b 100644 --- a/atst/routes/applications/settings.py +++ b/atst/routes/applications/settings.py @@ -77,6 +77,7 @@ def filter_env_roles_form_data(member, environments): "environment_id": str(env.id), "environment_name": env.name, "role": NO_ACCESS, + "deleted": env.deleted, } env_roles_set = set(env.roles).intersection(set(member.environment_roles)) if len(env_roles_set) == 1: diff --git a/tests/forms/test_application_member.py b/tests/forms/test_application_member.py index 61a9e373..95ed6e91 100644 --- a/tests/forms/test_application_member.py +++ b/tests/forms/test_application_member.py @@ -10,6 +10,7 @@ def test_environment_form(): "environment_id": 123, "environment_name": "testing", "role": ENV_ROLES[0][0], + "deleted": True, } form = EnvironmentForm(data=form_data) assert form.validate() @@ -24,6 +25,7 @@ def test_environment_form_default_no_access(): "environment_id": 123, "environment_name": "testing", "role": None, + "deleted": False, } diff --git a/tests/routes/applications/test_settings.py b/tests/routes/applications/test_settings.py index a9336b72..18a89014 100644 --- a/tests/routes/applications/test_settings.py +++ b/tests/routes/applications/test_settings.py @@ -506,7 +506,7 @@ def test_update_member(client, user_session, session): EnvironmentRoleFactory.create( environment=env, application_role=app_role, role=CSPRole.BASIC_ACCESS.value ) - EnvironmentRoleFactory.create( + suspended_role = EnvironmentRoleFactory.create( environment=env_1, application_role=app_role, role=CSPRole.BASIC_ACCESS.value ) @@ -524,8 +524,8 @@ def test_update_member(client, user_session, session): "environment_roles-0-role": CSPRole.TECHNICAL_READ.value, "environment_roles-0-environment_name": env.name, "environment_roles-1-environment_id": env_1.id, - "environment_roles-1-role": NO_ACCESS, "environment_roles-1-environment_name": env_1.name, + "environment_roles-1-deleted": "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, @@ -561,6 +561,8 @@ def test_update_member(client, user_session, session): # 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 suspended_role.status == EnvironmentRole.Status.DISABLED + assert suspended_role.deleted def test_revoke_invite(client, user_session): From 3a1a996469d8a178273bb1630b2e876badd91add Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Thu, 24 Oct 2019 18:12:28 -0400 Subject: [PATCH 3/9] Create macro for environment role field and update route so the correct data is passed to Environments.update_env_role to update or delete roles --- atst/routes/applications/settings.py | 11 ++- .../fragments/member_form_fields.html | 90 ++++++++++++++----- 2 files changed, 76 insertions(+), 25 deletions(-) diff --git a/atst/routes/applications/settings.py b/atst/routes/applications/settings.py index c1081a1b..37e45e50 100644 --- a/atst/routes/applications/settings.py +++ b/atst/routes/applications/settings.py @@ -374,14 +374,21 @@ def remove_member(application_id, application_role_id): @user_can(Permissions.EDIT_APPLICATION_MEMBER, message="update application member") def update_member(application_id, application_role_id): app_role = ApplicationRoles.get_by_id(application_role_id) - form = UpdateMemberForm(http_request.form) + application = Applications.get(application_id) + existing_env_roles_data = filter_env_roles_form_data( + app_role, application.environments + ) + form = UpdateMemberForm( + formdata=http_request.form, environment_roles=existing_env_roles_data + ) if form.validate(): ApplicationRoles.update_permission_sets(app_role, form.data["permission_sets"]) for env_role in form.environment_roles: environment = Environments.get(env_role.environment_id.data) - Environments.update_env_role(environment, app_role, env_role.data["role"]) + new_role = None if env_role.deleted.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) else: diff --git a/templates/applications/fragments/member_form_fields.html b/templates/applications/fragments/member_form_fields.html index a466a46d..eb4ba097 100644 --- a/templates/applications/fragments/member_form_fields.html +++ b/templates/applications/fragments/member_form_fields.html @@ -2,6 +2,72 @@ {% from "components/text_input.html" import TextInput %} {% from "components/phone_input.html" import PhoneInput %} +{% macro EnvRoleInput(field, member_role_id=None) %} + {% if field.role.data == "No Access" and not field.deleted.data -%} + +
+
+
+
+ +
+ {{ field.environment_name.data }} +
+

+ {{ field.role.data }} +

+ +
+
+ {{ field.role(**{"v-model": "value", "id": "{}-{}".format(field.role.name, member_role_id)}) }} +
+
+
+
+ + {% elif field.role.data != "No Access" and not field.deleted.data -%} +
+
+ +
+ {{ field.environment_name.data }} +
+

+ {{ field.role.data }} +

+ +
+
+ +
+
+ +
+ + {% set id = "{}-{}".format(field.deleted.name, member_role_id) %} + {{ field.deleted(id=id, checked=True, **{"v-model": "isChecked"}) }} + {{ field.deleted.label(for=id) | safe }} + +
+
+
+ +
+
+ {%- endif %} + {{ field.environment_id() }} +{% endmacro %} + {% macro PermsFields(form, new=False, member_role_id=None) %}

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

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

@@ -26,29 +92,7 @@

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

{% for environment_data in form.environment_roles %} - -
-
-
-
- -
- {{ environment_data.environment_name.data }} -
- -
-
- {{ environment_data.role(**{"v-model": "value"}) }} -
-
-
-
- - {{ environment_data.environment_id() }} + {{ EnvRoleInput(environment_data, member_role_id) }}
{% endfor %} From d40c11a8f65ceeb41bbe26e2a7fbd669f71dd944 Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Fri, 25 Oct 2019 10:40:38 -0400 Subject: [PATCH 4/9] 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() From f928b776a688068e2e50b7fded4dac2b4f9e8b7f Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Fri, 25 Oct 2019 11:44:15 -0400 Subject: [PATCH 5/9] Properly set deleted data for UpdateMemberForm and display suspended env access text Styling for env name and role in update app member perms form --- atst/domain/environment_roles.py | 8 ++ atst/routes/applications/settings.py | 8 +- styles/sections/_application_edit.scss | 18 ++-- .../fragments/member_form_fields.html | 90 ++++++++----------- tests/domain/test_environment_roles.py | 23 +++-- tests/domain/test_environments.py | 4 +- 6 files changed, 80 insertions(+), 71 deletions(-) diff --git a/atst/domain/environment_roles.py b/atst/domain/environment_roles.py index 6e69ac96..4dfb29c5 100644 --- a/atst/domain/environment_roles.py +++ b/atst/domain/environment_roles.py @@ -126,3 +126,11 @@ 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/routes/applications/settings.py b/atst/routes/applications/settings.py index 37e45e50..65f6d00e 100644 --- a/atst/routes/applications/settings.py +++ b/atst/routes/applications/settings.py @@ -77,12 +77,16 @@ def filter_env_roles_form_data(member, environments): "environment_id": str(env.id), "environment_name": env.name, "role": NO_ACCESS, - "deleted": env.deleted, + "deleted": False, } - env_roles_set = set(env.roles).intersection(set(member.environment_roles)) + env_roles_set = set(env.roles).intersection( + 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_roles_form_data.append(env_data) diff --git a/styles/sections/_application_edit.scss b/styles/sections/_application_edit.scss index 1c1c395d..94afb41e 100644 --- a/styles/sections/_application_edit.scss +++ b/styles/sections/_application_edit.scss @@ -92,15 +92,21 @@ text-align: left; .usa-input { - margin: $gap 0 $gap 0; + margin: $gap * 2; - .usa-input__title-inline { - margin-top: $gap; - margin-left: $gap; + &__title-inline { + font-weight: $font-bold; } - .form-row { - margin: 0; + &__help { + margin-bottom: 0; + } + + .env-role__no-access { + .usa-input__title-inline, + .usa-input__help { + color: $color-gray; + } } } diff --git a/templates/applications/fragments/member_form_fields.html b/templates/applications/fragments/member_form_fields.html index eb4ba097..5c43c277 100644 --- a/templates/applications/fragments/member_form_fields.html +++ b/templates/applications/fragments/member_form_fields.html @@ -3,45 +3,30 @@ {% from "components/phone_input.html" import PhoneInput %} {% macro EnvRoleInput(field, member_role_id=None) %} - {% if field.role.data == "No Access" and not field.deleted.data -%} - -
-
-
-
- -
- {{ field.environment_name.data }} -
-

- {{ field.role.data }} -

- -
-
- {{ field.role(**{"v-model": "value", "id": "{}-{}".format(field.role.name, member_role_id)}) }} -
-
-
+ {% set role = field.role.data if not field.deleted.data else "Access Suspended" %} +
+
+
+
+ {{ field.environment_name.data }} +
+

+ {{ role }} +

- - {% elif field.role.data != "No Access" and not field.deleted.data -%} -
-
- -
- {{ field.environment_name.data }} -
-

- {{ field.role.data }} -

- -
-
+
+
+ {% if field.role.data == "No Access" and not field.deleted.data -%} + +
+ {{ field.role(**{"v-model": "value", "id": "{}-{}".format(field.role.name, member_role_id)}) }} +
+ + {% elif field.role.data != "No Access" and not field.deleted.data -%} -
-
- -
- - {% set id = "{}-{}".format(field.deleted.name, member_role_id) %} - {{ field.deleted(id=id, checked=True, **{"v-model": "isChecked"}) }} - {{ field.deleted.label(for=id) | safe }} - -
-
-
+
+ + {% set id = "{}-{}".format(field.deleted.name, member_role_id) %} + {{ field.deleted(id=id, checked=True, **{"v-model": "isChecked"}) }} + {{ field.deleted.label(for=id) | safe }} + +
-
+ {% elif field.deleted.data -%} +

+ Suspended access cannot be modified. +

+ {%- endif %} + {{ field.environment_id() }}
- {%- endif %} - {{ field.environment_id() }} +
{% endmacro %} {% macro PermsFields(form, new=False, member_role_id=None) %} diff --git a/tests/domain/test_environment_roles.py b/tests/domain/test_environment_roles.py index 71bbbdf2..8f68207f 100644 --- a/tests/domain/test_environment_roles.py +++ b/tests/domain/test_environment_roles.py @@ -93,12 +93,21 @@ def test_disable_completed(application_role, environment): 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) +def test_get_for_update(application_role, environment): + EnvironmentRoleFactory.create( + application_role=application_role, environment=environment, deleted=True + ) + role = EnvironmentRoles.get_for_update(application_role.id, environment.id) assert role - assert role.application_role == app_role - assert role.environment == env + 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 112c33af..a3562741 100644 --- a/tests/domain/test_environments.py +++ b/tests/domain/test_environments.py @@ -61,9 +61,7 @@ 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 - ) + 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 ) From d33fcb60733ec74eac4f536f31f81f12d667eb20 Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Mon, 28 Oct 2019 15:59:23 -0400 Subject: [PATCH 6/9] Fix issues with deleting roles: 1. Prevents roles from being created with the role 'None' 2. Only call EnvironmentRoles.delete() if the env_role exists 3. Update the filter on the role field of the app member form to return 'No Access'. This fixed an issue where if a role was deleted, then other env roles belonging to the app member could not be updated because the role field of the deleted env_role was invalid --- atst/domain/environments.py | 8 ++++---- atst/forms/application_member.py | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/atst/domain/environments.py b/atst/domain/environments.py index 2a9c5f61..39d6214c 100644 --- a/atst/domain/environments.py +++ b/atst/domain/environments.py @@ -56,20 +56,20 @@ class Environments(object): 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: + elif not env_role and new_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: + if env_role and not new_role: + env_role.role = None updated = EnvironmentRoles.delete(application_role.id, environment.id) if updated: + db.session.add(env_role) db.session.commit() return updated diff --git a/atst/forms/application_member.py b/atst/forms/application_member.py index 152e0ddb..e44d2ad9 100644 --- a/atst/forms/application_member.py +++ b/atst/forms/application_member.py @@ -16,7 +16,7 @@ class EnvironmentForm(Form): environment_name, choices=ENV_ROLES, default=NO_ACCESS, - filters=[lambda x: None if x == "None" else x], + filters=[lambda x: NO_ACCESS if x == "None" else x], ) deleted = BooleanField("Revoke Access", default=False) From b653546768de41d20a2bac45266e6e0de844bcce Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Mon, 28 Oct 2019 16:49:46 -0400 Subject: [PATCH 7/9] Styling --- styles/sections/_application_edit.scss | 102 +++++++++++++-- .../fragments/member_form_fields.html | 118 +++++++++++------- 2 files changed, 168 insertions(+), 52 deletions(-) diff --git a/styles/sections/_application_edit.scss b/styles/sections/_application_edit.scss index 94afb41e..8ff79b7b 100644 --- a/styles/sections/_application_edit.scss +++ b/styles/sections/_application_edit.scss @@ -91,9 +91,23 @@ margin-bottom: 8 * $gap; text-align: left; - .usa-input { + .form-row { margin: $gap * 2; + } + .usa-alert { + margin: -0.5em 0; + padding: $gap; + background-image: unset; + + &-body { + padding: 0; + width: 100%; + display: unset; + } + } + + .usa-input { &__title-inline { font-weight: $font-bold; } @@ -102,12 +116,28 @@ margin-bottom: 0; } - .env-role__no-access { + &.env-role__no-access { .usa-input__title-inline, .usa-input__help { color: $color-gray; } } + + &__choices { + &.revoke-button label { + padding: 1rem 2rem; + + &::before { + display: none; + } + + &.link { + color: $color-blue; + text-decoration: underline; + text-align: right; + } + } + } } &__head { @@ -118,15 +148,69 @@ margin: 0.5em 0; } } - - .form-row { - margin: 0; - } } -.environment-name--gray { - font-weight: $font-normal; - color: $color-gray-medium; +.environment-role { + padding: ($gap * 2) 0; + + h4 { + margin-bottom: $gap / 4; + } + + &__users { + background-color: $color-gray-lightest; + padding: ($gap * 1.2) ($gap * 0.6); + font-size: $small-font-size; + display: flex; + flex-wrap: wrap; + + .environment-role__user { + background-color: $color-white; + border-radius: 0.5rem; + padding: ($gap / 2) $gap; + border: solid 2px $color-blue; + margin: $gap; + white-space: nowrap; + width: 20rem; + position: relative; + height: 3.6rem; + + &-field { + position: absolute; + background-color: $color-white; + margin-top: $gap * 2; + padding: $gap; + left: -0.1rem; + border: solid 1px $color-gray-light; + width: 20rem; + z-index: 3; + + .usa-input { + margin: 0; + + li { + background-color: $color-white; + border: none; + } + } + } + + &.unassigned { + border: solid 1px $color-gray-light; + } + + .icon-link { + padding: 0; + } + } + + .environment-role__no-user { + margin: $gap; + padding: ($gap / 2) $gap; + font-weight: $font-normal; + height: 3.6rem; + } + } } .application-list-item { diff --git a/templates/applications/fragments/member_form_fields.html b/templates/applications/fragments/member_form_fields.html index 5c43c277..7b57cac1 100644 --- a/templates/applications/fragments/member_form_fields.html +++ b/templates/applications/fragments/member_form_fields.html @@ -1,55 +1,87 @@ +{% from "components/alert.html" import Alert %} {% from "components/checkbox_input.html" import CheckboxInput %} {% 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" %} -
-
-
-
- {{ field.environment_name.data }} + {% if field.role.data != "No Access" and not field.deleted.data -%} + +
+ {% set id = "{}-{}".format(field.deleted.name, member_role_id) %} +
+
+
+ {{ field.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 }} +
-

- {{ role }} -

+
+ {% call Alert(level='warning') %} +
+
+
+ {{ field.environment_name.data }} +
+

+ Save changes to revoke access, this can not be undone. +

+
+
+ {{ field.deleted(id=id, checked=True, **{"v-model": "isChecked"}) }} + +
+
+ {% endcall %} +
+
+ + {% else %} +
+ +
+
+
+ {{ field.environment_name.data }} +
+

+ {{ role }} +

+
+
+
+ {% if field.role.data == "No Access" and not field.deleted.data -%} + +
+ {{ field.role(**{"v-model": "value", "id": "{}-{}".format(field.role.name, member_role_id)}) }} +
+ + {% elif field.deleted.data -%} +

+ Suspended access cannot be modified. +

+ {%- endif %} + {{ field.environment_id() }}
-
- {% if field.role.data == "No Access" and not field.deleted.data -%} - -
- {{ field.role(**{"v-model": "value", "id": "{}-{}".format(field.role.name, member_role_id)}) }} -
- - {% elif field.role.data != "No Access" and not field.deleted.data -%} - -
- - {% set id = "{}-{}".format(field.deleted.name, member_role_id) %} - {{ field.deleted(id=id, checked=True, **{"v-model": "isChecked"}) }} - {{ field.deleted.label(for=id) | safe }} - -
- - {% elif field.deleted.data -%} -

- Suspended access cannot be modified. -

- {%- endif %} - {{ field.environment_id() }} -
-
+ {% endif %} {% endmacro %} {% macro PermsFields(form, new=False, member_role_id=None) %} From e8f21acf5bd117dfd02ffe63bca1b14c99a01ab6 Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Thu, 31 Oct 2019 10:12:41 -0400 Subject: [PATCH 8/9] 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") %}
    {% for member in env['members'] %} + {% set status = ": Access Suspended" if member['status'] == 'disabled' %}
  • - {{ member }} + {{ member['user_name'] }}{{ status }}
    • {% endfor %}
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 From 06a36f23bc191f2be7e86b85d02a795ca6fc7043 Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Tue, 12 Nov 2019 16:54:01 -0500 Subject: [PATCH 9/9] Raise error when a user attempts to update a disabled env role --- atst/domain/environments.py | 20 +++++++++----------- atst/domain/exceptions.py | 10 ++++++++++ tests/domain/test_environments.py | 31 +++++++++++-------------------- 3 files changed, 30 insertions(+), 31 deletions(-) diff --git a/atst/domain/environments.py b/atst/domain/environments.py index f36c1a3b..2114ce21 100644 --- a/atst/domain/environments.py +++ b/atst/domain/environments.py @@ -14,7 +14,7 @@ from atst.models import ( ) from atst.domain.environment_roles import EnvironmentRoles -from .exceptions import NotFoundError +from .exceptions import NotFoundError, DisabledError class Environments(object): @@ -57,33 +57,31 @@ class Environments(object): @classmethod 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.status == EnvironmentRole.Status.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 ): env_role.role = new_role - updated = True + db.session.add(env_role) elif not env_role and new_role: env_role = EnvironmentRoles.create( application_role=application_role, environment=environment, role=new_role, ) - updated = True + db.session.add(env_role) if env_role and not new_role: EnvironmentRoles.disable(env_role.id) - updated = True - if updated: - db.session.add(env_role) - db.session.commit() - - return updated + db.session.commit() @classmethod def revoke_access(cls, environment, target_user): diff --git a/atst/domain/exceptions.py b/atst/domain/exceptions.py index a89aa986..ab60ad61 100644 --- a/atst/domain/exceptions.py +++ b/atst/domain/exceptions.py @@ -53,3 +53,13 @@ class ClaimFailedException(Exception): f"Could not acquire claim for {resource.__class__.__name__} {resource.id}." ) super().__init__(message) + + +class DisabledError(Exception): + def __init__(self, resource_name, resource_id=None): + self.resource_name = resource_name + self.resource_id = resource_id + + @property + def message(self): + return f"Cannot update disabled {self.resource_name} {self.resource_id}." diff --git a/tests/domain/test_environments.py b/tests/domain/test_environments.py index 277c47ea..55d54f44 100644 --- a/tests/domain/test_environments.py +++ b/tests/domain/test_environments.py @@ -4,7 +4,7 @@ from uuid import uuid4 from atst.domain.environments import Environments from atst.domain.environment_roles import EnvironmentRoles -from atst.domain.exceptions import NotFoundError +from atst.domain.exceptions import NotFoundError, DisabledError from atst.models.environment_role import CSPRole, EnvironmentRole from tests.factories import ( @@ -28,8 +28,7 @@ def test_create_environments(): def test_update_env_role(): env_role = EnvironmentRoleFactory.create(role=CSPRole.BASIC_ACCESS.value) new_role = CSPRole.TECHNICAL_READ.value - - assert Environments.update_env_role( + Environments.update_env_role( env_role.environment, env_role.application_role, new_role ) assert env_role.role == new_role @@ -37,10 +36,7 @@ def test_update_env_role(): def test_update_env_role_no_access(): env_role = EnvironmentRoleFactory.create(role=CSPRole.BASIC_ACCESS.value) - - assert Environments.update_env_role( - env_role.environment, env_role.application_role, None - ) + Environments.update_env_role(env_role.environment, env_role.application_role, None) assert not EnvironmentRoles.get( env_role.application_role.id, env_role.environment.id @@ -49,21 +45,16 @@ def test_update_env_role_no_access(): assert env_role.status == EnvironmentRole.Status.DISABLED -def test_update_env_role_no_change(): - env_role = EnvironmentRoleFactory.create(role=CSPRole.BASIC_ACCESS.value) - new_role = CSPRole.BASIC_ACCESS.value - - assert not Environments.update_env_role( - env_role.environment, env_role.application_role, new_role - ) - - -def test_update_env_role_deleted_role(): +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) - assert not Environments.update_env_role( - env_role.environment, env_role.application_role, CSPRole.TECHNICAL_READ.value - ) + + 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