From e3ace36466ef6579bc9f968750f128a84e837702 Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 22 May 2019 15:42:20 -0400 Subject: [PATCH] Use portfolio_role.id for updating primary point of contact. Our forms should rely on role IDs for displaying user information on the portfolio page. This way they are decoupled from user table data and can eventually rely on invitation user data where an invitation has been sent but a user does not exist yet. --- atst/forms/portfolio_member.py | 2 +- atst/routes/portfolios/admin.py | 18 ++++-------- templates/fragments/admin/change_ppoc.html | 2 +- tests/routes/portfolios/test_admin.py | 32 ++++++++-------------- 4 files changed, 18 insertions(+), 36 deletions(-) diff --git a/atst/forms/portfolio_member.py b/atst/forms/portfolio_member.py index 75a6e793..69f53a7a 100644 --- a/atst/forms/portfolio_member.py +++ b/atst/forms/portfolio_member.py @@ -80,7 +80,7 @@ class NewForm(BaseForm): class AssignPPOCForm(PermissionsForm): - user_id = SelectField( + role_id = SelectField( label=translate("forms.assign_ppoc.dod_id"), validators=[Required()], choices=[("", "- Select -")], diff --git a/atst/routes/portfolios/admin.py b/atst/routes/portfolios/admin.py index a79b0586..fb90800c 100644 --- a/atst/routes/portfolios/admin.py +++ b/atst/routes/portfolios/admin.py @@ -4,10 +4,8 @@ from . import portfolios_bp from atst.domain.portfolios import Portfolios from atst.domain.portfolio_roles import PortfolioRoles from atst.domain.permission_sets import PermissionSets -from atst.domain.users import Users from atst.domain.audit_log import AuditLog from atst.domain.common import Paginator -from atst.domain.exceptions import NotFoundError from atst.forms.portfolio import PortfolioForm import atst.forms.portfolio_member as member_forms from atst.models.permissions import Permissions @@ -73,9 +71,7 @@ def render_admin_page(portfolio, form=None): assign_ppoc_form = member_forms.AssignPPOCForm() for pf_role in portfolio.roles: if pf_role.user != portfolio.owner and pf_role.is_active: - assign_ppoc_form.user_id.choices += [ - (pf_role.user.id, pf_role.user.full_name) - ] + assign_ppoc_form.role_id.choices += [(pf_role.id, pf_role.full_name)] current_member = first_or_none( lambda m: m.user_id == g.current_user.id, portfolio.members @@ -135,18 +131,14 @@ def edit_members(portfolio_id): @portfolios_bp.route("/portfolios//update_ppoc", methods=["POST"]) @user_can(Permissions.EDIT_PORTFOLIO_POC, message="update portfolio ppoc") def update_ppoc(portfolio_id): - user_id = http_request.form.get("user_id") + role_id = http_request.form.get("role_id") portfolio = Portfolios.get(g.current_user, portfolio_id) - new_ppoc = Users.get(user_id) + new_ppoc_role = PortfolioRoles.get_by_id(role_id) - if new_ppoc not in portfolio.users: - raise NotFoundError("user not in portfolio") + PortfolioRoles.make_ppoc(portfolio_role=new_ppoc_role) - portfolio_role = PortfolioRoles.get(portfolio_id=portfolio_id, user_id=user_id) - PortfolioRoles.make_ppoc(portfolio_role=portfolio_role) - - flash("primary_point_of_contact_changed", ppoc_name=new_ppoc.full_name) + flash("primary_point_of_contact_changed", ppoc_name=new_ppoc_role.full_name) return redirect( url_for( diff --git a/templates/fragments/admin/change_ppoc.html b/templates/fragments/admin/change_ppoc.html index 0d16dbfa..ddd5b7d1 100644 --- a/templates/fragments/admin/change_ppoc.html +++ b/templates/fragments/admin/change_ppoc.html @@ -22,7 +22,7 @@
{{ OptionsInput( - assign_ppoc_form.user_id + assign_ppoc_form.role_id ) }}
diff --git a/tests/routes/portfolios/test_admin.py b/tests/routes/portfolios/test_admin.py index 91c80fc3..370bf3db 100644 --- a/tests/routes/portfolios/test_admin.py +++ b/tests/routes/portfolios/test_admin.py @@ -181,7 +181,7 @@ def test_update_portfolio_name(client, user_session): def updating_ppoc_successfully(client, old_ppoc, new_ppoc, portfolio): response = client.post( url_for("portfolios.update_ppoc", portfolio_id=portfolio.id, _external=True), - data={"user_id": new_ppoc.id}, + data={"role_id": new_ppoc.id}, follow_redirects=False, ) @@ -193,17 +193,9 @@ def updating_ppoc_successfully(client, old_ppoc, new_ppoc, portfolio): _anchor="primary-point-of-contact", _external=True, ) - assert portfolio.owner.id == new_ppoc.id - assert ( - Permissions.EDIT_PORTFOLIO_POC - in PortfolioRoles.get( - portfolio_id=portfolio.id, user_id=new_ppoc.id - ).permissions - ) - assert ( - Permissions.EDIT_PORTFOLIO_POC - not in PortfolioRoles.get(portfolio.id, old_ppoc.id).permissions - ) + assert portfolio.owner_role.id == new_ppoc.id + assert Permissions.EDIT_PORTFOLIO_POC in new_ppoc.permissions + assert Permissions.EDIT_PORTFOLIO_POC not in old_ppoc.permissions def test_update_ppoc_no_user_id_specified(client, user_session): @@ -238,15 +230,14 @@ def test_update_ppoc_to_member_not_on_portfolio(client, user_session): def test_update_ppoc_when_ppoc(client, user_session): portfolio = PortfolioFactory.create() - original_ppoc = portfolio.owner - new_ppoc = UserFactory.create() - Portfolios.add_member( - member=new_ppoc, + original_ppoc = portfolio.owner_role + new_ppoc = Portfolios.add_member( + member=UserFactory.create(), portfolio=portfolio, permission_sets=[PermissionSets.VIEW_PORTFOLIO], ) - user_session(original_ppoc) + user_session(original_ppoc.user) updating_ppoc_successfully( client=client, new_ppoc=new_ppoc, old_ppoc=original_ppoc, portfolio=portfolio @@ -256,10 +247,9 @@ def test_update_ppoc_when_ppoc(client, user_session): def test_update_ppoc_when_cpo(client, user_session): ccpo = UserFactory.create_ccpo() portfolio = PortfolioFactory.create() - original_ppoc = portfolio.owner - new_ppoc = UserFactory.create() - Portfolios.add_member( - member=new_ppoc, + original_ppoc = portfolio.owner_role + new_ppoc = Portfolios.add_member( + member=UserFactory.create(), portfolio=portfolio, permission_sets=[PermissionSets.VIEW_PORTFOLIO], )