Merge pull request #1249 from dod-ccpo/app-name-uniqueness

App name uniqueness

.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"
       }
     ]

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 ###

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

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)
-
+        update_or_raise_already_exists_error(message="task_order")
-        try:
-            db.session.commit()
-        except IntegrityError:
-            db.session.rollback()
-            raise AlreadyExistsError("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:
+        update_or_raise_already_exists_error(message="task_order")
-            db.session.commit()
-        except IntegrityError:
-            db.session.rollback()
-            raise AlreadyExistsError("task_order")
-
         return task_order
 
     @classmethod

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):

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/<portfolio_id>/applications/new")
 @applications_bp.route("/applications/<application_id>/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():
+    if application:
-        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)
         return redirect(
             url_for(
                 "applications.update_new_application_step_2",

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)

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.",

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})

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"],
         )

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)

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",
+    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(ccpo, url, 302, data=_form_data())
-    post_url_assert_status(owner, url, 302, data=step_1_form_data)
+    post_url_assert_status(owner, url, 302, data=_form_data())
-    post_url_assert_status(rando, url, 404, data=step_1_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(ccpo, url, 302, data=_form_data())
-    post_url_assert_status(owner, url, 302, data=step_1_form_data)
+    post_url_assert_status(owner, url, 302, data=_form_data())
-    post_url_assert_status(rando, url, 404, data=step_1_form_data)
+    post_url_assert_status(rando, url, 404, data=_form_data())
 
 
 # applications.view_new_application_step_2

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.'