fix: sample app issues #551

Shahroz16 · 2024-02-22T15:04:54Z

closes: https://linear.app/customerio/issue/MBL-109/findings-from-sample-app-cleanup-and-using-ios-cdp-sdk

more context: https://www.loom.com/share/971cb87f0216460faa0cefbf4fc9591e

solves a couple of issues:

incorrect header
nse image not showing at times

github-actions · 2024-02-22T15:05:13Z

Sample app builds 📱

Below you will find the list of the latest versions of the sample apps. It's recommended to always download the latest builds of the sample apps to accurately test the pull request.

CocoaPods-FCM: Build failed. See CI job logs to determine the issue and try re-building.
APN-UIKit: Build failed. See CI job logs to determine the issue and try re-building.

levibostian

Should this PR be pointing to main?

The PR title says this is a sample app issue, but it's a NSE rich push image issue. I suggest modifying the PR title to indicate that this is a bug fix for rich push images being more reliable.

levibostian · 2024-02-22T15:58:42Z

Sources/MessagingPush/RichPush/MessagingPush+RichPush.swift

+                self.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"
+                    )


Suggested change

self.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"

)

Since we are not running the BQ anymore in the NSE, we can remove this log statement.

levibostian · 2024-02-22T16:02:09Z

Sources/MessagingPush/RichPush/MessagingPush+RichPush.swift

+                    )
+
+                self.logger.info("Customer.io push processing is done!")
+                contentHandler(composedRichPush.notificationContent)


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?

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

levibostian · 2024-02-22T16:02:46Z

Sources/Common/Service/HttpRequestRunner.swift

                return onComplete(nil)
            }

+            let uniqueFileName = UUID().uuidString + "_" + suggestedFileName


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"

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.

levibostian · 2024-02-22T16:04:10Z

Sources/MessagingPush/RichPush/RichPushHttpClient.swift

+        configuration.timeoutIntervalForResource = 30
+        configuration.timeoutIntervalForRequest = 60
+        configuration.httpAdditionalHeaders = [:]


Why were these lines added?

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

levibostian · 2024-02-22T16:16:03Z

Tests/MessagingPush/HttpTest.swift

+    }
+
+    func testDownloadFileCreatesExpectedFile() {
+        if let session = session {


I like this test and think we should get it running in the CI, at least in the short-term.

Assuming you agree, could we modify session to be non-nil? So we don't need this if let condition? I would like us to feel more confident that this test will execute and having this if let makes me think sometimes it might not run.

Also, what do you think about performing a 2nd HTTP request in this test function, downloading the same image URL, and expect that the 2 local file paths are unique?

We could run it in CI, if needed. if let was there to ensure it doesn't run on CI but we can update that.

Shahroz16

Will create a separate PR directed at main with this fix. This was for CDP sample app to be able to pass all tests.

Shahroz16 · 2024-02-22T18:36:45Z

Sources/MessagingPush/RichPush/RichPushHttpClient.swift

+        configuration.timeoutIntervalForResource = 30
+        configuration.timeoutIntervalForRequest = 60
+        configuration.httpAdditionalHeaders = [:]


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

Shahroz16 · 2024-02-22T18:41:22Z

Sources/Common/Service/HttpRequestRunner.swift

                return onComplete(nil)
            }

+            let uniqueFileName = UUID().uuidString + "_" + suggestedFileName


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.

Shahroz16 · 2024-02-22T18:44:41Z

Sources/MessagingPush/RichPush/MessagingPush+RichPush.swift

+                    )
+
+                self.logger.info("Customer.io push processing is done!")
+                contentHandler(composedRichPush.notificationContent)


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

Shahroz16 · 2024-02-22T18:46:56Z

Tests/MessagingPush/HttpTest.swift

+    }
+
+    func testDownloadFileCreatesExpectedFile() {
+        if let session = session {


We could run it in CI, if needed. if let was there to ensure it doesn't run on CI but we can update that.

## [3.0.0](2.12.5...3.0.0) (2024-03-20) ### ⚠ BREAKING CHANGES * iOS as a source for Data Pipelines (#659) ### Features * iOS as a source for Data Pipelines ([#659](#659)) ([0a68373](0a68373)) * migration module to cater to all migration tasks ([#530](#530)) ([2feb1d4](2feb1d4)) ### Bug Fixes * add attributes to properties ([#649](#649)) ([4b02e92](4b02e92)) * all sdk modules can only be initialized once ([ae46c7f](ae46c7f)) * app crash on identify method ([#458](#458)) ([13e9862](13e9862)) * compilation for test ([f14b773](f14b773)) * compilation issue ([420a61e](420a61e)) * eventbus handler ref ([#469](#469)) ([8c8ef91](8c8ef91)) * journey id in migration payload ([#653](#653)) ([3b649c9](3b649c9)) * prevent duplicate automatic screenview events from being tracked ([fea9ec5](fea9ec5)) * pushEventHandler test ([dc80fc2](dc80fc2)) * remove occurrence of autoTrackDeviceAttributes from all push modules ([#505](#505)) ([8dc6507](8dc6507)) * removed last_used from properties ([#477](#477)) ([b0b9631](b0b9631)) * sample app issues ([#551](#551)) ([05544b3](05544b3)) * use git commit instead of git branch for segment dependency ([d245015](d245015))

Shahroz16 added 10 commits February 20, 2024 11:16

refactored post initialize to be independent of digraph

f62ddc6

fix compilation

879519a

pr suggestion

cc6dece

Merge branch 'main-replica-for-cdp' into shahroz/mbl-123

2b049ce

Merge branch 'main-replica-for-cdp' into shahroz/mbl-123

5dbff1b

Merge branch 'main-replica-for-cdp' into shahroz/mbl-123

539ea5a

fixed auth header

acc197e

fix displaying image issue

5e8573b

update

fd28e8e

added nse image not showing fix and test

ff17022

Shahroz16 requested a review from a team February 22, 2024 15:05

levibostian approved these changes Feb 22, 2024

View reviewed changes

Shahroz16 commented Feb 22, 2024

View reviewed changes

Shahroz16 added 3 commits February 22, 2024 23:48

remove log

b3bb9cf

updated test to run on ci

19fb72e

added comment

3bb7da8

Shahroz16 merged commit 05544b3 into main-replica-for-cdp Feb 22, 2024
3 of 8 checks passed

Shahroz16 deleted the shahroz/mbl-123 branch February 22, 2024 21:00

Shahroz16 mentioned this pull request Feb 23, 2024

fix: rich push image downloading path conflict when its already downloaded #561

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: sample app issues #551

fix: sample app issues #551

Shahroz16 commented Feb 22, 2024

github-actions bot commented Feb 22, 2024 •

edited

Loading

levibostian left a comment

levibostian Feb 22, 2024

levibostian Feb 22, 2024

Shahroz16 Feb 22, 2024

levibostian Feb 22, 2024

Shahroz16 Feb 22, 2024

levibostian Feb 22, 2024

Shahroz16 Feb 22, 2024

levibostian Feb 22, 2024

Shahroz16 Feb 22, 2024

Shahroz16 left a comment

Shahroz16 Feb 22, 2024

Shahroz16 Feb 22, 2024

Shahroz16 Feb 22, 2024

Shahroz16 Feb 22, 2024

fix: sample app issues #551

fix: sample app issues #551

Conversation

Shahroz16 commented Feb 22, 2024

github-actions bot commented Feb 22, 2024 • edited Loading

Sample app builds 📱

levibostian 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

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

Shahroz16 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

Choose a reason for hiding this comment

github-actions bot commented Feb 22, 2024 •

edited

Loading