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

Regression: New Livechat methods and processes #15242

Merged
merged 23 commits into from
Sep 9, 2019

Conversation

renatobecker-zz
Copy link

@renatobecker-zz renatobecker-zz commented Aug 23, 2019

After reviewed the codebase where there are a lot of new implementations related to Livechat pull requests, I noticed that some methods needed improvements and fixes to work as well as expected.

Important change:
We're renaming the Livechat Routing methods, as described below:

  • From Guest Pool to Manual Selection;
  • From Least Amount to Auto Selection;

In addition: The Room model(client side) has been replaced with LivechatRoom model in some Livechat templates, such as visitorInfo and visitorEdit. This change was necessary because the reactivity was not working for Livechat Managers because the models based on CachedCollection only emit changes to the users who have the related subscription and the Livechat Managers have permission to access those templates even they don't have that subscription.

This is a PR that needs to be merged before the next release -> 2.0.0.

@ggazzo ggazzo changed the title [FIX] New Livechat methods and processes Regression: New Livechat methods and processes Aug 26, 2019
app/livechat/server/methods/transfer.js Outdated Show resolved Hide resolved
@@ -9,7 +9,7 @@ Meteor.publish('livechat:rooms', function(filter = {}, offset = 0, limit = 20) {
return this.error(new Meteor.Error('error-not-authorized', 'Not authorized', { publish: 'livechat:rooms' }));
}

if (!hasPermission(this.userId, 'view-livechat-rooms')) {
if (!hasPermission(this.userId, 'view-livechat-rooms') && !(filter._id && hasPermission(this.userId, 'view-l-room'))) {
Copy link
Member

Choose a reason for hiding this comment

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

I think it is always harder to read conditions like this, where you test the truth value but negates afterwards.

is this the same?

Suggested change
if (!hasPermission(this.userId, 'view-livechat-rooms') && !(filter._id && hasPermission(this.userId, 'view-l-room'))) {
if (!hasPermission(this.userId, 'view-livechat-rooms') && !(filter._id && hasPermission(this.userId, 'view-l-room'))) {
if (!hasPermission(this.userId, 'view-livechat-rooms') || (filter._id && !hasPermission(this.userId, 'view-l-room'))) {

Copy link
Member

Choose a reason for hiding this comment

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

also is this the case where an agent is trying to view a room's data? in this case should we only allow it to view the data if he is the servedBy agent?

Copy link
Author

@renatobecker-zz renatobecker-zz Sep 6, 2019

Choose a reason for hiding this comment

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

Yeah, this is exactly that case.
I improved the code, so now it will check the right validation/permission just like your last question.
Thanks.

app/livechat/client/views/app/tabbar/visitorEdit.js Outdated Show resolved Hide resolved
app/livechat/client/views/app/tabbar/visitorInfo.js Outdated Show resolved Hide resolved
app/livechat/server/lib/Helper.js Show resolved Hide resolved
app/livechat/server/lib/Helper.js Show resolved Hide resolved
@renatobecker-zz
Copy link
Author

Hi @sampaiodiego!
I just pushed some improvements according to your suggestions.
I have also replied to one of your comments, please take a look at that explanation.

Thanks.

@renatobecker-zz renatobecker-zz changed the title Regression: New Livechat methods and processes [WIP] Regression: New Livechat methods and processes Sep 6, 2019
@renatobecker-zz renatobecker-zz changed the title [WIP] Regression: New Livechat methods and processes Regression: New Livechat methods and processes Sep 6, 2019
@renatobecker-zz
Copy link
Author

renatobecker-zz commented Sep 9, 2019

@sampaiodiego
Just one more thing pushed in this PR. We're renaming the Livechat routing methods, as described below:

  • From Guest Pool to Manual Selection;
  • From Least Amount to Auto Selection;

The documentation will be updated later.
Thanks.

@sampaiodiego sampaiodiego merged commit 7c798f5 into develop Sep 9, 2019
@sampaiodiego sampaiodiego deleted the fix-livechat-new-methods branch September 9, 2019 23:23
@sampaiodiego sampaiodiego mentioned this pull request Sep 12, 2019
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