-
Notifications
You must be signed in to change notification settings - Fork 138
Replace Jetpack banner with a WP dashboard card UI #15180
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
base: trunk
Are you sure you want to change the base?
Replace Jetpack banner with a WP dashboard card UI #15180
Conversation
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
...lin/com/woocommerce/android/ui/dashboard/pushnotifications/DashboardPushNotificationsCard.kt
Fixed
Show fixed
Hide fixed
...lin/com/woocommerce/android/ui/dashboard/pushnotifications/DashboardPushNotificationsCard.kt
Fixed
Show fixed
Hide fixed
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #15180 +/- ##
============================================
- Coverage 38.77% 38.77% -0.01%
Complexity 10690 10690
============================================
Files 2208 2211 +3
Lines 125713 125802 +89
Branches 17409 17424 +15
============================================
+ Hits 48745 48776 +31
- Misses 72025 72081 +56
- Partials 4943 4945 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…th-wp-dashboard-card-ui # Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/util/FeatureFlag.kt
|
Version |
…th-wp-dashboard-card-ui # Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/util/FeatureFlag.kt
67446c7 to
d72393d
Compare
…th-wp-dashboard-card-ui # Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/util/FeatureFlag.kt
…th-wp-dashboard-card-ui # Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/dashboard/DashboardViewModel.kt # WooCommerce/src/main/kotlin/com/woocommerce/android/util/FeatureFlag.kt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a new configurable “Never miss a new order” dashboard card to encourage enabling push notifications, and wires it into the dynamic dashboard system while gating it behind a new feature flag and existing push-registration state.
Changes:
- Adds a new
DashboardWidget.Type.PUSH_NOTIFICATIONSwidget, its strings, icon (wp_woo.xml), Compose UI card, and integration into the configurable dashboard and widget editor. - Introduces
ShouldShowEnablePushNotificationsUiandObservePushNotificationsWidgetStatusto determine when the push notifications card and Jetpack benefits banner should be shown, based on feature flag, connection type, andPushNotificationRegistrationStatus. - Extends dashboard data/repository/test wiring and feature flags to support the new widget and gating behavior.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| WooCommerce/src/test/kotlin/com/woocommerce/android/ui/dashboard/data/DashboardRepositoryTest.kt | Updates repository test constructor wiring to include the new ObservePushNotificationsWidgetStatus dependency. |
| WooCommerce/src/test/kotlin/com/woocommerce/android/ui/dashboard/DashboardViewModelTest.kt | Injects and mocks ShouldShowEnablePushNotificationsUi so the updated DashboardViewModel constructor and Jetpack banner logic can be instantiated in tests. |
| WooCommerce/src/main/res/values/strings.xml | Adds title, menu title, and body copy strings for the new push notifications dashboard card. |
| WooCommerce/src/main/res/drawable/wp_woo.xml | Adds the vector asset used in the push notifications card to visually represent the WordPress.com/Woo integration. |
| WooCommerce/src/main/kotlin/com/woocommerce/android/util/FeatureFlag.kt | Introduces WOO_PUSH_NOTIFICATIONS_SYSTEM_M2 and ensures both push-related flags default to false. |
| WooCommerce/src/main/kotlin/com/woocommerce/android/ui/dashboard/pushnotifications/DashboardPushNotificationsCard.kt | Implements the Compose UI for the “Never miss a new order” card, including title, description, artwork, and the standard “Hide widget” overflow menu wiring. |
| WooCommerce/src/main/kotlin/com/woocommerce/android/ui/dashboard/data/ObservePushNotificationsWidgetStatus.kt | Adds a widget-status observer that exposes the push notifications widget as Available or Hidden based on ShouldShowEnablePushNotificationsUi. |
| WooCommerce/src/main/kotlin/com/woocommerce/android/ui/dashboard/data/DashboardRepository.kt | Wires the push notifications widget status into the combined dashboard widget stream and status mapping so the new widget participates in the dynamic dashboard. |
| WooCommerce/src/main/kotlin/com/woocommerce/android/ui/dashboard/data/DashboardDataStore.kt | Ensures the new widget type is part of the default dashboard configuration and can be enabled/disabled via the widget editor, while being filtered when not supported. |
| WooCommerce/src/main/kotlin/com/woocommerce/android/ui/dashboard/DashboardViewModel.kt | Injects ShouldShowEnablePushNotificationsUi and updates jetpackBenefitsBannerState to suppress the Jetpack banner when the new push notifications UI is available. |
| WooCommerce/src/main/kotlin/com/woocommerce/android/ui/dashboard/DashboardContainer.kt | Renders DashboardPushNotificationsCard whenever a configurable widget of type PUSH_NOTIFICATIONS is present and visible. |
| WooCommerce/src/main/kotlin/com/woocommerce/android/notifications/push/ShouldShowEnablePushNotificationsUi.kt | Adds core business logic for when the “Enable Push Notifications” UI should be shown (feature-flag + app-password connection + registration status), but currently contains a boolean condition that is always true for any single status, so registered sites still see the card. |
| WooCommerce/src/main/kotlin/com/woocommerce/android/model/DashboardWidget.kt | Adds the PUSH_NOTIFICATIONS enum entry, with appropriate title resource and tracking identifier, so it can participate in dashboard configuration and analytics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...ain/kotlin/com/woocommerce/android/notifications/push/ShouldShowEnablePushNotificationsUi.kt
Outdated
Show resolved
Hide resolved
| .map { | ||
| val durationSinceDismissal = | ||
| (System.currentTimeMillis() - appPrefsWrapper.getJetpackBenefitsDismissalDate()).milliseconds | ||
| val showBanner = | ||
| val enablePnUiAvailable = shouldShowEnablePushNotificationsUi() | ||
| val showBanner = !enablePnUiAvailable && | ||
| pushNotificationRegistrationStatus(selectedSite.getIfExists()?.siteId) == Status.UNREGISTERED && | ||
| durationSinceDismissal >= DAYS_TO_REDISPLAY_JP_BENEFITS_BANNER.days | ||
| durationSinceDismissal >= DAYS_TO_REDISPLAY_JP_BENEFITS_BANNER.days |
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new logic that hides the Jetpack Benefits banner when the "Enable Push Notifications" UI is available (enablePnUiAvailable = shouldShowEnablePushNotificationsUi(), then showBanner = !enablePnUiAvailable && …) is not covered by tests: all existing DashboardViewModelTest cases stub ShouldShowEnablePushNotificationsUi to return false. Given the detailed scenarios in the PR description (TC1–TC4), please add unit tests that exercise cases where ShouldShowEnablePushNotificationsUi() returns true versus false so the banner visibility behavior is validated (e.g., ensuring the Jetpack banner is suppressed when the new push card should be shown).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @irfano I've noticed a few issues that are worth checking particularly around TC3 not working. Some are NPs but some others are blocking issues.
| * | ||
| * This is part of the Woo Core push notifications system for app-password authenticated sites. | ||
| */ | ||
| class ShouldShowEnablePushNotificationsUi @Inject constructor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Np, it be nice to have unit tests added for this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since FeatureFlag.WOO_PUSH_NOTIFICATIONS_SYSTEM_M2 is a local feature flag that defaults to false and cannot be mocked in tests, we cannot write meaningful unit tests for this class when the flag is disabled.
We could add a wrapper similar to IsRemoteFeatureFlagEnabled for local feature flags, but given that a feature flag refactor (WOOMOB-2129) is already planned in the near future, I'd prefer not to introduce this change at this point.
| val site = selectedSite.getIfExists() ?: return false | ||
| if (site.connectionType != SiteConnectionType.ApplicationPasswords) return false | ||
|
|
||
| return pushNotificationRegistrationStatus(site.siteId) != Status.WOO_REGISTERED || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Np, pushNotificationRegistrationStatus() function access disk several times via app prefs and datastore. It be nice to call this function only once and keep the returned value in a function variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic has changed in my latest commits.
...ain/kotlin/com/woocommerce/android/notifications/push/ShouldShowEnablePushNotificationsUi.kt
Outdated
Show resolved
Hide resolved
| modifier: Modifier = Modifier, | ||
| onHideClicked: () -> Unit, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor NP, but modifier should be placed last as it is the first optional parameter:
We will adopt official guidelines on how to pass the Modifier as parameter: "MUST be named "modifier" and MUST appear as the first optional parameter in the element function's parameter list".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻 Done: 194efdd
| class ObservePushNotificationsWidgetStatus @Inject constructor( | ||
| private val shouldShowEnablePushNotificationsUi: ShouldShowEnablePushNotificationsUi | ||
| ) { | ||
| operator fun invoke(): Flow<DashboardWidget.Status> = flow { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is called from DashboardRepository,kt which has @ActivityRetainedScoped. This means that when we log in with site credentials into a site that supports Woo PN notifications we'll always display the new "connect to WP.com" banner even after registering in Woo core PN system. Basically TC3 doesn't work with the current implementation even after fixing the issue reported above. We probably need to refresh the PNWidgetStatus after successfully registering in Woo system.
Screen_recording_20260203_100654.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch @JorgeMucientes! This isn't actually reproducible in #15283 but I still think it's better to apply your suggestion. Done here: c61cd28
…ons/push/ShouldShowEnablePushNotificationsUi.kt Co-authored-by: Copilot <[email protected]>
…th-wp-dashboard-card-ui
4f32249 to
2d71511
Compare
b7f995e to
bd5dac5
Compare
54f95ac to
de07126
Compare
|
Thanks for your review, @JorgeMucientes! Ready for another round. |
Closes WOOMOB-1921
Description
Adds a new "Never miss a new order" dashboard card that prompts users to connect their store to WordPress.com for push notifications. The card is implemented as a configurable widget that can be hidden and reordered via the Customize screen.
The card is currently gated behind FeatureFlag.WOO_PUSH_NOTIFICATIONS_SYSTEM and will be shown when the flag is enabled. The onClick action is currently a no-op and will be implemented in a follow-up PR.
Test Steps
TC1: Feature flag disabled, authenticated with app passwords to site without support of new PN.
WOO_PUSH_NOTIFICATIONS_SYSTEM_M2and build the app.TC2: Feature flag enabled, authenticated with app passwords to site without support of new PN.
WOO_PUSH_NOTIFICATIONS_SYSTEM_M2and build the app.TC3: Feature flag enabled, authenticated with app passwords to site with support of new PN.
WOO_PUSH_NOTIFICATIONS_SYSTEM_M2and build the app.TC4: Feature flag enabled, authenticated with WPCom to site without support of new PN.
WOO_PUSH_NOTIFICATIONS_SYSTEM_M2and build the app.Images/gif
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.