From 1c0c5dd9c54f8e4fa44b9bd3f5865a1f05f789ad Mon Sep 17 00:00:00 2001 From: dandds Date: Tue, 9 Apr 2019 06:35:50 -0400 Subject: [PATCH 01/11] soft deletes available for applications and environments - parent relation will not include applications or environments marked as deleted - domain classes will exclude deleted objects from selections - changed some test factories to use domain_word for resource names, because they were using person names and it bugged me --- ...82_add_soft_delete_to_applications_and_.py | 31 +++++++++++++++++++ atst/domain/applications.py | 6 +++- atst/domain/environments.py | 6 +++- atst/models/application.py | 10 ++++-- atst/models/environment.py | 4 ++- atst/models/portfolio.py | 6 +++- tests/domain/test_applications.py | 13 ++++++-- tests/domain/test_environments.py | 18 ++++++++++- tests/factories.py | 6 ++-- tests/models/test_application.py | 14 ++++++++- tests/models/test_portfolio.py | 9 ++++++ 11 files changed, 111 insertions(+), 12 deletions(-) create mode 100644 alembic/versions/fd0cf917f682_add_soft_delete_to_applications_and_.py create mode 100644 tests/models/test_portfolio.py diff --git a/alembic/versions/fd0cf917f682_add_soft_delete_to_applications_and_.py b/alembic/versions/fd0cf917f682_add_soft_delete_to_applications_and_.py new file mode 100644 index 00000000..86949bd9 --- /dev/null +++ b/alembic/versions/fd0cf917f682_add_soft_delete_to_applications_and_.py @@ -0,0 +1,31 @@ +"""add soft delete to applications and environments + +Revision ID: fd0cf917f682 +Revises: 32438a35cfb5 +Create Date: 2019-04-09 06:16:15.445951 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.sql import expression + + +# revision identifiers, used by Alembic. +revision = 'fd0cf917f682' +down_revision = '32438a35cfb5' +branch_labels = None +depends_on = None + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.add_column('applications', sa.Column('deleted', sa.Boolean(), nullable=False, server_default=expression.false())) + op.add_column('environments', sa.Column('deleted', sa.Boolean(), nullable=False, server_default=expression.false())) + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_column('environments', 'deleted') + op.drop_column('applications', 'deleted') + # ### end Alembic commands ### diff --git a/atst/domain/applications.py b/atst/domain/applications.py index 30884c59..1639e7e2 100644 --- a/atst/domain/applications.py +++ b/atst/domain/applications.py @@ -1,3 +1,5 @@ +from sqlalchemy.orm.exc import NoResultFound + from atst.database import db from atst.domain.environments import Environments from atst.domain.exceptions import NotFoundError @@ -23,7 +25,9 @@ class Applications(object): def get(cls, application_id): try: application = ( - db.session.query(Application).filter_by(id=application_id).one() + db.session.query(Application) + .filter_by(id=application_id, deleted=False) + .one() ) except NoResultFound: raise NotFoundError("application") diff --git a/atst/domain/environments.py b/atst/domain/environments.py index 9b0fa9b5..4682adcd 100644 --- a/atst/domain/environments.py +++ b/atst/domain/environments.py @@ -51,7 +51,11 @@ class Environments(object): @classmethod def get(cls, environment_id): try: - env = db.session.query(Environment).filter_by(id=environment_id).one() + env = ( + db.session.query(Environment) + .filter_by(id=environment_id, deleted=False) + .one() + ) except NoResultFound: raise NotFoundError("environment") diff --git a/atst/models/application.py b/atst/models/application.py index d10bd773..d29998f7 100644 --- a/atst/models/application.py +++ b/atst/models/application.py @@ -1,4 +1,4 @@ -from sqlalchemy import Column, ForeignKey, String +from sqlalchemy import Column, ForeignKey, String, Boolean from sqlalchemy.orm import relationship from atst.models import Base @@ -15,9 +15,15 @@ class Application(Base, mixins.TimestampsMixin, mixins.AuditableMixin): portfolio_id = Column(ForeignKey("portfolios.id"), nullable=False) portfolio = relationship("Portfolio") - environments = relationship("Environment", back_populates="application") + environments = relationship( + "Environment", + back_populates="application", + primaryjoin="and_(Environment.application_id==Application.id, Environment.deleted==False)", + ) roles = relationship("ApplicationRole") + deleted = Column(Boolean, default=False) + @property def users(self): return set(role.user for role in self.roles) diff --git a/atst/models/environment.py b/atst/models/environment.py index a520e787..5f1e19d2 100644 --- a/atst/models/environment.py +++ b/atst/models/environment.py @@ -1,4 +1,4 @@ -from sqlalchemy import Column, ForeignKey, String +from sqlalchemy import Column, ForeignKey, String, Boolean from sqlalchemy.orm import relationship from atst.models import Base @@ -17,6 +17,8 @@ class Environment(Base, mixins.TimestampsMixin, mixins.AuditableMixin): cloud_id = Column(String) + deleted = Column(Boolean, default=False) + @property def users(self): return [r.user for r in self.roles] diff --git a/atst/models/portfolio.py b/atst/models/portfolio.py index 599de3e6..23c19482 100644 --- a/atst/models/portfolio.py +++ b/atst/models/portfolio.py @@ -16,7 +16,11 @@ class Portfolio(Base, mixins.TimestampsMixin, mixins.AuditableMixin): name = Column(String) defense_component = Column(String) # Department of Defense Component - applications = relationship("Application", back_populates="portfolio") + applications = relationship( + "Application", + back_populates="portfolio", + primaryjoin="and_(Application.portfolio_id==Portfolio.id, Application.deleted==False)", + ) roles = relationship("PortfolioRole") task_orders = relationship("TaskOrder") diff --git a/tests/domain/test_applications.py b/tests/domain/test_applications.py index 98dde0e2..8756bb7c 100644 --- a/tests/domain/test_applications.py +++ b/tests/domain/test_applications.py @@ -1,6 +1,9 @@ +import pytest + from atst.domain.applications import Applications -from tests.factories import UserFactory, PortfolioFactory -from atst.domain.portfolios import Portfolios +from atst.domain.exceptions import NotFoundError + +from tests.factories import ApplicationFactory, UserFactory, PortfolioFactory def test_create_application_with_multiple_environments(): @@ -53,3 +56,9 @@ def test_can_only_update_name_and_description(): assert application.description == "a new application" assert len(application.environments) == 1 assert application.environments[0].name == env_name + + +def test_get_excludes_deleted(): + app = ApplicationFactory.create(deleted=True) + with pytest.raises(NotFoundError): + Applications.get(app.id) diff --git a/tests/domain/test_environments.py b/tests/domain/test_environments.py index dee03fb6..c51b878c 100644 --- a/tests/domain/test_environments.py +++ b/tests/domain/test_environments.py @@ -1,8 +1,16 @@ +import pytest + from atst.domain.environments import Environments from atst.domain.environment_roles import EnvironmentRoles from atst.domain.portfolio_roles import PortfolioRoles +from atst.domain.exceptions import NotFoundError -from tests.factories import ApplicationFactory, UserFactory, PortfolioFactory +from tests.factories import ( + ApplicationFactory, + UserFactory, + PortfolioFactory, + EnvironmentFactory, +) def test_create_environments(): @@ -186,3 +194,11 @@ def test_get_scoped_environments(db): application2_envs = Environments.for_user(developer, portfolio.applications[1]) assert [env.name for env in application2_envs] == ["application2 staging"] + + +def test_get_excludes_deleted(): + env = EnvironmentFactory.create( + deleted=True, application=ApplicationFactory.create() + ) + with pytest.raises(NotFoundError): + Environments.get(env.id) diff --git a/tests/factories.py b/tests/factories.py index f9b5af17..29dc7fa0 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -105,7 +105,7 @@ class PortfolioFactory(Base): class Meta: model = Portfolio - name = factory.Faker("name") + name = factory.Faker("domain_word") defense_component = factory.LazyFunction(random_service_branch) @classmethod @@ -157,7 +157,7 @@ class ApplicationFactory(Base): model = Application portfolio = factory.SubFactory(PortfolioFactory) - name = factory.Faker("name") + name = factory.Faker("domain_word") description = "A test application" @classmethod @@ -192,6 +192,8 @@ class EnvironmentFactory(Base): class Meta: model = Environment + name = factory.Faker("domain_word") + @classmethod def _create(cls, model_class, *args, **kwargs): with_members = kwargs.pop("members", []) diff --git a/tests/models/test_application.py b/tests/models/test_application.py index d7bf564b..1aded029 100644 --- a/tests/models/test_application.py +++ b/tests/models/test_application.py @@ -1,4 +1,8 @@ -from tests.factories import ApplicationFactory, ApplicationRoleFactory +from tests.factories import ( + ApplicationFactory, + ApplicationRoleFactory, + EnvironmentFactory, +) def test_application_num_users(): @@ -9,3 +13,11 @@ def test_application_num_users(): ApplicationRoleFactory.create(application=application) assert application.num_users == 1 + + +def test_application_environments_excludes_deleted(): + app = ApplicationFactory.create() + env = EnvironmentFactory.create(application=app) + EnvironmentFactory.create(application=app, deleted=True) + assert len(app.environments) == 1 + assert app.environments[0].id == env.id diff --git a/tests/models/test_portfolio.py b/tests/models/test_portfolio.py new file mode 100644 index 00000000..613b6435 --- /dev/null +++ b/tests/models/test_portfolio.py @@ -0,0 +1,9 @@ +from tests.factories import ApplicationFactory, PortfolioFactory + + +def test_portfolio_applications_excludes_deleted(): + portfolio = PortfolioFactory.create() + app = ApplicationFactory.create(portfolio=portfolio) + ApplicationFactory.create(portfolio=portfolio, deleted=True) + assert len(portfolio.applications) == 1 + assert portfolio.applications[0].id == app.id From b58aef2c6b65ea613cc3a3dd04e4aa63ed18471f Mon Sep 17 00:00:00 2001 From: dandds Date: Tue, 9 Apr 2019 11:02:47 -0400 Subject: [PATCH 02/11] domain methods for deleting apps and environments --- atst/domain/applications.py | 10 ++++++++++ atst/domain/environments.py | 10 ++++++++++ tests/domain/test_applications.py | 25 ++++++++++++++++++++++++- tests/domain/test_environments.py | 14 ++++++++++++++ 4 files changed, 58 insertions(+), 1 deletion(-) diff --git a/atst/domain/applications.py b/atst/domain/applications.py index 1639e7e2..bd94942b 100644 --- a/atst/domain/applications.py +++ b/atst/domain/applications.py @@ -67,3 +67,13 @@ class Applications(object): db.session.commit() return application + + @classmethod + def delete(cls, application): + for env in application.environments: + Environments.delete(env) + + application.deleted = True + + db.session.add(application) + db.session.commit() diff --git a/atst/domain/environments.py b/atst/domain/environments.py index 4682adcd..a89d0063 100644 --- a/atst/domain/environments.py +++ b/atst/domain/environments.py @@ -98,3 +98,13 @@ class Environments(object): @classmethod def revoke_access(cls, environment, target_user): EnvironmentRoles.delete(environment.id, target_user.id) + + @classmethod + def delete(cls, environment, commit=False): + environment.deleted = True + + db.session.add(environment) + if commit: + db.session.commit() + + return environment diff --git a/tests/domain/test_applications.py b/tests/domain/test_applications.py index 8756bb7c..31e32ba4 100644 --- a/tests/domain/test_applications.py +++ b/tests/domain/test_applications.py @@ -3,7 +3,12 @@ import pytest from atst.domain.applications import Applications from atst.domain.exceptions import NotFoundError -from tests.factories import ApplicationFactory, UserFactory, PortfolioFactory +from tests.factories import ( + ApplicationFactory, + UserFactory, + PortfolioFactory, + EnvironmentFactory, +) def test_create_application_with_multiple_environments(): @@ -62,3 +67,21 @@ def test_get_excludes_deleted(): app = ApplicationFactory.create(deleted=True) with pytest.raises(NotFoundError): Applications.get(app.id) + + +def test_delete_application(session): + app = ApplicationFactory.create() + env1 = EnvironmentFactory.create(application=app) + env2 = EnvironmentFactory.create(application=app) + assert not app.deleted + assert not env1.deleted + assert not env2.deleted + + Applications.delete(app) + + assert app.deleted + assert env1.deleted + assert env2.deleted + + # changes are flushed + assert not session.dirty diff --git a/tests/domain/test_environments.py b/tests/domain/test_environments.py index c51b878c..99f73f9d 100644 --- a/tests/domain/test_environments.py +++ b/tests/domain/test_environments.py @@ -202,3 +202,17 @@ def test_get_excludes_deleted(): ) with pytest.raises(NotFoundError): Environments.get(env.id) + + +def test_delete_environment(session): + env = EnvironmentFactory.create(application=ApplicationFactory.create()) + assert not env.deleted + Environments.delete(env) + assert env.deleted + # did not flush + assert env in session.dirty + + Environments.delete(env, commit=True) + assert env.deleted + # flushed the change + assert not session.dirty From 0348af7ce72c08a60438c0c56057a2bce99dad9a Mon Sep 17 00:00:00 2001 From: dandds Date: Tue, 9 Apr 2019 15:53:35 -0400 Subject: [PATCH 03/11] record soft deletion in audit log for applications and environments --- atst/models/application.py | 9 +++++++++ atst/models/environment.py | 9 +++++++++ tests/models/test_application.py | 17 +++++++++++++++++ tests/models/test_environments.py | 26 +++++++++++++++++++++++++- 4 files changed, 60 insertions(+), 1 deletion(-) diff --git a/atst/models/application.py b/atst/models/application.py index d29998f7..aabcb477 100644 --- a/atst/models/application.py +++ b/atst/models/application.py @@ -40,3 +40,12 @@ class Application(Base, mixins.TimestampsMixin, mixins.AuditableMixin): return "".format( self.name, self.description, self.portfolio.name, self.id ) + + @property + def history(self): + previous_state = self.get_changes() + change_set = {} + if "deleted" in previous_state: + change_set["deleted"] = previous_state["deleted"] + + return change_set diff --git a/atst/models/environment.py b/atst/models/environment.py index 5f1e19d2..d36e90eb 100644 --- a/atst/models/environment.py +++ b/atst/models/environment.py @@ -46,3 +46,12 @@ class Environment(Base, mixins.TimestampsMixin, mixins.AuditableMixin): self.application.portfolio.name, self.id, ) + + @property + def history(self): + previous_state = self.get_changes() + change_set = {} + if "deleted" in previous_state: + change_set["deleted"] = previous_state["deleted"] + + return change_set diff --git a/tests/models/test_application.py b/tests/models/test_application.py index 1aded029..d90e4cd6 100644 --- a/tests/models/test_application.py +++ b/tests/models/test_application.py @@ -1,3 +1,5 @@ +from atst.models import AuditEvent + from tests.factories import ( ApplicationFactory, ApplicationRoleFactory, @@ -21,3 +23,18 @@ def test_application_environments_excludes_deleted(): EnvironmentFactory.create(application=app, deleted=True) assert len(app.environments) == 1 assert app.environments[0].id == env.id + + +def test_audit_event_for_application_deletion(session): + app = ApplicationFactory.create() + app.deleted = True + session.add(app) + session.commit() + + update_event = ( + session.query(AuditEvent) + .filter(AuditEvent.resource_id == app.id, AuditEvent.action == "update") + .one() + ) + assert update_event.changed_state.get("deleted") + assert update_event.changed_state["deleted"] == [False, True] diff --git a/tests/models/test_environments.py b/tests/models/test_environments.py index 17a32ec8..977849f2 100644 --- a/tests/models/test_environments.py +++ b/tests/models/test_environments.py @@ -1,6 +1,13 @@ +from atst.models import AuditEvent from atst.domain.environments import Environments from atst.domain.applications import Applications -from tests.factories import PortfolioFactory, UserFactory + +from tests.factories import ( + PortfolioFactory, + UserFactory, + EnvironmentFactory, + ApplicationFactory, +) def test_add_user_to_environment(): @@ -15,3 +22,20 @@ def test_add_user_to_environment(): dev_environment = Environments.add_member(dev_environment, developer, "developer") assert developer in dev_environment.users + + +def test_audit_event_for_environment_deletion(session): + EnvironmentFactory._meta.sqlalchemy_session_persistence = "flush" + + env = EnvironmentFactory.create(application=ApplicationFactory.create()) + env.deleted = True + session.add(env) + session.commit() + + update_event = ( + session.query(AuditEvent) + .filter(AuditEvent.resource_id == env.id, AuditEvent.action == "update") + .one() + ) + assert update_event.changed_state.get("deleted") + assert update_event.changed_state["deleted"] == [False, True] From dac764ab822a6338b0009e3456530552a9866957 Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 10 Apr 2019 07:15:15 -0400 Subject: [PATCH 04/11] route for deleting an application --- atst/domain/permission_sets.py | 1 + atst/models/permissions.py | 1 + atst/routes/portfolios/applications.py | 16 ++++++++ atst/utils/flash.py | 9 +++++ tests/routes/portfolios/test_applications.py | 41 +++++++++++++++++++- tests/test_access.py | 38 ++++++++++++++++++ 6 files changed, 105 insertions(+), 1 deletion(-) diff --git a/atst/domain/permission_sets.py b/atst/domain/permission_sets.py index 4d02682a..52131a20 100644 --- a/atst/domain/permission_sets.py +++ b/atst/domain/permission_sets.py @@ -86,6 +86,7 @@ _PORTFOLIO_APP_MGMT_PERMISSION_SETS = [ "permissions": [ Permissions.EDIT_APPLICATION, Permissions.CREATE_APPLICATION, + Permissions.DELETE_APPLICATION, Permissions.EDIT_APPLICATION_MEMBER, Permissions.CREATE_APPLICATION_MEMBER, Permissions.EDIT_ENVIRONMENT, diff --git a/atst/models/permissions.py b/atst/models/permissions.py index cb03020d..98f25b36 100644 --- a/atst/models/permissions.py +++ b/atst/models/permissions.py @@ -8,6 +8,7 @@ class Permissions(object): VIEW_APPLICATION = "view_application" EDIT_APPLICATION = "edit_application" CREATE_APPLICATION = "create_application" + DELETE_APPLICATION = "delete_application" VIEW_APPLICATION_MEMBER = "view_application_member" EDIT_APPLICATION_MEMBER = "edit_application_member" CREATE_APPLICATION_MEMBER = "create_application_member" diff --git a/atst/routes/portfolios/applications.py b/atst/routes/portfolios/applications.py index 20603b3c..955683dd 100644 --- a/atst/routes/portfolios/applications.py +++ b/atst/routes/portfolios/applications.py @@ -15,6 +15,7 @@ from atst.domain.portfolios import Portfolios from atst.forms.application import NewApplicationForm, ApplicationForm from atst.domain.authz.decorator import user_can_access_decorator as user_can from atst.models.permissions import Permissions +from atst.utils.flash import formatted_flash as flash @portfolios_bp.route("/portfolios//applications") @@ -118,3 +119,18 @@ def access_environment(portfolio_id, environment_id): token = app.csp.cloud.get_access_token(env_role) return redirect(url_for("atst.csp_environment_access", token=token)) + + +@portfolios_bp.route( + "/portfolios//applications//delete", methods=["POST"] +) +@user_can(Permissions.DELETE_APPLICATION, message="delete application") +def delete_application(portfolio_id, application_id): + application = Applications.get(application_id) + Applications.delete(application) + + flash("application_deleted", application_name=application.name) + + return redirect( + url_for("portfolios.portfolio_applications", portfolio_id=portfolio_id) + ) diff --git a/atst/utils/flash.py b/atst/utils/flash.py index d2b42979..dd32f5cc 100644 --- a/atst/utils/flash.py +++ b/atst/utils/flash.py @@ -148,6 +148,15 @@ MESSAGES = { """, "category": "success", }, + "application_deleted": { + "title_template": "Success!", + "message_template": """ + You have successfully deleted the {{ application_name }} application. + To view the retained activity log, visit the portfolio administration + page. Undo this action. + """, + "category": "success", + }, } diff --git a/tests/routes/portfolios/test_applications.py b/tests/routes/portfolios/test_applications.py index ab958f8a..8df582e3 100644 --- a/tests/routes/portfolios/test_applications.py +++ b/tests/routes/portfolios/test_applications.py @@ -1,4 +1,4 @@ -from flask import url_for +from flask import url_for, get_flashed_messages from tests.factories import ( UserFactory, @@ -290,3 +290,42 @@ def test_environment_access_with_no_role(client, user_session): ) ) assert response.status_code == 404 + + +def test_delete_application(client, user_session): + user = UserFactory.create() + port = PortfolioFactory.create( + owner=user, + applications=[ + { + "name": "mos eisley", + "environments": [ + {"name": "bar"}, + {"name": "booth"}, + {"name": "band stage"}, + ], + } + ], + ) + application = port.applications[0] + user_session(user) + + response = client.post( + url_for( + "portfolios.delete_application", + portfolio_id=port.id, + application_id=application.id, + ) + ) + # appropriate response and redirect + assert response.status_code == 302 + assert response.location == url_for( + "portfolios.portfolio_applications", portfolio_id=port.id, _external=True + ) + # appropriate flash message + message = get_flashed_messages()[0] + assert "deleted" in message["message"] + assert application.name in message["message"] + # app and envs are soft deleted + assert len(port.applications) == 0 + assert len(application.environments) == 0 diff --git a/tests/test_access.py b/tests/test_access.py index c79fd64d..0208887b 100644 --- a/tests/test_access.py +++ b/tests/test_access.py @@ -203,6 +203,44 @@ def test_portfolios_create_member_access(post_url_assert_status): post_url_assert_status(rando, url, 404) +# portfolios.delete_application +def test_portfolios_delete_application_access(post_url_assert_status, monkeypatch): + ccpo = UserFactory.create_ccpo() + owner = user_with() + app_admin = user_with() + rando = user_with() + + portfolio = PortfolioFactory.create( + owner=owner, applications=[{"name": "mos eisley"}] + ) + application = portfolio.applications[0] + + ApplicationRoleFactory.create( + user=app_admin, + application=application, + permission_sets=PermissionSets.get_many( + [ + PermissionSets.VIEW_APPLICATION, + PermissionSets.EDIT_APPLICATION_ENVIRONMENTS, + PermissionSets.EDIT_APPLICATION_TEAM, + PermissionSets.DELETE_APPLICATION_ENVIRONMENTS, + ] + ), + ) + + monkeypatch.setattr("atst.domain.applications.Applications.delete", lambda *a: True) + + url = url_for( + "portfolios.delete_application", + portfolio_id=portfolio.id, + application_id=application.id, + ) + post_url_assert_status(app_admin, url, 404) + post_url_assert_status(rando, url, 404) + post_url_assert_status(owner, url, 302) + post_url_assert_status(ccpo, url, 302) + + # portfolios.edit_application def test_portfolios_edit_application_access(get_url_assert_status): ccpo = user_with(PermissionSets.EDIT_PORTFOLIO_APPLICATION_MANAGEMENT) From 0bde431a70e9befac6bcc9925e3e7e02bc525f52 Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 10 Apr 2019 10:42:36 -0400 Subject: [PATCH 05/11] extend soft delete functionality to app and env roles --- ...eb947_add_soft_delete_to_application_and_.py} | 16 ++++++++++------ atst/domain/applications.py | 4 ++++ atst/domain/environments.py | 6 +++++- atst/models/application.py | 15 +++++---------- atst/models/application_role.py | 10 +++++++++- atst/models/environment.py | 15 +++++---------- atst/models/environment_role.py | 4 +++- atst/models/mixins/__init__.py | 1 + atst/models/mixins/deletable.py | 6 ++++++ atst/models/user.py | 6 +++++- .../kubernetes/test/atst-envvars-configmap.yml | 1 + tests/domain/test_applications.py | 4 ++++ tests/domain/test_environments.py | 7 ++++++- tests/models/test_environments.py | 6 +++--- tests/models/test_user.py | 15 ++++++++++++++- 15 files changed, 81 insertions(+), 35 deletions(-) rename alembic/versions/{fd0cf917f682_add_soft_delete_to_applications_and_.py => 014e4bceb947_add_soft_delete_to_application_and_.py} (52%) create mode 100644 atst/models/mixins/deletable.py diff --git a/alembic/versions/fd0cf917f682_add_soft_delete_to_applications_and_.py b/alembic/versions/014e4bceb947_add_soft_delete_to_application_and_.py similarity index 52% rename from alembic/versions/fd0cf917f682_add_soft_delete_to_applications_and_.py rename to alembic/versions/014e4bceb947_add_soft_delete_to_application_and_.py index 86949bd9..2b0a87b0 100644 --- a/alembic/versions/fd0cf917f682_add_soft_delete_to_applications_and_.py +++ b/alembic/versions/014e4bceb947_add_soft_delete_to_application_and_.py @@ -1,8 +1,8 @@ -"""add soft delete to applications and environments +"""add soft delete to application and environment resources and roles -Revision ID: fd0cf917f682 +Revision ID: 014e4bceb947 Revises: 32438a35cfb5 -Create Date: 2019-04-09 06:16:15.445951 +Create Date: 2019-04-10 09:40:37.688157 """ from alembic import op @@ -11,7 +11,7 @@ from sqlalchemy.sql import expression # revision identifiers, used by Alembic. -revision = 'fd0cf917f682' +revision = '014e4bceb947' down_revision = '32438a35cfb5' branch_labels = None depends_on = None @@ -19,13 +19,17 @@ depends_on = None def upgrade(): # ### commands auto generated by Alembic - please adjust! ### - op.add_column('applications', sa.Column('deleted', sa.Boolean(), nullable=False, server_default=expression.false())) - op.add_column('environments', sa.Column('deleted', sa.Boolean(), nullable=False, server_default=expression.false())) + op.add_column('application_roles', sa.Column('deleted', sa.Boolean(), server_default=expression.false(), nullable=False)) + op.add_column('applications', sa.Column('deleted', sa.Boolean(), server_default=expression.false(), nullable=False)) + op.add_column('environment_roles', sa.Column('deleted', sa.Boolean(), server_default=expression.false(), nullable=False)) + op.add_column('environments', sa.Column('deleted', sa.Boolean(), server_default=expression.false(), nullable=False)) # ### end Alembic commands ### def downgrade(): # ### commands auto generated by Alembic - please adjust! ### op.drop_column('environments', 'deleted') + op.drop_column('environment_roles', 'deleted') op.drop_column('applications', 'deleted') + op.drop_column('application_roles', 'deleted') # ### end Alembic commands ### diff --git a/atst/domain/applications.py b/atst/domain/applications.py index bd94942b..ebb37606 100644 --- a/atst/domain/applications.py +++ b/atst/domain/applications.py @@ -75,5 +75,9 @@ class Applications(object): application.deleted = True + for role in application.roles: + role.deleted = True + db.session.add(role) + db.session.add(application) db.session.commit() diff --git a/atst/domain/environments.py b/atst/domain/environments.py index a89d0063..367646fa 100644 --- a/atst/domain/environments.py +++ b/atst/domain/environments.py @@ -102,8 +102,12 @@ class Environments(object): @classmethod def delete(cls, environment, commit=False): environment.deleted = True - db.session.add(environment) + + for role in environment.roles: + role.deleted = True + db.session.add(role) + if commit: db.session.commit() diff --git a/atst/models/application.py b/atst/models/application.py index aabcb477..d06f20b7 100644 --- a/atst/models/application.py +++ b/atst/models/application.py @@ -1,4 +1,4 @@ -from sqlalchemy import Column, ForeignKey, String, Boolean +from sqlalchemy import Column, ForeignKey, String from sqlalchemy.orm import relationship from atst.models import Base @@ -6,7 +6,9 @@ from atst.models.types import Id from atst.models import mixins -class Application(Base, mixins.TimestampsMixin, mixins.AuditableMixin): +class Application( + Base, mixins.TimestampsMixin, mixins.AuditableMixin, mixins.DeletableMixin +): __tablename__ = "applications" id = Id() @@ -22,8 +24,6 @@ class Application(Base, mixins.TimestampsMixin, mixins.AuditableMixin): ) roles = relationship("ApplicationRole") - deleted = Column(Boolean, default=False) - @property def users(self): return set(role.user for role in self.roles) @@ -43,9 +43,4 @@ class Application(Base, mixins.TimestampsMixin, mixins.AuditableMixin): @property def history(self): - previous_state = self.get_changes() - change_set = {} - if "deleted" in previous_state: - change_set["deleted"] = previous_state["deleted"] - - return change_set + return self.get_changes() diff --git a/atst/models/application_role.py b/atst/models/application_role.py index 054006cc..9b8fe05d 100644 --- a/atst/models/application_role.py +++ b/atst/models/application_role.py @@ -26,7 +26,11 @@ application_roles_permission_sets = Table( class ApplicationRole( - Base, mixins.TimestampsMixin, mixins.AuditableMixin, mixins.PermissionsMixin + Base, + mixins.TimestampsMixin, + mixins.AuditableMixin, + mixins.PermissionsMixin, + mixins.DeletableMixin, ): __tablename__ = "application_roles" @@ -51,6 +55,10 @@ class ApplicationRole( self.application.name, self.user_id, self.id, self.permissions ) + @property + def history(self): + return self.get_changes() + Index( "application_role_user_application", diff --git a/atst/models/environment.py b/atst/models/environment.py index d36e90eb..410d9e91 100644 --- a/atst/models/environment.py +++ b/atst/models/environment.py @@ -1,4 +1,4 @@ -from sqlalchemy import Column, ForeignKey, String, Boolean +from sqlalchemy import Column, ForeignKey, String from sqlalchemy.orm import relationship from atst.models import Base @@ -6,7 +6,9 @@ from atst.models.types import Id from atst.models import mixins -class Environment(Base, mixins.TimestampsMixin, mixins.AuditableMixin): +class Environment( + Base, mixins.TimestampsMixin, mixins.AuditableMixin, mixins.DeletableMixin +): __tablename__ = "environments" id = Id() @@ -17,8 +19,6 @@ class Environment(Base, mixins.TimestampsMixin, mixins.AuditableMixin): cloud_id = Column(String) - deleted = Column(Boolean, default=False) - @property def users(self): return [r.user for r in self.roles] @@ -49,9 +49,4 @@ class Environment(Base, mixins.TimestampsMixin, mixins.AuditableMixin): @property def history(self): - previous_state = self.get_changes() - change_set = {} - if "deleted" in previous_state: - change_set["deleted"] = previous_state["deleted"] - - return change_set + return self.get_changes() diff --git a/atst/models/environment_role.py b/atst/models/environment_role.py index 37207e59..55cf742e 100644 --- a/atst/models/environment_role.py +++ b/atst/models/environment_role.py @@ -13,7 +13,9 @@ class CSPRole(Enum): TECHNICAL_READ = "Technical Read-only" -class EnvironmentRole(Base, mixins.TimestampsMixin, mixins.AuditableMixin): +class EnvironmentRole( + Base, mixins.TimestampsMixin, mixins.AuditableMixin, mixins.DeletableMixin +): __tablename__ = "environment_roles" id = types.Id() diff --git a/atst/models/mixins/__init__.py b/atst/models/mixins/__init__.py index ebc2b362..54c85d61 100644 --- a/atst/models/mixins/__init__.py +++ b/atst/models/mixins/__init__.py @@ -1,3 +1,4 @@ from .timestamps import TimestampsMixin from .auditable import AuditableMixin from .permissions import PermissionsMixin +from .deletable import DeletableMixin diff --git a/atst/models/mixins/deletable.py b/atst/models/mixins/deletable.py new file mode 100644 index 00000000..a8e430ab --- /dev/null +++ b/atst/models/mixins/deletable.py @@ -0,0 +1,6 @@ +from sqlalchemy import Column, Boolean +from sqlalchemy.sql import expression + + +class DeletableMixin(object): + deleted = Column(Boolean, nullable=False, server_default=expression.false()) diff --git a/atst/models/user.py b/atst/models/user.py index 971f0a94..a5379698 100644 --- a/atst/models/user.py +++ b/atst/models/user.py @@ -25,7 +25,11 @@ class User( permission_sets = relationship("PermissionSet", secondary=users_permission_sets) portfolio_roles = relationship("PortfolioRole", backref="user") - application_roles = relationship("ApplicationRole", backref="user") + application_roles = relationship( + "ApplicationRole", + backref="user", + primaryjoin="and_(ApplicationRole.user_id==User.id, ApplicationRole.deleted==False)", + ) email = Column(String, unique=True) dod_id = Column(String, unique=True, nullable=False) diff --git a/deploy/kubernetes/test/atst-envvars-configmap.yml b/deploy/kubernetes/test/atst-envvars-configmap.yml index bc82c333..1ac4cb03 100644 --- a/deploy/kubernetes/test/atst-envvars-configmap.yml +++ b/deploy/kubernetes/test/atst-envvars-configmap.yml @@ -12,3 +12,4 @@ data: RQ_QUEUES: atat-test CRL_STORAGE_PROVIDER: CLOUDFILES LOG_JSON: "true" + DEBUG: "false" diff --git a/tests/domain/test_applications.py b/tests/domain/test_applications.py index 31e32ba4..e955ed4e 100644 --- a/tests/domain/test_applications.py +++ b/tests/domain/test_applications.py @@ -5,6 +5,7 @@ from atst.domain.exceptions import NotFoundError from tests.factories import ( ApplicationFactory, + ApplicationRoleFactory, UserFactory, PortfolioFactory, EnvironmentFactory, @@ -71,17 +72,20 @@ def test_get_excludes_deleted(): def test_delete_application(session): app = ApplicationFactory.create() + app_role = ApplicationRoleFactory.create(user=UserFactory.create(), application=app) env1 = EnvironmentFactory.create(application=app) env2 = EnvironmentFactory.create(application=app) assert not app.deleted assert not env1.deleted assert not env2.deleted + assert not app_role.deleted Applications.delete(app) assert app.deleted assert env1.deleted assert env2.deleted + assert app_role.deleted # changes are flushed assert not session.dirty diff --git a/tests/domain/test_environments.py b/tests/domain/test_environments.py index 99f73f9d..b572a7e5 100644 --- a/tests/domain/test_environments.py +++ b/tests/domain/test_environments.py @@ -10,6 +10,7 @@ from tests.factories import ( UserFactory, PortfolioFactory, EnvironmentFactory, + EnvironmentRoleFactory, ) @@ -206,13 +207,17 @@ def test_get_excludes_deleted(): def test_delete_environment(session): env = EnvironmentFactory.create(application=ApplicationFactory.create()) + env_role = EnvironmentRoleFactory.create(user=UserFactory.create(), environment=env) assert not env.deleted + assert not env_role.deleted Environments.delete(env) assert env.deleted + assert env_role.deleted # did not flush - assert env in session.dirty + assert session.dirty Environments.delete(env, commit=True) assert env.deleted + assert env_role.deleted # flushed the change assert not session.dirty diff --git a/tests/models/test_environments.py b/tests/models/test_environments.py index 977849f2..bccfdb94 100644 --- a/tests/models/test_environments.py +++ b/tests/models/test_environments.py @@ -25,8 +25,6 @@ def test_add_user_to_environment(): def test_audit_event_for_environment_deletion(session): - EnvironmentFactory._meta.sqlalchemy_session_persistence = "flush" - env = EnvironmentFactory.create(application=ApplicationFactory.create()) env.deleted = True session.add(env) @@ -38,4 +36,6 @@ def test_audit_event_for_environment_deletion(session): .one() ) assert update_event.changed_state.get("deleted") - assert update_event.changed_state["deleted"] == [False, True] + before, after = update_event.changed_state["deleted"] + assert not before + assert after diff --git a/tests/models/test_user.py b/tests/models/test_user.py index ca3dc83d..2a3ae51e 100644 --- a/tests/models/test_user.py +++ b/tests/models/test_user.py @@ -3,7 +3,7 @@ from sqlalchemy.exc import InternalError from atst.models.user import User -from tests.factories import UserFactory +from tests.factories import UserFactory, ApplicationFactory, ApplicationRoleFactory def test_profile_complete_with_all_info(): @@ -24,3 +24,16 @@ def test_cannot_update_dod_id(session): session.add(user) with pytest.raises(InternalError): session.commit() + + +def test_deleted_application_roles_are_ignored(session): + user = UserFactory.create() + app = ApplicationFactory.create() + app_role = ApplicationRoleFactory.create(user=user, application=app) + assert len(user.application_roles) == 1 + + app_role.deleted = True + session.add(app_role) + session.commit() + + assert len(user.application_roles) == 0 From 2b06e281ae4d6682dc2eeecb203517d3b11ae03b Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 10 Apr 2019 13:43:29 -0400 Subject: [PATCH 06/11] confirmation modal for deleting an application - adds delete-confirmation Vue component - refactors some button styles to make them globally available --- js/components/delete_confirmation.js | 15 ++++ js/index.js | 2 + styles/components/_portfolio_layout.scss | 10 --- styles/elements/_buttons.scss | 20 +++++ templates/fragments/admin/members_edit.html | 2 +- templates/portfolios/applications/edit.html | 91 +++++++++++++++++++++ translations.yaml | 7 ++ 7 files changed, 136 insertions(+), 11 deletions(-) create mode 100644 js/components/delete_confirmation.js diff --git a/js/components/delete_confirmation.js b/js/components/delete_confirmation.js new file mode 100644 index 00000000..cd710de3 --- /dev/null +++ b/js/components/delete_confirmation.js @@ -0,0 +1,15 @@ +export default { + name: 'delete-confirmation', + + data: function() { + return { + deleteText: '', + } + }, + + computed: { + valid: function() { + return this.deleteText.toLowerCase() === 'delete' + }, + }, +} diff --git a/js/index.js b/js/index.js index dee29320..44461fc2 100644 --- a/js/index.js +++ b/js/index.js @@ -35,6 +35,7 @@ import DateSelector from './components/date_selector' import SidenavToggler from './components/sidenav_toggler' import KoReview from './components/forms/ko_review' import BaseForm from './components/forms/base_form' +import DeleteConfirmation from './components/delete_confirmation' Vue.config.productionTip = false @@ -72,6 +73,7 @@ const app = new Vue({ SidenavToggler, KoReview, BaseForm, + DeleteConfirmation, }, mounted: function() { diff --git a/styles/components/_portfolio_layout.scss b/styles/components/_portfolio_layout.scss index 906e14a6..06b9f416 100644 --- a/styles/components/_portfolio_layout.scss +++ b/styles/components/_portfolio_layout.scss @@ -213,12 +213,6 @@ border-top: 0; padding: 3 * $gap 2 * $gap; - .usa-button-secondary { - color: $color-red; - background-color: $color-red-lightest; - box-shadow: inset 0 0 0 1px $color-red; - } - .usa-button-disabled { color: $color-gray-medium; background-color: $color-gray-lightest; @@ -271,10 +265,6 @@ height: 4rem; } - .usa-button-danger { - background: $color-red; - } - select { padding-left: 1.2rem } diff --git a/styles/elements/_buttons.scss b/styles/elements/_buttons.scss index 59a6481e..af167ade 100644 --- a/styles/elements/_buttons.scss +++ b/styles/elements/_buttons.scss @@ -11,3 +11,23 @@ button, opacity: 0.75; } } + +.button-danger { + background: $color-red; + + &:hover { + background-color: $color-red-darkest; + } +} + +.button-danger-outline, input[type="button"].button-danger-outline { + color: $color-red; + background-color: $color-red-lightest; + box-shadow: inset 0 0 0 1px $color-red; + + &:hover { + color: white; + background-color: $color-red-darkest; + box-shadow: inset 0 0 0 1px $color-red-darkest; + } +} diff --git a/templates/fragments/admin/members_edit.html b/templates/fragments/admin/members_edit.html index d2f9889f..8f5f8fd2 100644 --- a/templates/fragments/admin/members_edit.html +++ b/templates/fragments/admin/members_edit.html @@ -12,7 +12,7 @@ {% elif ppoc %} {% set archive_button_class = 'usa-button-disabled' %} {% else %} - {% set archive_button_class = 'usa-button-secondary' %} + {% set archive_button_class = 'button-danger-outline' %} {% endif %} diff --git a/templates/portfolios/applications/edit.html b/templates/portfolios/applications/edit.html index db842f80..602a42f5 100644 --- a/templates/portfolios/applications/edit.html +++ b/templates/portfolios/applications/edit.html @@ -1,7 +1,9 @@ {% extends "portfolios/applications/base.html" %} +{% from "components/alert.html" import Alert %} {% from "components/text_input.html" import TextInput %} {% from "components/icon.html" import Icon %} +{% from "components/modal.html" import Modal %} {% set secondary_breadcrumb = 'portfolios.applications.existing_application_title' | translate({ "application_name": application.name }) %} @@ -14,6 +16,58 @@
{% include "fragments/applications/edit_application_form.html" %} + {{ form.csrf_token }} +

+ {{ "fragments.edit_application_form.explain" | translate }} +

+
+
+ {{ TextInput(form.name) }} +
+
+ {% if user_can(permissions.DELETE_APPLICATION) %} +
+ + +
+ {% endif %} +
+
+
+
+ {{ TextInput(form.description, paragraph=True) }} +
+
+ +
+
+

{{ 'portfolios.applications.environments_heading' | translate }}

+

+ {{ 'portfolios.applications.environments_description' | translate }} +

+
+ +
    + {% for environment in application.environments %} +
  • +
    + + +
    +
    • + {% endfor %} +
+
@@ -36,5 +90,42 @@
+ {% if user_can(permissions.DELETE_APPLICATION) %} + {% call Modal(name="delete-application") %} +

{{ "portfolios.applications.delete.header" | translate }}

+ + {{ + Alert( + title="portfolios.applications.delete.alert.title" | translate, + message="portfolios.applications.delete.alert.message" | translate, + level="warning" + ) + }} + + +
+
+ + +
+
+
+ {{ form.csrf_token }} + +
+
+ {{ "common.cancel" | translate }} +
+
+
+ + {% endcall %} + {% endif %} {% endblock %} diff --git a/translations.yaml b/translations.yaml index a9711883..c1e5de97 100644 --- a/translations.yaml +++ b/translations.yaml @@ -43,6 +43,7 @@ common: contracting_officer: Contracting Officer security_officer: Security Officer contracting_officer_representative: Contracting Officer Representative + delete_confirm: "Please type the word DELETE to confirm:" components: modal: close: Close @@ -599,6 +600,12 @@ portfolios: name: Name members: Members add_environment: Add New Environment + delete: + button: Delete Application + header: Are you sure you want to delete this application? + alert: + title: Warning! This action is permanent. + message: You will lose access to this application and all of its reporting and metrics tools. The activity log will be retained. admin: portfolio_members_title: Portfolio members portfolio_members_subheading: These members have different levels of access to the portfolio. From 963bf454fbefce77457be7775d1a0b921e40ce5b Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 10 Apr 2019 15:46:35 -0400 Subject: [PATCH 07/11] display flash messages for application deletion on application index page --- templates/portfolios/applications/index.html | 1 + 1 file changed, 1 insertion(+) diff --git a/templates/portfolios/applications/index.html b/templates/portfolios/applications/index.html index 0ecddde4..dab1a214 100644 --- a/templates/portfolios/applications/index.html +++ b/templates/portfolios/applications/index.html @@ -8,6 +8,7 @@ {% block portfolio_content %}
+ {% include "fragments/flash.html" %}
Applications
From af850c5789fa5f6e4172e0373ec5901add73bd12 Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 10 Apr 2019 15:47:05 -0400 Subject: [PATCH 08/11] modal z-index should be higher than US govt banner --- styles/components/_modal.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/styles/components/_modal.scss b/styles/components/_modal.scss index dd68fd64..b2c31798 100644 --- a/styles/components/_modal.scss +++ b/styles/components/_modal.scss @@ -8,7 +8,7 @@ body { .modal { position: fixed; - z-index: 3; + z-index: 6; left: 0; right: 0; top: 0; From 453d2664af1a166857d619f5ebd642bdfe7d74c4 Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 10 Apr 2019 16:31:58 -0400 Subject: [PATCH 09/11] restore regular uswds alerts --- templates/components/alert.html | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/templates/components/alert.html b/templates/components/alert.html index b510457e..d6834f0d 100644 --- a/templates/components/alert.html +++ b/templates/components/alert.html @@ -21,26 +21,25 @@ } } %} -
- {{ Icon(levels.get(level).get('icon'), classes='alert__icon icon--large') }} +
-
+
{% if vue_template %} -

+

{% else %} -

{{title}}

+

{{title}}

{% endif %} {% if message %} -
{{ message | safe }}
+
{{ message | safe }}
{% endif %} {% if caller %} -
{{ caller() }}
+
{{ caller() }}
{% endif %} {% if fragment %} -
+
{% include fragment %}
{% endif %} From a2e815afd926d982ae78e08f9b29a0aebd176d7c Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 10 Apr 2019 16:51:01 -0400 Subject: [PATCH 10/11] move application deletion alert content into translations file --- atst/utils/flash.py | 7 +++---- translations.yaml | 2 ++ 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/atst/utils/flash.py b/atst/utils/flash.py index dd32f5cc..7970cc03 100644 --- a/atst/utils/flash.py +++ b/atst/utils/flash.py @@ -149,11 +149,10 @@ MESSAGES = { "category": "success", }, "application_deleted": { - "title_template": "Success!", + "title_template": translate("flash.success"), "message_template": """ - You have successfully deleted the {{ application_name }} application. - To view the retained activity log, visit the portfolio administration - page. Undo this action. + {{ "flash.application.deleted" | translate({"application_name": application_name}) }} + Undo. """, "category": "success", }, diff --git a/translations.yaml b/translations.yaml index c1e5de97..cf3a3fdb 100644 --- a/translations.yaml +++ b/translations.yaml @@ -28,6 +28,8 @@ flash: delete_member_success: You have successfully deleted {member_name} from the portfolio. new_ppoc_title: Primary point of contact updated new_ppoc_message: You have successfully added {ppoc_name} as the primary point of contact. You are no longer the PPoC. + application: + deleted: You have successfully deleted the {application_name} application. To view the retained activity log, visit the portfolio administration page. common: back: Back cancel: Cancel From e4c50da363dd95b63562fe2a4adf7a7940f75c1d Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 10 Apr 2019 17:20:17 -0400 Subject: [PATCH 11/11] push environment deletion info to CSP --- atst/domain/csp/cloud.py | 11 +++++++++++ atst/domain/environments.py | 2 ++ 2 files changed, 13 insertions(+) diff --git a/atst/domain/csp/cloud.py b/atst/domain/csp/cloud.py index 529897ad..c498a1fe 100644 --- a/atst/domain/csp/cloud.py +++ b/atst/domain/csp/cloud.py @@ -8,6 +8,12 @@ class CloudProviderInterface: """ raise NotImplementedError() + def delete_application(self, cloud_id): # pragma: no cover + """Delete an application in the cloud with the provided cloud_id. Returns + True for success or raises an error. + """ + raise NotImplementedError() + def create_user(self, user): # pragma: no cover """Create an account in the CSP for specified user. Returns the ID of the created user. @@ -49,6 +55,11 @@ class MockCloudProvider(CloudProviderInterface): cloud.""" return uuid4().hex + def delete_application(self, name): + """Returns an id that represents what would be an application in the + cloud.""" + return True + def create_user(self, user): """Returns an id that represents what would be an user in the cloud.""" return uuid4().hex diff --git a/atst/domain/environments.py b/atst/domain/environments.py index 367646fa..5e7102b1 100644 --- a/atst/domain/environments.py +++ b/atst/domain/environments.py @@ -111,4 +111,6 @@ class Environments(object): if commit: db.session.commit() + app.csp.cloud.delete_application(environment.cloud_id) + return environment