From d295798d75dc9f6b9ab1530fba2d4edd32e5f110 Mon Sep 17 00:00:00 2001 From: dandds Date: Tue, 4 Sep 2018 17:28:16 -0400 Subject: [PATCH 01/16] ccpo should see request approval screen for all request --- tests/routes/test_requests_index.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/routes/test_requests_index.py b/tests/routes/test_requests_index.py index 29300b65..c8d6fa5d 100644 --- a/tests/routes/test_requests_index.py +++ b/tests/routes/test_requests_index.py @@ -30,11 +30,13 @@ def test_ccpo_sees_approval_screen(): request = RequestFactory.create() Requests.submit(request) ccpo_context = RequestsIndex(ccpo).execute() - assert ccpo_context["requests"][0]["edit_link"] == url_for( - "requests.approval", request_id=request.id + assert ( + ccpo_context["requests"][0]["edit_link"] + == url_for("requests.approval", request_id=request.id) ) mo_context = RequestsIndex(request.creator).execute() - assert mo_context["requests"][0]["edit_link"] != url_for( - "requests.approval", request_id=request.id + assert ( + mo_context["requests"][0]["edit_link"] + != url_for("requests.approval", request_id=request.id) ) From 8e000e3e77f03cdfa003753d78227cf1552e4ba0 Mon Sep 17 00:00:00 2001 From: dandds Date: Thu, 6 Sep 2018 15:43:14 -0400 Subject: [PATCH 02/16] add request review --- .../7bdb2055d7c7_add_request_review_table.py | 42 +++++++++++++++++++ atst/models/__init__.py | 1 + atst/models/request.py | 12 +++++- atst/models/request_review.py | 19 +++++++++ atst/models/request_status_event.py | 3 ++ tests/factories.py | 6 +++ tests/models/test_requests.py | 21 +++++++++- 7 files changed, 101 insertions(+), 3 deletions(-) create mode 100644 alembic/versions/7bdb2055d7c7_add_request_review_table.py create mode 100644 atst/models/request_review.py diff --git a/alembic/versions/7bdb2055d7c7_add_request_review_table.py b/alembic/versions/7bdb2055d7c7_add_request_review_table.py new file mode 100644 index 00000000..cdc979d0 --- /dev/null +++ b/alembic/versions/7bdb2055d7c7_add_request_review_table.py @@ -0,0 +1,42 @@ +"""add request review table + +Revision ID: 7bdb2055d7c7 +Revises: 06aa23166ca9 +Create Date: 2018-09-06 15:15:40.666840 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = '7bdb2055d7c7' +down_revision = '06aa23166ca9' +branch_labels = None +depends_on = None + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.create_table('request_reviews', + sa.Column('id', sa.Integer(), nullable=False), + sa.Column('comments', sa.String(), nullable=True), + sa.Column('fname_mao', sa.String(), nullable=True), + sa.Column('lname_mao', sa.String(), nullable=True), + sa.Column('email_mao', sa.String(), nullable=True), + sa.Column('phone_mao', sa.String(), nullable=True), + sa.Column('fname_ccpo', sa.String(), nullable=True), + sa.Column('lname_ccpo', sa.String(), nullable=True), + sa.PrimaryKeyConstraint('id') + ) + op.add_column('request_status_events', sa.Column('request_review_id', sa.Integer(), nullable=True)) + op.create_foreign_key(None, 'request_status_events', 'request_reviews', ['request_review_id'], ['id']) + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_constraint(None, 'request_status_events', type_='foreignkey') + op.drop_column('request_status_events', 'request_review_id') + op.drop_table('request_reviews') + # ### end Alembic commands ### diff --git a/atst/models/__init__.py b/atst/models/__init__.py index 4bedebfe..245e3806 100644 --- a/atst/models/__init__.py +++ b/atst/models/__init__.py @@ -15,3 +15,4 @@ from .project import Project from .environment import Environment from .attachment import Attachment from .request_revision import RequestRevision +from .request_review import RequestReview diff --git a/atst/models/request.py b/atst/models/request.py index 4e155504..71609702 100644 --- a/atst/models/request.py +++ b/atst/models/request.py @@ -115,13 +115,17 @@ class Request(Base): return body + @property + def latest_status(self): + return self.status_events[-1] + @property def status(self): - return self.status_events[-1].new_status + return self.latest_status.new_status @property def status_displayname(self): - return self.status_events[-1].displayname + return self.latest_status.displayname @property def annual_spend(self): @@ -154,3 +158,7 @@ class Request(Base): RequestStatus.PENDING_FINANCIAL_VERIFICATION: "mission_owner", RequestStatus.PENDING_CCPO_APPROVAL: "ccpo", }.get(self.status) + + @property + def reviews(self): + return [status.review for status in self.status_events if status.review] diff --git a/atst/models/request_review.py b/atst/models/request_review.py new file mode 100644 index 00000000..3b4df410 --- /dev/null +++ b/atst/models/request_review.py @@ -0,0 +1,19 @@ +from sqlalchemy import Column, Integer, String +from sqlalchemy.orm import relationship + +from atst.models import Base + + +class RequestReview(Base): + __tablename__ = "request_reviews" + + id = Column(Integer, primary_key=True) + status = relationship("RequestStatusEvent", back_populates="review") + + comments = Column(String) + fname_mao = Column(String) + lname_mao = Column(String) + email_mao = Column(String) + phone_mao = Column(String) + fname_ccpo = Column(String) + lname_ccpo = Column(String) diff --git a/atst/models/request_status_event.py b/atst/models/request_status_event.py index ba13c242..11ae7dad 100644 --- a/atst/models/request_status_event.py +++ b/atst/models/request_status_event.py @@ -35,6 +35,9 @@ class RequestStatusEvent(Base): request_revision_id = Column(ForeignKey("request_revisions.id"), nullable=False) revision = relationship("RequestRevision") + request_review_id = Column(ForeignKey("request_reviews.id"), nullable=True) + review = relationship("RequestReview", back_populates="status") + @property def displayname(self): return self.new_status.value diff --git a/tests/factories.py b/tests/factories.py index b73ea74a..e2b659a7 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -7,6 +7,7 @@ import datetime from atst.forms.data import SERVICE_BRANCHES from atst.models.request import Request from atst.models.request_revision import RequestRevision +from atst.models.request_review import RequestReview from atst.models.request_status_event import RequestStatusEvent, RequestStatus from atst.models.pe_number import PENumber from atst.models.task_order import TaskOrder @@ -55,6 +56,11 @@ class RequestRevisionFactory(factory.alchemy.SQLAlchemyModelFactory): id = factory.Sequence(lambda x: uuid4()) +class RequestReviewFactory(factory.alchemy.SQLAlchemyModelFactory): + class Meta: + model = RequestReview + + class RequestFactory(factory.alchemy.SQLAlchemyModelFactory): class Meta: model = Request diff --git a/tests/models/test_requests.py b/tests/models/test_requests.py index cf17afa8..1b16d7db 100644 --- a/tests/models/test_requests.py +++ b/tests/models/test_requests.py @@ -1,4 +1,9 @@ -from tests.factories import RequestFactory, UserFactory +from tests.factories import ( + RequestFactory, + UserFactory, + RequestStatusEventFactory, + RequestReviewFactory, +) from atst.domain.requests import Requests, RequestStatus @@ -69,3 +74,17 @@ def test_annual_spend(): request = RequestFactory.create() monthly = request.body.get("details_of_use").get("estimated_monthly_spend") assert request.annual_spend == monthly * 12 + + +def test_reviews(): + request = RequestFactory.create() + request.status_events = [ + RequestStatusEventFactory.create( + revision=request.latest_revision, review=RequestReviewFactory.create() + ), + RequestStatusEventFactory.create( + revision=request.latest_revision, review=RequestReviewFactory.create() + ), + RequestStatusEventFactory.create(revision=request.latest_revision), + ] + assert len(request.reviews) == 2 From 14d03e7e660e1f061c11ded9b2fbd756f11d7b43 Mon Sep 17 00:00:00 2001 From: dandds Date: Fri, 7 Sep 2018 10:28:03 -0400 Subject: [PATCH 03/16] requests domain can approve a request for financial verification --- atst/domain/requests.py | 12 ++++++++++++ tests/domain/test_requests.py | 11 +++++++++++ tests/factories.py | 32 +++++++++++++++++++++++--------- 3 files changed, 46 insertions(+), 9 deletions(-) diff --git a/atst/domain/requests.py b/atst/domain/requests.py index 5f7e68bc..7c2897cf 100644 --- a/atst/domain/requests.py +++ b/atst/domain/requests.py @@ -12,6 +12,7 @@ from atst.domain.workspaces import Workspaces from atst.models.request import Request from atst.models.request_revision import RequestRevision from atst.models.request_status_event import RequestStatusEvent, RequestStatus +from atst.models.request_review import RequestReview from atst.utils import deep_merge from .exceptions import NotFoundError, UnauthorizedError @@ -260,3 +261,14 @@ WHERE requests_with_status.status = :status db.session.commit() return request + + @classmethod + def approve_for_financial_verification(cls, request, review_data): + Requests.set_status(request, RequestStatus.PENDING_FINANCIAL_VERIFICATION) + + request.latest_status.review = RequestReview(**review_data) + + db.session.add(request) + db.session.commit() + + return request diff --git a/tests/domain/test_requests.py b/tests/domain/test_requests.py index e938d7bc..a0fd97ed 100644 --- a/tests/domain/test_requests.py +++ b/tests/domain/test_requests.py @@ -13,6 +13,7 @@ from tests.factories import ( RequestStatusEventFactory, TaskOrderFactory, RequestRevisionFactory, + RequestReviewFactory, ) @@ -178,3 +179,13 @@ def test_set_status_sets_revision(): request = RequestFactory.create() Requests.set_status(request, RequestStatus.APPROVED) assert request.latest_revision == request.status_events[-1].revision + + +def test_approve_for_financial_verification(): + request = RequestFactory.create() + review_data = RequestReviewFactory.dictionary() + Requests.approve_for_financial_verification(request, review_data) + assert request.status == RequestStatus.PENDING_FINANCIAL_VERIFICATION + current_review = request.latest_status.review + assert current_review.fname_mao == review_data["fname_mao"] + diff --git a/tests/factories.py b/tests/factories.py index e2b659a7..e8165393 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -17,14 +17,20 @@ from atst.models.workspace import Workspace from atst.domain.roles import Roles -class RoleFactory(factory.alchemy.SQLAlchemyModelFactory): +class Base(factory.alchemy.SQLAlchemyModelFactory): + @classmethod + def dictionary(cls, **attrs): + return factory.build(dict, FACTORY_CLASS=cls, **attrs) + + +class RoleFactory(Base): class Meta: model = Role permissions = [] -class UserFactory(factory.alchemy.SQLAlchemyModelFactory): +class UserFactory(Base): class Meta: model = User @@ -41,7 +47,7 @@ class UserFactory(factory.alchemy.SQLAlchemyModelFactory): return cls.create(atat_role=role, **kwargs) -class RequestStatusEventFactory(factory.alchemy.SQLAlchemyModelFactory): +class RequestStatusEventFactory(Base): class Meta: model = RequestStatusEvent @@ -49,19 +55,27 @@ class RequestStatusEventFactory(factory.alchemy.SQLAlchemyModelFactory): sequence = 1 -class RequestRevisionFactory(factory.alchemy.SQLAlchemyModelFactory): +class RequestRevisionFactory(Base): class Meta: model = RequestRevision id = factory.Sequence(lambda x: uuid4()) -class RequestReviewFactory(factory.alchemy.SQLAlchemyModelFactory): +class RequestReviewFactory(Base): class Meta: model = RequestReview + comments = factory.Faker("sentence") + fname_mao = factory.Faker("first_name") + lname_mao = factory.Faker("last_name") + email_mao = factory.Faker("email") + phone_mao = factory.Faker("phone_number") + fname_ccpo = factory.Faker("first_name") + lname_ccpo = factory.Faker("last_name") -class RequestFactory(factory.alchemy.SQLAlchemyModelFactory): + +class RequestFactory(Base): class Meta: model = Request @@ -133,17 +147,17 @@ class RequestFactory(factory.alchemy.SQLAlchemyModelFactory): return RequestRevisionFactory.build(**data) -class PENumberFactory(factory.alchemy.SQLAlchemyModelFactory): +class PENumberFactory(Base): class Meta: model = PENumber -class TaskOrderFactory(factory.alchemy.SQLAlchemyModelFactory): +class TaskOrderFactory(Base): class Meta: model = TaskOrder -class WorkspaceFactory(factory.alchemy.SQLAlchemyModelFactory): +class WorkspaceFactory(Base): class Meta: model = Workspace From 15713d78a462643f11467671c1be1f6a81545d3c Mon Sep 17 00:00:00 2001 From: dandds Date: Fri, 7 Sep 2018 10:50:08 -0400 Subject: [PATCH 04/16] resolve migration chain --- alembic/versions/7bdb2055d7c7_add_request_review_table.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/alembic/versions/7bdb2055d7c7_add_request_review_table.py b/alembic/versions/7bdb2055d7c7_add_request_review_table.py index cdc979d0..99274e85 100644 --- a/alembic/versions/7bdb2055d7c7_add_request_review_table.py +++ b/alembic/versions/7bdb2055d7c7_add_request_review_table.py @@ -1,7 +1,7 @@ """add request review table Revision ID: 7bdb2055d7c7 -Revises: 06aa23166ca9 +Revises: ad30159ef19b Create Date: 2018-09-06 15:15:40.666840 """ @@ -11,7 +11,7 @@ import sqlalchemy as sa # revision identifiers, used by Alembic. revision = '7bdb2055d7c7' -down_revision = '06aa23166ca9' +down_revision = 'ad30159ef19b' branch_labels = None depends_on = None From c123cdd6e9a0bd53570ffcbc146b5cd5d8154b30 Mon Sep 17 00:00:00 2001 From: dandds Date: Mon, 10 Sep 2018 09:58:42 -0400 Subject: [PATCH 05/16] ccpo can submit basic review --- atst/forms/ccpo_review.py | 22 +++++++++++ atst/routes/requests/approval.py | 33 ++++++++++++---- templates/requests/approval.html | 56 ++++++--------------------- tests/factories.py | 2 +- tests/routes/test_request_approval.py | 15 ++++++- 5 files changed, 74 insertions(+), 54 deletions(-) create mode 100644 atst/forms/ccpo_review.py diff --git a/atst/forms/ccpo_review.py b/atst/forms/ccpo_review.py new file mode 100644 index 00000000..6db8c9a5 --- /dev/null +++ b/atst/forms/ccpo_review.py @@ -0,0 +1,22 @@ +from wtforms.fields.html5 import EmailField, TelField +from wtforms.fields import StringField, TextAreaField +from wtforms.validators import Required, Email + +from .forms import ValidatedForm +from .validators import Alphabet, PhoneNumber + + +class CCPOReviewForm(ValidatedForm): + comments = TextAreaField( + "Comments", + description="Add notes or comments explaining what changes are being requested or why further discussion is needed about this request.", + ) + fname_mao = StringField("First Name", validators=[Required(), Alphabet()]) + lname_mao = StringField("Last Name", validators=[Required(), Alphabet()]) + email_mao = EmailField("Mission Owner e-mail (optional)", validators=[Email()]) + phone_mao = TelField( + "Mission Owner phone number (optional)", + validators=[Required(), PhoneNumber()], + ) + fname_ccpo = StringField("First Name", validators=[Required(), Alphabet()]) + lname_ccpo = StringField("Last Name", validators=[Required(), Alphabet()]) diff --git a/atst/routes/requests/approval.py b/atst/routes/requests/approval.py index 20b9c890..f9385dbd 100644 --- a/atst/routes/requests/approval.py +++ b/atst/routes/requests/approval.py @@ -1,10 +1,11 @@ -from flask import render_template, g, Response +from flask import render_template, g, Response, request as http_request, redirect, url_for from flask import current_app as app from . import requests_bp from atst.domain.requests import Requests from atst.domain.exceptions import NotFoundError from atst.domain.authz import Authorization +from atst.forms.ccpo_review import CCPOReviewForm def task_order_dictionary(task_order): @@ -15,11 +16,7 @@ def task_order_dictionary(task_order): } -@requests_bp.route("/requests/approval/", methods=["GET"]) -def approval(request_id): - request = Requests.get(g.current_user, request_id) - Authorization.check_can_approve_request(g.current_user) - +def render_approval(request, form=None): data = request.body if request.task_order: data["task_order"] = task_order_dictionary(request.task_order) @@ -27,13 +24,35 @@ def approval(request_id): return render_template( "requests/approval.html", data=data, - request_id=request_id, + request_id=request.id, status=request.status.value, financial_review=True, pdf_available=request.task_order and request.task_order.pdf, + f=form or CCPOReviewForm(), ) +@requests_bp.route("/requests/approval/", methods=["GET"]) +def approval(request_id): + request = Requests.get(g.current_user, request_id) + Authorization.check_can_approve_request(g.current_user) + + return render_approval(request) + + +@requests_bp.route("/requests/submit_approval/", methods=["POST"]) +def submit_approval(request_id): + request = Requests.get(g.current_user, request_id) + Authorization.check_can_approve_request(g.current_user) + + form = CCPOReviewForm(http_request.form) + if form.validate(): + Requests.approve_for_financial_verification(request, form.data) + return redirect(url_for("requests.requests_index")) + else: + return render_approval(request, form) + + @requests_bp.route("/requests/task_order_download/", methods=["GET"]) def task_order_pdf_download(request_id): request = Requests.get(g.current_user, request_id) diff --git a/templates/requests/approval.html b/templates/requests/approval.html index e67bd935..d9952981 100644 --- a/templates/requests/approval.html +++ b/templates/requests/approval.html @@ -2,12 +2,14 @@ {% from "components/icon.html" import Icon %} {% from "components/alert.html" import Alert %} +{% from "components/text_input.html" import TextInput %} {% block content %}
-
+ + {{ f.csrf_token }}

Request #{{ request_id }}

@@ -58,21 +60,11 @@
- -
- - -
- + {{ TextInput(f.fname_mao, placeholder="First name of mission authorizing official") }}
- -
- - -
- + {{ TextInput(f.lname_mao, placeholder="Last name of mission authorizing official") }}
@@ -80,23 +72,12 @@
- - -
- - -
- + {{ TextInput(f.email_mao, placeholder="name@mail.mil") }}
- -
- - -
- + {{ TextInput(f.phone_mao, placeholder="(123) 456-7890", validation='usPhone') }}
@@ -107,21 +88,11 @@
- -
- - -
- + {{ TextInput(f.fname_ccpo, placeholder="First name of CCPO authorizing official") }}
- -
- - -
- + {{ TextInput(f.lname_ccpo, placeholder="Last name of CCPO authorizing official") }}
@@ -135,12 +106,7 @@
- -
- - -
- + {{ TextInput(f.comments, paragraph=True, placeholder="Add notes or comments for internal CCPO reference.") }}
@@ -150,7 +116,7 @@
- Approve Request + Mark as Changes Requested {{ Icon('x') }} diff --git a/tests/factories.py b/tests/factories.py index e8165393..fa1036c9 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -70,7 +70,7 @@ class RequestReviewFactory(Base): fname_mao = factory.Faker("first_name") lname_mao = factory.Faker("last_name") email_mao = factory.Faker("email") - phone_mao = factory.Faker("phone_number") + phone_mao = factory.LazyFunction(lambda: "".join(random.choices(string.digits, k=10))) fname_ccpo = factory.Faker("first_name") lname_ccpo = factory.Faker("last_name") diff --git a/tests/routes/test_request_approval.py b/tests/routes/test_request_approval.py index 5680e30c..0198f816 100644 --- a/tests/routes/test_request_approval.py +++ b/tests/routes/test_request_approval.py @@ -4,7 +4,9 @@ from flask import url_for from atst.models.attachment import Attachment from atst.domain.roles import Roles -from tests.factories import RequestFactory, TaskOrderFactory, UserFactory +from tests.factories import ( + RequestFactory, TaskOrderFactory, UserFactory, RequestReviewFactory +) def test_ccpo_can_view_approval(user_session, client): @@ -59,3 +61,14 @@ def test_task_order_download_does_not_exist(client, user_session): url_for("requests.task_order_pdf_download", request_id=request.id) ) assert response.status_code == 404 + + +def test_can_submit_request_approval(client, user_session): + user = UserFactory.from_atat_role("ccpo") + user_session(user) + request = RequestFactory.create() + review_data = RequestReviewFactory.dictionary() + response = client.post( + url_for("requests.submit_approval", request_id=request.id), data=review_data + ) + assert response.status_code == 301 From c4e99712062766439832a6ea05a2a26fcad533f4 Mon Sep 17 00:00:00 2001 From: dandds Date: Mon, 10 Sep 2018 10:28:44 -0400 Subject: [PATCH 06/16] distinguish final CCPO approval from acceptance of initial request --- atst/domain/requests.py | 39 +++++++++++++++------------ atst/models/request.py | 1 + atst/models/request_status_event.py | 1 + atst/routes/requests/index.py | 8 +++--- tests/domain/test_requests.py | 4 +-- tests/routes/test_request_approval.py | 2 +- tests/test_integration.py | 2 +- 7 files changed, 32 insertions(+), 25 deletions(-) diff --git a/atst/domain/requests.py b/atst/domain/requests.py index 7c2897cf..c34009e5 100644 --- a/atst/domain/requests.py +++ b/atst/domain/requests.py @@ -76,10 +76,9 @@ class Requests(object): filters.append(Request.creator == creator) requests = ( - db.session.query(Request) - .filter(*filters) - .order_by(Request.time_created.desc()) - .all() + db.session.query(Request).filter(*filters).order_by( + Request.time_created.desc() + ).all() ) return requests @@ -91,7 +90,7 @@ class Requests(object): if Requests.should_auto_approve(request): new_status = RequestStatus.PENDING_FINANCIAL_VERIFICATION else: - new_status = RequestStatus.PENDING_CCPO_APPROVAL + new_status = RequestStatus.PENDING_CCPO_ACCEPTANCE request = Requests.set_status(request, new_status) @@ -119,10 +118,9 @@ class Requests(object): # Query for request matching id, acquiring a row-level write lock. # https://www.postgresql.org/docs/10/static/sql-select.html#SQL-FOR-UPDATE-SHARE return ( - db.session.query(Request) - .filter_by(id=request_id) - .with_for_update(of=Request) - .one() + db.session.query(Request).filter_by(id=request_id).with_for_update( + of=Request + ).one() ) except NoResultFound: @@ -156,16 +154,13 @@ class Requests(object): return dollar_value < cls.AUTO_APPROVE_THRESHOLD _VALID_SUBMISSION_STATUSES = [ - RequestStatus.STARTED, - RequestStatus.CHANGES_REQUESTED, + RequestStatus.STARTED, RequestStatus.CHANGES_REQUESTED ] @classmethod def should_allow_submission(cls, request): all_request_sections = [ - "details_of_use", - "information_about_you", - "primary_poc", + "details_of_use", "information_about_you", "primary_poc" ] existing_request_sections = request.body.keys() return request.status in Requests._VALID_SUBMISSION_STATUSES and all( @@ -176,6 +171,10 @@ class Requests(object): def is_pending_financial_verification(cls, request): return request.status == RequestStatus.PENDING_FINANCIAL_VERIFICATION + @classmethod + def is_pending_ccpo_acceptance(cls, request): + return request.status == RequestStatus.PENDING_CCPO_ACCEPTANCE + @classmethod def is_pending_ccpo_approval(cls, request): return request.status == RequestStatus.PENDING_CCPO_APPROVAL @@ -215,7 +214,12 @@ WHERE requests_with_status.status = :status @classmethod def pending_ccpo_count(cls): - return Requests.status_count(RequestStatus.PENDING_CCPO_APPROVAL) + return sum( + [ + Requests.status_count(RequestStatus.PENDING_CCPO_ACCEPTANCE), + Requests.status_count(RequestStatus.PENDING_CCPO_APPROVAL), + ] + ) @classmethod def completed_count(cls): @@ -237,8 +241,9 @@ WHERE requests_with_status.status = :status else: task_order_number = request_data.get("task_order_number") - if "task_order" in request_data and isinstance( - request_data["task_order"], FileStorage + if ( + "task_order" in request_data + and isinstance(request_data["task_order"], FileStorage) ): task_order_data["pdf"] = request_data.pop("task_order") diff --git a/atst/models/request.py b/atst/models/request.py index 71609702..c991a708 100644 --- a/atst/models/request.py +++ b/atst/models/request.py @@ -157,6 +157,7 @@ class Request(Base): return { RequestStatus.PENDING_FINANCIAL_VERIFICATION: "mission_owner", RequestStatus.PENDING_CCPO_APPROVAL: "ccpo", + RequestStatus.PENDING_CCPO_ACCEPTANCE: "ccpo", }.get(self.status) @property diff --git a/atst/models/request_status_event.py b/atst/models/request_status_event.py index 11ae7dad..a5373bdd 100644 --- a/atst/models/request_status_event.py +++ b/atst/models/request_status_event.py @@ -13,6 +13,7 @@ class RequestStatus(Enum): STARTED = "Started" SUBMITTED = "Submitted" PENDING_FINANCIAL_VERIFICATION = "Pending Financial Verification" + PENDING_CCPO_ACCEPTANCE = "Pending CCPO Acceptance" PENDING_CCPO_APPROVAL = "Pending CCPO Approval" CHANGES_REQUESTED = "Changes Requested" APPROVED = "Approved" diff --git a/atst/routes/requests/index.py b/atst/routes/requests/index.py index 75be2c87..7032c12f 100644 --- a/atst/routes/requests/index.py +++ b/atst/routes/requests/index.py @@ -30,7 +30,7 @@ class RequestsIndex(object): return { "requests": mapped_requests, "pending_financial_verification": False, - "pending_ccpo_approval": False, + "pending_ccpo_acceptance": False, "extended_view": True, "kpi_inprogress": Requests.in_progress_count(), "kpi_pending": Requests.pending_ccpo_count(), @@ -47,12 +47,12 @@ class RequestsIndex(object): 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) + pending_ccpo = any(Requests.is_pending_ccpo_acceptance(r) for r in requests) return { "requests": mapped_requests, "pending_financial_verification": pending_fv, - "pending_ccpo_approval": pending_ccpo, + "pending_ccpo_acceptance": pending_ccpo, "num_action_required": num_action_required, "extended_view": False, } @@ -76,7 +76,7 @@ class RequestsIndex(object): edit_link = url_for( "requests.financial_verification", request_id=request.id ) - elif Requests.is_pending_ccpo_approval(request): + elif Requests.is_pending_ccpo_acceptance(request) or Requests.is_pending_ccpo_approval(request): edit_link = url_for("requests.view_pending_request", request_id=request.id) else: edit_link = url_for( diff --git a/tests/domain/test_requests.py b/tests/domain/test_requests.py index a0fd97ed..3d587cfa 100644 --- a/tests/domain/test_requests.py +++ b/tests/domain/test_requests.py @@ -51,14 +51,14 @@ def test_dont_auto_approve_if_dollar_value_is_1m_or_above(): new_request = RequestFactory.create(initial_revision={"dollar_value": 1000000}) request = Requests.submit(new_request) - assert request.status == RequestStatus.PENDING_CCPO_APPROVAL + assert request.status == RequestStatus.PENDING_CCPO_ACCEPTANCE def test_dont_auto_approve_if_no_dollar_value_specified(): new_request = RequestFactory.create(initial_revision={}) request = Requests.submit(new_request) - assert request.status == RequestStatus.PENDING_CCPO_APPROVAL + assert request.status == RequestStatus.PENDING_CCPO_ACCEPTANCE def test_should_allow_submission(): diff --git a/tests/routes/test_request_approval.py b/tests/routes/test_request_approval.py index 0198f816..36881805 100644 --- a/tests/routes/test_request_approval.py +++ b/tests/routes/test_request_approval.py @@ -71,4 +71,4 @@ def test_can_submit_request_approval(client, user_session): response = client.post( url_for("requests.submit_approval", request_id=request.id), data=review_data ) - assert response.status_code == 301 + assert response.status_code == 302 diff --git a/tests/test_integration.py b/tests/test_integration.py index 51a75789..152e960b 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -81,4 +81,4 @@ def test_stepthrough_request_form(user_session, screens, client): ) finished_request = Requests.get(user, req_id) - assert finished_request.status == RequestStatus.PENDING_CCPO_APPROVAL + assert finished_request.status == RequestStatus.PENDING_CCPO_ACCEPTANCE From bad78b7c644342adf4c595ea42177031e12b93d7 Mon Sep 17 00:00:00 2001 From: dandds Date: Mon, 10 Sep 2018 10:42:04 -0400 Subject: [PATCH 07/16] refine required fields for request review --- ...d26_request_reviews_non_nullable_fields.py | 50 +++++++++++++++++++ atst/forms/ccpo_review.py | 5 +- atst/models/request_review.py | 8 +-- templates/requests/approval.html | 13 ++--- 4 files changed, 63 insertions(+), 13 deletions(-) create mode 100644 alembic/versions/bc6edccded26_request_reviews_non_nullable_fields.py diff --git a/alembic/versions/bc6edccded26_request_reviews_non_nullable_fields.py b/alembic/versions/bc6edccded26_request_reviews_non_nullable_fields.py new file mode 100644 index 00000000..e143275a --- /dev/null +++ b/alembic/versions/bc6edccded26_request_reviews_non_nullable_fields.py @@ -0,0 +1,50 @@ +"""request_reviews non-nullable fields + +Revision ID: bc6edccded26 +Revises: 7bdb2055d7c7 +Create Date: 2018-09-10 10:38:00.107297 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = 'bc6edccded26' +down_revision = '7bdb2055d7c7' +branch_labels = None +depends_on = None + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.alter_column('request_reviews', 'fname_ccpo', + existing_type=sa.VARCHAR(), + nullable=False) + op.alter_column('request_reviews', 'fname_mao', + existing_type=sa.VARCHAR(), + nullable=False) + op.alter_column('request_reviews', 'lname_ccpo', + existing_type=sa.VARCHAR(), + nullable=False) + op.alter_column('request_reviews', 'lname_mao', + existing_type=sa.VARCHAR(), + nullable=False) + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.alter_column('request_reviews', 'lname_mao', + existing_type=sa.VARCHAR(), + nullable=True) + op.alter_column('request_reviews', 'lname_ccpo', + existing_type=sa.VARCHAR(), + nullable=True) + op.alter_column('request_reviews', 'fname_mao', + existing_type=sa.VARCHAR(), + nullable=True) + op.alter_column('request_reviews', 'fname_ccpo', + existing_type=sa.VARCHAR(), + nullable=True) + # ### end Alembic commands ### diff --git a/atst/forms/ccpo_review.py b/atst/forms/ccpo_review.py index 6db8c9a5..89b5ed03 100644 --- a/atst/forms/ccpo_review.py +++ b/atst/forms/ccpo_review.py @@ -8,15 +8,14 @@ from .validators import Alphabet, PhoneNumber class CCPOReviewForm(ValidatedForm): comments = TextAreaField( - "Comments", - description="Add notes or comments explaining what changes are being requested or why further discussion is needed about this request.", + "Comments (optional)", ) fname_mao = StringField("First Name", validators=[Required(), Alphabet()]) lname_mao = StringField("Last Name", validators=[Required(), Alphabet()]) email_mao = EmailField("Mission Owner e-mail (optional)", validators=[Email()]) phone_mao = TelField( "Mission Owner phone number (optional)", - validators=[Required(), PhoneNumber()], + validators=[PhoneNumber()], ) fname_ccpo = StringField("First Name", validators=[Required(), Alphabet()]) lname_ccpo = StringField("Last Name", validators=[Required(), Alphabet()]) diff --git a/atst/models/request_review.py b/atst/models/request_review.py index 3b4df410..a17367ff 100644 --- a/atst/models/request_review.py +++ b/atst/models/request_review.py @@ -11,9 +11,9 @@ class RequestReview(Base): status = relationship("RequestStatusEvent", back_populates="review") comments = Column(String) - fname_mao = Column(String) - lname_mao = Column(String) + fname_mao = Column(String, nullable=False) + lname_mao = Column(String, nullable=False) email_mao = Column(String) phone_mao = Column(String) - fname_ccpo = Column(String) - lname_ccpo = Column(String) + fname_ccpo = Column(String, nullable=False) + lname_ccpo = Column(String, nullable=False) diff --git a/templates/requests/approval.html b/templates/requests/approval.html index d9952981..49a526d8 100644 --- a/templates/requests/approval.html +++ b/templates/requests/approval.html @@ -39,11 +39,7 @@ Provide instructions or notes for additional information that is necessary to approve the request here. The requestor may then re-submit the updated request or initiate contact outside of AT-AT if further discussion is required. These notes will be visible to the person making the JEDI request. -
- - -
- + {{ TextInput(f.comments, paragraph=True, placeholder="Add notes or comments explaining what changes are being requested or why further discussion is needed about this request.") }} @@ -106,7 +102,12 @@
- {{ TextInput(f.comments, paragraph=True, placeholder="Add notes or comments for internal CCPO reference.") }} + +
+ + +
+
From c58851710029976eb905d3353266e713ebc74457 Mon Sep 17 00:00:00 2001 From: dandds Date: Mon, 10 Sep 2018 10:59:19 -0400 Subject: [PATCH 08/16] only show financial details on ccpo review page is request is awaiting final approval --- atst/routes/requests/approval.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/atst/routes/requests/approval.py b/atst/routes/requests/approval.py index f9385dbd..9bb09294 100644 --- a/atst/routes/requests/approval.py +++ b/atst/routes/requests/approval.py @@ -18,7 +18,8 @@ def task_order_dictionary(task_order): def render_approval(request, form=None): data = request.body - if request.task_order: + pending_final_approval = Requests.is_pending_ccpo_approval(request) + if pending_final_approval and request.task_order: data["task_order"] = task_order_dictionary(request.task_order) return render_template( @@ -26,7 +27,7 @@ def render_approval(request, form=None): data=data, request_id=request.id, status=request.status.value, - financial_review=True, + financial_review=pending_final_approval, pdf_available=request.task_order and request.task_order.pdf, f=form or CCPOReviewForm(), ) From 24cbb90ce2076060f33b65a413c9c9a40aa1ec85 Mon Sep 17 00:00:00 2001 From: dandds Date: Mon, 10 Sep 2018 11:17:47 -0400 Subject: [PATCH 09/16] ccpo can request changes on a request --- atst/domain/requests.py | 16 +++++++++++++--- atst/routes/requests/approval.py | 6 +++++- templates/requests/approval.html | 6 +++--- tests/domain/test_requests.py | 4 ++-- tests/routes/test_request_approval.py | 16 ++++++++++++++++ 5 files changed, 39 insertions(+), 9 deletions(-) diff --git a/atst/domain/requests.py b/atst/domain/requests.py index c34009e5..3eb3617f 100644 --- a/atst/domain/requests.py +++ b/atst/domain/requests.py @@ -268,12 +268,22 @@ WHERE requests_with_status.status = :status return request @classmethod - def approve_for_financial_verification(cls, request, review_data): - Requests.set_status(request, RequestStatus.PENDING_FINANCIAL_VERIFICATION) - + def _add_review(cls, request, review_data): request.latest_status.review = RequestReview(**review_data) db.session.add(request) db.session.commit() return request + + @classmethod + def accept_for_financial_verification(cls, request, review_data): + Requests.set_status(request, RequestStatus.PENDING_FINANCIAL_VERIFICATION) + + return Requests._add_review(request, review_data) + + @classmethod + def request_changes(cls, request, review_data): + Requests.set_status(request, RequestStatus.CHANGES_REQUESTED) + + return Requests._add_review(request, review_data) diff --git a/atst/routes/requests/approval.py b/atst/routes/requests/approval.py index 9bb09294..b78b0375 100644 --- a/atst/routes/requests/approval.py +++ b/atst/routes/requests/approval.py @@ -48,7 +48,11 @@ def submit_approval(request_id): form = CCPOReviewForm(http_request.form) if form.validate(): - Requests.approve_for_financial_verification(request, form.data) + if http_request.form.get("approved"): + Requests.accept_for_financial_verification(request, form.data) + else: + Requests.request_changes(request, form.data) + return redirect(url_for("requests.requests_index")) else: return render_approval(request, form) diff --git a/templates/requests/approval.html b/templates/requests/approval.html index 49a526d8..3135f0f5 100644 --- a/templates/requests/approval.html +++ b/templates/requests/approval.html @@ -117,9 +117,9 @@
- - Mark as Changes Requested - + + + {{ Icon('x') }} Cancel diff --git a/tests/domain/test_requests.py b/tests/domain/test_requests.py index 3d587cfa..50227ba1 100644 --- a/tests/domain/test_requests.py +++ b/tests/domain/test_requests.py @@ -181,10 +181,10 @@ def test_set_status_sets_revision(): assert request.latest_revision == request.status_events[-1].revision -def test_approve_for_financial_verification(): +def test_accept_for_financial_verification(): request = RequestFactory.create() review_data = RequestReviewFactory.dictionary() - Requests.approve_for_financial_verification(request, review_data) + Requests.accept_for_financial_verification(request, review_data) assert request.status == RequestStatus.PENDING_FINANCIAL_VERIFICATION current_review = request.latest_status.review assert current_review.fname_mao == review_data["fname_mao"] diff --git a/tests/routes/test_request_approval.py b/tests/routes/test_request_approval.py index 36881805..3a0a05c2 100644 --- a/tests/routes/test_request_approval.py +++ b/tests/routes/test_request_approval.py @@ -2,6 +2,7 @@ import os from flask import url_for from atst.models.attachment import Attachment +from atst.models.request_status_event import RequestStatus from atst.domain.roles import Roles from tests.factories import ( @@ -68,7 +69,22 @@ def test_can_submit_request_approval(client, user_session): user_session(user) request = RequestFactory.create() review_data = RequestReviewFactory.dictionary() + review_data["approved"] = True response = client.post( url_for("requests.submit_approval", request_id=request.id), data=review_data ) assert response.status_code == 302 + assert request.status == RequestStatus.PENDING_FINANCIAL_VERIFICATION + + +def test_can_submit_request_denial(client, user_session): + user = UserFactory.from_atat_role("ccpo") + user_session(user) + request = RequestFactory.create() + review_data = RequestReviewFactory.dictionary() + review_data["denied"] = True + response = client.post( + url_for("requests.submit_approval", request_id=request.id), data=review_data + ) + assert response.status_code == 302 + assert request.status == RequestStatus.CHANGES_REQUESTED From ebac93bc23af85e6c54c8a557db95829a3331fcd Mon Sep 17 00:00:00 2001 From: dandds Date: Mon, 10 Sep 2018 11:22:59 -0400 Subject: [PATCH 10/16] run reformatter --- atst/domain/requests.py | 26 +++++++++++++++----------- atst/forms/ccpo_review.py | 7 ++----- atst/routes/requests/approval.py | 9 ++++++++- atst/routes/requests/index.py | 4 +++- tests/domain/test_requests.py | 1 - tests/factories.py | 4 +++- tests/routes/test_request_approval.py | 5 ++++- tests/routes/test_requests_index.py | 10 ++++------ 8 files changed, 39 insertions(+), 27 deletions(-) diff --git a/atst/domain/requests.py b/atst/domain/requests.py index 3eb3617f..5c853630 100644 --- a/atst/domain/requests.py +++ b/atst/domain/requests.py @@ -76,9 +76,10 @@ class Requests(object): filters.append(Request.creator == creator) requests = ( - db.session.query(Request).filter(*filters).order_by( - Request.time_created.desc() - ).all() + db.session.query(Request) + .filter(*filters) + .order_by(Request.time_created.desc()) + .all() ) return requests @@ -118,9 +119,10 @@ class Requests(object): # Query for request matching id, acquiring a row-level write lock. # https://www.postgresql.org/docs/10/static/sql-select.html#SQL-FOR-UPDATE-SHARE return ( - db.session.query(Request).filter_by(id=request_id).with_for_update( - of=Request - ).one() + db.session.query(Request) + .filter_by(id=request_id) + .with_for_update(of=Request) + .one() ) except NoResultFound: @@ -154,13 +156,16 @@ class Requests(object): return dollar_value < cls.AUTO_APPROVE_THRESHOLD _VALID_SUBMISSION_STATUSES = [ - RequestStatus.STARTED, RequestStatus.CHANGES_REQUESTED + RequestStatus.STARTED, + RequestStatus.CHANGES_REQUESTED, ] @classmethod def should_allow_submission(cls, request): all_request_sections = [ - "details_of_use", "information_about_you", "primary_poc" + "details_of_use", + "information_about_you", + "primary_poc", ] existing_request_sections = request.body.keys() return request.status in Requests._VALID_SUBMISSION_STATUSES and all( @@ -241,9 +246,8 @@ WHERE requests_with_status.status = :status else: task_order_number = request_data.get("task_order_number") - if ( - "task_order" in request_data - and isinstance(request_data["task_order"], FileStorage) + if "task_order" in request_data and isinstance( + request_data["task_order"], FileStorage ): task_order_data["pdf"] = request_data.pop("task_order") diff --git a/atst/forms/ccpo_review.py b/atst/forms/ccpo_review.py index 89b5ed03..b1809ad6 100644 --- a/atst/forms/ccpo_review.py +++ b/atst/forms/ccpo_review.py @@ -7,15 +7,12 @@ from .validators import Alphabet, PhoneNumber class CCPOReviewForm(ValidatedForm): - comments = TextAreaField( - "Comments (optional)", - ) + comments = TextAreaField("Comments (optional)") fname_mao = StringField("First Name", validators=[Required(), Alphabet()]) lname_mao = StringField("Last Name", validators=[Required(), Alphabet()]) email_mao = EmailField("Mission Owner e-mail (optional)", validators=[Email()]) phone_mao = TelField( - "Mission Owner phone number (optional)", - validators=[PhoneNumber()], + "Mission Owner phone number (optional)", validators=[PhoneNumber()] ) fname_ccpo = StringField("First Name", validators=[Required(), Alphabet()]) lname_ccpo = StringField("Last Name", validators=[Required(), Alphabet()]) diff --git a/atst/routes/requests/approval.py b/atst/routes/requests/approval.py index b78b0375..66be2233 100644 --- a/atst/routes/requests/approval.py +++ b/atst/routes/requests/approval.py @@ -1,4 +1,11 @@ -from flask import render_template, g, Response, request as http_request, redirect, url_for +from flask import ( + render_template, + g, + Response, + request as http_request, + redirect, + url_for, +) from flask import current_app as app from . import requests_bp diff --git a/atst/routes/requests/index.py b/atst/routes/requests/index.py index 7032c12f..8553c0d6 100644 --- a/atst/routes/requests/index.py +++ b/atst/routes/requests/index.py @@ -76,7 +76,9 @@ class RequestsIndex(object): edit_link = url_for( "requests.financial_verification", request_id=request.id ) - elif Requests.is_pending_ccpo_acceptance(request) or Requests.is_pending_ccpo_approval(request): + elif Requests.is_pending_ccpo_acceptance( + request + ) or Requests.is_pending_ccpo_approval(request): edit_link = url_for("requests.view_pending_request", request_id=request.id) else: edit_link = url_for( diff --git a/tests/domain/test_requests.py b/tests/domain/test_requests.py index 50227ba1..78a66389 100644 --- a/tests/domain/test_requests.py +++ b/tests/domain/test_requests.py @@ -188,4 +188,3 @@ def test_accept_for_financial_verification(): assert request.status == RequestStatus.PENDING_FINANCIAL_VERIFICATION current_review = request.latest_status.review assert current_review.fname_mao == review_data["fname_mao"] - diff --git a/tests/factories.py b/tests/factories.py index fa1036c9..58c2861c 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -70,7 +70,9 @@ class RequestReviewFactory(Base): fname_mao = factory.Faker("first_name") lname_mao = factory.Faker("last_name") email_mao = factory.Faker("email") - phone_mao = factory.LazyFunction(lambda: "".join(random.choices(string.digits, k=10))) + phone_mao = factory.LazyFunction( + lambda: "".join(random.choices(string.digits, k=10)) + ) fname_ccpo = factory.Faker("first_name") lname_ccpo = factory.Faker("last_name") diff --git a/tests/routes/test_request_approval.py b/tests/routes/test_request_approval.py index 3a0a05c2..ced46240 100644 --- a/tests/routes/test_request_approval.py +++ b/tests/routes/test_request_approval.py @@ -6,7 +6,10 @@ from atst.models.request_status_event import RequestStatus from atst.domain.roles import Roles from tests.factories import ( - RequestFactory, TaskOrderFactory, UserFactory, RequestReviewFactory + RequestFactory, + TaskOrderFactory, + UserFactory, + RequestReviewFactory, ) diff --git a/tests/routes/test_requests_index.py b/tests/routes/test_requests_index.py index c8d6fa5d..29300b65 100644 --- a/tests/routes/test_requests_index.py +++ b/tests/routes/test_requests_index.py @@ -30,13 +30,11 @@ def test_ccpo_sees_approval_screen(): request = RequestFactory.create() Requests.submit(request) ccpo_context = RequestsIndex(ccpo).execute() - assert ( - ccpo_context["requests"][0]["edit_link"] - == url_for("requests.approval", request_id=request.id) + assert ccpo_context["requests"][0]["edit_link"] == url_for( + "requests.approval", request_id=request.id ) mo_context = RequestsIndex(request.creator).execute() - assert ( - mo_context["requests"][0]["edit_link"] - != url_for("requests.approval", request_id=request.id) + assert mo_context["requests"][0]["edit_link"] != url_for( + "requests.approval", request_id=request.id ) From 139103b2cead63cfe9eb642c40198af4889acbc2 Mon Sep 17 00:00:00 2001 From: dandds Date: Mon, 10 Sep 2018 12:01:30 -0400 Subject: [PATCH 11/16] all request review fields are nullable --- ...d26_request_reviews_non_nullable_fields.py | 50 ------------------- atst/forms/ccpo_review.py | 10 ++-- atst/models/request_review.py | 6 +-- 3 files changed, 8 insertions(+), 58 deletions(-) delete mode 100644 alembic/versions/bc6edccded26_request_reviews_non_nullable_fields.py diff --git a/alembic/versions/bc6edccded26_request_reviews_non_nullable_fields.py b/alembic/versions/bc6edccded26_request_reviews_non_nullable_fields.py deleted file mode 100644 index e143275a..00000000 --- a/alembic/versions/bc6edccded26_request_reviews_non_nullable_fields.py +++ /dev/null @@ -1,50 +0,0 @@ -"""request_reviews non-nullable fields - -Revision ID: bc6edccded26 -Revises: 7bdb2055d7c7 -Create Date: 2018-09-10 10:38:00.107297 - -""" -from alembic import op -import sqlalchemy as sa - - -# revision identifiers, used by Alembic. -revision = 'bc6edccded26' -down_revision = '7bdb2055d7c7' -branch_labels = None -depends_on = None - - -def upgrade(): - # ### commands auto generated by Alembic - please adjust! ### - op.alter_column('request_reviews', 'fname_ccpo', - existing_type=sa.VARCHAR(), - nullable=False) - op.alter_column('request_reviews', 'fname_mao', - existing_type=sa.VARCHAR(), - nullable=False) - op.alter_column('request_reviews', 'lname_ccpo', - existing_type=sa.VARCHAR(), - nullable=False) - op.alter_column('request_reviews', 'lname_mao', - existing_type=sa.VARCHAR(), - nullable=False) - # ### end Alembic commands ### - - -def downgrade(): - # ### commands auto generated by Alembic - please adjust! ### - op.alter_column('request_reviews', 'lname_mao', - existing_type=sa.VARCHAR(), - nullable=True) - op.alter_column('request_reviews', 'lname_ccpo', - existing_type=sa.VARCHAR(), - nullable=True) - op.alter_column('request_reviews', 'fname_mao', - existing_type=sa.VARCHAR(), - nullable=True) - op.alter_column('request_reviews', 'fname_ccpo', - existing_type=sa.VARCHAR(), - nullable=True) - # ### end Alembic commands ### diff --git a/atst/forms/ccpo_review.py b/atst/forms/ccpo_review.py index b1809ad6..ba3116d0 100644 --- a/atst/forms/ccpo_review.py +++ b/atst/forms/ccpo_review.py @@ -1,6 +1,6 @@ from wtforms.fields.html5 import EmailField, TelField from wtforms.fields import StringField, TextAreaField -from wtforms.validators import Required, Email +from wtforms.validators import Email from .forms import ValidatedForm from .validators import Alphabet, PhoneNumber @@ -8,11 +8,11 @@ from .validators import Alphabet, PhoneNumber class CCPOReviewForm(ValidatedForm): comments = TextAreaField("Comments (optional)") - fname_mao = StringField("First Name", validators=[Required(), Alphabet()]) - lname_mao = StringField("Last Name", validators=[Required(), Alphabet()]) + fname_mao = StringField("First Name (optional)", validators=[Alphabet()]) + lname_mao = StringField("Last Name (optional)", validators=[Alphabet()]) email_mao = EmailField("Mission Owner e-mail (optional)", validators=[Email()]) phone_mao = TelField( "Mission Owner phone number (optional)", validators=[PhoneNumber()] ) - fname_ccpo = StringField("First Name", validators=[Required(), Alphabet()]) - lname_ccpo = StringField("Last Name", validators=[Required(), Alphabet()]) + fname_ccpo = StringField("First Name (optional)", validators=[Alphabet()]) + lname_ccpo = StringField("Last Name (optional)", validators=[Alphabet()]) diff --git a/atst/models/request_review.py b/atst/models/request_review.py index a17367ff..ac0ba88f 100644 --- a/atst/models/request_review.py +++ b/atst/models/request_review.py @@ -11,9 +11,9 @@ class RequestReview(Base): status = relationship("RequestStatusEvent", back_populates="review") comments = Column(String) - fname_mao = Column(String, nullable=False) + fname_mao = Column(String) lname_mao = Column(String, nullable=False) email_mao = Column(String) phone_mao = Column(String) - fname_ccpo = Column(String, nullable=False) - lname_ccpo = Column(String, nullable=False) + fname_ccpo = Column(String) + lname_ccpo = Column(String) From d3c98523c4386d07ee28efdaa360920d9a88893f Mon Sep 17 00:00:00 2001 From: dandds Date: Mon, 10 Sep 2018 12:18:30 -0400 Subject: [PATCH 12/16] ccpo should only see review form on review screen if request is pending CCPO action --- atst/routes/requests/approval.py | 4 + templates/requests/approval.html | 133 ++++++++++++++++--------------- 2 files changed, 72 insertions(+), 65 deletions(-) diff --git a/atst/routes/requests/approval.py b/atst/routes/requests/approval.py index 66be2233..8c21493d 100644 --- a/atst/routes/requests/approval.py +++ b/atst/routes/requests/approval.py @@ -26,6 +26,9 @@ def task_order_dictionary(task_order): def render_approval(request, form=None): data = request.body pending_final_approval = Requests.is_pending_ccpo_approval(request) + pending_review = ( + Requests.is_pending_ccpo_acceptance(request) or pending_final_approval + ) if pending_final_approval and request.task_order: data["task_order"] = task_order_dictionary(request.task_order) @@ -34,6 +37,7 @@ def render_approval(request, form=None): data=data, request_id=request.id, status=request.status.value, + pending_review=pending_review, financial_review=pending_final_approval, pdf_available=request.task_order and request.task_order.pdf, f=form or CCPOReviewForm(), diff --git a/templates/requests/approval.html b/templates/requests/approval.html index 3135f0f5..cb9df6fc 100644 --- a/templates/requests/approval.html +++ b/templates/requests/approval.html @@ -26,104 +26,107 @@
-
-
-

Approval Notes

-
+ {% if pending_review %} +
+
+

Approval Notes

+
-
+
-
+
-

Instructions for the Requestor

+

Instructions for the Requestor

- Provide instructions or notes for additional information that is necessary to approve the request here. The requestor may then re-submit the updated request or initiate contact outside of AT-AT if further discussion is required. These notes will be visible to the person making the JEDI request. + Provide instructions or notes for additional information that is necessary to approve the request here. The requestor may then re-submit the updated request or initiate contact outside of AT-AT if further discussion is required. These notes will be visible to the person making the JEDI request. - {{ TextInput(f.comments, paragraph=True, placeholder="Add notes or comments explaining what changes are being requested or why further discussion is needed about this request.") }} - -
- -
-
-

Authorizing Officials

-

This section is not visible to the person making the request. It is only viewable by CCPO staff.

-

Provide the name of the key officials for both parties that have authorized this request to move forward.

+ {{ TextInput(f.comments, paragraph=True, placeholder="Add notes or comments explaining what changes are being requested or why further discussion is needed about this request.") }}
-
-

Mission Authorizing Official

-
+
+
+

Authorizing Officials

+

This section is not visible to the person making the request. It is only viewable by CCPO staff.

+

Provide the name of the key officials for both parties that have authorized this request to move forward.

+ +
+
+

Mission Authorizing Official

+ + +
+ +
+ {{ TextInput(f.fname_mao, placeholder="First name of mission authorizing official") }} +
+ +
+ {{ TextInput(f.lname_mao, placeholder="Last name of mission authorizing official") }} +
-
- {{ TextInput(f.fname_mao, placeholder="First name of mission authorizing official") }}
-
- {{ TextInput(f.lname_mao, placeholder="Last name of mission authorizing official") }} -
+
-
+
+ {{ TextInput(f.email_mao, placeholder="name@mail.mil") }} +
-
-
- {{ TextInput(f.email_mao, placeholder="name@mail.mil") }} +
+ {{ TextInput(f.phone_mao, placeholder="(123) 456-7890", validation='usPhone') }} +
+
-
- {{ TextInput(f.phone_mao, placeholder="(123) 456-7890", validation='usPhone') }} +

CCPO Authorizing Official

+ +
+ +
+ {{ TextInput(f.fname_ccpo, placeholder="First name of CCPO authorizing official") }} +
+ +
+ {{ TextInput(f.lname_ccpo, placeholder="Last name of CCPO authorizing official") }} +
+
-
-

CCPO Authorizing Official

+

CCPO Internal Notes

-
+

You may add additional comments and notes for internal CCPO reference and follow-up here.

-
- {{ TextInput(f.fname_ccpo, placeholder="First name of CCPO authorizing official") }} -
+
-
- {{ TextInput(f.lname_ccpo, placeholder="Last name of CCPO authorizing official") }} -
+
-
+
+ + +
- - -

CCPO Internal Notes

- -

You may add additional comments and notes for internal CCPO reference and follow-up here.

- -
- -
- -
- -
-
+
-
- -
- - - - {{ Icon('x') }} - Cancel - -
+
+ + + + {{ Icon('x') }} + Cancel + +
+ {% endif %}
From aadedfc3b04c8efcdb39677b7945dc5795e2f262 Mon Sep 17 00:00:00 2001 From: dandds Date: Mon, 10 Sep 2018 13:33:10 -0400 Subject: [PATCH 13/16] small schema refinements for request_reviews --- ...34b_request_reviews_comment_column_name.py | 30 ++++++++++++++++ ...7ded5c57a0_bigint_for_request_review_id.py | 36 +++++++++++++++++++ atst/forms/ccpo_review.py | 2 +- atst/models/request_review.py | 8 ++--- templates/requests/approval.html | 2 +- tests/factories.py | 2 +- 6 files changed, 73 insertions(+), 7 deletions(-) create mode 100644 alembic/versions/53ab3edd334b_request_reviews_comment_column_name.py create mode 100644 alembic/versions/777ded5c57a0_bigint_for_request_review_id.py diff --git a/alembic/versions/53ab3edd334b_request_reviews_comment_column_name.py b/alembic/versions/53ab3edd334b_request_reviews_comment_column_name.py new file mode 100644 index 00000000..35b52d1d --- /dev/null +++ b/alembic/versions/53ab3edd334b_request_reviews_comment_column_name.py @@ -0,0 +1,30 @@ +"""request_reviews comment column name + +Revision ID: 53ab3edd334b +Revises: 777ded5c57a0 +Create Date: 2018-09-10 13:29:02.648359 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = '53ab3edd334b' +down_revision = '777ded5c57a0' +branch_labels = None +depends_on = None + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.add_column('request_reviews', sa.Column('comment', sa.String(), nullable=True)) + op.drop_column('request_reviews', 'comments') + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.add_column('request_reviews', sa.Column('comments', sa.VARCHAR(), autoincrement=False, nullable=True)) + op.drop_column('request_reviews', 'comment') + # ### end Alembic commands ### diff --git a/alembic/versions/777ded5c57a0_bigint_for_request_review_id.py b/alembic/versions/777ded5c57a0_bigint_for_request_review_id.py new file mode 100644 index 00000000..c76ca8e4 --- /dev/null +++ b/alembic/versions/777ded5c57a0_bigint_for_request_review_id.py @@ -0,0 +1,36 @@ +"""bigint for request review id + +Revision ID: 777ded5c57a0 +Revises: 7bdb2055d7c7 +Create Date: 2018-09-10 13:24:36.328610 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = '777ded5c57a0' +down_revision = '7bdb2055d7c7' +branch_labels = None +depends_on = None + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.alter_column('request_reviews', 'id', + existing_type=sa.Integer(), + type_=sa.BigInteger(), + nullable=False) + pass + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.alter_column('request_reviews', 'id', + existing_type=sa.BigInteger(), + type_=sa.Integer(), + nullable=False) + pass + # ### end Alembic commands ### diff --git a/atst/forms/ccpo_review.py b/atst/forms/ccpo_review.py index ba3116d0..1742ca6d 100644 --- a/atst/forms/ccpo_review.py +++ b/atst/forms/ccpo_review.py @@ -7,7 +7,7 @@ from .validators import Alphabet, PhoneNumber class CCPOReviewForm(ValidatedForm): - comments = TextAreaField("Comments (optional)") + comment = TextAreaField("Comments (optional)") fname_mao = StringField("First Name (optional)", validators=[Alphabet()]) lname_mao = StringField("Last Name (optional)", validators=[Alphabet()]) email_mao = EmailField("Mission Owner e-mail (optional)", validators=[Email()]) diff --git a/atst/models/request_review.py b/atst/models/request_review.py index ac0ba88f..58a6b121 100644 --- a/atst/models/request_review.py +++ b/atst/models/request_review.py @@ -1,4 +1,4 @@ -from sqlalchemy import Column, Integer, String +from sqlalchemy import Column, BigInteger, String from sqlalchemy.orm import relationship from atst.models import Base @@ -7,12 +7,12 @@ from atst.models import Base class RequestReview(Base): __tablename__ = "request_reviews" - id = Column(Integer, primary_key=True) + id = Column(BigInteger, primary_key=True) status = relationship("RequestStatusEvent", back_populates="review") - comments = Column(String) + comment = Column(String) fname_mao = Column(String) - lname_mao = Column(String, nullable=False) + lname_mao = Column(String) email_mao = Column(String) phone_mao = Column(String) fname_ccpo = Column(String) diff --git a/templates/requests/approval.html b/templates/requests/approval.html index cb9df6fc..deaf8504 100644 --- a/templates/requests/approval.html +++ b/templates/requests/approval.html @@ -40,7 +40,7 @@ Provide instructions or notes for additional information that is necessary to approve the request here. The requestor may then re-submit the updated request or initiate contact outside of AT-AT if further discussion is required. These notes will be visible to the person making the JEDI request. - {{ TextInput(f.comments, paragraph=True, placeholder="Add notes or comments explaining what changes are being requested or why further discussion is needed about this request.") }} + {{ TextInput(f.comment, paragraph=True, placeholder="Add notes or comments explaining what changes are being requested or why further discussion is needed about this request.") }} diff --git a/tests/factories.py b/tests/factories.py index 58c2861c..ad8cb048 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -66,7 +66,7 @@ class RequestReviewFactory(Base): class Meta: model = RequestReview - comments = factory.Faker("sentence") + comment = factory.Faker("sentence") fname_mao = factory.Faker("first_name") lname_mao = factory.Faker("last_name") email_mao = factory.Faker("email") From ac51931d8949896b1a4cd50f88cdb26cc4cb028c Mon Sep 17 00:00:00 2001 From: richard-dds Date: Mon, 10 Sep 2018 14:20:47 -0400 Subject: [PATCH 14/16] Flag request for MO action required when status is CHANGES_REQUESTED --- atst/models/request.py | 1 + 1 file changed, 1 insertion(+) diff --git a/atst/models/request.py b/atst/models/request.py index c991a708..05f52610 100644 --- a/atst/models/request.py +++ b/atst/models/request.py @@ -156,6 +156,7 @@ class Request(Base): def action_required_by(self): return { RequestStatus.PENDING_FINANCIAL_VERIFICATION: "mission_owner", + RequestStatus.CHANGES_REQUESTED: "mission_owner", RequestStatus.PENDING_CCPO_APPROVAL: "ccpo", RequestStatus.PENDING_CCPO_ACCEPTANCE: "ccpo", }.get(self.status) From 041cfe878ffef18ae959bab853e8aecdbd054409 Mon Sep 17 00:00:00 2001 From: dandds Date: Mon, 10 Sep 2018 14:38:04 -0400 Subject: [PATCH 15/16] ensure ccpo review fields are optional, add form error alert to review screen --- atst/forms/ccpo_review.py | 24 ++++++++++++++++-------- templates/requests/approval.html | 7 +++++++ 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/atst/forms/ccpo_review.py b/atst/forms/ccpo_review.py index 1742ca6d..8b6ea64d 100644 --- a/atst/forms/ccpo_review.py +++ b/atst/forms/ccpo_review.py @@ -1,6 +1,6 @@ from wtforms.fields.html5 import EmailField, TelField from wtforms.fields import StringField, TextAreaField -from wtforms.validators import Email +from wtforms.validators import Email, Optional from .forms import ValidatedForm from .validators import Alphabet, PhoneNumber @@ -8,11 +8,19 @@ from .validators import Alphabet, PhoneNumber class CCPOReviewForm(ValidatedForm): comment = TextAreaField("Comments (optional)") - fname_mao = StringField("First Name (optional)", validators=[Alphabet()]) - lname_mao = StringField("Last Name (optional)", validators=[Alphabet()]) - email_mao = EmailField("Mission Owner e-mail (optional)", validators=[Email()]) - phone_mao = TelField( - "Mission Owner phone number (optional)", validators=[PhoneNumber()] + fname_mao = StringField( + "First Name (optional)", validators=[Optional(), Alphabet()] + ) + lname_mao = StringField("Last Name (optional)", validators=[Optional(), Alphabet()]) + email_mao = EmailField( + "Mission Owner e-mail (optional)", validators=[Optional(), Email()] + ) + phone_mao = TelField( + "Mission Owner phone number (optional)", validators=[Optional(), PhoneNumber()] + ) + fname_ccpo = StringField( + "First Name (optional)", validators=[Optional(), Alphabet()] + ) + lname_ccpo = StringField( + "Last Name (optional)", validators=[Optional(), Alphabet()] ) - fname_ccpo = StringField("First Name (optional)", validators=[Alphabet()]) - lname_ccpo = StringField("Last Name (optional)", validators=[Alphabet()]) diff --git a/templates/requests/approval.html b/templates/requests/approval.html index deaf8504..8cf4ec2c 100644 --- a/templates/requests/approval.html +++ b/templates/requests/approval.html @@ -8,6 +8,13 @@
+{% if f.errors %} + {{ Alert('There were some errors', + message="

Please see below.

", + level='error' + ) }} +{% endif %} + {{ f.csrf_token }}
From d38261f4392d1d63461a723ae7fc54fd1270d76d Mon Sep 17 00:00:00 2001 From: dandds Date: Mon, 10 Sep 2018 16:42:42 -0400 Subject: [PATCH 16/16] associate ccpo user to request review --- .../7bdb2055d7c7_add_request_review_table.py | 4 ++++ atst/domain/requests.py | 12 ++++++------ atst/models/request_review.py | 5 ++++- atst/routes/requests/approval.py | 6 ++++-- tests/domain/test_requests.py | 4 +++- tests/models/test_requests.py | 7 +++++-- 6 files changed, 26 insertions(+), 12 deletions(-) diff --git a/alembic/versions/7bdb2055d7c7_add_request_review_table.py b/alembic/versions/7bdb2055d7c7_add_request_review_table.py index 99274e85..ba669921 100644 --- a/alembic/versions/7bdb2055d7c7_add_request_review_table.py +++ b/alembic/versions/7bdb2055d7c7_add_request_review_table.py @@ -7,6 +7,7 @@ Create Date: 2018-09-06 15:15:40.666840 """ from alembic import op import sqlalchemy as sa +from sqlalchemy.dialects import postgresql # revision identifiers, used by Alembic. @@ -27,16 +28,19 @@ def upgrade(): sa.Column('phone_mao', sa.String(), nullable=True), sa.Column('fname_ccpo', sa.String(), nullable=True), sa.Column('lname_ccpo', sa.String(), nullable=True), + sa.Column('user_id', postgresql.UUID(as_uuid=True), nullable=False), sa.PrimaryKeyConstraint('id') ) op.add_column('request_status_events', sa.Column('request_review_id', sa.Integer(), nullable=True)) op.create_foreign_key(None, 'request_status_events', 'request_reviews', ['request_review_id'], ['id']) + op.create_foreign_key(None, 'request_reviews', 'users', ['user_id'], ['id']) # ### end Alembic commands ### def downgrade(): # ### commands auto generated by Alembic - please adjust! ### op.drop_constraint(None, 'request_status_events', type_='foreignkey') + op.drop_constraint(None, 'request_reviews', type_='foreignkey') op.drop_column('request_status_events', 'request_review_id') op.drop_table('request_reviews') # ### end Alembic commands ### diff --git a/atst/domain/requests.py b/atst/domain/requests.py index 5c853630..f87a8382 100644 --- a/atst/domain/requests.py +++ b/atst/domain/requests.py @@ -272,8 +272,8 @@ WHERE requests_with_status.status = :status return request @classmethod - def _add_review(cls, request, review_data): - request.latest_status.review = RequestReview(**review_data) + def _add_review(cls, user, request, review_data): + request.latest_status.review = RequestReview(reviewer=user, **review_data) db.session.add(request) db.session.commit() @@ -281,13 +281,13 @@ WHERE requests_with_status.status = :status return request @classmethod - def accept_for_financial_verification(cls, request, review_data): + def accept_for_financial_verification(cls, user, request, review_data): Requests.set_status(request, RequestStatus.PENDING_FINANCIAL_VERIFICATION) - return Requests._add_review(request, review_data) + return Requests._add_review(user, request, review_data) @classmethod - def request_changes(cls, request, review_data): + def request_changes(cls, user, request, review_data): Requests.set_status(request, RequestStatus.CHANGES_REQUESTED) - return Requests._add_review(request, review_data) + return Requests._add_review(user, request, review_data) diff --git a/atst/models/request_review.py b/atst/models/request_review.py index 58a6b121..3c991ea5 100644 --- a/atst/models/request_review.py +++ b/atst/models/request_review.py @@ -1,4 +1,4 @@ -from sqlalchemy import Column, BigInteger, String +from sqlalchemy import Column, BigInteger, String, ForeignKey from sqlalchemy.orm import relationship from atst.models import Base @@ -10,6 +10,9 @@ class RequestReview(Base): id = Column(BigInteger, primary_key=True) status = relationship("RequestStatusEvent", back_populates="review") + user_id = Column(ForeignKey("users.id"), nullable=False) + reviewer = relationship("User") + comment = Column(String) fname_mao = Column(String) lname_mao = Column(String) diff --git a/atst/routes/requests/approval.py b/atst/routes/requests/approval.py index 8c21493d..ca02933f 100644 --- a/atst/routes/requests/approval.py +++ b/atst/routes/requests/approval.py @@ -60,9 +60,11 @@ def submit_approval(request_id): form = CCPOReviewForm(http_request.form) if form.validate(): if http_request.form.get("approved"): - Requests.accept_for_financial_verification(request, form.data) + Requests.accept_for_financial_verification( + g.current_user, request, form.data + ) else: - Requests.request_changes(request, form.data) + Requests.request_changes(g.current_user, request, form.data) return redirect(url_for("requests.requests_index")) else: diff --git a/tests/domain/test_requests.py b/tests/domain/test_requests.py index 78a66389..34045076 100644 --- a/tests/domain/test_requests.py +++ b/tests/domain/test_requests.py @@ -184,7 +184,9 @@ def test_set_status_sets_revision(): def test_accept_for_financial_verification(): request = RequestFactory.create() review_data = RequestReviewFactory.dictionary() - Requests.accept_for_financial_verification(request, review_data) + Requests.accept_for_financial_verification( + UserFactory.create(), request, review_data + ) assert request.status == RequestStatus.PENDING_FINANCIAL_VERIFICATION current_review = request.latest_status.review assert current_review.fname_mao == review_data["fname_mao"] diff --git a/tests/models/test_requests.py b/tests/models/test_requests.py index 1b16d7db..a5bbef93 100644 --- a/tests/models/test_requests.py +++ b/tests/models/test_requests.py @@ -78,12 +78,15 @@ def test_annual_spend(): def test_reviews(): request = RequestFactory.create() + ccpo = UserFactory.from_atat_role("ccpo") request.status_events = [ RequestStatusEventFactory.create( - revision=request.latest_revision, review=RequestReviewFactory.create() + revision=request.latest_revision, + review=RequestReviewFactory.create(reviewer=ccpo), ), RequestStatusEventFactory.create( - revision=request.latest_revision, review=RequestReviewFactory.create() + revision=request.latest_revision, + review=RequestReviewFactory.create(reviewer=ccpo), ), RequestStatusEventFactory.create(revision=request.latest_revision), ]