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

[NEW] Livechat notifications on new incoming inquiries for guest-pool #10588

Conversation

mrsimpson
Copy link
Collaborator

@mrsimpson mrsimpson commented Apr 25, 2018

Closes #7292

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

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.

@renatobecker-zz
Copy link

@mrsimpson, can you fix the conflicts in this PR, please?
Thx.

@mrsimpson
Copy link
Collaborator Author

@renatobecker of course. Before I do, please let me know if it’s ok to have livechat specific code in these files

@renatobecker-zz
Copy link

@mrsimpson, You will see that there have been many changes in the packages/rocketchat-lib/server/lib/sendNotificationsOnMessage.js file since your implementation.
I suggest you to move your code to rocketchat-livechat package because it's a specific approach to that feature.

Thanks.

@mrsimpson
Copy link
Collaborator Author

@renatobecker

You will see that there have been many changes in the packages/rocketchat-lib/server/lib/sendNotificationsOnMessage.js file since your implementation.

I had already fixed this, however I'm not too happy with the implementation (as said four days ago)

Still, I just updated this PR now to match the current development state. Because of the way notifications in RC are implemented, it's quite tricky to move the code to the livechat-package without duplicating a lot of code.

Please have a look at it whether it matches your architectural needs or if it really has to move. In the later case, I'd appreciate if someone from the core-team did the refactoring of the notifications-stuff into a library).

@renatobecker-zz
Copy link

renatobecker-zz commented Jun 29, 2018

@mrsimpson, looking deeply your implementation, I can see that you are using an approach where the notifications will be triggered for every new messages that the guest/visitor sends, not just the first message when the inquiry is created, am I right?
If I'm not wrong, this behaviour is not the same as requested on the issue #11137, because there the user is asking for notifications just when the inquiry is created what, IMO, would be best for a standard implementation.

If I'm wrong about your use case, please let me know.

In addition: The migration of the code to rockechat-livechat package could be easy if the purpose of this approach is to notify agents for new incoming Livechats.

I'll be waiting for your feedback.
Thanks.

@mrsimpson mrsimpson force-pushed the port/#7292-livechat-guestpool-notifications branch from 2f1466a to 493dde3 Compare September 12, 2018 15:36
@CLAassistant
Copy link

CLAassistant commented Sep 12, 2018

CLA assistant check
All committers have signed the CLA.

@mrsimpson mrsimpson force-pushed the port/#7292-livechat-guestpool-notifications branch 2 times, most recently from 2f1466a to d3e9d60 Compare September 12, 2018 15:50
@mrsimpson
Copy link
Collaborator Author

mrsimpson commented Sep 12, 2018

@renatobecker like this? Now, a new notification is being create only for the first message which creates the queue entry - fixes #11137

@mrsimpson mrsimpson self-assigned this Sep 17, 2018
@sampaiodiego sampaiodiego changed the title [FIX] livechat guest-pool notifications [NEW] Livechat notifications for new incoming inquiries for guest-pool Sep 20, 2018
@sampaiodiego sampaiodiego changed the title [NEW] Livechat notifications for new incoming inquiries for guest-pool [NEW] Livechat notifications on new incoming inquiries for guest-pool Sep 20, 2018
@sampaiodiego sampaiodiego merged commit b5e3802 into RocketChat:develop Sep 20, 2018
@mrsimpson mrsimpson deleted the port/#7292-livechat-guestpool-notifications branch September 20, 2018 22:09
This was referenced Sep 28, 2018
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.

Live Chat: No desktop notification when someone joins live chat queue
4 participants