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 2 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
18 changes: 14 additions & 4 deletions openslides_backend/action/actions/user/user_mixins.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
from ...mixins.archived_meeting_check_mixin import CheckForArchivedMeetingMixin
from ...util.typing import ActionData
from ..meeting_user.set_data import MeetingUserSetData
from .set_present import UserSetPresentAction


class UsernameMixin(Action):
Expand Down Expand Up @@ -142,10 +143,19 @@ 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:
if instance.get("group_ids") == []:
self.execute_other_action(
UserSetPresentAction,
[
{
"id": instance["id"],
"meeting_id": meeting_id,
"present": False,
}
Copy link
Member

Choose a reason for hiding this comment

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

Does this handle the case, if a user is deleted and presence in two meetings?

Copy link
Member Author

Choose a reason for hiding this comment

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

Now in its new place it does. :)

],
)
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,63 @@
from collections import defaultdict

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

from openslides_backend.shared.patterns import fqid_from_collection_and_id


class Migration(BaseModelMigration):
"""
This migration adds the user_id "-1" to all existing action_workers.
This is the number usually used for calls using the internal route.
Copy link
Member

Choose a reason for hiding this comment

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

user_id -1 is not an Id and will lead to errors. Why use such an id?

Copy link
Member Author

Choose a reason for hiding this comment

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

Wrote the actual description.

"""

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)
meetings = self.reader.get_all(
"meeting", ["group_ids", "present_user_ids", "meeting_user_ids"]
)
users = self.reader.get_all(
"user", ["is_present_in_meeting_ids", "meeting_user_ids"]
)
Copy link
Member

Choose a reason for hiding this comment

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

Ui, that is a large operation, getting 1000+ meetings and 10000+ users in worse case. Use get_all with cautious
and use filter, etc instead.

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 thought about this but I couldn't think of a solution, because I have to compare in both directions if the presence relation between the meeting and user is corrupted. If only one way was necessary I could at least filter for all the users that are present in meetings.

Copy link
Member Author

Choose a reason for hiding this comment

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

Due to refactoring this does not exist anymore. Now I'm using one filter for the meeting users and get_many for meeting and users.


def helper() -> None:
Copy link
Member

Choose a reason for hiding this comment

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

helper is a bad name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Due to refactoring this function does not exist anymore.

if not (
(meeting_user_ids := user.get("meeting_user_ids", []))
Copy link
Member

Choose a reason for hiding this comment

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

Where does helper get his user from?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added arguments. Before the nested function would use all local variables in the enclosing scope.

Copy link
Member Author

Choose a reason for hiding this comment

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

Due to refactoring this function does not exist anymore.

and any(
meeting_user_id in meeting_user_ids
for meeting_user_id in meeting.get("meeting_user_ids", [])
)
):
meetings_per_present_user[user_id].append(meeting_id)
present_users_per_meeting[meeting_id].append(user_id)

for meeting_id, meeting in meetings.items():
for user_id in meeting.get("present_user_ids", []):
user = users.get(user_id, dict())
helper()

for user_id, user in users.items():
for meeting_id in user.get("is_present_in_meeting_ids", []):
helper()

return [
RequestUpdateEvent(
fqid_from_collection_and_id(what_collection, what_id),
{
field: [
id_
for id_ in lookup.get(what_id, dict()).get(field, [])
if id_ not in which_ids
]
},
)
for lookup, cross_lookup, what_collection, field in [
(meetings, present_users_per_meeting, "meeting", "present_user_ids"),
(users, meetings_per_present_user, "user", "is_present_in_meeting_ids"),
]
for what_id, which_ids in cross_lookup.items()
Copy link
Member

Choose a reason for hiding this comment

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

Better to use two loops here. This code no one understands, is bogus.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

]
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,75 @@
from typing import Any


def create_data() -> dict[str, dict[str, Any]]:
return {
"meeting/11": {
"id": 1,
"name": "meeting name",
"present_user_ids": [1, 2],
"meeting_user_ids": [3],
},
"meeting/111": {
"id": 1,
"name": "meeting name",
"present_user_ids": [2],
"meeting_user_ids": [4],
},
"user/1": {
"id": 1,
"username": "wrong_user",
"is_present_in_meeting_ids": [11],
},
"user/2": {
"id": 2,
"username": "correct_user",
"is_present_in_meeting_ids": [11, 111],
"meeting_user_ids": [3, 4],
},
"meeting_user/3": {"id": 3, "user_id": 2, "meeting_id": 11},
"meeting_user/4": {"id": 4, "user_id": 2, "meeting_id": 111},
}


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/11"]["present_user_ids"] = [2]
data["user/1"]["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/11"]["present_user_ids"] = [2]

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

finalize("0062_unset_presence_of_removed_users")

data["user/1"]["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/1"]["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/11"]["present_user_ids"] = [2]

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