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 an issue that causes the fire dialog to show multiple times for the same website after dismissing it #3305

Merged

Conversation

alessandroboron
Copy link
Contributor

@alessandroboron alessandroboron commented Sep 2, 2024

Task/Issue URL: https://app.asana.com/0/1206329551987282/1208180024223015/f
Cc: @SabrinaTardio

Description:
This PR addresses an issue that causes the fire dialog to show multiple times for the same website.

Context

We have logic in place to re-present the dialog if the user reloads the webpage or relaunches the app, allowing them to resume onboarding from where they left off.

We consider the onboarding flow “complete" when the user taps the "High Five" button.

Problem

The issue was that the dialog presentation logic was executed before verifying if the onboarding was complete. As a result, the dialog would reappear every time the website for which it was shown was opened.

Solution

  1. Cancel held information about last visited onboarding website and presented dialog when onboarding completes.
  2. Execute the logic to present the last shown dialog if the onboarding has not been considered completed.

Video Before

Simulator.Screen.Recording.-.iPad.Pro.13-inch.M4.-.2024-09-02.at.16.42.08.mov

Video After

Simulator.Screen.Recording.-.iPad.Pro.13-inch.M4.-.2024-09-02.at.16.49.43.mp4

Steps to test this PR:
Ensure experiment group is returned by adding return VariantIOS(name: "mb", weight: 1, isIncluded: VariantIOS.When.always, features: [.newOnboardingIntro]) in DefaultVariantManager at line 153.

  1. Run the new onboarding flow and wait for the first dialogue to appear on screen.
  2. Tap on “Let’s do it”.
  3. Tap on “Skip".
  4. Go Back to DuckDuckGo App.
  5. The New Tab Page should appear on the screen with a new Dax dialogue suggesting DDG searches.
  6. Tap on one of the searches. E.g. “local weather”.
  7. A Dax dialogue should appear informing the user about the DDG search.
  8. Tap on the "Got it!” button.
  9. The Dax dialogue should update and suggest to the user a list of websites to try.
  10. Tap on one of the websites. E.g. “yahoo.com”.
  11. The Dax dialogue should now inform the user of the trackers that have been blocked.
  12. Tap the “Got It!” Button.
  13. The Fire Button dialog should appear.
  14. Open a new tab.
  15. The end of journey dialog should appear.
  16. Tap “High Five” button.
  17. Type the same website loaded when the fire dialog was shown, e.g. “yahoo.com”
    Expected Result: The fire dialog should not show again.

Definition of Done (Internal Only):

Copy Testing:

  • Use of correct apostrophes in new copy, ie rather than '

Orientation Testing:

  • Portrait
  • Landscape

Device Testing:

  • iPhone SE (1st Gen)
  • iPhone 8
  • iPhone X
  • iPhone 14 Pro
  • iPad

OS Testing:

  • iOS 15
  • iOS 16
  • iOS 17

Theme Testing:

  • Light theme
  • Dark theme

Internal references:

Software Engineering Expectations
Technical Design Template

@@ -316,6 +316,9 @@ final class DaxDialogs: NewTabDialogSpecProvider, ContextualOnboardingLogic {

func dismiss() {
settings.isDismissed = true
// Reset last shown dialog as we don't have to show it anymore.
removeLastShownDaxDialog()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove held information during onboarding.

@@ -513,6 +512,11 @@ final class DaxDialogs: NewTabDialogSpecProvider, ContextualOnboardingLogic {

guard isEnabled, nextHomeScreenMessageOverride == nil else { return nil }

if let lastVisitedOnboardingWebsiteURLPath,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Executes the logic to show last shown dialog after isEnabled check.

@alessandroboron alessandroboron changed the base branch from main to release/7.136.0 September 3, 2024 07:26
Copy link
Contributor

@brindy brindy left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, LGTM

@alessandroboron alessandroboron merged commit 4802df6 into release/7.136.0 Sep 3, 2024
31 checks passed
@alessandroboron alessandroboron deleted the alessandro/fix-repeating-fire-dialog branch September 3, 2024 13:36
samsymons added a commit that referenced this pull request Sep 9, 2024
# By Christopher Brind (4) and others
# Via Alessandro Boron (1) and others
* main: (27 commits)
  Bump C-S-S to 6.14.1 (#3331)
  DuckPlayer Launch Experiment for iOS (#3328)
  defer loading the tab switcher button until view did load (#3326)
  Release 7.136.0-3 (#3324)
  Release PR: Check for the negative attribution case (#3311)
  fix tab switcher crashes (speculative fix) (#3319)
  Onboarding highlights feature flag setup (#3308)
  Release 7.136.0-2 (#3320)
  Attempt to fix dissapearing privacy icon (#3317)
  Fix bookmarks toolbar behaviour with Sync Promo on iOS 15 (#3313)
  Bump BSK with C-S-S to 6.14.0 (#3314)
  ensure no atb or app version sent with pixel (#3315)
  Fix #3298: Add support for Xcode 16 (#3299)
  Fix Keychain Debug view controller segue (#3310)
  Release 7.136.0-1 (#3309)
  Fix an issue that causes the fire dialog to show multiple times for the same website after dismissing it (#3305)
  [DuckPlayer] 28. Open in Youtube -> Youtube App (#3290)
  usage segmentation (#3263)
  Fix wrong URL displayed for auth dialog (#3307)
  iOS Integration of BSK Onboarding (#3282)
  ...

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
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.

2 participants