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

[NTV-647] Braze In-App Notification Fix #1791

Merged
merged 3 commits into from Feb 14, 2023

Conversation

msadoon
Copy link
Contributor

@msadoon msadoon commented Feb 10, 2023

📲 What

We noticed in-app notifications were broken (or not appearing) in release 5.6.1.
Needed to fix those.

🤔 Why

Strangely two different instances of SEGAppboyIntegrationFactory were being created when calling instance() on that class. Somehow this caused in-app notifications not to show up.

🛠 How

Created one instance of SEGAppboyIntegrationFactory inside the Analytics configuredClient function and pass it back out to be configure in the AppDelegate. (Previously passing it into the function).

Also it seems like segment-appboy-ios is pinning Appboy_iOS_SDK to 4.5.1 now, so let's use it's dependency instead of doing the pinning on our own.

✅ Acceptance criteria

  • Test that in-app notifications show on staging.
  • Test push notifications deeplink on staging.
  • Test in-app notification deeplinks on staging.

⏰ TODO

  • Validate that Apply Pay crash scenario doesn't occur without manually pinning Appboy_iOS_SDK.
  • Check to see if 5.6.0 had in-app notifications deeplink correctly and re-test with this code.

…n is deeplinking from within an in-app notification.
@msadoon msadoon self-assigned this Feb 10, 2023
@msadoon msadoon added this to the release-5.7.0 milestone Feb 10, 2023
@msadoon msadoon added the WIP label Feb 10, 2023
@codecov
Copy link

codecov bot commented Feb 10, 2023

Codecov Report

Merging #1791 (63d35ac) into main (ae5792e) will increase coverage by 0.00%.
The diff coverage is 68.00%.

@@           Coverage Diff           @@
##             main    #1791   +/-   ##
=======================================
  Coverage   85.24%   85.24%           
=======================================
  Files        1277     1277           
  Lines      116662   116682   +20     
  Branches    30795    30801    +6     
=======================================
+ Hits        99443    99462   +19     
  Misses      16146    16146           
- Partials     1073     1074    +1     
Impacted Files Coverage Δ
Library/Tracking/Segment.swift 0.00% <0.00%> (ø)
Kickstarter-iOS/AppDelegateViewModelTests.swift 98.92% <91.66%> (-0.05%) ⬇️
Kickstarter-iOS/AppDelegateViewModel.swift 91.63% <100.00%> (+0.06%) ⬆️
Library/Navigation.swift 85.85% <0.00%> (+0.64%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@msadoon msadoon added needs review and removed WIP labels Feb 13, 2023
@msadoon msadoon marked this pull request as ready for review February 13, 2023 18:10
Copy link
Contributor

@scottkicks scottkicks left a comment

Choose a reason for hiding this comment

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

code LGTM. Odd that the appBoyInstance is changing.

I still can't test app notifications on my physical device so I'll defer to you to make sure it is working.

@msadoon
Copy link
Contributor Author

msadoon commented Feb 14, 2023

code LGTM. Odd that the appBoyInstance is changing.

I still can't test app notifications on my physical device so I'll defer to you to make sure it is working.

They do work I tested them yesterday.

@msadoon msadoon merged commit 7586740 into main Feb 14, 2023
@msadoon msadoon deleted the ntv-647/braze-pn-in-app-notification-fix branch February 14, 2023 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants