-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Incorperate Mjolnir's new Policy List Manager #5
Conversation
Gnuxie
commented
Jan 19, 2023
- This should hopefully fix some startup woes matrix-org/mjolnir#462
- PolicyListManager: Let's not forget to remove the room also from the set of rooms we failed to resolve matrix-org/mjolnir#463
// We filter out all policy rooms so that we only protect ones that are | ||
// explicitly protected, so that we don't try to protect lists that we are just watching. | ||
const filterOutManagementAndPolicyRooms = (roomId: string) => { | ||
const policyListIds = this.policyLists.map(list => list.roomId); | ||
const policyListIds = this.policyListManager.lists.map(list => list.roomId); | ||
return roomId !== this.managementRoomId && !policyListIds.includes(roomId); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a smell that this filter has been ignored and not included as part of the PolicyListManager
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that's because it's to do with protected rooms, not the policy list manager... maybe it could be moved there though?
@@ -70,7 +71,7 @@ describe("UnbanBanCommand", () => { | |||
|
|||
it("should be able to detect servers with ban reasons", async () => { | |||
const mjolnir = createTestMjolnir(); | |||
(<any>mjolnir).lists = [{listShortcode: "test"}]; | |||
(<any>mjolnir).policyListManager.lists = [{listShortcode: "test"}]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I think there has been a precedent for exposing these utilities externally from Mjolnir, I think we should at least try keeping them internal to Mjolnir and delegating.
96ee740
to
cb6af7e
Compare
Splitting PolicyListManager from Mjolnir, making it more resilient to startup errors
…set of rooms we failed to resolve (#463)
cb6af7e
to
9797e32
Compare