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.

Sign up for GitHub

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

New ARTPush/getDevicePushDetails method for simplifying push state access #1186

Closed
wants to merge 15 commits into from
+1,044 −943
Closed

New ARTPush/getDevicePushDetails method for simplifying push state access #1186

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
d1f1c49
New ARTPush/getState method for simplifying push state access
maratal Oct 10, 2021
37486f1
ARTDevicePushDetails/state serialisation helpers moved to private header
maratal Oct 13, 2021
e378eb7
Debug strings for ARTDevicePushDetails/state
maratal Oct 13, 2021
2758a9b
Renamed getState to getDevicePushDetails
maratal Oct 13, 2021
454a72c
Revert "ARTDevicePushDetails/state serialisation helpers moved to pri…
maratal Jan 9, 2022
8d659f8
Merge remote-tracking branch 'origin/main' into fix/1162-get-push-state
maratal Jan 9, 2022
a7a1e16
ARTDevicePushDetails/state serialisation helpers moved to private hea…
maratal Jan 9, 2022
b6f1eaf
Moved ARTPushDeviceRegistrations and ARTPushChannelSubscriptions from…
maratal Jan 9, 2022
3de9965
Tests updated for moved Admin functionality
maratal Jan 9, 2022
bc168d6
PushAdmin tests moved to Push tests
maratal Jan 9, 2022
54a6fac
PushAdminTests cleanup
maratal Jan 9, 2022
32d1bb5
devicePushDetailsFromDictionary error field name fix
maratal Jan 10, 2022
b03a251
More obvious naming for converting json into value
maratal Jan 13, 2022
d447d2c
Moved files from `Admin` group into `Push` group
maratal Jan 13, 2022
d15531a
Test for failed state (draft)
maratal Jan 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions Source/ARTDevicePushDetails.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,21 @@
#import <Foundation/Foundation.h>
#import <Ably/ARTTypes.h>

@class ARTErrorInfo;

NS_ASSUME_NONNULL_BEGIN

@interface ARTDevicePushDetails : NSObject

@property (nonatomic) NSMutableDictionary<NSString *, NSString *> *recipient;
@property (nullable, nonatomic) NSString *state;
@property (nonatomic) ARTPushState state;
Copy link
Collaborator

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 LocalDevice issue. I've created an issue for it: #1264.

@property (nullable, nonatomic) ARTErrorInfo *errorReason;
@property (nonatomic) NSMutableDictionary<NSString *, NSString *> *recipient;

- (instancetype)init;

- (NSString *)stateString;
ben-xD marked this conversation as resolved.
Show resolved Hide resolved

+ (ARTPushState)stateFromString:(NSString *)string;
ben-xD marked this conversation as resolved.
Show resolved Hide resolved

@end

NS_ASSUME_NONNULL_END
29 changes: 28 additions & 1 deletion Source/ARTDevicePushDetails.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,35 @@ - (id)copyWithZone:(NSZone *)zone {
return push;
}

- (NSString *)stateString {
switch (_state) {
case ARTPushStateActive:
return @"Active";
case ARTPushStateFailing:
return @"Failing";
case ARTPushStateFailed:
return @"Failed";
default:
return @"Unknown";
}
}

+ (ARTPushState)stateFromString:(NSString *)string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably just naming, but I expect to use here values from stateString like ARTPushStateActive, ARTPushStateFailing etc.
Maybe additionally checking these values in this method could be a better idea than renaming this method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, I'm not sure what you are saying here, this method is for obtaining ARTPushState values from strings that came from JSON data. Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, so you should rename it to stateFromJsonString.
I meant you have this method:

- (NSString *)stateString {
    switch (_state) {
        case ARTPushStateActive:
            return @"ARTPushStateActive";
        case ARTPushStateFailing:
            return @"ARTPushStateFailing";
        case ARTPushStateFailed:
            return @"ARTPushStateFailed";
        default:
            return @"ARTPushStateUnknown";
    }
}

which also produce strings from ARTPushState and the method name stateFromString is a bit confusing because I don't know "from which string" it creates the ARTPushState

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.state, self.errorReason];
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
2 changes: 1 addition & 1 deletion Source/ARTJsonLikeEncoder.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -699,7 +699,7 @@ - (ARTDevicePushDetails *)devicePushDetailsFromDictionary:(NSDictionary *)input
}

ARTDevicePushDetails *devicePushDetails = [[ARTDevicePushDetails alloc] init];
devicePushDetails.state = [input artString:@"state"];
devicePushDetails.state = [ARTDevicePushDetails stateFromString:[input artString:@"state"]];
NSDictionary *errorReason = [input valueForKey:@"errorReason"];
if (errorReason) {
devicePushDetails.errorReason = [ARTErrorInfo createWithCode:[[errorReason artNumber:@"code"] intValue] status:[[errorReason artNumber:@"statusCode"] intValue] message:[errorReason artString:@"message"]];
Expand Down
2 changes: 2 additions & 0 deletions Source/ARTPush.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ NS_ASSUME_NONNULL_BEGIN
*/
- (void)deactivate;

- (void)getState:(ARTPushStateCallback)callback;

#endif

@end
Expand Down
14 changes: 14 additions & 0 deletions Source/ARTPush.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,10 @@ - (void)deactivate {
[_internal deactivate];
}

- (void)getState:(nonnull ARTPushStateCallback)callback {
[_internal getState:callback];
}

#endif

@end
Expand Down Expand Up @@ -235,6 +239,16 @@ - (void)deactivate {
}];
}

- (void)getState:(nonnull ARTPushStateCallback)callback {
[_admin.deviceRegistrations get:_rest.device.id callback:^(ARTDeviceDetails *device, ARTErrorInfo *error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

@maratal maratal Oct 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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:

{
   code = 40160;
   href = "https://help.ably.io/error/40160";
   message = "action not permitted, app = 5****g";
   serverId = "frontend.****";
   statusCode = 401;
}

WDYT @ben-xD

Copy link
Contributor

@ben-xD ben-xD Oct 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

@maratal maratal Oct 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

You can't publish by device ID without push admin, so that makes sense. So in summary, you discovered that this endpoint requires push admin?

No, I've discovered that I can't send pushes neither by deviceId not clientId, but that's I guess how it should be.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It appears like the _admin instance is being used, which would be very confusing

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
DeviceRegistrations is on the same level as Admin, and the latter contains nothing except publish method. So I assume ARTPushAdmin in cocoa contains ARTPushDeviceRegistrations and ARTPushChannelSubscriptions by mistake. So I moved it from there into the ARTPush namespace - b6f1eaf
@QuintinWillison @lawrence-forooghian @lukasz-szyszkowski

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved files out of Admin group as well d447d2c

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@QuintinWillison if you are not against these changes please resolve this conversation since Ben started it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@maratal is your question "can we move deviceRegistrations and channelSubscriptions from ARTPushAdmin to ARTPush"? If so, I think this comment of mine addresses it.

if (error != nil) {
callback(nil, error);
return;
}
callback(device.push, nil);
}];
}

#endif

@end
9 changes: 9 additions & 0 deletions Source/ARTTypes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
@class ARTStats;
@class ARTPushChannelSubscription;
@class ARTDeviceDetails;
@class ARTDevicePushDetails;
@protocol ARTTokenDetailsCompatible;

// More context
Expand All @@ -38,6 +39,12 @@ typedef NS_ENUM(NSUInteger, ARTAuthMethod) {
ARTAuthMethodToken
};

typedef NS_ENUM(NSUInteger, ARTPushState) {
ARTPushStateUnknown,
ARTPushStateActive,
ARTPushStateFailing,
ARTPushStateFailed
};

#pragma mark - ARTRealtimeConnectionState

Expand Down Expand Up @@ -279,6 +286,8 @@ typedef void (^ARTPaginatedMessagesCallback)(ARTPaginatedResult<ARTMessage *> *_
typedef void (^ARTPaginatedDeviceDetailsCallback)(ARTPaginatedResult<ARTDeviceDetails *> *_Nullable result, ARTErrorInfo *_Nullable error);
typedef void (^ARTPaginatedTextCallback)(ARTPaginatedResult<NSString *> *_Nullable result, ARTErrorInfo *_Nullable error);

typedef void (^ARTPushStateCallback)(ARTDevicePushDetails *_Nullable details, ARTErrorInfo *_Nullable error);

#pragma mark - Functions

/**
Expand Down