From 6487fe91baa7c653a32cf8bfbe717e99e202b352 Mon Sep 17 00:00:00 2001 From: George Drummond Date: Mon, 20 May 2019 15:18:26 -0400 Subject: [PATCH 1/6] Applications users were invited to were not showing in the portfolios sidebar --- atst/domain/portfolios/query.py | 36 ++++++++++++++++++++- atst/routes/portfolios/index.py | 10 +----- atst/routes/portfolios/invitations.py | 2 +- atst/utils/flash.py | 2 +- templates/navigation/global_sidenav.html | 2 +- tests/routes/portfolios/test_invitations.py | 4 +-- tests/test_access.py | 13 -------- 7 files changed, 41 insertions(+), 28 deletions(-) diff --git a/atst/domain/portfolios/query.py b/atst/domain/portfolios/query.py index eb78c56d..f38c70d6 100644 --- a/atst/domain/portfolios/query.py +++ b/atst/domain/portfolios/query.py @@ -2,6 +2,11 @@ from atst.database import db from atst.domain.common import Query from atst.models.portfolio import Portfolio from atst.models.portfolio_role import PortfolioRole, Status as PortfolioRoleStatus +from atst.models.application_role import ( + ApplicationRole, + Status as ApplicationRoleStatus, +) +from atst.models.application import Application class PortfoliosQuery(Query): @@ -9,7 +14,24 @@ class PortfoliosQuery(Query): @classmethod def get_for_user(cls, user): - return ( + granted_applications = ( + db.session.query(Application) + .join(ApplicationRole) + .filter(ApplicationRole.application_id == Application.id) + .filter(ApplicationRole.user_id == user.id) + .filter(ApplicationRole.status == ApplicationRoleStatus.ACTIVE) + .subquery() + ) + + application_portfolios = ( + db.session.query(Portfolio) + .join(Application) + .filter(Portfolio.id == Application.portfolio_id) + .filter(Application.id == granted_applications.c.id) + .all() + ) + + portfolios = ( db.session.query(Portfolio) .join(PortfolioRole) .filter(PortfolioRole.user == user) @@ -17,6 +39,18 @@ class PortfoliosQuery(Query): .all() ) + portfolio_ids = [] + + [portfolio_ids.append(p.id) for p in portfolios] + [portfolio_ids.append(p.id) for p in application_portfolios] + + return ( + db.session.query(Portfolio) + .filter(Portfolio.id.in_(portfolio_ids)) + .order_by(Portfolio.name.desc()) + .all() + ) + @classmethod def create_portfolio_role(cls, user, portfolio, **kwargs): return PortfolioRole(user=user, portfolio=portfolio, **kwargs) diff --git a/atst/routes/portfolios/index.py b/atst/routes/portfolios/index.py index 28bb079e..f0758b93 100644 --- a/atst/routes/portfolios/index.py +++ b/atst/routes/portfolios/index.py @@ -1,6 +1,6 @@ from datetime import date, timedelta -from flask import render_template, request as http_request, g, redirect, url_for +from flask import render_template, request as http_request, g from . import portfolios_bp from atst.domain.reports import Reports @@ -19,14 +19,6 @@ def portfolios(): return render_template("portfolios/blank_slate.html") -@portfolios_bp.route("/portfolios/") -@user_can(Permissions.VIEW_PORTFOLIO, message="view portfolio") -def show_portfolio(portfolio_id): - return redirect( - url_for("applications.portfolio_applications", portfolio_id=portfolio_id) - ) - - @portfolios_bp.route("/portfolios//reports") @user_can(Permissions.VIEW_PORTFOLIO_REPORTS, message="view portfolio reports") def reports(portfolio_id): diff --git a/atst/routes/portfolios/invitations.py b/atst/routes/portfolios/invitations.py index 4c3a53a8..dd1f1c5a 100644 --- a/atst/routes/portfolios/invitations.py +++ b/atst/routes/portfolios/invitations.py @@ -30,7 +30,7 @@ def accept_invitation(portfolio_token): ) return redirect( - url_for("portfolios.show_portfolio", portfolio_id=invite.portfolio.id) + url_for("applications.portfolio_applications", portfolio_id=invite.portfolio.id) ) diff --git a/atst/utils/flash.py b/atst/utils/flash.py index 0a96366a..001293af 100644 --- a/atst/utils/flash.py +++ b/atst/utils/flash.py @@ -145,7 +145,7 @@ MESSAGES = { "actions": """ {% from "components/icon.html" import Icon %}
- + {{ Icon('shield') }} {{ "flash.portfolio_home" | translate }} diff --git a/templates/navigation/global_sidenav.html b/templates/navigation/global_sidenav.html index 555b7fdd..e61524ee 100644 --- a/templates/navigation/global_sidenav.html +++ b/templates/navigation/global_sidenav.html @@ -21,7 +21,7 @@ {% if portfolios %} {% for other_portfolio in portfolios|sort(attribute='name') %} {{ SidenavItem(other_portfolio.name, - href=url_for("portfolios.show_portfolio", portfolio_id=other_portfolio.id), + href=url_for("applications.portfolio_applications", portfolio_id=other_portfolio.id), active=(other_portfolio.id | string) == request.view_args.get('portfolio_id') ) }} {% endfor %} diff --git a/tests/routes/portfolios/test_invitations.py b/tests/routes/portfolios/test_invitations.py index 5134957b..9dbf481f 100644 --- a/tests/routes/portfolios/test_invitations.py +++ b/tests/routes/portfolios/test_invitations.py @@ -34,7 +34,7 @@ def test_existing_member_accepts_valid_invite(client, user_session): # user is redirected to the portfolio view assert response.status_code == 302 assert ( - url_for("portfolios.show_portfolio", portfolio_id=invite.portfolio.id) + url_for("applications.portfolio_applications", portfolio_id=invite.portfolio.id) in response.headers["Location"] ) # the one-time use invite is no longer usable @@ -77,7 +77,7 @@ def test_new_member_accepts_valid_invite(monkeypatch, client, user_session): # user is redirected to the portfolio view assert response.status_code == 302 assert ( - url_for("portfolios.show_portfolio", portfolio_id=portfolio.id) + url_for("applications.portfolio_applications", portfolio_id=portfolio.id) in response.headers["Location"] ) # the user has access to the portfolio diff --git a/tests/test_access.py b/tests/test_access.py index 84f1d167..2d7874a4 100644 --- a/tests/test_access.py +++ b/tests/test_access.py @@ -466,19 +466,6 @@ def test_portfolios_revoke_invitation_access(post_url_assert_status): post_url_assert_status(user, url, status) -# portfolios.show_portfolio -def test_portfolios_show_portfolio_access(get_url_assert_status): - ccpo = user_with(PermissionSets.VIEW_PORTFOLIO) - owner = user_with() - rando = user_with() - portfolio = PortfolioFactory.create(owner=owner) - - url = url_for("portfolios.show_portfolio", portfolio_id=portfolio.id) - get_url_assert_status(ccpo, url, 302) - get_url_assert_status(owner, url, 302) - get_url_assert_status(rando, url, 404) - - # task_orders.so_review def test_task_orders_so_review_access(get_url_assert_status): ccpo = UserFactory.create_ccpo() From a9fac1ca5945c0257a6aab1077322f73a12608f9 Mon Sep 17 00:00:00 2001 From: George Drummond Date: Tue, 21 May 2019 11:09:57 -0400 Subject: [PATCH 2/6] Add in missing test --- tests/domain/test_portfolios.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/domain/test_portfolios.py b/tests/domain/test_portfolios.py index 6819b36b..bf6a415e 100644 --- a/tests/domain/test_portfolios.py +++ b/tests/domain/test_portfolios.py @@ -7,9 +7,12 @@ from atst.domain.portfolio_roles import PortfolioRoles from atst.domain.applications import Applications from atst.domain.environments import Environments from atst.domain.permission_sets import PermissionSets, PORTFOLIO_PERMISSION_SETS +from atst.models.application_role import Status as ApplicationRoleStatus from atst.models.portfolio_role import Status as PortfolioRoleStatus from tests.factories import ( + ApplicationFactory, + ApplicationRoleFactory, UserFactory, PortfolioRoleFactory, PortfolioFactory, @@ -164,6 +167,17 @@ def test_scoped_portfolio_returns_all_applications_for_portfolio_owner( assert len(scoped_portfolio.applications[0].environments) == 3 +def test_for_user_returns_portfolios_for_applications_user_invited_to(): + bob = UserFactory.create() + portfolio = PortfolioFactory.create() + application = ApplicationFactory.create(portfolio=portfolio) + ApplicationRoleFactory.create( + application=application, user=bob, status=ApplicationRoleStatus.ACTIVE + ) + + assert portfolio in Portfolios.for_user(user=bob) + + def test_for_user_returns_active_portfolios_for_user(portfolio, portfolio_owner): bob = UserFactory.create() PortfolioRoleFactory.create( From 9098bc7c0b72cd390a987784502fc7b883e2ac78 Mon Sep 17 00:00:00 2001 From: George Drummond Date: Wed, 22 May 2019 11:36:43 -0400 Subject: [PATCH 3/6] Reduce number of queries performed during the lookup --- atst/domain/portfolios/query.py | 40 ++++++++++++++------------------- 1 file changed, 17 insertions(+), 23 deletions(-) diff --git a/atst/domain/portfolios/query.py b/atst/domain/portfolios/query.py index f38c70d6..a24c719e 100644 --- a/atst/domain/portfolios/query.py +++ b/atst/domain/portfolios/query.py @@ -23,31 +23,25 @@ class PortfoliosQuery(Query): .subquery() ) - application_portfolios = ( - db.session.query(Portfolio) - .join(Application) - .filter(Portfolio.id == Application.portfolio_id) - .filter(Application.id == granted_applications.c.id) - .all() - ) - - portfolios = ( - db.session.query(Portfolio) - .join(PortfolioRole) - .filter(PortfolioRole.user == user) - .filter(PortfolioRole.status == PortfolioRoleStatus.ACTIVE) - .all() - ) - - portfolio_ids = [] - - [portfolio_ids.append(p.id) for p in portfolios] - [portfolio_ids.append(p.id) for p in application_portfolios] - return ( db.session.query(Portfolio) - .filter(Portfolio.id.in_(portfolio_ids)) - .order_by(Portfolio.name.desc()) + .filter( + Portfolio.id.in_( + db.session.query(Portfolio.id) + .join(Application) + .filter(Portfolio.id == Application.portfolio_id) + .filter(Application.id == granted_applications.c.id) + .subquery() + ) + | Portfolio.id.in_( + db.session.query(Portfolio.id) + .join(PortfolioRole) + .filter(PortfolioRole.user == user) + .filter(PortfolioRole.status == PortfolioRoleStatus.ACTIVE) + .subquery() + ) + ) + .order_by(Portfolio.name.asc()) .all() ) From b61f304bad8d6a9df24f8d8ad85d6d8b0d1e3768 Mon Sep 17 00:00:00 2001 From: George Drummond Date: Wed, 22 May 2019 13:32:13 -0400 Subject: [PATCH 4/6] Make one query --- atst/domain/portfolios/query.py | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/atst/domain/portfolios/query.py b/atst/domain/portfolios/query.py index a24c719e..e4242531 100644 --- a/atst/domain/portfolios/query.py +++ b/atst/domain/portfolios/query.py @@ -14,15 +14,6 @@ class PortfoliosQuery(Query): @classmethod def get_for_user(cls, user): - granted_applications = ( - db.session.query(Application) - .join(ApplicationRole) - .filter(ApplicationRole.application_id == Application.id) - .filter(ApplicationRole.user_id == user.id) - .filter(ApplicationRole.status == ApplicationRoleStatus.ACTIVE) - .subquery() - ) - return ( db.session.query(Portfolio) .filter( @@ -30,8 +21,18 @@ class PortfoliosQuery(Query): db.session.query(Portfolio.id) .join(Application) .filter(Portfolio.id == Application.portfolio_id) - .filter(Application.id == granted_applications.c.id) - .subquery() + .filter( + Application.id.in_( + db.session.query(Application.id) + .join(ApplicationRole) + .filter(ApplicationRole.application_id == Application.id) + .filter(ApplicationRole.user_id == user.id) + .filter( + ApplicationRole.status == ApplicationRoleStatus.ACTIVE + ) + .subquery() + ) + ) ) | Portfolio.id.in_( db.session.query(Portfolio.id) From bb4b52c6acf1ee2cb3ca0820244b2b9223e5229e Mon Sep 17 00:00:00 2001 From: George Drummond Date: Wed, 22 May 2019 13:46:49 -0400 Subject: [PATCH 5/6] Add in missing route --- atst/routes/applications/index.py | 1 + 1 file changed, 1 insertion(+) diff --git a/atst/routes/applications/index.py b/atst/routes/applications/index.py index 3aebb6b1..d4b2f276 100644 --- a/atst/routes/applications/index.py +++ b/atst/routes/applications/index.py @@ -15,6 +15,7 @@ def has_portfolio_applications(_user, portfolio=None, **_kwargs): return True +@applications_bp.route("/portfolios/") @applications_bp.route("/portfolios//applications") @user_can( Permissions.VIEW_APPLICATION, From 41e2d14e7dab649d3ef8a9d683bb65d756c86040 Mon Sep 17 00:00:00 2001 From: George Drummond Date: Wed, 22 May 2019 15:17:31 -0400 Subject: [PATCH 6/6] Use sqlalchemy or_ --- atst/domain/portfolios/query.py | 48 ++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/atst/domain/portfolios/query.py b/atst/domain/portfolios/query.py index e4242531..e215b9d3 100644 --- a/atst/domain/portfolios/query.py +++ b/atst/domain/portfolios/query.py @@ -1,3 +1,4 @@ +from sqlalchemy import or_ from atst.database import db from atst.domain.common import Query from atst.models.portfolio import Portfolio @@ -17,29 +18,34 @@ class PortfoliosQuery(Query): return ( db.session.query(Portfolio) .filter( - Portfolio.id.in_( - db.session.query(Portfolio.id) - .join(Application) - .filter(Portfolio.id == Application.portfolio_id) - .filter( - Application.id.in_( - db.session.query(Application.id) - .join(ApplicationRole) - .filter(ApplicationRole.application_id == Application.id) - .filter(ApplicationRole.user_id == user.id) - .filter( - ApplicationRole.status == ApplicationRoleStatus.ACTIVE + or_( + Portfolio.id.in_( + db.session.query(Portfolio.id) + .join(Application) + .filter(Portfolio.id == Application.portfolio_id) + .filter( + Application.id.in_( + db.session.query(Application.id) + .join(ApplicationRole) + .filter( + ApplicationRole.application_id == Application.id + ) + .filter(ApplicationRole.user_id == user.id) + .filter( + ApplicationRole.status + == ApplicationRoleStatus.ACTIVE + ) + .subquery() ) - .subquery() ) - ) - ) - | Portfolio.id.in_( - db.session.query(Portfolio.id) - .join(PortfolioRole) - .filter(PortfolioRole.user == user) - .filter(PortfolioRole.status == PortfolioRoleStatus.ACTIVE) - .subquery() + ), + Portfolio.id.in_( + db.session.query(Portfolio.id) + .join(PortfolioRole) + .filter(PortfolioRole.user == user) + .filter(PortfolioRole.status == PortfolioRoleStatus.ACTIVE) + .subquery() + ), ) ) .order_by(Portfolio.name.asc())