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

Do not retry and log warning when push notification was not authorised #18562

Merged
merged 1 commit into from
Aug 22, 2020
Merged

Do not retry and log warning when push notification was not authorised #18562

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion app/push/server/push.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ export class PushClass {
}

return HTTP.post(`${ gateway }/push/${ service }/send`, data, (error, response) => {
if (response && response.statusCode === 406) {
if (response?.statusCode === 406) {
logger.info('removing push token', token);
appTokensCollection.remove({
$or: [{
Expand All @@ -128,6 +128,11 @@ export class PushClass {
return;
}

if (response?.statusCode === 401) {
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is one of the bad habits that optional chaining introduces, with this change the code is now checking twice if response is defined.. IMO we should avoid this and in this case we could use a single if to check if response.statusCode exists:

			if (response?.statusCode) {
				if (response.statusCode === 406) {
					logger.info('removing push token', token);
					appTokensCollection.remove({
						$or: [{
							'token.apn': token,
						}, {
							'token.gcm': token,
						}],
					});
					return;
				}

				if (response.statusCode === 401) {
					logger.warn('Error sending push to gateway (not authorized)', response);
					return;
				}
			}
...

Copy link
Member

Choose a reason for hiding this comment

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

I agree, sometimes this feature leads us not to question if some structures are correct, for example here, why would the respose be empty ?, maybe the error of this being tested first ... I saw it a lot on the coffescript code and I dont like it... try to write using ifs or ternaries before, and maybe these questions would appear 🤔

logger.warn('Error sending push to gateway (not authorized)', response);
return;
}

if (!error) {
return;
}
Expand Down