-
Notifications
You must be signed in to change notification settings - Fork 13k
fix: push notifications not delivered when message length is too large #37852
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
Merged
Merged
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
1a7bc7e
chore: adds new validations to push notifications
lucas-a-pelegrino 07e8540
chore: removes unnecessary comments
lucas-a-pelegrino dfc26c4
docs: adds .changeset
lucas-a-pelegrino c27908f
tests: adds unit testing for PushClass.send function
lucas-a-pelegrino 0b3b24d
fix: proxyquire import path
lucas-a-pelegrino f525c11
chore: adds minor improvement to push text size limit
lucas-a-pelegrino f82b3bf
fix: PushClass.send failing tests
lucas-a-pelegrino 412a75f
tests: adds improvements to error assertion
lucas-a-pelegrino 5a206b8
chore: removes .only
lucas-a-pelegrino 857629d
chore: adds minor improvement from code review
lucas-a-pelegrino 999ae4e
chore: removes unused sinon.useFakeTimers
lucas-a-pelegrino d1ece1e
chores: adds some more improvements to push logic and new helper func…
lucas-a-pelegrino 776e2f4
chore: adds improvements to truncateString
lucas-a-pelegrino f0aba03
chore: adds improvements from code review
lucas-a-pelegrino fe1f84f
docs: updates changeset
lucas-a-pelegrino 947a0dd
chore: increases body message and title limits
lucas-a-pelegrino d1b3abd
Merge branch 'develop' into bugfix/SUP-951
lucas-a-pelegrino f1d05c7
Merge branch 'develop' into bugfix/SUP-951
kodiakhq[bot] b2f2d0e
Merge branch 'develop' into bugfix/SUP-951
kodiakhq[bot] 371cc37
Merge branch 'develop' into bugfix/SUP-951
kodiakhq[bot] File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| "@rocket.chat/meteor": patch | ||
| "@rocket.chat/tools": patch | ||
| --- | ||
|
|
||
| Adds improvements to the push notifications logic; the logic now truncates messages and titles larger than 240, and 65 characters respectively. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| import type { IPushNotificationConfig } from '@rocket.chat/core-typings/src/IPushNotificationConfig'; | ||
| import { pick, truncateString } from '@rocket.chat/tools'; | ||
| import { expect } from 'chai'; | ||
| import proxyquire from 'proxyquire'; | ||
| import sinon from 'sinon'; | ||
|
|
||
| const loggerStub = { debug: sinon.stub(), warn: sinon.stub(), error: sinon.stub(), info: sinon.stub(), log: sinon.stub() }; | ||
| const settingsStub = { get: sinon.stub().returns('') }; | ||
|
|
||
| const { Push } = proxyquire.noCallThru().load('../../../../app/push/server/push', { | ||
| './logger': { logger: loggerStub }, | ||
| '../../settings/server': { settings: settingsStub }, | ||
| '@rocket.chat/tools': { pick, truncateString }, | ||
| 'meteor/check': { | ||
| check: sinon.stub(), | ||
| Match: { | ||
| Optional: () => sinon.stub(), | ||
| Integer: Number, | ||
| OneOf: () => sinon.stub(), | ||
| test: sinon.stub().returns(true), | ||
| }, | ||
| }, | ||
| 'meteor/meteor': { | ||
| Meteor: { | ||
| absoluteUrl: sinon.stub().returns('http://localhost'), | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| describe('Push Notifications [PushClass]', () => { | ||
| afterEach(() => { | ||
| sinon.restore(); | ||
| }); | ||
|
|
||
| describe('send()', () => { | ||
| let sendNotificationStub: sinon.SinonStub; | ||
| beforeEach(() => { | ||
| sendNotificationStub = sinon.stub(Push, 'sendNotification').resolves({ apn: [], gcm: [] }); | ||
| }); | ||
|
|
||
| it('should call sendNotification with required fields', async () => { | ||
| const options: IPushNotificationConfig = { | ||
| from: 'test', | ||
| title: 'title', | ||
| text: 'body', | ||
| userId: 'user1', | ||
| apn: { category: 'MESSAGE' }, | ||
| gcm: { style: 'inbox', image: 'url' }, | ||
| }; | ||
|
|
||
| await Push.send(options); | ||
|
|
||
| expect(sendNotificationStub.calledOnce).to.be.true; | ||
|
|
||
| const notification = sendNotificationStub.firstCall.args[0]; | ||
| expect(notification.from).to.equal('test'); | ||
| expect(notification.title).to.equal('title'); | ||
| expect(notification.text).to.equal('body'); | ||
| expect(notification.userId).to.equal('user1'); | ||
| }); | ||
|
|
||
| it('should truncate text if longer than 240 chars', async () => { | ||
| const longText = 'a'.repeat(300); | ||
| const options: IPushNotificationConfig = { | ||
| from: 'test', | ||
| title: 'title', | ||
| text: longText, | ||
| userId: 'user1', | ||
| apn: { category: 'MESSAGE' }, | ||
| gcm: { style: 'inbox', image: 'url' }, | ||
| }; | ||
|
|
||
| await Push.send(options); | ||
|
|
||
| const notification = sendNotificationStub.firstCall.args[0]; | ||
|
|
||
| expect(notification.text.length).to.equal(240); | ||
| }); | ||
|
|
||
| it('should truncate title if longer than 65 chars', async () => { | ||
| const longTitle = 'a'.repeat(100); | ||
| const options: IPushNotificationConfig = { | ||
| from: 'test', | ||
| title: longTitle, | ||
| text: 'bpdu', | ||
| userId: 'user1', | ||
| apn: { category: 'MESSAGE' }, | ||
| gcm: { style: 'inbox', image: 'url' }, | ||
| }; | ||
|
|
||
| await Push.send(options); | ||
|
|
||
| const notification = sendNotificationStub.firstCall.args[0]; | ||
|
|
||
| expect(notification.title.length).to.equal(65); | ||
| }); | ||
|
|
||
| it('should throw if userId is missing', async () => { | ||
| const options = { | ||
| from: 'test', | ||
| title: 'title', | ||
| text: 'body', | ||
| apn: { category: 'MESSAGE' }, | ||
| gcm: { style: 'inbox', image: 'url' }, | ||
| }; | ||
|
|
||
| await expect(Push.send(options)).to.be.rejectedWith('No userId found'); | ||
|
|
||
| expect(sendNotificationStub.called).to.be.false; | ||
| }); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| /** | ||
| * Truncates a string to a specified maximum length, optionally adding ellipses. | ||
| * @param str | ||
| * @param maxLength | ||
| * @param shouldAddEllipses | ||
| * @return {string} | ||
| */ | ||
| export function truncateString(str: string, maxLength: number, shouldAddEllipses = true): string { | ||
| const ellipsis = '...'; | ||
| if (str.length <= maxLength) { | ||
| return str; | ||
| } | ||
|
|
||
| if (shouldAddEllipses && str.length > maxLength) { | ||
| if (maxLength <= ellipsis.length) { | ||
| return str.slice(0, maxLength); | ||
| } | ||
|
|
||
| return `${str.slice(0, maxLength - ellipsis.length)}${ellipsis}`; | ||
| } | ||
|
|
||
| return str.slice(0, maxLength); | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.