Allow configuration of multiple moderator users to be invited to all rooms. #234
Conversation
reivilibre
force-pushed
the
rei/fd25_multimods
branch
from
113b526 to
57ffd23
Compare
There was a problem hiding this comment.
The template thing bugs me, but it might just be hard to tell from the review window.
I'd like to say if we have the time then a test (based maybe off https://github.com/matrix-org/conference-bot/blob/main/spec/basic.spec.ts#L24-L48) would be helpful here to ensure behavior.
| @@ -203,7 +203,7 @@ export class E2ETestEnv { | |||
| } | |||
| }, | |||
| }, | |||
| moderatorUserId: `@modbot:${homeserver.domain}`, | |||
| moderatorUserIds: [`@modbot:${homeserver.domain}`], | |||
There was a problem hiding this comment.
Should we attempt a test for this, should be easy to whip one up?
There was a problem hiding this comment.
wrote this now, a little fiddly but I think it's better than nothing? :-)
| // We can't do this directly with `createSpace` unfortunately, as we could for plain rooms. | ||
| await this.client.setUserPowerLevel(this.config.moderatorUserId, subspace.roomId, 100); | ||
| for (let moderator of this.config.moderatorUserIds) { |
There was a problem hiding this comment.
Technically we could convert createSpace to createRoom (with the necessary space state added in) and then just use the getSpace function to pull it as a space object, if we did prefer doing it that way?
There was a problem hiding this comment.
I feel more happy leaving it as it is (in a 'known working' state) than messing TBH. It is a shame that these don't compose well though.
src/models/room_kinds.ts
Outdated
| @@ -149,7 +146,7 @@ export const SPECIAL_INTEREST_CREATION_TEMPLATE = (moderatorUserId: string) => ( | |||
| "m.room.power_levels": 50, | |||
| }, | |||
| }, | |||
| invite: [moderatorUserId], | |||
| invite: moderatorUserIds, | |||
There was a problem hiding this comment.
Hard to tell, but does PUBLIC_ROOM_POWER_LEVELS_TEMPLATE work here? It takes a parameter but we're not giving it one?
There was a problem hiding this comment.
as you point out, this is probably broken. We haven't used special interest rooms for at least a couple of years now so I guess it wouldn't be a surprise. I may as well fix that obvious part up now though
Issue unrelated to this PR, probably been broken for a while
| @@ -30,7 +30,7 @@ export interface IConfig { | |||
| managementRoom: string; | |||
| idServerDomain?: string; | |||
| idServerBrand?: string; | |||
| moderatorUserId: string; | |||
| moderatorUserIds: string[]; | |||
There was a problem hiding this comment.
Do we want to still have the legacy moderatorUserId option, which we internally convert to moderatorUserIds at runtime just for anyone running an old config.
Or failing that, warn on startup that the old field is unused?
There was a problem hiding this comment.
refusing to start up seems like a reasonable option. I don't think we should force ourselves into too much backwards compat given we don't expect anyone else to be running this really. But refusing to start up sounds like a decent footgun-avoider.
We want the ability to invite someone else alongside the moderator bot, in all rooms.
This makes
moderatorUserIdan array instead,moderatorUserIdsbut otherwise the semantics should be all the same?