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.

Sign up for GitHub

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

Jump to bottom

[NEW] Audio Notification updated in sidebar #7817

Merged
merged 8 commits into from
Aug 24, 2017
Merged

[NEW] Audio Notification updated in sidebar #7817

merged 8 commits into from
Aug 24, 2017

Conversation

aditya-kumawat
Copy link
Contributor

@RocketChat/core

Closes #7696

maarten-v and others added 3 commits August 21, 2017 10:04
@maarten-v
Fix Dutch translation
686d953 
The word "account" was translated with "bank account"
@maarten-v
[FIX] Dutch translations
929cc15 
"Port" was translated to "harbour" and other fixes
@aditya-kumawat
Issue resolved
1936a19
@RocketChat RocketChat deleted a comment Aug 21, 2017
@ggazzo ggazzo self-requested a review August 22, 2017 13:52
@ggazzo
Copy link
Member

ggazzo commented Aug 23, 2017

hey @aditya19496 nice code! I made some changes, we had a cyclomatic complexity in this function and I fixed that. and here, I think we are missing this param disableNotifications: 1

return this.find(query, { fields: { 'u._id': 1, audioNotifications: 1, audioNotificationValue: 1, desktopNotificationDuration: 1, desktopNotifications: 1, mobilePushNotifications: 1, disableNotifications: 1 } });

what do you think?

sampaiodiego
sampaiodiego previously approved these changes Aug 23, 2017
View reviewed changes
@sampaiodiego sampaiodiego dismissed their stale review August 23, 2017 19:19

not well viewed by me

rodrigok
rodrigok requested changes Aug 23, 2017
View reviewed changes
packages/rocketchat-lib/server/startup/settings.js Outdated
@@ -433,6 +433,27 @@ RocketChat.settings.addGroup('General', function() {
i18nDescription: 'Desktop_Notification_Durations_Description'
});

this.add('Audio_Notifications_Value', 'chime', {
type: 'int',
Copy link
Member

Choose a reason for hiding this comment

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

Int or String?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

String. My mistake.

packages/rocketchat-lib/server/lib/sendNotificationsOnMessage.js Outdated
}
RocketChat.Notifications.notifyUser(userId, 'audioNotification', {
title,
text: message.msg,
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to send the title and text?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well i don't know about this part. I just changed the notification -> 'audioNotification'

Copy link
Member

Choose a reason for hiding this comment

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

done

client/notifications/notification.js Outdated
const hasFocus = readMessage.isEnable();
const messageIsInOpenedRoom = openedRoomId === notification.payload.rid;

fireGlobalEvent('notification', {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should fire the notification event here again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was already there.

Copy link
Member

Choose a reason for hiding this comment

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

done

client/notifications/notification.js Outdated
let openedRoomId = undefined;
if (['channel', 'group', 'direct'].includes(FlowRouter.getRouteName())) {
openedRoomId = Session.get('openedRoom');
}
Copy link
Member

Choose a reason for hiding this comment

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

we can always get the openedRoomId from session and compare below comparing with != null too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was already there.

Copy link
Member

Choose a reason for hiding this comment

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

done

packages/rocketchat-lib/server/lib/sendNotificationsOnMessage.js Outdated
$ne: user._id
}
}, {
username: { $in: room.usernames }, _id: { $ne: user._id } }, {
Copy link
Member

Choose a reason for hiding this comment

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

Fix identation

Copy link
Member

Choose a reason for hiding this comment

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

ok

packages/rocketchat-lib/server/models/Subscriptions.js
@@ -12,7 +12,7 @@ class ModelSubscriptions extends RocketChat.models._Base {
this.tryEnsureIndex({ 'unread': 1 });
this.tryEnsureIndex({ 'ts': 1 });
this.tryEnsureIndex({ 'ls': 1 });
this.tryEnsureIndex({ 'audioNotification': 1 }, { sparse: 1 });
this.tryEnsureIndex({ 'audioNotifications': 1 }, { sparse: 1 });
Copy link
Member

Choose a reason for hiding this comment

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

Migration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Copy link
Member

Choose a reason for hiding this comment

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

🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed audioNotification to 'audioNotifications' so that all will be of the same form, like 'desktopNotifications', 'mobileNotifications'.

Copy link
Member

@ggazzo ggazzo Aug 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

RocketChat.Migrations.add({
	version: 100,
	up() {
		RocketChat.models.Subscriptions.update({$exists:{audioNotification:1}}, { $rename: { 'audioNotification': 'audioNotifications' } });
	}
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry...i forgot that part i guess.

@aditya-kumawat
Copy link
Contributor Author

@ggazzo yes you are right about the 'disableNotification' thing.

@rodrigok rodrigok merged commit b80f5fc into RocketChat:develop Aug 24, 2017
@rodrigok rodrigok added this to the 0.59.0 milestone Aug 24, 2017
@aditya-kumawat aditya-kumawat deleted the audio-notification-update branch August 24, 2017 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rodrigok rodrigok rodrigok approved these changes

@sampaiodiego sampaiodiego sampaiodiego left review comments

@ggazzo ggazzo Awaiting requested review from ggazzo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.59.0
Development

Successfully merging this pull request may close these issues.
6 participants
@aditya-kumawat @ggazzo @rodrigok @sampaiodiego @maarten-v @engelgabriel