Move role modifying method to the model

and also first check whether we have an occupant with that nickname.

Since roles are session based, it doesn't make any sense to try to
modify the role for a user not in the room.

spec/muc.js

@@ -3145,7 +3145,7 @@
                 done();
             }));
 
-            it("takes /kick to kick a user",
+            it("accepts a /kick command to kick a user",
                 mock.initConverse(
                     null, ['rosterGroupsFetched'], {},
                     async function (done, _converse) {
@@ -3159,7 +3159,7 @@
 
                 await test_utils.openAndEnterChatRoom(_converse, 'lounge', 'localhost', 'dummy');
                 const view = _converse.chatboxviews.get('lounge@localhost');
-                spyOn(view, 'modifyRole').and.callThrough();
+                spyOn(view.model, 'setRole').and.callThrough();
                 spyOn(view, 'showErrorMessage').and.callThrough();
                 spyOn(view, 'validateRoleChangeCommand').and.callThrough();
 
@@ -3176,7 +3176,7 @@
                         });
                 _converse.connection._dataRecv(test_utils.createRequest(presence));
 
-                var textarea = view.el.querySelector('.chat-textarea')
+                const textarea = view.el.querySelector('.chat-textarea')
                 textarea.value = '/kick';
                 view.onKeyDown({
                     target: textarea,
@@ -3186,7 +3186,7 @@
                 expect(view.validateRoleChangeCommand).toHaveBeenCalled();
                 expect(view.showErrorMessage).toHaveBeenCalledWith(
                     "Error: the \"kick\" command takes two arguments, the user's nickname and optionally a reason.");
-                expect(view.modifyRole).not.toHaveBeenCalled();
+                expect(view.model.setRole).not.toHaveBeenCalled();
                 // Call now with the correct amount of arguments.
                 // XXX: Calling onFormSubmitted directly, trying
                 // again via triggering Event doesn't work for some weird
@@ -3196,7 +3196,7 @@
 
                 expect(view.validateRoleChangeCommand.calls.count()).toBe(2);
                 expect(view.showErrorMessage.calls.count()).toBe(1);
-                expect(view.modifyRole).toHaveBeenCalled();
+                expect(view.model.setRole).toHaveBeenCalled();
                 expect(sent_IQ.toLocaleString()).toBe(
                     `<iq id="${IQ_id}" to="lounge@localhost" type="set" xmlns="jabber:client">`+
                         `<query xmlns="http://jabber.org/protocol/muc#admin">`+
@@ -3247,7 +3247,7 @@
                     IQ_id = sendIQ.bind(this)(iq, callback, errback);
                 });
                 var view = _converse.chatboxviews.get('lounge@localhost');
-                spyOn(view, 'modifyRole').and.callThrough();
+                spyOn(view.model, 'setRole').and.callThrough();
                 spyOn(view, 'showErrorMessage').and.callThrough();
                 spyOn(view, 'showChatEvent').and.callThrough();
                 spyOn(view, 'validateRoleChangeCommand').and.callThrough();
@@ -3289,7 +3289,7 @@
                 expect(view.showErrorMessage).toHaveBeenCalledWith(
                     "Error: the \"op\" command takes two arguments, the user's nickname and optionally a reason.");
 
-                expect(view.modifyRole).not.toHaveBeenCalled();
+                expect(view.model.setRole).not.toHaveBeenCalled();
                 // Call now with the correct amount of arguments.
                 // XXX: Calling onFormSubmitted directly, trying
                 // again via triggering Event doesn't work for some weird
@@ -3299,7 +3299,7 @@
 
                 expect(view.validateRoleChangeCommand.calls.count()).toBe(2);
                 expect(view.showErrorMessage.calls.count()).toBe(1);
-                expect(view.modifyRole).toHaveBeenCalled();
+                expect(view.model.setRole).toHaveBeenCalled();
                 expect(sent_IQ.toLocaleString()).toBe(
                     `<iq id="${IQ_id}" to="lounge@localhost" type="set" xmlns="jabber:client">`+
                         `<query xmlns="http://jabber.org/protocol/muc#admin">`+
@@ -3341,7 +3341,7 @@
 
                 expect(view.validateRoleChangeCommand.calls.count()).toBe(3);
                 expect(view.showChatEvent.calls.count()).toBe(1);
-                expect(view.modifyRole).toHaveBeenCalled();
+                expect(view.model.setRole).toHaveBeenCalled();
                 expect(sent_IQ.toLocaleString()).toBe(
                     `<iq id="${IQ_id}" to="lounge@localhost" type="set" xmlns="jabber:client">`+
                         `<query xmlns="http://jabber.org/protocol/muc#admin">`+
@@ -3389,7 +3389,7 @@
                     IQ_id = sendIQ.bind(this)(iq, callback, errback);
                 });
                 var view = _converse.chatboxviews.get('lounge@localhost');
-                spyOn(view, 'modifyRole').and.callThrough();
+                spyOn(view.model, 'setRole').and.callThrough();
                 spyOn(view, 'showErrorMessage').and.callThrough();
                 spyOn(view, 'showChatEvent').and.callThrough();
                 spyOn(view, 'validateRoleChangeCommand').and.callThrough();
@@ -3430,7 +3430,7 @@
                 expect(view.validateRoleChangeCommand).toHaveBeenCalled();
                 expect(view.showErrorMessage).toHaveBeenCalledWith(
                     "Error: the \"mute\" command takes two arguments, the user's nickname and optionally a reason.");
-                expect(view.modifyRole).not.toHaveBeenCalled();
+                expect(view.model.setRole).not.toHaveBeenCalled();
                 // Call now with the correct amount of arguments.
                 // XXX: Calling onFormSubmitted directly, trying
                 // again via triggering Event doesn't work for some weird
@@ -3440,7 +3440,7 @@
 
                 expect(view.validateRoleChangeCommand.calls.count()).toBe(2);
                 expect(view.showErrorMessage.calls.count()).toBe(1);
-                expect(view.modifyRole).toHaveBeenCalled();
+                expect(view.model.setRole).toHaveBeenCalled();
                 expect(sent_IQ.toLocaleString()).toBe(
                     `<iq id="${IQ_id}" to="lounge@localhost" type="set" xmlns="jabber:client">`+
                         `<query xmlns="http://jabber.org/protocol/muc#admin">`+
@@ -3483,7 +3483,7 @@
 
                 expect(view.validateRoleChangeCommand.calls.count()).toBe(3);
                 expect(view.showChatEvent.calls.count()).toBe(1);
-                expect(view.modifyRole).toHaveBeenCalled();
+                expect(view.model.setRole).toHaveBeenCalled();
                 expect(sent_IQ.toLocaleString()).toBe(
                     `<iq id="${IQ_id}" to="lounge@localhost" type="set" xmlns="jabber:client">`+
                         `<query xmlns="http://jabber.org/protocol/muc#admin">`+

src/converse-muc-views.js

@@ -96,6 +96,8 @@ converse.plugins.add('converse-muc-views', {
             }
         });
 
+        const OCCUPANT_NOT_FOUND = __("Could not find an occupant with that nickname");
+
 
         function renderRoomsPanel () {
             if (this.roomspanel && u.isVisible(this.roomspanel.el)) {
@@ -862,13 +864,6 @@ converse.plugins.add('converse-muc-views', {
                 return _converse.api.sendIQ(iq);
             },
 
-            modifyRole (groupchat, nick, role, reason, onSuccess, onError) {
-                const item = $build("item", {nick, role});
-                const iq = $iq({to: groupchat, type: "set"}).c("query", {xmlns: Strophe.NS.MUC_ADMIN}).cnode(item.node);
-                if (reason !== null) { iq.c("reason", reason); }
-                return _converse.api.sendIQ(iq).then(onSuccess).catch(onError);
-            },
-
             verifyRoles (roles, occupant, show_error=true) {
                 if (!occupant) {
                     occupant = this.model.occupants.findWhere({'jid': _converse.bare_jid});
@@ -974,8 +969,13 @@ converse.plugins.add('converse-muc-views', {
                         if (!this.verifyAffiliations(['admin', 'owner']) || !this.validateRoleChangeCommand(command, args)) {
                             break;
                         }
-                        this.modifyRole(
-                            this.model.get('jid'), args[0], 'participant', args[1],
+                        const occupant = this.model.getOccupantByNickname(args[0]);
+                        if (!occupant) {
+                            this.showErrorMessage(OCCUPANT_NOT_FOUND);
+                            break;
+                        }
+                        this.model.setRole(
+                            occupant, 'participant', args[1],
                             undefined, this.onCommandError.bind(this));
                         break;
                     }
@@ -1033,18 +1033,28 @@ converse.plugins.add('converse-muc-views', {
                         if (!this.verifyRoles(['moderator']) || !this.validateRoleChangeCommand(command, args)) {
                             break;
                         }
-                        this.modifyRole(
-                                this.model.get('jid'), args[0], 'none', args[1],
-                                undefined, this.onCommandError.bind(this));
+                        const occupant = this.model.getOccupantByNickname(args[0]);
+                        if (!occupant) {
+                            this.showErrorMessage(OCCUPANT_NOT_FOUND);
+                            break;
+                        }
+                        this.model.setRole(
+                            occupant, 'none', args[1],
+                            undefined, this.onCommandError.bind(this));
                         break;
                     }
                     case 'mute': {
                         if (!this.verifyRoles(['moderator']) || !this.validateRoleChangeCommand(command, args)) {
                             break;
                         }
-                        this.modifyRole(
-                                this.model.get('jid'), args[0], 'visitor', args[1],
-                                undefined, this.onCommandError.bind(this));
+                        const occupant = this.model.getOccupantByNickname(args[0]);
+                        if (!occupant) {
+                            this.showErrorMessage(OCCUPANT_NOT_FOUND);
+                            break;
+                        }
+                        this.model.setRole(
+                            occupant, 'visitor', args[1],
+                            undefined, this.onCommandError.bind(this));
                         break;
                     }
                     case 'member': {
@@ -1092,9 +1102,14 @@ converse.plugins.add('converse-muc-views', {
                         if (!this.verifyAffiliations(['admin', 'owner']) || !this.validateRoleChangeCommand(command, args)) {
                             break;
                         }
-                        this.modifyRole(
-                                this.model.get('jid'), args[0], 'moderator', args[1],
-                                undefined, this.onCommandError.bind(this));
+                        const occupant = this.model.getOccupantByNickname(args[0]);
+                        if (!occupant) {
+                            this.showErrorMessage();
+                            break;
+                        }
+                        this.model.setRole(
+                            occupant, 'moderator', args[1],
+                            undefined, this.onCommandError.bind(this));
                         break;
                     }
                     case 'register': {
@@ -1135,9 +1150,15 @@ converse.plugins.add('converse-muc-views', {
                         if (!this.verifyRoles(['moderator']) || !this.validateRoleChangeCommand(command, args)) {
                             break;
                         }
-                        this.modifyRole(
-                                this.model.get('jid'), args[0], 'participant', args[1],
-                                undefined, this.onCommandError.bind(this));
+                        const occupant = this.model.getOccupantByNickname(args[0]);
+                        if (!occupant) {
+                            this.showErrorMessage(OCCUPANT_NOT_FOUND);
+                            break;
+                        }
+                        this.model.setRole(
+                            occupant, 'participant', args[1], undefined,
+                            this.onCommandError.bind(this)
+                        );
                         break;
                     }
                     default:

src/headless/converse-muc.js

@@ -846,6 +846,41 @@ converse.plugins.add('converse-muc', {
                 return Promise.all(_.map(affiliations, _.partial(this.setAffiliation.bind(this), _, members)));
             },
 
+            /**
+             * Send an IQ stanza to modify an occupant's role
+             * @private
+             * @method _converse.ChatRoom#setRole
+             * @param { _converse.ChatRoomOccupant } occupant
+             * @param { String } role
+             * @param { String } reason
+             * @param { function } onSuccess - callback for a succesful response
+             * @param { function } onError - callback for an error response
+             */
+            setRole (occupant, role, reason, onSuccess, onError) {
+                const item = $build("item", {
+                    'nick': occupant.get('nick'),
+                    role
+                });
+                const iq = $iq({
+                    'to': this.get('jid'),
+                    'type': 'set'
+                }).c("query", {xmlns: Strophe.NS.MUC_ADMIN}).cnode(item.node);
+                if (reason !== null) {
+                    iq.c("reason", reason);
+                }
+                return _converse.api.sendIQ(iq).then(onSuccess).catch(onError);
+            },
+
+            /**
+             * @private
+             * @method _converse.ChatRoom#getOccupantByNickname
+             * @param { String } nick - The nickname of the occupant to be returned
+             * @returns { _converse.ChatRoomOccupant }
+             */
+            getOccupantByNickname (nick) {
+                return this.occupants.findWhere({'nick': nick});
+            },
+
             async getJidsWithAffiliations (affiliations) {
                 /* Returns a map of JIDs that have the affiliations
                  * as provided.