Bugfix FXIOS-10595 Settings screen: "Learn More" links in Send Usage …

…Feedback & Studies descriptions are not tappable (#23212) Fix failing Learn More navigation for Settings by repairing delegate chain on initialization.
mozilla-mobile · Nov 19, 2024 · db99b05 · db99b05
1 parent 8334ccd
commit db99b05
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 11 deletions.
diff --git a/firefox-ios/Client/Coordinators/SettingsCoordinator.swift b/firefox-ios/Client/Coordinators/SettingsCoordinator.swift
@@ -23,7 +23,7 @@ class SettingsCoordinator: BaseCoordinator,
                            AboutSettingsDelegate,
                            ParentCoordinatorDelegate,
                            QRCodeNavigationHandler {
-    var settingsViewController: AppSettingsScreen
+    var settingsViewController: AppSettingsScreen?
     private let wallpaperManager: WallpaperManagerInterface
     private let profile: Profile
     private let tabManager: TabManager
@@ -40,13 +40,15 @@ class SettingsCoordinator: BaseCoordinator,
         self.profile = profile
         self.tabManager = tabManager
         self.themeManager = themeManager
-        self.settingsViewController = AppSettingsTableViewController(with: profile,
-                                                                     and: tabManager)
         super.init(router: router)
 
+        // It's important we initialize AppSettingsTableViewController with a settingsDelegate and parentCoordinator
+        let settingsViewController = AppSettingsTableViewController(with: profile,
+                                                                    and: tabManager,
+                                                                    settingsDelegate: self,
+                                                                    parentCoordinator: self)
+        self.settingsViewController = settingsViewController
         router.setRootViewController(settingsViewController)
-        settingsViewController.settingsDelegate = self
-        settingsViewController.parentCoordinator = self
     }
 
     func start(with settingsSection: Route.SettingsSection) {
@@ -55,7 +57,8 @@ class SettingsCoordinator: BaseCoordinator,
         if let viewController = getSettingsViewController(settingsSection: settingsSection) {
             router.push(viewController)
         } else {
-            settingsViewController.handle(route: settingsSection)
+            assert(settingsViewController != nil)
+            settingsViewController?.handle(route: settingsSection)
         }
     }
 
@@ -406,7 +409,8 @@ class SettingsCoordinator: BaseCoordinator,
     // MARK: - AboutSettingsDelegate
 
     func pressedRateApp() {
-        settingsViewController.handle(route: .rateApp)
+        assert(settingsViewController != nil)
+        settingsViewController?.handle(route: .rateApp)
     }
 
     func pressedLicense(url: URL, title: NSAttributedString) {

diff --git a/firefox-ios/Client/Frontend/Settings/Main/AppSettingsTableViewController.swift b/firefox-ios/Client/Frontend/Settings/Main/AppSettingsTableViewController.swift
@@ -59,7 +59,8 @@ class AppSettingsTableViewController: SettingsTableViewController,
     // MARK: - Initializers
     init(with profile: Profile,
          and tabManager: TabManager,
-         delegate: SettingsDelegate? = nil,
+         settingsDelegate: SettingsDelegate,
+         parentCoordinator: SettingsFlowDelegate,
          appAuthenticator: AppAuthenticationProtocol = AppAuthenticator(),
          applicationHelper: ApplicationHelper = DefaultApplicationHelper(),
          logger: Logger = DefaultLogger.shared) {
@@ -70,7 +71,8 @@ class AppSettingsTableViewController: SettingsTableViewController,
         super.init(windowUUID: tabManager.windowUUID)
         self.profile = profile
         self.tabManager = tabManager
-        self.settingsDelegate = delegate
+        self.settingsDelegate = settingsDelegate
+        self.parentCoordinator = parentCoordinator
         setupNavigationBar()
         setupDataSettings()
     }

diff --git a/firefox-ios/firefox-ios-tests/Tests/ClientTests/Coordinators/SettingsCoordinatorTests.swift b/firefox-ios/firefox-ios-tests/Tests/ClientTests/Coordinators/SettingsCoordinatorTests.swift
@@ -210,8 +210,8 @@ final class SettingsCoordinatorTests: XCTestCase {
     func testDelegatesAreSet() {
         let subject = createSubject()
 
-        XCTAssertNotNil(subject.settingsViewController.settingsDelegate)
-        XCTAssertNotNil(subject.settingsViewController.parentCoordinator)
+        XCTAssertNotNil(subject.settingsViewController?.settingsDelegate)
+        XCTAssertNotNil(subject.settingsViewController?.parentCoordinator)
     }
 
     func testHandleRouteCalled_whenCreditCardRouteIsSet() {

diff --git a/...os/firefox-ios-tests/Tests/ClientTests/Settings/AppSettingsTableViewControllerTests.swift b/...os/firefox-ios-tests/Tests/ClientTests/Settings/AppSettingsTableViewControllerTests.swift
@@ -12,6 +12,8 @@ class AppSettingsTableViewControllerTests: XCTestCase {
     private var appAuthenticator: MockAppAuthenticator!
     private var delegate: MockSettingsFlowDelegate!
     private var applicationHelper: MockApplicationHelper!
+    private var mockSettingsDelegate: MockSettingsDelegate!
+    private var mockParentCoordinator: MockSettingsFlowDelegate!
 
     override func setUp() {
         super.setUp()
@@ -23,6 +25,8 @@ class AppSettingsTableViewControllerTests: XCTestCase {
         self.appAuthenticator = MockAppAuthenticator()
         self.delegate = MockSettingsFlowDelegate()
         self.applicationHelper = MockApplicationHelper()
+        self.mockSettingsDelegate = MockSettingsDelegate()
+        self.mockParentCoordinator = MockSettingsFlowDelegate()
     }
 
     override func tearDown() {
@@ -117,10 +121,21 @@ class AppSettingsTableViewControllerTests: XCTestCase {
         XCTAssertEqual(delegate.showExperimentsCalled, 1)
     }
 
+    func testDelegatesAreSet() {
+        let subject = createSubject()
+
+        // NOTE: The subject holds a weak reference to these delegates, so we have to store them for the length of the test
+        // duration, or else they will deallocate before the following assertion checks.
+        XCTAssertNotNil(subject.settingsDelegate)
+        XCTAssertNotNil(subject.parentCoordinator)
+    }
+
     // MARK: - Helper
     private func createSubject() -> AppSettingsTableViewController {
         let subject = AppSettingsTableViewController(with: profile,
                                                      and: tabManager,
+                                                     settingsDelegate: mockSettingsDelegate,
+                                                     parentCoordinator: mockParentCoordinator,
                                                      appAuthenticator: appAuthenticator,
                                                      applicationHelper: applicationHelper)
         trackForMemoryLeaks(subject)