From 0bde431a70e9befac6bcc9925e3e7e02bc525f52 Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 10 Apr 2019 10:42:36 -0400 Subject: [PATCH] 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