Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

update m.direct list for InviteAutoAccepter module #11578

Closed
wants to merge 8 commits into from
Closed

update m.direct list for InviteAutoAccepter module #11578

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
896e67b
update m.direct list for InviteAutoAccepter module
Dec 14, 2021
ebaf4d1
adding file to change log
chagai95 Dec 14, 2021
3d9ff23
forgot a period 🤦‍♂️
chagai95 Dec 14, 2021
c057768
put the room id in the right list
Dec 15, 2021
15b3424
Merge branch 'update-m-direct-list' of https://github.com/chagai95/sy…
Dec 15, 2021
0c20d53
fix for lint
Dec 15, 2021
4ab0858
exposing another method to update m direct list
Jan 5, 2022
e0012a5
False is nicer than None
Jan 5, 2022
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
1 change: 1 addition & 0 deletions changelog.d/11578.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Update the m.direct list when auto accepting invites with the [auto-accept invite module](https://github.com/matrix-org/synapse-auto-accept-invite).
44 changes: 44 additions & 0 deletions synapse/handlers/room_member.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,6 +503,50 @@ async def update_membership(

return result

async def update_m_direct(
Copy link
Contributor Author

@chagai95 chagai95 Dec 15, 2021

Choose a reason for hiding this comment

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

I'm not sure about async for the function I added, does it need this?

Copy link
Contributor

@reivilibre reivilibre Dec 16, 2021

Choose a reason for hiding this comment

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

Yes since it contains await in the body — perfectly correct

self,
sender: str,
target: str,
room_id: str,
) -> None:
"""Update a user's m_direct list.
Copy link
Member

@anoadragon453 anoadragon453 Mar 30, 2022

Choose a reason for hiding this comment

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

Note that this PR should ensure that there is a way of allowing the Module API to ignore updates from itself via the proposed on_account_data_change callback (#12327). Perhaps by adding an argument to that method to inform modules that it was a module vs. a user that updated the account data.

Copy link
Contributor

@babolivier babolivier Mar 30, 2022

Choose a reason for hiding this comment

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

Perhaps by adding an argument to that method to inform modules that it was a module vs. a user that updated the account data.

If we want to do that, then we should do it before #12327 merges. I guess we can add an argument to the account data handler's methods to differentiate changes done by the server vs changes done by an user (I'm not feeling comfortable about adding a flag about whether the change is done by a module specifically).

Though it's worth noting that we have other similar callbacks, such as on_new_event, which don't have such an argument and it's never been an issue, so I'm wondering whether we actually need that.


Params:
sender: The user who sent the invite, used as the key to update the list.
target: The user whose user's m_direct list is being updated.
room_id: The room ID which is being added to the list.
"""

logger.debug(
"InviteAutoAccepter: update_m_direct is triggered with room id: %s, sender user id: %s, target user id: %s",
room_id,
sender,
target,
)

# Retrieve user account data
user_account_data, _ = await self.store.get_account_data_for_user(target)

# Retrieve m direct list
direct_rooms = user_account_data.get(AccountDataTypes.DIRECT, {})

# Check which key this room is under
if isinstance(direct_rooms, dict):
m_list_changed = False
if sender in direct_rooms:
if room_id not in direct_rooms[sender]:
# Add new room_id to this key
direct_rooms[sender].append(room_id)
m_list_changed = True
else:
direct_rooms[sender] = [room_id]
m_list_changed = True
if m_list_changed:
# Save back to user's m.direct account data
await self.account_data_handler.add_account_data_for_user(
target, AccountDataTypes.DIRECT, direct_rooms
)

async def update_membership_locked(
self,
requester: Requester,
Expand Down
34 changes: 34 additions & 0 deletions synapse/module_api/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -889,6 +889,40 @@ async def update_room_membership(

return event

async def update_m_direct(
Copy link
Contributor

@reivilibre reivilibre Jan 10, 2022

Choose a reason for hiding this comment

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

Sorry, I believe I have been misunderstood here.

What I was trying to say is that update_m_direct should live in the synapse-auto-accept-invite module.

Instead, I would suggest adding add_account_data_for_user (which is just a thin wrapper around add_account_data_for_user) and get_account_data_for_user (a thin wrapper again) to the Module API.

Copy link
Contributor Author

@chagai95 chagai95 Jan 10, 2022

Choose a reason for hiding this comment

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

Yep but as a first step I added just updating the list. I guess the logic for that is also better in the module, could we keep that also for a next step?

Copy link
Contributor

@babolivier babolivier Jan 11, 2022

Choose a reason for hiding this comment

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

The Module API is supposed to be a stable API. So I don't think we should add things to it if we're already planning to get rid of them soon, as it would mean we would need to maintain an unnecessary layer of backwards compatibility when we do the switch. So since we already have an idea of what the long-term solution should look like (and it's not a complex one) I think we should just do it.
As a nitpick, I think add_account_data_for_user should instead be named set_account_data_for_user - it makes it clearer that the method can be used for both creating new account data and updating existing one (sadly this makes it a bit inconsistent with the method from the account data handler, but I think it's best to make things clearer to module developers).

Copy link
Contributor

@reivilibre reivilibre Jan 21, 2022

Choose a reason for hiding this comment

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

As a nitpick, I think add_account_data_for_user should instead be named set_account_data_for_user

No objections here with that; it does reflect that it will overwrite account data so sounds right.

self,
sender: str,
target: str,
room_id: str,
content: Optional[JsonDict] = None,
) -> None:
"""Update a user's m_direct list.

Params:
sender: The user who sent the invite, used as the key to update the list.
target: The user whose user's m_direct list is being updated.
room_id: The room ID which is being added to the list.
"""
logger.debug("InviteAutoAccepter: content of update_m_direct %s", content)
if not self.is_mine(sender):
raise RuntimeError(
"Tried to send an event as a user that isn't local to this homeserver",
)

if content is None:
content = {}
# Update the m direct list
is_direct = content.get("is_direct", False)
logger.debug("InviteAutoAccepter: is_direct is %s", is_direct)
logger.debug("InviteAutoAccepter: target is %s", target)
logger.debug("InviteAutoAccepter: sender is %s", sender)
if target != sender and is_direct:
await self._hs.get_room_member_handler().update_m_direct(
sender=sender,
target=target,
room_id=room_id,
)

async def create_and_send_event_into_room(self, event_dict: JsonDict) -> EventBase:
"""Create and send an event into a room.

Expand Down