Skip to content

Conversation

TimNowaczynski
Copy link

This is currently a WIP because tests are still missing :)

Context

Approach

Other

Before merging

Checklist

  • I performed some relevant testing on a real device and/or simulator
  • I wrote Unit Tests that confirm the expected behaviour
  • I updated only the english localization source files if needed
  • I added the // Ecosia: helper comments where needed
  • I made sure that any change to the Analytics events included in PR won't alter current analytics (e.g. new users, upgrading users)
  • I included documentation updates to the coding standards or Confluence doc, when needed

Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Navigation Context

The PR introduces new method calls that pass a navigation controller (e.g. getLastSection(navigationController) and getHelpAction(navigationController)). Please ensure that this change handles nil or unexpected values correctly and remains consistent throughout the UI flow.

    getLastSection(navigationController)
])
Alert Cancel Option

The new action sheet in getHelpAction presents options for "Yes" and "No" but does not include a cancel action. Consider adding a cancel option to allow users to dismiss the prompt gracefully.

private func getHelpAction(_ viewController: UIViewController?) -> PhotonRowActions {
    return SingleActionViewModel(title: .LegacyAppMenu.Help,
                                 iconString: StandardImageIdentifiers.Large.helpCircle) { _ in

        let rateAction = UIAlertAction(title: .localized("Yes"), style:. default) { _ in
            self.delegate?.openURLInCurrentTab(Environment.current.urlProvider.storePage)
        }

        let helpPageAction = UIAlertAction(title: .localized("No"), style: .default) { _ in
            self.delegate?.openURLInCurrentTab(Environment.current.urlProvider.faq)
            // Analytics.shared.menuClick(.help)
        }

        let alertController = UIAlertController.init(title: nil, message: .localized("Do you enjoy Ecosia?"), preferredStyle: .actionSheet)
        alertController.addAction(rateAction)
        alertController.addAction(helpPageAction)

        viewController?.present(alertController, animated: true)
    }.items
Forced URL Unwrap

The storePage property force unwraps URLs created from string literals. Ensure that these URLs are validated or consider handling potential errors to prevent runtime crashes.

public var storePage: URL {
    switch Language.current {
        case .de:
            return URL(string: "https://apps.apple.com/de/app/ecosia/id1474845552")!
        case .fr:
            return URL(string: "https://apps.apple.com/fr/app/ecosia/id1474845552")!
        default:
            return URL(string: "https://apps.apple.com/us/app/ecosia/id1474845552")!
    }
}

@TimNowaczynski TimNowaczynski changed the title [MOB-1589] MOB-1589: App Store rating prompt improvements [MOB-1589] App Store rating prompt improvements Apr 16, 2025
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Safeguard view presentation

Add a check to ensure that the viewController is non-nil before presenting the
alert.

firefox-ios/Client/Frontend/Browser/MainMenuActionHelper.swift [597]

-viewController?.present(alertController, animated: true)
+guard let viewController = viewController else {
+    // Optionally log or handle the nil case
+    return
+}
+viewController.present(alertController, animated: true)
Suggestion importance[1-10]: 5

__

Why: The suggestion improves safety by checking for a nil viewController before presenting the alert. However, the change is stylistic and non-critical, and may alter the behavior if nil is an expected case.

Low
General
Re-enable analytics tracking

Ensure that analytics tracking is performed by enabling the analytics event call for
the 'No' option.

firefox-ios/Client/Frontend/Browser/MainMenuActionHelper.swift [588-591]

 let helpPageAction = UIAlertAction(title: .localized("No"), style: .default) { _ in
     self.delegate?.openURLInCurrentTab(Environment.current.urlProvider.faq)
-    // Analytics.shared.menuClick(.help)
+    Analytics.shared.menuClick(.help)
 }
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly reinstates analytics tracking by uncommenting the relevant code, restoring behavior seen in the old hunk. While useful, it addresses a minor issue, hence the moderate score.

Low

@TimNowaczynski TimNowaczynski force-pushed the MOB-1589-rating-prompt-improvements branch from 265c904 to 0ed722b Compare May 7, 2025 08:47
@TimNowaczynski TimNowaczynski requested a review from a team May 7, 2025 08:48
@TimNowaczynski TimNowaczynski force-pushed the MOB-1589-rating-prompt-improvements branch from 03075e3 to 6aca4cc Compare May 12, 2025 08:51
@TimNowaczynski
Copy link
Author

Ah, I put it into the wrong place :face_palm: Working on porting things to the rate action,

@TimNowaczynski TimNowaczynski removed the request for review from a team May 12, 2025 12:09
@TimNowaczynski
Copy link
Author

Hello! I realized that I managed to put the dialog in the wrong place for some reason, I'll try to move it to the correct place 😓

@TimNowaczynski TimNowaczynski force-pushed the MOB-1589-rating-prompt-improvements branch from 6aca4cc to b4db845 Compare June 4, 2025 10:39
@d4r1091
Copy link
Member

d4r1091 commented Jun 24, 2025

Hey @TimNowaczynski 👋 - do you need any help to move this implementation further?

[MOB-1589] MOB-1589: App Store rating prompt improvements
@TimNowaczynski TimNowaczynski force-pushed the MOB-1589-rating-prompt-improvements branch from 3c7d0a5 to 3234674 Compare August 6, 2025 12:49
Copy link
Member

@d4r1091 d4r1091 left a comment

Choose a reason for hiding this comment

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

Hey @TimNowaczynski 👋
added a few notes as summary/reminder of the call we had earlier 😉 .
Happy 🍎 coding!

Comment on lines +47 to +51

enum activityType: String {
case openURL = "org.mozilla.ios.Firefox.newTab"
}

Copy link
Member

Choose a reason for hiding this comment

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

We discussed on removing these 👍

// MARK: Ecosia

private func interceptNegativeReviews() {
let rateAction = UIAlertAction(title: .localized("Yes"), style: .default) { _ in
Copy link
Member

Choose a reason for hiding this comment

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

We should reference our Ecosia's String enum cases here instead of the raw string

RatingPromptManager.goToAppStoreReview()
}

let helpAction = UIAlertAction(title: .localized("No"), style: .destructive) { _ in
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

}

let helpAction = UIAlertAction(title: .localized("No"), style: .destructive) { _ in
let helpActivity = NSUserActivity(activityType: activityType.openURL.rawValue)
Copy link
Member

Choose a reason for hiding this comment

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

No activityType but instead open the Help Page. For reference see how we open it from the "Help Center" item in settings

Comment on lines +2832 to +2846
public static let SettingsRatingPromptTitle = MZLocalizedString(
key: "Settings.RatingPrompt.Title",
tableName: "Settings",
value: "Do you enjoy Ecosia?",
comment: "This is what we prompt our users if they hit the rate button from within the settings")
public static let SettingsRatingPromptYes = MZLocalizedString(
key: "Settings.RatingPrompt.Yes",
tableName: "Settings",
value: "Yes",
comment: "When hit will redirect the user to rate the app on the app store")
public static let SettingsRatingPromptNo = MZLocalizedString(
key: "Settings.RatingPrompt.No",
tableName: "Settings",
value: "No",
comment: "Used to indicate the user does not like ecosia and will redirect them to our help pages")
Copy link
Member

Choose a reason for hiding this comment

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

These will be removed

Comment on lines +160 to +167
switch Language.current {
case .de:
return URL(string: "https://apps.apple.com/de/app/ecosia/id1474845552")!
case .fr:
return URL(string: "https://apps.apple.com/fr/app/ecosia/id1474845552")!
default:
return URL(string: "https://apps.apple.com/us/app/ecosia/id1474845552")!
}
Copy link
Member

Choose a reason for hiding this comment

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

Will be replaced as you shown me in the video call

Comment on lines +233 to +235
"Do you enjoy Ecosia?" = "Do you enjoy Ecosia?";
"Yes" = "Yes";
"No" = "No";
Copy link
Member

Choose a reason for hiding this comment

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

No need of these dupes, we can remove.

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