mjolnir icon indicating copy to clipboard operation
mjolnir copied to clipboard

Refactor syncing protected rooms with ban lists

Open Gnuxie opened this issue 3 years ago • 0 comments

While working on #221 it became clear there's a lot of overlap and going back and forwards between Mjolnir.syncLists(), Mjolnir.syncWithBanList() and BanList.updateList()

Basically we have Mjolnir that has all the BanLists, Mjolnir currently is in control of calling updateList() on them. Each time it gets an event from sync from a ban list it calls BanList.updateForEvent() to be batched by the batcher BanList has, which will later call BanList.updateList().

Unfortunately when you let BanList control when updateList() is called (because of new events from sync), you need some way of going back to Mjolnir to call Mjolnir.syncWithBanList() or an equivalent. We currently have the BanList.batch event on the BanList event emitter to do this job because:

  1. Using a callback and giving it each time to BanList.updateForEvent() would be strange as the callback is only called when there is a batch, and we are always giving the same callback and only want it to be called once. So we would have a really unintuitive interface where even though we provide a callback for every event, we only call one of them once each batch.

  2. Using the existing BanList.update event (instead of the new BanList.batchevent) requires refactoring Mjolnir.syncLists() because when Mjolnir calls syncsList (to initialize the banLists for the first time or because someone used the mjolnir! sync command) we would also be calling the listener for when the batcher calls BanList.updateList (that is replacing Mjolnir.syncWithBanList() and both of them apply bans, acl and do printing. We don't want that. So we would have had to refactor Mjolnir.syncLists to also not do those things and we would have to consider that syncLists reports all the errors at the same time for all lists (synchronously from the perspective of the command executor), and with the BanList.update listener would instead be reporting each individually (there are also different uses of the verbosity config setting between the two aswell just to complicate things further) which is a change of behaviour.

Gnuxie avatar Feb 15 '22 13:02 Gnuxie