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

fix: sample app issues #551

Merged
merged 13 commits into from
Feb 22, 2024
7 changes: 5 additions & 2 deletions Sources/Common/Service/HttpRequestRunner.swift
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,11 @@ public class UrlRequestHttpRequestRunner: HttpRequestRunner {
let directoryURL = fileType.directoryToSaveFiles(fileManager: FileManager.default)

session.downloadTask(with: url) { tempLocation, response, _ in
guard let tempLocation = tempLocation, let uniqueFileName = response?.suggestedFilename else {
guard let tempLocation = tempLocation, let suggestedFileName = response?.suggestedFilename else {
return onComplete(nil)
}

// create a unique file name so when trying to move temp file to destination it doesn't give an exception
let uniqueFileName = UUID().uuidString + "_" + suggestedFileName
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should have a comment in the code explaining why having a unique file name is important. Or, why this UUID is needed beyond "suggest file name"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i would have hoped naming would have done the trick, as in suggestedFileName instead of uniqueFileName which was before, because suggested file name even from the name doesn't say its going to be unique.

and then adding UUID().uuidString and referring it as uniqueFileName to mention we would have a unique path.

So from a presepective of someone who is looking for the first time, they would understand whats happening just by looking into names.
But i can add comment on it, if it helps.

let destinationURL = directoryURL
.appendingPathComponent(uniqueFileName)

Expand All @@ -70,6 +71,8 @@ public class UrlRequestHttpRequestRunner: HttpRequestRunner {
withIntermediateDirectories: true,
attributes: nil
)

// Now attempt the move
try FileManager.default.moveItem(at: tempLocation, to: destinationURL)
} catch {
// XXX: log error when error handling for the customer enabled
Expand Down
20 changes: 2 additions & 18 deletions Sources/MessagingPush/RichPush/MessagingPush+RichPush.swift
Original file line number Diff line number Diff line change
Expand Up @@ -73,30 +73,14 @@ extension MessagingPushImplementation {

// This conditional will only work in production and not in automated tests. But this file cannot be in automated tests so this conditional is OK for now.
if let composedRichPush = composedRichPush as? UNNotificationWrapper {
self.finishTasksThenReturn(contentHandler: contentHandler, notificationContent: composedRichPush.notificationContent)
self.logger.info("Customer.io push processing is done!")
contentHandler(composedRichPush.notificationContent)
Copy link
Contributor

Choose a reason for hiding this comment

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

When we call the contentHandler(), we are telling the OS that our NSE is completely done doing what it needs to do.

This might be out of scope of this PR, but I think that we need to make sure that before we call the contentHandler, we confirm the delivered metric HTTP request has finished, correct?

I see above this line of code trackMetricFromNSE(deliveryID: pushCioDeliveryInfo.id, event: .delivered, deviceToken: pushCioDeliveryInfo.token) and because there is no async callback in that line of code, we are not informed when the HTTP request concludes, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, that is correct, i'll create a separate PR for it. But good callout.

}
}

return true
}

private func finishTasksThenReturn(
contentHandler: @escaping (UNNotificationContent) -> Void,
notificationContent: UNNotificationContent
) {
logger
.debug(
"running all background queue tasks and waiting until complete to prevent OS from killing notification service extension before all HTTP requests have been performed"
)
// FIXME: [CDP] Request to Journey
// backgroundQueue.run {
// self.logger.debug("all background queue tasks done running.")
// self.logger.info("Customer.io push processing is done!")
//
// contentHandler(notificationContent)
// }
}

/**
iOS telling the notification service to hurry up and stop modifying the push notifications.
Stop all network requests and modifying and show the push for what it looks like now.
Expand Down
8 changes: 5 additions & 3 deletions Sources/MessagingPush/RichPush/RichPushHttpClient.swift
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,8 @@ extension RichPushHttpClient {
public static let defaultAPIHost = "https://cdp.customer.io/v1"

static func authorizationHeaderForWriteKey(_ key: String) -> String {
var returnHeader = ""
if let encodedRawHeader = key.data(using: .utf8) {
var returnHeader = "\(key):"
if let encodedRawHeader = returnHeader.data(using: .utf8) {
returnHeader = encodedRawHeader.base64EncodedString(options: NSData.Base64EncodingOptions(rawValue: 0))
}
return returnHeader
Expand All @@ -160,8 +160,10 @@ extension RichPushHttpClient {

static func getBasicSession() -> URLSession {
let configuration = URLSessionConfiguration.ephemeral
configuration.httpMaximumConnectionsPerHost = 2
configuration.allowsCellularAccess = true
configuration.timeoutIntervalForResource = 30
configuration.timeoutIntervalForRequest = 60
configuration.httpAdditionalHeaders = [:]
Comment on lines +164 to +166
Copy link
Contributor

Choose a reason for hiding this comment

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

Why were these lines added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they are in main so I think they were adding values in there.

let session = URLSession(configuration: configuration, delegate: nil, delegateQueue: nil)
return session
}
Expand Down
2 changes: 1 addition & 1 deletion Tests/MessagingPush/HttpClientTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ class HttpClientTest: UnitTest {
func test_getBasicAuthHeaderString_givenHardCodedCredentials_expectCorrectString() {
let givenWriteKey = "oofjwo88283899c9jend"

let expected = "b29mandvODgyODM4OTljOWplbmQ="
let expected = "b29mandvODgyODM4OTljOWplbmQ6"
let actual = RichPushHttpClient.authorizationHeaderForWriteKey(givenWriteKey)

XCTAssertEqual(actual, expected)
Expand Down
2 changes: 1 addition & 1 deletion Tests/MessagingPush/HttpRequestRunnerTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import XCTest
*/
class HttpRequestRunnerTest: HttpTest {
func test_getAccountRegion() throws {
guard let runner = runner, let session = session else { return try XCTSkipIf(true) }
guard let runner = runner, let session = cioSession else { return try XCTSkipIf(true) }

let endpoint = CIOApiEndpoint.trackPushMetricsCdp

Expand Down
58 changes: 53 additions & 5 deletions Tests/MessagingPush/HttpTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -13,33 +13,81 @@ import XCTest

In order to *run* tests on your local machine, follow these setup steps:
1. In XCode, go to: Edit Scheme > Run
2. Create 2 environment variables: `SITE_ID` and `API_KEY`. Populate those values with a set
of test credentials from a Workspace that you control.
2. Create an environment variables: `WRITE_KEY`. Populate the values with test credentials from a source that you control.
3. Manually run the tests below. Use the XCode debug console to see the log output for debugging.
*/
open class HttpTest: UnitTest {
public var runner: HttpRequestRunner?
public var deviceInfo: DeviceInfo!
public var session: URLSession?
public var cioSession: URLSession?
public var session: URLSession = RichPushHttpClient.getBasicSession()

override open func setUp() {
super.setUp()

deviceInfo = diGraph.deviceInfo
session = RichPushHttpClient.getBasicSession()
runner = UrlRequestHttpRequestRunner()

/*
We don't want to run these tests on a CI server (flaky!) so, only populate the runner if
we see environment variables set in XCode.
*/
if let writeKey = getEnvironmentVariable("WRITE_KEY") {
runner = UrlRequestHttpRequestRunner()
session = RichPushHttpClient.getCIOApiSession(
cioSession = RichPushHttpClient.getCIOApiSession(
key: writeKey,
userAgentHeaderValue: deviceInfo.getUserAgentHeaderValue()
)
}
}

func testParallelDownloadFileCreatesUniquePaths() {
let expectation1 = expectation(description: "Parallel download file 1")
let expectation2 = expectation(description: "Parallel download file 2")

let url = URL(string: "https://thumbs.dreamstime.com/b/bee-flower-27533578.jpg")!
var path1: URL?
var path2: URL?

XCTAssertNotNil(runner)

// Initiate the first download
runner?.downloadFile(
url: url,
fileType: .richPushImage,
session: session,
onComplete: { path in
XCTAssertNotNil(path)
path1 = path
expectation1.fulfill()
}
)

// Initiate the second download in parallel
runner?.downloadFile(
url: url,
fileType: .richPushImage,
session: session,
onComplete: { path in
XCTAssertNotNil(path)
path2 = path
expectation2.fulfill()
}
)

// Wait for both downloads to complete
waitForExpectations(timeout: 20.0) { error in
if let error = error {
XCTFail("Test failed with error: \(error)")
}

// Verify that both paths are not nil and unique
XCTAssertNotNil(path1, "First path should not be nil")
XCTAssertNotNil(path2, "Second path should not be nil")
XCTAssertNotEqual(path1, path2, "Expected unique path for each parallel download")
}
}

override open func tearDown() {
runner = nil
// assert there isn't a memory leak and the runner can be deconstructed.
Expand Down
Loading