Iterable / swift-sdk

Iterable's iOS SDK. Receive and track pushes to Iterable from your iOS app.
https://iterable.com/
MIT License
86 stars 71 forks source link

[MOB-4431] Clear lastpushpayload if app is not launched via a notification click #603

Closed amanjayshahomnicg closed 1 year ago

roninopf commented 1 year ago

Hi @amanjayshahomnicg! Can we change this to make lastPushPayload just an in memory variable? That way it will match the Android SDK's behavior. I'm also worried that the failing integration tests point to a deeper issue I don't understand.

amanjayshahomnicg commented 1 year ago

Hi @roninopf, the reason I hadn't done it that way is to preserve this advertised feature: https://support.iterable.com/hc/en-us/articles/360035018152-Iterable-s-iOS-SDK-#upgrading-to-6-4-8

amanjayshahomnicg commented 1 year ago

Hi @roninopf I reviewed the logs for the E2E test failures and I suspect it could be because the PR is opened from my fork -> the main repo. This might cause the secrets to not be available to this PR. For reference: https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/#improvements-for-public-repository-forks

roninopf commented 1 year ago

Hi @roninopf, the reason I hadn't done it that way is to preserve this advertised feature: https://support.iterable.com/hc/en-us/articles/360035018152-Iterable-s-iOS-SDK-#upgrading-to-6-4-8

Ah, okay. Please feel free to continue with the change mentioned earlier; I'll handle making sure the documentation is updated to reflect the change on our SDK release.

Hi @roninopf I reviewed the logs for the E2E test failures and I suspect it could be because the PR is opened from my fork -> the main repo. This might cause the secrets to not be available to this PR. For reference: https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/#improvements-for-public-repository-forks

Ah, that's a good catch. Let me see what I can do to get this working for you. Thanks for linking.

And thank you for your work!

devcsomnicg commented 1 year ago

@roninopf Could you start reviewing this PR ?

roninopf commented 1 year ago

Will do and get started on! Thanks!

roninopf commented 1 year ago

@devcsomnicg The changes look good; if you'd like to try to fix some of the unit tests (not the integration tests; these can be run with command + U), please feel free, but otherwise I can do it as they are small changes and I'm already familiar with the unit testing structure.

devcsomnicg commented 1 year ago

ok, we will work on unit tests tomorrow

roninopf commented 1 year ago

@devcsomnicg Looks like there's one last test that just needs to be deleted (NotificationResponsesTests.testSavePushPayload()), but it's otherwise done!

devcsomnicg commented 1 year ago

@roninopf Removed testSavePushPayload. Thanks

roninopf commented 1 year ago

Merging in. Thanks for your work on this!