Support Safari Push and Mobile Device Management #9
Conversation
|
Can we add a test case around this? |
|
I'd like to. Still test failures on master though, even after #8. I'm new to Ginkgo. |
| @@ -85,7 +90,11 @@ func (p *Payload) SetCustomValue(key string, value interface{}) error { | |||
| } | |||
|
|
|||
| func (p *Payload) MarshalJSON() ([]byte, error) { | |||
| p.customValues["aps"] = p.APS | |||
| if len(p.MDM) != 0 { | |||
There was a problem hiding this comment.
Sorry on the delay on this.
This feels a little awkward – there is some implicit behavior that isn't clear from the API. It feels like it should only be the if and no else. Whether or not "aps" is included shouldn't really depend on whether "mdm" exists.
My suggestion is:
if len(p.MDM) != 0 {
p.customValues["mdm"] = p.MDM
}
p.customValues["aps"] = p.APS
And then including the approach for APS in this PR https://github.com/nathany/apns/pull/1 – it'll make the code much cleaner.
There was a problem hiding this comment.
@bdotdub The thing is the json shouldn't include an empty "aps" node when using MDM.
Perhaps it makes more sense to check if aps is empty? Of course APS isn't just a string, so that's a deep check to ensure the entirety of APS is zero values.
There was a problem hiding this comment.
@bdotdub So I looked into your suggestion because I agree it's awkward as is.
But as far as I know, for Passbook notifications, the JSON should be {"aps":{}} even though there is nothing in the APS struct (aps.isZero() returns true).
For Mobile Device Management, there should be no aps key, as in {"mdm":"00000000-1111-3333-4444-555555555555"}.
So unfortunately there is a dependency there, at least as far as I can tell based on the documentation and other implementations. :-(
Conflicts: notification.go
|
I merged #14 into this. Once that's merged, I can rebase so that you can see just these changes. |
| if aps.Category != "" { | ||
| data["category"] = aps.Category | ||
| } | ||
| if aps.URLArgs != nil && len(aps.URLArgs) != 0 { |
There was a problem hiding this comment.
The need for this confuses me a bit, but it was crashing tests without both.
There was a problem hiding this comment.
The first condition is redundant.
There was a problem hiding this comment.
I thought so, but I was getting crashes later on after adding URLArgs in here. Weird. I can play with it a little more.
There was a problem hiding this comment.
That's odd... I'm guessing it's something else. http://play.golang.org/p/lGAUCTAjGz
|
Merging this! |
Support Safari Push and Mobile Device Management
|
Thanks! |
Rounding out support different types push notifications:
This just allows these types of notifications through, it doesn't ensure that they are fully valid. I'm doing validations in advance.