Conversation
test/integration/messaging.spec.ts
Outdated
| body: 'Message body', | ||
| }, | ||
| android: { | ||
| restrictedPackageName: 'com.hkj.testing' |
There was a problem hiding this comment.
hkj is not in the codebase yet, maybe put something generic here.
bojeil-google
left a comment
bojeil-google
left a comment
There was a problem hiding this comment.
Mostly nits. Would recommend someone on FCM team to take another look.
| return null; | ||
| } | ||
|
|
||
| private static getErrorMessage(response: any): string | null { |
There was a problem hiding this comment.
Define javadoc for this.
|
|
||
| private static getErrorMessage(response: any): string | null { | ||
| if (validator.isNonNullObject(response) && 'error' in response) { | ||
| if (validator.isNonEmptyString(response.error.message)) { |
There was a problem hiding this comment.
You can group the 2 nested if statements in one.
if (validator.isNonNullObject(response) &&
'error' in response &&
validator.isNonEmptyString(response.error.message) {
| titleLocArgs?: string[]; | ||
| } | ||
|
|
||
| function validateStringMap(map: object, label: string) { |
There was a problem hiding this comment.
Consider adding javadoc descriptions for all these functions in this file to be consistent with existing convention.
src/messaging/messaging.ts
Outdated
|
|
||
| const apsAlert: ApsAlert = alert as ApsAlert; | ||
| if (validator.isNonEmptyArray(apsAlert.locArgs)) { | ||
| if (!validator.isNonEmptyString(apsAlert.locKey)) { |
There was a problem hiding this comment.
Same here, the 2 nested if statements can be grouped into 1.
src/messaging/messaging.ts
Outdated
| } | ||
| } | ||
| if (validator.isNonEmptyArray(apsAlert.titleLocArgs)) { | ||
| if (!validator.isNonEmptyString(apsAlert.titleLocKey)) { |
There was a problem hiding this comment.
Same here, the 2 nested if statements can be grouped into 1.
| MessagingClientErrorCode.INVALID_PAYLOAD, 'android.notification must be a non-null object'); | ||
| } | ||
|
|
||
| if (typeof notification.color !== 'undefined' && !/^#[0-9a-fA-F]{6}$/.test(notification.color)) { |
There was a problem hiding this comment.
seems like a color validator is useful to add to our validator class.
There was a problem hiding this comment.
Right now this is the only place where we do this specific validation. So I'll keep it as is for now.
src/messaging/messaging.ts
Outdated
| MessagingClientErrorCode.INVALID_PAYLOAD, 'android.notification.color must be in the form #RRGGBB'); | ||
| } | ||
| if (validator.isNonEmptyArray(notification.bodyLocArgs)) { | ||
| if (!validator.isNonEmptyString(notification.bodyLocKey)) { |
There was a problem hiding this comment.
nested if statements can be grouped to 1.
src/messaging/messaging.ts
Outdated
| } | ||
| } | ||
| if (validator.isNonEmptyArray(notification.titleLocArgs)) { | ||
| if (!validator.isNonEmptyString(notification.titleLocKey)) { |
There was a problem hiding this comment.
nested if statements can be grouped to 1.
| .then(() => { | ||
| expect(requestWriteSpy).to.have.been.calledOnce; | ||
| const requestData = JSON.parse(requestWriteSpy.args[0][0]); | ||
| const want = config.want || config.req; |
There was a problem hiding this comment.
Suggestion: maybe call it expectedServerReq or expectedReq or something of the like, instead of want ?
* Accept prefixed topic names * Further validating topic names
* Using new FCM error codes * Adding new SDK error code message-rate-exceeded
3 participants
Implements the
admin.messaging.send()operation based on the new FCM server API.go/firebase-admin-fcm-api