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.

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

[MOB-6055] callback on setEmail/setUserId #629

Merged
merged 8 commits into from
Jun 1, 2023

Conversation

devcsomnicg
Copy link
Contributor

@devcsomnicg devcsomnicg commented Apr 26, 2023

🔹 Jira Ticket(s)

✏️ Description

Add callbacks for setEmail/setUserId, and it returns success/failure based on device token is registered successfully or not. It will always return success if autoPushRegistration is false.

@devcsomnicg
Copy link
Contributor Author

@roninopf Could you please start review for this ?

@roninopf roninopf self-requested a review April 26, 2023 19:31
@roninopf
Copy link
Contributor

@devcsomnicg Will do! Would you like to look over the tests as well?

@devcsomnicg
Copy link
Contributor Author

@roninopf updated code and tests, could you please run the tests again

@Ayyanchira
Copy link
Member

Will have to resolve the conflict. But looks like its replacing the existing method with one with additional parameter. As like for other SDKs, we should have additional methods for the ones with completionHandlers. This is to avoid breakages for developers who would just update to newer versions of SDK

@devcsomnicg
Copy link
Contributor Author

@Ayyanchira done, added additional methods.

@Ayyanchira Ayyanchira requested review from davidtruong, Ayyanchira and evantk91 and removed request for roninopf May 17, 2023 20:18
@devcsomnicg
Copy link
Contributor Author

@Ayyanchira Updated this with suggestions on PR. please review

1 similar comment
@devcsomnicg
Copy link
Contributor Author

@Ayyanchira Updated this with suggestions on PR. please review

@Ayyanchira Ayyanchira changed the base branch from master to omni-cg-master June 1, 2023 16:21
Copy link
Member

@Ayyanchira Ayyanchira left a comment

Choose a reason for hiding this comment

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

Added some comments. Merging to omni cg branch for internal testing. Looks good so far.

@@ -111,7 +111,7 @@ final class InternalIterableAPI: NSObject, PushTrackerProtocol, AuthProvider {
_payloadData = data
}

func setEmail(_ email: String?, authToken: String? = nil) {
func setEmail(_ email: String?, authToken: String? = nil, successHandler: OnSuccessHandler? = nil, failureHandler: OnFailureHandler? = nil) {
Copy link
Member

Choose a reason for hiding this comment

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

Had initial thought that we might need additional functions with and without handlers as we do in Android. But looks like having optional parameters makes it modern and lets us have only one signature instead of bloating up the IterableApi class. 👍🏼

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ayyanchira ok so for this we are good right ?

@@ -428,6 +440,9 @@ final class InternalIterableAPI: NSObject, PushTrackerProtocol, AuthProvider {
private var _email: String?
private var _payloadData: [AnyHashable: Any]?
private var _userId: String?
private var _successCallback: OnSuccessHandler? = nil
Copy link
Member

Choose a reason for hiding this comment

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

If this is going to be global but only used for setting the user credentials, we might have to rename it that way. Because, we will be adding success failure callbacks to other api calls / function calls as well and they will have to pass in success and failure callbacks too. using this global variable to store that and replace whats been stored in that can cause inconsistent behaviors

Copy link
Member

Choose a reason for hiding this comment

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

Like for example, updateEmail also uses success and failure callback handler to notify the state. But by not having it stored globally, it allows for good separation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ayyanchira yes we can rename it so that is identified differently, but it would be class variable because it is used from the whole different method and that is why we need to hold a reference to it

@Ayyanchira Ayyanchira merged commit 8a71b18 into Iterable:omni-cg-master Jun 1, 2023
@Ayyanchira
Copy link
Member

Checked and its working well. So lets go ahead with current changes as it is for now until other methods also start needing success and failure callback handlers.

@Ayyanchira
Copy link
Member

Test methods are failing @devcsomnicg . It looks like setEmail leads to AppExtensionHelper.application?.registerForRemoteNotifications(), but that doesn't lead to registerDevice Flow.

@devcsomnicg
Copy link
Contributor Author

Test methods are failing @devcsomnicg . It looks like setEmail leads to AppExtensionHelper.application?.registerForRemoteNotifications(), but that doesn't lead to registerDevice Flow.

I am not sure about where this code is AppExtensionHelper.application.registerForRemoteNotifications() but, to give you a example when you do registerForRemoteNotifications it calls this function didRegisterForRemoteNotificationsWithDeviceToken here https://github.com/Iterable/swift-sdk/blob/5c033ded01e200acee52c9bd1fca377dbdcbbbce/sample-apps/swift-sample-app/swift-sample-app/AppDelegate.swift#L105.

@Ayyanchira
Copy link
Member

Added a call to registerDevice() with no success failure callback after setEmail -> here in this PR. This commit.
Verified that it eventually triggers the callbacks.
This means the test assumes that registerDevice will eventually get called. And it checks if the handlers set while setting the credentials still remain active or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants