Skip to content

Fix motion history entry with deleted meeting #3222

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hjanott merged 7 commits into from
Dec 4, 2025
Merged

Fix motion history entry with deleted meeting #3222

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fa4733e
fix bug and extend test
hjanott Nov 28, 2025
ca4c6e5
add migration plus test
hjanott Nov 28, 2025
d0f7223
update json files
hjanott Nov 28, 2025
67e182e
requested changes
hjanott Dec 1, 2025
c9f6898
remove wrong folder
hjanott Dec 1, 2025
d2c5215
remove unused function
hjanott Dec 1, 2025
487bfa4
Merge branch 'main' into fix_history_entry_meeting_id
hjanott Dec 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion data/example-data.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"_migration_index": 72,
"_migration_index": 73,
"gender":{
"1":{
"id": 1,
Expand Down
2 changes: 1 addition & 1 deletion data/initial-data.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"_migration_index": 72,
"_migration_index": 73,
"gender":{
"1":{
"id": 1,
Expand Down
1 change: 1 addition & 0 deletions global/meta
Open in desktop
Submodule meta added at 1cd0ee
4 changes: 4 additions & 0 deletions openslides_backend/action/action.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,10 @@ def _build_write_request(
for collection, models in data.items()
for id_, date in models.items()
if (meeting_id := date.get("meeting_id"))
and not any(
f"meeting/{meeting_id}" == event["fqid"]
for event in events_by_type[EventType.Delete]
)
},
touched_fqids - deleted_fqids,
)
Expand Down
66 changes: 66 additions & 0 deletions openslides_backend/migrations/migrations/0072_unset_deleted_meeting_id_in_history_entry.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
from datastore.migrations import BaseModelMigration
from datastore.writer.core.write_request import BaseRequestEvent, RequestUpdateEvent

from ...shared.filters import And, FilterOperator, Or


class Migration(BaseModelMigration):
"""
This migration removes mistakenly created history collection entries
"""

target_migration_index = 73

def migrate_models(self) -> list[BaseRequestEvent] | None:
if self.reader.is_in_memory_migration:
return None
events: list[BaseRequestEvent] = []
filter_ = And(
FilterOperator("original_model_id", "%=", "motion/%"),
Copy link
Copy Markdown
Member

@luisa-beerboom luisa-beerboom Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really only for motions?
Why not just do it for all collections?
Shouldn't affect the rest of the code, right?

Copy link
Copy Markdown
Member Author

@hjanott hjanott Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user history doesn't use the meeting_id and the assignment isn't affected as shown by the alteration of that meeting delete test. Doing it though for extra security as this has just little affect on the request.

FilterOperator("meeting_id", "!=", None),
FilterOperator("meta_deleted", "!=", True),
)
potentially_broken_entries = self.reader.filter(
"history_entry", filter_, ["meeting_id"]
)
potentially_affected_meetings = {
meeting_id
for entry in potentially_broken_entries.values()
if (meeting_id := entry.get("meeting_id"))
}
if potentially_broken_entries:
deleted_meetings = self.reader.filter(
"meeting",
And(
FilterOperator("meta_deleted", "=", True),
Or(
*[
FilterOperator("id", "=", id_)
for id_ in potentially_affected_meetings
]
),
),
)
Copy link
Copy Markdown
Member

@luisa-beerboom luisa-beerboom Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also just do a get_many on the meeting ids.
Deleted meetings will be filtered out and you won't potentially have a filter with 100s of lines of conditions.
Like this I am afraid the datastore request may break with too large of an instance.

Copy link
Copy Markdown
Member Author

@hjanott hjanott Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. That was something I was afraid of, too. I returned the commented out code instead and altered it a little.

events = [
*[
RequestUpdateEvent(
f"history_entry/{id_}", fields={"meeting_id": None}
)
for id_, entry in potentially_broken_entries.items()
if entry["meeting_id"] in deleted_meetings
],
]
# meetings = self.reader.get_many(
# [
# GetManyRequestPart("meeting", list(affected_meetings), ["meta_deleted"])
# ]
# ).get("meeting", {})
# events = [
# *[
# RequestUpdateEvent(f"history_entry/{id_}", fields={"meeting_id": None})
# for id_, entry in potentially_broken_entries.items()
# if meetings[entry["meeting_id"]].get("meta_deleted") != True
# ],
# ]
Copy link
Copy Markdown
Member

@luisa-beerboom luisa-beerboom Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete

Copy link
Copy Markdown
Member Author

@hjanott hjanott Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using this now but taking advantage of the fact that no deleted meetings are returned.


return events
111 changes: 111 additions & 0 deletions tests/system/action/meeting/test_delete.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,9 +232,47 @@ def test_delete_meeting_with_relations(self) -> None:
"group/11": {
"meeting_user_ids": [2],
},
"assignment/53": {"history_entry_ids": [113], "meeting_id": 1},
"motion/53": {"history_entry_ids": [112], "meeting_id": 1},
"history_position/148": {
"user_id": 1,
"entry_ids": [112],
"timestamp": 1761760881,
"original_user_id": 1,
},
"history_entry/112": {
"entries": [
"Motion created",
],
"meeting_id": 1,
"position_id": 148,
"original_model_id": "motion/53",
"model_id": "motion/53",
},
"history_position/149": {
"user_id": 1,
"entry_ids": [113],
"timestamp": 1761760881,
"original_user_id": 1,
},
"history_entry/113": {
"entries": [
"Candidate removed",
"Candidate removed",
"Candidate removed",
"Ballot deleted",
],
"meeting_id": 1,
"position_id": 149,
"original_model_id": "assignment/53",
"model_id": "assignment/53",
},
"meeting/1": {
"user_ids": [2],
"meeting_user_ids": [2],
"motion_ids": [53],
"assignment_ids": [53],
"relevant_history_entry_ids": [112, 113],
},
}
)
Expand Down Expand Up @@ -282,6 +320,79 @@ def test_delete_meeting_with_relations(self) -> None:
self.assert_model_deleted(
"meeting_user/2", {"meeting_id": 1, "user_id": 2, "group_ids": [11]}
)
self.assert_model_deleted("motion/53")
self.assert_model_deleted("assignment/53")
self.assert_model_exists(
"history_position/148",
{
"user_id": 1,
"entry_ids": [112],
"timestamp": 1761760881,
"original_user_id": 1,
},
)
self.assert_model_exists(
"history_entry/112",
{
"entries": ["Motion created"],
"position_id": 148,
"original_model_id": "motion/53",
"model_id": None,
"meeting_id": None,
},
)
self.assert_model_exists(
"history_position/149",
{
"user_id": 1,
"entry_ids": [113],
"timestamp": 1761760881,
"original_user_id": 1,
},
)
self.assert_model_exists(
"history_entry/113",
{
"entries": [
"Candidate removed",
"Candidate removed",
"Candidate removed",
"Ballot deleted",
],
"position_id": 149,
"original_model_id": "assignment/53",
"model_id": None,
"meeting_id": None,
},
)
self.assert_model_exists(
"history_position/150",
{
"user_id": 1,
"entry_ids": [114, 115],
"original_user_id": 1,
},
)
self.assert_model_exists(
"history_entry/114",
{
"meeting_id": None,
"entries": ["Participant removed from meeting {}", "meeting/1"],
"model_id": "user/2",
"position_id": 150,
},
)
self.assert_model_exists(
"history_entry/115",
{
"meeting_id": None,
"entries": ["Motion deleted"],
"original_model_id": "motion/53",
"model_id": None,
"position_id": 150,
},
)
self.assert_model_not_exists("history_entry/116")

def test_delete_archived_meeting(self) -> None:
self.set_models(
Expand Down
Loading
Loading