Feat: Add OneSignal push provider adapter (#7726)#131
Conversation
Greptile SummaryThis PR adds a OneSignal push notification adapter that maps the shared
Confidence Score: 3/5The adapter is functional for happy-path delivery but has two defects in the error-handling and content_available branches that produce incorrect output rather than failing gracefully. The unsafe response parsing in the error path corrupts every recipient's error message when OneSignal (or an intervening proxy) returns a non-JSON body, turning actionable error strings into a single garbage character. The content_available branch independently ignores the caller's boolean and always enables silent/background push, which inverts the caller's intent when they explicitly pass false. Both defects are in the changed file and affect observable output. Files Needing Attention: src/Utopia/Messaging/Adapter/Push/OneSignal.php — error-response parsing (line 112-114) and content_available handling (line 84-85) need fixes before merging. Important Files Changed
Reviews (5): Last reviewed commit: "fix: add parent constructor, fix deliver..." | Re-trigger Greptile |
| if (!\is_null($message->getSound())) { | ||
| $payload['android_sound'] = $message->getSound(); | ||
| $payload['ios_sound'] = $message->getSound() . '.wav'; | ||
| } |
There was a problem hiding this comment.
Other push adapters pass the message sound through unchanged, and callers may pass values like default or an already-qualified filename. Appending .wav here changes default to default.wav and chime.wav to chime.wav.wav, which can make iOS use the wrong sound or no custom sound at all.
| if (!\is_null($message->getSound())) { | |
| $payload['android_sound'] = $message->getSound(); | |
| $payload['ios_sound'] = $message->getSound() . '.wav'; | |
| } | |
| if (!\is_null($message->getSound())) { | |
| $payload['android_sound'] = $message->getSound(); | |
| $payload['ios_sound'] = $message->getSound(); | |
| } |
| if (!\is_null($message->getContentAvailable())) { | ||
| $payload['content_available'] = true; | ||
| } |
There was a problem hiding this comment.
content_available condition inverts intent when explicitly set to false
The guard !\is_null($message->getContentAvailable()) is true for both true and false. If a caller explicitly constructs Push with contentAvailable: false, the adapter still writes 'content_available' => true into the payload, silently enabling silent/background push on OneSignal devices when the caller's intent was the opposite. The fix is to check the boolean value directly: if ($message->getContentAvailable()).
|
Following up - OneSignal push adapter. Ready for review. |
…h fields (icon, color, tag, contentAvailable, priority)
No description provided.