-
Notifications
You must be signed in to change notification settings - Fork 26
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
Deleted participants stay present in meeting #2730
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found some stuff in this pr. Please fix it.
def check_meeting_and_users( | ||
self, instance: dict[str, Any], user_fqid: FullQualifiedId |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'
"meeting_id": meeting_id, | ||
"present": False, | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. :)
This migration adds the user_id "-1" to all existing action_workers. | ||
This is the number usually used for calls using the internal route. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrote the actual description.
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"] | ||
) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
"user", ["is_present_in_meeting_ids", "meeting_user_ids"] | ||
) | ||
|
||
def helper() -> None: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it.
There was a problem hiding this comment.
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.
|
||
def helper() -> None: | ||
if not ( | ||
(meeting_user_ids := user.get("meeting_user_ids", [])) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks okay.
Closes #2729