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

PM-15377: Rolled back review prompt legacy api #1218

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import StoreKit
import SwiftUI

/// A view modifier that requests a review when the view appears.
@available(iOS 16.0, *)
struct ReviewModifier: ViewModifier {
/// The environment key for the request review function.
@Environment(\.requestReview) var requestReview

/// The eligibility for requesting a review.
let isEligible: Bool

/// The closure to execute after requesting a review.
let afterClosure: () -> Void

func body(content: Content) -> some View {
content
.task(id: isEligible) {
if isEligible {
requestReview()
afterClosure()
}

Check warning on line 22 in BitwardenShared/UI/Platform/Application/Appearance/Modifiers/ReviewModifier.swift

View check run for this annotation

Codecov / codecov/patch

BitwardenShared/UI/Platform/Application/Appearance/Modifiers/ReviewModifier.swift#L20-L22

Added lines #L20 - L22 were not covered by tests
}
}
}

/// A view modifier that requests a review via legacy API when the view appears.
struct RequestReviewLegacyModifier: ViewModifier {
/// The eligibility for requesting a review.
let isEligible: Bool

/// The window scene to request a review.
let windowScene: UIWindowScene

/// The closure to execute after requesting a review.
let afterClosure: () -> Void

func body(content: Content) -> some View {
content
.task(id: isEligible) {
if isEligible {
SKStoreReviewController.requestReview(in: windowScene)
afterClosure()
}
}
}

Check warning on line 46 in BitwardenShared/UI/Platform/Application/Appearance/Modifiers/ReviewModifier.swift

View check run for this annotation

Codecov / codecov/patch

BitwardenShared/UI/Platform/Application/Appearance/Modifiers/ReviewModifier.swift#L38-L46

Added lines #L38 - L46 were not covered by tests
}

/// A view extension that requests a review when the view appears.
extension View {
func requestReview(windowScene: UIWindowScene?, isEligible: Bool, afterClosure: @escaping () -> Void) -> some View {
apply { view in
if #available(iOS 16.0, *) {
view.modifier(
ReviewModifier(
isEligible: isEligible,
afterClosure: afterClosure
)
)
} else {
if let windowScene {
view.modifier(
RequestReviewLegacyModifier(
isEligible: isEligible,
windowScene: windowScene,
afterClosure: afterClosure
)
)
}

Check warning on line 69 in BitwardenShared/UI/Platform/Application/Appearance/Modifiers/ReviewModifier.swift

View check run for this annotation

Codecov / codecov/patch

BitwardenShared/UI/Platform/Application/Appearance/Modifiers/ReviewModifier.swift#L61-L69

Added lines #L61 - L69 were not covered by tests
}
}
}
}

/// An error that represents a window scene error.
public enum WindowSceneError: Error, Equatable {
/// The window scene is null.
case nullWindowScene
}
7 changes: 6 additions & 1 deletion BitwardenShared/UI/Vault/Vault/VaultCoordinator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -289,10 +289,15 @@ final class VaultCoordinator: Coordinator, HasStackNavigator {
)
)
let store = Store(processor: processor)
let windowScene = stackNavigator?.rootViewController?.view.window?.windowScene
let view = VaultListView(
store: store,
timeProvider: services.timeProvider
timeProvider: services.timeProvider,
windowScene: windowScene
)
if windowScene == nil {
services.errorReporter.log(error: WindowSceneError.nullWindowScene)
}
stackNavigator?.replace(view, animated: false)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ class VaultCoordinatorTests: BitwardenTestCase {
let action = try XCTUnwrap(stackNavigator.actions.last)
XCTAssertEqual(action.type, .replaced)
XCTAssertTrue(action.view is VaultListView)
XCTAssertEqual(errorReporter.errors.last as? WindowSceneError, WindowSceneError.nullWindowScene)
}

/// `navigate(to:)` with `.lockVault` navigates the user to the login view.
Expand Down
22 changes: 6 additions & 16 deletions BitwardenShared/UI/Vault/Vault/VaultList/VaultListView.swift
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// swiftlint:disable file_length

import BitwardenSdk
import StoreKit
import SwiftUI

// MARK: - SearchableVaultListView
Expand Down Expand Up @@ -283,6 +282,9 @@
/// The `TimeProvider` used to calculate TOTP expiration.
var timeProvider: any TimeProvider

/// The window scene for requesting a review.
var windowScene: UIWindowScene?

var body: some View {
ZStack {
SearchableVaultListView(
Expand Down Expand Up @@ -346,12 +348,6 @@
.task(id: store.state.vaultFilterType) {
await store.perform(.streamVaultList)
}
.task(id: store.state.isEligibleForAppReview) {
if store.state.isEligibleForAppReview {
requestReview()
store.send(.appReviewPromptShown)
}
}
.onAppear {
Task {
await store.perform(.checkAppReviewEligibility)
Expand All @@ -360,6 +356,9 @@
.onDisappear {
store.send(.disappeared)
}
.requestReview(windowScene: windowScene, isEligible: store.state.isEligibleForAppReview) {
store.send(.appReviewPromptShown)
}

Check warning on line 361 in BitwardenShared/UI/Vault/Vault/VaultList/VaultListView.swift

View check run for this annotation

Codecov / codecov/patch

BitwardenShared/UI/Vault/Vault/VaultList/VaultListView.swift#L360-L361

Added lines #L360 - L361 were not covered by tests
}

// MARK: Private properties
Expand All @@ -380,15 +379,6 @@
)
)
}

/// Requests a review of the app.
private func requestReview() {
if #available(iOS 16.0, *) {
Environment(\.requestReview).wrappedValue()
} else {
SKStoreReviewController.requestReview()
}
}
Comment on lines -384 to -391
Copy link
Member

Choose a reason for hiding this comment

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

๐Ÿค” As stated in the previous PR, SKStoreReviewController has been deprecated in iOS 18. Ezimet had some problems with this so and we've also tried using a ViewModifier with .apply() + #available to include or not the view modifier but Ezimet said it was throwing some compiler issues.
@matt-livefront @KatherineInCode do you know any workaround for this so we can use the new environment approach for iOS 16+ and use the old approach for the older iOS version?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if it'd work but I've thought something like:

@available(iOS 16.0, *)
struct ReviewModifier: ViewModifier {
    @Environment(\.requestReview) var requestReview
    
    let afterClosure: () -> Void
    
    func body(content: Content) -> some View {
        content.onAppear {
            requestReview()
            afterClosure()
        }
    }
}

struct OldReviewModifier: ViewModifier {
    let afterClosure: () -> Void
    func body(content: Content) -> some View {
        content.onAppear {
            SKStoreReviewController.requestReview()
            afterClosure()
        }
    }
}

And applying it like:

.apply { view in
    if store.state.isEligibleForAppReview {
        if #available(iOS 16.0, *) {
            view.modifier(ReviewModifier(afterClosure: {
                store.send(.appReviewPromptShown)
            }))
        } else {
            view.modifier(OldReviewModifier(afterClosure: {
                store.send(.appReviewPromptShown)
            }))
        }
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

And another similar way:

@available(iOS 16.0, *)
struct ReviewModifier: ViewModifier {
    @Environment(\.requestReview) var requestReview
    
    let checkEligible: Bool
    let afterClosure: () -> Void
    
    func body(content: Content) -> some View {
        content
            .task(id: checkEligible) {
                if checkEligible {
                    requestReview()
                    afterClosure()
                }
            }
    }
}

struct OldReviewModifier: ViewModifier {
    let checkEligible: Bool
    let afterClosure: () -> Void

    func body(content: Content) -> some View {
        content
            .task(id: checkEligible) {
                if checkEligible {
                    SKStoreReviewController.requestReview()
                    afterClosure()
                }
            }
    }
}

Applying like:

.apply { view in
    if #available(iOS 16.0, *) {
        view.modifier(ReviewModifier(checkEligible: store.state.isEligibleForAppReview, afterClosure: {
            store.send(.appReviewPromptShown)
        }))
    } else {
        view.modifier(OldReviewModifier(checkEligible: store.state.isEligibleForAppReview, afterClosure: {
            store.send(.appReviewPromptShown)
        }))
    }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The second approach worked perfectly.
Thanks @fedemkr

}

// MARK: Previews
Expand Down
Loading