Merge pull request #1257 from dod-ccpo/environment-name-uniqueness

Env and App name uniqueness

.secrets.baseline

@@ -3,7 +3,7 @@
     "files": "^.secrets.baseline$|^.*pgsslrootcert.yml$",
     "lines": null
   },
-  "generated_at": "2019-12-13T20:38:57Z",
+  "generated_at": "2019-12-18T15:29:41Z",
   "plugins_used": [
     {
       "base64_limit": 4.5,
@@ -170,7 +170,7 @@
         "hashed_secret": "e4f14805dfd1e6af030359090c535e149e6b4207",
         "is_secret": false,
         "is_verified": false,
-        "line_number": 659,
+        "line_number": 665,
         "type": "Hex High Entropy String"
       }
     ]

alembic/versions/08f2a640e9c2_add_uniqueness_contraint_to_environment_.py

@@ -0,0 +1,26 @@
+"""add uniqueness contraint to environment within an application
+
+Revision ID: 08f2a640e9c2
+Revises: c487d91f1a26
+Create Date: 2019-12-16 10:43:12.331095
+
+"""
+from alembic import op
+
+# revision identifiers, used by Alembic.
+revision = '08f2a640e9c2' # pragma: allowlist secret
+down_revision = 'c487d91f1a26' # pragma: allowlist secret
+branch_labels = None
+depends_on = None
+
+
+def upgrade():
+    # ### commands auto generated by Alembic - please adjust! ###
+    op.create_unique_constraint('environments_name_application_id_key', 'environments', ['name', 'application_id'])
+    # ### end Alembic commands ###
+
+
+def downgrade():
+    # ### commands auto generated by Alembic - please adjust! ###
+    op.drop_constraint('environments_name_application_id_key', 'environments', 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, update_or_raise_already_exists_error
+from atst.utils import first_or_none, commit_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)
 
-        update_or_raise_already_exists_error(message="application")
+        commit_or_raise_already_exists_error(message="application")
         return application
 
     @classmethod
@@ -55,7 +55,7 @@ class Applications(BaseDomainClass):
             )
 
         db.session.add(application)
-        update_or_raise_already_exists_error(message="application")
+        commit_or_raise_already_exists_error(message="application")
         return application
 
     @classmethod

atst/domain/environments.py

@@ -12,6 +12,7 @@ from atst.models import (
     CLIN,
 )
 from atst.domain.environment_roles import EnvironmentRoles
+from atst.utils import commit_or_raise_already_exists_error
 
 from .exceptions import NotFoundError, DisabledError
 
@@ -21,7 +22,7 @@ class Environments(object):
     def create(cls, user, application, name):
         environment = Environment(application=application, name=name, creator=user)
         db.session.add(environment)
-        db.session.commit()
+        commit_or_raise_already_exists_error(message="environment")
         return environment
 
     @classmethod
@@ -39,7 +40,8 @@ class Environments(object):
         if name is not None:
             environment.name = name
             db.session.add(environment)
-            db.session.commit()
+            commit_or_raise_already_exists_error(message="environment")
+            return environment
 
     @classmethod
     def get(cls, environment_id):

atst/domain/task_orders.py

@@ -4,7 +4,7 @@ from atst.database import db
 from atst.models.clin import CLIN
 from atst.models.task_order import TaskOrder, SORT_ORDERING
 from . import BaseDomainClass
-from atst.utils import update_or_raise_already_exists_error
+from atst.utils import commit_or_raise_already_exists_error
 
 
 class TaskOrders(BaseDomainClass):
@@ -15,7 +15,7 @@ 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")
+        commit_or_raise_already_exists_error(message="task_order")
         TaskOrders.create_clins(task_order.id, clins)
         return task_order
 
@@ -34,7 +34,7 @@ class TaskOrders(BaseDomainClass):
             task_order.number = number
             db.session.add(task_order)
 
-        update_or_raise_already_exists_error(message="task_order")
+        commit_or_raise_already_exists_error(message="task_order")
         return task_order
 
     @classmethod

atst/models/environment.py

@@ -1,4 +1,4 @@
-from sqlalchemy import Column, ForeignKey, String, TIMESTAMP
+from sqlalchemy import Column, ForeignKey, String, TIMESTAMP, UniqueConstraint
 from sqlalchemy.orm import relationship
 from sqlalchemy.dialects.postgresql import JSONB
 from enum import Enum
@@ -38,6 +38,12 @@ class Environment(
         primaryjoin="and_(EnvironmentRole.environment_id == Environment.id, EnvironmentRole.deleted == False)",
     )
 
+    __table_args__ = (
+        UniqueConstraint(
+            "name", "application_id", name="environments_name_application_id_key"
+        ),
+    )
+
     class ProvisioningStatus(Enum):
         PENDING = "pending"
         COMPLETED = "completed"

atst/routes/applications/new.py

@@ -1,9 +1,7 @@
-from flask import redirect, render_template, request as http_request, url_for, g
+from flask import redirect, render_template, request as http_request, url_for
 
 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
 from atst.models.permissions import Permissions
@@ -13,6 +11,7 @@ from atst.routes.applications.settings import (
     get_new_member_form,
     handle_create_member,
     handle_update_member,
+    handle_update_application,
 )
 
 
@@ -38,31 +37,6 @@ 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")
@@ -90,7 +64,7 @@ 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)
+    application = handle_update_application(form, application_id, portfolio_id)
 
     if application:
         return redirect(

atst/routes/applications/settings.py

@@ -1,4 +1,10 @@
-from flask import redirect, render_template, request as http_request, url_for, g
+from flask import (
+    redirect,
+    render_template,
+    request as http_request,
+    url_for,
+    g,
+)
 
 from .blueprint import applications_bp
 from atst.domain.exceptions import AlreadyExistsError
@@ -10,6 +16,7 @@ from atst.domain.csp.cloud import GeneralCSPException
 from atst.domain.common import Paginator
 from atst.domain.environment_roles import EnvironmentRoles
 from atst.domain.invitations import ApplicationInvitations
+from atst.domain.portfolios import Portfolios
 from atst.forms.application_member import NewForm as NewMemberForm, UpdateMemberForm
 from atst.forms.application import NameAndDescriptionForm, EditEnvironmentForm
 from atst.forms.data import ENV_ROLE_NO_ACCESS as NO_ACCESS
@@ -245,16 +252,59 @@ def handle_update_member(application_id, application_role_id, form_data):
         # TODO: flash error message
 
 
+def handle_update_environment(form, application=None, environment=None):
+    if form.validate():
+        try:
+            if environment:
+                environment = Environments.update(
+                    environment=environment, name=form.name.data
+                )
+                flash("application_environments_updated")
+            else:
+                environment = Environments.create(
+                    g.current_user, application=application, name=form.name.data
+                )
+                flash("environment_added", environment_name=form.name.data)
+
+            return environment
+
+        except AlreadyExistsError:
+            flash("application_environments_name_error", name=form.name.data)
+            return False
+
+    else:
+        return False
+
+
+def handle_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
+
+
 @applications_bp.route("/applications/<application_id>/settings")
 @user_can(Permissions.VIEW_APPLICATION, message="view application edit form")
 def settings(application_id):
     application = Applications.get(application_id)
 
-    return render_settings_page(
+    return render_settings_page(application=application,)
-        application=application,
-        active_toggler=http_request.args.get("active_toggler"),
-        active_toggler_section=http_request.args.get("active_toggler_section"),
-    )
 
 
 @applications_bp.route("/environments/<environment_id>/edit", methods=["POST"])
@@ -264,31 +314,21 @@ def update_environment(environment_id):
     application = environment.application
 
     env_form = EditEnvironmentForm(obj=environment, formdata=http_request.form)
+    updated_environment = handle_update_environment(
+        form=env_form, application=application, environment=environment
+    )
 
-    if env_form.validate():
+    if updated_environment:
-        Environments.update(environment=environment, name=env_form.name.data)
-
-        flash("application_environments_updated")
-
         return redirect(
             url_for(
                 "applications.settings",
                 application_id=application.id,
                 fragment="application-environments",
                 _anchor="application-environments",
-                active_toggler=environment.id,
-                active_toggler_section="edit",
             )
         )
     else:
-        return (
+        return (render_settings_page(application=application, show_flash=True), 400)
-            render_settings_page(
-                application=application,
-                active_toggler=environment.id,
-                active_toggler_section="edit",
-            ),
-            400,
-        )
 
 
 @applications_bp.route(
@@ -298,14 +338,9 @@ def update_environment(environment_id):
 def new_environment(application_id):
     application = Applications.get(application_id)
     env_form = EditEnvironmentForm(formdata=http_request.form)
+    environment = handle_update_environment(form=env_form, application=application)
 
-    if env_form.validate():
+    if environment:
-        Environments.create(
-            g.current_user, application=application, name=env_form.name.data
-        )
-
-        flash("environment_added", environment_name=env_form.data["name"])
-
         return redirect(
             url_for(
                 "applications.settings",
@@ -315,7 +350,7 @@ def new_environment(application_id):
             )
         )
     else:
-        return (render_settings_page(application=application), 400)
+        return (render_settings_page(application=application, show_flash=True), 400)
 
 
 @applications_bp.route("/applications/<application_id>/edit", methods=["POST"])
@@ -323,10 +358,9 @@ def new_environment(application_id):
 def update(application_id):
     application = Applications.get(application_id)
     form = NameAndDescriptionForm(http_request.form)
-    if form.validate():
+    updated_application = handle_update_application(form, application_id)
-        application_data = form.data
-        Applications.update(application, application_data)
 
+    if updated_application:
         return redirect(
             url_for(
                 "applications.portfolio_applications",
@@ -334,7 +368,10 @@ def update(application_id):
             )
         )
     else:
-        return render_settings_page(application=application, application_form=form)
+        return (
+            render_settings_page(application=application, show_flash=True),
+            400,
+        )
 
 
 @applications_bp.route("/applications/<application_id>/delete", methods=["POST"])

atst/utils/__init__.py

@@ -30,7 +30,7 @@ def pick(keys, dct):
     return {k: v for (k, v) in dct.items() if k in _keys}
 
 
-def update_or_raise_already_exists_error(message):
+def commit_or_raise_already_exists_error(message):
     try:
         db.session.commit()
     except IntegrityError:

atst/utils/flash.py

@@ -29,6 +29,11 @@ MESSAGES = {
         """,
         "category": "success",
     },
+    "application_environments_name_error": {
+        "title_template": "",
+        "message_template": """{{ 'flash.application.env_name_error.message' | translate({ 'name': name }) }}""",
+        "category": "error",
+    },
     "application_environments_updated": {
         "title_template": "Application environments updated",
         "message_template": "Application environments have been updated",

templates/applications/settings.html

@@ -13,6 +13,9 @@
 
 {% block application_content %}
 
+  {% if show_flash -%}
+    {% include "fragments/flash.html" %}
+  {%- endif %}
   <h3>{{ 'portfolios.applications.settings.name_description' | translate }}</h3>
 
   {% if user_can(permissions.EDIT_APPLICATION) %}

tests/domain/test_environments.py

@@ -4,7 +4,7 @@ from uuid import uuid4
 
 from atst.domain.environments import Environments
 from atst.domain.environment_roles import EnvironmentRoles
-from atst.domain.exceptions import NotFoundError, DisabledError
+from atst.domain.exceptions import AlreadyExistsError, DisabledError, NotFoundError
 from atst.models.environment_role import CSPRole, EnvironmentRole
 
 from tests.factories import (
@@ -100,6 +100,27 @@ def test_update_environment():
     assert environment.name == "name 2"
 
 
+def test_create_does_not_duplicate_names_within_application():
+    application = ApplicationFactory.create()
+    name = "Your Environment"
+    user = application.portfolio.owner
+
+    assert Environments.create(user, application, name)
+    with pytest.raises(AlreadyExistsError):
+        Environments.create(user, application, name)
+
+
+def test_update_does_not_duplicate_names_within_application():
+    application = ApplicationFactory.create()
+    name = "Your Environment"
+    environment = EnvironmentFactory.create(application=application, name=name)
+    dupe_env = EnvironmentFactory.create(application=application)
+    user = application.portfolio.owner
+
+    with pytest.raises(AlreadyExistsError):
+        Environments.update(dupe_env, name)
+
+
 class EnvQueryTest:
     @property
     def NOW(self):

tests/routes/applications/test_settings.py

@@ -52,8 +52,6 @@ def test_updating_application_environments_success(client, user_session):
         _external=True,
         fragment="application-environments",
         _anchor="application-environments",
-        active_toggler=environment.id,
-        active_toggler_section="edit",
     )
     assert environment.name == "new name a"
 
@@ -78,6 +76,24 @@ def test_update_environment_failure(client, user_session):
     assert environment.name == "original name"
 
 
+def test_enforces_unique_env_name(client, user_session, session):
+    application = ApplicationFactory.create()
+    user = application.portfolio.owner
+    name = "New Environment"
+    environment = EnvironmentFactory.create(application=application, name=name)
+    form_data = {"name": name}
+    user_session(user)
+
+    session.begin_nested()
+    response = client.post(
+        url_for("applications.new_environment", application_id=application.id),
+        data=form_data,
+    )
+    session.rollback()
+
+    assert response.status_code == 400
+
+
 def test_application_settings(client, user_session):
     portfolio = PortfolioFactory.create()
     application = Applications.create(
@@ -258,6 +274,23 @@ def test_user_without_permission_cannot_update_application(client, user_session)
     assert application.description == "Cool stuff happening here!"
 
 
+def test_update_application_enforces_unique_name(client, user_session, session):
+    portfolio = PortfolioFactory.create()
+    name = "Test Application"
+    application = ApplicationFactory.create(portfolio=portfolio, name=name)
+    dupe_application = ApplicationFactory.create(portfolio=portfolio)
+    user_session(portfolio.owner)
+
+    session.begin_nested()
+    response = client.post(
+        url_for("applications.update", application_id=dupe_application.id),
+        data={"name": name, "description": dupe_application.description},
+    )
+    session.rollback()
+
+    assert response.status_code == 400
+
+
 def test_user_can_only_access_apps_in_their_portfolio(client, user_session):
     portfolio = PortfolioFactory.create()
     other_portfolio = PortfolioFactory.create(

tests/test_access.py

@@ -538,10 +538,16 @@ def test_applications_update_access(post_url_assert_status):
     )
     app = portfolio.applications[0]
 
+    def _form_data():
+        return {
+            "name": "Test Application %s" % (random.randrange(1, 1000)),
+            "description": "This is only a test",
+        }
+
     url = url_for("applications.update", application_id=app.id)
-    post_url_assert_status(dev, url, 200)
+    post_url_assert_status(dev, url, 302, data=_form_data())
-    post_url_assert_status(ccpo, url, 200)
+    post_url_assert_status(ccpo, url, 302, data=_form_data())
-    post_url_assert_status(rando, url, 404)
+    post_url_assert_status(rando, url, 404, data=_form_data())
 
 
 # applications.update_environments

translations.yaml

@@ -116,6 +116,8 @@ flash:
     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.'
+    env_name_error:
+      message: 'The environment name {name} has already been used in this application. 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.'
@@ -199,7 +201,7 @@ forms:
             </ul>
           </p>
         </div>
-    defense_component: 
+    defense_component:
       label: "Select DoD component(s) funding your Portfolio:"
       choices:
         air_force: Air Force
@@ -211,7 +213,7 @@ forms:
       help_text: |
         <p>
           Select the DOD component(s) that will fund all Applications within this Portfolio.
-          In JEDI, multiple DoD organizations can fund the same Portfolio.<br/> 
+          In JEDI, multiple DoD organizations can fund the same Portfolio.<br/>
           Select all that apply.<br/>
         </p>
   attachment: