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.

Sign up for GitHub

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

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

Merged
merged 8 commits into from
Aug 19, 2020
Merged

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

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9b46e5e
Add agentId parameter to changeLivechatStatus method
MartinSchoeler Aug 14, 2020
4042004
Fix reviews
MartinSchoeler Aug 17, 2020
277a22e
fix problems
MartinSchoeler Aug 17, 2020
a3e0ddd
return if the same as before
MartinSchoeler Aug 17, 2020
ae7a8a8
Update app/livechat/server/methods/changeLivechatStatus.js
ggazzo Aug 19, 2020
d57aaae
Fix review
ggazzo Aug 19, 2020
1002ab1
Merge branch 'add-parameter-livechatStatus-Method' of github.com:Rock…
ggazzo Aug 19, 2020
91cbea1
Merge branch 'develop' into add-parameter-livechatStatus-Method
renatobecker Aug 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions app/livechat/server/methods/changeLivechatStatus.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,23 @@
import { Meteor } from 'meteor/meteor';

import { Livechat } from '../lib/Livechat';
import { hasPermission, hasRole } from '../../../authorization';
Copy link
Contributor

Choose a reason for hiding this comment

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

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

import { Users } from '../../../models';
Copy link
Contributor

Choose a reason for hiding this comment

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

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);
};

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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


Meteor.methods({
'livechat:changeLivechatStatus'() {
'livechat:changeLivechatStatus'(agentId) {
if (!Meteor.userId()) {
throw new Meteor.Error('error-not-allowed', 'Not allowed', { method: 'livechat:changeLivechatStatus' });
}

const user = Meteor.user();
let user = Meteor.user();
Copy link
Contributor

Choose a reason for hiding this comment

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

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' });
}


if (agentId && hasPermission(Meteor.userId(), 'manage-livechat-agents')) {
user = Users.findOneById(agentId);
if (!user || !hasRole(agentId, 'livechat-agent')) {
throw new Meteor.Error('error-user-is-not-agent', 'User is not a livechat agent', { method: 'livechat:saveAgentInfo' });
}
}

const newStatus = user.statusLivechat === 'available' ? 'not-available' : 'available';
if (!Livechat.allowAgentChangeServiceStatus(newStatus, user._id)) {
Expand Down