RNL/out-of-your-element-fork-that-i-have-perms-for
1
0
Fork 0
forked from cadence/out-of-your-element
Code Pull requests Activity

Start work on pinned events

Browse source
This commit is contained in:
Cadence Ember 2023-10-10 17:41:53 +13:00
parent bf0691f9bb
commit 48d69c0539
9 changed files with 427 additions and 2 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

22
d2m/actions/update-pins.js Normal file
View file

@ -0,0 +1,22 @@
// @ts-check
const passthrough = require("../../passthrough")
const {discord, sync} = passthrough
/** @type {import("../converters/pins-to-list")} */
const pinsToList = sync.require("../converters/pins-to-list")
/** @type {import("../../matrix/api")} */
const api = sync.require("../../matrix/api")
/**
* @param {string} channelID
* @param {string} roomID
*/
async function updatePins(channelID, roomID) {
const pins = await discord.snow.channel.getChannelPinnedMessages(channelID)
const eventIDs = pinsToList.pinsToList(pins)
await api.sendState(roomID, "m.room.pinned_events", "", {
pinned: eventIDs
})
}
module.exports.updatePins = updatePins

18
d2m/converters/pins-to-list.js Normal file
View file

@ -0,0 +1,18 @@
// @ts-check
const {select} = require("../../passthrough")
/**
* @param {import("discord-api-types/v10").RESTGetAPIChannelPinsResult} pins
*/
function pinsToList(pins) {
/** @type {string[]} */
const result = []
for (const message of pins) {
const eventID = select("event_message", "event_id", {message_id: message.id, part: 0}).pluck().get()
if (eventID) result.push(eventID)
}
return result
}
module.exports.pinsToList = pinsToList

12
d2m/converters/pins-to-list.test.js Normal file
View file

@ -0,0 +1,12 @@
const {test} = require("supertape")
const data = require("../../test/data")
const {pinsToList} = require("./pins-to-list")
test("pins2list: converts known IDs, ignores unknown IDs", t => {
const result = pinsToList(data.pins.faked)
t.deepEqual(result, [
"$X16nfVks1wsrhq4E9SSLiqrf2N8KD0erD0scZG7U5xg",
"$mtR8cJqM4fKno1bVsm8F4wUVqSntt2sq6jav1lyavuA",
"$lnAF9IosAECTnlv9p2e18FG8rHn-JgYKHEHIh5qdFv4"
])
})

3
d2m/discord-packets.js
View file

@ -133,6 +133,9 @@ const utils = {
} else if (message.t === "CHANNEL_UPDATE") {
await eventDispatcher.onChannelOrThreadUpdate(client, message.d, false)
} else if (message.t === "CHANNEL_PINS_UPDATE") {
await eventDispatcher.onChannelPinsUpdate(client, message.d)
} else if (message.t === "THREAD_CREATE") {
// @ts-ignore
await eventDispatcher.onThreadCreate(client, message.d)

12
d2m/event-dispatcher.js
View file

@ -19,6 +19,8 @@ const announceThread = sync.require("./actions/announce-thread")
const createRoom = sync.require("./actions/create-room")
/** @type {import("./actions/create-space")}) */
const createSpace = sync.require("./actions/create-space")
/** @type {import("./actions/update-pins")}) */
const updatePins = sync.require("./actions/update-pins")
/** @type {import("../matrix/api")}) */
const api = sync.require("../matrix/api")
/** @type {import("../discord/discord-command-handler")}) */
@ -157,6 +159,16 @@ module.exports = {
await createRoom.syncRoom(channelOrThread.id)
},
/**
* @param {import("./discord-client")} client
* @param {DiscordTypes.GatewayChannelPinsUpdateDispatchData} data
*/
async onChannelPinsUpdate(client, data) {
const roomID = select("channel_room", "room_id", {channel_id: data.channel_id}).pluck().get()
if (!roomID) return // No target room to update pins in
await updatePins.updatePins(data.channel_id, roomID)
},
/**
* @param {import("./discord-client")} client
* @param {DiscordTypes.GatewayMessageCreateDispatchData} message

349
docs/how-to-add-a-new-event-type.md Normal file
View file

@ -0,0 +1,349 @@
# How to add a new event type
It's much easier to understand code with examples. So let's go through it together and add support for **pinned events** to Out Of Your Element.
## Gathering intel
First, we need to know what pinned events are supposed to look like. The Matrix C-S spec gives this example:
> **pinned** ... [string] ... Required: An ordered list of event IDs to pin.
> ```json
> {
> "content": {
> "pinned": [
> "$someevent:example.org"
> ]
> },
> "event_id": "$143273582443PhrSn:example.org",
> "room_id": "!jEsUZKDJdhlrceRyVU:example.org",
> "sender": "@example:example.org",
> "state_key": "",
> "type": "m.room.pinned_events",
> }
> ```
This is part of the persistent room state. Simple enough. To update this, the state event and its list of pinned events is updated as a whole.
What does it look like on Discord-side?
> **Get Pinned Messages** \
> `GET` `/channels/{channel.id}/pins` \
> Returns all pinned messages in the channel as an array of message objects.
This is an API request to get the pinned messages. To update this, an API request will pin or unpin any specific message, adding or removing it from the list.
## What will the converter look like?
The converter will be very different in both directions.
For d2m, we will get the list of pinned messages, we will convert each message ID into the ID of an event we already have, and then we will set the entire `m.room.pinned_events` state to that list.
For m2d, we will have to diff the list of pinned messages against the previous version of the list, and for each event that was pinned or unpinned, we will send an API request to Discord to change its state.
## Missing messages
> ...we will convert each message ID into the ID of an event ***we already have***
As a couple of examples, the message might not be bridged if it was sent before OOYE was set up, or if OOYE just had a bug trying to handle that message. If a particular Discord message wasn't bridged, and then it gets pinned, we're in a bit of a pickle. We can't pin the Matrix equivalent because it doesn't exist.
In this situation we need to stop and think about the possible paths forward we could take.
* We could ignore this message and just not pin it.
* We could convert and send this message now with its original timestamp from the past, then pin this representation.
The latter method would still make the message appear at the bottom of the timeline for most Matrix clients, since for most the timestamp doesn't determine the actual _order._ It would then be confusing why an odd message suddenly appeared, because a pins change isn't that noticable in the room.
To avoid this problem, I'll just go with the former method and ignore the message, so Matrix will only have some of the pins that Discord has. We will need to watch out if a Matrix user edits this list of partial pins, because if we _only_ pinned things on Discord that were pinned on Matrix, those partial pins Discord would be lost from Discord side.
In this situation I will prefer to keep the pins list inconsistent between both sides and only bridge _changes_ to the list.
If you were implementing this for real, you might have made different decisions than me, and that's okay. It's a matter of taste. You just need to be aware of the consequences of what you choose.
## Test data for the d2m converter
Let's start writing the d2m converter. It's helpful to write unit tests for Out Of Your Element, since this lets you check if it worked without having to start up a local copy of the bridge or play around with the interface.
Normally for getting test data, I would `curl` the Discord API to grab some real data and put it into `data.js` (and possibly also `ooye-test-data.sql`. But this time, I'll fabricate some test data. Here it is:
```js
[
{id: "1126786462646550579"},
{id: "1141501302736695316"},
{id: "1106366167788044450"},
{id: "1115688611186193400"}
]
```
"These aren't message objects!" I hear you cry. Correct. I already know that my implementation is not going to care about any properties on these message object other than the IDs, so I'm just making a list of IDs to save time.
These IDs were carefully chosen. The first three are already in `ooye-test-data.sql` and are associated with event IDs. This is great, because in our test case, the Discord IDs will be converted to those event IDs. The fourth ID doesn't exist on Matrix-side. This is to test that partial pins are handled as expected, like I wrote in the previous section.
Now that I've got my list, I will make my first change to the code. I will add these IDs to `test/data.js`:
```diff
diff --git a/test/data.js b/test/data.js
index c36f252..4919beb 100644
--- a/test/data.js
+++ b/test/data.js
@@ -221,6 +221,14 @@ module.exports = {
deaf: false
}
},
+ pins: {
+ faked: [
+ {id: "1126786462646550579"},
+ {id: "1141501302736695316"},
+ {id: "1106366167788044450"},
+ {id: "1115688611186193400"}
+ ]
+ },
message: {
// Display order is text content, attachments, then stickers
simple_plaintext: {
```
## Writing the d2m converter
We can write a function that operates on this data to convert it to events. This is a _converter,_ not an _action._ it won't _do_ anything by itself. So it goes in the converters folder. The actual function is pretty simple since I've already planned what to do:
```diff
diff --git a/d2m/converters/pins-to-list.js b/d2m/converters/pins-to-list.js
new file mode 100644
index 0000000..e4107be
--- /dev/null
+++ b/d2m/converters/pins-to-list.js
@@ -0,0 +1,18 @@
+// @ts-check
+
+const {select} = require("../../passthrough")
+
+/**
+ * @param {import("discord-api-types/v10").RESTGetAPIChannelPinsResult} pins
+ */
+function pinsToList(pins) {
+ /** @type {string[]} */
+ const result = []
+ for (const message of pins) {
+ const eventID = select("event_message", "event_id", {message_id: message.id}).pluck().get()
+ if (eventID) result.push(eventID)
+ }
+ return result
+}
+
+module.exports.pinsToList = pinsToList
```
## Test case for the d2m converter
There's not much room for bugs in this function. A single manual test that it works would be good enough for me. But since this is an example of how you can add your own, let's add a test case for this. We'll take the data we just prepared and process it through the function we just wrote:
```diff
diff --git a/d2m/converters/pins-to-list.test.js b/d2m/converters/pins-to-list.test.js
new file mode 100644
index 0000000..c2e3774
--- /dev/null
+++ b/d2m/converters/pins-to-list.test.js
@@ -0,0 +1,12 @@
+const {test} = require("supertape")
+const data = require("../../test/data")
+const {pinsToList} = require("./pins-to-list")
+
+test("pins2list: converts known IDs, ignores unknown IDs", t => {
+ const result = pinsToList(data.pins.faked)
+ t.deepEqual(result, [
+ "$X16nfVks1wsrhq4E9SSLiqrf2N8KD0erD0scZG7U5xg",
+ "$mtR8cJqM4fKno1bVsm8F4wUVqSntt2sq6jav1lyavuA",
+ "$lnAF9IosAECTnlv9p2e18FG8rHn-JgYKHEHIh5qdFv4"
+ ])
+})
```
Don't forget to list your test in `test.js` so that it gets picked up:
```diff
diff --git a/test/test.js b/test/test.js
index 5cc851e..280503d 100644
--- a/test/test.js
+++ b/test/test.js
@@ -52,6 +52,7 @@ file._actuallyUploadDiscordFileToMxc = function(url, res) { throw new Error(`Not
require("../d2m/converters/message-to-event.test")
require("../d2m/converters/message-to-event.embeds.test")
require("../d2m/converters/edit-to-changes.test")
+ require("../d2m/converters/pins-to-list.test")
require("../d2m/converters/remove-reaction.test")
require("../d2m/converters/thread-to-announcement.test")
require("../d2m/converters/user-to-mxid.test")
```
Good to go.
```
><> $ npm t
> out-of-your-element@1.1.0 test
> cross-env FORCE_COLOR=true supertape --no-check-assertions-count --format tap test/test.js | tap-dot
pins2list: converts known IDs, ignores unknown IDs - should deep equal
operator: deepEqual
diff: |-
Array [
"$X16nfVks1wsrhq4E9SSLiqrf2N8KD0erD0scZG7U5xg",
- "$mtR8cJqM4fKno1bVsm8F4wUVqSntt2sq6jav1lyavuA",
- "$lnAF9IosAECTnlv9p2e18FG8rHn-JgYKHEHIh5qdFv4",
+ "$51f4yqHinwnSbPEQ9dCgoyy4qiIJSX0QYYVUnvwyTCI",
+ "$Ijf1MFCD39ktrNHxrA-i2aKoRWNYdAV2ZXYQeiZIgEU",
]
at out-of-your-element/d2m/converters/pins-to-list.test.js:7:4
Error: should deep equal
at run (file:///out-of-your-element/node_modules/supertape/lib/operators.mjs:272:33)
at Object.deepEqual (file:///out-of-your-element/node_modules/supertape/lib/operators.mjs:198:9)
at out-of-your-element/d2m/converters/pins-to-list.test.js:7:4
at module.exports (out-of-your-element/node_modules/try-to-catch/lib/try-to-catch.js:7:29)
```
Oh, this was actually an accident, I didn't make it fail for demonstration purposes! Let's see what this bug is. It's returning the right number of IDs, but 2 out of the 3 are incorrect. The green `-` lines are "expected" and the red `+` lines are "actual". I should check where that wrong ID `$51f...` got taken from.
```
- snip - ooye-test-data.sql
('$mtR8cJqM4fKno1bVsm8F4wUVqSntt2sq6jav1lyavuA', 'm.room.message', 'm.text', '1141501302736695316', 0, 1),
('$51f4yqHinwnSbPEQ9dCgoyy4qiIJSX0QYYVUnvwyTCI', 'm.room.message', 'm.image', '1141501302736695316', 1, 1),
```
Context: This Discord message `1141501302736695316` is actually part of 2 different Matrix events, `$mtR...` and `$51f...`. This often happens when a Discord user uploads an image with a caption. Matrix doesn't support combined image+text events, so the image and the text have to be bridged to separate events. We should consider the text to be the primary part, and pin that, and consider the image to be the secondary part, and not pin that.
We already have a column `part` in the `event_message` table for this reason! When `part = 0`, that's the primary part. I'll edit the converter to actually use that column:
```diff
diff --git a/d2m/converters/pins-to-list.js b/d2m/converters/pins-to-list.js
index e4107be..f401de2 100644
--- a/d2m/converters/pins-to-list.js
+++ b/d2m/converters/pins-to-list.js
@@ -9,7 +9,7 @@ function pinsToList(pins) {
/** @type {string[]} */
const result = []
for (const message of pins) {
- const eventID = select("event_message", "event_id", {message_id: message.id}).pluck().get()
+ const eventID = select("event_message", "event_id", {message_id: message.id, part: 0}).pluck().get()
if (eventID) result.push(eventID)
}
return result
```
```
><> $ npm t
144 tests
232 passed
Pass!
```
## Wiring it up to an action
Actions call converters to do the thing, but actions have to receive their input event from somewhere. Let's wire it up so we receive a "pins changed" event from Discord and do the whole flow from there. Checking the documentation again, Discord will trigger this gateway event when the pins change:
> **Channel Pins Update** \
> Sent when a message is pinned or unpinned in a text channel. This is not sent when a pinned message is deleted. \
> **guild_id?** ... snowflake ... ID of the guild \
> **channel_id** ... snowflake ... ID of the channel \
> **last_pin_timestamp?** ... ?ISO8601 timestamp ... Time at which the most recent pinned message was pinned
Notably, the event doesn't deliver the actual list of pinned messages to us. We'll have to listen for this event, then trigger an API request to `GET` the pins list. Alright, enough preparation, time to code.
All packets are delivered to `discord-packets.js` which manages the internal state of the Discord object and then passes it on to a function in `event-dispatcher.js`:
```diff
diff --git a/d2m/discord-packets.js b/d2m/discord-packets.js
index 83c31cd..4de84d9 100644
--- a/d2m/discord-packets.js
+++ b/d2m/discord-packets.js
@@ -133,6 +133,9 @@ const utils = {
} else if (message.t === "CHANNEL_UPDATE") {
await eventDispatcher.onChannelOrThreadUpdate(client, message.d, false)
+ } else if (message.t === "CHANNEL_PINS_UPDATE") {
+ await eventDispatcher.onChannelPinsUpdate(client, message.d)
+
} else if (message.t === "THREAD_CREATE") {
// @ts-ignore
await eventDispatcher.onThreadCreate(client, message.d)
```
`event-dispatcher.js` will now check if the event seems reasonable and is allowed in this context. For example, we can only update pins if the channel is actually bridged somewhere. This should be another quick check which passes to an action to do the API calls:
```diff
diff --git a/d2m/event-dispatcher.js b/d2m/event-dispatcher.js
index 0f9f1e6..6e91e9e 100644
--- a/d2m/event-dispatcher.js
+++ b/d2m/event-dispatcher.js
@@ -19,6 +19,8 @@ const announceThread = sync.require("./actions/announce-thread")
const createRoom = sync.require("./actions/create-room")
/** @type {import("./actions/create-space")}) */
const createSpace = sync.require("./actions/create-space")
+/** @type {import("./actions/update-pins")}) */
+const updatePins = sync.require("./actions/update-pins")
/** @type {import("../matrix/api")}) */
const api = sync.require("../matrix/api")
/** @type {import("../discord/discord-command-handler")}) */
@@ -157,6 +159,16 @@ module.exports = {
await createRoom.syncRoom(channelOrThread.id)
},
+ /**
+ * @param {import("./discord-client")} client
+ * @param {DiscordTypes.GatewayChannelPinsUpdateDispatchData} data
+ */
+ async onChannelPinsUpdate(client, data) {
+ const roomID = select("channel_room", "room_id", {channel_id: data.channel_id}).pluck().get()
+ if (!roomID) return // No target room to update pins in
+ await updatePins.updatePins(data.channel_id, roomID)
+ },
+
/**
* @param {import("./discord-client")} client
* @param {DiscordTypes.GatewayMessageCreateDispatchData} message
```
And now I can create the `update-pins.js` action:
```diff
diff --git a/d2m/actions/update-pins.js b/d2m/actions/update-pins.js
new file mode 100644
index 0000000..40cc358
--- /dev/null
+++ b/d2m/actions/update-pins.js
@@ -0,0 +1,22 @@
+// @ts-check
+
+const passthrough = require("../../passthrough")
+const {discord, sync} = passthrough
+/** @type {import("../converters/pins-to-list")} */
+const pinsToList = sync.require("../converters/pins-to-list")
+/** @type {import("../../matrix/api")} */
+const api = sync.require("../../matrix/api")
+
+/**
+ * @param {string} channelID
+ * @param {string} roomID
+ */
+async function updatePins(channelID, roomID) {
+ const pins = await discord.snow.channel.getChannelPinnedMessages(channelID)
+ const eventIDs = pinsToList.pinsToList(pins)
+ await api.sendState(roomID, "m.room.pinned_events", "", {
+ pinned: eventIDs
+ })
+}
+
+module.exports.updatePins = updatePins
```
I try to keep as much logic as possible out of the actions, meaning I never have to unit test the actions themselves, and I can skip right ahead to trying this with the real bot.
## Notes on missed events
Note that this will only sync pins _when the pins change._ Existing pins from Discord will not be backfilled to Matrix rooms. If I wanted, there's a couple of ways I could address this:
* I could create a one-shot script in `scripts/update-pins.js` which will sync pins for _all_ Discord channels right away. I can run this after finishing the feature, or if the bot has been offline for some time.
* I could create a database table that holds the timestamp of the most recently detected pin for each channel - the `last_pin_timestamp` field from the gateway. Every time the bot starts, it would automatically compare the database table against every channel, and if the pins have changed since it last looked, it could automatically update them.
I already have a mechanism for backfilling missed messages when the bridge starts up. Option 2 there would add a similar feature for backfilling missed pins. That could be worth considering, but it's less important and more complex. Perhaps we'll come back to it.

8
test/data.js
View file

@ -221,6 +221,14 @@ module.exports = {
deaf: false
}
},
pins: {
faked: [
{id: "1126786462646550579"},
{id: "1141501302736695316"},
{id: "1106366167788044450"},
{id: "1115688611186193400"}
]
},
message: {
// Display order is text content, attachments, then stickers
simple_plaintext: {

4
test/ooye-test-data.sql
View file

@ -51,8 +51,8 @@ INSERT INTO event_message (event_id, event_type, event_subtype, message_id, part
('$oLyUTyZ_7e_SUzGNWZKz880ll9amLZvXGbArJCKai2Q', 'm.room.message', 'm.text', '1128084748338741392', 0, 1),
('$FchUVylsOfmmbj-VwEs5Z9kY49_dt2zd0vWfylzy5Yo', 'm.room.message', 'm.text', '1143121514925928541', 0, 1),
('$lnAF9IosAECTnlv9p2e18FG8rHn-JgYKHEHIh5qdFv4', 'm.room.message', 'm.text', '1106366167788044450', 0, 1),
('$Ijf1MFCD39ktrNHxrA-i2aKoRWNYdAV2ZXYQeiZIgEU', 'm.room.message', 'm.image', '1106366167788044450', 0, 0),
('$f9cjKiacXI9qPF_nUAckzbiKnJEi0LM399kOkhdd8f8', 'm.sticker', NULL, '1106366167788044450', 0, 0),
('$Ijf1MFCD39ktrNHxrA-i2aKoRWNYdAV2ZXYQeiZIgEU', 'm.room.message', 'm.image', '1106366167788044450', 1, 0),
('$f9cjKiacXI9qPF_nUAckzbiKnJEi0LM399kOkhdd8f8', 'm.sticker', NULL, '1106366167788044450', 1, 0),
('$Fxy8SMoJuTduwReVkHZ1uHif9EuvNx36Hg79cltiA04', 'm.room.message', 'm.text', '1144865310588014633', 0, 1),
('$v_Gtr-bzv9IVlSLBO5DstzwmiDd-GSFaNfHX66IupV8', 'm.room.message', 'm.text', '1144874214311067708', 0, 0),
('$7LIdiJCEqjcWUrpzWzS8TELOlFfBEe4ytgS7zn2lbSs', 'm.room.message', 'm.text', '1145688633186193479', 0, 0),

1
test/test.js
View file

@ -52,6 +52,7 @@ file._actuallyUploadDiscordFileToMxc = function(url, res) { throw new Error(`Not
require("../d2m/converters/message-to-event.test")
require("../d2m/converters/message-to-event.embeds.test")
require("../d2m/converters/edit-to-changes.test")
require("../d2m/converters/pins-to-list.test")
require("../d2m/converters/remove-reaction.test")
require("../d2m/converters/thread-to-announcement.test")
require("../d2m/converters/user-to-mxid.test")