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

Allow message deletes from any platform #1991

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

yousefmansy1
Copy link
Contributor

Allow message deletes from any platform

This allows for message deletes to be synced from any of the gateway's channels, not just the "canonical"/source channel.

Note, the required changes in whatsappmulti handler depends on #1974
This is because when deleting a message that is not from the matterbridge user the sender jid is also required.

@@ -466,7 +466,8 @@ func (gw *Gateway) SendMessage(

// exclude file delete event as the msg ID here is the native file ID that needs to be deleted
if msg.Event != config.EventFileDelete {
msg.ID = gw.getDestMsgID(rmsg.Protocol+" "+rmsg.ID, dest, channel)
canonicalMsgID := gw.FindCanonicalMsgID(rmsg.Protocol, rmsg.ID)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

to be honest this is a bit of a hacky workaround and I don't necessarily like doing this pretty expensive operation on basically every message recived.

Hypothetically we can work around this

In practice this inefficiency would only matter in really large message caches (and we're mostly limited by I/O bandwidth).

I still don't really like it though.
I'd rather it be more of a graph-like structure where any message maps to the rest of the messages rather than 1 canonical message to the sent mesages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is less of an issue with the persistent message maps in #1996

@yousefmansy1 yousefmansy1 marked this pull request as ready for review March 1, 2023 10:40
@ilmaisin
Copy link
Contributor

ilmaisin commented Mar 5, 2023

What this should exatly do and do not do? I have this PR applied in my instance, but cannot observe any propagation of message deletions between a Whatsapp and Telegram group.

Edit: And what the relation with #1986 is? Should I have that too applied for it to work?

@yousefmansy1
Copy link
Contributor Author

What this should exatly do and do not do? I have this PR applied in my instance, but cannot observe any propagation of message deletions between a Whatsapp and Telegram group.

Edit: And what the relation with #1986 is? Should I have that too applied for it to work?

Yes you will have to have #1986 merged in as well as WhatsApp deletes wont propagate at all since those events are not handled.

(telegram APIs dont have events for any message deletes so that's also inevitable.)

In theory this should work as:

  1. Message is sent from platform A
  2. Message is deleted from platform B
  3. Message is deleted on all platforms.

If you want to test this PR without merging #1986 as well
try sending a message from telegram to discord.
delete the message from discord, observe original message on telegram is deleted (make sure bot is admin)

@ilmaisin
Copy link
Contributor

ilmaisin commented Mar 8, 2023

What this should exatly do and do not do? I have this PR applied in my instance, but cannot observe any propagation of message deletions between a Whatsapp and Telegram group.
Edit: And what the relation with #1986 is? Should I have that too applied for it to work?

Yes you will have to have #1986 merged in as well as WhatsApp deletes wont propagate at all since those events are not handled.

(telegram APIs dont have events for any message deletes so that's also inevitable.)

In theory this should work as:

1. Message is sent from platform A

2. Message is deleted from platform B

3. Message is deleted on all platforms.

If you want to test this PR without merging #1986 as well try sending a message from telegram to discord. delete the message from discord, observe original message on telegram is deleted (make sure bot is admin)

I merged that PR. When I delete a message sent by myself from Whastapp, something seems to appear according to log, but it doesn't get deleted from Telegram. My bot should be admin. How could I troubleshoot this?

@yousefmansy1
Copy link
Contributor Author

I merged that PR. When I delete a message sent by myself from Whastapp, something seems to appear according to log, but it doesn't get deleted from Telegram. My bot should be admin. How could I troubleshoot this?

Hmm, can you share the log that you said appears?

Presumably if you send a message from WhatsApp, then delete it on WhatsApp, it will delete on telegram?

@yousefmansy1
Copy link
Contributor Author

yousefmansy1 commented Mar 9, 2023

To expand, this CR is a relatively simple hack in which when it gets all the corresponding message IDs with a message those are only stored as a map from original ("canonical") message -> messages[]
there is a method that already existed will find the original message sent for any message.

In reality what we are doing is almost pretending the message delete came from the source platform (at least with regards to the message map.)

In #1996 I have two separate maps, one to store the mapping of message -> canonical message and another that does canonical message -> messages[]
We're getting O(1) complexity for some extra storage usage, in this case a worthy trade off as the data we're dealing with is quite light.
Does this matter in the long run? I don't know. But suppose you had a message history thousands long, it would start to bog down the system the longer you left it running.

@codeclimate
Copy link

codeclimate bot commented Mar 11, 2023

Code Climate has analyzed commit 27c7b1e and detected 0 issues on this pull request.

View more on Code Climate.

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.

2 participants