fix(mattermost): address review feedback on reactions PR

This commit is contained in:
Echo
2026-02-16 17:34:36 -05:00
committed by Peter Steinberger
parent 2a2372cd6c
commit 82861968c2
5 changed files with 128 additions and 33 deletions

View File

@@ -85,6 +85,56 @@ describe("mattermostPlugin", () => {
expect(actions).not.toContain("send");
});
it("respects per-account actions.reactions in listActions", () => {
const cfg: OpenClawConfig = {
channels: {
mattermost: {
enabled: true,
actions: { reactions: false },
accounts: {
default: {
enabled: true,
botToken: "test-token",
baseUrl: "https://chat.example.com",
actions: { reactions: true },
},
},
},
},
};
const actions = mattermostPlugin.actions?.listActions?.({ cfg }) ?? [];
expect(actions).toContain("react");
});
it("blocks react when default account disables reactions and accountId is omitted", async () => {
const cfg: OpenClawConfig = {
channels: {
mattermost: {
enabled: true,
actions: { reactions: true },
accounts: {
default: {
enabled: true,
botToken: "test-token",
baseUrl: "https://chat.example.com",
actions: { reactions: false },
},
},
},
},
};
await expect(
mattermostPlugin.actions?.handleAction?.({
channel: "mattermost",
action: "react",
params: { messageId: "POST1", emoji: "thumbsup" },
cfg,
} as any),
).rejects.toThrow("Mattermost reactions are disabled in config");
});
it("handles react by calling Mattermost reactions API", async () => {
const cfg: OpenClawConfig = {
channels: {

View File

@@ -30,22 +30,22 @@ import { getMattermostRuntime } from "./runtime.js";
const mattermostMessageActions: ChannelMessageActionAdapter = {
listActions: ({ cfg }) => {
const accounts = listMattermostAccountIds(cfg)
const actionsConfig = cfg.channels?.mattermost?.actions as { reactions?: boolean } | undefined;
const baseReactions = actionsConfig?.reactions;
const hasReactionCapableAccount = listMattermostAccountIds(cfg)
.map((accountId) => resolveMattermostAccount({ cfg, accountId }))
.filter((account) => account.enabled)
.filter((account) => Boolean(account.botToken?.trim() && account.baseUrl?.trim()));
.filter((account) => Boolean(account.botToken?.trim() && account.baseUrl?.trim()))
.some((account) => {
const accountActions = account.config.actions as { reactions?: boolean } | undefined;
return (accountActions?.reactions ?? baseReactions ?? true) !== false;
});
if (accounts.length === 0) {
if (!hasReactionCapableAccount) {
return [];
}
const actions: ChannelMessageActionName[] = [];
const actionsConfig = cfg.channels?.mattermost?.actions as { reactions?: boolean } | undefined;
const reactionsEnabled = actionsConfig?.reactions !== false;
if (reactionsEnabled) {
actions.push("react");
}
return actions;
return ["react"];
},
supportsAction: ({ action }) => {
return action === "react";
@@ -57,7 +57,8 @@ const mattermostMessageActions: ChannelMessageActionAdapter = {
// Check reactions gate: per-account config takes precedence over base config
const mmBase = cfg?.channels?.mattermost as Record<string, unknown> | undefined;
const accounts = mmBase?.accounts as Record<string, Record<string, unknown>> | undefined;
const acctConfig = accountId && accounts ? accounts[accountId] : undefined;
const resolvedAccountId = accountId ?? resolveDefaultMattermostAccountId(cfg);
const acctConfig = accounts?.[resolvedAccountId];
const acctActions = acctConfig?.actions as { reactions?: boolean } | undefined;
const baseActions = mmBase?.actions as { reactions?: boolean } | undefined;
const reactionsEnabled = acctActions?.reactions ?? baseActions?.reactions ?? true;
@@ -88,7 +89,7 @@ const mattermostMessageActions: ChannelMessageActionAdapter = {
cfg,
postId,
emojiName,
accountId: accountId ?? undefined,
accountId: resolvedAccountId,
});
if (!result.ok) {
throw new Error(result.error);
@@ -105,7 +106,7 @@ const mattermostMessageActions: ChannelMessageActionAdapter = {
cfg,
postId,
emojiName,
accountId: accountId ?? undefined,
accountId: resolvedAccountId,
});
if (!result.ok) {
throw new Error(result.error);

View File

@@ -59,16 +59,9 @@ type CreateMattermostConnectOnceOpts = {
export const defaultMattermostWebSocketFactory: MattermostWebSocketFactory = (url) =>
new WebSocket(url) as MattermostWebSocketLike;
export function parsePostedEvent(
data: WebSocket.RawData,
export function parsePostedPayload(
payload: MattermostEventPayload,
): { payload: MattermostEventPayload; post: MattermostPost } | null {
const raw = rawDataToString(data);
let payload: MattermostEventPayload;
try {
payload = JSON.parse(raw) as MattermostEventPayload;
} catch {
return null;
}
if (payload.event !== "posted") {
return null;
}
@@ -92,6 +85,19 @@ export function parsePostedEvent(
return { payload, post };
}
export function parsePostedEvent(
data: WebSocket.RawData,
): { payload: MattermostEventPayload; post: MattermostPost } | null {
const raw = rawDataToString(data);
let payload: MattermostEventPayload;
try {
payload = JSON.parse(raw) as MattermostEventPayload;
} catch {
return null;
}
return parsePostedPayload(payload);
}
export function createMattermostConnectOnce(
opts: CreateMattermostConnectOnceOpts,
): () => Promise<void> {
@@ -160,7 +166,7 @@ export function createMattermostConnectOnce(
if (payload.event !== "posted") {
return;
}
const parsed = parsePostedEvent(data);
const parsed = parsePostedPayload(payload);
if (!parsed) {
return;
}

View File

@@ -49,6 +49,36 @@ describe("mattermost reactions", () => {
expect(fetchImpl).toHaveBeenCalled();
});
it("returns a Result error when add reaction API call fails", async () => {
const fetchImpl = vi.fn(async (url: any) => {
if (String(url).endsWith("/api/v4/users/me")) {
return new Response(JSON.stringify({ id: "BOT123" }), {
status: 200,
headers: { "content-type": "application/json" },
});
}
if (String(url).endsWith("/api/v4/reactions")) {
return new Response(JSON.stringify({ id: "err", message: "boom" }), {
status: 500,
headers: { "content-type": "application/json" },
});
}
throw new Error(`unexpected url: ${url}`);
});
const result = await addMattermostReaction({
cfg: createCfg(),
postId: "POST1",
emojiName: "thumbsup",
fetchImpl,
});
expect(result.ok).toBe(false);
if (!result.ok) {
expect(result.error).toContain("Mattermost add reaction failed");
}
});
it("removes reactions by calling /users/me then DELETE /users/:id/posts/:postId/reactions/:emoji", async () => {
const fetchImpl = vi.fn(async (url: any, init?: any) => {
if (String(url).endsWith("/api/v4/users/me")) {

View File

@@ -50,11 +50,15 @@ export async function addMattermostReaction(params: {
return { ok: false, error: "Mattermost reactions failed: could not resolve bot user id." };
}
await createReaction(client, {
userId,
postId: params.postId,
emojiName: params.emojiName,
});
try {
await createReaction(client, {
userId,
postId: params.postId,
emojiName: params.emojiName,
});
} catch (err) {
return { ok: false, error: `Mattermost add reaction failed: ${String(err)}` };
}
return { ok: true };
}
@@ -85,11 +89,15 @@ export async function removeMattermostReaction(params: {
return { ok: false, error: "Mattermost reactions failed: could not resolve bot user id." };
}
await deleteReaction(client, {
userId,
postId: params.postId,
emojiName: params.emojiName,
});
try {
await deleteReaction(client, {
userId,
postId: params.postId,
emojiName: params.emojiName,
});
} catch (err) {
return { ok: false, error: `Mattermost remove reaction failed: ${String(err)}` };
}
return { ok: true };
}