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

Upload files in threads #20

Merged
merged 47 commits into from Sep 11, 2023
Merged

Upload files in threads #20

merged 47 commits into from Sep 11, 2023

Conversation

psrpinto
Copy link
Member

@psrpinto psrpinto commented Sep 6, 2023

This PR implements file uploads in threads. Previously, files uploaded in a thread on Slack, would render on the main timeline on Matrix. With this PR, the files is correctly uploaded to the Matrix thread.

The upstream issue is: matrix-org#671


This PR is the culmination of refactoring the Slack message parsing logic into a SlackMessageParser which exposes a single method:

import {ISlackMessageEvent} from "./BaseSlackHandler";
import {MessageEventContent} from "matrix-bot-sdk";

async parse(message: ISlackMessageEvent): Promise<MessageEventContent[]>;

Since one Slack message might result in multiple Matrix events (each file referenced by the message is a separate event on Matrix), the parse() method returns an array of Matrix events. The array might be empty if the parser failed to parse the message.

For matrix events that are files, the parser does not upload them to Matrix. Instead, it sets the Slack URL to the file in the Matrix event, with an appended access token:

{
    "body": "foo.png",
    "info": {
      "mimetype": "image/png",
      "size": 97335,
      "w": 715,
      "h": 715,
      "thumbnail_url": "https://files.slack.com/...?token=abc123",
      "thumbnail_info": {
        "w": 360,
        "h": 360
      }
    },
    "msgtype": "m.image",
    "url": "https://files.slack.com/...?token=abc123"
}

Consumers of the parser then need to actually upload the file, and replace the URL in the Matrix event.

@akirk
Copy link
Member

akirk commented Sep 11, 2023

Could you for now include (code) comments of which functionality should be actually in https://github.com/matrix-org/matrix-appservice-bridge/

We also discussed to update the README.md with the differences to upstream but this can be done in an upcoming PR.

@psrpinto psrpinto force-pushed the files-in-thread branch 3 times, most recently from 20363af to edfca1b Compare September 11, 2023 13:54
@psrpinto psrpinto marked this pull request as ready for review September 11, 2023 14:00
@psrpinto
Copy link
Member Author

Could you for now include (code) comments of which functionality should be actually in matrix-appservice-bridge.
We also discussed to update the README.md with the differences to upstream but this can be done in an upcoming PR.

@akirk, since the topic of which logic should be in matrix-appservice-bridge is a more generic one (not just relating to files), I will address it together with the other topic of documenting differences between this fork and the upstream bridge, in a separate PR.

@akirk
Copy link
Member

akirk commented Sep 11, 2023

Sounds good to me, I just thought it would be good to note it down whenever you encounter it, not restricted to this PR.

@psrpinto
Copy link
Member Author

I tested quite a bit locally, merging.

@psrpinto psrpinto merged commit 3dd1ee1 into develop Sep 11, 2023
2 checks passed
@psrpinto psrpinto deleted the files-in-thread branch September 11, 2023 14:21
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.

None yet

2 participants