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 livechat notifications for guest pool based routing #327

Merged
merged 2 commits into from
Jun 19, 2018

Conversation

mrsimpson
Copy link
Member

@mrsimpson mrsimpson commented Apr 25, 2018

This fixes #271 desktop notifications not being created if a live chat visitor created a room by sending the first message.

How to test

  • Configure a livechat with guest pool, add an agent.
  • Create a new live chat and verify the desktop notification appears
  • add two departments and configure an agent
  • Create a new live chat selecting a department
  • Send a new message and check the notification appears

Remarks

  • Notifications other than livechat and guest pool are based upon the user's subscriptions. Only for the live chat inquiries (the "requests for an agent to join"), there's a need for an alternative mechanism.
  • Once an agent is joined, he's the only one to notify which matches the default: He'll have a valid subscription once joined.
  • All code added is non-destructive (new functions, arrays concatenated, ...) - it will most likely not make things worse, even if it doesn't meet all the aspects
  • From an architectural perspective, it would be better to have the code for creating the livechat-notifications in a separate hook defined in the livechat-package. The way it's been implemented creates a dependency from the notifications-api to the livechat-models - which is not good. On the other hand, the dependency is de-facto already there (room.t !== 'l') and implementing it there has a high re-use.

@ghost ghost assigned mrsimpson Apr 25, 2018
@ghost ghost added the progress:working label Apr 25, 2018
@mrsimpson mrsimpson requested a review from a team April 25, 2018 20:26
@mrsimpson
Copy link
Member Author

mrsimpson commented Apr 25, 2018

PR to RC core addressing RocketChat#7796: RocketChat#10588

@mrsimpson
Copy link
Member Author

Observe RocketChat#10318 which adds even better notification title ;)

@mrsimpson
Copy link
Member Author

@janrudolph can you actually test this?

@janrudolph
Copy link

janrudolph commented May 18, 2018

Verified on Mac OS with Chrome (to be repeated on Windows):

  • Configure a livechat with guest pool, add an agent.
  • Create a new live chat and verify the desktop notification appears

Not tested yet:

  • add two departments and configure an agent
  • Create a new live chat selecting a department
  • Send a new message and check the notification appears

Additionally, I tested the following case:

  • Configure Livechat_Routing_Method to Least Amount. Check if notification appears when a new customer writes its first message

Copy link

@vickyokrm vickyokrm left a comment

Choose a reason for hiding this comment

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

Testing comments:

  1. Created a guest pool live chat with multiple agents. As per the test steps indicated, there was no notification appeared when the live chat was created. But the notification shows up, when the user write a message.

@vickyokrm
Copy link

vickyokrm commented Jun 12, 2018

Scenarios Tested successfully on Mac:

  • Configure a livechat with guest pool, add an agent.
  • Create a new live chat and verify the desktop notification appears
  • add two departments and configure an agent
  • Create a new live chat selecting a department
  • Send a new message and check the notification appears

@janrudolph
Copy link

lgtm
However, there is another issue: #364

Copy link

@janrudolph janrudolph left a comment

Choose a reason for hiding this comment

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

lgtm

@janrudolph janrudolph merged commit 9274752 into develop Jun 19, 2018
@ghost ghost removed the progress:working label Jun 19, 2018
@janrudolph janrudolph deleted the fix/#271-notifications-livechat branch June 19, 2018 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Proxy] [Bug] No desktop notifications in livechat #10074
3 participants