[rel-db] Fix base tests and deletion #3119

hjanott · 2025-08-27T12:45:19Z

Everything related to migrations and organization import was left out or still won't work.
In equal fields tests generic relation lists to generic relations lists cannot be checked as this is not yet implemented by the backend/meta.
Two tests treating locked fields behaviour were unclear to me if they are still needed that way. I require feedback on that.
Needs OpenSlides/openslides-meta#306

luisa-beerboom

Haven't reviewed everything yet, but here's a start

luisa-beerboom · 2025-08-27T14:21:57Z

openslides_backend/action/actions/meeting/delete.py

+                self.execute_other_action(TopicDelete, [{"id": topic_fqid}])
+        return instance


Why is this necessary? Shouldn't topics be cascade deleted?

They are cascade deleted but the problem lies in the fact that the list_of_speakers content_object_id will be set to None if not deleted as one of the first models. That leads later to the topics list_of_speakers_id to also be set to None by the relation handling, which is not legal as it is a required field. By first deleting the topic this is no problem any longer.

the fact that the content_object_id will be set to None if not deleted as one of the first models but the content object will not be deleted before that.

I don't understand what this is supposed to refer to. Please rephrase it.

Rephrased it. I'm currently thinking if this would better be solved in the los delete action. But there also seems to be a problem with the other referenced collections like motion_block.

luisa-beerboom · 2025-08-27T14:47:52Z

openslides_backend/action/generics/delete.py

        this_fqid = fqid_from_collection_and_id(self.model.collection, instance["id"])
+
+        # Have been here. Seen it all.


luisa-beerboom · 2025-08-27T15:25:42Z

openslides_backend/services/database/extended_database.py

@@ -118,6 +120,14 @@ def apply_changed_model(
        if "id" not in self._changed_models[collection][id_]:
            self._changed_models[collection][id_]["id"] = id_

+    def apply_to_be_deleted(self, fqid: FullQualifiedId) -> None:
+        """Meaning both: to be deleted in the future and the past."""


What does this comment mean? (The same question also concerns the other three below it that have a similar format)

The model will be marked as to be deleted. This will not be undone when it is marked as deleted.

Then write that.
Please also do the same for the three other places where you used the same formulation.

tests/system/action/test_action_command_format.py

luisa-beerboom

Here's the rest of the review

luisa-beerboom · 2025-08-28T11:03:32Z

tests/system/action/test_action_command_format.py


 from .base import BaseActionTestCase


 class GeneralActionCommandFormat(BaseActionTestCase):
+    # TODO later: either delete commented out code or implement locked fields


It would be better to write this TODO above the commented-out code to avoid misunderstandings in case something else is also commented out later for a different reason

tests/system/action/test_delete_cascade.py

luisa-beerboom · 2025-08-28T13:07:12Z

tests/system/action/test_delete_cascade.py

+    @classmethod
+    def setUpClass(cls) -> None:
+        super().setUpClass()
+        create_table_view(yml)
+
+    @classmethod
+    def tearDownClass(cls) -> None:
+        super().tearDownClass()
+        with get_new_os_conn() as conn:
+            with conn.cursor() as curs:
+                curs.execute(
+                    f"""
+                    DROP TABLE {collection_a}_t CASCADE;
+                    DROP TABLE {collection_b}_t CASCADE;
+                    DROP TABLE {collection_c}_t CASCADE;
+                    DROP TABLE {collection_d}_t CASCADE;
+                    """
+                )
+
+    def tearDown(self) -> None:
+        super().tearDown()
+        with self.connection.cursor() as curs:
+            curs.execute(
+                f"""
+                TRUNCATE TABLE {collection_a}_t RESTART IDENTITY CASCADE;
+                TRUNCATE TABLE {collection_b}_t RESTART IDENTITY CASCADE;
+                TRUNCATE TABLE {collection_c}_t RESTART IDENTITY CASCADE;
+                TRUNCATE TABLE {collection_d}_t RESTART IDENTITY CASCADE;
+                """
+            )


I see this exact code in test_create_relation and test_equal_fields_check as well. Perhaps you could reduce boilerplate by creating a new class that does this?

Good idea. Will do.

luisa-beerboom · 2025-08-28T13:14:54Z

tests/system/action/test_equal_fields_check.py

+    @classmethod
+    def setUpClass(cls) -> None:
+        super().setUpClass()
+        create_table_view(yml)
+
+    @classmethod
+    def tearDownClass(cls) -> None:
+        super().tearDownClass()
+        with get_new_os_conn() as conn:
+            with conn.cursor() as curs:
+                curs.execute(
+                    f"""
+                    DROP TABLE nm_fake_model_ef_b_c_ids_fake_model_ef_c_t CASCADE;
+                    DROP TABLE {collection_a}_t CASCADE;
+                    DROP TABLE {collection_b}_t CASCADE;
+                    DROP TABLE {collection_c}_t CASCADE;
+                    """
+                )
+
+    def tearDown(self) -> None:
+        super().tearDown()
+        with self.connection.cursor() as curs:
+            curs.execute(
+                f"""
+                TRUNCATE TABLE nm_fake_model_ef_b_c_ids_fake_model_ef_c_t RESTART IDENTITY CASCADE;
+                TRUNCATE TABLE {collection_a}_t RESTART IDENTITY CASCADE;
+                TRUNCATE TABLE {collection_b}_t RESTART IDENTITY CASCADE;
+                TRUNCATE TABLE {collection_c}_t RESTART IDENTITY CASCADE;
+                """
+            )


Same here as what I already said in test_delete_cascade

luisa-beerboom · 2025-08-28T13:18:56Z

tests/system/action/test_update_relation.py

@@ -42,13 +76,46 @@ class FakeModelURAUpdateAction(UpdateAction):


 class TestUpdateRelation(BaseActionTestCase):
-    def test_set_to_null(self) -> None:
+    @classmethod


Same kind of setUpClass, tearDownClass and tearDown functions as in test_delete_cascade

vkrasnovyd · 2025-09-04T13:03:41Z

openslides_backend/action/generics/delete.py

-        self.datastore.apply_changed_model(this_fqid, DeletedModel())
-        for field in self.model.get_relation_fields():
+        delete_actions: list[tuple[type[Action], ActionData]] = []
+        for field_name in sorted(db_instance):


Is there a reason why these fields have to be sorted?

There is no reason anymore. So I removed that.

vkrasnovyd · 2025-09-04T13:06:31Z

tests/system/action/test_action_command_format.py

-        self.assertCountEqual(
-            write_requests[0].locked_fields.keys(),
-            [
-                "group/weight",
-                "meeting/1/group_ids",
-            ],
-        )
+        # self.assertCountEqual(
+        #     write_requests[0].locked_fields.keys(),
+        #     [
+        #         "group/weight",
+        #         "meeting/1/group_ids",
+        #     ],
+        # )


Are this and next 3 commented asserts not needed anymore or you were planning to replace them? If it's something we want to observe in the future, maybe it's worth adding a comment about the reasons of commenting in this file.

* error handling on 1_1 not null constraint * improve deletion logic * type handling for organization import * try_get_field as class method * type handling for Checker * add list_of_speakers to create_motion * various improvements

base test and fix deletion

f17aacc

hjanott added this to the 4.x milestone Aug 27, 2025

hjanott requested review from luisa-beerboom and vkrasnovyd August 27, 2025 12:45

hjanott assigned luisa-beerboom and vkrasnovyd Aug 27, 2025

hjanott added the tests label Aug 27, 2025

change field type in equal fields tests

c1ebfc9

luisa-beerboom requested changes Aug 27, 2025

View reviewed changes

luisa-beerboom assigned hjanott and unassigned luisa-beerboom Aug 27, 2025

check group weight in action command test

bffbb40

luisa-beerboom requested changes Aug 28, 2025

View reviewed changes

generic base tests generically

cd5f505

vkrasnovyd reviewed Sep 4, 2025

View reviewed changes

hjanott added 3 commits September 10, 2025 17:31

* readd json files

793d51b

* error handling on 1_1 not null constraint * improve deletion logic * type handling for organization import * try_get_field as class method * type handling for Checker * add list_of_speakers to create_motion * various improvements

add sqlfluff to config and requirements

21ac611

add missing test data to already fixed tests

def9f58

hjanott requested review from vkrasnovyd and luisa-beerboom September 10, 2025 17:15

hjanott assigned luisa-beerboom and unassigned hjanott Sep 10, 2025

hjanott added 2 commits September 11, 2025 11:27

lint

ef7976c

adapt meeting import for class method also

d16cdf5

		self.execute_other_action(TopicDelete, [{"id": topic_fqid}])
		return instance

		this_fqid = fqid_from_collection_and_id(self.model.collection, instance["id"])

		# Have been here. Seen it all.

[rel-db] Fix base tests and deletion #3119

Are you sure you want to change the base?

[rel-db] Fix base tests and deletion #3119

Uh oh!

Conversation

hjanott commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luisa-beerboom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hjanott Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

luisa-beerboom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hjanott commented Aug 27, 2025 •

edited

Loading

hjanott Aug 27, 2025 •

edited

Loading