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

maratal · 2021-10-10T21:41:02Z

This is what I came up with:

Until there are separate server endpoints for getting ARTDevicePushDetails/recipient and ARTPushState, they should be kept in one place (ARTDevicePushDetails).
We should never persist ARTPushState inside ARTLocalDevice, because it has no sense. If you need state - you just request it.

So, the new method is just a wrapper for a simple access to the push state of the local device (which makes sense).

ben-xD

When a user gets the local device information for their current device, the state will always be .unknown. It's definitely not 'nice' to do this, but it is an improvement of having it always be null. Would it be better to create a new type, so that we are not using this type for both push admin and normal push?

I have been testing the current changes in Flutter, but that has required some work (fixing serialization, adding buttons and UI to show the state), since this is a new API.

Also, your new getState actually returns a ARTDevicePushDetails in the callback, not just state. Should it be getDeviceDetails instead? This is because you have defined and implemented:

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

Source/ARTDevicePushDetails.h

Source/ARTDevicePushDetails.m

ben-xD · 2021-10-11T14:47:34Z

I've made changes in Ably-Flutter to use this new API. In the screenshot below, you can see state being updated in the UI (see get push state):

Once this work is merged, this feature will probably not be released on Ably-Flutter until after Ably-Java gets it as well. See ably/ably-java#697 We need to update the Ably-Java issue so that the work is consistent with this PR.

Note: In the Flutter interface in my PR, I have called the method getDevicePushDetails, since this is what it is doing, rather than calling it getState. What do you think? @maratal , should you rename it? (In the UI, it still says "push state", because the UI is only showing that state, im not showing the rest of it.)

maratal · 2021-10-12T13:19:14Z

I've named it getState since it's already inside ARTPush object. Also in case of different server endpoints for state and other push details there will be different data structure in response. So naming it getState will make sense. On the other hand, I'm sure this information (push state and other push details) is stored on the server in the same db record, so having different endpoints seems unnecessary. So probably there will not be different endpoints, thus response structure will stay the same as it's now (ARTDevicePushDetails) and naming this new method getDevicePushDetails is a good idea.

ben-xD · 2021-10-12T13:38:34Z

Source/ARTPush.m

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

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


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?

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

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?

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.

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.

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

Moved files out of Admin group as well d447d2c

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

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

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

maratal · 2021-10-13T20:00:20Z

@ben-xD Regarding different type for getDevicePushDetails. It requires removing properties of the new type from an old type, which requires duplicating the request/response code for the same endpoint for the sole purpose of having different type. I still think it's unnecessary.

ben-xD · 2021-10-20T15:01:12Z

@ben-xD Regarding different type for getDevicePushDetails. It requires removing properties of the new type from an old type, which requires duplicating the request/response code for the same endpoint for the sole purpose of having different type. I still think it's unnecessary.

The problem is this endpoint is designed for a wider use case: Push admin. We should have a more restricted endpoint or functionality which allows users to get their device details without push admin.

jamienewcomb · 2021-11-04T16:41:06Z

@maratal have you addressed Bens comment here? or can @lawrence-forooghian now review

maratal · 2021-11-10T14:27:49Z

ably/specification#26

maratal · 2021-11-12T19:23:24Z

@ben-xD Should we merge this? (since further fixes are server-side)

ben-xD · 2021-11-14T09:35:32Z

@ben-xD Should we merge this? (since further fixes are server-side)

Nope I think we should wait for the fix to be done. We would be releasing an API to users which doesn't work as expected, which is a bad thing.

ben-xD · 2021-11-14T09:38:17Z

Since the method has been renamed to getDevicePushDetails,the title of the issue and your new replies should stop using getState, opting for getDevicePushDetails instead?

Additionally, the feature spec needs to be updated with this API as well: getDevicePushDetails. This needs to be done asap. It should be done as part of: ably/specification#26

…vate header" This reverts commit 37486f1.

…der (merge fixed)

… Admin to Push namespace

lukasz-szyszkowski · 2022-01-10T06:54:51Z

Source/ARTDevicePushDetails.m

+    }
+}
+
+ (ARTPushState)stateFromString:(NSString *)string {


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.

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?

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

good point b03a251

lawrence-forooghian

I think we should come up with a cross-platform consensus on this before adding this functionality.

I also think we need to decide what we want to do about the still-existing ARTLocalDevice.{state, errorReason} properties, which are still returning nil (which was the original problem described in the linked #1162). Do we want to remove them?

I think https://github.com/ably/docs/issues/1288 would be the best place to discuss this more widely.

lawrence-forooghian · 2022-01-18T14:08:08Z

Source/ARTDevicePushDetails.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;


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.

lawrence-forooghian · 2022-01-18T14:11:09Z

Source/ARTPushAdmin+Private.h

@@ -9,9 +7,6 @@ NS_ASSUME_NONNULL_BEGIN

 @interface ARTPushAdminInternal : NSObject <ARTPushAdminProtocol>

-@property (nonatomic, readonly) ARTPushDeviceRegistrationsInternal *deviceRegistrations;


Moving these properties is a change to the public API, and also these properties are described in the features spec; we’d need to update the features spec before making this change and need to make sure we do a new major release of the library afterwards.

lawrence-forooghian · 2022-01-18T14:12:28Z

Source/ARTPush.h

@@ -75,13 +77,17 @@ NS_ASSUME_NONNULL_BEGIN
 */
 - (void)deactivate;

+- (void)getDevicePushDetails:(ARTPushStateCallback)callback;


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.

maratal · 2022-01-20T17:29:28Z

I'm closing this PR in favor of upcoming PRs for issues: #1268, #1264 and other issues spawned from this one - ably/specification#25

New ARTPush/getState method for simplifying push state access

d1f1c49

maratal marked this pull request as draft October 10, 2021 21:41

maratal linked an issue Oct 10, 2021 that may be closed by this pull request

Read and persist state returned in ARTLocalDevice/ ARTDeviceDetails #1162

Closed

maratal requested review from ben-xD, lukasz-szyszkowski and QuintinWillison October 10, 2021 21:42

maratal marked this pull request as ready for review October 11, 2021 11:20

ben-xD reviewed Oct 11, 2021

View reviewed changes

Source/ARTDevicePushDetails.h Outdated Show resolved Hide resolved

Source/ARTDevicePushDetails.h Outdated Show resolved Hide resolved

Source/ARTDevicePushDetails.m Show resolved Hide resolved

ben-xD mentioned this pull request Oct 11, 2021

Get DevicePushDetails (e.g. state) by making network request ably/ably-flutter#187

Closed

ben-xD reviewed Oct 12, 2021

View reviewed changes

maratal added 3 commits October 13, 2021 21:40

ARTDevicePushDetails/state serialisation helpers moved to private header

37486f1

Debug strings for ARTDevicePushDetails/state

e378eb7

Renamed getState to getDevicePushDetails

2758a9b

maratal self-assigned this Oct 29, 2021

QuintinWillison requested a review from lawrence-forooghian November 2, 2021 07:54

maratal mentioned this pull request Nov 7, 2021

Push#getDevicePushDetails method to make HTTP request ably/specification#26

Open

maratal changed the title ~~New ARTPush/getState method for simplifying push state access~~ New ARTPush/getDevicePushDetails method for simplifying push state access Nov 14, 2021

maratal added 4 commits January 9, 2022 18:48

Revert "ARTDevicePushDetails/state serialisation helpers moved to pri…

454a72c

…vate header" This reverts commit 37486f1.

Merge remote-tracking branch 'origin/main' into fix/1162-get-push-state

8d659f8

ARTDevicePushDetails/state serialisation helpers moved to private hea…

a7a1e16

…der (merge fixed)

Moved ARTPushDeviceRegistrations and ARTPushChannelSubscriptions from…

b6f1eaf

… Admin to Push namespace

maratal added 4 commits January 9, 2022 19:53

Tests updated for moved Admin functionality

3de9965

PushAdmin tests moved to Push tests

bc168d6

PushAdminTests cleanup

54a6fac

devicePushDetailsFromDictionary error field name fix

32d1bb5

lukasz-szyszkowski reviewed Jan 10, 2022

View reviewed changes

maratal added 2 commits January 13, 2022 14:53

More obvious naming for converting json into value

b03a251

Moved files from Admin group into Push group

d447d2c

maratal mentioned this pull request Jan 17, 2022

Read and persist state returned in ARTLocalDevice/ ARTDeviceDetails #1162

Closed

Test for failed state (draft)

d15531a

maratal marked this pull request as draft January 17, 2022 00:55

lawrence-forooghian mentioned this pull request Oct 3, 2022

We do not explain how a client lib should populate LocalDevice.push ably/specification#25

Open

lawrence-forooghian reviewed Jan 18, 2022

View reviewed changes

maratal closed this Jan 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

maratal commented Oct 10, 2021

ben-xD left a comment •

edited

Loading

ben-xD commented Oct 11, 2021 •

edited

Loading

maratal commented Oct 12, 2021

ben-xD Oct 12, 2021

maratal Oct 15, 2021 •

edited

Loading

ben-xD Oct 15, 2021 •

edited

Loading

maratal Oct 15, 2021 •

edited

Loading

ben-xD Oct 15, 2021

maratal Jan 10, 2022

maratal Jan 13, 2022

maratal Jan 13, 2022

QuintinWillison Jan 18, 2022

lawrence-forooghian Jan 18, 2022

maratal commented Oct 13, 2021

ben-xD commented Oct 20, 2021

jamienewcomb commented Nov 4, 2021 •

edited

Loading

maratal commented Nov 10, 2021

maratal commented Nov 12, 2021

ben-xD commented Nov 14, 2021

ben-xD commented Nov 14, 2021 •

edited

Loading

lukasz-szyszkowski Jan 10, 2022

maratal Jan 10, 2022

lukasz-szyszkowski Jan 13, 2022

maratal Jan 13, 2022

lawrence-forooghian left a comment

lawrence-forooghian Jan 18, 2022

lawrence-forooghian Jan 18, 2022

lawrence-forooghian Jan 18, 2022

maratal commented Jan 20, 2022 •

edited

Loading

		@@ -9,9 +7,6 @@ NS_ASSUME_NONNULL_BEGIN

		@interface ARTPushAdminInternal : NSObject <ARTPushAdminProtocol>

		@property (nonatomic, readonly) ARTPushDeviceRegistrationsInternal *deviceRegistrations;

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

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

Conversation

maratal commented Oct 10, 2021

ben-xD left a comment • edited Loading

Choose a reason for hiding this comment

ben-xD commented Oct 11, 2021 • edited Loading

maratal commented Oct 12, 2021

Choose a reason for hiding this comment

maratal Oct 15, 2021 • edited Loading

Choose a reason for hiding this comment

ben-xD Oct 15, 2021 • edited Loading

Choose a reason for hiding this comment

maratal Oct 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maratal commented Oct 13, 2021

ben-xD commented Oct 20, 2021

jamienewcomb commented Nov 4, 2021 • edited Loading

maratal commented Nov 10, 2021

maratal commented Nov 12, 2021

ben-xD commented Nov 14, 2021

ben-xD commented Nov 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lawrence-forooghian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maratal commented Jan 20, 2022 • edited Loading

ben-xD left a comment •

edited

Loading

ben-xD commented Oct 11, 2021 •

edited

Loading

maratal Oct 15, 2021 •

edited

Loading

ben-xD Oct 15, 2021 •

edited

Loading

maratal Oct 15, 2021 •

edited

Loading

jamienewcomb commented Nov 4, 2021 •

edited

Loading

ben-xD commented Nov 14, 2021 •

edited

Loading

maratal commented Jan 20, 2022 •

edited

Loading