-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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] Send category and title fields to iOS push notification #8905
Conversation
}); | ||
return message; | ||
if (canBeNotified(userOfMentionId, 'mobile')) { | ||
if (Push.enabled === true && userOfMention.statusConnection !== 'online') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
busy
also receives notification?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rafaelks the behavior was not changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that's a bug, when User is away it should receive notification on iOS as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its kind of confusing but there are two different status's.
Users Presence status (what they want to be perceived as) and their connection status.
Their connection status only goes between basically 2 states. online and away. Offline would be that connection would not exist.
So if all connection status's are away the users status is automatically set to away which would allow them to receive push notifications to the phone.
Same with if all connections are offline (aka there aren't any connection) the users status should be offline and they will again receive notifications.
usersTo: { | ||
userId: userOfMention._id | ||
}, | ||
category: canSendMessageToRoom(room, userOfMention.username) ? 'MESSAGE' : 'MESSAGE_NOREPLY' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that you're duplicating the strings... maybe use constants?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If everything else is working as planned, I would only change this strings to constants before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed. I thought doing this, but since we don't have project wide constants (which would make sense for this), I decided using raw strings..
I'll change for in-file constants though. 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to constants.. can you review again pls?
@@ -24,6 +24,10 @@ function replaceMentionedUsernamesWithFullNames(message, mentions) { | |||
return message; | |||
} | |||
|
|||
function canSendMessageToRoom(room, username) { | |||
return !((room.muted || []).includes(username)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I got the logic here... the username needs to be in the mentions
to be able to reply? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did you see mentions
@rafaelks ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rafaelks The code is a little bit confusing, but it is the array of users that will receive the push, not the mentioned ones only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK... and what if the room is read only? This code isn't checking that AFAIK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually this is the code that checks it.. readonly rooms have a list of muted
users..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sampaiodiego So a readonly
room will have all usernames in muted
array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep.. not a good implementation, I know, but this is how it works now 🤷♂️
@@ -24,6 +24,10 @@ function replaceMentionedUsernamesWithFullNames(message, mentions) { | |||
return message; | |||
} | |||
|
|||
function canSendMessageToRoom(room, username) { | |||
return !((room.muted || []).includes(username)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RocketChat/core
Closes #8863
This is blocked by Meteor-Community-Packages/raix-push#322 and Meteor-Community-Packages/raix-push#321 though.New
raix:push
package version published so we are good to merge this as well 🎉