fix: Setting the iOS alert property overwrites the title property#287
Open
chadpav wants to merge 4 commits intoparse-community:masterfrom
Open
fix: Setting the iOS alert property overwrites the title property#287chadpav wants to merge 4 commits intoparse-community:masterfrom
alert property overwrites the title property#287chadpav wants to merge 4 commits intoparse-community:masterfrom
Conversation
…that alert will overwrite title.
Thanks for opening this pull request!
|
chadpav
changed the title
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
mtrezza
requested changes
Oct 31, 2024
| }); | ||
| fcm.pushType = 'apple'; | ||
| const data = { data: { title: 'title' } }; | ||
| const data = { data: { title: 'title', alert: 'alert' } }; |
Member
There was a problem hiding this comment.
Could you please add a new test to test specifically what this fix is about, rather than modifying an existing test?
mtrezza
reviewed
Oct 31, 2024
| // When we receive a value, prepare `alert` dictionary if needed | ||
| // and set its `body` property | ||
| apnsPayload['apns']['payload']['aps']['alert'] = {}; | ||
| if (!apnsPayload['apns']['payload']['aps'].hasOwnProperty('alert')) { |
Member
There was a problem hiding this comment.
I understand that this is a replication of the logic under caes 'title':, correct? However, I find it strange to use hasOwnProperty for this, did you just use this from the other case?
mtrezza
changed the title
alert property is overwriting iOS title property
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
alert property is overwriting iOS title propertyalert property is overwriting iOS title property
mtrezza
changed the title
alert property is overwriting iOS title propertyalert property overwrites the title property
Member
|
Just a friendly ping, if you find some time to address the review feedback, then we could go ahead and merge this... |
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
2 participants
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.
New Pull Request Checklist
Issue Description
When
titleandalertare provided as a strings, the adapter code has to convert the Parse dictionary to the format that FCM is expecting. This would set the title property first and then reinitialize the alert key before setting the alert. This would wipe out the title property.Related issue: #286
Approach
In the switch statement where both alert and title get set, the title property was correctly checking if the alert property was already set before initializing it. All I had to do was copy that code up to the
alertcase.Closes: #286
TODOs before merging