Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
a92bddd
Add IPP banner visibility state as a new publisher
iamgabrielma Jan 17, 2023
8de3516
Add initial dismiss alert using ActionSheet
iamgabrielma Jan 17, 2023
cf03baf
Make UIAlertController style to display as alert
iamgabrielma Jan 17, 2023
232f4bc
Update banner status as dismissed, remind later, or never
iamgabrielma Jan 17, 2023
bcb327e
Merge branch 'trunk' into issue/8589-ipp-banner-dismiss
iamgabrielma Jan 19, 2023
398874b
Remove trailing whitespace
iamgabrielma Jan 19, 2023
c7356c7
Add remindAfter param to setFeatureAnnouncementDismissed action
iamgabrielma Jan 19, 2023
2aa3298
Unit Test new remindAfter param
iamgabrielma Jan 19, 2023
7b68aa7
Fix Line Length Violation
iamgabrielma Jan 19, 2023
523fe0a
Add dismiss upon tapping on survey button
iamgabrielma Jan 19, 2023
70d9cbd
Add missing weak references to self in closures
iamgabrielma Jan 19, 2023
909bea5
Rename method
iamgabrielma Jan 19, 2023
f8a2586
Rename test method for consistency
iamgabrielma Jan 19, 2023
80a1b1b
Move dismiss logic to view model
iamgabrielma Jan 20, 2023
2177584
Modify setFeatureAnnouncementCampaign action
iamgabrielma Jan 20, 2023
d6b1d0a
Simplify dismissIPPFeedbackBanner to use only remindAfterDays
iamgabrielma Jan 20, 2023
cdc5089
Add comment to NSLocalizedString
iamgabrielma Jan 20, 2023
f294478
Resolve storeDismissedSetting with Optional remindAfterDays
iamgabrielma Jan 20, 2023
d5e02a2
Contain dismissal logic to view model only
iamgabrielma Jan 20, 2023
6a53fd6
Rename test for consistency
iamgabrielma Jan 20, 2023
9884f22
Rename dismiss methods to be more explicit
iamgabrielma Jan 20, 2023
914d62c
Merge branch 'trunk' into issue/8589-ipp-banner-dismiss
iamgabrielma Jan 20, 2023
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
Expand Up @@ -4,6 +4,7 @@ public enum FeatureAnnouncementCampaign: String, Codable, Equatable {
case upsellCardReaders = "upsell_card_readers"
case linkedProductsPromo = "linked_products_promo"
case productsOnboarding = "products_onboarding_first_product"
case IPP = "IPP_feedback_request"

/// Added for use in `test_setFeatureAnnouncementDismissed_with_another_campaign_previously_dismissed_keeps_values_for_both`
/// This can be removed when we have a second campaign, which can be used in the above test instead.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ class FeatureAnnouncementCardViewModel: AnnouncementCardViewModelProtocol {
private func storeDismissedSetting(remindLater: Bool) {
let action = AppSettingsAction.setFeatureAnnouncementDismissed(campaign: config.campaign,
remindLater: remindLater,
remindAfter: nil,
onCompletion: nil)
stores.dispatch(action)
shouldBeVisible = false
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ struct ProductsOnboardingAnnouncementCardViewModel: AnnouncementCardViewModelPro
func dontShowAgainTapped() {
let action = AppSettingsAction.setFeatureAnnouncementDismissed(campaign: .productsOnboarding,
remindLater: false,
remindAfter: nil,
onCompletion: nil)
ServiceLocator.stores.dispatch(action)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -796,14 +796,19 @@ private extension OrderListViewController {
private func createIPPFeedbackTopBanner() -> TopBannerView {
let shareIPPFeedbackAction = TopBannerViewModel.ActionButton(title: Localization.shareFeedbackButton, action: { _ in
self.displayIPPFeedbackBannerSurvey()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to add [weak self] for these references? Also for other self usage within a block in this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We definitely do 🙇 , added both on 70d9cbd

// We dismiss the banner at this point as well, as we cannot know if was successfully submited or not at this point
// We will check the FeedbackStatus to know if we must display it again or not
self.viewModel.dismissIPPFeedbackBanner(remindLater: false, remindAfter: nil)
})

let viewModel = TopBannerViewModel(
title: Localization.feedbackBannerTitle,
infoText: Localization.feedbackBannerContent,
icon: UIImage.gridicon(.comment),
isExpanded: true,
topButton: .dismiss(handler: { }),
topButton: .dismiss(handler: {
self.dismissIPPFeedbackBannerSurvey()
}),
actionButtons: [shareIPPFeedbackAction]
)
let topBannerView = TopBannerView(viewModel: viewModel)
Expand All @@ -816,6 +821,34 @@ private extension OrderListViewController {
let surveyNavigation = SurveyCoordinatingController(survey: .IPPFeedback)
self.present(surveyNavigation, animated: true, completion: nil)
}

private func dismissIPPFeedbackBannerSurvey() {
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to be more precise, I would rename this function to actually reflect what's happening inside. Perhaps something like func showIPPFeedbackDismissAlert() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the method internals changed but I didn't change the name. Modified on 909bea5

let actionSheet = UIAlertController(
title: Localization.dismissTitle,
message: Localization.dismissMessage,
preferredStyle: .alert
)

let remindMeLaterAction = UIAlertAction( title: Localization.remindMeLater, style: .default) { [weak self] _ in
self?.remindMeLaterTapped()
}
actionSheet.addAction(remindMeLaterAction)

let dontShowAgainAction = UIAlertAction( title: Localization.dontShowAgain, style: .default) { [weak self] _ in
self?.dontShowAgainTapped()
}
actionSheet.addAction(dontShowAgainAction)

self.present(actionSheet, animated: true)
}

private func remindMeLaterTapped() {
viewModel.dismissIPPFeedbackBanner(remindLater: true, remindAfter: Settings.remindAfterDays)
Copy link
Contributor

Choose a reason for hiding this comment

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

The remindLater and remindAfter logic seem to fit better in the view model logic, I would move that there while keeping the UI actions here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, moved to the view model on 80a1b1b

}

private func dontShowAgainTapped() {
viewModel.dismissIPPFeedbackBanner(remindLater: false, remindAfter: nil)
}
}

// MARK: - Constants
Expand Down Expand Up @@ -846,6 +879,17 @@ private extension OrderListViewController {
comment: "Title of the feedback action button on the In-Person Payments feedback banner"
)

static let dismissTitle = NSLocalizedString("Give feedback",
comment: "Title of the modal confirmation screen when the In-Person Payments feedback banner is dismissed"
)

static let dismissMessage = NSLocalizedString("No worries! You can always go to Settings in the Menu to send us feedback.",
comment: "Message of the modal confirmation screen when the In-Person Payments feedback banner is dismissed")

static let remindMeLater = NSLocalizedString("Remind me later", comment: "")

static let dontShowAgain = NSLocalizedString("Don't show again", comment: "")

static func markCompletedNoticeTitle(orderID: Int64) -> String {
let format = NSLocalizedString(
"Order #%1$d marked as completed",
Expand All @@ -867,6 +911,7 @@ private extension OrderListViewController {
static let estimatedHeaderHeight = CGFloat(43)
static let estimatedRowHeight = CGFloat(86)
static let placeholderRowsPerSection = [3]
static let remindAfterDays = 7
Copy link
Contributor

Choose a reason for hiding this comment

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

I would be more specific here, what is it that we are using this value for? IPP Feedback or something else? Also, this pertains to view model logic, as mentioned before.

}

enum State {
Expand Down
17 changes: 17 additions & 0 deletions WooCommerce/Classes/ViewRelated/Orders/OrderListViewModel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,23 @@ final class OrderListViewModel {
stores.dispatch(action)
}

func dismissIPPFeedbackBanner(remindLater: Bool, remindAfter: Int?) {
Copy link
Contributor

@toupper toupper Jan 19, 2023

Choose a reason for hiding this comment

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

As mentioned before, remindLater and remindAfter shouldn't be exposed to the view, but kept here inside the view model logic. We can also test it that way.

// Updates the IPP feedback banner status as dismissed
let updateFeedbackStatus = AppSettingsAction.updateFeedbackStatus(type: .IPP, status: .dismissed, onCompletion: { _ in
self.hideIPPFeedbackBanner = true
})
stores.dispatch(updateFeedbackStatus)

// Updates the IPP feedback banner status to be reminded later, or never
let updateBannerVisibility = AppSettingsAction.setFeatureAnnouncementDismissed(
campaign: .IPP,
remindLater: remindLater,
remindAfter: remindAfter,
onCompletion: nil
)
stores.dispatch(updateBannerVisibility)
}

/// Starts the snapshotsProvider, logging any errors.
private func startReceivingSnapshots() {
do {
Expand Down
7 changes: 6 additions & 1 deletion Yosemite/Yosemite/Actions/AppSettingsAction.swift
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,12 @@ public enum AppSettingsAction: Action {

// MARK: - Feature Announcement Card Visibility

case setFeatureAnnouncementDismissed(campaign: FeatureAnnouncementCampaign, remindLater: Bool, onCompletion: ((Result<Bool, Error>) -> ())?)
case setFeatureAnnouncementDismissed(
campaign: FeatureAnnouncementCampaign,
remindLater: Bool,
remindAfter: Int?,
onCompletion: ((Result<Bool, Error>) -> ())?
)

case getFeatureAnnouncementVisibility(campaign: FeatureAnnouncementCampaign, onCompletion: (Result<Bool, Error>) -> ())

Expand Down
34 changes: 19 additions & 15 deletions Yosemite/Yosemite/Stores/AppSettingsStore.swift
Original file line number Diff line number Diff line change
Expand Up @@ -178,8 +178,8 @@ public class AppSettingsStore: Store {
setCouponManagementFeatureSwitchState(isEnabled: isEnabled, onCompletion: onCompletion)
case .loadCouponManagementFeatureSwitchState(let onCompletion):
loadCouponManagementFeatureSwitchState(onCompletion: onCompletion)
case .setFeatureAnnouncementDismissed(campaign: let campaign, remindLater: let remindLater, onCompletion: let completion):
setFeatureAnnouncementDismissed(campaign: campaign, remindLater: remindLater, onCompletion: completion)
case .setFeatureAnnouncementDismissed(campaign: let campaign, remindLater: let remindLater, remindAfter: let remindAfter, onCompletion: let completion):
Copy link
Contributor

Choose a reason for hiding this comment

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

I would encapsulate remindLater and remindLater in one value, I don't think we need both here. If we have both we can give place to inconsistent states, e.g., remindLater false and remindAfter to 5 days. For instance, we can solve this as follows:

  • We can have only remindAfter, where 0 means it shouldn't be reminded. Or,
  • remindAfter can be an optional, being nil meaning it shouldn't be reminded. I prefer this one.

Copy link
Contributor Author

@iamgabrielma iamgabrielma Jan 20, 2023

Choose a reason for hiding this comment

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

Much cleaner indeed, changes here: 2177584 Now we have a unique Optional parameter remindAfterDays rather than two.

Copy link
Contributor

Choose a reason for hiding this comment

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

If keep remindAfter, it would be valuable to specify what time unit are we expecting: remindAfterDays

setFeatureAnnouncementDismissed(campaign: campaign, remindLater: remindLater, remindAfter: remindAfter, onCompletion: completion)
case .getFeatureAnnouncementVisibility(campaign: let campaign, onCompletion: let completion):
getFeatureAnnouncementVisibility(campaign: campaign, onCompletion: completion)
case .setSkippedCashOnDeliveryOnboardingStep(siteID: let siteID):
Expand Down Expand Up @@ -758,20 +758,24 @@ private extension AppSettingsStore {

extension AppSettingsStore {

func setFeatureAnnouncementDismissed(campaign: FeatureAnnouncementCampaign, remindLater: Bool, onCompletion: ((Result<Bool, Error>) -> ())?) {
do {
let remindAfter = remindLater ? Date().addingDays(14) : nil
let newSettings = FeatureAnnouncementCampaignSettings(dismissedDate: Date(), remindAfter: remindAfter)

let settings = generalAppSettings.settings
let settingsToSave = settings.replacing(featureAnnouncementSettings: newSettings, for: campaign)
try generalAppSettings.saveSettings(settingsToSave)

onCompletion?(.success(true))
} catch {
onCompletion?(.failure(error))
func setFeatureAnnouncementDismissed(
campaign: FeatureAnnouncementCampaign,
remindLater: Bool,
remindAfter: Int? = 14,
onCompletion: ((Result<Bool, Error>) -> ())?) {
do {
let remindAfter = remindLater ? Date().addingDays(remindAfter ?? 14) : nil
let newSettings = FeatureAnnouncementCampaignSettings(dismissedDate: Date(), remindAfter: remindAfter)

let settings = generalAppSettings.settings
let settingsToSave = settings.replacing(featureAnnouncementSettings: newSettings, for: campaign)
try generalAppSettings.saveSettings(settingsToSave)

onCompletion?(.success(true))
} catch {
onCompletion?(.failure(error))
}
}
}

func getFeatureAnnouncementVisibility(campaign: FeatureAnnouncementCampaign, onCompletion: (Result<Bool, Error>) -> ()) {
guard let campaignSettings = generalAppSettings.value(for: \.featureAnnouncementCampaignSettings)[campaign] else {
Expand Down
24 changes: 21 additions & 3 deletions Yosemite/YosemiteTests/Stores/AppSettingsStoreTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -848,7 +848,7 @@ extension AppSettingsStoreTests {
try fileStorage?.deleteFile(at: expectedGeneralStoreSettingsFileURL)

// When
let action = AppSettingsAction.setFeatureAnnouncementDismissed(campaign: .upsellCardReaders, remindLater: false, onCompletion: nil)
let action = AppSettingsAction.setFeatureAnnouncementDismissed(campaign: .upsellCardReaders, remindLater: false, remindAfter: nil, onCompletion: nil)
subject?.onAction(action)

// Then
Expand All @@ -866,7 +866,7 @@ extension AppSettingsStoreTests {
try fileStorage?.deleteFile(at: expectedGeneralStoreSettingsFileURL)

// When
let action = AppSettingsAction.setFeatureAnnouncementDismissed(campaign: .upsellCardReaders, remindLater: true, onCompletion: nil)
let action = AppSettingsAction.setFeatureAnnouncementDismissed(campaign: .upsellCardReaders, remindLater: true, remindAfter: nil, onCompletion: nil)
subject?.onAction(action)

// Then
Expand All @@ -877,6 +877,24 @@ extension AppSettingsStoreTests {
XCTAssert(Calendar.current.isDate(actualRemindAfter, inSameDayAs: twoWeeksTime))
}

func test_setFeatureAnnouncementDismissed_with_remindAfter_seven_days_stores_reminder_date_in_one_week() throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great to use the agreed test function naming syntax here as well:

test_setFeatureAnnouncementDismissed_when_remindAfter_is_seven_days_stores_reminder_then_date_saved_date_is_one_week

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right! Changed f8a2586

// Given
let oneWeekTime = Calendar.current.date(byAdding: .day, value: 7, to: Date())!

try fileStorage?.deleteFile(at: expectedGeneralStoreSettingsFileURL)

// When
let action = AppSettingsAction.setFeatureAnnouncementDismissed(campaign: .upsellCardReaders, remindLater: true, remindAfter: 7, onCompletion: nil)
subject?.onAction(action)

// Then
let savedSettings: GeneralAppSettings = try XCTUnwrap(fileStorage?.data(for: expectedGeneralAppSettingsFileURL))

let actualRemindAfter = try XCTUnwrap( savedSettings.featureAnnouncementCampaignSettings[.upsellCardReaders]?.remindAfter)

XCTAssert(Calendar.current.isDate(actualRemindAfter, inSameDayAs: oneWeekTime))
}

func test_setFeatureAnnouncementDismissed_with_another_campaign_previously_dismissed_keeps_values_for_both() throws {
// Given
try fileStorage?.deleteFile(at: expectedGeneralStoreSettingsFileURL)
Expand All @@ -888,7 +906,7 @@ extension AppSettingsStoreTests {
try fileStorage?.write(settings, to: expectedGeneralAppSettingsFileURL)

// When
let action = AppSettingsAction.setFeatureAnnouncementDismissed(campaign: .upsellCardReaders, remindLater: false, onCompletion: nil)
let action = AppSettingsAction.setFeatureAnnouncementDismissed(campaign: .upsellCardReaders, remindLater: false, remindAfter: nil, onCompletion: nil)
subject?.onAction(action)

// Then
Expand Down