Allow forwarding motions with attachments #2949

vkrasnovyd · 2025-05-16T10:41:18Z

luisa-beerboom

I already wrote some of this in my code reviews, but there are some new points as well:

Amendment attachments should also be forwarded, so please make sure to pass that information into every create_forwarded_amendment payload
If multiple motions (and amendments!) use the same mediafile as an attachment, is it duplicated multiple times? You should write a test and, if yes, look into avoiding that. We shouldn't unnecessarily clutter the media service.
From what I can see the ids of your mediafiles and motions usually form a perfect sequence in the tests, just to ensure that the code is actually matching the content up correctly, you should probably write a test with some random order of model to attachments (smth like motion 1 -> [attachment 4]; 2 -> [1, 6]; 3 -> [7,1]; 4 -> [5]; 5 -> [1]), just to ensure it actually works
You are using events to write the mediafile and meeting_mediafile models, which can be a bit unsafe because, if you write into events directly, the back relations will not be filled automatically, meaning that you'll have to add them manually. Best way to test if you're doing that is probably by constructing a minimum viable dataset with at least one orga- and meeting mediafile as attachments of a motion, forwarding that motion with attachments and then running the datastore checker to see if the database content is still consistent. The latter can be achieved by using the Checker class from openslides-backend/openslides_backend/models/checker.py, or alternatively seeing if a call to the check_database_all presenter comes back positive.

luisa-beerboom · 2025-05-19T13:40:28Z

openslides_backend/action/actions/motion/base_create_forwarded.py

+            new_meeting_mediafile = {
+                "id": target_meeting_mediafile_id,
+                "is_public": origin_meeting_mediafile["is_public"],
+                "mediafile_id": target_mediafile_id,
+                "meeting_id": meeting_id,
+                "meta_new": True,
+                "attachment_ids": [
+                    fqid_from_collection_and_id("motion", instance["id"])
+                ],
+            }


Even if they're just empty lists. You should probably set access_group_ids and inherited_access_group_ids. These are called on in various parts of the mediafile-related backend code and these calls are probably not necessarily safe.

luisa-beerboom · 2025-05-19T13:58:59Z

openslides_backend/action/actions/motion/base_create_forwarded.py

+            meeting_id: {
+                "meeting_mediafile_ids": meeting.get("meeting_mediafile_ids", [])
+                + list(target_meeting_mediafiles)
+            }


You create the back relations of the meeting_mediafiles here, but I don't see where you create them for the newly created mediafiles

luisa-beerboom · 2025-05-19T14:11:04Z

tests/system/action/motion/test_create_forwarded.py

+                "mimetype": "text/plain",
+                "is_directory": True,


directories wouldn't have a mimetype

tests/system/action/motion/test_create_forwarded.py

luisa-beerboom · 2025-05-19T14:48:03Z

openslides_backend/action/actions/motion/create_forwarded_amendment.py

@@ -52,3 +52,6 @@ def create_amendments(self, amendment_data: ActionData) -> ActionResults | None:

    def should_forward_amendments(self, instance: dict[str, Any]) -> bool:
        return True
+
+    def should_forward_attachments(self, instance: dict[str, Any]) -> bool:
+        return False


If the main motion is forwarded with attachments, any amendments forwarded along with it should also be forwarded with attachments. If they are the same attachment, try looking into the possibility of using the same newly created mediafiles, instead of duplicating the same mediafile over and over again. Perhaps the mediafile stuff should be moved to get_updated_instances for that purpose and the amendment code should also receive a map of already duplicated mediafiles to the duplicate ids.

luisa-beerboom · 2025-05-19T15:45:23Z

Alternatively use actions instead of writing directly into events, write a new internal one for cloning a mediafile into another meeting if necessary, that's going to be a lot easier and safer and won't require such a massive test.

luisa-beerboom

Did not look at the tests yet, comments so far are purely on the function code.

The duplicate_mediafile function is pretty long, perhaps look into splitting some parts off into concisely named helper functions

luisa-beerboom · 2025-05-26T16:19:46Z

openslides_backend/action/actions/motion/base_create_forwarded.py

+        Duplicates the meeting-wide mediafile models that must be forwarded to
+        the target motion along with the corresponding mediafiles in the
+        mediaservice.
+
+        Builds meeting_mediafiles replace map for each meeting where the motions
+        are forwarded.


You seem to be doing this not only for the motions called in this action, but also for the amendments of those motions. These aren't necessarily forwarded even if with_amendments is set, however. To be forwarded an amendment also needs to be in a state with allow_amendment_forwarding set. I don't see you checking for that in this function, so I assume that if an amendment doesn't have this kind of state, you'll still forward the mediafiles, causing the mediafiles to be ported even if the amendment they are attached to isn't.

Also, you call this function in perform. Please take note that this is a base class for both the main action motion.create_forwarded and the sub-action motion.create_forwarded_amendment. So this function is going to be called both in the main action and in every single call of create_forwarded_amendment (potentially once for every call of the main actions update_instance).
To eliminate the chance of accidentally duplicating mediafiles multiple times into the same meeting, perhaps move this method to the create_forwarded and simply pass the map to the create_forwarded_amendment as a parameter when calling it. Alternatively you could leave the function here and just create the map for the main action motions in create_forwarded, pass it to the amendment sub-action, have it update the map with all the additional mediafiles it created with this same function and then have it return the map as a return object that the create_forwarded action can use for the next create_forwarded_amendment call. The latter option also gets rid of the requirement to check for that state, since the amendments given to create_forwarded_amendment are already filtered to be the ones that should be forwarded.

Adjusted the expected_models for 2 tests to match the order in which ids are assigned to new instances. Current logic: - Attachments of the lead motions are proceesed first, then - attachments of their amendments. - Within each of these stages, ids of the origin attachments are sorted in the ascending order. Ids for the corresponding meeting_mediafiles and mediafiles are assigned in this order. - Each meeting_mediafile has amendments before the lead_motions in the attachment_ids field.

vkrasnovyd · 2025-05-30T08:07:18Z

tests/system/action/motion/test_create_forwarded.py

+                "meeting_id": 2,
+                "mediafile_id": 21,
+                "is_public": True,
+                "attachment_ids": ["motion/21", "motion/20"],


@luisa-beerboom is it possible and reasonable to sort the value in the attachment_ids field of the new meeting_mediafiles after creating the models so the attachments are listed in ascending order (by id)? If yes, what is the correct place for this logic? Currently the amendments are listed before their lead motions which seems to be not intuitive to me

I wouldn't say there is much of a necessity to keep this sorted. Based on my knowledge I can say: As far as the client is concerned this field specifically is not even used. We only have it because our system requires back relations. Furthermore even if it was, it's pretty established that relation lists are unordered and that if the client wants things ordered it'll have to do so itself, with the backend only aiding that when necessary through weight fields to allow specific ordering. Any other sorting of relation lists in the backend is usually coincidental.

But just for the sake of answering the question:
There is no standard way of doing that as back-relation lists are usually generated automatically by the relation handling in the backend and the event handling in the datastore. If you really wanted it sorted, the most practical way is probably to overwrite key parts of the relation handling, which is generally not recommended because of the instability (specifically: ensure that the write events generated for the meeting_mediafile are replaced with entire list replacement events containing the new order).
Unless that is done you can generally expect lists that were calculated as back-relations to be ordered according to the order in which they were processed. Based on that I'd say keeping the motions attachment list sorted while also keeping the meeting_mediafiles attachment list sorted in the same way as before is not all too plausible.

luisa-beerboom · 2025-06-02T10:03:48Z

openslides_backend/action/actions/motion/base_create_forwarded.py


+    def _fetch_and_map_mediafiles(
+        self, mm_id_target_meeting_ids_map: dict[int, set[int]]
+    ) -> tuple[dict, dict[int, dict[int, dict[str, Any]]], list[dict[str, Any]]]:


Please add the format of the first dict in the tuple

luisa-beerboom · 2025-06-02T10:39:46Z

openslides_backend/action/actions/motion/create_forwarded_amendment.py

        return self.execute_other_action(MotionCreateForwardedAmendment, amendment_data)

    def should_forward_amendments(self, instance: dict[str, Any]) -> bool:
        return True

    def should_forward_attachments(self, instance: dict[str, Any]) -> bool:
-        return True
+        return self.with_attachments


This is the same as in motion.create_forwarded, so this code may as well be moved to the base class

I've kept it like that on purpose. It is done like that to avoid mypy error "BaseMotionCreateForwarded" has no attribute "with_attachments".

I can not move initializing of self.with_attachments to the base class because in MotionCreateForwarded its value must be used after super().update_instance(instance) and in the MotionCreateForwardedAmendment - before it.

luisa-beerboom · 2025-06-02T11:51:33Z

openslides_backend/action/actions/motion/base_create_forwarded.py

+    @classmethod
+    def reset_forwarded_state(cls) -> None:
+        cls.forwarded_attachments.clear()
+        cls.meeting_mediafile_replace_map.clear()


This method clears two class attribute the contents of which, to my understanding, are vital to the correct functioning of this action complex.
I am a bit worried about what would happen if a second motion.create_forwarded action is called while another is still running. I don't think the threads are isolated in a way that would cause the class attributes to not be shared. As such I am afraid that the second call of the action would clear the first calls replace map, causing the first action to fail or, even worse, write bogus data in such a case.

If you are zealous enough, feel free to read through the code in action_worker.py, action_handler.pyandaction_view.pyand assess whether my worry is unfounded. Otherwise I'd suggest you use instance attributes instead and pass them back and forth internally through themotion.create_forwarded_amendment-payload and -result element (the latter is created via the create_action_result_element` method of the Action class).

luisa-beerboom · 2025-06-02T12:27:19Z

tests/system/action/motion/test_create_forwarded.py

+        self.base_test_forward_with_attachments_false(
+            origin_mediafiles=[{"mediafile_id": 1, "owner_meeting_id": 1}]
+        )


For readabilities sake it would be best to move helper methods like base_test_forward_with_attachments_false above the proper test methods that use them

luisa-beerboom · 2025-06-02T14:14:20Z

tests/system/action/motion/test_create_forwarded.py

+                "meeting_id": 2,
+                "mediafile_id": 21,
+                "is_public": True,
+                "attachment_ids": ["motion/21", "motion/20"],


I wouldn't say there is much of a necessity to keep this sorted. Based on my knowledge I can say: As far as the client is concerned this field specifically is not even used. We only have it because our system requires back relations. Furthermore even if it was, it's pretty established that relation lists are unordered and that if the client wants things ordered it'll have to do so itself, with the backend only aiding that when necessary through weight fields to allow specific ordering. Any other sorting of relation lists in the backend is usually coincidental.

But just for the sake of answering the question:
There is no standard way of doing that as back-relation lists are usually generated automatically by the relation handling in the backend and the event handling in the datastore. If you really wanted it sorted, the most practical way is probably to overwrite key parts of the relation handling, which is generally not recommended because of the instability (specifically: ensure that the write events generated for the meeting_mediafile are replaced with entire list replacement events containing the new order).
Unless that is done you can generally expect lists that were calculated as back-relations to be ordered according to the order in which they were processed. Based on that I'd say keeping the motions attachment list sorted while also keeping the meeting_mediafiles attachment list sorted in the same way as before is not all too plausible.

Moved base methods above the tests that use them. No logic changes were made.

Allow forwarding motions with attachments

c6f687f

vkrasnovyd added this to the 4.3 milestone May 16, 2025

vkrasnovyd added feature Onnes labels May 16, 2025

Apply linter

f317a52

vkrasnovyd requested a review from luisa-beerboom May 16, 2025 12:06

vkrasnovyd assigned luisa-beerboom May 16, 2025

luisa-beerboom requested changes May 19, 2025

View reviewed changes

luisa-beerboom assigned vkrasnovyd and unassigned luisa-beerboom May 19, 2025

Elblinator mentioned this pull request May 21, 2025

Add checkbox to motion forward dialog OpenSlides/openslides-client#4852

Open

vkrasnovyd added 2 commits May 21, 2025 17:00

Remove mimetype from directory mediafiles in tests

04a81b6

Replace events generation with actions calls

5eb0e2e

vkrasnovyd force-pushed the 2889-motion-forward-with-attachments branch from dcd5084 to 5eb0e2e Compare May 22, 2025 08:48

vkrasnovyd added 3 commits May 22, 2025 11:16

Refactor mediafiles forwarding logic

99628de

Prevent redundant mediafile copies in batch-forwarding

77ef05b

Refactor and add new tests

aa941ff

vkrasnovyd requested a review from luisa-beerboom May 26, 2025 09:50

vkrasnovyd assigned luisa-beerboom and unassigned vkrasnovyd May 26, 2025

luisa-beerboom requested changes May 26, 2025

View reviewed changes

vkrasnovyd added 6 commits May 26, 2025 21:56

Add tests for the requested changes

ff17585

Refactor attachments forwarding

bb36780

Add test for amendment with amendment

8e85c0b

Correctly forward attachments of nested amendments

f6e52d2

Forward with_attachments to nested amendments

5d59e00

vkrasnovyd commented May 30, 2025

View reviewed changes

vkrasnovyd requested a review from luisa-beerboom May 30, 2025 08:07

luisa-beerboom requested changes Jun 2, 2025

View reviewed changes

luisa-beerboom assigned vkrasnovyd and unassigned luisa-beerboom Jun 3, 2025

vkrasnovyd added 4 commits June 6, 2025 10:12

Reorder tests

a8e86d9

Moved base methods above the tests that use them. No logic changes were made.

Update type annotation

293c862

Pass maps to the actions instead of using class properties

75d55fa

Update motion.create_forwarded_amendment docs

b7bd305

vkrasnovyd requested a review from luisa-beerboom June 6, 2025 15:07

vkrasnovyd assigned luisa-beerboom and unassigned vkrasnovyd Jun 6, 2025

Allow forwarding motions with attachments #2949

Are you sure you want to change the base?

Allow forwarding motions with attachments #2949

Uh oh!

Conversation

vkrasnovyd commented May 16, 2025

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luisa-beerboom commented May 19, 2025

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!

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!