Issue/woomob 1909 add new local feature flag for woo notifications disabled by#15157
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.
|
| message = "Registering FCM token in Woo Core instance${if (BuildConfig.DEBUG) ": $token" else ""}" | ||
| ) | ||
| selectedSite.getIfExists()?.let { | ||
| val uuid = appPrefsWrapper.wooCorePushDeviceUUID.orNullIfEmpty() ?: generateAndStoreUUID() |
There was a problem hiding this comment.
For now we are reusing the existing logic to create the device UUID. Asked Hannah here: https://wp.me/pe5sF9-4OM#comment-5081 if the current strategy is good enough.
|
|
||
| WOO_POS_LOCAL_CATALOG_FILE_APPROACH -> false | ||
| WOO_POS_LOCAL_CATALOG_FILE_APPROACH, | ||
| WOO_PUSH_NOTIFICATIONS_SYSTEM -> false |
There was a problem hiding this comment.
Disabled by default for now to avoid breaking PN for all devs until the endpoint starts working
…g-for-woo-notifications-disabled-by # Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/AppPrefs.kt
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #15157 +/- ##
============================================
- Coverage 38.69% 38.68% -0.01%
Complexity 10548 10548
============================================
Files 2192 2193 +1
Lines 124712 124741 +29
Branches 17243 17248 +5
============================================
+ Hits 48258 48260 +2
- Misses 71576 71602 +26
- Partials 4878 4879 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
irfano
left a comment
There was a problem hiding this comment.
LGTM! I just have one question before approving: why do we need to kill and restart the app to trigger the push-tokens endpoint? Shouldn’t it be triggered right after the first login?
WooCommerce/src/main/kotlin/com/woocommerce/android/notifications/push/RegisterDevice.kt
Outdated
Show resolved
Hide resolved
...rce/src/main/kotlin/com/woocommerce/android/notifications/push/PushNotificationRepository.kt
Outdated
Show resolved
Hide resolved
|
Version |
…g-for-woo-notifications-disabled-by
So the problem is that this new enpoint requires a site to be selected first in order to register the PN token. So, on log in, by the time |
irfano
left a comment
There was a problem hiding this comment.
So, on log in, by the time RegisterDevice.invoke() is called, there's no selected site yet. But this is not a big deal given PN token registration will be attempted the next time you resume the app (you don't actually have to kill the app, just send it to the background).
This is also the current behavior for the wpcom push registration, right? If so, we can keep improving it outside this project’s scope.
Not really @irfano. Because registering for WP.com push notifications doesn't require a site to be selected. So the device registration for WP.com system will happen right away. In any case agree we can improve this in subsequent PRs and I don't think is a big deal tbh, as soon as they resume the app from background the next time they'll get registered. |
Closes: WOOMOB-1909
CLoses: WOOMOB-1910
Description
This PR introduces a new feature flag for the new Woo core PN system and wires it into the existing registration logic so that, when enabled, the app uses the new PN registration endpoint instead of the WP.com one.
This PR doesn't include the changes required to display any PN sent through the new system. Neither includes any fallback strategy for cases where PN toke registration fails. That is the reason why the feature flag is disabled by default for now.
NOT included in this PRs:
Test Steps