From 85252812e08cd5db4da2e3711eaa476c63f52211 Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Tue, 10 Dec 2019 16:26:00 -0500 Subject: [PATCH 1/6] Update number column on task orders table to have a unique constraint --- ...57_add_unique_constraint_to_task_order_.py | 28 +++++++++++++++++++ atst/models/task_order.py | 2 +- 2 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 alembic/versions/3bd8552f1c57_add_unique_constraint_to_task_order_.py diff --git a/alembic/versions/3bd8552f1c57_add_unique_constraint_to_task_order_.py b/alembic/versions/3bd8552f1c57_add_unique_constraint_to_task_order_.py new file mode 100644 index 00000000..01794a93 --- /dev/null +++ b/alembic/versions/3bd8552f1c57_add_unique_constraint_to_task_order_.py @@ -0,0 +1,28 @@ +"""add unique constraint to task order number + +Revision ID: 3bd8552f1c57 +Revises: 67a2151d6269 +Create Date: 2019-12-10 12:45:17.535973 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +# revision identifiers, used by Alembic. +revision = '3bd8552f1c57' # pragma: allowlist secret +down_revision = '67a2151d6269' # pragma: allowlist secret +branch_labels = None +depends_on = None + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.create_unique_constraint(None, 'task_orders', ['number']) + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_constraint(None, 'task_orders', type_='unique') + # ### end Alembic commands ### diff --git a/atst/models/task_order.py b/atst/models/task_order.py index 1a46e505..eba34147 100644 --- a/atst/models/task_order.py +++ b/atst/models/task_order.py @@ -39,7 +39,7 @@ class TaskOrder(Base, mixins.TimestampsMixin): pdf_attachment_id = Column(ForeignKey("attachments.id")) _pdf = relationship("Attachment", foreign_keys=[pdf_attachment_id]) - number = Column(String) # Task Order Number + number = Column(String, unique=True,) # Task Order Number signer_dod_id = Column(String) signed_at = Column(DateTime) From 6446b4fbd0781cdc0387fd0af14c726935ba530c Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Wed, 11 Dec 2019 11:41:47 -0500 Subject: [PATCH 2/6] Raise AlreadyExistsError if a task order is created or updated with a number of an existing task order --- atst/domain/task_orders.py | 17 +++++++++++++---- tests/domain/test_task_orders.py | 16 ++++++++++++++++ 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/atst/domain/task_orders.py b/atst/domain/task_orders.py index 24acdee7..ce4ba77b 100644 --- a/atst/domain/task_orders.py +++ b/atst/domain/task_orders.py @@ -1,9 +1,11 @@ import datetime +from sqlalchemy.exc import IntegrityError from atst.database import db from atst.models.clin import CLIN from atst.models.task_order import TaskOrder, SORT_ORDERING from . import BaseDomainClass +from .exceptions import AlreadyExistsError class TaskOrders(BaseDomainClass): @@ -12,9 +14,12 @@ class TaskOrders(BaseDomainClass): @classmethod def create(cls, portfolio_id, number, clins, pdf): - task_order = TaskOrder(portfolio_id=portfolio_id, number=number, pdf=pdf) - db.session.add(task_order) - db.session.commit() + try: + task_order = TaskOrder(portfolio_id=portfolio_id, number=number, pdf=pdf) + db.session.add(task_order) + db.session.commit() + except IntegrityError: + raise AlreadyExistsError("task_order") TaskOrders.create_clins(task_order.id, clins) @@ -35,7 +40,11 @@ class TaskOrders(BaseDomainClass): task_order.number = number db.session.add(task_order) - db.session.commit() + try: + db.session.commit() + except IntegrityError: + raise AlreadyExistsError("task_order") + return task_order @classmethod diff --git a/tests/domain/test_task_orders.py b/tests/domain/test_task_orders.py index 7bc4cf41..49b9bae6 100644 --- a/tests/domain/test_task_orders.py +++ b/tests/domain/test_task_orders.py @@ -2,6 +2,7 @@ import pytest from datetime import date, timedelta from decimal import Decimal +from atst.domain.exceptions import AlreadyExistsError from atst.domain.task_orders import TaskOrders from atst.models import Attachment from atst.models.task_order import TaskOrder, SORT_ORDERING, Status @@ -154,3 +155,18 @@ def test_task_order_sort_by_status(): assert len(sorted_by_status["Expired"]) == 2 assert len(sorted_by_status["Unsigned"]) == 1 assert list(sorted_by_status.keys()) == [status.value for status in SORT_ORDERING] + + +def test_create_enforces_unique_number(): + portfolio = PortfolioFactory.create() + number = "1234567890123" + assert TaskOrders.create(portfolio.id, number, [], None) + with pytest.raises(AlreadyExistsError): + TaskOrders.create(portfolio.id, number, [], None) + + +def test_update_enforces_unique_number(): + task_order = TaskOrderFactory.create() + dupe_task_order = TaskOrderFactory.create() + with pytest.raises(AlreadyExistsError): + TaskOrders.update(dupe_task_order.id, task_order.number, [], None) From 9caef2883a32a085a7b43a6340e63a6dabf6962a Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Thu, 12 Dec 2019 11:58:59 -0500 Subject: [PATCH 3/6] migration fix --- .../3bd8552f1c57_add_unique_constraint_to_task_order_.py | 4 ++-- atst/domain/task_orders.py | 7 +++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/alembic/versions/3bd8552f1c57_add_unique_constraint_to_task_order_.py b/alembic/versions/3bd8552f1c57_add_unique_constraint_to_task_order_.py index 01794a93..24a0fcfa 100644 --- a/alembic/versions/3bd8552f1c57_add_unique_constraint_to_task_order_.py +++ b/alembic/versions/3bd8552f1c57_add_unique_constraint_to_task_order_.py @@ -18,11 +18,11 @@ depends_on = None def upgrade(): # ### commands auto generated by Alembic - please adjust! ### - op.create_unique_constraint(None, 'task_orders', ['number']) + op.create_unique_constraint('task_orders_number_key', 'task_orders', ['number']) # ### end Alembic commands ### def downgrade(): # ### commands auto generated by Alembic - please adjust! ### - op.drop_constraint(None, 'task_orders', type_='unique') + op.drop_constraint('task_orders_number_key', 'task_orders', type_='unique') # ### end Alembic commands ### diff --git a/atst/domain/task_orders.py b/atst/domain/task_orders.py index ce4ba77b..835590c9 100644 --- a/atst/domain/task_orders.py +++ b/atst/domain/task_orders.py @@ -14,11 +14,13 @@ class TaskOrders(BaseDomainClass): @classmethod def create(cls, portfolio_id, number, clins, pdf): + task_order = TaskOrder(portfolio_id=portfolio_id, number=number, pdf=pdf) + db.session.add(task_order) + try: - task_order = TaskOrder(portfolio_id=portfolio_id, number=number, pdf=pdf) - db.session.add(task_order) db.session.commit() except IntegrityError: + db.session.rollback() raise AlreadyExistsError("task_order") TaskOrders.create_clins(task_order.id, clins) @@ -43,6 +45,7 @@ class TaskOrders(BaseDomainClass): try: db.session.commit() except IntegrityError: + db.session.rollback() raise AlreadyExistsError("task_order") return task_order From 78ef47f649751607769ae89c1b80223433b73ebd Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Thu, 12 Dec 2019 14:21:30 -0500 Subject: [PATCH 4/6] Update TO route helper function to catch error and display flash message when a user tries to save a TO with an existing number. Update TaskOrderForm so that it converts empty string for number into None, this was causing an issue where new TOs were being saved with an empty string for the number, which violated the unique constraint. --- atst/forms/task_order.py | 8 +++++ atst/routes/task_orders/new.py | 47 +++++++++++++++++----------- atst/utils/flash.py | 5 +++ tests/forms/test_task_order.py | 8 ++++- tests/routes/task_orders/test_new.py | 20 ++++++++++++ translations.yaml | 2 ++ 6 files changed, 70 insertions(+), 20 deletions(-) diff --git a/atst/forms/task_order.py b/atst/forms/task_order.py index 2a1b8ad2..41ebeb81 100644 --- a/atst/forms/task_order.py +++ b/atst/forms/task_order.py @@ -142,6 +142,14 @@ class TaskOrderForm(BaseForm): ) clins = FieldList(FormField(CLINForm)) + @property + def data(self): + _data = super().data + if _data["number"] == "": + _data["number"] = None + + return _data + class SignatureForm(BaseForm): signature = BooleanField( diff --git a/atst/routes/task_orders/new.py b/atst/routes/task_orders/new.py index dca751e3..f6e53b75 100644 --- a/atst/routes/task_orders/new.py +++ b/atst/routes/task_orders/new.py @@ -10,7 +10,7 @@ from flask import ( from .blueprint import task_orders_bp from atst.domain.authz.decorator import user_can_access_decorator as user_can -from atst.domain.exceptions import NoAccessError +from atst.domain.exceptions import NoAccessError, AlreadyExistsError from atst.domain.task_orders import TaskOrders from atst.forms.task_order import TaskOrderForm, SignatureForm from atst.models.permissions import Permissions @@ -50,7 +50,26 @@ def render_task_orders_edit( return render_template(template, **render_args) -def update_task_order( +def update_task_order(form, portfolio_id=None, task_order_id=None, flash_invalid=True): + if form.validate(flash_invalid=flash_invalid): + task_order = None + try: + if task_order_id: + task_order = TaskOrders.update(task_order_id, **form.data) + portfolio_id = task_order.portfolio_id + else: + task_order = TaskOrders.create(portfolio_id, **form.data) + + return task_order + + except AlreadyExistsError: + flash("task_order_number_error", to_number=form.data["number"]) + return False + else: + return False + + +def update_and_render_next( form_data, next_page, current_template, portfolio_id=None, task_order_id=None ): form = None @@ -60,14 +79,8 @@ def update_task_order( else: form = TaskOrderForm(form_data) - if form.validate(): - task_order = None - if task_order_id: - task_order = TaskOrders.update(task_order_id, **form.data) - portfolio_id = task_order.portfolio_id - else: - task_order = TaskOrders.create(portfolio_id, **form.data) - + task_order = update_task_order(form, portfolio_id, task_order_id) + if task_order: return redirect(url_for(next_page, task_order_id=task_order.id)) else: return ( @@ -149,7 +162,7 @@ def submit_form_step_one_add_pdf(portfolio_id=None, task_order_id=None): next_page = "task_orders.form_step_two_add_number" current_template = "task_orders/step_1.html" - return update_task_order( + return update_and_render_next( form_data, next_page, current_template, @@ -176,12 +189,8 @@ def cancel_edit(task_order_id=None, portfolio_id=None): else: form = TaskOrderForm(form_data) - if form.validate(flash_invalid=False): - task_order = None - if task_order_id: - task_order = TaskOrders.update(task_order_id, **form.data) - else: - task_order = TaskOrders.create(portfolio_id, **form.data) + update_task_order(form, portfolio_id, task_order_id, flash_invalid=False) + elif not save and task_order_id: TaskOrders.delete(task_order_id) @@ -205,7 +214,7 @@ def submit_form_step_two_add_number(task_order_id): next_page = "task_orders.form_step_three_add_clins" current_template = "task_orders/step_2.html" - return update_task_order( + return update_and_render_next( form_data, next_page, current_template, task_order_id=task_order_id ) @@ -225,7 +234,7 @@ def submit_form_step_three_add_clins(task_order_id): next_page = "task_orders.form_step_four_review" current_template = "task_orders/step_3.html" - return update_task_order( + return update_and_render_next( form_data, next_page, current_template, task_order_id=task_order_id ) diff --git a/atst/utils/flash.py b/atst/utils/flash.py index 87b543b8..fabba4d2 100644 --- a/atst/utils/flash.py +++ b/atst/utils/flash.py @@ -165,6 +165,11 @@ MESSAGES = { "message_template": translate("task_orders.form.draft_alert_message"), "category": "warning", }, + "task_order_number_error": { + "title_template": "", + "message_template": """{{ 'flash.task_order_number_error.message' | translate({ 'to_number': to_number }) }}""", + "category": "error", + }, "task_order_submitted": { "title_template": "Your Task Order has been uploaded successfully.", "message_template": """ diff --git a/tests/forms/test_task_order.py b/tests/forms/test_task_order.py index de95e555..97759c81 100644 --- a/tests/forms/test_task_order.py +++ b/tests/forms/test_task_order.py @@ -2,7 +2,7 @@ import datetime from dateutil.relativedelta import relativedelta from flask import current_app as app -from atst.forms.task_order import CLINForm +from atst.forms.task_order import CLINForm, TaskOrderForm from atst.models import JEDICLINType from atst.utils.localization import translate @@ -106,3 +106,9 @@ def test_clin_form_dollar_amounts_out_of_range(): assert ( translate("forms.task_order.clin_funding_errors.funding_range_error") ) in invalid_clin_form.obligated_amount.errors + + +def test_no_number(): + http_request_form_data = {} + form = TaskOrderForm(http_request_form_data) + assert form.data["number"] is None diff --git a/tests/routes/task_orders/test_new.py b/tests/routes/task_orders/test_new.py index 1e8e16eb..a7f5a991 100644 --- a/tests/routes/task_orders/test_new.py +++ b/tests/routes/task_orders/test_new.py @@ -170,6 +170,26 @@ def test_task_orders_submit_form_step_two_add_number(client, user_session, task_ assert task_order.number == "1234567890" +def test_task_orders_submit_form_step_two_enforces_unique_number( + client, user_session, task_order, session +): + number = "1234567890123" + dupe_task_order = TaskOrderFactory.create(number=number) + portfolio = task_order.portfolio + user_session(task_order.portfolio.owner) + form_data = {"number": number} + session.begin_nested() + response = client.post( + url_for( + "task_orders.submit_form_step_two_add_number", task_order_id=task_order.id + ), + data=form_data, + ) + session.rollback() + + assert response.status_code == 400 + + def test_task_orders_submit_form_step_two_add_number_existing_to( client, user_session, task_order ): diff --git a/translations.yaml b/translations.yaml index 1fa09b83..1d876e97 100644 --- a/translations.yaml +++ b/translations.yaml @@ -123,6 +123,8 @@ flash: new_ppoc_message: 'You have successfully added {ppoc_name} as the primary point of contact. You are no longer the PPoC.' new_ppoc_title: Primary point of contact updated success: Success! + task_order_number_error: + message: 'The TO number has already been entered for a JEDI task order #{to_number}. Please double-check the TO number you are entering. If you believe this is in error, please contact support@cloud.mil.' new_application_member: title: "{user_name}'s invitation has been sent" message: "{user_name}'s access to this Application is pending until they sign in for the first time." From f0505ee1228eb5fb80f358276eb42e331a157811 Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Fri, 13 Dec 2019 11:57:26 -0500 Subject: [PATCH 5/6] Remove unused imports Use remove_empty_string filter for TaskOrderForm.number instead of declaring the data property Update display of TOs without a number --- ...2f1c57_add_unique_constraint_to_task_order_.py | 2 -- atst/forms/task_order.py | 15 +++++---------- templates/task_orders/index.html | 2 +- 3 files changed, 6 insertions(+), 13 deletions(-) diff --git a/alembic/versions/3bd8552f1c57_add_unique_constraint_to_task_order_.py b/alembic/versions/3bd8552f1c57_add_unique_constraint_to_task_order_.py index 24a0fcfa..8128a40a 100644 --- a/alembic/versions/3bd8552f1c57_add_unique_constraint_to_task_order_.py +++ b/alembic/versions/3bd8552f1c57_add_unique_constraint_to_task_order_.py @@ -6,8 +6,6 @@ Create Date: 2019-12-10 12:45:17.535973 """ from alembic import op -import sqlalchemy as sa -from sqlalchemy.dialects import postgresql # revision identifiers, used by Alembic. revision = '3bd8552f1c57' # pragma: allowlist secret diff --git a/atst/forms/task_order.py b/atst/forms/task_order.py index 41ebeb81..a5e02e8b 100644 --- a/atst/forms/task_order.py +++ b/atst/forms/task_order.py @@ -13,7 +13,7 @@ from numbers import Number from .data import JEDI_CLIN_TYPES from .fields import SelectField -from .forms import BaseForm +from .forms import BaseForm, remove_empty_string from atst.utils.localization import translate from flask import current_app as app @@ -134,7 +134,10 @@ class AttachmentForm(BaseForm): class TaskOrderForm(BaseForm): - number = StringField(label=translate("forms.task_order.number_description")) + number = StringField( + label=translate("forms.task_order.number_description"), + filters=[remove_empty_string], + ) pdf = FormField( AttachmentForm, label=translate("task_orders.form.supporting_docs_size_limit"), @@ -142,14 +145,6 @@ class TaskOrderForm(BaseForm): ) clins = FieldList(FormField(CLINForm)) - @property - def data(self): - _data = super().data - if _data["number"] == "": - _data["number"] = None - - return _data - class SignatureForm(BaseForm): signature = BooleanField( diff --git a/templates/task_orders/index.html b/templates/task_orders/index.html index b9277123..160fb483 100644 --- a/templates/task_orders/index.html +++ b/templates/task_orders/index.html @@ -19,7 +19,7 @@ {% if task_orders|length > 0 %} {% for task_order in task_orders %} {% set to_number %} - {% if task_order.number != "" %} + {% if task_order.number != None %} Task Order #{{ task_order.number }} {% else %} New Task Order From e38358992d8fa7f35820baf170d4b01856a4e1f3 Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Fri, 13 Dec 2019 14:56:31 -0500 Subject: [PATCH 6/6] Fix migration chain --- .../3bd8552f1c57_add_unique_constraint_to_task_order_.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/alembic/versions/3bd8552f1c57_add_unique_constraint_to_task_order_.py b/alembic/versions/3bd8552f1c57_add_unique_constraint_to_task_order_.py index 8128a40a..f1375626 100644 --- a/alembic/versions/3bd8552f1c57_add_unique_constraint_to_task_order_.py +++ b/alembic/versions/3bd8552f1c57_add_unique_constraint_to_task_order_.py @@ -1,7 +1,7 @@ """add unique constraint to task order number Revision ID: 3bd8552f1c57 -Revises: 67a2151d6269 +Revises: 802071bcd013 Create Date: 2019-12-10 12:45:17.535973 """ @@ -9,7 +9,7 @@ from alembic import op # revision identifiers, used by Alembic. revision = '3bd8552f1c57' # pragma: allowlist secret -down_revision = '67a2151d6269' # pragma: allowlist secret +down_revision = '802071bcd013' # pragma: allowlist secret branch_labels = None depends_on = None