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.

Sign up for GitHub

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
+92 −17
Open

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3ca2abb
rolled back review prompt to legacy api
ezimet-livefront Dec 18, 2024
45755df
address feedback
ezimet-livefront Dec 18, 2024
c234380
reindent the code
ezimet-livefront Dec 18, 2024
0918818
fix lint warning
ezimet-livefront Dec 18, 2024
5f51298
address feedbacks
ezimet-livefront Dec 21, 2024
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
44 changes: 44 additions & 0 deletions BitwardenShared/UI/Platform/Application/Appearance/Modifiers/ReviewModifier.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
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

ezimet-livefront marked this conversation as resolved.
Show resolved Hide resolved
/// 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 closure to execute after requesting a review.
let afterClosure: () -> Void

func body(content: Content) -> some View {
content
.task(id: isEligible) {
if isEligible {
SKStoreReviewController.requestReview()
ezimet-livefront marked this conversation as resolved.
Show resolved Hide resolved
afterClosure()
}
}
}

Check warning on line 43 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#L35-L43 

Added lines #L35 - L43 were not covered by tests
}
37 changes: 21 additions & 16 deletions BitwardenShared/UI/Vault/Vault/VaultList/VaultListView.swift
View file
Open in desktop
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 @@ -346,12 +345,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 +353,27 @@
.onDisappear {
store.send(.disappeared)
}
.apply { view in
if #available(iOS 16.0, *) {
view.modifier(
ReviewModifier(
isEligible: store.state.isEligibleForAppReview,
afterClosure: {
store.send(.appReviewPromptShown)
}

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

View check run for this annotation

Codecov / codecov/patch

BitwardenShared/UI/Vault/Vault/VaultList/VaultListView.swift#L362-L363 

Added lines #L362 - L363 were not covered by tests
)
)
} else {
view.modifier(
RequestReviewLegacyModifier(
isEligible: store.state.isEligibleForAppReview,
afterClosure: {
store.send(.appReviewPromptShown)
}
)
)
}

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

View check run for this annotation

Codecov / codecov/patch

BitwardenShared/UI/Vault/Vault/VaultList/VaultListView.swift#L367-L375 

Added lines #L367 - L375 were not covered by tests
}
ezimet-livefront marked this conversation as resolved.
Show resolved Hide resolved
}

// MARK: Private properties
Expand All @@ -380,15 +394,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