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

[NT-1055] LandingPage deeplink handling bug fix #1146

Merged
merged 5 commits into from
Apr 8, 2020

Conversation

Scollaco
Copy link
Contributor

@Scollaco Scollaco commented Apr 6, 2020

πŸ“² What

Fixes a bug where a LandingPage is presented and immediately dismissed if the app is launched through deeplink.

πŸ€” Why

It's a bug and we don't like bugs.

πŸ›  How

  • This was caused by a race condition between the goToLandingPage and the deeplinkActivated. So in order to solve this, a take(until:) function was added to prevent the deeplinkActivated from emitting if goToLandingPage already emitted.

πŸ‘€ See

Before πŸ› After πŸ¦‹
deeplink_bug deeplink_fix

βœ… Acceptance criteria

In order to test this, follow the steps:

  1. Set Launch setting to Wait for executable to be launched
  2. On line 1116, return true immediately after the open bracket:
    private func shouldGoToLandingPage() -> Bool { return true
    
3. Add a 2 sec delay to the `openUrl` property:

let openUrl = self.applicationOpenUrlProperty.signal
.delay(2, on: AppEnvironment.current.scheduler)
.skipNil()

4. Build the app on a device in the release configuration
5. Click the link to open the app through deeplink. I tested using [this link]( https://www.kickstarter.com/projects/chelsea-punk/chelsea-punk-band-the-final-album))

- [ ] The landing page should be presented on the first time.
- [ ] Close the app and tap the link again. The app should show the Project Page and the Landing Page should **not** be presented.

self.vm.inputs.applicationDidFinishLaunching(application: UIApplication.shared, launchOptions: nil)
self.vm.inputs.didUpdateOptimizelyClient(MockOptimizelyClient())

_ = self.vm.inputs.applicationOpenUrl(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we actually run this test with a project deeplink? The output that was being triggered during the bug was presentViewController, which first dismisses any previously presented VC.

Copy link
Contributor

@ifbarrera ifbarrera left a comment

Choose a reason for hiding this comment

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

Nice!

@Scollaco Scollaco merged commit 3506287 into master Apr 8, 2020
@Scollaco Scollaco deleted the NT-1055-landing-page-deeplink-fix branch April 8, 2020 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants