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 not sending desktop notifications #11266

Merged
merged 3 commits into from Jul 6, 2018

Conversation

Projects
4 participants
@renatobecker
Copy link
Contributor

commented Jun 26, 2018

Closes #11031.

This PR fixes Livechat desktop notifications that weren't showing up on new messages.

@renatobecker renatobecker added this to the 0.66.0 milestone Jun 26, 2018

@renatobecker renatobecker added this to Desireable in Livechat via automation Jun 26, 2018

@renatobecker renatobecker requested review from ggazzo and sampaiodiego Jun 26, 2018

@renatobecker renatobecker added this to Review/QA in June/2018 via automation Jun 26, 2018

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-11266 Jun 26, 2018 Inactive

@renatobecker renatobecker moved this from Desireable to Review/QA in Livechat Jun 26, 2018

@sampaiodiego
Copy link
Member

left a comment

When enabling Use Real Name I'm getting this notifications:
image

disclaimer: ao is the livechat visitor's name (the sharp added to it is also weird).. and aue is the message the visitor sent

@sampaiodiego sampaiodiego temporarily deployed to rocket-chat-pr-11266 Jul 5, 2018 Inactive

@sampaiodiego
Copy link
Member

left a comment

I've changed to show a [livechat] in front of the notification title (since we don't a single character to show there - like @ or #):

image

if visitor has no name, it shows his guest counter:
image

@rodrigok rodrigok modified the milestones: 0.66.0, 0.66.2 Jul 5, 2018

@rodrigok
Copy link
Member

left a comment

Code approved

@sampaiodiego sampaiodiego changed the title [FIX] Livechat desktop notifications [FIX] Livechat not sending desktop notifications Jul 6, 2018

@sampaiodiego sampaiodiego dismissed stale reviews from rodrigok and themself via aec6d40 Jul 6, 2018

@sampaiodiego sampaiodiego force-pushed the livechat-desktop-notifications branch from 5a691ca to aec6d40 Jul 6, 2018

@sampaiodiego sampaiodiego temporarily deployed to rocket-chat-pr-11266 Jul 6, 2018 Inactive

@sampaiodiego sampaiodiego merged commit 22d1cae into develop Jul 6, 2018

4 of 5 checks passed

ci/circleci: build-and-test/hold Your job is on hold on CircleCI!
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test-with-oplog Your tests passed on CircleCI!
Details
ci/circleci: test-without-oplog Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details

June/2018 automation moved this from Review/QA to Closed Jul 6, 2018

@sampaiodiego sampaiodiego deleted the livechat-desktop-notifications branch Jul 6, 2018

sampaiodiego added a commit that referenced this pull request Jul 6, 2018

@sampaiodiego sampaiodiego referenced this pull request Jul 6, 2018

Merged

Release 0.66.2 #11366

@renatobecker

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2018

@sampaiodiego, just give you a feedback on the changes made in this fix..
I really like the approach using roomTypes in cases like this because through this pattern we can keep the code in the correct place, avoiding injecting specific subsystem code into the core notifications system, for example.
So, if we can keep this pattern in the next implementations that are coming, this can make implementation more consistent.

Thanks.

@renatobecker renatobecker moved this from Review/QA to Closed in Livechat Aug 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.