Split join and goto slash commands, the latter shouldn't auto_join (#11259)

* Split join and goto slash commands, the latter shouldn't auto_join

* i18n

* Add tests

* Iterate

* Improve coverage

src/SlashCommands.tsx

@@ -36,7 +36,6 @@ import { textToHtmlRainbow } from "./utils/colour";
 import { AddressType, getAddressType } from "./UserAddress";
 import { abbreviateUrl } from "./utils/UrlUtils";
 import { getDefaultIdentityServerUrl, setToDefaultIdentityServer } from "./utils/IdentityServerUtils";
-import { isPermalinkHost, parsePermalink } from "./utils/permalinks/Permalinks";
 import { WidgetType } from "./widgets/WidgetType";
 import { Jitsi } from "./widgets/Jitsi";
 import BugReportDialog from "./components/views/dialogs/BugReportDialog";
@@ -66,6 +65,7 @@ import { isCurrentLocalRoom, reject, singleMxcUpload, success, successSync } fro
 import { deop, op } from "./slash-commands/op";
 import { CommandCategories } from "./slash-commands/interface";
 import { Command } from "./slash-commands/command";
+import { goto, join } from "./slash-commands/join";
 
 export { CommandCategories, Command };
 
@@ -458,118 +458,8 @@ export const Commands = [
         category: CommandCategories.actions,
         renderingTypes: [TimelineRenderingType.Room],
     }),
-    new Command({
+    goto,
-        command: "join",
+    join,
-        aliases: ["j", "goto"],
-        args: "<room-address>",
-        description: _td("Joins room with given address"),
-        runFn: function (cli, roomId, threadId, args) {
-            if (args) {
-                // Note: we support 2 versions of this command. The first is
-                // the public-facing one for most users and the other is a
-                // power-user edition where someone may join via permalink or
-                // room ID with optional servers. Practically, this results
-                // in the following variations:
-                //   /join #example:example.org
-                //   /join !example:example.org
-                //   /join !example:example.org altserver.com elsewhere.ca
-                //   /join https://matrix.to/#/!example:example.org?via=altserver.com
-                // The command also supports event permalinks transparently:
-                //   /join https://matrix.to/#/!example:example.org/$something:example.org
-                //   /join https://matrix.to/#/!example:example.org/$something:example.org?via=altserver.com
-                const params = args.split(" ");
-                if (params.length < 1) return reject(this.getUsage());
-
-                let isPermalink = false;
-                if (params[0].startsWith("http:") || params[0].startsWith("https:")) {
-                    // It's at least a URL - try and pull out a hostname to check against the
-                    // permalink handler
-                    const parsedUrl = new URL(params[0]);
-                    const hostname = parsedUrl.host || parsedUrl.hostname; // takes first non-falsey value
-
-                    // if we're using a Element permalink handler, this will catch it before we get much further.
-                    // see below where we make assumptions about parsing the URL.
-                    if (isPermalinkHost(hostname)) {
-                        isPermalink = true;
-                    }
-                }
-                if (params[0][0] === "#") {
-                    let roomAlias = params[0];
-                    if (!roomAlias.includes(":")) {
-                        roomAlias += ":" + cli.getDomain();
-                    }
-
-                    dis.dispatch<ViewRoomPayload>({
-                        action: Action.ViewRoom,
-                        room_alias: roomAlias,
-                        auto_join: true,
-                        metricsTrigger: "SlashCommand",
-                        metricsViaKeyboard: true,
-                    });
-                    return success();
-                } else if (params[0][0] === "!") {
-                    const [roomId, ...viaServers] = params;
-
-                    dis.dispatch<ViewRoomPayload>({
-                        action: Action.ViewRoom,
-                        room_id: roomId,
-                        via_servers: viaServers, // for the rejoin button
-                        auto_join: true,
-                        metricsTrigger: "SlashCommand",
-                        metricsViaKeyboard: true,
-                    });
-                    return success();
-                } else if (isPermalink) {
-                    const permalinkParts = parsePermalink(params[0]);
-
-                    // This check technically isn't needed because we already did our
-                    // safety checks up above. However, for good measure, let's be sure.
-                    if (!permalinkParts) {
-                        return reject(this.getUsage());
-                    }
-
-                    // If for some reason someone wanted to join a user, we should
-                    // stop them now.
-                    if (!permalinkParts.roomIdOrAlias) {
-                        return reject(this.getUsage());
-                    }
-
-                    const entity = permalinkParts.roomIdOrAlias;
-                    const viaServers = permalinkParts.viaServers;
-                    const eventId = permalinkParts.eventId;
-
-                    const dispatch: ViewRoomPayload = {
-                        action: Action.ViewRoom,
-                        auto_join: true,
-                        metricsTrigger: "SlashCommand",
-                        metricsViaKeyboard: true,
-                    };
-
-                    if (entity[0] === "!") dispatch["room_id"] = entity;
-                    else dispatch["room_alias"] = entity;
-
-                    if (eventId) {
-                        dispatch["event_id"] = eventId;
-                        dispatch["highlighted"] = true;
-                    }
-
-                    if (viaServers) {
-                        // For the join, these are passed down to the js-sdk's /join call
-                        dispatch["opts"] = { viaServers };
-
-                        // For if the join fails (rejoin button)
-                        dispatch["via_servers"] = viaServers;
-                    }
-
-                    dis.dispatch(dispatch);
-                    return success();
-                }
-            }
-            return reject(this.getUsage());
-        },
-        category: CommandCategories.actions,
-        renderingTypes: [TimelineRenderingType.Room],
-    }),
     new Command({
         command: "part",
         args: "[<room-address>]",

src/i18n/strings/en_EN.json

@@ -435,7 +435,6 @@
     "Continue": "Continue",
     "Use an identity server to invite by email. Manage in Settings.": "Use an identity server to invite by email. Manage in Settings.",
     "User (%(user)s) did not end up as invited to %(roomId)s but no error was given from the inviter utility": "User (%(user)s) did not end up as invited to %(roomId)s but no error was given from the inviter utility",
-    "Joins room with given address": "Joins room with given address",
     "Leave room": "Leave room",
     "Unrecognised room address: %(roomAlias)s": "Unrecognised room address: %(roomAlias)s",
     "Removes user with given id from this room": "Removes user with given id from this room",
@@ -934,6 +933,8 @@
     "Advanced": "Advanced",
     "Effects": "Effects",
     "Other": "Other",
+    "Joins room with given address": "Joins room with given address",
+    "Views room with given address": "Views room with given address",
     "Command failed: Unable to find room (%(roomId)s": "Command failed: Unable to find room (%(roomId)s",
     "Could not find user in room": "Could not find user in room",
     "Define the power level of a user": "Define the power level of a user",

src/slash-commands/join.ts

@@ -0,0 +1,166 @@
+/*
+Copyright 2015, 2016 OpenMarket Ltd
+Copyright 2018 New Vector Ltd
+Copyright 2019 Michael Telatynski <7t3chguy@gmail.com>
+Copyright 2020 The Matrix.org Foundation C.I.C.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+import { MatrixClient } from "matrix-js-sdk/src/matrix";
+
+import { _td } from "../languageHandler";
+import { reject, success } from "./utils";
+import { isPermalinkHost, parsePermalink } from "../utils/permalinks/Permalinks";
+import dis from "../dispatcher/dispatcher";
+import { ViewRoomPayload } from "../dispatcher/payloads/ViewRoomPayload";
+import { Action } from "../dispatcher/actions";
+import { TimelineRenderingType } from "../contexts/RoomContext";
+import { Command } from "./command";
+import { CommandCategories, RunResult } from "./interface";
+
+// A return of undefined here signals a usage error, where the command should return `reject(this.getUsage());`
+function openRoom(cli: MatrixClient, args: string | undefined, autoJoin: boolean): RunResult | undefined {
+    if (!args) return;
+    const params = args.split(" ");
+    if (params.length < 1) return;
+
+    let isPermalink = false;
+    if (params[0].startsWith("http:") || params[0].startsWith("https:")) {
+        // It's at least a URL - try and pull out a hostname to check against the
+        // permalink handler
+        const parsedUrl = new URL(params[0]);
+        const hostname = parsedUrl.host || parsedUrl.hostname; // takes first non-falsey value
+
+        // if we're using a Element permalink handler, this will catch it before we get much further.
+        // see below where we make assumptions about parsing the URL.
+        if (isPermalinkHost(hostname)) {
+            isPermalink = true;
+        }
+    }
+
+    if (params[0][0] === "#") {
+        let roomAlias = params[0];
+        if (!roomAlias.includes(":")) {
+            roomAlias += ":" + cli.getDomain();
+        }
+
+        dis.dispatch<ViewRoomPayload>({
+            action: Action.ViewRoom,
+            room_alias: roomAlias,
+            auto_join: autoJoin,
+            metricsTrigger: "SlashCommand",
+            metricsViaKeyboard: true,
+        });
+        return success();
+    }
+
+    if (params[0][0] === "!") {
+        const [roomId, ...viaServers] = params;
+
+        dis.dispatch<ViewRoomPayload>({
+            action: Action.ViewRoom,
+            room_id: roomId,
+            via_servers: viaServers, // for the rejoin button
+            auto_join: autoJoin,
+            metricsTrigger: "SlashCommand",
+            metricsViaKeyboard: true,
+        });
+        return success();
+    }
+
+    if (isPermalink) {
+        const permalinkParts = parsePermalink(params[0]);
+
+        // This check technically isn't needed because we already did our
+        // safety checks up above. However, for good measure, let's be sure.
+        if (!permalinkParts) {
+            return;
+        }
+
+        // If for some reason someone wanted to join a user, we should
+        // stop them now.
+        if (!permalinkParts.roomIdOrAlias) {
+            return;
+        }
+
+        const entity = permalinkParts.roomIdOrAlias;
+        const viaServers = permalinkParts.viaServers;
+        const eventId = permalinkParts.eventId;
+
+        const dispatch: ViewRoomPayload = {
+            action: Action.ViewRoom,
+            auto_join: autoJoin,
+            metricsTrigger: "SlashCommand",
+            metricsViaKeyboard: true,
+        };
+
+        if (entity[0] === "!") dispatch["room_id"] = entity;
+        else dispatch["room_alias"] = entity;
+
+        if (eventId) {
+            dispatch["event_id"] = eventId;
+            dispatch["highlighted"] = true;
+        }
+
+        if (viaServers) {
+            // For the join, these are passed down to the js-sdk's /join call
+            dispatch["opts"] = { viaServers };
+
+            // For if the join fails (rejoin button)
+            dispatch["via_servers"] = viaServers;
+        }
+
+        dis.dispatch(dispatch);
+        return success();
+    }
+
+    // Otherwise, it's a usage error. Return `undefined`.
+}
+
+// Note: we support 2 versions of this command. The first is
+// the public-facing one for most users and the other is a
+// power-user edition where someone may join via permalink or
+// room ID with optional servers. Practically, this results
+// in the following variations:
+//   /join #example:example.org
+//   /join !example:example.org
+//   /join !example:example.org altserver.com elsewhere.ca
+//   /join https://matrix.to/#/!example:example.org?via=altserver.com
+// The command also supports event permalinks transparently:
+//   /join https://matrix.to/#/!example:example.org/$something:example.org
+//   /join https://matrix.to/#/!example:example.org/$something:example.org?via=altserver.com
+export const join = new Command({
+    command: "join",
+    aliases: ["j"],
+    args: "<room-address>",
+    description: _td("Joins room with given address"),
+    runFn: function (cli, roomId, threadId, args) {
+        return openRoom(cli, args, true) ?? reject(this.getUsage());
+    },
+    category: CommandCategories.actions,
+    renderingTypes: [TimelineRenderingType.Room],
+});
+
+// Similar to join but doesn't auto join the room if you aren't already joined to it
+export const goto = new Command({
+    command: "goto",
+    aliases: ["view"],
+    args: "<room-address>",
+    description: _td("Views room with given address"),
+    runFn: function (cli, roomId, threadId, args) {
+        return openRoom(cli, args, false) ?? reject(this.getUsage());
+    },
+    category: CommandCategories.actions,
+    renderingTypes: [TimelineRenderingType.Room],
+});

test/SlashCommands-test.tsx

@@ -27,6 +27,7 @@ import Modal from "../src/Modal";
 import WidgetUtils from "../src/utils/WidgetUtils";
 import { WidgetType } from "../src/widgets/WidgetType";
 import { warnSelfDemote } from "../src/components/views/right_panel/UserInfo";
+import dispatcher from "../src/dispatcher/dispatcher";
 
 jest.mock("../src/components/views/right_panel/UserInfo");
 
@@ -345,4 +346,58 @@ describe("SlashCommands", () => {
             );
         });
     });
+
+    describe("/join", () => {
+        beforeEach(() => {
+            jest.spyOn(dispatcher, "dispatch");
+            command = findCommand("join")!;
+        });
+
+        it("should return usage if no args", () => {
+            expect(command.run(client, roomId, null, undefined).error).toBe(command.getUsage());
+        });
+
+        it("should handle matrix.org permalinks", () => {
+            command.run(client, roomId, null, "https://matrix.to/#/!roomId:server/$eventId");
+            expect(dispatcher.dispatch).toHaveBeenCalledWith(
+                expect.objectContaining({
+                    action: "view_room",
+                    room_id: "!roomId:server",
+                    event_id: "$eventId",
+                    highlighted: true,
+                }),
+            );
+        });
+
+        it("should handle room aliases", () => {
+            command.run(client, roomId, null, "#test:server");
+            expect(dispatcher.dispatch).toHaveBeenCalledWith(
+                expect.objectContaining({
+                    action: "view_room",
+                    room_alias: "#test:server",
+                }),
+            );
+        });
+
+        it("should handle room aliases with no server component", () => {
+            command.run(client, roomId, null, "#test");
+            expect(dispatcher.dispatch).toHaveBeenCalledWith(
+                expect.objectContaining({
+                    action: "view_room",
+                    room_alias: `#test:${client.getDomain()}`,
+                }),
+            );
+        });
+
+        it("should handle room IDs and via servers", () => {
+            command.run(client, roomId, null, "!foo:bar serv1.com serv2.com");
+            expect(dispatcher.dispatch).toHaveBeenCalledWith(
+                expect.objectContaining({
+                    action: "view_room",
+                    room_id: "!foo:bar",
+                    via_servers: ["serv1.com", "serv2.com"],
+                }),
+            );
+        });
+    });
 });