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.

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

Update Payload.php #131

Merged
merged 5 commits into from Mar 22, 2021
Merged

Update Payload.php #131

merged 5 commits into from Mar 22, 2021

Conversation

KoIIIeY
Copy link
Contributor

@KoIIIeY KoIIIeY commented Mar 10, 2021

Fix for able to do like this:

public function toApnVoip($notifiable)
{
return ApnVoipMessage::create()
->body(json_encode($this->card))
->custom('aps', ['alert' => ['incoming_caller_id' => $this->card->patient->user_id, 'incoming_caller_name' => $this->card->patient->fullName, 'uuid' => \uniqid()]])
->badge(1);
}

Fix for able to do like this:

public function toApnVoip($notifiable)
    {
        return ApnVoipMessage::create()
                ->body(json_encode($this->card))
                ->custom('aps', ['alert' => ['incoming_caller_id' => $this->card->patient->user_id, 'incoming_caller_name' => $this->card->patient->fullName, 'uuid' => \uniqid()]])
            ->badge(1);
    }
In case when alert used as string (as I understand, it should be JSON), serializator serialize JSON string to another JSON, so alert becomes JSON string in field alert except JSON object
Example:
Needed - {"aps":{"alert":{"a":1,"b":2}}}

Real - {"aps": {"alert":"{\"a\":1,\"b\":2}"}}
src/Payload.php Outdated
$payload[self::PAYLOAD_ROOT_KEY]->{self::PAYLOAD_ALERT_KEY} = $this->alert;
} elseif(is_string($this->alert)){
$payload[self::PAYLOAD_ROOT_KEY]->{self::PAYLOAD_ALERT_KEY} = json_decode($this->alert, true);
Copy link
Owner

Choose a reason for hiding this comment

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

What if $this->alert is a string, but it is not valid json? Seems we need one more check - if it is a valid json then use json_decode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I will add check.

}

$this->customValues[$key] = $value;
$this->customValues = array_merge_recursive($this->customValues ? $this->customValues : [], [$key => $value]);
Copy link
Owner

Choose a reason for hiding this comment

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

Why don't we need here the check - if $key === self::PAYLOAD_ROOT_KEY ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because now it's not removes previously defined data in PAYLOAD_ROOT_KEY, it's merges.

In my code I do the following:
public function toApnVoip($notifiable)
{
return ApnVoipMessage::create()
->custom('aps', [
'alert' => [
'incoming_caller_id' => (string)$this->card->card_id,
'incoming_caller_name' => $this->card->therapist->fullName,
'uuid' => (string) \Str::uuid(),
'card' => $this->card
]
])
->badge(1);
}

So I set 'aps' as custom array, but I dont removes preciously setted data

Copy link
Owner

Choose a reason for hiding this comment

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

I think it should be a separate method. Something like addCustomValue, not setCustomValue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May be you can just remove check that field named 'aps' ?

@KoIIIeY KoIIIeY requested a review from edamov March 17, 2021 12:06
$json = json_decode($this->alert, true);
if($json){
$payload[self::PAYLOAD_ROOT_KEY]->{self::PAYLOAD_ALERT_KEY} = $json;
}
Copy link
Owner

Choose a reason for hiding this comment

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

We still miss the case when $this->alert is not a json, but just a string. In this case it will not be added to payload, but I think it should be added

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You want to add exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still miss the case when $this->alert is not a json, but just a string. In this case it will not be added to payload, but I think it should be added

I understand that you want ^)
fixed

@edamov
Copy link
Owner

edamov commented Mar 18, 2021

Now I understand what you want to achieve :) I think it is better to add an ability to add custom value in the Alert class, and not change the payload custom value method.

@KoIIIeY
Copy link
Contributor Author

KoIIIeY commented Mar 18, 2021

Now I understand what you want to achieve :) I think it is better to add an ability to add custom value in the Alert class, and not change the payload custom value method.

The problem is that I also need to change the laravel notification channels in order for it to work :)
In theory, it will be enough for me to at least insert data directly as a string, given that no one has noticed this problem before, then nothing will break for anyone.

@KoIIIeY KoIIIeY requested a review from edamov March 19, 2021 11:09
@KoIIIeY
Copy link
Contributor Author

KoIIIeY commented Mar 19, 2021

Ok, I done all changes that we both wants.

Added array as value for alert in setAlert, so we doesnt need always use JSON serialize and deserialize for settings.
Your setCustom returned to original and my new method addCustomValue added.

Plz check and merge

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.4%) to 80.662% when pulling cdeeb24 on KoIIIeY:patch-1 into aada72b on edamov:master.

@KoIIIeY
Copy link
Contributor Author

KoIIIeY commented Mar 22, 2021

@edamov , can you check?
My project needs that changes :)

And I already created new pull request to Laravel notification channels
laravel-notification-channels/apn#122

@edamov edamov merged commit 743397b into edamov:master Mar 22, 2021
@edamov edamov linked an issue Mar 22, 2021 that may be closed by this pull request
@KoIIIeY KoIIIeY deleted the patch-1 branch March 22, 2021 09:55
@KoIIIeY
Copy link
Contributor Author

KoIIIeY commented Mar 22, 2021

Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set custom alert values
3 participants