From 6f1eb43de4f3b6ad33736fe52ab9cd4263e4712a Mon Sep 17 00:00:00 2001 From: George Drummond Date: Tue, 26 Mar 2019 15:45:32 -0400 Subject: [PATCH 01/11] Remove Portfolio User --- atst/domain/portfolio_roles.py | 9 ++++ atst/routes/portfolios/index.py | 28 ++++++++++ atst/utils/flash.py | 5 ++ styles/components/_portfolio_layout.scss | 4 ++ templates/fragments/admin/members_edit.html | 9 +++- .../fragments/admin/portfolio_members.html | 31 +++++++++++ tests/domain/test_portfolio_roles.py | 8 +++ .../portfolios/test_portfolios_index.py | 52 +++++++++++++++++++ 8 files changed, 145 insertions(+), 1 deletion(-) diff --git a/atst/domain/portfolio_roles.py b/atst/domain/portfolio_roles.py index 153d4707..cb123b4a 100644 --- a/atst/domain/portfolio_roles.py +++ b/atst/domain/portfolio_roles.py @@ -121,6 +121,15 @@ class PortfolioRoles(object): ) return PermissionSets.get_many(perms_set_names) + @classmethod + def disable(cls, portfolio_role): + portfolio_role.status = PortfolioRoleStatus.DISABLED + + db.session.add(portfolio_role) + db.session.commit() + + return portfolio_role + @classmethod def update(cls, portfolio_role, set_names): new_permission_sets = PortfolioRoles._permission_sets_for_names(set_names) diff --git a/atst/routes/portfolios/index.py b/atst/routes/portfolios/index.py index 541a8194..767f56aa 100644 --- a/atst/routes/portfolios/index.py +++ b/atst/routes/portfolios/index.py @@ -15,6 +15,7 @@ import atst.forms.portfolio_member as member_forms from atst.models.permissions import Permissions from atst.domain.permission_sets import PermissionSets from atst.domain.authz.decorator import user_can_access_decorator as user_can +from atst.utils.flash import formatted_flash as flash @portfolios_bp.route("/portfolios") @@ -174,3 +175,30 @@ def portfolio_reports(portfolio_id): expiration_date=expiration_date, remaining_days=remaining_days, ) + + +@portfolios_bp.route( + "/portfolios//members//delete", methods=["POST"] +) +@user_can(Permissions.EDIT_PORTFOLIO_USERS, message="update portfolio members") +def remove_member(portfolio_id, member_id): + if member_id == str(g.current_user.id): + raise UnauthorizedError( + user=user, message="you cant remove yourself from the portfolio" + ) + + portfolio = Portfolios.get(g.current_user, portfolio_id) + portfolio_role = PortfolioRoles.get(portfolio_id=portfolio_id, user_id=member_id) + + PortfolioRoles.disable(portfolio_role=portfolio_role) + + flash("portfolio_member_removed", member_name=portfolio_role.user.full_name) + + return redirect( + url_for( + "portfolios.portfolio_admin", + portfolio_id=portfolio.id, + _anchor="portfolio-members", + fragment="portfolio-members", + ) + ) diff --git a/atst/utils/flash.py b/atst/utils/flash.py index 89f81132..1fc6a740 100644 --- a/atst/utils/flash.py +++ b/atst/utils/flash.py @@ -138,6 +138,11 @@ MESSAGES = { """, "category": "error", }, + "portfolio_member_removed": { + "title_template": "Portfolio Member Removed", + "message_template": "You have successfully removed {{ member_name }} from the portfolio.", + "category": "success", + }, } diff --git a/styles/components/_portfolio_layout.scss b/styles/components/_portfolio_layout.scss index 539ef47b..7a33345b 100644 --- a/styles/components/_portfolio_layout.scss +++ b/styles/components/_portfolio_layout.scss @@ -291,6 +291,10 @@ height: 4rem; } + .usa-button-danger { + background: $color-red; + } + .members-table-footer { float: right; padding: 3 * $gap; diff --git a/templates/fragments/admin/members_edit.html b/templates/fragments/admin/members_edit.html index 0d873622..04d2ed21 100644 --- a/templates/fragments/admin/members_edit.html +++ b/templates/fragments/admin/members_edit.html @@ -1,3 +1,7 @@ +{% from "components/confirmation_button.html" import ConfirmationButton %} + +{% set modal_id = "portfolio_id_{}_user_id_{}".format(portfolio.id, user.id) %} + {% for subform in member_perms_form.members_permissions %} {{ subform.member.data }} @@ -14,7 +18,10 @@ {{ OptionsInput(subform.perms_reporting, label=False) }} {{ OptionsInput(subform.perms_portfolio_mgmt, label=False) }} - + + + {{ "portfolios.members.archive_button" | translate }} + {{ subform.user_id() }} diff --git a/templates/fragments/admin/portfolio_members.html b/templates/fragments/admin/portfolio_members.html index 08e856e0..da980c3d 100644 --- a/templates/fragments/admin/portfolio_members.html +++ b/templates/fragments/admin/portfolio_members.html @@ -1,6 +1,9 @@ {% from "components/icon.html" import Icon %} {% from "components/options_input.html" import OptionsInput %} +{% from "components/modal.html" import Modal %} +{% from "components/alert.html" import Alert %} +
{% if g.matchesPath("portfolio-members") %} @@ -51,6 +54,34 @@ {% endif %} + + {% if user_can(permissions.EDIT_PORTFOLIO_USERS) %} + {% for member in portfolio.members %} + {% set modal_id = "portfolio_id_{}_user_id_{}".format(portfolio.id, user.id) %} + {% call Modal(name=modal_id, dismissable=False) %} +

Are you sure you want to archive this user?

+ + {{ + Alert( + title="Warning! You are about to archive a user from the portfolio admin.", + message="User will be removed from the portfolio, but their log history will be retained.", + level="warning" + ) + }} + +
+
+ {{ member_perms_form.csrf_token }} + +
+ Cancel +
+ {% endcall %} + {% endfor %} + {% endif %} +
{% if user_can(permissions.EDIT_PORTFOLIO_USERS) %} diff --git a/tests/domain/test_portfolio_roles.py b/tests/domain/test_portfolio_roles.py index 5bdc9bc0..fedd7faf 100644 --- a/tests/domain/test_portfolio_roles.py +++ b/tests/domain/test_portfolio_roles.py @@ -29,3 +29,11 @@ def test_add_portfolio_role_with_permission_sets(): ] actual_names = [prms.name for prms in port_role.permission_sets] assert expected_names == expected_names + + +def test_disable_portfolio_role(): + portfolio_role = PortfolioRoleFactory.create(status=PortfolioRoleStatus.ACTIVE) + assert portfolio_role.status == PortfolioRoleStatus.ACTIVE + + PortfolioRoles.disable(portfolio_role=portfolio_role) + assert portfolio_role.status == PortfolioRoleStatus.DISABLED diff --git a/tests/routes/portfolios/test_portfolios_index.py b/tests/routes/portfolios/test_portfolios_index.py index 7f631c52..d4fdeaba 100644 --- a/tests/routes/portfolios/test_portfolios_index.py +++ b/tests/routes/portfolios/test_portfolios_index.py @@ -2,6 +2,8 @@ from flask import url_for from atst.domain.permission_sets import PermissionSets from atst.models.permissions import Permissions +from atst.domain.portfolio_roles import PortfolioRoles +from atst.models.portfolio_role import Status as PortfolioRoleStatus from tests.factories import ( random_future_date, @@ -81,6 +83,56 @@ def test_portfolio_admin_screen_when_not_ppoc(client, user_session): assert translate("fragments.ppoc.update_btn").encode("utf8") not in response.data +def test_remove_portfolio_member(client, user_session): + portfolio = PortfolioFactory.create() + + user = UserFactory.create() + PortfolioRoleFactory.create(portfolio=portfolio, user=user) + + user_session(portfolio.owner) + + response = client.post( + url_for( + "portfolios.remove_member", portfolio_id=portfolio.id, member_id=user.id + ), + follow_redirects=False, + ) + + assert response.status_code == 302 + assert response.headers["Location"] == url_for( + "portfolios.portfolio_admin", + portfolio_id=portfolio.id, + _anchor="portfolio-members", + fragment="portfolio-members", + _external=True, + ) + assert ( + PortfolioRoles.get(portfolio_id=portfolio.id, user_id=user.id).status + == PortfolioRoleStatus.DISABLED + ) + + +def test_remove_portfolio_member_self(client, user_session): + portfolio = PortfolioFactory.create() + + user_session(portfolio.owner) + + response = client.post( + url_for( + "portfolios.remove_member", + portfolio_id=portfolio.id, + member_id=portfolio.owner.id, + ), + follow_redirects=False, + ) + + assert response.status_code == 500 + assert ( + PortfolioRoles.get(portfolio_id=portfolio.id, user_id=portfolio.owner.id).status + == PortfolioRoleStatus.ACTIVE + ) + + def test_portfolio_reports(client, user_session): portfolio = PortfolioFactory.create( applications=[ From 6d7400b7d2829af00ec713d1e34d8643cd1c41a2 Mon Sep 17 00:00:00 2001 From: George Drummond Date: Fri, 29 Mar 2019 14:14:21 -0400 Subject: [PATCH 02/11] Add in correct modal close call --- templates/fragments/admin/portfolio_members.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/fragments/admin/portfolio_members.html b/templates/fragments/admin/portfolio_members.html index da980c3d..64e0a58b 100644 --- a/templates/fragments/admin/portfolio_members.html +++ b/templates/fragments/admin/portfolio_members.html @@ -76,7 +76,7 @@ {{ "portfolios.members.archive_button" | translate }} - Cancel + Cancel
{% endcall %} {% endfor %} From bb5b8e212264acb3e13b97fde9745d2369525202 Mon Sep 17 00:00:00 2001 From: George Drummond Date: Fri, 29 Mar 2019 14:19:44 -0400 Subject: [PATCH 03/11] Fix incorrect modal id --- templates/fragments/admin/portfolio_members.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/fragments/admin/portfolio_members.html b/templates/fragments/admin/portfolio_members.html index 64e0a58b..d9bdbc81 100644 --- a/templates/fragments/admin/portfolio_members.html +++ b/templates/fragments/admin/portfolio_members.html @@ -57,7 +57,7 @@ {% if user_can(permissions.EDIT_PORTFOLIO_USERS) %} {% for member in portfolio.members %} - {% set modal_id = "portfolio_id_{}_user_id_{}".format(portfolio.id, user.id) %} + {% set modal_id = "portfolio_id_{}_user_id_{}".format(portfolio.id, member.user_id) %} {% call Modal(name=modal_id, dismissable=False) %}

Are you sure you want to archive this user?

From a22b416c6a749751341696372932de4a8904ee30 Mon Sep 17 00:00:00 2001 From: George Drummond Date: Fri, 29 Mar 2019 14:25:18 -0400 Subject: [PATCH 04/11] Open correct form id --- templates/fragments/admin/portfolio_members.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/fragments/admin/portfolio_members.html b/templates/fragments/admin/portfolio_members.html index d9bdbc81..5b4ac977 100644 --- a/templates/fragments/admin/portfolio_members.html +++ b/templates/fragments/admin/portfolio_members.html @@ -76,7 +76,7 @@ {{ "portfolios.members.archive_button" | translate }} - Cancel + Cancel
{% endcall %} {% endfor %} From 5cc8c05dbdd5e7a3084eec7ac735cff81f837adf Mon Sep 17 00:00:00 2001 From: George Drummond Date: Fri, 29 Mar 2019 15:24:06 -0400 Subject: [PATCH 05/11] Return correct error code --- atst/routes/portfolios/index.py | 3 ++- tests/routes/portfolios/test_portfolios_index.py | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/atst/routes/portfolios/index.py b/atst/routes/portfolios/index.py index 767f56aa..ecb1f361 100644 --- a/atst/routes/portfolios/index.py +++ b/atst/routes/portfolios/index.py @@ -16,6 +16,7 @@ from atst.models.permissions import Permissions from atst.domain.permission_sets import PermissionSets from atst.domain.authz.decorator import user_can_access_decorator as user_can from atst.utils.flash import formatted_flash as flash +from atst.domain.exceptions import UnauthorizedError @portfolios_bp.route("/portfolios") @@ -184,7 +185,7 @@ def portfolio_reports(portfolio_id): def remove_member(portfolio_id, member_id): if member_id == str(g.current_user.id): raise UnauthorizedError( - user=user, message="you cant remove yourself from the portfolio" + user=g.current_user, action="you cant remove yourself from the portfolio" ) portfolio = Portfolios.get(g.current_user, portfolio_id) diff --git a/tests/routes/portfolios/test_portfolios_index.py b/tests/routes/portfolios/test_portfolios_index.py index d4fdeaba..29672f9c 100644 --- a/tests/routes/portfolios/test_portfolios_index.py +++ b/tests/routes/portfolios/test_portfolios_index.py @@ -126,7 +126,7 @@ def test_remove_portfolio_member_self(client, user_session): follow_redirects=False, ) - assert response.status_code == 500 + assert response.status_code == 404 assert ( PortfolioRoles.get(portfolio_id=portfolio.id, user_id=portfolio.owner.id).status == PortfolioRoleStatus.ACTIVE From 358b00a6e2dbbb4b19a49217782866138f506bf4 Mon Sep 17 00:00:00 2001 From: George Drummond Date: Mon, 1 Apr 2019 10:23:19 -0400 Subject: [PATCH 06/11] Import on one line --- tests/routes/portfolios/test_portfolios_index.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/routes/portfolios/test_portfolios_index.py b/tests/routes/portfolios/test_portfolios_index.py index 29672f9c..d7d6b4dd 100644 --- a/tests/routes/portfolios/test_portfolios_index.py +++ b/tests/routes/portfolios/test_portfolios_index.py @@ -2,8 +2,7 @@ from flask import url_for from atst.domain.permission_sets import PermissionSets from atst.models.permissions import Permissions -from atst.domain.portfolio_roles import PortfolioRoles -from atst.models.portfolio_role import Status as PortfolioRoleStatus +from atst.domain.portfolio_roles import PortfolioRoles, Status as PortfolioRoleStatus from tests.factories import ( random_future_date, From dee14b98bea537442c4b51578e91dd96a74be637 Mon Sep 17 00:00:00 2001 From: George Drummond Date: Mon, 1 Apr 2019 10:44:53 -0400 Subject: [PATCH 07/11] Remove portfolio permissions when role is disabled --- atst/domain/authz/__init__.py | 4 +++- tests/domain/test_authz.py | 9 +++++++++ tests/routes/portfolios/test_portfolios_index.py | 3 ++- 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/atst/domain/authz/__init__.py b/atst/domain/authz/__init__.py index 6e8cdfea..37e06ab8 100644 --- a/atst/domain/authz/__init__.py +++ b/atst/domain/authz/__init__.py @@ -1,6 +1,8 @@ from atst.utils import first_or_none from atst.models.permissions import Permissions from atst.domain.exceptions import UnauthorizedError +from atst.domain.portfolio_roles import PortfolioRoles +from atst.models.portfolio_role import Status as PortfolioRoleStatus class Authorization(object): @@ -9,7 +11,7 @@ class Authorization(object): port_role = first_or_none( lambda pr: pr.portfolio == portfolio, user.portfolio_roles ) - if port_role: + if port_role and port_role.status is not PortfolioRoleStatus.DISABLED: return permission in port_role.permissions else: return False diff --git a/tests/domain/test_authz.py b/tests/domain/test_authz.py index bbd6ecba..37749533 100644 --- a/tests/domain/test_authz.py +++ b/tests/domain/test_authz.py @@ -11,6 +11,7 @@ from atst.domain.authz.decorator import user_can_access_decorator from atst.domain.permission_sets import PermissionSets from atst.domain.exceptions import UnauthorizedError from atst.models.permissions import Permissions +from atst.domain.portfolio_roles import PortfolioRoles from tests.utils import FakeLogger @@ -101,6 +102,14 @@ def test_user_can_access(): view_admin, Permissions.EDIT_PORTFOLIO_NAME, portfolio=portfolio ) + # check when portfolio_role is disabled + view_admin_pr = PortfolioRoles.get(portfolio_id=portfolio.id, user_id=view_admin.id) + PortfolioRoles.disable(portfolio_role=view_admin_pr) + with pytest.raises(UnauthorizedError): + user_can_access( + view_admin, Permissions.EDIT_PORTFOLIO_NAME, portfolio=portfolio + ) + @pytest.fixture def set_current_user(request_ctx): diff --git a/tests/routes/portfolios/test_portfolios_index.py b/tests/routes/portfolios/test_portfolios_index.py index d7d6b4dd..29672f9c 100644 --- a/tests/routes/portfolios/test_portfolios_index.py +++ b/tests/routes/portfolios/test_portfolios_index.py @@ -2,7 +2,8 @@ from flask import url_for from atst.domain.permission_sets import PermissionSets from atst.models.permissions import Permissions -from atst.domain.portfolio_roles import PortfolioRoles, Status as PortfolioRoleStatus +from atst.domain.portfolio_roles import PortfolioRoles +from atst.models.portfolio_role import Status as PortfolioRoleStatus from tests.factories import ( random_future_date, From 933d90b203ef31178d3914abd4d27701fd1b0b0f Mon Sep 17 00:00:00 2001 From: George Drummond Date: Mon, 1 Apr 2019 13:32:11 -0400 Subject: [PATCH 08/11] Save PR and don't do an extra lookup --- tests/domain/test_authz.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/domain/test_authz.py b/tests/domain/test_authz.py index 37749533..bfebe562 100644 --- a/tests/domain/test_authz.py +++ b/tests/domain/test_authz.py @@ -76,7 +76,7 @@ def test_user_can_access(): portfolio = PortfolioFactory.create(owner=edit_admin) # factory gives view perms by default - PortfolioRoleFactory.create(user=view_admin, portfolio=portfolio) + view_admin_pr = PortfolioRoleFactory.create(user=view_admin, portfolio=portfolio) # check a site-wide permission assert user_can_access(ccpo, Permissions.VIEW_AUDIT_LOG) @@ -103,7 +103,6 @@ def test_user_can_access(): ) # check when portfolio_role is disabled - view_admin_pr = PortfolioRoles.get(portfolio_id=portfolio.id, user_id=view_admin.id) PortfolioRoles.disable(portfolio_role=view_admin_pr) with pytest.raises(UnauthorizedError): user_can_access( From 270b8d0db6c2a2dc8459d912ca0b5a122efd1e15 Mon Sep 17 00:00:00 2001 From: George Drummond Date: Mon, 1 Apr 2019 13:34:01 -0400 Subject: [PATCH 09/11] We aren't using this import --- atst/domain/authz/__init__.py | 1 - 1 file changed, 1 deletion(-) diff --git a/atst/domain/authz/__init__.py b/atst/domain/authz/__init__.py index 37e06ab8..38e3fa38 100644 --- a/atst/domain/authz/__init__.py +++ b/atst/domain/authz/__init__.py @@ -1,7 +1,6 @@ from atst.utils import first_or_none from atst.models.permissions import Permissions from atst.domain.exceptions import UnauthorizedError -from atst.domain.portfolio_roles import PortfolioRoles from atst.models.portfolio_role import Status as PortfolioRoleStatus From bcfc8ee8e1f31a62f3f6673e5fd4642e98d9c1f9 Mon Sep 17 00:00:00 2001 From: George Drummond Date: Wed, 3 Apr 2019 09:48:30 -0400 Subject: [PATCH 10/11] Remove double import --- atst/routes/portfolios/index.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/atst/routes/portfolios/index.py b/atst/routes/portfolios/index.py index ecb1f361..cf0a87b0 100644 --- a/atst/routes/portfolios/index.py +++ b/atst/routes/portfolios/index.py @@ -2,8 +2,6 @@ from datetime import date, timedelta from flask import render_template, request as http_request, g, redirect, url_for -from atst.utils.flash import formatted_flash as flash - from . import portfolios_bp from atst.domain.reports import Reports from atst.domain.portfolios import Portfolios From 83b071bf209a97338f7d20ad1ccf363f38feba95 Mon Sep 17 00:00:00 2001 From: George Drummond Date: Wed, 3 Apr 2019 10:32:03 -0400 Subject: [PATCH 11/11] Get changes working with merged PRs --- atst/routes/portfolios/index.py | 14 ++++++-------- templates/fragments/admin/members_edit.html | 4 ++-- templates/fragments/admin/portfolio_members.html | 2 +- tests/routes/portfolios/test_portfolios_index.py | 6 ++---- 4 files changed, 11 insertions(+), 15 deletions(-) diff --git a/atst/routes/portfolios/index.py b/atst/routes/portfolios/index.py index cf0a87b0..daf768ae 100644 --- a/atst/routes/portfolios/index.py +++ b/atst/routes/portfolios/index.py @@ -177,18 +177,16 @@ def portfolio_reports(portfolio_id): @portfolios_bp.route( - "/portfolios//members//delete", methods=["POST"] + "/portfolios//members//delete", methods=["POST"] ) @user_can(Permissions.EDIT_PORTFOLIO_USERS, message="update portfolio members") -def remove_member(portfolio_id, member_id): - if member_id == str(g.current_user.id): +def remove_member(portfolio_id, user_id): + if str(g.current_user.id) == user_id: raise UnauthorizedError( - user=g.current_user, action="you cant remove yourself from the portfolio" + g.current_user, "you cant remove yourself from the portfolio" ) - portfolio = Portfolios.get(g.current_user, portfolio_id) - portfolio_role = PortfolioRoles.get(portfolio_id=portfolio_id, user_id=member_id) - + portfolio_role = PortfolioRoles.get(portfolio_id=portfolio_id, user_id=user_id) PortfolioRoles.disable(portfolio_role=portfolio_role) flash("portfolio_member_removed", member_name=portfolio_role.user.full_name) @@ -196,7 +194,7 @@ def remove_member(portfolio_id, member_id): return redirect( url_for( "portfolios.portfolio_admin", - portfolio_id=portfolio.id, + portfolio_id=portfolio_id, _anchor="portfolio-members", fragment="portfolio-members", ) diff --git a/templates/fragments/admin/members_edit.html b/templates/fragments/admin/members_edit.html index 04d2ed21..2c4df95f 100644 --- a/templates/fragments/admin/members_edit.html +++ b/templates/fragments/admin/members_edit.html @@ -1,8 +1,8 @@ {% from "components/confirmation_button.html" import ConfirmationButton %} -{% set modal_id = "portfolio_id_{}_user_id_{}".format(portfolio.id, user.id) %} - {% for subform in member_perms_form.members_permissions %} + {% set modal_id = "portfolio_id_{}_user_id_{}".format(portfolio.id, subform.user_id.data) %} + {{ subform.member.data }} {% if subform.member.data == user.full_name %} diff --git a/templates/fragments/admin/portfolio_members.html b/templates/fragments/admin/portfolio_members.html index 5b4ac977..0763aad0 100644 --- a/templates/fragments/admin/portfolio_members.html +++ b/templates/fragments/admin/portfolio_members.html @@ -70,7 +70,7 @@ }}
-
+ {{ member_perms_form.csrf_token }}