Fix some message formatting failures, improve test coverage

changelog.d/781.bugfix

@@ -0,0 +1 @@
+Fix various message formatting issues.

src/BridgedRoom.ts

@@ -241,13 +241,13 @@
             id: `INTEG-${this.inboundId}`,
             matrix_id: this.matrixRoomId,
             remote: {
                 id: this.slackChannelId!,
                 name: this.slackChannelName!,
                 slack_team_id: this.slackTeamId!,
                 slack_type: this.slackType!,
                 slack_private: this.isPrivate!,
                 webhook_uri: this.slackWebhookUri!,
                 puppet_owner: this.puppetOwner!,
             },
             remote_id: this.inboundId,
         };
@@ -256,7 +256,7 @@
     }
 
     public async getClientForRequest(userId: string): Promise<{id: string, client: WebClient}|null> {
         const puppet = await this.main.clientFactory.getClientForUserWithId(this.SlackTeamId!, userId);
         if (puppet) {
             return puppet;
         }
@@ -269,7 +269,7 @@
         return null;
     }
 
     public async onMatrixReaction(message: any): Promise<void> {
         const relatesTo = message.content["m.relates_to"];
         const eventStore = this.main.datastore;
         const event = await eventStore.getEventByMatrixId(message.room_id, relatesTo.event_id);
@@ -403,7 +403,7 @@
         // re-write the message so the matrixToSlack converter works as expected.
         let newMessage = JSON.parse(JSON.stringify(message));
         newMessage.content = message.content["m.new_content"];
-        newMessage = await this.stripMatrixReplyFallback(newMessage);
+        newMessage = substitutions.stripMatrixReplyFallback(newMessage);
 
         const body = await substitutions.matrixToSlack(newMessage, this.main, this.SlackTeamId!);
 
@@ -443,8 +443,11 @@
         if (!this.slackWebhookUri && !this.botClient && !puppetedClient) { return false; }
         const slackClient = puppetedClient || this.botClient;
         const user = this.main.getOrCreateMatrixUser(message.sender);
-        message = await this.stripMatrixReplyFallback(message);
+
+        message = substitutions.stripMatrixReplyFallback(message);
+
         const matrixToSlackResult = await substitutions.matrixToSlack(message, this.main, this.SlackTeamId!);
+
         if (!matrixToSlackResult) {
             // Could not handle content, dropped.
             log.warn(`Dropped ${message.event_id}, message content could not be identified`);
@@ -994,7 +997,7 @@
         if (message.thread_ts !== undefined && message.text) {
             let replyMEvent = await this.getReplyEvent(this.MatrixRoomId, message, this.SlackChannelId!);
             if (replyMEvent) {
-                replyMEvent = await this.stripMatrixReplyFallback(replyMEvent);
+                replyMEvent = substitutions.stripMatrixReplyFallback(replyMEvent);
                 return await ghost.sendInThread(
                     this.MatrixRoomId, message.text, this.SlackChannelId!, eventTS, replyMEvent,
                 );
@@ -1048,7 +1051,7 @@
                 let replyEvent = await this.getReplyEvent(
                     this.MatrixRoomId, message.message as unknown as ISlackMessageEvent, this.slackChannelId!,
                 );
-                replyEvent = await this.stripMatrixReplyFallback(replyEvent);
+                replyEvent = substitutions.stripMatrixReplyFallback(replyEvent);
                 if (replyEvent) {
                     const bodyFallback = ghost.getFallbackText(replyEvent);
                     const formattedFallback = ghost.getFallbackHtml(this.MatrixRoomId, replyEvent);
@@ -1152,43 +1155,6 @@
         return intent.getEvent(roomID, replyToEvent.eventId);
     }
 
-    /*
-        Strip out reply fallbacks. Borrowed from
-        https://github.com/turt2live/matrix-js-bot-sdk/blob/master/src/preprocessors/RichRepliesPreprocessor.ts
-    */
-    private async stripMatrixReplyFallback(event: any): Promise<any> {
-        if (!event.content?.body) {
-            return event;
-        }
-
-        let realHtml = event.content.formatted_body;
-        let realText = event.content.body || "";
-
-        if (event.content.format === "org.matrix.custom.html" && realHtml) {
-            const formattedBody = realHtml;
-            if (formattedBody.startsWith("<mx-reply>") && formattedBody.indexOf("</mx-reply>") !== -1) {
-                const parts = formattedBody.split("</mx-reply>");
-                realHtml = parts[1];
-                event.content.formatted_body = realHtml.trim();
-            }
-        }
-
-        let processedFallback = false;
-        for (const line of realText.split("\n")) {
-            if (line.startsWith("> ") && !processedFallback) {
-                continue;
-            } else if (!processedFallback) {
-                realText = line;
-                processedFallback = true;
-            } else {
-                realText += line + "\n";
-            }
-        }
-
-        event.content.body = realText.trim();
-        return event;
-    }
-
     /*
         Given an event which is in reply to something else return the event ID of the
         top most event in the reply chain, i.e. the one without a relates to.

src/SlackGhost.ts

@@ -318,7 +318,7 @@ export class SlackGhost {
     }
 
     public prepareFormattedBody(body: string): string {
-        return Slackdown.parse(body);
+        return Slackdown.parse(body).replace("\n", "<br>");
     }
 
     public async sendInThread(roomId: string, text: string, slackRoomId: string,
@@ -361,7 +361,7 @@ export class SlackGhost {
         const content = {
             body,
             format: "org.matrix.custom.html",
-            formatted_body: Slackdown.parse(text),
+            formatted_body: this.prepareFormattedBody(text),
             msgtype: "m.text",
             ...extra,
         };

src/substitutions.ts

@@ -14,14 +14,11 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-import { Logger } from "matrix-appservice-bridge";
 import * as emoji from "node-emoji";
 import { Main } from "./Main";
 import { ISlackFile } from "./BaseSlackHandler";
 import escapeStringRegexp from "escape-string-regexp";
 
-const log = new Logger("substitutions");
-
 const ATTACHMENT_TYPES = ["m.audio", "m.video", "m.file", "m.image"];
 const PILL_REGEX = /<a href="https:\/\/matrix\.to\/#\/(#|@|\+)([^"]+)">([^<]+)<\/a>/g;
 
@@ -59,8 +56,6 @@ class Substitutions {
      * @param file options slack file object
      */
     public slackToMatrix(body: string, file?: ISlackFile): string {
-        log.debug("running substitutions on ", body);
-        body = this.htmlUnescape(body);
         body = body.replace("<!channel>", "@room");
         body = body.replace("<!here>", "@room");
         body = body.replace("<!everyone>", "@room");
@@ -317,6 +312,43 @@ class Substitutions {
             return `${file.url_private}?pub_secret=${pubSecret[1]}`;
         }
     }
+
+    /*
+        Strip out reply fallbacks. Borrowed from
+        https://github.com/turt2live/matrix-js-bot-sdk/blob/master/src/preprocessors/RichRepliesPreprocessor.ts
+    */
+    public stripMatrixReplyFallback<T>(event: T & any): T {
+        if (!event.content?.body) {
+            return event;
+        }
+
+        let realHtml = event.content.formatted_body;
+        let realText = event.content.body || "";
+
+        if (event.content.format === "org.matrix.custom.html" && realHtml) {
+            const formattedBody = realHtml;
+            if (formattedBody.startsWith("<mx-reply>") && formattedBody.indexOf("</mx-reply>") !== -1) {
+                const parts = formattedBody.split("</mx-reply>");
+                realHtml = parts[1];
+                event.content.formatted_body = realHtml.trim();
+            }
+        }
+
+        let processedFallback = false;
+        for (const line of realText.split("\n")) {
+            if (line.startsWith("> ") && !processedFallback) {
+                continue;
+            } else if (!processedFallback) {
+                realText = line + "\n";
+                processedFallback = true;
+            } else {
+                realText += line + "\n";
+            }
+        }
+
+        event.content.body = realText.trim();
+        return event;
+    }
 }
 
 const substitutions = new Substitutions();

tests/unit/substitutionsTest.ts

@@ -297,34 +297,35 @@ describe("Substitutions", () => {
         });
     });
 
-    // describe("slackTextToMatrixHTML", () => {
-    //     it("should repeat a plain string", async () => {
-    //         const res = await substitutions.slackTextToMatrixHTML("Hello World!");
-    //         expect(res).to.equal("Hello World!");
-    //     });
-    //     it("should convert < and >", async () => {
-    //         const res = await substitutions.slackTextToMatrixHTML("<html>Hello</html>");
-    //         expect(res).to.equal("&lt;html&gt;Hello&lt;/html&gt;");
-    //     });
-    //     it("should convert a single new line to a <br />", async () => {
-    //         const res = substitutions.slackTextToMatrixHTML("line 1\nline 2");
-    //         expect(res).to.equal("line 1<br />line 2");
-    //     });
-    //     it("should convert two new lines to paragraphs", async () => {
-    //         const res = substitutions.slackTextToMatrixHTML("line 1\n\nline 3");
-    //         expect(res).to.equal("<p>line 1</p><p>line 3</p>");
-    //     });
-    //     it("should convert bold formatting", async () => {
-    //         const res = substitutions.slackTextToMatrixHTML("This is *bold*!");
-    //         expect(res).to.equal("This is <strong>bold</strong>!");
-    //     });
-    //     it("should convert italic formatting", async () => {
-    //         const res = substitutions.slackTextToMatrixHTML("This is /italics/!");
-    //         expect(res).to.equal("This is <em>italics</em>!");
-    //     });
-    //     it("should convert strikethrough formatting", async () => {
-    //         const res = substitutions.slackTextToMatrixHTML("This is ~strikethrough~!");
-    //         expect(res).to.equal("This is <del>strikethrough</del>");
-    //     });
-    // });
+    describe("slackToMatrix", () => {
+        it("should repeat a plain string", () => {
+            const res = substitutions.slackToMatrix("Hello World!");
+            expect(res).to.equal("Hello World!");
+        });
+        it("should leave newlines intact", () => {
+            const res = substitutions.slackToMatrix("Hello\nWorld!");
+            expect(res).to.equal("Hello\nWorld!");
+        });
+        it("should leave html encoding intact", () => {
+            const res = substitutions.slackToMatrix("<joke>");
+            expect(res).to.equal("<joke>");
+        });
+        it("should convert !here", () => {
+            const res = substitutions.slackToMatrix("<!here> hello");
+            expect(res).to.equal("@room hello");
+        });
+    });
+
+    describe("stripMatrixReplyFallback", () => {
+        it("should leave newlines intact in messages", () => {
+            let message = { content: { body: "foo\nbar" } };
+            message = substitutions.stripMatrixReplyFallback(message);
+            expect(message.content.body).to.equal("foo\nbar");
+        });
+        it("should not break code block contents", () => {
+            let message = { content: { body: "```\nfoo\n```" } };
+            message = substitutions.stripMatrixReplyFallback(message);
+            expect(message.content.body).to.equal("```\nfoo\n```");
+        });
+    });
 });