Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 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
4 changes: 4 additions & 0 deletions Storage/Storage/Model/FeedbackType.swift
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,8 @@ public enum FeedbackType: String, Codable {
/// Identifier for the orders creation feedback survey
///
case ordersCreation

/// Identifier for the In-Person Payments feedback survey
///
case IPPFeedback
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor thing here, if the enum type name already contains "Feedback", I think we can omit it here: case IPP, as in the case of the ordersCreation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed on 83835c4

}
2 changes: 2 additions & 0 deletions WooCommerce/Classes/System/WooConstants.swift
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,10 @@ extension WooConstants {
///
#if DEBUG
case inAppFeedback = "https://automattic.survey.fm/woo-app-general-feedback-test-survey"
case IPPFeedback = "https://automattic.survey.fm/woo-app-ipp-in-app-feedback-testing"
#else
case inAppFeedback = "https://automattic.survey.fm/woo-app-general-feedback-user-survey"
case IPPFeedback = "https://automattic.survey.fm/woo-app-ipp-in-app-feedback-testing"
#endif

/// URL for the products feedback survey
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,8 @@ private extension OrderListViewController {
self.setErrorTopBanner()
case .orderCreation:
self.setOrderCreationTopBanner()
case .IPPFeedback:
self.setIPPFeedbackTopBanner()
}
}
.store(in: &cancellables)
Expand Down Expand Up @@ -783,6 +785,44 @@ private extension OrderListViewController {
})
showTopBannerView()
}

/// Sets the `topBannerView` property to an IPP feedback banner.
///
func setIPPFeedbackTopBanner() {
topBannerView = createIPPFeedbackTopBanner()
showTopBannerView()
}

private func createIPPFeedbackTopBanner() -> TopBannerView {
let shareIPPFeedbackAction = TopBannerViewModel.ActionButton(title: Localization.shareFeedbackButton, action: { _ in
self.displayIPPFeedbackBannerSurvey()
})

let viewModel = TopBannerViewModel(
title: Localization.feedbackBannerTitle,
infoText: Localization.feedbackBannerContent,
icon: UIImage.gridicon(.comment),
isExpanded: true,
topButton: .dismiss(handler: {
self.dismissIPPFeedbackBannerSurvey()
}),
actionButtons: [shareIPPFeedbackAction]
)
let topBannerView = TopBannerView(viewModel: viewModel)
topBannerView.translatesAutoresizingMaskIntoConstraints = false
return topBannerView
}

private func displayIPPFeedbackBannerSurvey() {
// TODO: Survey will change based on conditions
let surveyNavigation = SurveyCoordinatingController(survey: .IPPFeedback)
self.present(surveyNavigation, animated: true, completion: nil)
}

private func dismissIPPFeedbackBannerSurvey() {
// TODO: Dismissal logic to not show the banner again for X days/never
viewModel.dismissIPPFeedbackBanner()
}
}

// MARK: - Constants
Expand All @@ -801,6 +841,18 @@ private extension OrderListViewController {

static let markCompleted = NSLocalizedString("Mark Completed", comment: "Title for the swipe order action to mark it as completed")

static let feedbackBannerTitle = NSLocalizedString("Let us know what you think",
comment: "Title of the In-Person Payments feedback banner in the Orders tab"
)

static let feedbackBannerContent = NSLocalizedString("Rate your In-Person Payment experience.",
comment: "Content of the In-Person Payments feedback banner in the Orders tab"
)

static let shareFeedbackButton = NSLocalizedString("Share feedback",
comment: "Title of the feedback action button on the In-Person Payments feedback banner"
)

static func markCompletedNoticeTitle(orderID: Int64) -> String {
let format = NSLocalizedString(
"Order #%1$d marked as completed",
Expand Down
44 changes: 42 additions & 2 deletions WooCommerce/Classes/ViewRelated/Orders/OrderListViewModel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,11 @@ final class OrderListViewModel {
///
@Published var hideOrdersBanners: Bool = true

/// If true, no IPP feedback banner will be shown as the user has told us that they are not interested in this information.
/// It is persisted through app sessions.
///
@Published var hideIPPFeedbackBanner: Bool = false

init(siteID: Int64,
stores: StoresManager = ServiceLocator.stores,
storageManager: StorageManagerType = ServiceLocator.storageManager,
Expand All @@ -143,6 +148,12 @@ final class OrderListViewModel {
self.pushNotificationsManager = pushNotificationsManager
self.notificationCenter = notificationCenter
self.filters = filters

if ServiceLocator.featureFlagService.isFeatureFlagEnabled(.IPPInAppFeedbackBanner) {
// TODO: Remove. Temporarily set to false so we can debug the IPPFeedback banner easily.
hideOrdersBanners = false
Copy link
Contributor

Choose a reason for hiding this comment

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

I find a bit dangerous to keep this in the PR, even if we add the TODO comment. If it goes to trunk it would disable the banner, which is not what we want

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, that makes sense. I'll experiment with the @Published properties to find a better way to display/hide these.

I see we're using Publishers.CombineLatest(:) so the view model internal state figures out which banner should assign to the view, we could pass the $hideIPPFeedbackBanner binding as well there and use CombineLatest3(:) to figure this one out.

topBanner = .IPPFeedback
}
}

deinit {
Expand All @@ -165,10 +176,12 @@ final class OrderListViewModel {

observeForegroundRemoteNotifications()
bindTopBannerState()
loadOrdersBannerVisibility()

if ServiceLocator.featureFlagService.isFeatureFlagEnabled(.IPPInAppFeedbackBanner) {
loadIPPFeedbackBannerVisibility()
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a question here, don't we load the orders banner if the IPP Feedback is enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, after thinking further about it I was unsure how to fully deal with the Order Creation banner if IPP is enabled.

As per discussion at p1673883711765819?thread_ts=1673876649.359489&cid=C025A8VV728-slack-C025A8VV728 we're opting for:

  • Prioritise showing the IPP survey. Show the Order Creation banner only when the IPP survey is not displayed.
  • If IPP survey is dismissed, ideally we shouldn't show Order Creation immediately, unless that's the default behavior and changing it involves a certain level of complexity (as we're targeting to release this survey as soon as possible, then iterate later if needed).

fetchIPPTransactions()
} else {
loadOrdersBannerVisibility()
}
}

Expand All @@ -185,6 +198,15 @@ final class OrderListViewModel {
stores.dispatch(action)
}

func dismissIPPFeedbackBanner() {
print("dismiss tapped")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we delete these prints here, as this is likely going to trunk?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely, removed on ff7a3ee

let action = AppSettingsAction.updateFeedbackStatus(type: .IPPFeedback, status: .dismissed, onCompletion: { _ in
self.hideIPPFeedbackBanner = true
print("hideIPPFeedbackBanner? \(self.hideIPPFeedbackBanner)")
})
stores.dispatch(action)
}

/// Starts the snapshotsProvider, logging any errors.
private func startReceivingSnapshots() {
do {
Expand All @@ -208,6 +230,19 @@ final class OrderListViewModel {
stores.dispatch(action)
}

private func loadIPPFeedbackBannerVisibility() {
let action = AppSettingsAction.loadFeedbackVisibility(type: .IPPFeedback) { [weak self] result in
switch result {
case .success(let visible):
self?.hideIPPFeedbackBanner = !visible
case .failure(let error):
self?.hideIPPFeedbackBanner = true
ServiceLocator.crashLogging.logError(error)
}
}
stores.dispatch(action)
}

@objc private func handleAppDeactivation() {
isAppActive = false
}
Expand Down Expand Up @@ -376,7 +411,11 @@ extension OrderListViewModel {
return .none
}

return .orderCreation
if ServiceLocator.featureFlagService.isFeatureFlagEnabled(.IPPInAppFeedbackBanner) {
return .IPPFeedback
} else {
return .orderCreation
}
}
.assign(to: &$topBanner)
}
Expand Down Expand Up @@ -419,6 +458,7 @@ extension OrderListViewModel {
enum TopBanner {
case error
case orderCreation
case IPPFeedback
case none
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ extension SurveyViewController {
case addOnsI1
case orderCreation
case couponManagement
case IPPFeedback

fileprivate var url: URL {
switch self {
Expand Down Expand Up @@ -102,22 +103,27 @@ extension SurveyViewController {
.asURL()
.tagPlatform("ios")
.tagAppVersion(Bundle.main.bundleVersion())
case .IPPFeedback:
return WooConstants.URLs.IPPFeedback
.asURL()
.tagPlatform("ios")
.tagAppVersion(Bundle.main.bundleVersion())
}
}

fileprivate var title: String {
switch self {
case .inAppFeedback:
return Localization.title
case .productsFeedback, .shippingLabelsRelease3Feedback, .addOnsI1, .orderCreation, .couponManagement:
case .productsFeedback, .shippingLabelsRelease3Feedback, .addOnsI1, .orderCreation, .couponManagement, .IPPFeedback:
return Localization.giveFeedback
}
}

/// The corresponding `FeedbackContext` for event tracking purposes.
var feedbackContextForEvents: WooAnalyticsEvent.FeedbackContext {
switch self {
case .inAppFeedback:
case .inAppFeedback, .IPPFeedback:
return .general
case .productsFeedback:
return .productsGeneral
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ final class OrderListViewModelTests: XCTestCase {
XCTAssertFalse(resynchronizeRequested)
}

func test_when_having_no_error__and_orders_banner_should_not_be_shown_shows_nothing() {
func test_when_having_no_error_and_orders_banner_should_not_be_shown_shows_nothing() {
// Given
let viewModel = OrderListViewModel(siteID: siteID, stores: stores, filters: nil)
stores.whenReceivingAction(ofType: AppSettingsAction.self) { action in
Expand All @@ -255,8 +255,14 @@ final class OrderListViewModelTests: XCTestCase {
viewModel.activate()

// Then
waitUntil {
viewModel.topBanner == .none
if ServiceLocator.featureFlagService.isFeatureFlagEnabled(.IPPInAppFeedbackBanner) {
waitUntil {
viewModel.topBanner == .IPPFeedback
}
} else {
waitUntil {
viewModel.topBanner == .none
}
}
}

Expand Down Expand Up @@ -298,8 +304,14 @@ final class OrderListViewModelTests: XCTestCase {
viewModel.activate()

// Then
waitUntil {
viewModel.topBanner == .none
if ServiceLocator.featureFlagService.isFeatureFlagEnabled(.IPPInAppFeedbackBanner) {
waitUntil {
viewModel.topBanner == .IPPFeedback
}
} else {
waitUntil {
viewModel.topBanner == .none
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ struct InAppFeedbackCardVisibilityUseCase {
switch feedbackType {
case .general:
return try shouldGeneralFeedbackBeVisible(currentDate: currentDate)
case .shippingLabelsRelease3, .couponManagement, .ordersCreation:
case .shippingLabelsRelease3, .couponManagement, .ordersCreation, .IPPFeedback:
return settings.feedbackStatus(of: feedbackType) == .pending
}
}
Expand Down