In-App Feedback Prompt UI by salimbraksa · Pull Request #22052 · wordpress-mobile/WordPress-iOS

salimbraksa · 2023-11-16T16:31:20Z

Related PR

This PR depends on:

In App Feedback Prompt Groundwork #22050

Description

This PR introduces the In-App Feedback Prompt UI. The prompt becomes visible after a combination of any of these actions is performed at least 20 times:

User taps on an unread notification
User publishes a post
User visits stats
User reads a post

Positive Feedback Flow	Negative Feedback Flow
CleanShot.2024-01-09.at.22.28.11.mp4	CleanShot.2024-01-09.at.22.30.37.mp4

Test Instruction

Positive Feedback Flow

Open WordPressAppDelegate.swift and on line 411, set utility.systemWideSignificantEventCountRequiredForPrompt to 5.
Run Jetpack and Login.
Perform a combination of these actions at least 5 times:
- User taps on an unread notification
- User publishes a post
- User visits stats
- User reads a post.
Navigate to My Site Dashboard screen.
Expect a "Your feecback matters" alert to appear.
Expect the event app_reviews_saw_prompt to be tracked.
Tap "It's great".
Expect the event app_reviews_liked_app to be tracked.
Expect "Enjoying Jetpack" alert to appear with a component to rate the app.
- You may notice the Submit button is disabled after rating the app, the reason is that the alert is a native component and the button is disabled in dev environment.
Navigate to any other screen then navigate back to My Site Dashboard.
Expect the In-App Feedback Prompt not to appear.

Negative Feedback Flow

For the In-App Feedback to appear again, you can either delete, and reinstall the Jetpack to clear the cache, or force the prompt to appear programmatically:
- Open InAppFeedbackPromptCoordinator.swift and on line 25, force shouldShowPromptForAppReview to return true.
Navigate to My Site Dashboard screen.
Run Jetpack and Login.
Expect a "Your feedback matters" alert to appear.
Tap "Not really".
Expect the event app_reviews_didnt_like_app to be tracked.
Expect another alert with title "Help us improve" to appear.
Tap "Sure".
Expect Submit Feedback screen to appear.
Expect the event app_reviews_feedback_screen_opened to be tracked.
Write a feedback.
Tap "Submit".
Expect a success message to appear.
Expect the event app_reviews_feedback_sent <feedback: The app should be improved!> to be tracked.

Cancel Negative Feedback Flow

Trigger the In-App Feedback Prompt
Tap "Not Really"
Tap "Not Now"
Expect the event app_reviews_declined_to_rate_app to be tracked
Repeat 1 to 3
Tap "Sure"
Tap "Cancel"
Expect the event app_reviews_feedback_screen_canceled to be tracked

Regression: Compliance Popover

Make the following code changes to force the compliance popover to appear:
- Go to CompliancePopoverCoordinator.swift
- On line 32, comment this code: defaults.didShowCompliancePopup
- On line 60, force the method to return true.
Run Jetpack and Login.
Expect the Compliance Popover to appear.

Regression Notes

Potential unintended areas of impact
The presentation of the Compliance Popover should be tested.
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual Testing.
What automated tests I added (or what prevented me from doing so)
None.

PR submission checklist:

I have completed the Regression Notes.
I have considered adding unit tests for my changes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist:

Portrait and landscape orientations.
Light and dark modes.
Fonts: Larger, smaller and bold text.
High contrast.
VoiceOver.
Languages with large words or with letters/accents not frequently used in English.
Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
iPhone and iPad.
Multi-tasking: Split view and Slide over. (iPad)

wpmobilebot · 2023-11-16T17:12:09Z

📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr22052-46e2143
	Version	24.0
	Bundle ID	org.wordpress.alpha
	Commit	`46e2143`
	App Center Build	WPiOS - One-Offs #8479

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

wpmobilebot · 2023-11-16T17:17:25Z

📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr22052-9b8db01
	Version	24.0
	Bundle ID	com.jetpack.alpha
	Commit	`9b8db01`
	App Center Build	jetpack-installable-builds #7499

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

mokagio · 2024-01-08T10:27:52Z

I've been going trough PRs to update them via git merge trunk to include #22270 and run on the new Apple Silicon CI with Xcode 15.1.

I did not work on this one because it's based off another PR.

salimbraksa · 2024-01-09T21:55:34Z

-        if !unreadNotificationIds.isEmpty {
-            // Track as significant event for App Rating calculations
-            AppRatingUtility.shared.incrementSignificantEvent()
-        }


This code was supposed to increment the significant events count when the user visits the Notifications screen and has unread notifications. But that didn't work, the unreadNotificationIds was empty when I visit the screen even though there are unread notifications.

After some digging, I realized the unreadNotificationIds is populated when the user switches to Unread tab, then it is emptied when switching to another tab.

This behavior is buggy in my opinion, and I replaced it with the "User taps on an unread notification" action.

cc @hassaanelgarem

…ler overlays presentation

salimbraksa · 2024-01-10T13:58:51Z

    /// uses the app in a broad fashion.
    ///
-    @objc var systemWideSignificantEventCountRequiredForPrompt: Int = 1
+    @objc var systemWideSignificantEventCountRequiredForPrompt: Int = 5


This is strange. I pushed a commit to revert this change, so locally the value of this variable should be 1, and yet it shows 5 in the PR.

But I moved this file to a different folder, which is probably why Git got confused.

I'll checkout to your branch and verify

salimbraksa · 2024-01-10T15:31:05Z

    }

-    func presentIfNeeded() {
-        guard FeatureFlag.compliancePopover.enabled/*, !defaults.didShowCompliancePopup */else {


The change /*, !defaults.didShowCompliancePopup */ was accidentally merged in release/24.0. This PR reverts the change but I'll follow up with another PR targeting 24.0 and adding a Unit Test for the presentIfNeeded method.

alpavanoglu · 2024-01-10T16:20:35Z

+    // MARK: - Properties
+
+    private let appRatingUtility: AppRatingUtility
+    private let userDefaults: UserDefaults


UserPersistentRepository can be used instead of UserDefaults as type. It simply adds an abstraction over UserDefaults to allow us injecting.

Resolved in cb10a4a. I've removed the userDefaults property as it was not used.

alpavanoglu · 2024-01-10T16:23:37Z

+        let yes = UIAlertAction(title: Strings.FeedbackAlert.yes, style: .default) { _ in
+            self.handlePositiveFeedback(in: controller)
+        }


I suppose there's no retain cycle as UIAlertController will be deallocated once the function ends. Though I thought it could make it less prone for a cycle in the future. Up to you.

Good catch, I'll add [weak self] to this closure and all other UIAlertAction closures.

I got this error when adding [weak self]:

'weak' may only be applied to class and class-bound protocol types, not 'InAppFeedbackPromptCoordinator'

That makes sense since InAppFeedbackPromptCoordinator is a struct.

Oh, that actually may not be such a great idea as it contains other reference types. So when you pass it around, its value type properties will be copied but the reference types will be modified throughout every copy.

alpavanoglu · 2024-01-10T17:56:11Z

+    deinit {
+        guard !feedbackWasSubmitted else {
+            return
+        }
+        WPAnalytics.track(.appReviewsCanceledFeedbackScreen)
+    }


Shouldn't we do this in viewDidDisappear or some other view lifecycle function rather than deinit which is about memory deallocation?

Resolved in a9dd8ab

alpavanoglu · 2024-01-10T18:02:33Z

+    private var complianceCoordinator: CompliancePopoverCoordinator?
+    private var inAppFeedbackCoordinator: InAppFeedbackPromptPresenting?


I am not sure what kind of structure you have followed but if VC retains Coordinators (whether directly or via ViewModel), these should be unowned variables rather than strongly held. Since we would wish the children Coordinators to be deallocated once their respective VCs are.

I just wanted to point that out.

these should be unowned variables rather than strongly held. Since we would wish the children Coordinators to be deallocated once their respective VCs are.

Their respective VCs don't hold a reference to these children coordinators, so if we mark them as unowned , they will be deallocated immediately.

I have considered instantiating those children coordinators in MySiteOverlaysCoordinator.presentOverlayIfNeeded, but I'm planning to write unit tests for MySiteOverlaysCoordinator and mock those children coordinators dependencies.

I see. So how are the Coordinators retained? A parent coordinator retains a child coordinator and that one retains its children coordinators? If that's the case, do we also need to deallocate them once the screen is dismissed?

So if Coordinator A (CA) is the parent:

CA -> CB -> CC and the ViewController that is linked to CC is dismissed, is there a mechanism to deallocate CC in place?

@alpavanoglu MySiteViewController retains MySiteOverlaysCoordinator, which in turns retains those child coordinators.

MySiteVC > MySiteOverlaysCoordinator > Child Coordinators

Also, none of the coordinators retain a View Controller. And the coordinators will be deallocated when MySiteVC is deallocated.

Keep in mind that the coordinators are only responsible for displaying their respective VCs, but they don't retain them.

Okay. So if a child coordinator's ViewController is dismissed, the child coordinator itself will not be deallocated because it is being referenced by the parent coordinator. If you think in terms of navigation stack, this would be like a whole flow being retained until their root VC is dismissed/deallocated.

Because of this problem, I personally prefer that each respective VC retains their Coordinator rather than a parent Coordinator retaining children coordinators. Otherwise coordinators have to replicate a deallocation chain that is provided by Navigation Stack out of the box.

Of course all of this is with the assumption that Coordinators are class and not struct. Since they often contain states that change over its lifetime, I find class to be a better fit.

I personally prefer that each respective VC retains their Coordinator rather than a parent Coordinator retaining children coordinators.

I think this is not always possible. In cases like the In-App Feedback coordinator, where the VC is just a UIAlertViewController, we have to create a subclass to retain the Coordinator.

Additionally, the InAppFeedbackCoordinator was inspired by the CompliancePopoverCoordinator, and the latter doesn't retain its VC.

I'm not against your suggestion, but I still don't know what to change exactly. Could you provide a code example to clarify this approach?

If you think in terms of navigation stack, this would be like a whole flow being retained until their root VC is dismissed/deallocated.

I understand, but this is not happening in our case. MySiteViewController retains ONLY the coordinators of the screens it is responsible for presenting.

The coordinators are just an alternative to writing the presentation logic within the MySiteViewController.

I see. The structure confused me to think otherwise. MySiteOverlaysCoordinator is merely a helper to ensure only one coordinator is active at a given time. No action needed then.

Additionally, the InAppFeedbackCoordinator was inspired by the CompliancePopoverCoordinator, and the latter doesn't retain its VC.

Yeah I think it is better that the Coordinator does not retain VCs. Maybe I wasn't articulate enough. Navigation stack already retains VCs down the chain. When Coordinators do it too, we need to deal with deallocating them.

alpavanoglu · 2024-01-10T20:17:11Z

🧪 Test Results 🧪

Positive Feedback Flow

❓ Since I cannot Submit (on simulator), I had to press Cancel. After that, the view is dismissed. And the rest of steps passed.

Negative Feedback Flow

✅ All steps work as expected.

Cancel Negative Feedback Flow

✅ All steps work as expected.

Regression: Compliance Popover

✅ All steps work as expected.

alpavanoglu

Approving to not block the progress. However I think that my last 2 comments might be important. If after considering them, you think it is safe to go ahead, please feel free to merge.

Thanks for taking care of my earlier comments!

peril-wordpress-mobile · 2024-01-18T13:53:46Z

	Warnings
⚠️	This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

salimbraksa self-assigned this Nov 16, 2023

salimbraksa added Jetpack App In-App Rating Prompt labels Nov 16, 2023

salimbraksa force-pushed the task/in-app-feedback-ui branch from c71a915 to fbb5165 Compare November 17, 2023 00:56

salimbraksa changed the base branch from trunk to task/in-app-feedback November 17, 2023 00:57

salimbraksa force-pushed the task/in-app-feedback branch 2 times, most recently from a65af62 to 6a98364 Compare January 9, 2024 12:36

salimbraksa mentioned this pull request Jan 9, 2024

In App Feedback Prompt Groundwork #22050

Merged

4 tasks

salimbraksa and others added 8 commits January 9, 2024 13:56

Implement groundwork for the in-app feedback feature

b6891d9

Update: add feature flag for in-app rating

7a620e0

Implement the In-App feedback flow

1c26645

Add screen to submit in-app feedback

4b5ee9e

Fix lint warnings

3284a12

Display a notice when feedback is submitted

872d12b

Update in-app feedback copywriting

92585c2

Change submit button style

5d77ef8

salimbraksa force-pushed the task/in-app-feedback-ui branch 3 times, most recently from 100425c to 9bdd9c8 Compare January 9, 2024 13:05

salimbraksa added 5 commits January 9, 2024 14:12

Revert unwanted changes

76ab2c5

Fix build error due to missing import

5737549

Call the In App Feedback coordinator on Blog Dashboard view did appear

57df283

Increment significant event when a notification is tapped

8e12f44

Revert unwanted changes

5129319

salimbraksa changed the title ~~In-App Feedback Alerts~~ In-App Feedback Prompt Jan 9, 2024

salimbraksa changed the title ~~In-App Feedback Prompt~~ In-App Feedback Prompt UI Jan 9, 2024

salimbraksa added this to the Pending milestone Jan 9, 2024

salimbraksa requested a review from alpavanoglu January 9, 2024 21:50

salimbraksa commented Jan 9, 2024

View reviewed changes

salimbraksa added 2 commits January 10, 2024 14:34

Move in-app feedback presentation to My Site view controller

c7be962

Implement an object responsible for coordinating My Site view control…

41fa80f

…ler overlays presentation

salimbraksa force-pushed the task/in-app-feedback-ui branch from a9fa8b9 to 41fa80f Compare January 10, 2024 13:45

salimbraksa added 2 commits January 10, 2024 14:46

Rename showPromptIfNeeded to presentIfNeeded

d742309

Revert unwanted changes

05e3f2d

salimbraksa commented Jan 10, 2024

View reviewed changes

alpavanoglu reviewed Jan 10, 2024

View reviewed changes

salimbraksa force-pushed the task/in-app-feedback-ui branch from 1162ccb to 650d08e Compare January 17, 2024 15:10

Replace the UserDefaults type to UserPersistentRepositoryReader

cb10a4a

salimbraksa force-pushed the task/in-app-feedback-ui branch from 650d08e to cb10a4a Compare January 17, 2024 15:11

salimbraksa added 3 commits January 17, 2024 16:20

Track appReviewsCanceledFeedbackScreen in viewDidDisappear

a9dd8ab

Add localized strings comments

44c4b3f

Fix failing unit test

9b8db01

salimbraksa modified the milestones: Pending, 24.1 Jan 17, 2024

salimbraksa requested a review from alpavanoglu January 17, 2024 15:53

alpavanoglu approved these changes Jan 17, 2024

View reviewed changes

salimbraksa mentioned this pull request Jan 18, 2024

Fix an issue where the All Domains flow wasn't taking into account sites with existing plan #22343

Closed

4 tasks

Convert InAppFeedbackCoordinator to class

35a048e

Revert Podfile.lock changes

46e2143

salimbraksa merged commit 4dc2a78 into task/in-app-feedback Jan 18, 2024

salimbraksa deleted the task/in-app-feedback-ui branch January 18, 2024 16:03

		private var complianceCoordinator: CompliancePopoverCoordinator?
		private var inAppFeedbackCoordinator: InAppFeedbackPromptPresenting?

Uh oh!

Conversation

salimbraksa commented Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related PR

Description

Test Instruction

Positive Feedback Flow

Negative Feedback Flow

Cancel Negative Feedback Flow

Regression: Compliance Popover

Regression Notes

PR submission checklist:

UI Changes testing checklist:

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

mokagio commented Jan 8, 2024

Uh oh!

salimbraksa Jan 9, 2024 • 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!

salimbraksa Jan 10, 2024 • 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!

salimbraksa Jan 10, 2024 • 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!

salimbraksa Jan 17, 2024 • 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!

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!

alpavanoglu Jan 10, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

salimbraksa Jan 17, 2024 • 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!

alpavanoglu Jan 17, 2024 • 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!

salimbraksa Jan 18, 2024 • 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!

alpavanoglu commented Jan 10, 2024

salimbraksa commented Nov 16, 2023 •

edited

Loading

wpmobilebot commented Nov 16, 2023 •

edited

Loading

wpmobilebot commented Nov 16, 2023 •

edited

Loading

salimbraksa Jan 9, 2024 •

edited

Loading

salimbraksa Jan 10, 2024 •

edited

Loading

salimbraksa Jan 10, 2024 •

edited

Loading

salimbraksa Jan 17, 2024 •

edited

Loading

alpavanoglu Jan 10, 2024 •

edited

Loading

salimbraksa Jan 17, 2024 •

edited

Loading

alpavanoglu Jan 17, 2024 •

edited

Loading

salimbraksa Jan 18, 2024 •

edited

Loading