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

[FIX] Read Receipt for Livechat Messages fixed #13832

Merged
merged 9 commits into from
Mar 28, 2019

Conversation

knrt10
Copy link
Contributor

@knrt10 knrt10 commented Mar 21, 2019

Closes #13302

Screenshot 2019-03-21 at 10 59 29 PM

cc @renatobecker please review.

@engelgabriel engelgabriel added this to the 1.0.0 milestone Mar 21, 2019
@renatobecker-zz renatobecker-zz self-requested a review March 22, 2019 16:19
if (this.user) {
return (settings.get('UI_Use_Real_Name') && this.user.name) || this.user.username;
}
return this.guest.name;

Choose a reason for hiding this comment

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

If the Livechat registration form is not enabled, then the name of the guest will not be defined.
Anyway, we don't need to create another property to display the user data, let's use the user property for both regular users and Livechat users.

@@ -6,7 +6,11 @@
<ul class="read-receipts">
{{#each receipts}}
<li class="read-receipts__user background-transparent-dark-hover">
{{> avatar username=user.username}}
{{#if user}}

Choose a reason for hiding this comment

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

Please, undo this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@renatobecker Do you want an avatar with guest name or username, cause if it is the guest name, it will change accordingly, otherwise it will always be G

Choose a reason for hiding this comment

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

We don't have Avatars for Livechat users.
It's ok showing the default avatar as you just described.

@@ -81,6 +81,7 @@ export const ReadReceipt = {
return ReadReceipts.findByMessageId(message._id).map((receipt) => ({
...receipt,
user: Users.findOneById(receipt.userId, { fields: { username: 1, name: 1 } }),
Copy link

@renatobecker-zz renatobecker-zz Mar 22, 2019

Choose a reason for hiding this comment

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

We can check the receipt object to ensure that it came from a Livechat user or not. If the receipt.token exists, then you need to get the guest by finding it with the LivechatVisitors model, as you can see below:

Suggested change
user: Users.findOneById(receipt.userId, { fields: { username: 1, name: 1 } }),
user: message.token? LivechatVisitors.findById(receipt.userId, { fields: { username: 1, name: 1 } }) : Users.findOneById(receipt.userId, { fields: { username: 1, name: 1 } }) ,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, let me update that.

@knrt10 knrt10 force-pushed the issue13302 branch 3 times, most recently from fc9963d to 607929f Compare March 22, 2019 20:16
@@ -13,7 +13,7 @@ callbacks.add('afterSaveMessage', (message, room) => {
Subscriptions.setAsReadByRoomIdAndUserId(room._id, message.u._id);

// mark message as read as well
ReadReceipt.markMessageAsReadBySender(message, room._id, message.u._id);
ReadReceipt.markMessageAsReadBySender(message, room._id, message.u._id, message.token);
Copy link

@renatobecker-zz renatobecker-zz Mar 22, 2019

Choose a reason for hiding this comment

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

This is not a good approach.
The token needs to come from a specific Livechat Room Type method since this package is not the Livechat package.
In addition, the markMessageAsReadBySender needs to expect an object, not a regular variable.

@@ -40,7 +40,7 @@ export const ReadReceipt = {
updateMessages(roomId);
},

markMessageAsReadBySender(message, roomId, userId) {
markMessageAsReadBySender(message, roomId, userId, extraData) {

Choose a reason for hiding this comment

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

Suggested change
markMessageAsReadBySender(message, roomId, userId, extraData) {
markMessageAsReadBySender(message, roomId, userId, extraData = {}) {

@@ -63,6 +63,7 @@ export const ReadReceipt = {
userId,
messageId: message._id,
ts,
token: extraData,

Choose a reason for hiding this comment

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

Suggested change
token: extraData,
...extraData,

@@ -80,7 +81,7 @@ export const ReadReceipt = {
getReceipts(message) {
return ReadReceipts.findByMessageId(message._id).map((receipt) => ({
...receipt,
user: Users.findOneById(receipt.userId, { fields: { username: 1, name: 1 } }),
user: (receipt.token && !(Object.keys(receipt.token).length === 0 && receipt.token.constructor === Object)) ? LivechatVisitors.getVisitorByToken(message.token, { fields: { username: 1, name: 1 } }) : Users.findOneById(receipt.userId, { fields: { username: 1, name: 1 } }),

Choose a reason for hiding this comment

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

Suggested change
user: (receipt.token && !(Object.keys(receipt.token).length === 0 && receipt.token.constructor === Object)) ? LivechatVisitors.getVisitorByToken(message.token, { fields: { username: 1, name: 1 } }) : Users.findOneById(receipt.userId, { fields: { username: 1, name: 1 } }),
user: receipt.token ? LivechatVisitors.getVisitorByToken(receipt.token, { fields: { username: 1, name: 1 } }) : Users.findOneById(receipt.userId, { fields: { username: 1, name: 1 } }),

@knrt10 knrt10 force-pushed the issue13302 branch 2 times, most recently from 54ee6eb to 91e3ed8 Compare March 22, 2019 22:47
@@ -264,4 +264,17 @@ export class RoomTypeConfig {
return false;
}

/**
* Get receipt's extra token for livechat users

Choose a reason for hiding this comment

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

These comments are related to Livechat stuff, not the default Room Type Config

* @return {object} Token for user
*/
getReadReceiptsExtraData(message) {
if (!Meteor.isServer) {

Choose a reason for hiding this comment

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

Here the return will be always return {};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Copy link

@renatobecker-zz renatobecker-zz Mar 25, 2019

Choose a reason for hiding this comment

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

@knrt10, are you sure? The code has not changed to me.

@knrt10 knrt10 force-pushed the issue13302 branch 2 times, most recently from 3412bc5 to c1c61f3 Compare March 25, 2019 14:08
@@ -50,11 +51,12 @@ export const ReadReceipt = {
if (message.unread && message.ts < firstSubscription.ls) {
Messages.setAsReadById(message._id, firstSubscription.ls);
}
const extraData = roomTypes.getConfig('l').getReadReceiptsExtraData(message);
Copy link

@renatobecker-zz renatobecker-zz Mar 25, 2019

Choose a reason for hiding this comment

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

He discussed about it last week:

Suggested change
const extraData = roomTypes.getConfig('l').getReadReceiptsExtraData(message);
const extraData = roomTypes.getConfig(room.t).getReadReceiptsExtraData(message);

You need to find the room and get the room type field:
const room = Rooms.findOneById(roomId, { fields: { t: 1 } });

@knrt10
Copy link
Contributor Author

knrt10 commented Mar 25, 2019

Rebased and updated, latest changes

@renatobecker-zz renatobecker-zz added this to In progress in Omnichannel Roadmap via automation Mar 25, 2019
Omnichannel Roadmap automation moved this from In progress to Reviewer approved Mar 28, 2019
@renatobecker-zz renatobecker-zz merged commit c13708a into RocketChat:develop Mar 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Read Receipt for Livechat Messages is not showing information of livechat-guest
5 participants