Disable notifications on WP app in migration flow on JP app #17513

ravishanker · 2022-11-23T01:13:36Z

Disable notifications on WP app while in the migration flow to JP app

References:

Notifications screen on JP	Notifications settings on WP

To test:

Test 1:

Ensure either Jetpack app is installed
Ensure WordPress app is also on the device
Ensure you already receive some push notifications

Test 2:

Build and Run WordPress app or install using QR code below
Build and Run Jetpack app or install using QR code below

On Jetpack app

Open the Jetpack app & login
Go to Me → App Settings → Debug Settings
Enable the JetpackMigrationFlowFeatureConfig
Enable the prevent_duplicate_notifs_remote_field
Restart the app
Tap the Continue button on the "Welcome to Jetpack!" screen
- Note: Temporarily the migration progress is simulated by showing for 2 seconds a processing state.
Expect to see the "Notifications now come from Jetpack" screen
Tap Continue on Notifications screen
Verify Intent { act=org.wordpress.android.broadcast.DISABLE_NOTIFICATIONS pkg=org.wordpress.android.prealpha } is fired in Logcat in AS

On WordPress app

Verify switch is set to Off on Notification Settings ( Home > Notifications > ⚙️ on the top right hand corner )

Test 3

Wait till the next day if required or push a debug notification
Verify push notifications are not displayed for WordPress app
Ensure notifications are displayed for Jetpack app as before.

🔔 Notice: Once the feature flag is enabled the migration flow will always be presented. The Jetpack app build has to be uninstalled (or "Clear Data" should be done) in order to revert back to the normal functionality. Further development on the data layer will make sure the migration is not attempted again once it's successful. To be partially handled in #17406.

Regression Notes

Potential unintended areas of impact
None
What I did to test those areas of impact (or what existing automated tests I relied on)
Tested manually
What automated tests I added (or what prevented me from doing so)
Existing unit tests

PR submission checklist:

I have completed the Regression Notes.
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.

Disable notifications on WP app while in the migration flow to JP app

wpmobilebot · 2022-11-23T01:27:46Z

📲 You can test these changes on Jetpack by downloading jetpack-installable-build-pr17513-36a6fd2.apk

	💡 Scan this QR code with your Android phone to download and install the APK directly on it.
	App	`Jetpack`
	Build Flavor	`Jalapeno`
	Build Type	`Debug`
	Commit	`36a6fd2`

Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

wpmobilebot · 2022-11-23T01:29:02Z

📲 You can test these changes on WordPress by downloading wordpress-installable-build-pr17513-36a6fd2.apk

	💡 Scan this QR code with your Android phone to download and install the APK directly on it.
	App	`WordPress`
	Build Flavor	`Jalapeno`
	Build Type	`Debug`
	Commit	`36a6fd2`

Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

irfano

Thanks a lot for adding a notice for reappearing migration screens and saving me from investigating it. 😅

We disable WP notifs after tapping the continue button on the Notifications screen. But the message is telling "We've disabled notifications for the WordPress app."
I think the better place to trigger disabling WP notifs would be tryMigration(), just before postNotificationsState(). Wdyt?

...s/src/main/java/org/wordpress/android/ui/main/jetpack/migration/JetpackMigrationViewModel.kt

ravishanker · 2022-11-24T01:07:30Z

We disable WP notifs after tapping the continue button on the Notifications screen. But the message is telling "We've disabled notifications for the WordPress app." I think the better place to trigger disabling WP notifs would be tryMigration(), just before postNotificationsState(). Wdyt?

Good point! However, probably better to update the copy on Notifications screen to read "We're going to disable notifications for the WordPress app."

@ovitrif Wdyt?

…tpack-app

ovitrif · 2022-11-24T12:17:25Z

Good point! However, probably better to update the copy on Notifications screen to read "We're going to disable notifications for the WordPress app."

@ovitrif Wdyt?

Hey @ravishanker & @irfano 👋
Imho the messaging "We've disabled notifications for the WordPress app." is good, since the users don't really have an option to go somewhere else / avoid tapping the "Continue" button, so I see it more like an informative message as they progress through the migration flow.

@osullivanchris Wdyt?

The question was raised since we actually disable the WP notifications from the JP app on the moment users tap Continue on the notifications screen, and on that screen they see:

We've disabled notifications for the WordPress app.
Question was if it's not more proper to say We're going to disable notifications for the WordPress app.

Personally I don't have a strong opinion about this, for the reason mentioned above.

ovitrif

Great work @ravishanker!

I've left a comment for updating the changes to be aligned with the PR integrating the data layer with the UI.

After applying this change and using the setup steps from that PR, the migration flow works as expected and the notifications are disabled in the WP app. The migration flow is also showing only once now.

The setup is a bit different:

These flags should be enabled in build.gradle for both apps:

        buildConfigField "boolean", "JETPACK_POWERED_BOTTOM_SHEET", "true"
        buildConfigField "boolean", "JETPACK_SHARED_LOGIN", "true"
        buildConfigField "boolean", "JETPACK_LOCAL_USER_FLAGS", "true"
        buildConfigField "boolean", "JETPACK_BLOGGING_REMINDERS_SYNC", "true"
        buildConfigField "boolean", "JETPACK_READER_SAVED_POSTS", "true"
        buildConfigField "boolean", "JETPACK_PROVIDER_SYNC", "true"
        buildConfigField "boolean", "JETPACK_MIGRATION_FLOW", "true"

as well as the one for this PR:

buildConfigField "boolean", "PREVENT_DUPLICATE_NOTIFS_REMOTE_FIELD", "true"

⬆️ This one is also mentioned in the testing steps.

➕ very important also, local changes should be done in the WP app (edit a post and go back)

P.S. Imho the messaging for disabling notifications is acceptable (see other comment).

...s/src/main/java/org/wordpress/android/ui/main/jetpack/migration/JetpackMigrationViewModel.kt

irfano · 2022-11-24T14:52:01Z

Imho the messaging "We've disabled notifications for the WordPress app." is good, since the users don't really have an option to go somewhere else / avoid tapping the "Continue" button, so I see it more like an informative message as they progress through the migration flow.

I share your opinion, @ovitrif. I see the Notifications screen as an informative screen. It doesn't make a difference from the user's perspective. Because notifications will be disabled in any way and they can't control it. But I think disabling notifications should be a part of the migration process, on the screen with a loading indicator over the site list.

I can think the only way could be killing the app on the Notifications screen and relaunching it. Can users skip the Notifications screen in that case, @ovitrif?

ovitrif · 2022-11-24T15:11:30Z

But I think disabling notifications should be a part of the migration process, on the screen with a loading indicator over the site list.

That's a valid point @irfano , especially considering the next remark, which would work as you described.

I can think the only way could be killing the app on the Notifications screen and relaunching it. Can users skip the Notifications screen in that case, @ovitrif?

You're right about this, if the user kills the app on the Notifications screen, they'll end up on the My Site screen next time they launch the app, and notifications won't be disabled in the WordPress app.

Considering this I agree your suggested change should be applied in this PR. @ravishanker Does that sounds good?

osullivanchris · 2022-11-24T16:42:18Z

Question was if it's not more proper to say We're going to disable notifications for the WordPress app.

As you say @ovitrif I don't think it makes a big difference. But I'd lean towards this. Its accurate information about the current state and what is about to happen. It's possible the user could kill the app or lose network connection before hitting 'Continue'. And then the notifications would not have already been disabled as we stated.

I might tweak the copy to say:

We'll turn off notifications from the WordPress app

I know there was more discussion after your question. So I hope the question I've answered is still relevant/ up-to-date!

…tpack-app

peril-wordpress-mobile · 2022-11-24T22:25:42Z

	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

ravishanker · 2022-11-24T23:32:38Z

Thank you all for jumping into this. I've made the following updates

removed postDoneState
Set the buildConfigFields to true
Updated Notifications copy as per Chris suggestion

I noticed that setting buildConfigField to true is not working, I had to manually set all these flags to true through debug settings.

ovitrif · 2022-11-25T10:42:23Z

Set the buildConfigFields to true

My comment was misleading, really sorry for that @ravishanker.
I should have mentioned the suggestion for enabling the flags was meant as testing steps for this PR.

If ok I'll do some small changes to be able to merge this PR today:

Make sure only the PREVENT_DUPLICATE_NOTIFS_REMOTE_FIELD is true
Enabling the other build flags will be handled in this PR: [JP Content Migration Flow] Enables Jetpack migration flow #17538
Fix the merge conflicts in JetpackMigrationViewModelTest
Merge the PR to trunk when it's ready

ovitrif · 2022-11-25T10:45:20Z

I noticed that setting buildConfigField to true is not working, I had to manually set all these flags to true through debug settings.

Interesting note, thank you for mentioning this @ravishanker 🙇 .
I've never encountered this but it might have to do with the fact that some of the flags are remote or they have been somehow already set to a different value on your instance of the installed app.

Definitely worth double checking in preparation for launching the Jetpack Content Migration flow 🙇 , we've added a task for this.

…tpack-app # Conflicts: # WordPress/src/test/java/org/wordpress/android/ui/main/jetpack/migration/JetpackMigrationViewModelTest.kt

irfano

Your changes LGTM, @ovitrif! Thank you for helping us to merge this today.
The message on the notifications screen is also updated. All looks good. 👍🏻

ovitrif

LGTM after the change for the notifications continue event 🎉

Disable notifications on WP

b46f9e1

Disable notifications on WP app while in the migration flow to JP app

ravishanker added [Type] Task Jetpack Focus labels Nov 23, 2022

ravishanker added this to the 21.3 milestone Nov 23, 2022

ravishanker requested a review from irfano November 23, 2022 01:13

ravishanker added 2 commits November 23, 2022 12:29

Update RELEASE-NOTES.txt

b8dbfb5

Apply feature flag

e62486b

irfano requested changes Nov 23, 2022

View reviewed changes

...s/src/main/java/org/wordpress/android/ui/main/jetpack/migration/JetpackMigrationViewModel.kt Outdated Show resolved Hide resolved

ravishanker requested a review from ovitrif November 24, 2022 01:12

ravishanker added 2 commits November 24, 2022 12:24

Merge branch 'trunk' into Jetpack-Focus-Disable-notifications-from-Je…

acda0fb

…tpack-app

refactor reading app package

0c9c43d

ovitrif suggested changes Nov 24, 2022

View reviewed changes

...s/src/main/java/org/wordpress/android/ui/main/jetpack/migration/JetpackMigrationViewModel.kt Outdated Show resolved Hide resolved

ovitrif linked an issue Nov 24, 2022 that may be closed by this pull request

Android - Migration Notifications screen: Disable notifications in WP #17490

Closed

Merge branch 'trunk' into Jetpack-Focus-Disable-notifications-from-Je…

023c5ee

…tpack-app

Update notifications message copy

9103ccc

antonis mentioned this pull request Nov 25, 2022

[JP Content Migration Flow] Enables Jetpack migration flow #17538

Merged

3 tasks

ovitrif added 3 commits November 25, 2022 11:47

Update JetpackMigrationViewModelTest.kt

9a891db

Update build.gradle to enable PREVENT_DUPLICATE flag

457871a

Merge branch 'trunk' into Jetpack-Focus-Disable-notifications-from-Je…

36a6fd2

…tpack-app # Conflicts: # WordPress/src/test/java/org/wordpress/android/ui/main/jetpack/migration/JetpackMigrationViewModelTest.kt

ovitrif force-pushed the Jetpack-Focus-Disable-notifications-from-Jetpack-app branch from 4a06cc9 to 36a6fd2 Compare November 25, 2022 11:03

ovitrif requested review from ovitrif and irfano November 25, 2022 11:12

irfano approved these changes Nov 25, 2022

View reviewed changes

ovitrif approved these changes Nov 25, 2022

View reviewed changes

irfano merged commit 132ca7f into trunk Nov 25, 2022

irfano deleted the Jetpack-Focus-Disable-notifications-from-Jetpack-app branch November 25, 2022 11:50

irfano mentioned this pull request Dec 6, 2022

Disable PREVENT_DUPLICATE_NOTIFS_REMOTE_FIELD #17629

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable notifications on WP app in migration flow on JP app #17513

Disable notifications on WP app in migration flow on JP app #17513

ravishanker commented Nov 23, 2022 •

edited

Loading

wpmobilebot commented Nov 23, 2022 •

edited

Loading

wpmobilebot commented Nov 23, 2022 •

edited

Loading

irfano left a comment

ravishanker commented Nov 24, 2022 •

edited

Loading

ovitrif commented Nov 24, 2022

ovitrif left a comment •

edited

Loading

irfano commented Nov 24, 2022

ovitrif commented Nov 24, 2022 •

edited

Loading

osullivanchris commented Nov 24, 2022

peril-wordpress-mobile bot commented Nov 24, 2022

ravishanker commented Nov 24, 2022 •

edited

Loading

ovitrif commented Nov 25, 2022 •

edited

Loading

ovitrif commented Nov 25, 2022 •

edited

Loading

irfano left a comment

ovitrif left a comment

Disable notifications on WP app in migration flow on JP app #17513

Disable notifications on WP app in migration flow on JP app #17513

Conversation

ravishanker commented Nov 23, 2022 • edited Loading

Regression Notes

wpmobilebot commented Nov 23, 2022 • edited Loading

wpmobilebot commented Nov 23, 2022 • edited Loading

irfano left a comment

Choose a reason for hiding this comment

ravishanker commented Nov 24, 2022 • edited Loading

ovitrif commented Nov 24, 2022

ovitrif left a comment • edited Loading

Choose a reason for hiding this comment

irfano commented Nov 24, 2022

ovitrif commented Nov 24, 2022 • edited Loading

osullivanchris commented Nov 24, 2022

peril-wordpress-mobile bot commented Nov 24, 2022

ravishanker commented Nov 24, 2022 • edited Loading

ovitrif commented Nov 25, 2022 • edited Loading

ovitrif commented Nov 25, 2022 • edited Loading

irfano left a comment

Choose a reason for hiding this comment

ovitrif left a comment

Choose a reason for hiding this comment

ravishanker commented Nov 23, 2022 •

edited

Loading

wpmobilebot commented Nov 23, 2022 •

edited

Loading

wpmobilebot commented Nov 23, 2022 •

edited

Loading

ravishanker commented Nov 24, 2022 •

edited

Loading

ovitrif left a comment •

edited

Loading

ovitrif commented Nov 24, 2022 •

edited

Loading

ravishanker commented Nov 24, 2022 •

edited

Loading

ovitrif commented Nov 25, 2022 •

edited

Loading

ovitrif commented Nov 25, 2022 •

edited

Loading