Skip to content
This repository was archived by the owner on Sep 4, 2020. It is now read-only.

Conversation

@malwatte
Copy link
Contributor

@malwatte malwatte commented Aug 2, 2018

Description

FCM Instance ID will be deleted when unregistering. Added some code to check registered with FCM and delete the token if so.

Related Issue

#2082

Motivation and Context

I need to refresh FCM token in certain scenarios. But unregistering and registering again returns the same token. But with this change FCM instance ID will be deleted when unregistered. If you try to send a push notification after unregistering, you will get an "Not registered" error from fcm end-point (which should be the correct behavior I guess).

How Has This Been Tested?

Tested on a iPhone 6 (OS version 11.4).
Positive scenario:
Registered/unregistered multiple times. Got a new ID every time as expected. Sent notifications to all tokens. Notifications to the active token worked. Rest failed with "Not registered".
Negative scenario:
error callback was called if no network connection during unregistering.

##Note
Please review the pull request carefully since I am a Objective C noob.

Added code to delete Firebase token when unregistering in iOS
@macdonst macdonst mentioned this pull request Aug 2, 2018
6 tasks
@macdonst macdonst added this to the Release 3.0.0 milestone Aug 2, 2018
NSLog(@"unsubscribe from topic: %@", topic);
[pubSub unsubscribeFromTopic:topic];
}
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding an else if what about putting that code inside the else statement? So…

    else {
        if ([self usesFCM]) {
            [[FIRInstanceID instanceID] deleteIDWithHandler:^void(NSError *_Nullable error){
                if (error) {
                    [self failWithMessage:command.callbackId withMsg:@"" withError:error];
                } else {
                    [self successWithMessage:command.callbackId withMsg:@"unregistered"];
                }
            }];
        }
        [[UIApplication sharedApplication] unregisterForRemoteNotifications];
        [self successWithMessage:command.callbackId withMsg:@"unregistered"];
    }

This way the APNS token will get released as well.

Copy link
Contributor Author

@malwatte malwatte Aug 3, 2018

Choose a reason for hiding this comment

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

Yeah. Let me do that.
I do not know how APNS token works. Thats why I put inside an else. hehe.

Copy link
Contributor Author

@malwatte malwatte Aug 3, 2018

Choose a reason for hiding this comment

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

I tested it and there is a small problem. Success callback gets called even when FCM token delete failed. So I did something like this,

} else if ([self usesFCM]){
        [[FIRInstanceID instanceID] deleteIDWithHandler:^void(NSError *_Nullable error){
            if (error) {
                [self failWithMessage:command.callbackId withMsg:@"" withError:error];
            } else {
                [[UIApplication sharedApplication] unregisterForRemoteNotifications];
                [self successWithMessage:command.callbackId withMsg:@"unregistered"];
            }
        }];
} else {
        [[UIApplication sharedApplication] unregisterForRemoteNotifications];
        [self successWithMessage:command.callbackId withMsg:@"unregistered"];
}

Should be fine. Right?

Copy link
Contributor Author

@malwatte malwatte Aug 7, 2018

Choose a reason for hiding this comment

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

@macdonst
Are the changes ok?

malwatte and others added 5 commits August 3, 2018 08:44
PushInstanceIDListenerService doesn't have to be exported. So explicitly set the exported value to false
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants