Skip to content
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

Allow to make guests moderators #1078

Merged
merged 6 commits into from
Oct 5, 2018

Conversation

nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Jul 26, 2018

  • Introduces a new user type (6) GUEST_MODERATOR
  • GMs can do everything moderators can do apart from:
    • Adding users (endpoint in server is not public)
    • Make a room private/public (would kill them too)
  • GM can be assigned via the UI

Fix #1022


ToDo

  • Show participant list to guest moderators

->andWhere($query->expr()->eq('session_id', $query->createNamedParameter($participant->getSessionId())));
$query->execute();

$this->dispatcher->dispatch(self::class . '::postSetParticipantTypeBySession', new GenericEvent($this, [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a listener to notifiy the standalone backend server that the participant has been modified? You can use this as base:

$dispatcher->addListener(Room::class . '::postRemoveBySession', function(GenericEvent $event) {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -525,14 +525,19 @@ public function removeFromFavorites($token) {
public function renameRoom($token, $roomName) {
try {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same try { ... } block is in multiple functions now, can this somehow be refactored in a separate function to avoid code duplication?

Maybe a new decorator similar to @NoAdminRequired, e.g. @TalkModeratorRequired that performs the code and checks for hasModeratorPermissions().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, something for later.
I'm not sure if apps can already register middleware annotations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we would then also get the room twice etc, which is kind of dislike about the annotations.

@nickvergessen
Copy link
Member Author

@danxuliu any idea how we can only show the participant list for guest moderators (not all guests)? See last commit

lib/Controller/RoomController.php Outdated Show resolved Hide resolved
lib/Controller/RoomController.php Outdated Show resolved Hide resolved
@danxuliu danxuliu force-pushed the feature/noid/guest-moderators-prework branch from 2b09233 to f09dfec Compare August 30, 2018 15:19
@Peterede
Copy link
Contributor

Peterede commented Sep 24, 2018

The participant list for guests is not added by default and requires calling _showParticipantList(); and then _participantsView.setRoom(this.activeRoom); so guests who are not moderators will not see the list.

@nickvergessen nickvergessen force-pushed the feature/noid/guest-moderators-prework branch from 0853d0a to abff5d1 Compare September 28, 2018 09:10
@nickvergessen
Copy link
Member Author

#1215 is in @danxuliu
Please rebase here 😎

@danxuliu danxuliu force-pushed the feature/noid/guest-moderators-prework branch from abff5d1 to 17ebeb8 Compare October 2, 2018 10:55
@danxuliu
Copy link
Member

danxuliu commented Oct 2, 2018

I have rebased the branch and fixed some issues:

  • Fix removing a participant (@NoAdminRequired changed to @PublicPage)
  • Fix demoting as a moderator ($this->userId === $participant in demoteModerator() is always true as a guest moderator; the check was moved to demoteUserToModerator() and demoteGuestToModerator())

I have squashed in the first commit the commits that fixed code added in it; I have added some other commits with changes to it, but not squashed them (yet) just in case the changes are not desired so they can be easily removed (the rationale for the changes is in the commit message).

Other issues still remain, though.

The avatars in the chat are not properly shown when viewed as a guest user; it is not related to the changes in this pull request, so it will be fixed in a different one.

@nickvergessen There are no system messages when promoting/demoting a guest. Is this by design or simply missing?

@Peterede I agree that demoteUserToModerator and demoteGuestToModerator should be called demoteUserFromModerator and demoteGuestFromModerator instead, but according to GitHub you marked that as resolved (even if the old names are still used). What am I missing? :-)

Also, @Peterede, could you provide some steps that show why the change in js/views/tabview.js is needed? I have been trying to trigger the error (without that commit, of course :-P ) and I have not been able to; I guess it was just caused by some of the WIP code and it no longer applies, so I have removed the commit (although I have a reference to it just in case it has to be added back).

@nickvergessen nickvergessen force-pushed the feature/noid/guest-moderators-prework branch from 17ebeb8 to f91e79b Compare October 2, 2018 15:00
@nickvergessen
Copy link
Member Author

I removed the two commits as discussed.

I fixed the name to demote*FromModerator (copy paste mistake).

And I added new system notifications for promotion/demotion of guest moderators as well as the "You" detection for guest moderators.

@nickvergessen
Copy link
Member Author

Should be ready to review now?

Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
@danxuliu danxuliu force-pushed the feature/noid/guest-moderators-prework branch from 6a3de52 to 7e1de89 Compare October 3, 2018 23:48
@danxuliu
Copy link
Member

danxuliu commented Oct 3, 2018

I have removed the fix for the avatars, because it broke the contacts menu and the avatars in messages being composed. As I already said I will fix the issue with the avatars in a different pull request, because it is not related to the changes in this one.

I have also squashed the fixes for the first commit in that commit.

Besides that I found a bug with the participant menu (the toggle is not hidden) when demoting a user (guests work fine because the list is added and removed altogether instead of updated); I have to investigate it further, although I guess that it is caused by not listening to changes in the participant type of the current user.

Should be ready to review now?

Yes; 👍 from me.

@nickvergessen
Copy link
Member Author

im fixing the unit tests

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen merged commit 546526a into master Oct 5, 2018
@nickvergessen nickvergessen deleted the feature/noid/guest-moderators-prework branch October 5, 2018 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants