diff --git a/.secrets.baseline b/.secrets.baseline index 05921baa..24361804 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -3,7 +3,7 @@ "files": "^.secrets.baseline$|^.*pgsslrootcert.yml$", "lines": null }, - "generated_at": "2019-12-06T21:22:07Z", + "generated_at": "2019-12-13T20:38:57Z", "plugins_used": [ { "base64_limit": 4.5, @@ -170,7 +170,7 @@ "hashed_secret": "e4f14805dfd1e6af030359090c535e149e6b4207", "is_secret": false, "is_verified": false, - "line_number": 656, + "line_number": 659, "type": "Hex High Entropy String" } ] diff --git a/alembic/versions/c487d91f1a26_add_application_name_and_portfolio_id_.py b/alembic/versions/c487d91f1a26_add_application_name_and_portfolio_id_.py new file mode 100644 index 00000000..b70b4fde --- /dev/null +++ b/alembic/versions/c487d91f1a26_add_application_name_and_portfolio_id_.py @@ -0,0 +1,27 @@ +"""add application name and portfolio_id unique constraint + +Revision ID: c487d91f1a26 +Revises: 3bd8552f1c57 +Create Date: 2019-12-13 14:33:23.952450 + +""" +from alembic import op + + +# revision identifiers, used by Alembic. +revision = 'c487d91f1a26' # pragma: allowlist secret +down_revision = '3bd8552f1c57' # pragma: allowlist secret +branch_labels = None +depends_on = None + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.create_unique_constraint('applications_name_portfolio_id_key', 'applications', ['name', 'portfolio_id']) + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_constraint('applications_name_portfolio_id_key', 'applications', type_='unique') + # ### end Alembic commands ### diff --git a/atst/domain/applications.py b/atst/domain/applications.py index 21e8782b..c2321194 100644 --- a/atst/domain/applications.py +++ b/atst/domain/applications.py @@ -11,7 +11,7 @@ from atst.models import ( ApplicationRoleStatus, EnvironmentRole, ) -from atst.utils import first_or_none +from atst.utils import first_or_none, update_or_raise_already_exists_error class Applications(BaseDomainClass): @@ -28,7 +28,7 @@ class Applications(BaseDomainClass): if environment_names: Environments.create_many(user, application, environment_names) - db.session.commit() + update_or_raise_already_exists_error(message="application") return application @classmethod @@ -53,9 +53,9 @@ class Applications(BaseDomainClass): Environments.create_many( g.current_user, application, new_data["environment_names"] ) - db.session.add(application) - db.session.commit() + db.session.add(application) + update_or_raise_already_exists_error(message="application") return application @classmethod diff --git a/atst/domain/task_orders.py b/atst/domain/task_orders.py index 835590c9..8ad8b0f4 100644 --- a/atst/domain/task_orders.py +++ b/atst/domain/task_orders.py @@ -1,11 +1,10 @@ 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 +from atst.utils import update_or_raise_already_exists_error class TaskOrders(BaseDomainClass): @@ -16,15 +15,8 @@ class TaskOrders(BaseDomainClass): 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: - db.session.commit() - except IntegrityError: - db.session.rollback() - raise AlreadyExistsError("task_order") - + update_or_raise_already_exists_error(message="task_order") TaskOrders.create_clins(task_order.id, clins) - return task_order @classmethod @@ -42,12 +34,7 @@ class TaskOrders(BaseDomainClass): task_order.number = number db.session.add(task_order) - try: - db.session.commit() - except IntegrityError: - db.session.rollback() - raise AlreadyExistsError("task_order") - + update_or_raise_already_exists_error(message="task_order") return task_order @classmethod diff --git a/atst/models/application.py b/atst/models/application.py index b6cff6bd..a7bdadba 100644 --- a/atst/models/application.py +++ b/atst/models/application.py @@ -1,4 +1,4 @@ -from sqlalchemy import and_, Column, ForeignKey, String +from sqlalchemy import and_, Column, ForeignKey, String, UniqueConstraint from sqlalchemy.orm import relationship, synonym from atst.models.base import Base @@ -34,6 +34,11 @@ class Application( ), ) members = synonym("roles") + __table_args__ = ( + UniqueConstraint( + "name", "portfolio_id", name="applications_name_portfolio_id_key" + ), + ) @property def users(self): diff --git a/atst/routes/applications/new.py b/atst/routes/applications/new.py index a1a1a54c..e9238775 100644 --- a/atst/routes/applications/new.py +++ b/atst/routes/applications/new.py @@ -2,6 +2,7 @@ from flask import redirect, render_template, request as http_request, url_for, g from .blueprint import applications_bp from atst.domain.applications import Applications +from atst.domain.exceptions import AlreadyExistsError from atst.domain.portfolios import Portfolios from atst.forms.application import NameAndDescriptionForm, EnvironmentsForm from atst.domain.authz.decorator import user_can_access_decorator as user_can @@ -37,6 +38,31 @@ def render_new_application_form( return render_template(template, **render_args) +def update_application(form, application_id=None, portfolio_id=None): + if form.validate(): + application = None + try: + if application_id: + application = Applications.get(application_id) + application = Applications.update(application, form.data) + flash("application_updated", application_name=application.name) + else: + portfolio = Portfolios.get_for_update(portfolio_id) + application = Applications.create( + g.current_user, portfolio, **form.data + ) + flash("application_created", application_name=application.name) + + return application + + except AlreadyExistsError: + flash("application_name_error", name=form.data["name"]) + return False + + else: + return False + + @applications_bp.route("/portfolios//applications/new") @applications_bp.route("/applications//new/step_1") @user_can(Permissions.CREATE_APPLICATION, message="view create new application form") @@ -64,17 +90,9 @@ def create_or_update_new_application_step_1(portfolio_id=None, application_id=No form = get_new_application_form( {**http_request.form}, NameAndDescriptionForm, application_id ) + application = update_application(form, application_id, portfolio_id) - if form.validate(): - application = None - if application_id: - application = Applications.get(application_id) - application = Applications.update(application, form.data) - flash("application_updated", application_name=application.name) - else: - portfolio = Portfolios.get_for_update(portfolio_id) - application = Applications.create(g.current_user, portfolio, **form.data) - flash("application_created", application_name=application.name) + if application: return redirect( url_for( "applications.update_new_application_step_2", diff --git a/atst/utils/__init__.py b/atst/utils/__init__.py index 01988e10..9772af67 100644 --- a/atst/utils/__init__.py +++ b/atst/utils/__init__.py @@ -1,5 +1,10 @@ import re +from sqlalchemy.exc import IntegrityError + +from atst.database import db +from atst.domain.exceptions import AlreadyExistsError + def first_or_none(predicate, lst): return next((x for x in lst if predicate(x)), None) @@ -23,3 +28,11 @@ def camel_to_snake(camel_cased): def pick(keys, dct): _keys = set(keys) return {k: v for (k, v) in dct.items() if k in _keys} + + +def update_or_raise_already_exists_error(message): + try: + db.session.commit() + except IntegrityError: + db.session.rollback() + raise AlreadyExistsError(message) diff --git a/atst/utils/flash.py b/atst/utils/flash.py index fabba4d2..0de143bb 100644 --- a/atst/utils/flash.py +++ b/atst/utils/flash.py @@ -64,6 +64,11 @@ MESSAGES = { "message_template": "You have successfully updated the permissions for {{ user_name }}", "category": "success", }, + "application_name_error": { + "title_template": "", + "message_template": """{{ 'flash.application.name_error.message' | translate({ 'name': name }) }}""", + "category": "error", + }, "ccpo_user_added": { "title_template": translate("flash.success"), "message_template": "You have successfully given {{ user_name }} CCPO permissions.", diff --git a/tests/domain/test_applications.py b/tests/domain/test_applications.py index 66f4ee01..50ae8038 100644 --- a/tests/domain/test_applications.py +++ b/tests/domain/test_applications.py @@ -5,7 +5,7 @@ from atst.models import CSPRole, ApplicationRoleStatus from atst.domain.application_roles import ApplicationRoles from atst.domain.applications import Applications from atst.domain.environment_roles import EnvironmentRoles -from atst.domain.exceptions import NotFoundError +from atst.domain.exceptions import AlreadyExistsError, NotFoundError from atst.domain.permission_sets import PermissionSets from tests.factories import ( @@ -177,3 +177,22 @@ def test_invite_to_nonexistent_environment(): {"environment_id": uuid4(), "role": CSPRole.BASIC_ACCESS.value}, ], ) + + +def test_create_does_not_duplicate_names_within_portfolio(): + portfolio = PortfolioFactory.create() + name = "An Awesome Application" + + assert Applications.create(portfolio.owner, portfolio, name, "") + with pytest.raises(AlreadyExistsError): + Applications.create(portfolio.owner, portfolio, name, "") + + +def test_update_does_not_duplicate_names_within_portfolio(): + portfolio = PortfolioFactory.create() + name = "An Awesome Application" + application = ApplicationFactory.create(portfolio=portfolio, name=name) + dupe_application = ApplicationFactory.create(portfolio=portfolio) + + with pytest.raises(AlreadyExistsError): + Applications.update(dupe_application, {"name": name}) diff --git a/tests/domain/test_portfolios.py b/tests/domain/test_portfolios.py index 41e3fc81..0390f69f 100644 --- a/tests/domain/test_portfolios.py +++ b/tests/domain/test_portfolios.py @@ -1,4 +1,5 @@ import pytest +import random from uuid import uuid4 from atst.domain.exceptions import NotFoundError, UnauthorizedError @@ -97,7 +98,7 @@ def test_scoped_portfolio_returns_all_applications_for_portfolio_admin( Applications.create( portfolio.owner, portfolio, - "My Application", + "My Application %s" % (random.randrange(1, 1000)), "My application", ["dev", "staging", "prod"], ) @@ -120,7 +121,7 @@ def test_scoped_portfolio_returns_all_applications_for_portfolio_owner( Applications.create( portfolio.owner, portfolio, - "My Application", + "My Application %s" % (random.randrange(1, 1000)), "My application", ["dev", "staging", "prod"], ) diff --git a/tests/routes/applications/test_new.py b/tests/routes/applications/test_new.py index c4fb83ed..045ac19e 100644 --- a/tests/routes/applications/test_new.py +++ b/tests/routes/applications/test_new.py @@ -70,6 +70,24 @@ def test_post_name_and_description_for_update(client, session, user_session): assert application.description == "This is only a test" +def test_post_name_and_description_enforces_unique_name(client, user_session, session): + portfolio = PortfolioFactory.create() + name = "Test Application" + application = ApplicationFactory.create(portfolio=portfolio, name=name) + user_session(portfolio.owner) + + session.begin_nested() + response = client.post( + url_for( + "applications.create_new_application_step_1", portfolio_id=portfolio.id + ), + data={"name": name, "description": "This is only a test"}, + ) + session.rollback() + + assert response.status_code == 400 + + def test_get_environments(client, user_session): application = ApplicationFactory.create() user_session(application.portfolio.owner) diff --git a/tests/test_access.py b/tests/test_access.py index 19aaf749..ad4bd5be 100644 --- a/tests/test_access.py +++ b/tests/test_access.py @@ -1,6 +1,7 @@ from unittest.mock import Mock import pytest +import random from flask import url_for, Response @@ -264,26 +265,28 @@ def test_applications_post_application_step_1(post_url_assert_status): rando = user_with() portfolio = PortfolioFactory.create(owner=owner) application = ApplicationFactory.create(portfolio=portfolio) - step_1_form_data = { - "name": "Test Application", - "description": "This is only a test", - } + + def _form_data(): + return { + "name": "Test Application %s" % (random.randrange(1, 1000)), + "description": "This is only a test", + } url = url_for( "applications.create_new_application_step_1", portfolio_id=portfolio.id ) - post_url_assert_status(ccpo, url, 302, data=step_1_form_data) - post_url_assert_status(owner, url, 302, data=step_1_form_data) - post_url_assert_status(rando, url, 404, data=step_1_form_data) + post_url_assert_status(ccpo, url, 302, data=_form_data()) + post_url_assert_status(owner, url, 302, data=_form_data()) + post_url_assert_status(rando, url, 404, data=_form_data()) url = url_for( "applications.update_new_application_step_1", portfolio_id=portfolio.id, application_id=application.id, ) - post_url_assert_status(ccpo, url, 302, data=step_1_form_data) - post_url_assert_status(owner, url, 302, data=step_1_form_data) - post_url_assert_status(rando, url, 404, data=step_1_form_data) + post_url_assert_status(ccpo, url, 302, data=_form_data()) + post_url_assert_status(owner, url, 302, data=_form_data()) + post_url_assert_status(rando, url, 404, data=_form_data()) # applications.view_new_application_step_2 diff --git a/translations.yaml b/translations.yaml index 1d876e97..6a678f48 100644 --- a/translations.yaml +++ b/translations.yaml @@ -114,6 +114,8 @@ flash: message: '{application_name} has been successfully created. You may continue on to provision environments and assign team members now, or come back and complete these tasks at a later time.' updated: 'You have successfully updated the {application_name} application.' deleted: 'You have successfully deleted the {application_name} application. To view the retained activity log, visit the portfolio administration page.' + name_error: + message: 'The application name {name} has already been used in this portfolio. Please enter a unique name.' delete_member_success: 'You have successfully deleted {member_name} from the portfolio.' deleted_member: Portfolio member deleted environment_added: 'The environment "{env_name}" has been added to the application.'