From a152c66e617d728cf166293d4974e82e004051c6 Mon Sep 17 00:00:00 2001 From: Montana Date: Thu, 28 Mar 2019 10:30:48 -0400 Subject: [PATCH 01/10] Add hidden input for passing dod id in form --- atst/forms/portfolio_member.py | 3 ++- atst/routes/portfolios/index.py | 11 +++++++++++ templates/fragments/admin/members_edit.html | 1 + templates/fragments/admin/portfolio_members.html | 15 ++++++++------- 4 files changed, 22 insertions(+), 8 deletions(-) diff --git a/atst/forms/portfolio_member.py b/atst/forms/portfolio_member.py index 2293deed..a4ed18e3 100644 --- a/atst/forms/portfolio_member.py +++ b/atst/forms/portfolio_member.py @@ -1,6 +1,6 @@ -from wtforms.fields import StringField, FormField, FieldList from wtforms.fields.html5 import EmailField, TelField from wtforms.validators import Required, Email, Length, Optional +from wtforms.fields import StringField, FormField, FieldList, HiddenField from atst.domain.permission_sets import PermissionSets from .forms import BaseForm @@ -11,6 +11,7 @@ from atst.utils.localization import translate class PermissionsForm(BaseForm): member = StringField() + user_id = HiddenField() perms_app_mgmt = SelectField( None, choices=[ diff --git a/atst/routes/portfolios/index.py b/atst/routes/portfolios/index.py index 78f0ba68..9a65d318 100644 --- a/atst/routes/portfolios/index.py +++ b/atst/routes/portfolios/index.py @@ -34,6 +34,7 @@ def permission_str(member, edit_perm_set, view_perm_set): def serialize_member_form_data(member): return { "member": member.user.full_name, + "user_id": member.user_id, "perms_app_mgmt": permission_str( member, PermissionSets.EDIT_PORTFOLIO_APPLICATION_MANAGEMENT, @@ -86,6 +87,16 @@ def portfolio_admin(portfolio_id): return render_admin_page(portfolio) +@portfolios_bp.route("/portfolios//admin", methods=["POST"]) +@user_can(Permissions.EDIT_PORTFOLIO_USERS, message="view portfolio admin page") +def edit_portfolio_members(portfolio_id): + portfolio = Portfolios.get_for_update(portfolio_id) + member_perms_form = MembersPermissionsForm( + http_request.form + ) + return render_admin_page(portfolio) + + @portfolios_bp.route("/portfolios//edit", methods=["POST"]) @user_can(Permissions.EDIT_PORTFOLIO_NAME, message="edit portfolio") def edit_portfolio(portfolio_id): diff --git a/templates/fragments/admin/members_edit.html b/templates/fragments/admin/members_edit.html index c64dafcb..0d873622 100644 --- a/templates/fragments/admin/members_edit.html +++ b/templates/fragments/admin/members_edit.html @@ -16,5 +16,6 @@ + {{ subform.user_id() }} {% endfor %} diff --git a/templates/fragments/admin/portfolio_members.html b/templates/fragments/admin/portfolio_members.html index 63e44680..452cdfcc 100644 --- a/templates/fragments/admin/portfolio_members.html +++ b/templates/fragments/admin/portfolio_members.html @@ -6,13 +6,14 @@ {% if g.matchesPath("portfolio-members") %} {% include "fragments/flash.html" %} {% endif %} -
-
-
-
{{ "portfolios.admin.portfolio_members_title" | translate }}
-
- {{ "portfolios.admin.portfolio_members_subheading" | translate }} -
+ + {{ member_perms_form.csrf_token }} + +
+
+
{{ "portfolios.admin.portfolio_members_title" | translate }}
+
+ {{ "portfolios.admin.portfolio_members_subheading" | translate }}
{{ Icon('info') }} From 766e3b6f47e482c910cc2625cbf45f59e8f19bfc Mon Sep 17 00:00:00 2001 From: Montana Date: Thu, 28 Mar 2019 13:58:38 -0400 Subject: [PATCH 02/10] Update permissions --- atst/routes/portfolios/index.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/atst/routes/portfolios/index.py b/atst/routes/portfolios/index.py index 9a65d318..6832099b 100644 --- a/atst/routes/portfolios/index.py +++ b/atst/routes/portfolios/index.py @@ -5,6 +5,7 @@ from flask import render_template, request as http_request, g, redirect, url_for from . import portfolios_bp from atst.domain.reports import Reports from atst.domain.portfolios import Portfolios +from atst.domain.portfolio_roles import PortfolioRoles from atst.domain.audit_log import AuditLog from atst.domain.common import Paginator from atst.forms.portfolio import PortfolioForm @@ -91,9 +92,15 @@ def portfolio_admin(portfolio_id): @user_can(Permissions.EDIT_PORTFOLIO_USERS, message="view portfolio admin page") def edit_portfolio_members(portfolio_id): portfolio = Portfolios.get_for_update(portfolio_id) - member_perms_form = MembersPermissionsForm( - http_request.form - ) + member_perms_form = MembersPermissionsForm(http_request.form) + + for subform in member_perms_form.members_permissions: + new_perm_set = subform.data["permission_sets"] + user_id = subform.user_id.data + portfolio_role = PortfolioRoles.get(portfolio.id, user_id) + if portfolio_role.permission_sets != new_perm_set: + PortfolioRoles.update(portfolio_role, new_perm_set) + return render_admin_page(portfolio) From bfff2a94b8452204baf549b84b9205e40b6b10f7 Mon Sep 17 00:00:00 2001 From: Montana Date: Thu, 28 Mar 2019 16:45:19 -0400 Subject: [PATCH 03/10] Add tests --- tests/routes/portfolios/test_admin.py | 74 +++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/tests/routes/portfolios/test_admin.py b/tests/routes/portfolios/test_admin.py index 7f141fe8..1adeaa73 100644 --- a/tests/routes/portfolios/test_admin.py +++ b/tests/routes/portfolios/test_admin.py @@ -26,3 +26,77 @@ def test_member_table_access(client, user_session): user_session(rando) view_resp = client.get(url) assert " Date: Thu, 28 Mar 2019 17:14:24 -0400 Subject: [PATCH 04/10] Success banner --- atst/routes/portfolios/index.py | 15 ++++++++++++++- atst/utils/flash.py | 7 +++++++ templates/fragments/admin/portfolio_members.html | 11 ++++++----- 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/atst/routes/portfolios/index.py b/atst/routes/portfolios/index.py index 6832099b..66bcfa82 100644 --- a/atst/routes/portfolios/index.py +++ b/atst/routes/portfolios/index.py @@ -2,6 +2,8 @@ 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 @@ -92,7 +94,7 @@ def portfolio_admin(portfolio_id): @user_can(Permissions.EDIT_PORTFOLIO_USERS, message="view portfolio admin page") def edit_portfolio_members(portfolio_id): portfolio = Portfolios.get_for_update(portfolio_id) - member_perms_form = MembersPermissionsForm(http_request.form) + member_perms_form = member_forms.MembersPermissionsForm(http_request.form) for subform in member_perms_form.members_permissions: new_perm_set = subform.data["permission_sets"] @@ -101,6 +103,17 @@ def edit_portfolio_members(portfolio_id): if portfolio_role.permission_sets != new_perm_set: PortfolioRoles.update(portfolio_role, new_perm_set) + flash("update_portfolio_members", portfolio=portfolio) + + return redirect( + url_for( + "portfolios.portfolio_admin", + portfolio_id=portfolio.id, + fragment="portfolio-members", + _anchor="portfolio-members", + ) + ) + return render_admin_page(portfolio) diff --git a/atst/utils/flash.py b/atst/utils/flash.py index 69239977..11b042e8 100644 --- a/atst/utils/flash.py +++ b/atst/utils/flash.py @@ -21,6 +21,13 @@ MESSAGES = { """, "category": "success", }, + "update_portfolio_members": { + "title_template": "Success!", + "message_template": """ +

You have successfully updated access permissions for members of {{ portfolio.name }}.

+ """, + "category": "success", + }, "new_portfolio_member": { "title_template": "Success!", "message_template": """ diff --git a/templates/fragments/admin/portfolio_members.html b/templates/fragments/admin/portfolio_members.html index 452cdfcc..08e856e0 100644 --- a/templates/fragments/admin/portfolio_members.html +++ b/templates/fragments/admin/portfolio_members.html @@ -6,7 +6,7 @@ {% if g.matchesPath("portfolio-members") %} {% include "fragments/flash.html" %} {% endif %} - + {{ member_perms_form.csrf_token }}
@@ -15,10 +15,11 @@
{{ "portfolios.admin.portfolio_members_subheading" | translate }}
- - {{ Icon('info') }} - {{ "portfolios.admin.settings_info" | translate }} - +
+ + {{ Icon('info') }} + {{ "portfolios.admin.settings_info" | translate }} +
{% if not portfolio.members %} From d70332836b7c7bdfbedde677118530408f266e68 Mon Sep 17 00:00:00 2001 From: Montana Date: Fri, 29 Mar 2019 12:01:15 -0400 Subject: [PATCH 05/10] Only flash when permissions change --- atst/routes/portfolios/index.py | 6 +++++- templates/fragments/admin/portfolio_members.html | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/atst/routes/portfolios/index.py b/atst/routes/portfolios/index.py index 66bcfa82..8dc37ade 100644 --- a/atst/routes/portfolios/index.py +++ b/atst/routes/portfolios/index.py @@ -95,6 +95,7 @@ def portfolio_admin(portfolio_id): def edit_portfolio_members(portfolio_id): portfolio = Portfolios.get_for_update(portfolio_id) member_perms_form = member_forms.MembersPermissionsForm(http_request.form) + has_changed = False for subform in member_perms_form.members_permissions: new_perm_set = subform.data["permission_sets"] @@ -102,8 +103,10 @@ def edit_portfolio_members(portfolio_id): portfolio_role = PortfolioRoles.get(portfolio.id, user_id) if portfolio_role.permission_sets != new_perm_set: PortfolioRoles.update(portfolio_role, new_perm_set) + has_changed = True - flash("update_portfolio_members", portfolio=portfolio) + if has_changed: + flash("update_portfolio_members", portfolio=portfolio) return redirect( url_for( @@ -111,6 +114,7 @@ def edit_portfolio_members(portfolio_id): portfolio_id=portfolio.id, fragment="portfolio-members", _anchor="portfolio-members", + has_changed=has_changed, ) ) diff --git a/templates/fragments/admin/portfolio_members.html b/templates/fragments/admin/portfolio_members.html index 08e856e0..6739d8e2 100644 --- a/templates/fragments/admin/portfolio_members.html +++ b/templates/fragments/admin/portfolio_members.html @@ -3,7 +3,7 @@
- {% if g.matchesPath("portfolio-members") %} + {% if g.matchesPath("portfolio-members") and has_changed %} {% include "fragments/flash.html" %} {% endif %} From 25563cf06a72516da37ed7bbd5b64f42d9cb25bd Mon Sep 17 00:00:00 2001 From: Montana Date: Fri, 29 Mar 2019 14:30:25 -0400 Subject: [PATCH 06/10] Add helper function --- atst/routes/portfolios/index.py | 14 ++++++++++++++ tests/routes/portfolios/test_admin.py | 27 +++++++++++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/atst/routes/portfolios/index.py b/atst/routes/portfolios/index.py index 8dc37ade..f34fe316 100644 --- a/atst/routes/portfolios/index.py +++ b/atst/routes/portfolios/index.py @@ -90,6 +90,20 @@ def portfolio_admin(portfolio_id): return render_admin_page(portfolio) +def permission_set_has_changed(old_perm_set_names, new_perm_set_names): + has_changed = False + for perm_name in new_perm_set_names: + base = perm_name[4:] + if perm_name.split("_")[0] == "edit": + if perm_name not in old_perm_set_names: + has_changed = True + elif perm_name.split("_")[0] == "view": + edit_version = "edit" + base + if edit_version in old_perm_set_names: + has_changed = True + return has_changed + + @portfolios_bp.route("/portfolios//admin", methods=["POST"]) @user_can(Permissions.EDIT_PORTFOLIO_USERS, message="view portfolio admin page") def edit_portfolio_members(portfolio_id): diff --git a/tests/routes/portfolios/test_admin.py b/tests/routes/portfolios/test_admin.py index 1adeaa73..8096e62d 100644 --- a/tests/routes/portfolios/test_admin.py +++ b/tests/routes/portfolios/test_admin.py @@ -1,6 +1,7 @@ from flask import url_for from atst.domain.permission_sets import PermissionSets +from atst.routes.portfolios.index import permission_set_has_changed from tests.factories import PortfolioFactory, PortfolioRoleFactory, UserFactory @@ -28,6 +29,32 @@ def test_member_table_access(client, user_session): assert " Date: Fri, 29 Mar 2019 14:33:31 -0400 Subject: [PATCH 07/10] Only update when permissions are different --- atst/routes/portfolios/index.py | 13 +++++++------ templates/fragments/admin/portfolio_members.html | 2 +- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/atst/routes/portfolios/index.py b/atst/routes/portfolios/index.py index f34fe316..ae870410 100644 --- a/atst/routes/portfolios/index.py +++ b/atst/routes/portfolios/index.py @@ -109,17 +109,19 @@ def permission_set_has_changed(old_perm_set_names, new_perm_set_names): def edit_portfolio_members(portfolio_id): portfolio = Portfolios.get_for_update(portfolio_id) member_perms_form = member_forms.MembersPermissionsForm(http_request.form) - has_changed = False + have_any_perms_changed = False for subform in member_perms_form.members_permissions: new_perm_set = subform.data["permission_sets"] user_id = subform.user_id.data portfolio_role = PortfolioRoles.get(portfolio.id, user_id) - if portfolio_role.permission_sets != new_perm_set: - PortfolioRoles.update(portfolio_role, new_perm_set) - has_changed = True + old_perm_set = [perm.name for perm in portfolio_role.permission_sets] - if has_changed: + if permission_set_has_changed(old_perm_set, new_perm_set): + PortfolioRoles.update(portfolio_role, new_perm_set) + have_any_perms_changed = True + + if have_any_perms_changed: flash("update_portfolio_members", portfolio=portfolio) return redirect( @@ -128,7 +130,6 @@ def edit_portfolio_members(portfolio_id): portfolio_id=portfolio.id, fragment="portfolio-members", _anchor="portfolio-members", - has_changed=has_changed, ) ) diff --git a/templates/fragments/admin/portfolio_members.html b/templates/fragments/admin/portfolio_members.html index 6739d8e2..08e856e0 100644 --- a/templates/fragments/admin/portfolio_members.html +++ b/templates/fragments/admin/portfolio_members.html @@ -3,7 +3,7 @@
- {% if g.matchesPath("portfolio-members") and has_changed %} + {% if g.matchesPath("portfolio-members") %} {% include "fragments/flash.html" %} {% endif %} From ba2a63bffc744f18b278c53f1631a8c0b115a8d0 Mon Sep 17 00:00:00 2001 From: dandds <38955503+dandds@users.noreply.github.com> Date: Tue, 2 Apr 2019 09:23:44 -0400 Subject: [PATCH 08/10] Update test to use get_many Co-Authored-By: montana-mil <42577527+montana-mil@users.noreply.github.com> --- tests/routes/portfolios/test_admin.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/routes/portfolios/test_admin.py b/tests/routes/portfolios/test_admin.py index 8096e62d..068be561 100644 --- a/tests/routes/portfolios/test_admin.py +++ b/tests/routes/portfolios/test_admin.py @@ -68,7 +68,9 @@ def test_update_member_permissions(client, user_session): PortfolioRoleFactory.create( user=user, portfolio=portfolio, - permission_sets=[PermissionSets.get(PermissionSets.EDIT_PORTFOLIO_ADMIN)], + permission_sets=PermissionSets.get_many( + [PermissionSets.EDIT_PORTFOLIO_ADMIN, PermissionSets.VIEW_PORTFOLIO_ADMIN] + ), ) user_session(user) From c46746d43ddadd26ec897825168af30566d7b516 Mon Sep 17 00:00:00 2001 From: Montana Date: Tue, 2 Apr 2019 09:35:57 -0400 Subject: [PATCH 09/10] No need to manually check for update or flash --- atst/routes/portfolios/index.py | 24 ++---------------------- tests/routes/portfolios/test_admin.py | 27 --------------------------- 2 files changed, 2 insertions(+), 49 deletions(-) diff --git a/atst/routes/portfolios/index.py b/atst/routes/portfolios/index.py index ae870410..dfee5004 100644 --- a/atst/routes/portfolios/index.py +++ b/atst/routes/portfolios/index.py @@ -90,39 +90,19 @@ def portfolio_admin(portfolio_id): return render_admin_page(portfolio) -def permission_set_has_changed(old_perm_set_names, new_perm_set_names): - has_changed = False - for perm_name in new_perm_set_names: - base = perm_name[4:] - if perm_name.split("_")[0] == "edit": - if perm_name not in old_perm_set_names: - has_changed = True - elif perm_name.split("_")[0] == "view": - edit_version = "edit" + base - if edit_version in old_perm_set_names: - has_changed = True - return has_changed - - @portfolios_bp.route("/portfolios//admin", methods=["POST"]) @user_can(Permissions.EDIT_PORTFOLIO_USERS, message="view portfolio admin page") def edit_portfolio_members(portfolio_id): portfolio = Portfolios.get_for_update(portfolio_id) member_perms_form = member_forms.MembersPermissionsForm(http_request.form) - have_any_perms_changed = False for subform in member_perms_form.members_permissions: new_perm_set = subform.data["permission_sets"] user_id = subform.user_id.data portfolio_role = PortfolioRoles.get(portfolio.id, user_id) - old_perm_set = [perm.name for perm in portfolio_role.permission_sets] + PortfolioRoles.update(portfolio_role, new_perm_set) - if permission_set_has_changed(old_perm_set, new_perm_set): - PortfolioRoles.update(portfolio_role, new_perm_set) - have_any_perms_changed = True - - if have_any_perms_changed: - flash("update_portfolio_members", portfolio=portfolio) + flash("update_portfolio_members", portfolio=portfolio) return redirect( url_for( diff --git a/tests/routes/portfolios/test_admin.py b/tests/routes/portfolios/test_admin.py index 068be561..4a5f2a32 100644 --- a/tests/routes/portfolios/test_admin.py +++ b/tests/routes/portfolios/test_admin.py @@ -1,7 +1,6 @@ from flask import url_for from atst.domain.permission_sets import PermissionSets -from atst.routes.portfolios.index import permission_set_has_changed from tests.factories import PortfolioFactory, PortfolioRoleFactory, UserFactory @@ -29,32 +28,6 @@ def test_member_table_access(client, user_session): assert " Date: Tue, 2 Apr 2019 10:16:46 -0400 Subject: [PATCH 10/10] Validate the form --- atst/routes/portfolios/index.py | 31 ++++++++++++++------------- tests/routes/portfolios/test_admin.py | 28 ++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 15 deletions(-) diff --git a/atst/routes/portfolios/index.py b/atst/routes/portfolios/index.py index dfee5004..541a8194 100644 --- a/atst/routes/portfolios/index.py +++ b/atst/routes/portfolios/index.py @@ -96,24 +96,25 @@ def edit_portfolio_members(portfolio_id): portfolio = Portfolios.get_for_update(portfolio_id) member_perms_form = member_forms.MembersPermissionsForm(http_request.form) - for subform in member_perms_form.members_permissions: - new_perm_set = subform.data["permission_sets"] - user_id = subform.user_id.data - portfolio_role = PortfolioRoles.get(portfolio.id, user_id) - PortfolioRoles.update(portfolio_role, new_perm_set) + if member_perms_form.validate(): + for subform in member_perms_form.members_permissions: + new_perm_set = subform.data["permission_sets"] + user_id = subform.user_id.data + portfolio_role = PortfolioRoles.get(portfolio.id, user_id) + PortfolioRoles.update(portfolio_role, new_perm_set) - flash("update_portfolio_members", portfolio=portfolio) + flash("update_portfolio_members", portfolio=portfolio) - return redirect( - url_for( - "portfolios.portfolio_admin", - portfolio_id=portfolio.id, - fragment="portfolio-members", - _anchor="portfolio-members", + return redirect( + url_for( + "portfolios.portfolio_admin", + portfolio_id=portfolio.id, + fragment="portfolio-members", + _anchor="portfolio-members", + ) ) - ) - - return render_admin_page(portfolio) + else: + return render_admin_page(portfolio) @portfolios_bp.route("/portfolios//edit", methods=["POST"]) diff --git a/tests/routes/portfolios/test_admin.py b/tests/routes/portfolios/test_admin.py index 4a5f2a32..522dcb0c 100644 --- a/tests/routes/portfolios/test_admin.py +++ b/tests/routes/portfolios/test_admin.py @@ -102,3 +102,31 @@ def test_no_update_member_permissions_without_edit_access(client, user_session): assert not rando_pf_role.has_permission_set( PermissionSets.EDIT_PORTFOLIO_APPLICATION_MANAGEMENT ) + + +def test_rerender_admin_page_if_member_perms_form_does_not_validate( + client, user_session +): + portfolio = PortfolioFactory.create() + user = UserFactory.create() + PortfolioRoleFactory.create( + user=user, + portfolio=portfolio, + permission_sets=[PermissionSets.get(PermissionSets.EDIT_PORTFOLIO_ADMIN)], + ) + user_session(user) + form_data = { + "members_permissions-0-user_id": user.id, + "members_permissions-0-perms_app_mgmt": "bad input", + "members_permissions-0-perms_funding": "view_portfolio_funding", + "members_permissions-0-perms_reporting": "view_portfolio_reports", + "members_permissions-0-perms_portfolio_mgmt": "view_portfolio_admin", + } + + response = client.post( + url_for("portfolios.edit_portfolio_members", portfolio_id=portfolio.id), + data=form_data, + follow_redirects=True, + ) + assert response.status_code == 200 + assert "Portfolio Administration" in response.data.decode()