From 2b07491c471c252bee1ead163c4b2cfa3c0ebab0 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Mon, 12 Nov 2018 10:56:54 -0500 Subject: [PATCH 01/11] Don't validate anything when saving a draft --- atst/forms/financial.py | 21 +++++----- .../routes/requests/financial_verification.py | 15 +------ tests/routes/test_financial_verification.py | 39 +++++-------------- 3 files changed, 21 insertions(+), 54 deletions(-) diff --git a/atst/forms/financial.py b/atst/forms/financial.py index 15ddb955..6bcabab7 100644 --- a/atst/forms/financial.py +++ b/atst/forms/financial.py @@ -32,19 +32,20 @@ def coerce_choice(val): class DraftValidateMixin(object): def validate_draft(self): - """ - Make all fields optional before validation, and then return them to - their previous state. - """ - for field in self: - field.validators.insert(0, Optional()) + return True + # """ + # Make all fields optional before validation, and then return them to + # their previous state. + # """ + # for field in self: + # field.validators.insert(0, Optional()) - valid = self.validate() + # valid = self.validate() - for field in self: - field.validators.pop(0) + # for field in self: + # field.validators.pop(0) - return valid + # return valid class TaskOrderForm(ValidatedForm, DraftValidateMixin): diff --git a/atst/routes/requests/financial_verification.py b/atst/routes/requests/financial_verification.py index cb2e1bb5..b1ed930c 100644 --- a/atst/routes/requests/financial_verification.py +++ b/atst/routes/requests/financial_verification.py @@ -168,29 +168,16 @@ class SaveFinancialVerificationDraft(FinancialVerificationBase): def execute(self): form = self._get_form(self.request, self.is_extended, self.fv_data) - valid = True - if not form.validate_draft(): self._raise(form) - if form.pe_id.data and not self.pe_validator.validate(self.request, form.pe_id): - valid = False - - if form.task_order.number.data and not self.task_order_validator.validate( - form.task_order.number - ): - valid = False - attachment = self._process_attachment(self.is_extended, form) task_order = self._try_create_task_order(form, attachment, self.is_extended) updated_request = Requests.update_financial_verification( self.request.id, form.request.data, task_order=task_order ) - if valid: - return updated_request - else: - self._raise(form) + return updated_request @requests_bp.route("/requests/verify/", methods=["GET"]) diff --git a/tests/routes/test_financial_verification.py b/tests/routes/test_financial_verification.py index 8c1f3da4..94274551 100644 --- a/tests/routes/test_financial_verification.py +++ b/tests/routes/test_financial_verification.py @@ -193,39 +193,18 @@ def test_save_draft_with_ba_code(): assert save_draft.execute() -def test_save_draft_with_invalid_task_order(fv_data): +def test_save_draft_allows_invalid_data(): request = RequestFactory.create() user = UserFactory.create() - save_draft = SaveFinancialVerificationDraft( - TrueValidator, FalseValidator, user, request, fv_data, is_extended=False - ) + data = { + "task_order-number": MANUAL_TO_NUMBER, + "request-pe_id": "123", + "request-ba_code": "a" + } - with pytest.raises(FormValidationError): - assert save_draft.execute() - - -def test_save_draft_with_invalid_pe_number(fv_data): - request = RequestFactory.create() - user = UserFactory.create() - save_draft = SaveFinancialVerificationDraft( - FalseValidator, TrueValidator, user, request, fv_data, is_extended=False - ) - - with pytest.raises(FormValidationError): - assert save_draft.execute() - - -def test_save_draft_re_enter_pe_number(fv_data): - request = RequestFactory.create() - user = UserFactory.create() - data = {**fv_data, "pe_id": "0101228M"} - save_fv = SaveFinancialVerificationDraft( - PENumberValidator(), TrueValidator, user, request, data, is_extended=False - ) - - with pytest.raises(FormValidationError): - save_fv.execute() - save_fv.execute() + assert SaveFinancialVerificationDraft( + PENumberValidator(), TaskOrderNumberValidator(), user, request, data, is_extended=True + ).execute() def test_save_draft_and_then_submit(): From f2914509625445187edbd058e7b48710c131aa45 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Mon, 12 Nov 2018 11:17:47 -0500 Subject: [PATCH 02/11] Don't redirect after saving draft --- .../routes/requests/financial_verification.py | 19 +++++++++++++---- .../requests/financial_verification.html | 5 +++++ tests/routes/test_financial_verification.py | 21 +++++++------------ 3 files changed, 27 insertions(+), 18 deletions(-) diff --git a/atst/routes/requests/financial_verification.py b/atst/routes/requests/financial_verification.py index b1ed930c..dd0ead1c 100644 --- a/atst/routes/requests/financial_verification.py +++ b/atst/routes/requests/financial_verification.py @@ -240,15 +240,16 @@ def update_financial_verification(request_id): @requests_bp.route("/requests/verify//draft", methods=["POST"]) def save_financial_verification_draft(request_id): - request = Requests.get(g.current_user, request_id) + user = g.current_user + request = Requests.get(user, request_id) fv_data = {**http_request.form, **http_request.files} is_extended = fv_extended(http_request) try: - SaveFinancialVerificationDraft( + updated_request = SaveFinancialVerificationDraft( PENumberValidator(), TaskOrderNumberValidator(), - g.current_user, + user, request, fv_data, is_extended=is_extended, @@ -261,4 +262,14 @@ def save_financial_verification_draft(request_id): extended=is_extended, ) - return redirect(url_for("requests.requests_index")) + form = GetFinancialVerificationForm( + user, updated_request, is_extended=is_extended + ).execute() + return render_template( + "requests/financial_verification.html", + f=form, + jedi_request=request, + review_comment=request.review_comment, + extended=is_extended, + saved_draft=True, + ) diff --git a/templates/requests/financial_verification.html b/templates/requests/financial_verification.html index 827ff9f5..32234099 100644 --- a/templates/requests/financial_verification.html +++ b/templates/requests/financial_verification.html @@ -42,6 +42,11 @@ {% endcall %} {% endif %} + {% if saved_draft %} + {% call Alert('Draft saved', level='info') %} + {% endcall %} + {% endif %} +
{{ f.csrf_token }} diff --git a/tests/routes/test_financial_verification.py b/tests/routes/test_financial_verification.py index 94274551..18afa0de 100644 --- a/tests/routes/test_financial_verification.py +++ b/tests/routes/test_financial_verification.py @@ -17,7 +17,6 @@ from atst.domain.requests.financial_verification import ( PENumberValidator, TaskOrderNumberValidator, ) -from atst.utils import pick from atst.models.request_status_event import RequestStatus from atst.domain.requests.query import RequestsQuery @@ -199,11 +198,16 @@ def test_save_draft_allows_invalid_data(): data = { "task_order-number": MANUAL_TO_NUMBER, "request-pe_id": "123", - "request-ba_code": "a" + "request-ba_code": "a", } assert SaveFinancialVerificationDraft( - PENumberValidator(), TaskOrderNumberValidator(), user, request, data, is_extended=True + PENumberValidator(), + TaskOrderNumberValidator(), + user, + request, + data, + is_extended=True, ).execute() @@ -267,17 +271,6 @@ def test_update_ignores_empty_values(fv_data, e_fv_data): ).execute() -def test_simple_form_does_not_generate_task_order(fv_data): - request = RequestFactory.create() - user = UserFactory.create() - data = pick(["uii_ids"], fv_data) - updated_request = SaveFinancialVerificationDraft( - TrueValidator, TrueValidator, user, request, data, is_extended=False - ).execute() - - assert updated_request.task_order is None - - def test_can_save_draft_with_funding_type(fv_data, e_fv_data): request = RequestFactory.create() user = UserFactory.create() From f67215d1425b3f79cb99db4f592bf30dd4ec7631 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Mon, 12 Nov 2018 11:48:14 -0500 Subject: [PATCH 03/11] Remove unused draft validation code --- atst/forms/financial.py | 25 ++----------------- .../routes/requests/financial_verification.py | 3 --- 2 files changed, 2 insertions(+), 26 deletions(-) diff --git a/atst/forms/financial.py b/atst/forms/financial.py index 6bcabab7..a2166cb7 100644 --- a/atst/forms/financial.py +++ b/atst/forms/financial.py @@ -30,25 +30,7 @@ def coerce_choice(val): return val.value -class DraftValidateMixin(object): - def validate_draft(self): - return True - # """ - # Make all fields optional before validation, and then return them to - # their previous state. - # """ - # for field in self: - # field.validators.insert(0, Optional()) - - # valid = self.validate() - - # for field in self: - # field.validators.pop(0) - - # return valid - - -class TaskOrderForm(ValidatedForm, DraftValidateMixin): +class TaskOrderForm(ValidatedForm): def do_validate_number(self): for field in self: if field.name != "task_order-number": @@ -144,7 +126,7 @@ class TaskOrderForm(ValidatedForm, DraftValidateMixin): ) -class RequestFinancialVerificationForm(ValidatedForm, DraftValidateMixin): +class RequestFinancialVerificationForm(ValidatedForm): uii_ids = NewlineListField( "Unique Item Identifier (UII)s related to your application(s) if you already have them.", description="If you have more than one UII, place each one on a new line.", @@ -224,9 +206,6 @@ class FinancialVerificationForm(ValidatedForm): task_order_valid = self.task_order.do_validate_number() return request_valid and task_order_valid - def validate_draft(self): - return self.task_order.validate_draft() and self.request.validate_draft() - def reset(self): self.request.reset() diff --git a/atst/routes/requests/financial_verification.py b/atst/routes/requests/financial_verification.py index dd0ead1c..fa49af83 100644 --- a/atst/routes/requests/financial_verification.py +++ b/atst/routes/requests/financial_verification.py @@ -168,9 +168,6 @@ class SaveFinancialVerificationDraft(FinancialVerificationBase): def execute(self): form = self._get_form(self.request, self.is_extended, self.fv_data) - if not form.validate_draft(): - self._raise(form) - attachment = self._process_attachment(self.is_extended, form) task_order = self._try_create_task_order(form, attachment, self.is_extended) updated_request = Requests.update_financial_verification( From 9e4cec9a6a456ab892c9767ba3535e23ee40c9d9 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Mon, 12 Nov 2018 11:48:30 -0500 Subject: [PATCH 04/11] Show feedback when draft is saved --- js/components/forms/financial.js | 2 ++ templates/requests/financial_verification.html | 5 ++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/js/components/forms/financial.js b/js/components/forms/financial.js index 7c270081..c3adeaad 100644 --- a/js/components/forms/financial.js +++ b/js/components/forms/financial.js @@ -1,6 +1,7 @@ import FormMixin from '../../mixins/form' import optionsinput from '../options_input' import textinput from '../text_input' +import localdatetime from '../local_datetime' export default { name: 'financial', @@ -10,6 +11,7 @@ export default { components: { optionsinput, textinput, + localdatetime, }, props: { diff --git a/templates/requests/financial_verification.html b/templates/requests/financial_verification.html index 32234099..dbaad79d 100644 --- a/templates/requests/financial_verification.html +++ b/templates/requests/financial_verification.html @@ -43,7 +43,7 @@ {% endif %} {% if saved_draft %} - {% call Alert('Draft saved', level='info') %} + {% call Alert('Draft saved', level='success') %} {% endcall %} {% endif %} @@ -185,6 +185,9 @@ {% endblock form %} + {% if saved_draft %} + Draft saved at + {% endif %} {% block next %}
From b5fbeb5ca16ede24b25a1bddef0289120c75ae24 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Mon, 12 Nov 2018 16:51:53 -0500 Subject: [PATCH 05/11] Move "save draft" alert to top of the stack --- templates/requests/financial_verification.html | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/templates/requests/financial_verification.html b/templates/requests/financial_verification.html index dbaad79d..a9d8289e 100644 --- a/templates/requests/financial_verification.html +++ b/templates/requests/financial_verification.html @@ -9,6 +9,12 @@ {% include 'requests/review_menu.html' %} +{% if saved_draft %} + {% call Alert('Draft saved', level='success') %} + {% endcall %} +{% endif %} + + {% if jedi_request.is_pending_financial_verification and not f.errors and not extended %} {{ Alert('Pending Financial Verification', fragment="fragments/pending_financial_verification.html") }} {% endif %} @@ -19,7 +25,6 @@
- {% if extended %} {{ Alert('Manually enter Task Order information', message="Additional fields are displayed below, where you can manually enter financial information as documented in your Task Order.", @@ -42,11 +47,6 @@ {% endcall %} {% endif %} - {% if saved_draft %} - {% call Alert('Draft saved', level='success') %} - {% endcall %} - {% endif %} - {{ f.csrf_token }} From 873dc71d534c508635097550c09036647638913c Mon Sep 17 00:00:00 2001 From: richard-dds Date: Mon, 12 Nov 2018 17:00:38 -0500 Subject: [PATCH 06/11] Redirect back to GET route after saving draft --- .../routes/requests/financial_verification.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/atst/routes/requests/financial_verification.py b/atst/routes/requests/financial_verification.py index fa49af83..961ab9fa 100644 --- a/atst/routes/requests/financial_verification.py +++ b/atst/routes/requests/financial_verification.py @@ -181,6 +181,7 @@ class SaveFinancialVerificationDraft(FinancialVerificationBase): def financial_verification(request_id): request = Requests.get(g.current_user, request_id) is_extended = fv_extended(http_request) + saved_draft = http_request.args.get("saved_draft", False) should_be_extended = not is_extended and request.has_manual_task_order if should_be_extended: @@ -198,6 +199,7 @@ def financial_verification(request_id): jedi_request=request, review_comment=request.review_comment, extended=is_extended, + saved_draft=saved_draft ) @@ -259,14 +261,11 @@ def save_financial_verification_draft(request_id): extended=is_extended, ) - form = GetFinancialVerificationForm( - user, updated_request, is_extended=is_extended - ).execute() - return render_template( - "requests/financial_verification.html", - f=form, - jedi_request=request, - review_comment=request.review_comment, - extended=is_extended, - saved_draft=True, + return redirect( + url_for( + "requests.financial_verification", + request_id=updated_request.id, + is_extended=is_extended, + saved_draft=True, + ) ) From 53eb1dd5a2084f63ffc9d40b1e1a01ee53089944 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Mon, 12 Nov 2018 17:05:04 -0500 Subject: [PATCH 07/11] Formatting --- atst/routes/requests/financial_verification.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/atst/routes/requests/financial_verification.py b/atst/routes/requests/financial_verification.py index 961ab9fa..367d2ce3 100644 --- a/atst/routes/requests/financial_verification.py +++ b/atst/routes/requests/financial_verification.py @@ -199,7 +199,7 @@ def financial_verification(request_id): jedi_request=request, review_comment=request.review_comment, extended=is_extended, - saved_draft=saved_draft + saved_draft=saved_draft, ) From 480a88aff781ae3c30f34e2be4d99832ff8f6ced Mon Sep 17 00:00:00 2001 From: richard-dds Date: Mon, 12 Nov 2018 17:10:26 -0500 Subject: [PATCH 08/11] Fix test (we're redirecting now) --- tests/routes/test_financial_verification.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/routes/test_financial_verification.py b/tests/routes/test_financial_verification.py index 18afa0de..41c9ca47 100644 --- a/tests/routes/test_financial_verification.py +++ b/tests/routes/test_financial_verification.py @@ -305,7 +305,7 @@ def test_save_fv_draft_route(client, user_session, fv_data): response = client.post( url_for("requests.save_financial_verification_draft", request_id=request.id), data=fv_data, - follow_redirects=False, + follow_redirects=True, ) assert response.status_code == 200 From 587a0da0001dc2d592bea11a4437e55a2a8e0166 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Tue, 13 Nov 2018 10:45:00 -0500 Subject: [PATCH 09/11] More accurate "last saved at" timestamp --- atst/models/request.py | 7 +++++++ atst/models/request_revision.py | 21 +++++++++++++++++++ .../requests/financial_verification.html | 2 +- tests/models/test_requests.py | 7 +++++++ 4 files changed, 36 insertions(+), 1 deletion(-) diff --git a/atst/models/request.py b/atst/models/request.py index 101c1969..2a4cef41 100644 --- a/atst/models/request.py +++ b/atst/models/request.py @@ -238,6 +238,13 @@ class Request(Base, mixins.TimestampsMixin, mixins.AuditableMixin): else None ) + @property + def last_finver_draft_saved_at(self): + if self.latest_revision.any_finver_fields_saved: + return self.latest_revision.time_updated + else: + return None + def __repr__(self): return "".format( self.status_displayname, diff --git a/atst/models/request_revision.py b/atst/models/request_revision.py index 89c4987a..17ba19c5 100644 --- a/atst/models/request_revision.py +++ b/atst/models/request_revision.py @@ -83,3 +83,24 @@ class RequestRevision(Base, mixins.TimestampsMixin, mixins.AuditableMixin): return "".format( self.request_id, self.id ) + + @property + def any_finver_fields_saved(self): + return any( + getattr(self, n, None) + for n in [ + "pe_id", + "task_order_number", + "fname_co", + "lname_co", + "email_co", + "office_co", + "fname_cor", + "lname_cor", + "email_cor", + "office_cor", + "uii_ids", + "treasury_code", + "ba_code", + ] + ) diff --git a/templates/requests/financial_verification.html b/templates/requests/financial_verification.html index a9d8289e..c266c328 100644 --- a/templates/requests/financial_verification.html +++ b/templates/requests/financial_verification.html @@ -186,7 +186,7 @@ {% endblock form %} {% if saved_draft %} - Draft saved at + Draft saved at {% endif %} {% block next %}
diff --git a/tests/models/test_requests.py b/tests/models/test_requests.py index 3d5bcf72..f2a82528 100644 --- a/tests/models/test_requests.py +++ b/tests/models/test_requests.py @@ -3,6 +3,7 @@ from tests.factories import ( UserFactory, RequestStatusEventFactory, RequestReviewFactory, + RequestRevisionFactory, ) from atst.domain.requests import Requests from atst.models.request_status_event import RequestStatus @@ -113,3 +114,9 @@ def test_review_comment(): ) assert not request.review_comment + + +def test_finver_last_saved_at(): + request = RequestFactory.create() + RequestRevisionFactory.create(fname_co="Amanda", request=request) + assert request.last_finver_draft_saved_at From 56b6bdaab4d79e2a62e4a51d880e5c4be2462409 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Tue, 13 Nov 2018 17:18:51 -0500 Subject: [PATCH 10/11] Always show draft timestamp if there is one --- templates/requests/financial_verification.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/requests/financial_verification.html b/templates/requests/financial_verification.html index c266c328..0d62dd37 100644 --- a/templates/requests/financial_verification.html +++ b/templates/requests/financial_verification.html @@ -185,7 +185,7 @@
{% endblock form %} - {% if saved_draft %} + {% if jedi_request.last_finver_draft_saved_at %} Draft saved at {% endif %} {% block next %} From 4229c81dc6bca812b69c30a6137ab1aea7308a8b Mon Sep 17 00:00:00 2001 From: richard-dds Date: Tue, 13 Nov 2018 17:21:03 -0500 Subject: [PATCH 11/11] Italicize "draft saved at" text, move in line with buttons --- templates/requests/financial_verification.html | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/templates/requests/financial_verification.html b/templates/requests/financial_verification.html index 0d62dd37..c8d06002 100644 --- a/templates/requests/financial_verification.html +++ b/templates/requests/financial_verification.html @@ -185,13 +185,13 @@
{% endblock form %} - {% if jedi_request.last_finver_draft_saved_at %} - Draft saved at - {% endif %} {% block next %}
+ {% if jedi_request.last_finver_draft_saved_at %} + Draft saved at + {% endif %}
{% endblock %}