From b18b2386494f7e02c557595ff16eb23468e57a87 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Tue, 4 Sep 2018 14:18:25 -0400 Subject: [PATCH 1/8] Move action_required_by to Request model --- atst/domain/requests.py | 8 -------- atst/models/request.py | 8 ++++++++ tests/models/test_requests.py | 6 +++--- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/atst/domain/requests.py b/atst/domain/requests.py index d44be200..dbe8326d 100644 --- a/atst/domain/requests.py +++ b/atst/domain/requests.py @@ -153,14 +153,6 @@ class Requests(object): request.status_events.append(status_event) return request - @classmethod - def action_required_by(cls, request): - return { - RequestStatus.STARTED: "mission_owner", - RequestStatus.PENDING_FINANCIAL_VERIFICATION: "mission_owner", - RequestStatus.PENDING_CCPO_APPROVAL: "ccpo", - }.get(request.status) - @classmethod def should_auto_approve(cls, request): try: diff --git a/atst/models/request.py b/atst/models/request.py index 466c6d91..a560429a 100644 --- a/atst/models/request.py +++ b/atst/models/request.py @@ -60,3 +60,11 @@ class Request(Base): if last_submission: return pendulum.instance(last_submission.time_created) return None + + @property + def action_required_by(self): + return { + RequestStatus.STARTED: "mission_owner", + RequestStatus.PENDING_FINANCIAL_VERIFICATION: "mission_owner", + RequestStatus.PENDING_CCPO_APPROVAL: "ccpo", + }.get(self.status) diff --git a/tests/models/test_requests.py b/tests/models/test_requests.py index ba9478df..563c88b9 100644 --- a/tests/models/test_requests.py +++ b/tests/models/test_requests.py @@ -4,21 +4,21 @@ from atst.domain.requests import Requests, RequestStatus def test_started_request_requires_mo_action(): request = RequestFactory.create() - assert Requests.action_required_by(request) == "mission_owner" + assert request.action_required_by == "mission_owner" def test_pending_financial_requires_mo_action(): request = RequestFactory.create() request = Requests.set_status(request, RequestStatus.PENDING_FINANCIAL_VERIFICATION) - assert Requests.action_required_by(request) == "mission_owner" + assert request.action_required_by == "mission_owner" def test_pending_ccpo_approval_requires_ccpo(): request = RequestFactory.create() request = Requests.set_status(request, RequestStatus.PENDING_CCPO_APPROVAL) - assert Requests.action_required_by(request) == "ccpo" + assert request.action_required_by == "ccpo" def test_request_has_creator(): From 58593fc7d62753ad96683fa748daf572392f9e80 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Tue, 4 Sep 2018 14:19:19 -0400 Subject: [PATCH 2/8] Determine if action is required by current user on each request --- atst/routes/requests/index.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/atst/routes/requests/index.py b/atst/routes/requests/index.py index 6a5cc0f4..bebba5cf 100644 --- a/atst/routes/requests/index.py +++ b/atst/routes/requests/index.py @@ -6,7 +6,8 @@ from atst.domain.requests import Requests from atst.models.permissions import Permissions -def map_request(request): +def map_request(request, viewing_role): + time_created = pendulum.instance(request.time_created) is_new = time_created.add(days=1) > pendulum.now() app_count = request.body.get("details_of_use", {}).get("num_software_systems", 0) @@ -36,6 +37,7 @@ def map_request(request): "full_name": request.creator.full_name, "annual_usage": annual_usage, "edit_link": edit_link, + "action_required": request.action_required_by == viewing_role, } @@ -53,7 +55,8 @@ def requests_index(): def _ccpo_view(): requests = Requests.get_many() - mapped_requests = [map_request(r) for r in requests] + mapped_requests = [map_request(r, "ccpo") for r in requests] + num_action_required = len([r for r in mapped_requests if r.get("action_required")]) return render_template( "requests.html", @@ -64,12 +67,14 @@ def _ccpo_view(): kpi_inprogress=Requests.in_progress_count(), kpi_pending=Requests.pending_ccpo_count(), kpi_completed=Requests.completed_count(), + num_action_required=num_action_required, ) def _non_ccpo_view(): requests = Requests.get_many(creator=g.current_user) - mapped_requests = [map_request(r) for r in requests] + mapped_requests = [map_request(r, "mission_owner") for r in requests] + num_action_required = len([r for r in mapped_requests if r.get("action_required")]) pending_fv = any(Requests.is_pending_financial_verification(r) for r in requests) pending_ccpo = any(Requests.is_pending_ccpo_approval(r) for r in requests) @@ -79,5 +84,6 @@ def _non_ccpo_view(): requests=mapped_requests, pending_financial_verification=pending_fv, pending_ccpo_approval=pending_ccpo, + num_action_required=num_action_required, extended_view=False, ) From 319f6c05c24d76fb907ffb6d47d55c2470a3f537 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Tue, 4 Sep 2018 14:57:01 -0400 Subject: [PATCH 3/8] Refactor requests index route for easier testing --- atst/routes/requests/index.py | 158 +++++++++++++++------------- tests/factories.py | 5 +- tests/routes/test_requests_index.py | 23 ++++ 3 files changed, 112 insertions(+), 74 deletions(-) create mode 100644 tests/routes/test_requests_index.py diff --git a/atst/routes/requests/index.py b/atst/routes/requests/index.py index bebba5cf..ee398255 100644 --- a/atst/routes/requests/index.py +++ b/atst/routes/requests/index.py @@ -6,84 +6,96 @@ from atst.domain.requests import Requests from atst.models.permissions import Permissions -def map_request(request, viewing_role): +class RequestsIndex(object): + def __init__(self, user): + self.user = user - time_created = pendulum.instance(request.time_created) - is_new = time_created.add(days=1) > pendulum.now() - app_count = request.body.get("details_of_use", {}).get("num_software_systems", 0) - annual_usage = request.annual_spend - last_submission_timestamp = ( - request.last_submission_timestamp.format("M/DD/YYYY") - if request.last_submission_timestamp - else "-" - ) + def execute(self): + if ( + Permissions.REVIEW_AND_APPROVE_JEDI_WORKSPACE_REQUEST + in self.user.atat_permissions + ): + return self._ccpo_view(self.user) - if Requests.is_pending_financial_verification(request): - edit_link = url_for("requests.financial_verification", request_id=request.id) - elif Requests.is_pending_ccpo_approval(request): - edit_link = url_for("requests.view_pending_request", request_id=request.id) - else: - edit_link = url_for( - "requests.requests_form_update", screen=1, request_id=request.id + else: + return self._non_ccpo_view(self.user) + + def _ccpo_view(self, user): + requests = Requests.get_many() + mapped_requests = [self.map_request(r, "ccpo") for r in requests] + num_action_required = len( + [r for r in mapped_requests if r.get("action_required")] ) - return { - "workspace_id": request.workspace.id if request.workspace else None, - "order_id": request.id, - "is_new": is_new, - "status": request.status_displayname, - "app_count": app_count, - "last_submission_timestamp": last_submission_timestamp, - "full_name": request.creator.full_name, - "annual_usage": annual_usage, - "edit_link": edit_link, - "action_required": request.action_required_by == viewing_role, - } + return dict( + requests=mapped_requests, + pending_financial_verification=False, + pending_ccpo_approval=False, + extended_view=True, + kpi_inprogress=Requests.in_progress_count(), + kpi_pending=Requests.pending_ccpo_count(), + kpi_completed=Requests.completed_count(), + num_action_required=num_action_required, + ) + + def _non_ccpo_view(self, user): + requests = Requests.get_many(creator=user) + mapped_requests = [self.map_request(r, "mission_owner") for r in requests] + num_action_required = len( + [r for r in mapped_requests if r.get("action_required")] + ) + pending_fv = any( + Requests.is_pending_financial_verification(r) for r in requests + ) + pending_ccpo = any(Requests.is_pending_ccpo_approval(r) for r in requests) + + return dict( + requests=mapped_requests, + pending_financial_verification=pending_fv, + pending_ccpo_approval=pending_ccpo, + num_action_required=num_action_required, + extended_view=False, + ) + + def map_request(self, request, viewing_role): + time_created = pendulum.instance(request.time_created) + is_new = time_created.add(days=1) > pendulum.now() + app_count = request.body.get("details_of_use", {}).get( + "num_software_systems", 0 + ) + annual_usage = request.annual_spend + last_submission_timestamp = ( + request.last_submission_timestamp.format("M/DD/YYYY") + if request.last_submission_timestamp + else "-" + ) + + if Requests.is_pending_financial_verification(request): + edit_link = url_for( + "requests.financial_verification", request_id=request.id + ) + elif Requests.is_pending_ccpo_approval(request): + edit_link = url_for("requests.view_pending_request", request_id=request.id) + else: + edit_link = url_for( + "requests.requests_form_update", screen=1, request_id=request.id + ) + + return { + "workspace_id": request.workspace.id if request.workspace else None, + "order_id": request.id, + "is_new": is_new, + "status": request.status_displayname, + "app_count": app_count, + "last_submission_timestamp": last_submission_timestamp, + "full_name": request.creator.full_name, + "annual_usage": annual_usage, + "edit_link": edit_link, + "action_required": request.action_required_by == viewing_role, + } @requests_bp.route("/requests", methods=["GET"]) def requests_index(): - if ( - Permissions.REVIEW_AND_APPROVE_JEDI_WORKSPACE_REQUEST - in g.current_user.atat_permissions - ): - return _ccpo_view() - - else: - return _non_ccpo_view() - - -def _ccpo_view(): - requests = Requests.get_many() - mapped_requests = [map_request(r, "ccpo") for r in requests] - num_action_required = len([r for r in mapped_requests if r.get("action_required")]) - - return render_template( - "requests.html", - requests=mapped_requests, - pending_financial_verification=False, - pending_ccpo_approval=False, - extended_view=True, - kpi_inprogress=Requests.in_progress_count(), - kpi_pending=Requests.pending_ccpo_count(), - kpi_completed=Requests.completed_count(), - num_action_required=num_action_required, - ) - - -def _non_ccpo_view(): - requests = Requests.get_many(creator=g.current_user) - mapped_requests = [map_request(r, "mission_owner") for r in requests] - num_action_required = len([r for r in mapped_requests if r.get("action_required")]) - - pending_fv = any(Requests.is_pending_financial_verification(r) for r in requests) - pending_ccpo = any(Requests.is_pending_ccpo_approval(r) for r in requests) - - return render_template( - "requests.html", - requests=mapped_requests, - pending_financial_verification=pending_fv, - pending_ccpo_approval=pending_ccpo, - num_action_required=num_action_required, - extended_view=False, - ) + context = RequestsIndex(g.current_user).execute() + return render_template("requests.html", **context) diff --git a/tests/factories.py b/tests/factories.py index 51be428b..f6ed669f 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -11,7 +11,6 @@ from atst.models.task_order import TaskOrder from atst.models.user import User from atst.models.role import Role from atst.models.workspace import Workspace -from atst.models.request_status_event import RequestStatusEvent from atst.domain.roles import Roles @@ -33,6 +32,10 @@ class UserFactory(factory.alchemy.SQLAlchemyModelFactory): atat_role = factory.SubFactory(RoleFactory) dod_id = factory.LazyFunction(lambda: "".join(random.choices(string.digits, k=10))) + @classmethod + def from_atat_role(cls, atat_role_name, **kwargs): + role = Roles.get(atat_role_name) + return cls.create(atat_role=role, **kwargs) class RequestStatusEventFactory(factory.alchemy.SQLAlchemyModelFactory): class Meta: diff --git a/tests/routes/test_requests_index.py b/tests/routes/test_requests_index.py new file mode 100644 index 00000000..03d214a4 --- /dev/null +++ b/tests/routes/test_requests_index.py @@ -0,0 +1,23 @@ +from atst.routes.requests.index import RequestsIndex +from tests.factories import RequestFactory, UserFactory +from atst.domain.requests import Requests + + +def test_action_required_mission_owner(): + creator = UserFactory.create() + requests = RequestFactory.create_batch(5, creator=creator) + Requests.submit(requests[0]) + context = RequestsIndex(creator).execute() + + assert context['requests'][0]['action_required'] == False + + +def test_action_required_ccpo(): + creator = UserFactory.create() + requests = RequestFactory.create_batch(5, creator=creator) + Requests.submit(requests[0]) + + ccpo = UserFactory.from_atat_role("ccpo") + context = RequestsIndex(ccpo).execute() + + assert context['num_action_required'] == 1 From 156e19387d5158ec71da684460b4aeadbcf323c3 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Tue, 4 Sep 2018 14:57:18 -0400 Subject: [PATCH 4/8] Use action_required and num_action_required in template --- templates/requests.html | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/templates/requests.html b/templates/requests.html index ffa8a798..fb67d82c 100644 --- a/templates/requests.html +++ b/templates/requests.html @@ -25,6 +25,14 @@ {% endcall %} +{% if num_action_required %} + {% set title -%} + Action required on {{ num_action_required }} requests. + {%- endset %} + + {{ Alert (title)}} +{% endif %} + {% if not requests %} {{ EmptyState( @@ -109,7 +117,7 @@ {{ r['order_id'] }} - {% if r['is_new'] %}New{% endif %} + {% if r.action_required %}Action Required{% endif %} {{ r.last_submission_timestamp }} {% if extended_view %} From 39efee046c58f168003ce5518bf2923d7cd78f3d Mon Sep 17 00:00:00 2001 From: richard-dds Date: Tue, 4 Sep 2018 15:00:27 -0400 Subject: [PATCH 5/8] Started request requires no action --- atst/models/request.py | 1 - tests/models/test_requests.py | 5 ----- 2 files changed, 6 deletions(-) diff --git a/atst/models/request.py b/atst/models/request.py index a560429a..064ec9e8 100644 --- a/atst/models/request.py +++ b/atst/models/request.py @@ -64,7 +64,6 @@ class Request(Base): @property def action_required_by(self): return { - RequestStatus.STARTED: "mission_owner", RequestStatus.PENDING_FINANCIAL_VERIFICATION: "mission_owner", RequestStatus.PENDING_CCPO_APPROVAL: "ccpo", }.get(self.status) diff --git a/tests/models/test_requests.py b/tests/models/test_requests.py index 563c88b9..cf17afa8 100644 --- a/tests/models/test_requests.py +++ b/tests/models/test_requests.py @@ -2,11 +2,6 @@ from tests.factories import RequestFactory, UserFactory from atst.domain.requests import Requests, RequestStatus -def test_started_request_requires_mo_action(): - request = RequestFactory.create() - assert request.action_required_by == "mission_owner" - - def test_pending_financial_requires_mo_action(): request = RequestFactory.create() request = Requests.set_status(request, RequestStatus.PENDING_FINANCIAL_VERIFICATION) From b7aa4f38e4f4976887ab9d5b4daeeb5af3773081 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Tue, 4 Sep 2018 15:02:49 -0400 Subject: [PATCH 6/8] Formatting --- tests/factories.py | 1 + tests/routes/test_requests_index.py | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/factories.py b/tests/factories.py index f6ed669f..c49b585d 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -37,6 +37,7 @@ class UserFactory(factory.alchemy.SQLAlchemyModelFactory): role = Roles.get(atat_role_name) return cls.create(atat_role=role, **kwargs) + class RequestStatusEventFactory(factory.alchemy.SQLAlchemyModelFactory): class Meta: model = RequestStatusEvent diff --git a/tests/routes/test_requests_index.py b/tests/routes/test_requests_index.py index 03d214a4..e33c7cb8 100644 --- a/tests/routes/test_requests_index.py +++ b/tests/routes/test_requests_index.py @@ -9,7 +9,7 @@ def test_action_required_mission_owner(): Requests.submit(requests[0]) context = RequestsIndex(creator).execute() - assert context['requests'][0]['action_required'] == False + assert context["requests"][0]["action_required"] == False def test_action_required_ccpo(): @@ -20,4 +20,4 @@ def test_action_required_ccpo(): ccpo = UserFactory.from_atat_role("ccpo") context = RequestsIndex(ccpo).execute() - assert context['num_action_required'] == 1 + assert context["num_action_required"] == 1 From 982e3884a88efad82b29607bc6c650860c3adcdc Mon Sep 17 00:00:00 2001 From: richard-dds Date: Tue, 4 Sep 2018 15:12:17 -0400 Subject: [PATCH 7/8] Rename to _map_request, since it's a private method --- atst/routes/requests/index.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/atst/routes/requests/index.py b/atst/routes/requests/index.py index ee398255..f0564bdc 100644 --- a/atst/routes/requests/index.py +++ b/atst/routes/requests/index.py @@ -22,7 +22,7 @@ class RequestsIndex(object): def _ccpo_view(self, user): requests = Requests.get_many() - mapped_requests = [self.map_request(r, "ccpo") for r in requests] + mapped_requests = [self._map_request(r, "ccpo") for r in requests] num_action_required = len( [r for r in mapped_requests if r.get("action_required")] ) @@ -40,7 +40,7 @@ class RequestsIndex(object): def _non_ccpo_view(self, user): requests = Requests.get_many(creator=user) - mapped_requests = [self.map_request(r, "mission_owner") for r in requests] + mapped_requests = [self._map_request(r, "mission_owner") for r in requests] num_action_required = len( [r for r in mapped_requests if r.get("action_required")] ) @@ -57,7 +57,7 @@ class RequestsIndex(object): extended_view=False, ) - def map_request(self, request, viewing_role): + def _map_request(self, request, viewing_role): time_created = pendulum.instance(request.time_created) is_new = time_created.add(days=1) > pendulum.now() app_count = request.body.get("details_of_use", {}).get( From cbcb33d125318b1a93f5695d5798f205da4f2a55 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Wed, 5 Sep 2018 10:28:05 -0400 Subject: [PATCH 8/8] Use dict literal instead of dict() --- atst/routes/requests/index.py | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/atst/routes/requests/index.py b/atst/routes/requests/index.py index f0564bdc..b475a9e6 100644 --- a/atst/routes/requests/index.py +++ b/atst/routes/requests/index.py @@ -27,16 +27,16 @@ class RequestsIndex(object): [r for r in mapped_requests if r.get("action_required")] ) - return dict( - requests=mapped_requests, - pending_financial_verification=False, - pending_ccpo_approval=False, - extended_view=True, - kpi_inprogress=Requests.in_progress_count(), - kpi_pending=Requests.pending_ccpo_count(), - kpi_completed=Requests.completed_count(), - num_action_required=num_action_required, - ) + return { + "requests": mapped_requests, + "pending_financial_verification": False, + "pending_ccpo_approval": False, + "extended_view": True, + "kpi_inprogress": Requests.in_progress_count(), + "kpi_pending": Requests.pending_ccpo_count(), + "kpi_completed": Requests.completed_count(), + "num_action_required": num_action_required, + } def _non_ccpo_view(self, user): requests = Requests.get_many(creator=user) @@ -49,13 +49,13 @@ class RequestsIndex(object): ) pending_ccpo = any(Requests.is_pending_ccpo_approval(r) for r in requests) - return dict( - requests=mapped_requests, - pending_financial_verification=pending_fv, - pending_ccpo_approval=pending_ccpo, - num_action_required=num_action_required, - extended_view=False, - ) + return { + "requests": mapped_requests, + "pending_financial_verification": pending_fv, + "pending_ccpo_approval": pending_ccpo, + "num_action_required": num_action_required, + "extended_view": False, + } def _map_request(self, request, viewing_role): time_created = pendulum.instance(request.time_created)