[IMPROVE][Omnichannel] Allow set other agent status via method livechat:changeLivechatStatus

[MartinSchoeler]

Proposed changes

This allows to other users to change the status of a livechat agent, given that the user has the appropriate permission.
It should not break anything that uses this method.

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• Improvement (non-breaking change which improves a current function)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Hotfix (a major bugfix that has to be merged asap)
• Documentation Update (if none of the other choices apply)

Checklist

• I have read the CONTRIBUTING doc
• I have signed the CLA
• Lint and unit tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works (if applicable)
• I have added necessary documentation (if applicable)
• Any dependent changes have been merged and published in downstream modules

[renatobecker]

I left some suggestions in order to reduce the complexity of the code.

[renatobecker]

Suggested change

	import { hasPermission, hasRole } from '../../../authorization';
	import { hasPermission } from '../../../authorization';

[renatobecker]

Suggested change

	import { Users } from '../../../models';
	import { Users } from '../../../models';
	const getUser = (userId, agentId, options = {}) => {
	if (!agentId) {
	return Users.findOneById(userId, options);
	}
	if (!hasPermission(userId, 'manage-livechat-agents')) {
	throw new Meteor.Error('error-not-allowed', 'Not allowed', { method: 'livechat:changeLivechatStatus' });
	}
	return Users.findOneAgentById(agentId, options);
	};

[ggazzo]

I do really disagree about these 'helpers functions' it creates a special complexity and does not help if we perform a global search... and getUser means almost nothing

[ggazzo]

and the permission manage-livechat-agents does not look related to getUser why I can't get a user if I'm not a manager? (I know why, but I'm questioning the function purpose)

[renatobecker]

There are other(better) options to implement this functionally:

• REST API endpoint
• Specific Meteor method

The complexity is increasing because the current method wasn't designed to support calls from other users other than the user who will be impacted by this action. So, that's exactly the reason a helper(local) method has been suggested, the purpose is to avoid adding more and specific code inside the main method.
The main method just needs to get a valid user, the logic/permission doesn't matter and that's the reason a helper method will avoid more complexity.

The scope of the helper function is local, which means its context and its name is local, the helper method is not being exported so, anyone may be able to know that when searching globally if it's the case.

The point is that when the user who is calling this method isn't the same(agent) who will get their service status changed, then the user who is performing the action MUST have the permission mentioned in the code.
Also, after performing the necessary validation, a User is expected as a result and this is the reason for the name suggested, but the developer can define the helper name he wants, even the helper method is local.

[ggazzo]

I'm not talking about if this method should or not perform both actions, it's an omnichannel design/concern...
I'm only talking about creating small functions to do whatever we want just to hide some weird complexity, but we never gonna use the function again, it really turns the development hard instead of reduce the complexity, and I'm seeing a lot this behavior being spread around the code, that's why I'm reinforcing here...
thank you for the explanation, but I'm afraid I'm quite aware about scopes, exports, modules... and again I do really disagree with this pattern

[renatobecker]

Suggested change

	let user = Meteor.user();
	const user = getUser(Meteor.userId(), agentId, { fields: { _id: 1, statusLivechat: 1 } });
	if (!user) {
	throw new Meteor.Error('invalid-user', 'Invalid User', { method: 'livechat:changeLivechatStatus' });
	}

[renatobecker]

Please, move this check above this line. Otherwise, agent.statusLivechat may raise an error when the agent is not found.

[eduardocalazansjr]

Apparently this PR seems to break new RC installs on 3.6.0.
Installed 3.5.1 and this doesn't happen.

New db and user w/ admin (created on initial setup) permissions:

Exception while invoking method livechat:changeLivechatStatus Error: Not allowed [error-not-allowed]
--
at MethodInvocation.livechat:changeLivechatStatus (app/livechat/server/methods/changeLivechatStatus.js:23:10)
  | 2020-09-06T19:01:45.524-03:00 | at MethodInvocation.methodsMap.<computed> (app/lib/server/lib/debug.js:67:34)
  | 2020-09-06T19:01:45.524-03:00 | at maybeAuditArgumentChecks (packages/ddp-server/livedata_server.js:1771:12)
  | 2020-09-06T19:01:45.524-03:00 | at packages/ddp-server/livedata_server.js:1689:15
  | 2020-09-06T19:01:45.524-03:00 | at Meteor.EnvironmentVariable.EVp.withValue (packages/meteor.js:1234:12)
  | 2020-09-06T19:01:45.524-03:00 | at packages/ddp-server/livedata_server.js:1687:36
  | 2020-09-06T19:01:45.524-03:00 | at new Promise (<anonymous>)
  | 2020-09-06T19:01:45.524-03:00 | at Server.applyAsync (packages/ddp-server/livedata_server.js:1686:12)
  | 2020-09-06T19:01:45.524-03:00 | at Server.apply (packages/ddp-server/livedata_server.js:1625:26)
  | 2020-09-06T19:01:45.524-03:00 | at Server.call (packages/ddp-server/livedata_server.js:1607:17)
  | 2020-09-06T19:01:45.524-03:00 | at Object.post (app/api/server/v1/misc.js:262:26)
  | 2020-09-06T19:01:45.524-03:00 | at app/api/server/api.js:394:82
  | 2020-09-06T19:01:45.524-03:00 | at Meteor.EnvironmentVariable.EVp.withValue (packages/meteor.js:1234:12)
  | 2020-09-06T19:01:45.524-03:00 | at Object._internalRouteActionHandler [as action] (app/api/server/api.js:394:39)
  | 2020-09-06T19:01:45.524-03:00 | at Route.share.Route.Route._callEndpoint (packages/nimble_restivus/lib/route.coffee:150:32)
  | 2020-09-06T19:01:45.524-03:00 | at packages/nimble_restivus/lib/route.coffee:59:33
  | 2020-09-06T19:01:45.524-03:00 | at packages/simple_json-routes.js:98:9