From 6419d96bbecfa943a5631080cbd50792a2ecf0b1 Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 3 Oct 2018 10:50:38 -0400 Subject: [PATCH 1/6] WIP refactor fin ver routes --- .../routes/requests/financial_verification.py | 59 +++++++++++++++---- 1 file changed, 48 insertions(+), 11 deletions(-) diff --git a/atst/routes/requests/financial_verification.py b/atst/routes/requests/financial_verification.py index 86b7f383..4626302c 100644 --- a/atst/routes/requests/financial_verification.py +++ b/atst/routes/requests/financial_verification.py @@ -27,20 +27,57 @@ def financial_form(request, data): return FinancialForm(data=data) -@requests_bp.route("/requests/verify/", methods=["GET"]) -def financial_verification(request_id=None): - request = Requests.get(g.current_user, request_id) - form_data = request.body.get("financial_verification") - if request.task_order: - form_data.update(task_order_data(request.task_order)) +class FinancialVerification: + def __init__(self, user, request_id=None, extended=False, post_data=None): + self.request = Requests.get(user, request_id) + self._extended = extended + self.post_data = post_data + + @property + def is_extended(self): + return self._extended or self.is_pending_changes + + @property + def is_pending_changes(self): + return self.request.is_pending_financial_verification_changes + + @property + def _task_order_data(self): + if self.request.task_order: + data = self.request.task_order.to_dictionary() + data["task_order_number"] = task_order.number + data["funding_type"] = task_order.funding_type.value + return data + else: + return {} + + @property + def _form_data(self): + form_data = self.request.body.get("financial_verification", {}) + form_data.update(self._task_order_data) + + return form_data + + @property + def form(self): + if self.is_extended: + return ExtendedFinancialForm(self._form_data) + else: + return FinancialForm(self._form_data) + + +@requests_bp.route("/requests/verify/", methods=["GET"]) +def financial_verification(request_id): + finver = FinancialVerification( + g.current_user, request_id=request_id, extended=http_request.args.get("extended") + ) - form = financial_form(request, form_data) return render_template( "requests/financial_verification.html", - f=form, - jedi_request=request, - review_comment=request.review_comment, - extended=is_extended(request), + f=finver.form, + jedi_request=finver.request, + review_comment=finver.request.review_comment, + extended=finver.is_extended, ) From 3384738f5ded20747f988451f8e3b7ba094c0536 Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 3 Oct 2018 15:47:01 -0400 Subject: [PATCH 2/6] use financial verification service class for all routes --- atst/domain/requests/query.py | 2 + .../routes/requests/financial_verification.py | 136 ++++++++++-------- 2 files changed, 78 insertions(+), 60 deletions(-) diff --git a/atst/domain/requests/query.py b/atst/domain/requests/query.py index 10e6e434..d080f941 100644 --- a/atst/domain/requests/query.py +++ b/atst/domain/requests/query.py @@ -1,8 +1,10 @@ from sqlalchemy import exists, and_, exc, text +from sqlalchemy.orm.exc import NoResultFound from atst.database import db from atst.domain.common import Query from atst.models.request import Request +from atst.domain.exceptions import NotFoundError class RequestsQuery(Query): diff --git a/atst/routes/requests/financial_verification.py b/atst/routes/requests/financial_verification.py index 4626302c..31605a15 100644 --- a/atst/routes/requests/financial_verification.py +++ b/atst/routes/requests/financial_verification.py @@ -6,32 +6,20 @@ from atst.domain.requests import Requests from atst.forms.financial import FinancialForm, ExtendedFinancialForm -def task_order_data(task_order): - data = task_order.to_dictionary() - data["task_order_number"] = task_order.number - data["funding_type"] = task_order.funding_type.value - return data - - -def is_extended(request): - return ( - http_request.args.get("extended") - or request.is_pending_financial_verification_changes - ) - - -def financial_form(request, data): - if is_extended(request): - return ExtendedFinancialForm(data=data) - else: - return FinancialForm(data=data) - - class FinancialVerification: def __init__(self, user, request_id=None, extended=False, post_data=None): self.request = Requests.get(user, request_id) self._extended = extended self.post_data = post_data + self._form = None + self.reset() + + def reset(self): + self.updateable = False + self.valid = False + self.workspace = None + if self._form: + self._form.reset() @property def is_extended(self): @@ -53,23 +41,59 @@ class FinancialVerification: @property def _form_data(self): - form_data = self.request.body.get("financial_verification", {}) - form_data.update(self._task_order_data) + if self.post_data: + return self.post_data + else: + form_data = self.request.body.get("financial_verification", {}) + form_data.update(self._task_order_data) - return form_data + return form_data @property def form(self): - if self.is_extended: - return ExtendedFinancialForm(self._form_data) + if not self._form: + if self.is_extended: + self._form = ExtendedFinancialForm(data=self._form_data) + else: + self._form = FinancialForm(data=self._form_data) + + return self._form + + def validate(self): + if self.form.validate(): + self.updateable = True + self.valid = self.form.perform_extra_validation( + self.request.body.get("financial_verification") + ) else: - return FinancialForm(self._form_data) + self.updateable = False + self.valid = False + + return self.valid + + @property + def pending(self): + return self.request.is_pending_ccpo_approval + + def finalize(self): + if self.updateable: + self.request = Requests.update_financial_verification( + self.request.id, self.form.data + ) + + if self.valid: + self.request = Requests.submit_financial_verification(self.request) + + if self.request.is_financially_verified: + self.workspace = Requests.approve_and_create_workspace(self.request) @requests_bp.route("/requests/verify/", methods=["GET"]) def financial_verification(request_id): finver = FinancialVerification( - g.current_user, request_id=request_id, extended=http_request.args.get("extended") + g.current_user, + request_id=request_id, + extended=http_request.args.get("extended"), ) return render_template( @@ -83,40 +107,32 @@ def financial_verification(request_id): @requests_bp.route("/requests/verify/", methods=["POST"]) def update_financial_verification(request_id): - post_data = http_request.form - existing_request = Requests.get(g.current_user, request_id) - form = financial_form(existing_request, post_data) - rerender_args = dict( - jedi_request=existing_request, f=form, extended=is_extended(existing_request) + finver = FinancialVerification( + g.current_user, + request_id=request_id, + extended=http_request.args.get("extended"), + post_data=http_request.form, ) - if form.validate(): - valid = form.perform_extra_validation( - existing_request.body.get("financial_verification") - ) - updated_request = Requests.update_financial_verification(request_id, form.data) - if valid: - submitted_request = Requests.submit_financial_verification(updated_request) - if submitted_request.is_financially_verified: - new_workspace = Requests.approve_and_create_workspace(submitted_request) - return redirect( - url_for( - "workspaces.new_project", - workspace_id=new_workspace.id, - newWorkspace=True, - ) - ) - else: - return redirect( - url_for("requests.requests_index", modal="pendingCCPOApproval") - ) + finver.validate() - else: - form.reset() - return render_template( - "requests/financial_verification.html", **rerender_args + finver.finalize() + + if finver.workspace: + return redirect( + url_for( + "workspaces.new_project", + workspace_id=finver.workspace.id, + newWorkspace=True, ) - + ) + elif finver.pending: + return redirect(url_for("requests.requests_index", modal="pendingCCPOApproval")) else: - form.reset() - return render_template("requests/financial_verification.html", **rerender_args) + finver.reset() + return render_template( + "requests/financial_verification.html", + jedi_request=finver.request, + f=finver.form, + extended=finver.is_extended, + ) From a206fafe0a883eb2d5f206010c2789cb3ae86d12 Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 3 Oct 2018 15:55:19 -0400 Subject: [PATCH 3/6] fix bug in fin ver service object --- atst/routes/requests/financial_verification.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/atst/routes/requests/financial_verification.py b/atst/routes/requests/financial_verification.py index 31605a15..72b6b13a 100644 --- a/atst/routes/requests/financial_verification.py +++ b/atst/routes/requests/financial_verification.py @@ -32,7 +32,8 @@ class FinancialVerification: @property def _task_order_data(self): if self.request.task_order: - data = self.request.task_order.to_dictionary() + task_order = self.request.task_order + data = task_order.to_dictionary() data["task_order_number"] = task_order.number data["funding_type"] = task_order.funding_type.value return data From ef75c15b99a119d73db4ab8057376a2261196996 Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 3 Oct 2018 16:56:27 -0400 Subject: [PATCH 4/6] small tweaks and test setup for FinancialVerification --- .../routes/requests/financial_verification.py | 32 ++++++++--------- tests/routes/test_financial_verification.py | 34 +++++++++++++++++++ 2 files changed, 49 insertions(+), 17 deletions(-) diff --git a/atst/routes/requests/financial_verification.py b/atst/routes/requests/financial_verification.py index 72b6b13a..46d43b25 100644 --- a/atst/routes/requests/financial_verification.py +++ b/atst/routes/requests/financial_verification.py @@ -7,16 +7,16 @@ from atst.forms.financial import FinancialForm, ExtendedFinancialForm class FinancialVerification: - def __init__(self, user, request_id=None, extended=False, post_data=None): + def __init__(self, user, request_id, extended=False, post_data=None): self.request = Requests.get(user, request_id) self._extended = extended - self.post_data = post_data + self._post_data = post_data self._form = None self.reset() def reset(self): - self.updateable = False - self.valid = False + self._updateable = False + self._valid = False self.workspace = None if self._form: self._form.reset() @@ -42,8 +42,8 @@ class FinancialVerification: @property def _form_data(self): - if self.post_data: - return self.post_data + if self._post_data: + return self._post_data else: form_data = self.request.body.get("financial_verification", {}) form_data.update(self._task_order_data) @@ -62,27 +62,27 @@ class FinancialVerification: def validate(self): if self.form.validate(): - self.updateable = True - self.valid = self.form.perform_extra_validation( + self._updateable = True + self._valid = self.form.perform_extra_validation( self.request.body.get("financial_verification") ) else: - self.updateable = False - self.valid = False + self._updateable = False + self._valid = False - return self.valid + return self._valid @property def pending(self): return self.request.is_pending_ccpo_approval def finalize(self): - if self.updateable: + if self._updateable: self.request = Requests.update_financial_verification( self.request.id, self.form.data ) - if self.valid: + if self._valid: self.request = Requests.submit_financial_verification(self.request) if self.request.is_financially_verified: @@ -92,9 +92,7 @@ class FinancialVerification: @requests_bp.route("/requests/verify/", methods=["GET"]) def financial_verification(request_id): finver = FinancialVerification( - g.current_user, - request_id=request_id, - extended=http_request.args.get("extended"), + g.current_user, request_id, extended=http_request.args.get("extended") ) return render_template( @@ -110,7 +108,7 @@ def financial_verification(request_id): def update_financial_verification(request_id): finver = FinancialVerification( g.current_user, - request_id=request_id, + request_id, extended=http_request.args.get("extended"), post_data=http_request.form, ) diff --git a/tests/routes/test_financial_verification.py b/tests/routes/test_financial_verification.py index aaa9b54d..e7d2938f 100644 --- a/tests/routes/test_financial_verification.py +++ b/tests/routes/test_financial_verification.py @@ -4,6 +4,7 @@ from flask import url_for from atst.eda_client import MockEDAClient from atst.models.request_status_event import RequestStatus +from atst.routes.requests.financial_verification import FinancialVerification from tests.mocks import MOCK_REQUEST, MOCK_USER from tests.factories import ( @@ -172,3 +173,36 @@ def test_displays_ccpo_review_comment(user_session, client): response = client.get("/requests/verify/{}".format(request.id)) body = response.data.decode() assert review_comment in body + + +class TestFinancialVerification: + @pytest.fixture(scope="function", autouse=True) + def apply_monkeypath(self, monkeypatch): + monkeypatch.setattr( + "atst.domain.requests.Requests.get", lambda *args: self.request + ) + + def _service_object(self, request=None, extended=False, post_data={}): + if not request: + self.request = RequestFactory.create() + else: + self.request = request + + return FinancialVerification( + UserFactory.create(), + self.request.id, + extended=extended, + post_data=post_data, + ) + + def test_is_extended(self): + finver_one = self._service_object() + assert not finver_one.is_extended + finver_two = self._service_object( + request=RequestFactory.create_with_status( + RequestStatus.CHANGES_REQUESTED_TO_FINVER + ) + ) + assert finver_two.is_extended + finver_three = self._service_object(extended=True) + assert finver_three.is_extended From 39c5f3e00e7486c9c5f13328f8bd4e2564c68a8a Mon Sep 17 00:00:00 2001 From: Montana Date: Mon, 8 Oct 2018 08:53:12 -0400 Subject: [PATCH 5/6] Add two more tests --- tests/routes/test_financial_verification.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/routes/test_financial_verification.py b/tests/routes/test_financial_verification.py index e7d2938f..0c20d7d4 100644 --- a/tests/routes/test_financial_verification.py +++ b/tests/routes/test_financial_verification.py @@ -206,3 +206,23 @@ class TestFinancialVerification: assert finver_two.is_extended finver_three = self._service_object(extended=True) assert finver_three.is_extended + + def test_is_pending_changes(self): + finver_one = self._service_object() + assert not finver_one.is_pending_changes + finver_two = self._service_object( + request=RequestFactory.create_with_status( + RequestStatus.CHANGES_REQUESTED_TO_FINVER + ) + ) + assert finver_two.is_pending_changes + + def test_pending(self): + finver_one = self._service_object() + assert not finver_one.pending + finver_two = self._service_object( + request=RequestFactory.create_with_status( + RequestStatus.PENDING_CCPO_APPROVAL + ) + ) + assert finver_two.pending From 1329cd82064bb545c1f7d8fb5355a5fb796dd2dc Mon Sep 17 00:00:00 2001 From: dandds Date: Tue, 9 Oct 2018 10:49:08 -0400 Subject: [PATCH 6/6] pass request directly to FinancialVerification class --- atst/routes/requests/financial_verification.py | 15 ++++++--------- tests/routes/test_financial_verification.py | 11 +---------- 2 files changed, 7 insertions(+), 19 deletions(-) diff --git a/atst/routes/requests/financial_verification.py b/atst/routes/requests/financial_verification.py index 46d43b25..5b84392e 100644 --- a/atst/routes/requests/financial_verification.py +++ b/atst/routes/requests/financial_verification.py @@ -7,8 +7,8 @@ from atst.forms.financial import FinancialForm, ExtendedFinancialForm class FinancialVerification: - def __init__(self, user, request_id, extended=False, post_data=None): - self.request = Requests.get(user, request_id) + def __init__(self, request, extended=False, post_data=None): + self.request = request self._extended = extended self._post_data = post_data self._form = None @@ -91,9 +91,8 @@ class FinancialVerification: @requests_bp.route("/requests/verify/", methods=["GET"]) def financial_verification(request_id): - finver = FinancialVerification( - g.current_user, request_id, extended=http_request.args.get("extended") - ) + request = Requests.get(g.current_user, request_id) + finver = FinancialVerification(request, extended=http_request.args.get("extended")) return render_template( "requests/financial_verification.html", @@ -106,11 +105,9 @@ def financial_verification(request_id): @requests_bp.route("/requests/verify/", methods=["POST"]) def update_financial_verification(request_id): + request = Requests.get(g.current_user, request_id) finver = FinancialVerification( - g.current_user, - request_id, - extended=http_request.args.get("extended"), - post_data=http_request.form, + request, extended=http_request.args.get("extended"), post_data=http_request.form ) finver.validate() diff --git a/tests/routes/test_financial_verification.py b/tests/routes/test_financial_verification.py index 0c20d7d4..91630abe 100644 --- a/tests/routes/test_financial_verification.py +++ b/tests/routes/test_financial_verification.py @@ -176,12 +176,6 @@ def test_displays_ccpo_review_comment(user_session, client): class TestFinancialVerification: - @pytest.fixture(scope="function", autouse=True) - def apply_monkeypath(self, monkeypatch): - monkeypatch.setattr( - "atst.domain.requests.Requests.get", lambda *args: self.request - ) - def _service_object(self, request=None, extended=False, post_data={}): if not request: self.request = RequestFactory.create() @@ -189,10 +183,7 @@ class TestFinancialVerification: self.request = request return FinancialVerification( - UserFactory.create(), - self.request.id, - extended=extended, - post_data=post_data, + self.request, extended=extended, post_data=post_data ) def test_is_extended(self):