Skip to content
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.

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

Deleted participants stay present in meeting #2730

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 global/data/example-data.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"_migration_index": 62,
"_migration_index": 63,
"gender":{
"1":{
"id": 1,
Expand Down
2 changes: 1 addition & 1 deletion global/data/initial-data.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"_migration_index": 62,
"_migration_index": 63,
"gender":{
"1":{
"id": 1,
Expand Down
4 changes: 3 additions & 1 deletion openslides_backend/action/actions/meeting_user/delete.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,11 @@ def get_history_information(self) -> HistoryInformation | None:

def update_instance(self, instance: dict[str, Any]) -> dict[str, Any]:
meeting_user = self.datastore.get(
fqid_from_collection_and_id("meeting_user", instance["id"]), ["speaker_ids"]
fqid_from_collection_and_id("meeting_user", instance["id"]),
["speaker_ids", "user_id", "meeting_id"],
)
speaker_ids = meeting_user.get("speaker_ids", [])
self.conditionally_delete_speakers(speaker_ids)
self.remove_presence(meeting_user["user_id"], meeting_user["meeting_id"])

return super().update_instance(instance)
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from ....shared.patterns import fqid_from_collection_and_id
from ...action import Action
from ..speaker.delete import SpeakerDeleteAction
from .set_present import UserSetPresentAction


class ConditionalSpeakerCascadeMixinHelper(Action):
Expand Down Expand Up @@ -41,6 +42,18 @@ def conditionally_delete_speakers(self, speaker_ids: list[int]) -> None:
[{"id": speaker["id"]} for speaker in speakers_to_delete],
)

def remove_presence(self, user_id: int, meeting_id: int) -> None:
self.execute_other_action(
UserSetPresentAction,
[
{
"id": user_id,
"meeting_id": meeting_id,
"present": False,
}
],
)


class ConditionalSpeakerCascadeMixin(ConditionalSpeakerCascadeMixinHelper):
"""
Expand All @@ -65,6 +78,7 @@ def update_instance(self, instance: dict[str, Any]) -> dict[str, Any]:
for speaker_id in val.get("speaker_ids", [])
]
self.conditionally_delete_speakers(speaker_ids)
self.remove_presence(instance["id"], removed_meeting_id)

return super().update_instance(instance)

Expand Down
6 changes: 2 additions & 4 deletions openslides_backend/action/actions/user/user_mixins.py
Original file line number Diff line number Diff line change
Expand Up @@ -142,10 +142,8 @@ def strip_field(self, field: str, instance: dict[str, Any]) -> None:
def check_meeting_and_users(
self, instance: dict[str, Any], user_fqid: FullQualifiedId
Comment on lines 142 to 143
Copy link
Member

Choose a reason for hiding this comment

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

As far as I looked at the source code check_meeting_and_users is called every time if a user action with the mixin is called. I think, you want to call a presence cleaning only if the user is removed from an meeting or deleted.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've found that at the same condition the 'ConditionalSpeakerCascadeMixin' is executing its 'update_instance'. So I moved it there. I propose to rename this mixin to 'ConditionalMeetingUserCascadeMixin'

) -> None:
if instance.get("meeting_id") is not None:
self.datastore.apply_changed_model(
user_fqid, {"meeting_id": instance.get("meeting_id")}
)
if (meeting_id := instance.get("meeting_id")) is not None:
self.datastore.apply_changed_model(user_fqid, {"meeting_id": meeting_id})

def meeting_user_set_data(self, instance: dict[str, Any]) -> None:
meeting_user_data = {}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
from collections import defaultdict

from datastore.migrations import BaseModelMigration
from datastore.reader.core import GetManyRequestPart
from datastore.writer.core import BaseRequestEvent, RequestUpdateEvent

from openslides_backend.shared.patterns import fqid_from_collection_and_id

from ...shared.filters import And, FilterOperator


class Migration(BaseModelMigration):
"""
This migration removes the presence status if the user is not part of the meeting anymore.
"""

target_migration_index = 63

def migrate_models(self) -> list[BaseRequestEvent] | None:
present_users_per_meeting: dict[int, list[int]] = defaultdict(list)
meetings_per_present_user: dict[int, list[int]] = defaultdict(list)

meeting_users = self.reader.filter(
"meeting_user",
And(
FilterOperator("group_ids", "=", "[]"),
FilterOperator("meta_deleted", "!=", True),
),
["user_id", "meeting_id"],
)
for meeting_user in meeting_users.values():
user_id = meeting_user["user_id"]
meeting_id = meeting_user["meeting_id"]
meetings_per_present_user[user_id].append(meeting_id)
present_users_per_meeting[meeting_id].append(user_id)

meetings = self.reader.get_many(
[
GetManyRequestPart(
"meeting",
[meeting_id for meeting_id in present_users_per_meeting],
["present_user_ids"],
)
]
).get("meeting", dict())
users = self.reader.get_many(
[
GetManyRequestPart(
"user",
[present_user_id for present_user_id in meetings_per_present_user],
["is_present_in_meeting_ids"],
)
]
).get("user", dict())
return [
*[
RequestUpdateEvent(
fqid_from_collection_and_id("meeting", meeting_id),
{
"present_user_ids": [
id_
for id_ in meetings.get(meeting_id, dict()).get(
"present_user_ids", []
)
if id_ not in user_ids
]
},
)
for meeting_id, user_ids in present_users_per_meeting.items()
if meetings
],
*[
RequestUpdateEvent(
fqid_from_collection_and_id("user", user_id),
{
"is_present_in_meeting_ids": [
id_
for id_ in users.get(user_id, dict()).get(
"is_present_in_meeting_ids", []
)
if id_ not in meeting_ids
]
},
)
for user_id, meeting_ids in meetings_per_present_user.items()
],
]
18 changes: 17 additions & 1 deletion tests/system/action/meeting_user/test_delete.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,18 @@ def test_delete(self) -> None:
def test_delete_with_speaker(self) -> None:
self.set_models(
{
"meeting/10": {"is_active_in_organization_id": 1},
"meeting/10": {
"is_active_in_organization_id": 1,
"present_user_ids": [1],
},
"meeting/101": {
"is_active_in_organization_id": 1,
"present_user_ids": [1],
},
"user/1": {
"is_present_in_meeting_ids": [10, 101],
"meeting_user_ids": [5],
},
"meeting_user/5": {
"user_id": 1,
"meeting_id": 10,
Expand All @@ -38,6 +49,11 @@ def test_delete_with_speaker(self) -> None:
self.assert_model_deleted("meeting_user/5")
self.assert_model_deleted("speaker/1")
self.assert_model_exists("speaker/2", {"meeting_id": 10, "begin_time": 123456})
self.assert_model_exists(
"user/1", {"is_present_in_meeting_ids": [101], "meeting_user_ids": []}
)
self.assert_model_exists("meeting/10", {"present_user_ids": []})
self.assert_model_exists("meeting/101", {"present_user_ids": [1]})

def test_delete_with_chat_message(self) -> None:
self.set_models(
Expand Down
6 changes: 4 additions & 2 deletions tests/system/action/user/test_delete.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,13 +74,14 @@ def test_delete_with_speaker(self) -> None:
"user/111": {
"username": "username_srtgb123",
"meeting_user_ids": [1111],
"is_present_in_meeting_ids": [1],
},
"meeting_user/1111": {
"meeting_id": 1,
"user_id": 111,
"speaker_ids": [15, 16],
},
"meeting/1": {},
"meeting/1": {"present_user_ids": [111]},
"speaker/15": {
"meeting_user_id": 1111,
"meeting_id": 1,
Expand All @@ -95,13 +96,14 @@ def test_delete_with_speaker(self) -> None:
response = self.request("user.delete", {"id": 111})

self.assert_status_code(response, 200)
self.assert_model_deleted("user/111")
self.assert_model_deleted("user/111", {"is_present_in_meeting_ids": []})
self.assert_model_deleted("meeting_user/1111")
self.assert_model_exists(
"speaker/15",
{"meeting_user_id": None, "meeting_id": 1, "begin_time": 12345678},
)
self.assert_model_deleted("speaker/16")
self.assert_model_exists("meeting/1", {"present_user_ids": []})

def test_delete_with_candidate(self) -> None:
self.set_models(
Expand Down
16 changes: 16 additions & 0 deletions tests/system/action/user/test_update.py
Original file line number Diff line number Diff line change
Expand Up @@ -2436,6 +2436,7 @@ def test_group_removal_with_speaker(self) -> None:
"user/1234": {
"username": "username_abcdefgh123",
"meeting_user_ids": [4444, 5555],
"is_present_in_meeting_ids": [4, 5],
},
"meeting_user/4444": {
"meeting_id": 4,
Expand All @@ -2453,11 +2454,13 @@ def test_group_removal_with_speaker(self) -> None:
"is_active_in_organization_id": 1,
"meeting_user_ids": [4444],
"committee_id": 1,
"present_user_ids": [1234],
},
"meeting/5": {
"is_active_in_organization_id": 1,
"meeting_user_ids": [5555],
"committee_id": 1,
"present_user_ids": [1234],
},
"committee/1": {"meeting_ids": [4, 5]},
"speaker/14": {"meeting_user_id": 4444, "meeting_id": 4},
Expand All @@ -2481,6 +2484,19 @@ def test_group_removal_with_speaker(self) -> None:
{
"username": "username_abcdefgh123",
"meeting_user_ids": [4444, 5555],
"is_present_in_meeting_ids": [5],
},
)
self.assert_model_exists(
"meeting/4",
{
"present_user_ids": [],
},
)
self.assert_model_exists(
"meeting/5",
{
"present_user_ids": [1234],
},
)
self.assert_model_exists(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
from typing import Any


def create_data() -> dict[str, dict[str, Any]]:
return {
"meeting/1": {
"id": 1,
"name": "meeting name",
"present_user_ids": [1, 3, 2],
"meeting_user_ids": [3, 4, 5],
"group_ids": [10],
},
"meeting/2": {
"id": 1,
"name": "meeting name",
"present_user_ids": [2],
"meeting_user_ids": [6],
},
"user/1": {
"id": 1,
"username": "correct_user",
"is_present_in_meeting_ids": [1],
"meeting_user_ids": [3],
},
"user/2": {
"id": 2,
"username": "wrong_user",
"is_present_in_meeting_ids": [2, 1],
# meeting users do exist after user left meeting but have no group ids
"meeting_user_ids": [
5,
6,
],
},
"user/3": {
"id": 3,
"username": "correct_user",
"is_present_in_meeting_ids": [1],
"meeting_user_ids": [4],
},
"meeting_user/3": {"id": 3, "user_id": 1, "meeting_id": 1, "group_ids": [10]},
"meeting_user/4": {"id": 4, "user_id": 3, "meeting_id": 1, "group_ids": [10]},
"meeting_user/5": {"id": 5, "user_id": 2, "meeting_id": 1, "group_ids": []},
"meeting_user/6": {"id": 6, "user_id": 2, "meeting_id": 2, "group_ids": []},
"group/10": {"id": 10, "meeting_user_ids": [3, 4], "meeting_id": 1},
}


def test_migration_both_ways(write, finalize, assert_model):
data = create_data()
for fqid, fields in data.items():
write({"type": "create", "fqid": fqid, "fields": fields})

finalize("0062_unset_presence_of_removed_users")

data["meeting/1"]["present_user_ids"] = [1, 3]
data["meeting/2"]["present_user_ids"] = []
data["user/2"]["is_present_in_meeting_ids"] = []

for fqid, fields in data.items():
assert_model(fqid, fields)


def test_migration_one_way(write, finalize, assert_model):
data = create_data()
data["meeting/1"]["present_user_ids"] = [1, 3]
data["meeting/2"]["present_user_ids"] = []

for fqid, fields in data.items():
write({"type": "create", "fqid": fqid, "fields": fields})

finalize("0062_unset_presence_of_removed_users")

data["user/2"]["is_present_in_meeting_ids"] = []

for fqid, fields in data.items():
assert_model(fqid, fields)


def test_migration_other_way(write, finalize, assert_model):
data = create_data()
data["user/2"]["is_present_in_meeting_ids"] = []

for fqid, fields in data.items():
write({"type": "create", "fqid": fqid, "fields": fields})

finalize("0062_unset_presence_of_removed_users")

data["meeting/1"]["present_user_ids"] = [1, 3]
data["meeting/2"]["present_user_ids"] = []

for fqid, fields in data.items():
assert_model(fqid, fields)