Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jetpack focus: Disable push notifications for WP #17371

Merged

Conversation

ravishanker
Copy link
Contributor

@ravishanker ravishanker commented Oct 26, 2022

If an instance of JP is detected then disable push notifications for WP to prevent double notifications

  • Trigger this from Jetpack app, on notifications screen in the welcome flow in a separate PR ( See example )
  • Add permissions to exported BroadcastReceiver. Preferably, signature, otherwise normal in a separate PR

To test:

Test 1:

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

Test 2:

  • Remove any WordPress app instances
  • Build and Run WordPress app or install using QR code below
  • Open Terminal in AS or Terminal app. Execute adb shell am broadcast -p org.wordpress.android.prealpha -a org.wordpress.android.broadcast.DISABLE_NOTIFICATIONS command (Welcome flow is still in the works, till then use command line to test it on WP side)
  • Verify Action: org.wordpress.android.broadcast.DISABLE_NOTIFICATIONS is displayed in a toast message on the app. The same can be seen in the Logcat in AS
  • Verify switch is set to Off on Notification Settings ( Me > Notifications tab > ⚙️ on the top right hand corner )
  • 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.

NOTE: this doesn't cover any local notifications e.g. WeeklyRoundup

Regression Notes

  1. Potential unintended areas of impact
    None

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Tested manually

  3. What automated tests I added (or what prevented me from doing so)
    Existing 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.

If an instance of JP is detected then disable push notifications for WP to prevent double notifications
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Oct 26, 2022

WordPress📲 You can test these changes on WordPress by downloading wordpress-installable-build-pr17371-2626fc0.apk
💡 Scan this QR code with your Android phone to download and install the APK directly on it.
AppWordPress
Build FlavorJalapeno
Build TypeDebug
Commit2626fc0
Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Oct 26, 2022

Jetpack📲 You can test these changes on Jetpack by downloading jetpack-installable-build-pr17371-2626fc0.apk
💡 Scan this QR code with your Android phone to download and install the APK directly on it.
AppJetpack
Build FlavorJalapeno
Build TypeDebug
Commit2626fc0
Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

apply feature flag jetpack_powered_bottom_sheet_remote_field
@ravishanker ravishanker marked this pull request as ready for review October 27, 2022 01:27
if (mJetpackBrandingUtils.shouldShowJetpackPoweredBottomSheet()
&& !mBuildConfigWrapper.isJetpackApp()
&& isJetpackInstalled()) {
mGCMMessageHandler.removeAllNotifications(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

Once this disables notifications on WordPress app, does it get reflected in the Notification Settings view, showing disabled On flag?

Copy link
Contributor Author

@ravishanker ravishanker Oct 27, 2022

Choose a reason for hiding this comment

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

Good point. this PR is only aimed at testing the disabling notifications actually works for now.

@ravishanker ravishanker modified the milestones: 21.1, 21.2 Oct 27, 2022
@ravishanker ravishanker marked this pull request as draft October 28, 2022 04:46
Turn off switch on Notifications Settings screen
Copy link
Member

@irfano irfano left a comment

Choose a reason for hiding this comment

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

Thanks, Ravi! This only removed received notifications when the WordPress app is launched. We will prevent receiving new WP notifications. But there is a case that users can install the Jetpack app when they already have received WP notifications. This PR handles only this case, right?

The function works on my test. 👍🏻 I just commented for some code changes.

Updating implementation to use BroadcastReceiver to disable notifications
Remove initial implementation in favour of BroadcastReceiver
@ravishanker
Copy link
Contributor Author

ravishanker commented Oct 31, 2022

@irfano - I've updated the implementation to use a BroadcastReceiver to trigger this from Jetpack app, after install, login, and when in the welcome flow. wdyt?

This approach has advantage of removing the need for detecting Jetpack app install, login, and inform steps.

Welcome flow is still in the works, till then we can hook this somewhere on the Jetpack app side temporarily to test it out. Updated instructions in the description

Copy link
Member

@irfano irfano left a comment

Choose a reason for hiding this comment

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

Looks great! This is a clever solution. 👏🏻
I tested sending broadcast, and it worked. The notification switch was turned off successfully. But while testing, I couldn't receive push notifications for WP, even when the switch was on. I'll test it again.

@staskus
Copy link
Contributor

staskus commented Nov 3, 2022

I've updated the implementation to use a BroadcastReceiver to trigger this from Jetpack app, after install, login, and when in the welcome flow. wdyt?

Great news! iOS and Android solutions could pretty much match then. We can even align on conditions when to trigger this from Jetpack app 👍

@irfano
Copy link
Member

irfano commented Nov 9, 2022

  • I moved JetpackAppInstallReceiver to wordpress directory. Since it will work only in WP app,
  • I added org.wordpress.android.permission.DISABLE_NOTIFICATIONS permission,
  • Removed debug logging,
  • Synced with trunk,
  • Tested it with this code block, and it worked. But the package name should align with the build variant.
Intent().also { intent ->
    intent.action = "org.wordpress.android.broadcast.DISABLE_NOTIFICATIONS"
    intent.package = "org.wordpress.android"   
    context?.sendBroadcast(intent, "org.wordpress.android.permission.DISABLE_NOTIFICATIONS")
}

@ravishanker
Copy link
Contributor Author

* Tested it with this code block, and it worked. But the package name should align with the build variant.

I tried something like this earlier

val appSuffix = BuildConfig.APPLICATION_ID.split(".").last()
            val appPackage = if (appSuffix.isNotBlank()) {
                "org.wordpress.android.${appSuffix}"
            } else {
                "org.wordpress.android"
            }
            intent.setPackage(appPackage)
            AppLog.d(T.NOTIFS, intent.toString())

@ravishanker ravishanker merged commit 503a548 into trunk Nov 21, 2022
@ravishanker ravishanker deleted the Jetpack-Focus-Disable-notifications-for-WP-on-JP-Install branch November 21, 2022 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants