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

Support editing/removing multipart Slack messages #782

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

Conversation

tadzik
Copy link
Contributor

@tadzik tadzik commented Apr 5, 2024

We may produce multiple Matrix events per Slack events, if the Slack events contains text with attachments. This should have been forbidden with a DB constraint, but for some reason it happened regardless.

This updates the constraint to match reality, differentiating Matrix events by the type of incoming Slack event. For backwards compatibility reasons getEventBySlackId() returns the "main" Matrix event.

When handling message changes and message deletion, we will now correctly edit the right part of the message – either changing the text or removing some of the attachments (fixes #486). Deleting the whole message will now also remove all the attachments associated with it.

Depends on #780

Does not support NeDB, but I wonder if we want to support NeDB at all these days (ref #775)

This makes us the original sender's intent to redact messages,
so that we do not require an elevated PL to handle message deletion.
We may produce multiple Matrix events per Slack events, if the Slack events contains text with attachments.
This should have been forbidden with a DB constraint, but for some reason it happened regardless.

This updates the constraint to match reality, differentiating Matrix events by the type of incoming Slack event.
For backwards compatibility reasons getEventBySlackId() returns the "main" Matrix event.
Messages with a message_changed will have the same `ts` as the original message, but a different `event_ts`.
We now check both when considering whether or not to drop a message, and log our reasons regardless.
We now track which attachments was linked to which Matrix message,
and support both removing them individually and dropping the whole event "tree".
@tadzik tadzik changed the title Formalize our one-to-many relationship between Slack and Matrix events Support editing/removing multipart Slack messages Apr 5, 2024
@tadzik tadzik marked this pull request as ready for review April 5, 2024 15:01
@tadzik tadzik requested a review from a team as a code owner April 5, 2024 15:01
Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

Looks okay, could do with a test?

@tadzik
Copy link
Contributor Author

tadzik commented Jun 6, 2024

Note: this is RTM-only, as webhooks do not support multipart messages in the first place.

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.

Editing text in a Slack text+file message fails: "Multiple rows were not expected"
2 participants