New ARTPush/getDevicePushDetails method for simplifying push state access #1186
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| #import <Ably/ARTDevicePushDetails.h> | ||
|
|
||
| NS_ASSUME_NONNULL_BEGIN | ||
|
|
||
| @interface ARTDevicePushDetails () | ||
|
|
||
| - (NSString *)stateString; | ||
|
|
||
| + (ARTPushState)stateFromString:(NSString *)string; | ||
|
|
||
| @end | ||
|
|
||
| NS_ASSUME_NONNULL_END |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| #import "ARTDevicePushDetails.h" | ||
| #import "ARTDevicePushDetails+Private.h" | ||
| #import "ARTPush.h" | ||
|
|
||
| @implementation ARTDevicePushDetails | ||
|
|
@@ -20,8 +21,35 @@ - (id)copyWithZone:(NSZone *)zone { | |
| return push; | ||
| } | ||
|
|
||
| - (NSString *)stateString { | ||
| switch (_state) { | ||
| case ARTPushStateActive: | ||
| return @"ARTPushStateActive"; | ||
| case ARTPushStateFailing: | ||
| return @"ARTPushStateFailing"; | ||
| case ARTPushStateFailed: | ||
| return @"ARTPushStateFailed"; | ||
| default: | ||
| return @"ARTPushStateUnknown"; | ||
| } | ||
| } | ||
|
|
||
| + (ARTPushState)stateFromString:(NSString *)string { | ||
|
There was a problem hiding this comment. Probably just naming, but I expect to use here values from There was a problem hiding this comment. Hmm, I'm not sure what you are saying here, this method is for obtaining There was a problem hiding this comment. Ok, so you should rename it to - (NSString *)stateString {
switch (_state) {
case ARTPushStateActive:
return @"ARTPushStateActive";
case ARTPushStateFailing:
return @"ARTPushStateFailing";
case ARTPushStateFailed:
return @"ARTPushStateFailed";
default:
return @"ARTPushStateUnknown";
}
}which also produce strings from There was a problem hiding this comment. good point b03a251 |
||
| string = string.lowercaseString; | ||
| if ([string isEqualToString:@"active"]) { | ||
| return ARTPushStateActive; | ||
| } | ||
| else if ([string isEqualToString:@"failing"]) { | ||
| return ARTPushStateFailing; | ||
| } | ||
| else if ([string isEqualToString:@"failed"]) { | ||
| return ARTPushStateFailed; | ||
| } | ||
| return ARTPushStateUnknown; | ||
| } | ||
|
|
||
| - (NSString *)description { | ||
| return [NSString stringWithFormat:@"%@ - \n\t recipient: %@; \n\t state: %@; \n\t errorReason: %@;", [super description], self.recipient, self.stateString, self.errorReason]; | ||
ben-xD marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| @end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,6 +75,8 @@ NS_ASSUME_NONNULL_BEGIN | |
| */ | ||
| - (void)deactivate; | ||
|
|
||
| - (void)getDevicePushDetails:(ARTPushStateCallback)callback; | ||
|
There was a problem hiding this comment. I think this needs to be agreed cross-platform (i.e. via the features spec) before we add it here. I think https://github.com/ably/docs/issues/1288 would be the best place to come to a consensus on this. |
||
|
|
||
| #endif | ||
|
|
||
| @end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,6 +61,10 @@ - (void)deactivate { | |
| [_internal deactivate]; | ||
| } | ||
|
|
||
| - (void)getDevicePushDetails:(nonnull ARTPushStateCallback)callback { | ||
| [_internal getDevicePushDetails:callback]; | ||
| } | ||
|
|
||
| #endif | ||
|
|
||
| @end | ||
|
|
@@ -235,6 +239,16 @@ - (void)deactivate { | |
| }]; | ||
| } | ||
|
|
||
| - (void)getDevicePushDetails:(nonnull ARTPushStateCallback)callback { | ||
| [_admin.deviceRegistrations get:_rest.device.id callback:^(ARTDeviceDetails *device, ARTErrorInfo *error) { | ||
|
There was a problem hiding this comment. Interestingly, it is using the push admin class, but when I removed the push admin capability from the API key, it stil works. In other words: I did not need push admin. I wonder why this is the case. We should confirm and document if this method requires Push Admin. WDYT? There was a problem hiding this comment. What I've found out is if the Admin capability got turned off you need to deactivate/activate push on the device to changes to take effect. In my case not only I've lost admin capability, it stops me from doing anything with pushes, even publish them with deviceId with an error: WDYT @ben-xD There was a problem hiding this comment. You can't publish by device ID without push admin, so that makes sense. So in summary, you discovered that this method (& endpoint) requires push admin? I think this is a feature that we want clients without push-admin to have access to. Perhaps a separate endpoint should be created then? There was a problem hiding this comment.
No, I've discovered that I can't send pushes neither by deviceId not clientId, but that's I guess how it should be. There was a problem hiding this comment. You can only send pushes by messages (with push payload), if you don't have push admin. To send by client ID, you also need push admin. There was a problem hiding this comment.
As we can see here in java https://github.com/ably/ably-java/blob/main/lib/src/main/java/io/ably/lib/push/PushBase.java There was a problem hiding this comment. Moved files out of There was a problem hiding this comment. @QuintinWillison if you are not against these changes please resolve this conversation since Ben started it. There was a problem hiding this comment. @lawrence-forooghian could you please take a look and resolve this conversation if you feel it can be. I've not got enough context or time to get involved right now. There was a problem hiding this comment. @maratal is your question "can we move |
||
| if (error != nil) { | ||
| callback(nil, error); | ||
| return; | ||
| } | ||
| callback(device.push, nil); | ||
| }]; | ||
| } | ||
|
|
||
| #endif | ||
|
|
||
| @end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change (string to enum) can be a separate thing, which we should do regardless of how we solve the
LocalDeviceissue. I've created an issue for it: #1264.