Issue/12539 blaze push notification navigation #12564

JorgeMucientes · 2024-09-09T15:28:52Z

Description

When a Blaze campaign status update push notification is received upon tapping on it:

Load the campaign list screen and open the campaign detail screen when tapping on a single notification
If notifications are grouped, only open campaign list

Testing information

Make a clean install of the app (we need this to ensure the FCM token is fresh for 20.2 version of the app and Blaze push notifications are enabled)
Create a Blaze campaign.
Go to the Slack channel #wp-promote-moderation and select you campaign. Approve it. This will trigger an approved campaign push.
Tap on the notification and notice how the campaign detail screen is loaded correctly.
Navigate back from campaign detail and check that you first land on campaign list screen and finally in the more menu tab.

Repeat the steps above to generate at least 2 Blaze push notifications (tip:cancelling/Stopping the campaign from the detail screen webview will also trigger a push notification)

Once you have at least 2 notifications, click on the "grouped" notification and notice how the app open in the campaign list screen.

Finally, the smoke test will be done to ensure that new order notifications and product review notifications work as expected.

The tests that have been performed

Blaze push notifications tested (single and grouped)
New Order notification tested.
New review notification tested.

Images/gif

OpenBlazeCampaignDetail.mp4

I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on big (tablet) and small (phone) in case of UI changes, and no regressions are added.

dangermattic · 2024-09-09T15:31:19Z

	1 Warning
⚠️	This PR is assigned to the milestone 20.4. This milestone is due in less than 2 days. Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

wpmobilebot · 2024-09-09T15:35:36Z

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce-Wear Android
	Platform	⌚️ Wear OS
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`318fe1f`
	Direct Download	`woocommerce-wear-prototype-build-pr12564-318fe1f.apk`

wpmobilebot · 2024-09-09T18:20:07Z

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

	App Name	WooCommerce Android
	Platform	📱 Mobile
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`318fe1f`
	Direct Download	`woocommerce-prototype-build-pr12564-318fe1f.apk`

JorgeMucientes · 2024-09-10T16:11:58Z

WooCommerce/src/main/kotlin/com/woocommerce/android/extensions/NumberExtensionsWrapper.kt

+import com.woocommerce.android.extensions.compactNumberCompat as compactNumberCompatExt

 class NumberExtensionsWrapper @Inject constructor() {
    fun compactNumberCompat(
        number: Long,
        locale: Locale = Locale.getDefault()
-    ): String = compactNumberCompat(number, locale)
+    ): String = compactNumberCompatExt(number, locale)
 }


Unrelated to the PR sorry. Had to manually update this so I could prevent the app from crashing due to this: #12554

I hope that merging trunk later will get rid of this, it would be better to keep a clean Git history.

hichamboushaba

Nice work @JorgeMucientes.

I left a comment about a minor concern, but it's not a blocking one, so I'm pre-approving.

hichamboushaba · 2024-09-11T08:41:17Z

WooCommerce/src/main/kotlin/com/woocommerce/android/model/Notification.kt

+    val isBlazeNotification = noteType == WooNotificationType.BLAZE_APPROVED_NOTE ||
+        noteType == WooNotificationType.BLAZE_REJECTED_NOTE ||
+        noteType == WooNotificationType.BLAZE_CANCELLED_NOTE ||
+        noteType == WooNotificationType.BLAZE_PERFORMED_NOTE


Just a suggestion for a future improvement: can't we improve the API a bit if we convert WooNotificationType to be a sealed interface instead of an enum? I think if we do so we'll have a common parent for Blaze notification types.

hichamboushaba · 2024-09-11T09:16:30Z

...erce/src/main/kotlin/com/woocommerce/android/ui/blaze/campaigs/BlazeCampaignListViewModel.kt

            loadCampaigns(offset = 0)
+            if (navArgs.campaignId != null) {
+                onCampaignClicked(navArgs.campaignId!!)
+            }


I have two minor concerns with this:

We won't show the campaign details until loadCampaigns finish, which could take a while depending on the internet speed.

onCampaignClicked will track blaze_campaign_detail_selected, and I'm not sure this is valid for this case, as the event is coming from the notification.

For both of these issues, I suggest:

Move the logic outside of the launch to trigger instantly without waiting for the campaigns to load.

(1) alone will cause an issue, because navigateSafely will skip the event due to the throttle logic we have, so we'll need another change: update the ShowCampaignDetails event to differentiate push events and for them pass skipThrottling as true in the navigateSafely call.

I experimented with this, and the result is an instant display of the details:

Before After

before.mp4

after.mp4

WDYT?

hichamboushaba · 2024-09-11T09:17:16Z

WooCommerce/src/main/kotlin/com/woocommerce/android/extensions/NumberExtensionsWrapper.kt

+import com.woocommerce.android.extensions.compactNumberCompat as compactNumberCompatExt

 class NumberExtensionsWrapper @Inject constructor() {
    fun compactNumberCompat(
        number: Long,
        locale: Locale = Locale.getDefault()
-    ): String = compactNumberCompat(number, locale)
+    ): String = compactNumberCompatExt(number, locale)
 }


I hope that merging trunk later will get rid of this, it would be better to keep a clean Git history.

…ion-navigation # Conflicts: # build.gradle

wpmobilebot · 2024-09-11T14:14:10Z

Found 1 violations:

The PR caused the following dependency changes:

expand

-+--- org.wordpress:fluxc:2.95.0
-|    +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.25
-|    |    +--- org.jetbrains.kotlin:kotlin-stdlib:1.9.25 (*)
-|    |    \--- org.jetbrains.kotlin:kotlin-android-extensions-runtime:1.9.25
-|    |         \--- org.jetbrains.kotlin:kotlin-stdlib:1.9.25 (*)
-|    +--- androidx.exifinterface:exifinterface:1.0.0 -> 1.3.6
-|    |    \--- androidx.annotation:annotation:1.2.0 -> 1.8.0 (*)
-|    +--- androidx.security:security-crypto:1.0.0 -> 1.1.0-alpha03
-|    |    +--- androidx.annotation:annotation:1.1.0 -> 1.8.0 (*)
-|    |    +--- com.google.crypto.tink:tink-android:1.5.0
-|    |    \--- androidx.collection:collection:1.1.0 -> 1.4.0 (*)
-|    +--- com.squareup.okhttp3:okhttp-urlconnection:4.9.0
-|    |    +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.12.0 (*)
-|    |    \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.4.10 -> 1.9.10 (*)
-|    +--- com.google.code.gson:gson:2.8.5 -> 2.10.1
-|    +--- org.apache.commons:commons-text:1.10.0 (*)
-|    +--- androidx.room:room-runtime:2.6.1 (*)
-|    +--- androidx.room:room-ktx:2.6.1
-|    |    +--- androidx.room:room-common:2.6.1 (*)
-|    |    +--- androidx.room:room-runtime:2.6.1 (*)
-|    |    +--- org.jetbrains.kotlin:kotlin-stdlib:1.8.22 -> 1.9.25 (*)
-|    |    +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.7.1 -> 1.8.1 (*)
-|    |    +--- androidx.room:room-common:2.6.1 (c)
-|    |    \--- androidx.room:room-runtime:2.6.1 (c)
-|    +--- com.google.dagger:dagger:2.51.1
-|    |    \--- javax.inject:javax.inject:1
-|    +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.8.1 (*)
-|    +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.8.1 (*)
-|    +--- org.wordpress:wellsql:2.0.0
-|    |    +--- androidx.annotation:annotation:1.2.0 -> 1.8.0 (*)
-|    |    \--- org.wordpress.wellsql:wellsql-annotations:2.0.0
-|    +--- org.wordpress.fluxc:fluxc-annotations:2.95.0
-|    +--- org.greenrobot:eventbus-java:3.3.1
-|    +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.12.0 (*)
-|    +--- com.android.volley:volley:1.1.1 -> 1.2.0
-|    +--- androidx.paging:paging-runtime:2.1.2
-|    |    +--- androidx.paging:paging-common:2.1.2
-|    |    |    +--- androidx.annotation:annotation:1.0.0 -> 1.8.0 (*)
-|    |    |    \--- androidx.arch.core:core-common:2.0.0 -> 2.2.0 (*)
-|    |    +--- androidx.arch.core:core-runtime:2.0.0 -> 2.2.0 (*)
-|    |    +--- androidx.lifecycle:lifecycle-runtime:2.0.0 -> 2.7.0 (*)
-|    |    +--- androidx.lifecycle:lifecycle-livedata:2.0.0 -> 2.7.0 (*)
-|    |    \--- androidx.recyclerview:recyclerview:1.0.0 -> 1.3.2 (*)
-|    +--- com.goterl:lazysodium-android:5.0.2
-|    +--- net.java.dev.jna:jna:5.5.0
-|    \--- org.jetbrains.kotlin:kotlin-stdlib:1.9.25 (*)
++--- org.wordpress:fluxc:trunk-884fbb8e16259765bb490127476813962e92a113
+|    +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.25
+|    |    +--- org.jetbrains.kotlin:kotlin-stdlib:1.9.25 (*)
+|    |    \--- org.jetbrains.kotlin:kotlin-android-extensions-runtime:1.9.25
+|    |         \--- org.jetbrains.kotlin:kotlin-stdlib:1.9.25 (*)
+|    +--- androidx.exifinterface:exifinterface:1.0.0 -> 1.3.6
+|    |    \--- androidx.annotation:annotation:1.2.0 -> 1.8.0 (*)
+|    +--- androidx.security:security-crypto:1.0.0 -> 1.1.0-alpha03
+|    |    +--- androidx.annotation:annotation:1.1.0 -> 1.8.0 (*)
+|    |    +--- com.google.crypto.tink:tink-android:1.5.0
+|    |    \--- androidx.collection:collection:1.1.0 -> 1.4.0 (*)
+|    +--- com.squareup.okhttp3:okhttp-urlconnection:4.9.0
+|    |    +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.12.0 (*)
+|    |    \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.4.10 -> 1.9.10 (*)
+|    +--- com.google.code.gson:gson:2.8.5 -> 2.10.1
+|    +--- org.apache.commons:commons-text:1.10.0 (*)
+|    +--- androidx.room:room-runtime:2.6.1 (*)
+|    +--- androidx.room:room-ktx:2.6.1
+|    |    +--- androidx.room:room-common:2.6.1 (*)
+|    |    +--- androidx.room:room-runtime:2.6.1 (*)
+|    |    +--- org.jetbrains.kotlin:kotlin-stdlib:1.8.22 -> 1.9.25 (*)
+|    |    +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.7.1 -> 1.8.1 (*)
+|    |    +--- androidx.room:room-common:2.6.1 (c)
+|    |    \--- androidx.room:room-runtime:2.6.1 (c)
+|    +--- com.google.dagger:dagger:2.51.1
+|    |    \--- javax.inject:javax.inject:1
+|    +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.8.1 (*)
+|    +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.8.1 (*)
+|    +--- org.wordpress:wellsql:2.0.0
+|    |    +--- androidx.annotation:annotation:1.2.0 -> 1.8.0 (*)
+|    |    \--- org.wordpress.wellsql:wellsql-annotations:2.0.0
+|    +--- org.wordpress.fluxc:fluxc-annotations:trunk-884fbb8e16259765bb490127476813962e92a113
+|    +--- org.greenrobot:eventbus-java:3.3.1
+|    +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.12.0 (*)
+|    +--- com.android.volley:volley:1.1.1 -> 1.2.0
+|    +--- androidx.paging:paging-runtime:2.1.2
+|    |    +--- androidx.paging:paging-common:2.1.2
+|    |    |    +--- androidx.annotation:annotation:1.0.0 -> 1.8.0 (*)
+|    |    |    \--- androidx.arch.core:core-common:2.0.0 -> 2.2.0 (*)
+|    |    +--- androidx.arch.core:core-runtime:2.0.0 -> 2.2.0 (*)
+|    |    +--- androidx.lifecycle:lifecycle-runtime:2.0.0 -> 2.7.0 (*)
+|    |    +--- androidx.lifecycle:lifecycle-livedata:2.0.0 -> 2.7.0 (*)
+|    |    \--- androidx.recyclerview:recyclerview:1.0.0 -> 1.3.2 (*)
+|    +--- com.goterl:lazysodium-android:5.0.2
+|    +--- net.java.dev.jna:jna:5.5.0
+|    \--- org.jetbrains.kotlin:kotlin-stdlib:1.9.25 (*)
-\--- org.wordpress.fluxc.plugins:woocommerce:2.95.0
-     +--- org.wordpress:fluxc:2.95.0 (*)
-     +--- com.google.code.gson:gson:2.8.5 -> 2.10.1
-     +--- com.google.dagger:dagger:2.51.1 (*)
-     +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.8.1 (*)
-     +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.8.1 (*)
-     +--- androidx.room:room-runtime:2.6.1 (*)
-     +--- org.wordpress:wellsql:2.0.0 (*)
-     +--- org.wordpress.fluxc:fluxc-annotations:2.95.0
-     +--- androidx.room:room-ktx:2.6.1 (*)
-     \--- org.jetbrains.kotlin:kotlin-stdlib:1.9.25 (*)
+\--- org.wordpress.fluxc.plugins:woocommerce:trunk-884fbb8e16259765bb490127476813962e92a113
+     +--- org.wordpress:fluxc:trunk-884fbb8e16259765bb490127476813962e92a113 (*)
+     +--- com.google.code.gson:gson:2.8.5 -> 2.10.1
+     +--- com.google.dagger:dagger:2.51.1 (*)
+     +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.8.1 (*)
+     +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.8.1 (*)
+     +--- androidx.room:room-runtime:2.6.1 (*)
+     +--- org.wordpress:wellsql:2.0.0 (*)
+     +--- org.wordpress.fluxc:fluxc-annotations:trunk-884fbb8e16259765bb490127476813962e92a113
+     +--- androidx.room:room-ktx:2.6.1 (*)
+     \--- org.jetbrains.kotlin:kotlin-stdlib:1.9.25 (*)

Please review and act accordingly

codecov-commenter · 2024-09-11T15:12:19Z

Codecov Report

Attention: Patch coverage is 67.44186% with 14 lines in your changes missing coverage. Please review.

Project coverage is 40.62%. Comparing base (114603f) to head (318fe1f).
Report is 20 commits behind head on trunk.

Files with missing lines	Patch %	Lines
...ocommerce/android/ui/main/MainActivityViewModel.kt	79.16%	2 Missing and 3 partials ⚠️
...tlin/com/woocommerce/android/model/Notification.kt	20.00%	1 Missing and 3 partials ⚠️
...merce/android/notifications/WooNotificationType.kt	50.00%	4 Missing ⚠️
...id/ui/blaze/campaigs/BlazeCampaignListViewModel.kt	83.33%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##              trunk   #12564      +/-   ##
============================================
+ Coverage     40.61%   40.62%   +0.01%     
- Complexity     5665     5669       +4     
============================================
  Files          1228     1228              
  Lines         68917    68944      +27     
  Branches       9544     9549       +5     
============================================
+ Hits          27991    28010      +19     
- Misses        38354    38357       +3     
- Partials       2572     2577       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JorgeMucientes · 2024-09-11T15:47:06Z

Thanks for the feedback @hichamboushaba, I've addressed the main concerns.

About the possible API improvement of converting the WooNotificationType into a sealed interface I've applied the changes in a separate PR. I still need to test that all notifications work as expected, but feel free to take a quick look in case the design has any flaws.

Will proceed to merge this now.

JorgeMucientes added 4 commits September 9, 2024 15:55

Simplifies code handling taps on push notifications

8976180

Open campaign list from Blaze push notification and pass the campaign_id

557e86a

Add missing mapping for Blaze push notification type

2fe3177

Open campaign detail directly when tapping on Blaze notification

8e1e434

JorgeMucientes added type: task An internally driven task. feature: blaze Related to the Blaze project labels Sep 9, 2024

JorgeMucientes mentioned this pull request Sep 9, 2024

Adds new campaign_id field to map from push notification payload wordpress-mobile/WordPress-FluxC-Android#3092

Merged

Updates FluxC changeset

32c26a6

JorgeMucientes added 3 commits September 9, 2024 17:37

Set more consistent naming

24fdc33

Add new tests for Blaze notifications taps

0fe2a3e

Fix detekt indentation issue

58c24cc

JorgeMucientes added 6 commits September 9, 2024 20:46

Ensure campaign detail is loaded when a campaign id is passed

542fb2d

Handle Blaze grouped notifications

72cec8e

Keep the same Blaze notification types received in push payload

9f8fc2a

Fix detekt indentation issues

3684707

Fix existing unit tests after group notification implementation updates

05172e9

Add unit tests fro group notification handling

3b8502d

JorgeMucientes added the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Sep 10, 2024

JorgeMucientes added this to the 20.4 milestone Sep 10, 2024

JorgeMucientes requested review from hichamboushaba and irfano September 10, 2024 12:10

JorgeMucientes commented Sep 10, 2024

View reviewed changes

hichamboushaba self-assigned this Sep 10, 2024

hichamboushaba approved these changes Sep 11, 2024

View reviewed changes

JorgeMucientes added 3 commits September 11, 2024 15:13

Update FluxC changeset

905cf74

Merge branch 'refs/heads/trunk' into issue/12539-blaze-push-notificat…

37069bf

…ion-navigation # Conflicts: # build.gradle

Avoid waiting for campaigns to be loaded before loading campaign detail

9f4dd30

Avoid tracking campaign detail tapped event

12b9b3a

JorgeMucientes removed the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Sep 11, 2024

Fix unit tests compilation issue

318fe1f

JorgeMucientes merged commit 589df25 into trunk Sep 11, 2024
14 checks passed

JorgeMucientes deleted the issue/12539-blaze-push-notification-navigation branch September 11, 2024 15:47

JorgeMucientes mentioned this pull request Sep 16, 2024

Issue/12539 notification type api improvements #12580

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue/12539 blaze push notification navigation #12564

Issue/12539 blaze push notification navigation #12564

JorgeMucientes commented Sep 9, 2024 •

edited

Loading

dangermattic commented Sep 9, 2024 •

edited

Loading

wpmobilebot commented Sep 9, 2024 •

edited

Loading

wpmobilebot commented Sep 9, 2024 •

edited

Loading

JorgeMucientes Sep 10, 2024

hichamboushaba Sep 11, 2024

hichamboushaba left a comment

hichamboushaba Sep 11, 2024

hichamboushaba Sep 11, 2024

hichamboushaba Sep 11, 2024

wpmobilebot commented Sep 11, 2024

codecov-commenter commented Sep 11, 2024 •

edited

Loading

JorgeMucientes commented Sep 11, 2024

Issue/12539 blaze push notification navigation #12564

Issue/12539 blaze push notification navigation #12564

Conversation

JorgeMucientes commented Sep 9, 2024 • edited Loading

Description

Testing information

The tests that have been performed

Images/gif

Reviewer (or Author, in the case of optional code reviews):

dangermattic commented Sep 9, 2024 • edited Loading

wpmobilebot commented Sep 9, 2024 • edited Loading

wpmobilebot commented Sep 9, 2024 • edited Loading

JorgeMucientes Sep 10, 2024

Choose a reason for hiding this comment

hichamboushaba Sep 11, 2024

Choose a reason for hiding this comment

hichamboushaba left a comment

Choose a reason for hiding this comment

hichamboushaba Sep 11, 2024

Choose a reason for hiding this comment

hichamboushaba Sep 11, 2024

Choose a reason for hiding this comment

hichamboushaba Sep 11, 2024

Choose a reason for hiding this comment

wpmobilebot commented Sep 11, 2024

The PR caused the following dependency changes:

codecov-commenter commented Sep 11, 2024 • edited Loading

Codecov Report

JorgeMucientes commented Sep 11, 2024

JorgeMucientes commented Sep 9, 2024 •

edited

Loading

dangermattic commented Sep 9, 2024 •

edited

Loading

wpmobilebot commented Sep 9, 2024 •

edited

Loading

wpmobilebot commented Sep 9, 2024 •

edited

Loading

codecov-commenter commented Sep 11, 2024 •

edited

Loading