[IPPInAppFeedback] Banner dismiss logic #8657

iamgabrielma · 2023-01-17T03:00:28Z

Closes: #8589
Part of #8530

Description

This PR adds the logic to dismiss the IPP feedback banner.

TODO

Dismisses the IPP banner upon tapping
Presents confirmation screen upon dismissal
Dismisses the IPP banner upon survey completion.

Changes

Updates the setFeatureAnnouncementDismissed AppSettingsStore/AppSettingsAction to accept a new remindAfter parameter, this allows us to specify a number of days for the reminder to be triggered (previous default to 14)
Adds an UIAlertController to be shown when the IPP banner is dismissed, with 2 buttons: "Remind me later", and "Don't show again".
Once the IPP banner is dismissed, we update the feedback status to .dismissed, and the feedback visibility to be reminded later or never.
Updated, and added Unit Tests

Testing instructions

Go to My Store > Orders and see the IPP feedback banner appear
Tap on "x" and see a confirmation alert with 2 options: "remind me later", and "Don't show again"
Confirm that the banner dismisses no matter the option you tap
Note: If Order Creation feedback banner should appear in your store, this will show after we've dismissed the IPP banner.

Screenshots

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

wpmobilebot · 2023-01-17T03:16:31Z

You can test the changes from this Pull Request by:

Clicking here or scanning the QR code below to access App Center
Then installing the build number pr8657-914d62c on your iPhone

If you need access to App Center, please ask a maintainer to add you.

# Conflicts: # WooCommerce/Classes/ViewRelated/Orders/OrderListViewController.swift # WooCommerce/Classes/ViewRelated/Orders/OrderListViewModel.swift

iamgabrielma · 2023-01-19T03:46:49Z

👋 @toupper I'd appreciate an "early" review on this one before is fully ready for review, as I'm unsure how to deal with the case of "Dismisses the IPP banner upon survey completion", happy to get any pointer on what would work for this case. For context:

If I use the existing SurveyCoordinatingController (current solution), we get the WooCommerce navigation flow, but we can't access to any completion handler to work with when the survey is submitted.
If I use the existing SurveyViewController we get a completion handler that is effectively called when the survey is submitted, but we lose the WooCommerce flow and get the Crowdsignal one instead.

We seem to use the first option across other surveys, but I'm failing to understand how are we dealing with the logic that needs to happen when a user submits the survey from the modal view. This will be needed for tracking FeedbackStatus as well.

Edit: This is resolved with 523fe0a (Reference: p1674118095143339?thread_ts=1674101250.398509&cid=CGPNUU63E-slack-CGPNUU63E). We'll be dismissing the banner right away upon tapping, and then we'll figure out if we need to show it again or not via checking the FeedbackStatus

toupper · 2023-01-19T10:55:05Z

WooCommerce/Classes/ViewRelated/Orders/OrderListViewController.swift


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


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

We definitely do 🙇 , added both on 70d9cbd

toupper · 2023-01-19T10:57:11Z

WooCommerce/Classes/ViewRelated/Orders/OrderListViewController.swift

        self.present(surveyNavigation, animated: true, completion: nil)
    }
+
+    private func dismissIPPFeedbackBannerSurvey() {


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

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

toupper · 2023-01-19T10:59:58Z

WooCommerce/Classes/ViewRelated/Orders/OrderListViewController.swift

        static let estimatedHeaderHeight = CGFloat(43)
        static let estimatedRowHeight = CGFloat(86)
        static let placeholderRowsPerSection = [3]
+        static let remindAfterDays = 7


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.

toupper · 2023-01-19T11:01:13Z

WooCommerce/Classes/ViewRelated/Orders/OrderListViewController.swift

+    }
+
+    private func remindMeLaterTapped() {
+        viewModel.dismissIPPFeedbackBanner(remindLater: true, remindAfter: Settings.remindAfterDays)


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.

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

toupper · 2023-01-19T11:02:03Z

WooCommerce/Classes/ViewRelated/Orders/OrderListViewModel.swift

        stores.dispatch(action)
    }

+    func dismissIPPFeedbackBanner(remindLater: Bool, remindAfter: Int?) {


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.

toupper · 2023-01-19T11:05:34Z

Yosemite/Yosemite/Stores/AppSettingsStore.swift

            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):


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.

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

toupper · 2023-01-19T11:06:50Z

Yosemite/Yosemite/Stores/AppSettingsStore.swift

            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):


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

toupper · 2023-01-19T11:08:03Z

Great job @iamgabrielma! I left some comments there, let me know what you think of that. Once we are done with that, I will test the flow 👍

toupper · 2023-01-19T11:09:57Z

Yosemite/YosemiteTests/Stores/AppSettingsStoreTests.swift

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

+    func test_setFeatureAnnouncementDismissed_with_remindAfter_seven_days_stores_reminder_date_in_one_week() throws {


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

Yes, you're right! Changed f8a2586

We’re modifying the action by removing the parameters remindLater, and remindAfter in favor of an unique remindAfterDays parameter. If this is nil, no reminder is set. Otherwise the number of days will be set.

By making remindAfterDays an Optional Int, we need to edit storeDismissedSetting as well to either save the current date if is true (by passing 0, so adds 0 days to the current date) or nil if is false

Yosemite/YosemiteTests/Stores/AppSettingsStoreTests.swift

iamgabrielma · 2023-01-20T05:41:43Z

Yosemite/YosemiteTests/Stores/AppSettingsStoreTests.swift

    }

-    func test_setFeatureAnnouncementDismissed_with_remindLater_true_stores_reminder_date_in_two_weeks() throws {
+    func test_setFeatureAnnouncementDismissed_with_remindAfterDays_two_weeks_stores_reminder_date_in_two_weeks() throws {


Suggested change

func test_setFeatureAnnouncementDismissed_with_remindAfterDays_two_weeks_stores_reminder_date_in_two_weeks() throws {

func test_setFeatureAnnouncementDismissed_when_remindAfterDays_two_weeks_then_stores_reminder_date_in_two_weeks() throws {

test_setFeatureAnnouncementDismissed_when_remindAfterDays_is_two_weeks_then_stores_reminder_date_is_two_weeks()

iamgabrielma · 2023-01-20T05:42:26Z

Thanks for the review @toupper ! I made the changes and this is ready for review 🙇

toupper · 2023-01-20T09:31:17Z

WooCommerce/Classes/ViewRelated/Orders/OrderListViewModel.swift

+// MARK: - In-Person Payments Feedback Banner
+
+extension OrderListViewModel {
+    func dismissIPPFeedbackBanner(remindAfterDays: Int?) {


Nice change moving the logic to the view model! However, the remindAfterDays logic can be contained in the view model; the view doesn't need to know about when are we going to show it again. With that, we can remove this parameter remindAfterDays from this function.

Changed here the remaining use of remindAfterDays: d5e02a2 to a new method, so this is entirely contained in the view model now 👍

toupper · 2023-01-20T09:32:31Z

Yosemite/YosemiteTests/Stores/AppSettingsStoreTests.swift

    }

-    func test_setFeatureAnnouncementDismissed_with_remindLater_true_stores_reminder_date_in_two_weeks() throws {
+    func test_setFeatureAnnouncementDismissed_with_remindAfterDays_two_weeks_stores_reminder_date_in_two_weeks() throws {


test_setFeatureAnnouncementDismissed_when_remindAfterDays_is_two_weeks_then_stores_reminder_date_is_two_weeks()

toupper · 2023-01-20T09:56:31Z

WooCommerce/Classes/ViewRelated/Orders/OrderListViewModel.swift

+        stores.dispatch(updateBannerVisibility)
+    }
+
+    func remindMeLaterTapped() {


We can name this function more specifically for the IPP Feedback Banner.

Renamed on 9884f22

toupper · 2023-01-20T10:18:25Z

Great job! LGTM

Add IPP banner visibility state as a new publisher

a92bddd

iamgabrielma added the feature: mobile payments Related to mobile payments / card present payments / Woo Payments. label Jan 17, 2023

iamgabrielma added this to the 12.0 milestone Jan 17, 2023

iamgabrielma mentioned this pull request Jan 17, 2023

[IPPInAppFeedback] Add IPP in-app feedback survey #8530

Closed

16 tasks

iamgabrielma added 3 commits January 17, 2023 12:16

Add initial dismiss alert using ActionSheet

8de3516

Make UIAlertController style to display as alert

cf03baf

Update banner status as dismissed, remind later, or never

232f4bc

Base automatically changed from issue/8636-orders-view-ippiaf-banner to trunk January 18, 2023 11:43

iamgabrielma added 2 commits January 19, 2023 09:32

Merge branch 'trunk' into issue/8589-ipp-banner-dismiss

bcb327e

# Conflicts: # WooCommerce/Classes/ViewRelated/Orders/OrderListViewController.swift # WooCommerce/Classes/ViewRelated/Orders/OrderListViewModel.swift

Remove trailing whitespace

398874b

iamgabrielma marked this pull request as ready for review January 19, 2023 02:23

iamgabrielma added 3 commits January 19, 2023 10:47

Add remindAfter param to setFeatureAnnouncementDismissed action

c7356c7

Unit Test new remindAfter param

2aa3298

Fix Line Length Violation

7b68aa7

iamgabrielma requested a review from toupper January 19, 2023 03:28

Add dismiss upon tapping on survey button

523fe0a

toupper reviewed Jan 19, 2023

View reviewed changes

iamgabrielma modified the milestones: 12.0, 12.1 Jan 19, 2023

iamgabrielma added 7 commits January 20, 2023 00:02

Add missing weak references to self in closures

70d9cbd

Rename method

909bea5

Rename test method for consistency

f8a2586

Move dismiss logic to view model

80a1b1b

Modify setFeatureAnnouncementCampaign action

2177584

We’re modifying the action by removing the parameters remindLater, and remindAfter in favor of an unique remindAfterDays parameter. If this is nil, no reminder is set. Otherwise the number of days will be set.

Simplify dismissIPPFeedbackBanner to use only remindAfterDays

d6b1d0a

Add comment to NSLocalizedString

cdc5089

Resolve storeDismissedSetting with Optional remindAfterDays

f294478

By making remindAfterDays an Optional Int, we need to edit storeDismissedSetting as well to either save the current date if is true (by passing 0, so adds 0 days to the current date) or nil if is false

iamgabrielma commented Jan 20, 2023

View reviewed changes

Yosemite/YosemiteTests/Stores/AppSettingsStoreTests.swift Show resolved Hide resolved

iamgabrielma commented Jan 20, 2023

View reviewed changes

iamgabrielma requested a review from toupper January 20, 2023 05:42

toupper reviewed Jan 20, 2023

View reviewed changes

toupper self-assigned this Jan 20, 2023

iamgabrielma added 3 commits January 20, 2023 18:02

Contain dismissal logic to view model only

d5e02a2

Rename test for consistency

6a53fd6

Rename dismiss methods to be more explicit

9884f22

toupper approved these changes Jan 20, 2023

View reviewed changes

Merge branch 'trunk' into issue/8589-ipp-banner-dismiss

914d62c

iamgabrielma merged commit c487172 into trunk Jan 20, 2023

iamgabrielma deleted the issue/8589-ipp-banner-dismiss branch January 20, 2023 11:02

This was referenced Jan 23, 2023

[IPPInAppFeedback] Update and save feedback status upon merchant action #8729

Closed

[IPPInAppFeedback] Add Unit Tests #8655

Closed

	func test_setFeatureAnnouncementDismissed_with_remindAfterDays_two_weeks_stores_reminder_date_in_two_weeks() throws {
	func test_setFeatureAnnouncementDismissed_when_remindAfterDays_two_weeks_then_stores_reminder_date_in_two_weeks() throws {

[IPPInAppFeedback] Banner dismiss logic #8657

[IPPInAppFeedback] Banner dismiss logic #8657

Uh oh!

Conversation

iamgabrielma commented Jan 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

TODO

Changes

Testing instructions

Screenshots

Uh oh!

wpmobilebot commented Jan 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iamgabrielma commented Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toupper Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamgabrielma Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toupper commented Jan 19, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamgabrielma commented Jan 20, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toupper commented Jan 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iamgabrielma commented Jan 17, 2023 •

edited

Loading

wpmobilebot commented Jan 17, 2023 •

edited

Loading

iamgabrielma commented Jan 19, 2023 •

edited

Loading

toupper Jan 19, 2023 •

edited

Loading

iamgabrielma Jan 20, 2023 •

edited

Loading