-
Notifications
You must be signed in to change notification settings - Fork 83
[SDK-45] Fix iOS deep link resolution failures for greenFi #974
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,3 +39,5 @@ CI.swift | |
| *.mdb | ||
| .build/arm64-apple-macosx/debug/IterableSDK.build/output-file-map.json | ||
| .build | ||
|
|
||
| agent/ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -114,11 +114,13 @@ extension RedirectNetworkSession: URLSessionDelegate, URLSessionTaskDelegate { | |
|
|
||
| guard let headerFields = response.allHeaderFields as? [String: String] else { | ||
| delegate?.onRedirect(deepLinkLocation: deepLinkLocation, campaignId: campaignId, templateId: templateId, messageId: messageId) | ||
| completionHandler(nil) | ||
| return | ||
| } | ||
|
|
||
| guard let url = response.url else { | ||
| delegate?.onRedirect(deepLinkLocation: deepLinkLocation, campaignId: campaignId, templateId: templateId, messageId: messageId) | ||
| completionHandler(nil) | ||
|
Comment on lines
+117
to
+123
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need the nil completionHandler here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thats just a bug that was there the completionhandler wasnt fired if there was no info in the headers
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see! So, to double check, what we want is (as per the documentation) to pass nil on the completionHandler so we let "the body of the redirection response to be delivered as the payload of this request" correct?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. basically completion handler should be always fired before returning here
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we pass nil on the completionHandler to not follow any further redirects the redirection is passed in the delegate callbacks
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but effectively yes when the conpletionHandler(nil) is fired it would finish the redirection flow |
||
| return | ||
| } | ||
|
|
||
|
|
||
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 understand that the
deepLinkLocationis updated on theonRedirectdelegate method. It's just weird that we are conditioning the logic of thismakeDataRequestcompletionHandler be dependent on something happening elsewhere.Logically I think this makes sense, but is there a way for us to better document this in code or use a different architecture to ensure it makes sense to disregard an error on a
networkRequestcompletionHandler?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.
well i wouldnt recommend refactoring but logically it means that we have a redirect url we use it no matter what the http connection or tcp connection returns. Its more robust than before