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

Add a column participant to room_memberships table #18068

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

H-Shay
Copy link
Contributor

@H-Shay H-Shay commented Jan 7, 2025

Adds a column participant to room_memberships table to track whether a user has participated in a room - participation is defined as having sent a m.room.message event into the room.

Related to #18040, the approach there to determine room participation was deemed too inefficient and adding this column was the recommend remedy.

@H-Shay H-Shay requested a review from a team as a code owner January 7, 2025 20:04
@H-Shay H-Shay changed the title Add a column participant to room_membership table Add a column participant to room_memberships table Jan 7, 2025
@H-Shay
Copy link
Contributor Author

H-Shay commented Jan 7, 2025

Requesting @anoadragon453's feedback on this as he has context.

@H-Shay H-Shay marked this pull request as draft January 7, 2025 20:20
@@ -1440,6 +1440,9 @@ async def handle_new_client_event(
)
return prev_event

if event.type == "m.room.message":
Copy link
Member

Choose a reason for hiding this comment

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

Drive by comment: presumably we also want to include encrypted messages here? Which have their own event type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Thanks for picking up on that :)

@erikjohnston erikjohnston requested a review from a team January 8, 2025 11:08
AND rm.membership = 'join'
AND e.type = 'm.room.message'
AND rm.user_id = e.sender;
"""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Double checking the logic, on this, the original query was written using current_state_events so that only current room members would be included - however, won't changing to using room_memberships pull anyone who has ever posted, regardless if they have left the room?
Since the suggestion to use room_memberships originated in a performance concern, which should be alleviated by this being a background update, can I switch back to the query using current_state_events? Original discussion here: #18040 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

@H-Shay wouldn't limiting rows by the membership column of room_memberships to join values exclude users who have left the room?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants