From 27a4ef12c6373c04d73b4fd52f677c0c7f5377b0 Mon Sep 17 00:00:00 2001 From: George Drummond Date: Mon, 13 May 2019 10:14:48 -0400 Subject: [PATCH 1/5] Delete an application member --- atst/domain/applications.py | 13 ++++++ atst/routes/applications/team.py | 28 +++++++++++++ atst/utils/flash.py | 5 +++ js/components/forms/base_form.js | 12 +++--- .../fragments/applications/edit_team.html | 8 ++++ templates/portfolios/applications/team.html | 32 +++++++++++++++ tests/domain/test_applications.py | 31 ++++++++++++-- tests/routes/applications/test_team.py | 41 +++++++++++++++++++ translations.yaml | 6 +++ 9 files changed, 167 insertions(+), 9 deletions(-) diff --git a/atst/domain/applications.py b/atst/domain/applications.py index 091e8c37..964439c4 100644 --- a/atst/domain/applications.py +++ b/atst/domain/applications.py @@ -3,6 +3,7 @@ from sqlalchemy.orm.exc import NoResultFound from atst.database import db from . import BaseDomainClass from atst.domain.application_roles import ApplicationRoles +from atst.domain.environment_roles import EnvironmentRoles from atst.domain.environments import Environments from atst.domain.exceptions import NotFoundError from atst.domain.users import Users @@ -100,3 +101,15 @@ class Applications(BaseDomainClass): Environments.add_member(environment, user, env_role_data.get("role")) return application_role + + @classmethod + def remove_member(cls, application, user): + application_role = ApplicationRoles.get( + user_id=user.id, application_id=application.id + ) + + db.session.delete(application_role) + db.session.commit() + + for env in application.environments: + EnvironmentRoles.delete(user_id=user.id, environment_id=env.id) diff --git a/atst/routes/applications/team.py b/atst/routes/applications/team.py index 3b823f7a..aa06e0b5 100644 --- a/atst/routes/applications/team.py +++ b/atst/routes/applications/team.py @@ -158,3 +158,31 @@ def create_member(application_id): _anchor="application-members", ) ) + + +@applications_bp.route( + "/applications//members//delete", methods=["POST"] +) +# TODO: Is this correct?? +@user_can(Permissions.EDIT_APPLICATION_MEMBER, message="remove application member") +def remove_member(application_id, user_id): + application_role = ApplicationRoles.get( + application_id=application_id, user_id=user_id + ) + + Applications.remove_member(application=g.application, user=application_role.user) + + flash( + "application_member_removed", + user_name=application_role.user.full_name, + application_name=g.application.name, + ) + + return redirect( + url_for( + "applications.team", + _anchor="application-members", + application_id=g.application.id, + fragment="application-members", + ) + ) diff --git a/atst/utils/flash.py b/atst/utils/flash.py index f2b07ed6..28e09b3a 100644 --- a/atst/utils/flash.py +++ b/atst/utils/flash.py @@ -2,6 +2,11 @@ from flask import flash, render_template_string from atst.utils.localization import translate MESSAGES = { + "application_member_removed": { + "title_template": "Team member removed from application", + "message_template": "You have successfully deleted {{ user_name }} from {{ application_name }}", + "category": "success", + }, "environment_deleted": { "title_template": "{{ environment_name }} deleted", "message_template": 'The environment "{{ environment_name }}" has been deleted', diff --git a/js/components/forms/base_form.js b/js/components/forms/base_form.js index a7259eae..86612659 100644 --- a/js/components/forms/base_form.js +++ b/js/components/forms/base_form.js @@ -1,12 +1,12 @@ import ally from 'ally.js' -import checkboxinput from '../checkbox_input' import DateSelector from '../date_selector' import FormMixin from '../../mixins/form' -import levelofwarrant from '../levelofwarrant' import Modal from '../../mixins/modal' -import multicheckboxinput from '../multi_checkbox_input' import MultiStepModalForm from './multi_step_modal_form' +import checkboxinput from '../checkbox_input' +import levelofwarrant from '../levelofwarrant' +import multicheckboxinput from '../multi_checkbox_input' import optionsinput from '../options_input' import textinput from '../text_input' import toggler from '../toggler' @@ -14,12 +14,12 @@ import toggler from '../toggler' export default { name: 'base-form', components: { - checkboxinput, DateSelector, - levelofwarrant, Modal, - multicheckboxinput, MultiStepModalForm, + checkboxinput, + levelofwarrant, + multicheckboxinput, optionsinput, textinput, toggler, diff --git a/templates/fragments/applications/edit_team.html b/templates/fragments/applications/edit_team.html index d5d9f300..ea8ac9d0 100644 --- a/templates/fragments/applications/edit_team.html +++ b/templates/fragments/applications/edit_team.html @@ -3,6 +3,7 @@ {{ team_form.csrf_token }} {% for member_form in team_form.members %} + {% set delete_modal_id = "delete-user-{}".format(member_form.id) %} {% set environment_roles_form = member_form.environment_roles %} {% set permissions_form = member_form.permission_sets %} @@ -38,6 +39,13 @@ {{ environment_form.environment_name.data }} {% endfor %} + {% if user_can(permissions.EDIT_APPLICATION_MEMBER) %} +
  • + + {{ "portfolios.members.archive_button" | translate }} + +
    • + {% endif %} {% endcall %} {{ member_form.user_id() }} diff --git a/templates/portfolios/applications/team.html b/templates/portfolios/applications/team.html index c7270c05..d0af0b54 100644 --- a/templates/portfolios/applications/team.html +++ b/templates/portfolios/applications/team.html @@ -6,6 +6,9 @@ {% from "components/toggle_list.html" import ToggleButton, ToggleSection %} {% from "components/multi_step_modal_form.html" import MultiStepModalForm %} {% import "fragments/applications/new_member_modal_content.html" as member_steps %} +{% from "components/alert.html" import Alert %} +{% from "components/delete_confirmation.html" import DeleteConfirmation %} +{% from "components/modal.html" import Modal %} {% set secondary_breadcrumb = 'portfolios.applications.team_settings.title' | translate({ "application_name": application.name }) %} @@ -109,6 +112,35 @@ + + {% if user_can(permissions.EDIT_APPLICATION_MEMBER) %} + {% for member_form in team_form.members %} + {% set delete_modal_id = "delete-user-{}".format(member_form.id) %} + {% call Modal(name=delete_modal_id, dismissable=True) %} +

    + {{ "portfolios.applications.remove_member.header" | translate }} +

    + + {{ + Alert( + title=("portfolios.applications.remove_member.alert.title" | translate), + message=("portfolios.applications.remove_member.alert.message" | translate({"user_name": member_form.user_name.data})), + level="warning" + ) + }} + + {{ + DeleteConfirmation( + modal_id=delete_modal_id, + delete_text=('portfolios.applications.remove_member.button' | translate), + delete_action=url_for('applications.remove_member', application_id=application.id, user_id=member_form.data.user_id), + form=member_form + ) + }} + {% endcall %} + {% endfor %} + {% endif %} + {% if user_can(permissions.CREATE_APPLICATION_MEMBER) %} {% import "fragments/applications/new_member_modal_content.html" as member_steps %} {{ MultiStepModalForm( diff --git a/tests/domain/test_applications.py b/tests/domain/test_applications.py index 84aa80d4..9dccd0f7 100644 --- a/tests/domain/test_applications.py +++ b/tests/domain/test_applications.py @@ -2,16 +2,19 @@ import pytest from uuid import uuid4 from atst.models import CSPRole, ApplicationRoleStatus +from atst.domain.application_roles import ApplicationRoles from atst.domain.applications import Applications -from atst.domain.permission_sets import PermissionSets +from atst.domain.environment_roles import EnvironmentRoles from atst.domain.exceptions import NotFoundError +from atst.domain.permission_sets import PermissionSets from tests.factories import ( ApplicationFactory, ApplicationRoleFactory, - UserFactory, - PortfolioFactory, EnvironmentFactory, + EnvironmentRoleFactory, + PortfolioFactory, + UserFactory, ) @@ -155,3 +158,25 @@ def test_for_user(): assert len(portfolio.applications) == 4 user_applications = Applications.for_user(user, portfolio) assert len(user_applications) == 2 + + +def test_remove_member(): + application = ApplicationFactory.create() + user = UserFactory.create() + member_role = ApplicationRoleFactory.create(application=application, user=user) + environment = EnvironmentFactory.create(application=application) + environment_role = EnvironmentRoleFactory.create(user=user, environment=environment) + + assert member_role == ApplicationRoles.get( + user_id=user.id, application_id=application.id + ) + + Applications.remove_member(application=application, user=member_role.user) + + with pytest.raises(NotFoundError): + ApplicationRoles.get(user_id=user.id, application_id=application.id) + + # + # TODO: Why does above raise NotFoundError and this returns None + # + assert EnvironmentRoles.get(user_id=user.id, environment_id=environment.id) == None diff --git a/tests/routes/applications/test_team.py b/tests/routes/applications/test_team.py index 108c4f25..39aaf533 100644 --- a/tests/routes/applications/test_team.py +++ b/tests/routes/applications/test_team.py @@ -1,4 +1,5 @@ import pytest +import uuid from flask import url_for from atst.domain.permission_sets import PermissionSets @@ -128,3 +129,43 @@ def test_create_member(client, user_session): assert user.application_roles[0].application == application assert len(user.environment_roles) == 1 assert user.environment_roles[0].environment == env + + +def test_remove_member_success(client, user_session): + user = UserFactory.create() + application = ApplicationFactory.create() + application_role = ApplicationRoleFactory.create(application=application, user=user) + + user_session(application.portfolio.owner) + + response = client.post( + url_for( + "applications.remove_member", application_id=application.id, user_id=user.id + ) + ) + + assert response.status_code == 302 + assert response.location == url_for( + "applications.team", + _anchor="application-members", + _external=True, + application_id=application.id, + fragment="application-members", + ) + + +def test_remove_member_failure(client, user_session): + user = UserFactory.create() + application = ApplicationFactory.create() + + user_session(application.portfolio.owner) + + response = client.post( + url_for( + "applications.remove_member", + application_id=application.id, + user_id=uuid.uuid4(), + ) + ) + + assert response.status_code == 404 diff --git a/translations.yaml b/translations.yaml index 15f39fa8..41629c98 100644 --- a/translations.yaml +++ b/translations.yaml @@ -416,6 +416,12 @@ portfolios: app_settings_text: App settings create_button_text: Create csp_console_text: CSP console + remove_member: + alert: + message: '{user_name} will no longer be able to access this application' + title: Warning! This action is permanent. + button: Remove member + header: Are you sure you want to remove this team member? delete: alert: message: You will lose access to this application and all of its reporting and metrics tools. The activity log will be retained. From 4aea2640264e062d10f46cf0dc59050832c09906 Mon Sep 17 00:00:00 2001 From: George Drummond Date: Tue, 14 May 2019 11:37:33 -0400 Subject: [PATCH 2/5] Avoid double lookup --- atst/domain/applications.py | 6 +++--- atst/routes/applications/team.py | 10 ++++------ tests/domain/test_applications.py | 2 +- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/atst/domain/applications.py b/atst/domain/applications.py index 964439c4..f4b834ab 100644 --- a/atst/domain/applications.py +++ b/atst/domain/applications.py @@ -103,13 +103,13 @@ class Applications(BaseDomainClass): return application_role @classmethod - def remove_member(cls, application, user): + def remove_member(cls, application, user_id): application_role = ApplicationRoles.get( - user_id=user.id, application_id=application.id + user_id=user_id, application_id=application.id ) db.session.delete(application_role) db.session.commit() for env in application.environments: - EnvironmentRoles.delete(user_id=user.id, environment_id=env.id) + EnvironmentRoles.delete(user_id=user_id, environment_id=env.id) diff --git a/atst/routes/applications/team.py b/atst/routes/applications/team.py index aa06e0b5..ed075023 100644 --- a/atst/routes/applications/team.py +++ b/atst/routes/applications/team.py @@ -8,6 +8,7 @@ from atst.domain.authz.decorator import user_can_access_decorator as user_can from atst.domain.environment_roles import EnvironmentRoles from atst.domain.exceptions import AlreadyExistsError from atst.domain.permission_sets import PermissionSets +from atst.domain.users import Users from atst.forms.application_member import NewForm as NewMemberForm from atst.forms.team import TeamForm from atst.models import Permissions @@ -166,15 +167,12 @@ def create_member(application_id): # TODO: Is this correct?? @user_can(Permissions.EDIT_APPLICATION_MEMBER, message="remove application member") def remove_member(application_id, user_id): - application_role = ApplicationRoles.get( - application_id=application_id, user_id=user_id - ) - - Applications.remove_member(application=g.application, user=application_role.user) + Applications.remove_member(application=g.application, user_id=user_id) + user = Users.get(user_id) flash( "application_member_removed", - user_name=application_role.user.full_name, + user_name=user.full_name, application_name=g.application.name, ) diff --git a/tests/domain/test_applications.py b/tests/domain/test_applications.py index 9dccd0f7..6848795d 100644 --- a/tests/domain/test_applications.py +++ b/tests/domain/test_applications.py @@ -171,7 +171,7 @@ def test_remove_member(): user_id=user.id, application_id=application.id ) - Applications.remove_member(application=application, user=member_role.user) + Applications.remove_member(application=application, user_id=member_role.user.id) with pytest.raises(NotFoundError): ApplicationRoles.get(user_id=user.id, application_id=application.id) From f1299dc4b0e22af8f07714dbb56c9f9009a652f8 Mon Sep 17 00:00:00 2001 From: George Drummond Date: Tue, 14 May 2019 13:39:20 -0400 Subject: [PATCH 3/5] Add DELETE_APPLICATION_MEMBER permission --- atst/domain/permission_sets.py | 2 ++ atst/models/permissions.py | 1 + atst/routes/applications/team.py | 3 +-- templates/fragments/applications/edit_team.html | 2 +- templates/portfolios/applications/team.html | 2 +- 5 files changed, 6 insertions(+), 4 deletions(-) diff --git a/atst/domain/permission_sets.py b/atst/domain/permission_sets.py index 52131a20..d0d99031 100644 --- a/atst/domain/permission_sets.py +++ b/atst/domain/permission_sets.py @@ -88,6 +88,7 @@ _PORTFOLIO_APP_MGMT_PERMISSION_SETS = [ Permissions.CREATE_APPLICATION, Permissions.DELETE_APPLICATION, Permissions.EDIT_APPLICATION_MEMBER, + Permissions.DELETE_APPLICATION_MEMBER, Permissions.CREATE_APPLICATION_MEMBER, Permissions.EDIT_ENVIRONMENT, Permissions.CREATE_ENVIRONMENT, @@ -205,6 +206,7 @@ _APPLICATION_TEAM_PERMISSION_SET = { "display_name": "Manage team", "permissions": [ Permissions.EDIT_APPLICATION_MEMBER, + Permissions.DELETE_APPLICATION_MEMBER, Permissions.CREATE_APPLICATION_MEMBER, Permissions.ASSIGN_ENVIRONMENT_MEMBER, ], diff --git a/atst/models/permissions.py b/atst/models/permissions.py index 98f25b36..e4b797e4 100644 --- a/atst/models/permissions.py +++ b/atst/models/permissions.py @@ -11,6 +11,7 @@ class Permissions(object): DELETE_APPLICATION = "delete_application" VIEW_APPLICATION_MEMBER = "view_application_member" EDIT_APPLICATION_MEMBER = "edit_application_member" + DELETE_APPLICATION_MEMBER = "delete_application_member" CREATE_APPLICATION_MEMBER = "create_application_member" VIEW_ENVIRONMENT = "view_environment" EDIT_ENVIRONMENT = "edit_environment" diff --git a/atst/routes/applications/team.py b/atst/routes/applications/team.py index ed075023..97783f89 100644 --- a/atst/routes/applications/team.py +++ b/atst/routes/applications/team.py @@ -164,8 +164,7 @@ def create_member(application_id): @applications_bp.route( "/applications//members//delete", methods=["POST"] ) -# TODO: Is this correct?? -@user_can(Permissions.EDIT_APPLICATION_MEMBER, message="remove application member") +@user_can(Permissions.DELETE_APPLICATION_MEMBER, message="remove application member") def remove_member(application_id, user_id): Applications.remove_member(application=g.application, user_id=user_id) user = Users.get(user_id) diff --git a/templates/fragments/applications/edit_team.html b/templates/fragments/applications/edit_team.html index ea8ac9d0..81baff38 100644 --- a/templates/fragments/applications/edit_team.html +++ b/templates/fragments/applications/edit_team.html @@ -39,7 +39,7 @@ {{ environment_form.environment_name.data }} {% endfor %} - {% if user_can(permissions.EDIT_APPLICATION_MEMBER) %} + {% if user_can(permissions.DELETE_APPLICATION_MEMBER) %}
  • {{ "portfolios.members.archive_button" | translate }} diff --git a/templates/portfolios/applications/team.html b/templates/portfolios/applications/team.html index d0af0b54..7e75e3b9 100644 --- a/templates/portfolios/applications/team.html +++ b/templates/portfolios/applications/team.html @@ -113,7 +113,7 @@ - {% if user_can(permissions.EDIT_APPLICATION_MEMBER) %} + {% if user_can(permissions.DELETE_APPLICATION_MEMBER) %} {% for member_form in team_form.members %} {% set delete_modal_id = "delete-user-{}".format(member_form.id) %} {% call Modal(name=delete_modal_id, dismissable=True) %} From b884c8a762eb2ad92476345513aafd35babcbdcb Mon Sep 17 00:00:00 2001 From: George Drummond Date: Tue, 14 May 2019 13:48:43 -0400 Subject: [PATCH 4/5] Use common translation --- templates/portfolios/applications/settings.html | 4 ++-- templates/portfolios/applications/team.html | 2 +- translations.yaml | 2 -- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/templates/portfolios/applications/settings.html b/templates/portfolios/applications/settings.html index 7de3e1b5..124d7ca4 100644 --- a/templates/portfolios/applications/settings.html +++ b/templates/portfolios/applications/settings.html @@ -89,8 +89,8 @@ {{ Alert( - title="portfolios.applications.delete.alert.title" | translate, - message="portfolios.applications.delete.alert.message" | translate, + title=("components.modal.destructive_title" | translate), + message=("portfolios.applications.delete.alert.message" | translate), level="warning" ) }} diff --git a/templates/portfolios/applications/team.html b/templates/portfolios/applications/team.html index 7e75e3b9..1cda2139 100644 --- a/templates/portfolios/applications/team.html +++ b/templates/portfolios/applications/team.html @@ -123,7 +123,7 @@ {{ Alert( - title=("portfolios.applications.remove_member.alert.title" | translate), + title=("components.modal.destructive_title" | translate), message=("portfolios.applications.remove_member.alert.message" | translate({"user_name": member_form.user_name.data})), level="warning" ) diff --git a/translations.yaml b/translations.yaml index 41629c98..0cd7e783 100644 --- a/translations.yaml +++ b/translations.yaml @@ -419,13 +419,11 @@ portfolios: remove_member: alert: message: '{user_name} will no longer be able to access this application' - title: Warning! This action is permanent. button: Remove member header: Are you sure you want to remove this team member? delete: alert: message: You will lose access to this application and all of its reporting and metrics tools. The activity log will be retained. - title: Warning! This action is permanent. button: Delete application header: Are you sure you want to delete this application? environments: From 9927b22783e2deaac19e239fe068cd5e6e3d22e5 Mon Sep 17 00:00:00 2001 From: George Drummond Date: Tue, 14 May 2019 15:26:51 -0400 Subject: [PATCH 5/5] Disable rather than delete application_roles --- atst/domain/applications.py | 3 ++- tests/domain/test_applications.py | 6 ++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/atst/domain/applications.py b/atst/domain/applications.py index f4b834ab..c5a1d94a 100644 --- a/atst/domain/applications.py +++ b/atst/domain/applications.py @@ -108,7 +108,8 @@ class Applications(BaseDomainClass): user_id=user_id, application_id=application.id ) - db.session.delete(application_role) + application_role.status = ApplicationRoleStatus.DISABLED + db.session.add(application_role) db.session.commit() for env in application.environments: diff --git a/tests/domain/test_applications.py b/tests/domain/test_applications.py index 6848795d..ccd3c5e9 100644 --- a/tests/domain/test_applications.py +++ b/tests/domain/test_applications.py @@ -173,8 +173,10 @@ def test_remove_member(): Applications.remove_member(application=application, user_id=member_role.user.id) - with pytest.raises(NotFoundError): - ApplicationRoles.get(user_id=user.id, application_id=application.id) + assert ( + ApplicationRoles.get(user_id=user.id, application_id=application.id).status + == ApplicationRoleStatus.DISABLED + ) # # TODO: Why does above raise NotFoundError and this returns None