fix missing broadcast to all users

[pr4xx]

I think this sould resolve issues related to pads using regular as well as read only pad ids.
This commit basically updates each broadcast to include both ids. For that, the commentManager functions return both padIds in their callbacks.

[pr4xx]

I changed each occurence of readOnlyManager to use the getIds method. That way both ids are available independent of the given input id.

[JohnMcLear]

Really good stuff, think it's worth putting some test coverage in just so I can be sure it doesn't get broken by someone else in the future? 👍

[JohnMcLear]

Also, does read only before this PR = no comments or does it mean comments are visible but they don't update until refresh? If it's the latter that's fine.. If it's the former then we would need to add this under a setting. Having read only where comments aren't visible may be ideal to some.

[pr4xx]

for reference: #68 This comment

I am not really sure what happens prior to both my PRs.
My guess: at one point in time, etherpads core updated the readOnlyManager to use promises. That broke the translation in this plugin in commentManager whenever a read-only id was used.
My first PR solved that translation. But I noticed later that live syncing between non-read-only and read-only clients is not working. The problem was that the broadcast in index targeted the original id coming from the websocket. So all read-only clients could see their comments live. Same thing with all non-read-only clients. But not "between" those worlds.
My second (this) PR fixed this by sending each broadcast twice (to regular and read-only id).

When this PR gets merged, both "worlds" can see each other and even can edit each others comments. I am not sure if this is desired behaviour. Basically any "guest" (read-only) can edit and delete "admin" comments. I think #125 plays a role here.

So to fix this entirely I think we need to:

• add configuration options for how comments should be handled between regular and read-only ids
• fix authorization / add options for read-only permissions (guests can only read comments but not add own comments?)

I will add tests as soon as I find the time.

[JohnMcLear]

FWIW I'm finding the ep_comments tests pretty damn unreliable. I'm trying to narrow it down and get them stable now.

[JohnMcLear]

bumping @rhansen

[rhansen]

Heads up: I just rebased your branch and force-pushed. git pull should do the right thing.

[rhansen]

Looks good, thank you!

Would you mind updating the apiAddComments and apiAddCommentReplies events as well? Edit: I pushed a commit to your branch that updates them.

There are some pre-existing issues (not the fault of this PR) that should be addressed in separate PRs:

• There's an awkward mix of async and callbacks. Edit: This is now fixed (server-side anyway).
• There are a few significant security problems. (I don't want to go into details publicly until I've had a chance to patch them up.)
• Not all hook functions call the callback like they should. Edit: This is now fixed.

[rhansen]

I had a thought: Rather than call readOnlyManager.getIds(padId) deep in the code and propagate the IDs back up through the callbacks, perhaps the socket.io handlers should call readOnlyManager.getIds(padId) themselves and always pass the writable pad ID down. What do you think?

[rhansen]

I pushed a commit to your branch that moves the readOnlyManager.getIds() calls to the socket.io handlers. It also updates the apiAddComments and apiAddCommentReplies handlers to match the others. If the end result looks good to you we can squash it into your commit.

[rhansen]

I was curious about how Etherpad core handles read-write vs. read-only users for normal edit messages, and it looks like it puts both types of users in the same socket.io "room". I think we should do that here for consistency. I'll push another commit.

[rhansen]

I'll push another commit.

Done, and I rebased onto latest master. Let me know what you think @pr4xx.

[rhansen]

I went ahead and merged this. Thanks for the fix @pr4xx!