From c8682c089740d9f0ae9ea6955af54f1a2814656f Mon Sep 17 00:00:00 2001 From: dandds Date: Thu, 16 May 2019 08:04:48 -0400 Subject: [PATCH] Use portfolio_role entity to display and update portfolio member info. Previously, we were encoding the portfolio_role.user_id as part of the form data for the portfolio admin page. This was convenient because it allowed us to easily determine certain display attributes in the template. Instead, we should rely on the PortfolioRole as the source of truth for member information. This commit adds: - Portfolio.owner_role to return the PortfolioRole of the owner - explicitly passes the PortfolioRole IDs for the PPoC and current user to the template - PortfolioRole.full_name for deriving the member name --- atst/forms/portfolio_member.py | 4 ++-- atst/models/portfolio.py | 10 ++++++--- atst/models/portfolio_role.py | 4 ++++ atst/routes/portfolios/admin.py | 24 +++++++++++++-------- templates/fragments/admin/members_edit.html | 10 ++++----- templates/fragments/admin/members_view.html | 6 +++--- tests/routes/portfolios/test_admin.py | 10 ++++----- 7 files changed, 41 insertions(+), 27 deletions(-) diff --git a/atst/forms/portfolio_member.py b/atst/forms/portfolio_member.py index 4c39ad1e..75a6e793 100644 --- a/atst/forms/portfolio_member.py +++ b/atst/forms/portfolio_member.py @@ -9,8 +9,8 @@ from atst.utils.localization import translate class PermissionsForm(BaseForm): - member = StringField() - user_id = HiddenField() + member_name = StringField() + member_id = HiddenField() perms_app_mgmt = SelectField( translate("forms.new_member.app_mgmt"), choices=[ diff --git a/atst/models/portfolio.py b/atst/models/portfolio.py index 23c19482..0e65b146 100644 --- a/atst/models/portfolio.py +++ b/atst/models/portfolio.py @@ -26,14 +26,18 @@ class Portfolio(Base, mixins.TimestampsMixin, mixins.AuditableMixin): task_orders = relationship("TaskOrder") @property - def owner(self): + def owner_role(self): def _is_portfolio_owner(portfolio_role): return PermissionSets.PORTFOLIO_POC in [ perms_set.name for perms_set in portfolio_role.permission_sets ] - owner = first_or_none(_is_portfolio_owner, self.roles) - return owner.user if owner else None + return first_or_none(_is_portfolio_owner, self.roles) + + @property + def owner(self): + owner_role = self.owner_role + return owner_role.user if owner_role else None @property def users(self): diff --git a/atst/models/portfolio_role.py b/atst/models/portfolio_role.py index adb1017a..3b5859cf 100644 --- a/atst/models/portfolio_role.py +++ b/atst/models/portfolio_role.py @@ -160,6 +160,10 @@ class PortfolioRole( self.latest_invitation and self.latest_invitation.is_inactive ) + @property + def full_name(self): + return self.user.full_name + Index( "portfolio_role_user_portfolio", diff --git a/atst/routes/portfolios/admin.py b/atst/routes/portfolios/admin.py index bc51405d..a79b0586 100644 --- a/atst/routes/portfolios/admin.py +++ b/atst/routes/portfolios/admin.py @@ -12,6 +12,7 @@ from atst.forms.portfolio import PortfolioForm import atst.forms.portfolio_member as member_forms from atst.models.permissions import Permissions from atst.domain.authz.decorator import user_can_access_decorator as user_can +from atst.utils import first_or_none from atst.utils.flash import formatted_flash as flash from atst.domain.exceptions import UnauthorizedError @@ -25,8 +26,8 @@ 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, + "member_name": member.full_name, + "member_id": member.id, "perms_app_mgmt": permission_str( member, PermissionSets.EDIT_PORTFOLIO_APPLICATION_MANAGEMENT, @@ -53,7 +54,7 @@ def serialize_member_form_data(member): def get_members_data(portfolio): members = [serialize_member_form_data(member) for member in portfolio.members] for member in members: - if member["user_id"] == portfolio.owner.id: + if member["member_id"] == portfolio.owner_role.id: ppoc = member members.remove(member) members.insert(0, ppoc) @@ -76,6 +77,11 @@ def render_admin_page(portfolio, form=None): (pf_role.user.id, pf_role.user.full_name) ] + current_member = first_or_none( + lambda m: m.user_id == g.current_user.id, portfolio.members + ) + current_member_id = current_member.id if current_member else None + return render_template( "portfolios/admin.html", form=form, @@ -86,7 +92,8 @@ def render_admin_page(portfolio, form=None): portfolio=portfolio, audit_events=audit_events, user=g.current_user, - members_data=members_data, + ppoc_id=members_data[0].get("member_id"), + current_member_id=current_member_id, ) @@ -105,12 +112,11 @@ def edit_members(portfolio_id): if member_perms_form.validate(): for subform in member_perms_form.members_permissions: - user_id = subform.user_id.data - member = Users.get(user_id=user_id) - if member is not portfolio.owner: + member_id = subform.member_id.data + member = PortfolioRoles.get_by_id(member_id) + if member is not portfolio.owner_role: new_perm_set = subform.data["permission_sets"] - portfolio_role = PortfolioRoles.get(portfolio.id, user_id) - PortfolioRoles.update(portfolio_role, new_perm_set) + PortfolioRoles.update(member, new_perm_set) flash("update_portfolio_members", portfolio=portfolio) diff --git a/templates/fragments/admin/members_edit.html b/templates/fragments/admin/members_edit.html index ebcfb72a..d190a90d 100644 --- a/templates/fragments/admin/members_edit.html +++ b/templates/fragments/admin/members_edit.html @@ -1,18 +1,18 @@ {% from "components/options_input.html" import OptionsInput %} {% for subform in member_perms_form.members_permissions %} - {% set modal_id = "portfolio_id_{}_user_id_{}".format(portfolio.id, subform.user_id.data) %} - {% set ppoc = subform.user_id.data == portfolio.owner.id %} + {% set modal_id = "portfolio_id_{}_user_id_{}".format(portfolio.id, subform.member_id.data) %} + {% set ppoc = subform.member_id.data == ppoc_id %} {% set archive_button_class = 'button-danger-outline' %} - {{ subform.member.data }} + {{ subform.member_name.data }}
{% if ppoc %} {% set archive_button_class = 'usa-button-disabled' %} PPoC {% endif %} - {% if subform.user_id.data == user.id %} + {% if subform.member_id.data == current_member_id %} {% set archive_button_class = 'usa-button-disabled' %} (you) {% endif %} @@ -30,7 +30,7 @@ {% if not ppoc %} - {{ subform.user_id() }} + {{ subform.member_id() }} {% endif %} {% endfor %} diff --git a/templates/fragments/admin/members_view.html b/templates/fragments/admin/members_view.html index 9e3a3368..65fde544 100644 --- a/templates/fragments/admin/members_view.html +++ b/templates/fragments/admin/members_view.html @@ -1,14 +1,14 @@ {% for subform in member_perms_form.members_permissions %} - {% set ppoc = subform.user_id.data == portfolio.owner.id %} + {% set ppoc = subform.member_id.data == ppoc_id %} {% set heading_perms = [subform.perms_app_mgmt, subform.perms_funding, subform.perms_reporting, subform.perms_portfolio_mgmt] %} - {{ subform.member.data }} + {{ subform.member_name.data }}
{% if ppoc %} PPoC {% endif %} - {% if subform.user_id.data == user.id %} + {% if subform.member_id.data == current_member_id %} (you) {% endif %}
diff --git a/tests/routes/portfolios/test_admin.py b/tests/routes/portfolios/test_admin.py index 0085a9b5..91c80fc3 100644 --- a/tests/routes/portfolios/test_admin.py +++ b/tests/routes/portfolios/test_admin.py @@ -53,7 +53,7 @@ def test_update_member_permissions(client, user_session): user_session(user) form_data = { - "members_permissions-0-user_id": rando.id, + "members_permissions-0-member_id": rando_pf_role.id, "members_permissions-0-perms_app_mgmt": "edit_portfolio_application_management", "members_permissions-0-perms_funding": "view_portfolio_funding", "members_permissions-0-perms_reporting": "view_portfolio_reports", @@ -90,7 +90,7 @@ def test_no_update_member_permissions_without_edit_access(client, user_session): user_session(user) form_data = { - "members_permissions-0-user_id": rando.id, + "members_permissions-0-member_id": rando_pf_role.id, "members_permissions-0-perms_app_mgmt": "edit_portfolio_application_management", "members_permissions-0-perms_funding": "view_portfolio_funding", "members_permissions-0-perms_reporting": "view_portfolio_reports", @@ -114,14 +114,14 @@ def test_rerender_admin_page_if_member_perms_form_does_not_validate( ): portfolio = PortfolioFactory.create() user = UserFactory.create() - PortfolioRoleFactory.create( + role = 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-member_id": role.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", @@ -149,7 +149,7 @@ def test_cannot_update_portfolio_ppoc_perms(client, user_session): assert ppoc_pf_role.has_permission_set(PermissionSets.PORTFOLIO_POC) member_perms_data = { - "members_permissions-0-user_id": ppoc.id, + "members_permissions-0-member_id": ppoc_pf_role.id, "members_permissions-0-perms_app_mgmt": "view_portfolio_application_management", "members_permissions-0-perms_funding": "view_portfolio_funding", "members_permissions-0-perms_reporting": "view_portfolio_reports",