Do not retry and log warning when push notification was not authorised#18562
Merged
sampaiodiego merged 1 commit intodevelopfrom Aug 22, 2020
Merged
Do not retry and log warning when push notification was not authorised#18562sampaiodiego merged 1 commit intodevelopfrom
sampaiodiego merged 1 commit intodevelopfrom
Conversation
graywolf336
approved these changes
Aug 14, 2020
| return; | ||
| } | ||
|
|
||
| if (response?.statusCode === 401) { |
Member
There was a problem hiding this comment.
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;
}
}
...
Member
There was a problem hiding this comment.
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 🤔
Merged
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Proposed changes
Issue(s)
How to test or reproduce
Screenshots
Types of changes
Changelog
Further comments