-
Notifications
You must be signed in to change notification settings - Fork 54
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
fix config.protectAllJoinedRooms
leaking into explicitly protected rooms
#385
Conversation
0b81b8b
to
ed59727
Compare
await this.protectedRoomsConfig.loadProtectedRoomsFromConfig(this.config); | ||
await this.protectedRoomsConfig.loadProtectedRoomsFromAccountData(); | ||
this.protectedRoomsConfig.getExplicitlyProtectedRooms().forEach(this.protectRoom, this); |
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.
Can't this bit be replaced with a single call to await this.resyncJoinedRooms()
?
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.
Nope, because resyncJoinedRooms
doesn't do anything (returns early) when config.protectAllJoinedRooms
is disabled.
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.
Note: There could be a flag to resyncJoinedRooms
to prevent this early return. Your call.
ed59727
to
6009555
Compare
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.
That will make things much nicer!
private unprotectedWatchedListRooms: string[] = []; | ||
public readonly protectedRoomsTracker: ProtectedRooms; | ||
|
||
private protectedRoomsConfig: ProtectedRoomsConfig; |
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.
😄
// 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); |
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.
Why do we rebuild policyListsId
at every call?
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.
Literally: because Array.prototype.includes
doesn't accept a key function where i can put list.roomId
instead.
Really: I just find it much simpler than tracking yet another group/category of rooms. I don't think it is costing us much to do this either.
@@ -506,13 +485,7 @@ export class Mjolnir { | |||
|
|||
public async warnAboutUnprotectedPolicyListRoom(roomId: string) { | |||
if (!this.config.protectAllJoinedRooms) return; // doesn't matter | |||
if (this.explicitlyProtectedRoomIds.includes(roomId)) return; // explicitly protected | |||
|
|||
const createEvent = new CreateEvent(await this.client.getRoomStateEvent(roomId, "m.room.create", "")); |
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.
Do you understand what that was for?
I don't see any similar check in your new code, right? Should there be such a check?
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.
Yes, we remove this check because it's not an accurate way to determine whether Mjolnir should protect the policy room. Especially if another Mjolnir/user created it, or if some other Mjolnir is now responsible for that list.
I mentioned it in the description of the PR too
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.
We replace it with needing policy rooms to be explicitly protected in all circumstances and by showing you which policy rooms are protected distinctly when you use the status command (screenshot attached in PR too)
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.
We also still warn when you first watch a policy room and it is unprotected.
@@ -24,7 +24,7 @@ export async function execListProtectedRooms(roomId: string, event: any, mjolnir | |||
let text = `Protected rooms (${rooms.length}):\n`; | |||
|
|||
let hasRooms = false; | |||
for (const protectedRoomId in rooms) { | |||
for (const protectedRoomId of rooms) { |
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.
Ouch, good catch!
How did we let that pass?
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.
Yep, this was unfortunate.
src/commands/StatusCommand.ts
Outdated
if (mjolnir.lists.length === 0) { | ||
html += "<li><i>None</i></li>"; | ||
text += "* None\n"; | ||
// FIXME: Make an issue to rename protectedRoomsTracker to just protectedRooms. |
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.
Please do :)
const subscribedLists = mjolnir.lists.filter(list => !mjolnir.explicitlyProtectedRooms.includes(list.roomId)); | ||
renderPolicyLists("Subscribed policy lists", subscribedLists); | ||
const subscribedAndProtectedLists = mjolnir.lists.filter(list => mjolnir.explicitlyProtectedRooms.includes(list.roomId)); | ||
renderPolicyLists("Subscribed and protected policy lists", subscribedAndProtectedLists); |
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.
Nicer, thanks!
6009555
to
e7b32c3
Compare
The combination of `resyncJoinedRooms`, `unprotectedWatchedListRooms`, `explicitlyProtectedRoomIds`, `protectedJoinedRoomIds` was incomprehensible. #370 Separating out the management of `explicitlyProtectedRoomIds`, then making sure all policy lists have to be explicitly protected (in either setting of `config.protectAllJoinedRooms`) will make this code much much simpler. We will later change the `status` command to explicitly show which lists are watched and which are watched and protected.
e7b32c3
to
7986b0f
Compare
7986b0f
to
04b3bc6
Compare
2155669
to
048891c
Compare
Closes #370
Covers all of the points we wanted to refactor on in #370 too.
This PR factors out the management of
explicitlyProtectedRoomIds
into aProtectedRoomsConfig
class which also manages persisting the list to account data.It then nukes
protectedJoinedRoomIds
,unprotectedWatchedListRooms
andexplicitlyProtectedRoomIds
from Mjolnir.If
config.protectAllJoinedRooms
is true, rather than tracking which policy lists should not be protected based on the create event of the list (which is whatunprotectedWatchedListRooms
was for), we instead require that lists be explicitly protected. This will already be the case if the list was created with Mjolnir.To make sure it is obvious that a list is unprotected, the status command will also distinguish between watched and protected lists.
This in turn makes the code around
resyncJoinedRooms
that implementsconfig.protectAllJoinedRooms
much much simpler.