[DOP-22267] - remove is_deleted column to deleted object for (Group, …

…Queue, Transfer)

docs/changelog/next_release/168.breaking.rst

@@ -0,0 +1,2 @@
+Add deletion of **transfers**, **queues** and **groups** records
+instead of marking them as deleted

syncmaster/db/migrations/versions/2023-11-23_0002_create_group_table.py

@@ -27,7 +27,6 @@ def upgrade():
         sa.Column("owner_id", sa.BigInteger(), nullable=False),
         sa.Column("created_at", sa.DateTime(), server_default=sa.text("now()"), nullable=False),
         sa.Column("updated_at", sa.DateTime(), server_default=sa.text("now()"), nullable=False),
-        sa.Column("is_deleted", sa.Boolean(), nullable=False),
         sa.Column(
             "search_vector",
             postgresql.TSVECTOR(),

syncmaster/db/migrations/versions/2023-11-23_0003_create_queue_table.py

@@ -28,7 +28,6 @@ def upgrade():
         sa.Column("description", sa.String(length=512), nullable=False),
         sa.Column("created_at", sa.DateTime(), server_default=sa.text("now()"), nullable=False),
         sa.Column("updated_at", sa.DateTime(), server_default=sa.text("now()"), nullable=False),
-        sa.Column("is_deleted", sa.Boolean(), nullable=False),
         sa.Column(
             "search_vector",
             postgresql.TSVECTOR(),

syncmaster/db/migrations/versions/2023-11-23_0007_create_transfer_table.py

@@ -47,7 +47,6 @@ def upgrade():
         sa.Column("is_scheduled", sa.Boolean(), nullable=False),
         sa.Column("schedule", sa.String(length=32), nullable=False),
         sa.Column("queue_id", sa.BigInteger(), nullable=False),
-        sa.Column("is_deleted", sa.Boolean(), nullable=False),
         sa.Column("created_at", sa.DateTime(), server_default=sa.text("now()"), nullable=False),
         sa.Column("updated_at", sa.DateTime(), server_default=sa.text("now()"), nullable=False),
         sa.ForeignKeyConstraint(

syncmaster/db/models/group.py

@@ -10,7 +10,7 @@
 from sqlalchemy.orm import Mapped, mapped_column, relationship
 from sqlalchemy_utils import ChoiceType
 
-from syncmaster.db.mixins import DeletableMixin, TimestampMixin
+from syncmaster.db.mixins import TimestampMixin
 from syncmaster.db.models.base import Base
 from syncmaster.db.models.user import User
 
@@ -69,7 +69,7 @@ def roles_at_least(cls, role: str | GroupMemberRole) -> list[str]:
         return [r.value for r, level in cls.role_hierarchy().items() if level >= required_level]
 
 
-class Group(Base, TimestampMixin, DeletableMixin):
+class Group(Base, TimestampMixin):
     id: Mapped[int] = mapped_column(BigInteger, primary_key=True)
     name: Mapped[str] = mapped_column(String(256), nullable=False, unique=True)
     description: Mapped[str] = mapped_column(String(512), nullable=False, default="")

syncmaster/db/models/queue.py

@@ -8,15 +8,15 @@
 from sqlalchemy.dialects.postgresql import TSVECTOR
 from sqlalchemy.orm import Mapped, mapped_column, relationship
 
-from syncmaster.db.mixins import DeletableMixin, ResourceMixin, TimestampMixin
+from syncmaster.db.mixins import ResourceMixin, TimestampMixin
 from syncmaster.db.models.base import Base
 
 if TYPE_CHECKING:
     from syncmaster.db.models.group import Group
     from syncmaster.db.models.transfer import Transfer
 
 
-class Queue(Base, ResourceMixin, TimestampMixin, DeletableMixin):
+class Queue(Base, ResourceMixin, TimestampMixin):
     name: Mapped[str] = mapped_column(String(128), nullable=False)
     slug: Mapped[str] = mapped_column(String(256), nullable=False, unique=True)
 

syncmaster/db/models/transfer.py

@@ -17,7 +17,7 @@
 from sqlalchemy.dialects.postgresql import TSVECTOR
 from sqlalchemy.orm import Mapped, declared_attr, mapped_column, relationship
 
-from syncmaster.db.mixins import DeletableMixin, ResourceMixin, TimestampMixin
+from syncmaster.db.mixins import ResourceMixin, TimestampMixin
 from syncmaster.db.models.base import Base
 from syncmaster.db.models.group import Group
 
@@ -29,7 +29,6 @@
 class Transfer(
     Base,
     ResourceMixin,
-    DeletableMixin,
     TimestampMixin,
 ):
     source_connection_id: Mapped[int] = mapped_column(

syncmaster/db/repositories/group.py

@@ -32,7 +32,7 @@ async def paginate_all(
         page_size: int,
         search_query: str | None = None,
     ) -> Pagination:
-        stmt = select(Group).where(Group.is_deleted.is_(False))
+        stmt = select(Group)
         if search_query:
             stmt = self._construct_vector_search(stmt, search_query)
 
@@ -73,7 +73,6 @@ async def paginate_for_user(
                 select(Group)
                 .where(
                     Group.owner_id == current_user_id,
-                    Group.is_deleted.is_(False),
                 )
                 .order_by(Group.name)
             )
@@ -104,7 +103,6 @@ async def paginate_for_user(
             .join(Group, UserGroup.group_id == Group.id)
             .where(
                 UserGroup.user_id == current_user_id,
-                Group.is_deleted.is_(False),
             )
             .order_by(Group.name)
         )
@@ -154,7 +152,7 @@ async def read_by_id(
         self,
         group_id: int,
     ) -> Group:
-        stmt = select(Group).where(Group.id == group_id, Group.is_deleted.is_(False))
+        stmt = select(Group).where(Group.id == group_id)
         try:
             result: ScalarResult[Group] = await self._session.scalars(stmt)
             return result.one()
@@ -186,7 +184,7 @@ async def update(
         description: str,
         owner_id: int,
     ) -> Group:
-        args = [Group.id == group_id, Group.is_deleted.is_(False)]
+        args = [Group.id == group_id]
         try:
             return await self._update(
                 *args,
@@ -278,7 +276,7 @@ async def get_member_paginate(
     async def delete(self, group_id: int) -> None:
         try:
             await self._delete(group_id)
-        except EntityNotFoundError as e:
+        except (EntityNotFoundError, NoResultFound) as e:
             raise GroupNotFoundError from e
 
     async def add_user(

syncmaster/db/repositories/queue.py

@@ -38,7 +38,7 @@ async def read_by_id(
     ) -> Queue:
         stmt = (
             select(Queue)
-            .where(Queue.id == queue_id, Queue.is_deleted.is_(False))
+            .where(Queue.id == queue_id)
             .options(selectinload(Queue.transfers))
             .options(selectinload(Queue.group))
         )
@@ -56,7 +56,6 @@ async def paginate(
         search_query: str | None = None,
     ):
         stmt = select(Queue).where(
-            Queue.is_deleted.is_(False),
             Queue.group_id == group_id,
         )
         if search_query:
@@ -77,7 +76,6 @@ async def update(
             queue = await self.read_by_id(queue_id=queue_id)
             return await self._update(
                 Queue.id == queue_id,
-                Queue.is_deleted.is_(False),
                 description=queue_data.description or queue.description,
             )
         except IntegrityError as e:

syncmaster/db/repositories/transfer.py

@@ -41,7 +41,6 @@ async def paginate(
         is_scheduled: bool | None = None,
     ) -> Pagination:
         stmt = select(Transfer).where(
-            Transfer.is_deleted.is_(False),
             Transfer.group_id == group_id,
         )
 
@@ -97,7 +96,6 @@ async def read_by_id(
             select(Transfer)
             .where(
                 Transfer.id == transfer_id,
-                Transfer.is_deleted.is_(False),
             )
             .options(selectinload(Transfer.queue))
         )
@@ -172,7 +170,6 @@ async def update(
                     strategy_params[key] = transfer.strategy_params[key]
             return await self._update(
                 Transfer.id == transfer.id,
-                Transfer.is_deleted.is_(False),
                 name=name or transfer.name,
                 description=description or transfer.description,
                 strategy_params=strategy_params,

syncmaster/scheduler/transfer_job_manager.py

@@ -26,7 +26,7 @@ def update_jobs(self, transfers: list[Transfer]) -> None:
             job_id = str(transfer.id)
             existing_job = self.scheduler.get_job(job_id)
 
-            if not transfer.is_scheduled or transfer.is_deleted:
+            if not transfer.is_scheduled:
                 if existing_job:
                     self.scheduler.remove_job(job_id)
                 continue

tests/test_unit/test_connections/test_copy_connection.py

@@ -44,7 +44,6 @@ async def test_maintainer_plus_can_copy_connection_with_delete_source(
         "user": "user",
         "password": "password",
     }
-    assert not current.is_deleted
 
     query_copy_not_exist = select(Connection).filter(
         Connection.group_id == empty_group.id,
@@ -73,7 +72,6 @@ async def test_maintainer_plus_can_copy_connection_with_delete_source(
     }
     assert result.status_code == 200
 
-    await session.refresh(group_connection.connection)
     query_prev_row = select(Connection).where(Connection.id == curr_id)
     result_prev_row = await session.scalars(query_prev_row)
     origin = result_prev_row.one()
@@ -100,8 +98,6 @@ async def test_maintainer_plus_can_copy_connection_with_delete_source(
         "user": "user",
         "password": "password",
     }
-    assert origin.is_deleted == is_delete_source
-    assert not new.is_deleted
     assert not creds_new
 
 
@@ -132,7 +128,6 @@ async def test_superuser_can_copy_connection(
         "user": "user",
         "password": "password",
     }
-    assert not current.is_deleted
 
     query_copy_not_exist = select(Connection).filter(
         Connection.group_id == empty_group.id,
@@ -189,8 +184,6 @@ async def test_superuser_can_copy_connection(
         "user": "user",
         "password": "password",
     }
-    assert origin.is_deleted == is_delete_source
-    assert not new.is_deleted
     assert not creds_new
 
 
@@ -280,7 +273,7 @@ async def test_not_in_both_groups_user_can_not_copy_connection(
     query_current_row = select(Connection).where(Connection.id == group_connection.id)
 
     result_current_row = await session.scalars(query_current_row)
-    current = result_current_row.one()
+    result_current_row.one()
 
     query_current_creds = select(AuthData).where(AuthData.connection_id == group_connection.id)
     result_current_creds = await session.scalars(query_current_creds)
@@ -291,7 +284,6 @@ async def test_not_in_both_groups_user_can_not_copy_connection(
         "user": "user",
         "password": "password",
     }
-    assert not current.is_deleted
 
     query_copy_not_exist = select(Connection).filter(
         Connection.group_id == empty_group.id,
@@ -338,7 +330,7 @@ async def test_groupless_user_can_not_copy_connection(
     query_current_row = select(Connection).where(Connection.id == group_connection.id)
 
     result_current_row = await session.scalars(query_current_row)
-    current = result_current_row.one()
+    result_current_row.one()
 
     query_current_creds = select(AuthData).where(AuthData.connection_id == group_connection.id)
     result_current_creds = await session.scalars(query_current_creds)
@@ -349,7 +341,6 @@ async def test_groupless_user_can_not_copy_connection(
         "user": "user",
         "password": "password",
     }
-    assert not current.is_deleted
 
     query_copy_not_exist = select(Connection).filter(
         Connection.group_id == empty_group.id,

tests/test_unit/test_groups/test_delete_group_by_id.py

@@ -15,8 +15,7 @@ async def test_only_superuser_can_delete_group(
     session: AsyncSession,
 ):
     # Arrange
-    group = await session.get(Group, empty_group.group.id)
-    assert not group.is_deleted
+    await session.get(Group, empty_group.group.id)
 
     # Act
     result = await client.delete(
@@ -33,9 +32,6 @@ async def test_only_superuser_can_delete_group(
         "message": "Group was deleted",
     }
 
-    await session.refresh(group)
-    assert group.is_deleted
-
 
 async def test_not_superuser_cannot_delete_group(
     client: AsyncClient,

tests/test_unit/test_queue/test_delete_queue.py

@@ -31,11 +31,6 @@ async def test_maintainer_plus_can_delete_queue(
     }
     assert result.status_code == 200
 
-    queue = await session.get(Queue, group_queue.id)
-    await session.refresh(queue)
-
-    assert queue.is_deleted
-
 
 async def test_superuser_can_delete_queue(
     client: AsyncClient,
@@ -57,11 +52,6 @@ async def test_superuser_can_delete_queue(
     }
     assert result.status_code == 200
 
-    queue = await session.get(Queue, group_queue.id)
-    await session.refresh(queue)
-
-    assert queue.is_deleted
-
 
 async def test_groupless_user_cannot_delete_queue(
     client: AsyncClient,

tests/test_unit/test_scheduler/test_transfer_job_manager.py

@@ -45,8 +45,6 @@ async def test_modifying_existing_jobs(transfer_job_manager: TransferJobManager,
 @pytest.mark.parametrize(
     "transfer_attr, expected_state, is_existing_job",
     [
-        ("is_deleted", True, True),
-        ("is_deleted", True, False),
         ("is_scheduled", False, True),
         ("is_scheduled", False, False),
     ],

tests/test_unit/test_transfers/test_delete_transfer.py

@@ -16,9 +16,7 @@ async def test_maintainer_plus_can_delete_group_transfer(
 ):
     # Arrange
     user = group_transfer.owner_group.get_member_of_role(role_maintainer_plus)
-    transfer = await session.get(Transfer, group_transfer.id)
-
-    assert not transfer.is_deleted
+    await session.get(Transfer, group_transfer.id)
 
     # Act
     result = await client.delete(
@@ -33,8 +31,6 @@ async def test_maintainer_plus_can_delete_group_transfer(
         "status_code": 200,
         "message": "Transfer was deleted",
     }
-    await session.refresh(transfer)
-    assert transfer.is_deleted
 
 
 async def test_superuser_can_delete_transfer(
@@ -56,8 +52,6 @@ async def test_superuser_can_delete_transfer(
         "status_code": 200,
         "message": "Transfer was deleted",
     }
-    await session.refresh(group_transfer.transfer)
-    assert group_transfer.is_deleted
 
 
 async def test_groupless_user_cannot_delete_transfer(
@@ -106,8 +100,6 @@ async def test_developer_or_below_cannot_delete_transfer(
         },
     }
     assert result.status_code == 403
-    await session.refresh(group_transfer.transfer)
-    assert not group_transfer.is_deleted
 
 
 async def test_group_member_cannot_delete_other_group_transfer(