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

Issue/13213 disable notifications api request #13220

Open
wants to merge 9 commits into
base: trunk
Choose a base branch
from

Conversation

JorgeMucientes
Copy link
Contributor

@JorgeMucientes JorgeMucientes commented Dec 30, 2024

⚠️ Requires FluxC changes to be merged first:

Closes: #13213

Description

Integrates new FluxC changes to disable:

  • New order push notifications
  • New product review notifications
    When a site is disabled from the list of visible sites. Notifications will only be disabled for the current device used to hide the sites.

Testing information

Basic steps

  1. Log in with an account that has multiple sites
  2. Click Edit Stores button
  3. Edit selected stores
  4. Tap Save and check the loading spinner at the top while request is fired (you might not see it because the request is super fast).

Notification settings updated successfully case

  1. Test notifications for new orders and new product reviews are disabled for the sites that we unchecked in the prior steps. Notification should still work for those same sites when logged in from another device.
  2. Repeat the steps above and reenable the previously disabled sites. Check push notification are received again.

Notification settings failed to update case

  1. Go to settings > Developer options > Api faker
  2. Add the following endpoint and enable API faker:
  1. Now repeat the 4 steps above and check the following error dialog is displayed:
errorHandling.mp4

The tests that have been performed

Images/gif

(Demo adding delay to properly show the loading spinner)

LoadingState.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.

@JorgeMucientes JorgeMucientes added the feature: site picker Any task related to site picker screen label Dec 30, 2024
@JorgeMucientes JorgeMucientes added this to the 21.4 milestone Dec 30, 2024
@dangermattic
Copy link
Collaborator

dangermattic commented Dec 30, 2024

1 Error
🚫 This PR is tagged with status: do not merge label(s).

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

Project dependencies changes

The following changes in project dependencies were detected (configuration vanillaReleaseRuntimeClasspath):

list
Upgraded Dependencies
org.wordpress.fluxc.plugins:woocommerce:3122-c8c9f508066c0c8165420dee180b7d10cafbfbb6, (changed from trunk-b329e031481c8653a57c0a41c4dedfc0ba6a510e)
org.wordpress.fluxc:fluxc-annotations:3122-c8c9f508066c0c8165420dee180b7d10cafbfbb6, (changed from trunk-b329e031481c8653a57c0a41c4dedfc0ba6a510e)
org.wordpress:fluxc:3122-c8c9f508066c0c8165420dee180b7d10cafbfbb6, (changed from trunk-b329e031481c8653a57c0a41c4dedfc0ba6a510e)
tree
-+--- org.wordpress:fluxc:trunk-b329e031481c8653a57c0a41c4dedfc0ba6a510e
-|    +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.25 -> 2.0.21
-|    |    +--- org.jetbrains.kotlin:kotlin-stdlib:2.0.21 (*)
-|    |    \--- org.jetbrains.kotlin:kotlin-android-extensions-runtime:2.0.21
-|    |         \--- org.jetbrains.kotlin:kotlin-stdlib:2.0.21 (*)
-|    +--- androidx.exifinterface:exifinterface:1.0.0 -> 1.3.6
-|    |    \--- androidx.annotation:annotation:1.2.0 -> 1.8.1 (*)
-|    +--- androidx.security:security-crypto:1.0.0 -> 1.1.0-alpha03
-|    |    +--- androidx.annotation:annotation:1.1.0 -> 1.8.1 (*)
-|    |    +--- 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 -> 2.0.21 (*)
-|    |    +--- 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.1 (*)
-|    |    \--- org.wordpress.wellsql:wellsql-annotations:2.0.0
-|    +--- org.wordpress.fluxc:fluxc-annotations:trunk-b329e031481c8653a57c0a41c4dedfc0ba6a510e
-|    +--- org.greenrobot:eventbus:3.3.1 (*)
-|    +--- 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.1 (*)
-|    |    |    \--- 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.8.7 (*)
-|    |    +--- androidx.lifecycle:lifecycle-livedata:2.0.0 -> 2.8.7 (*)
-|    |    \--- 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 -> 2.0.21 (*)
++--- org.wordpress:fluxc:3122-c8c9f508066c0c8165420dee180b7d10cafbfbb6
+|    +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.25 -> 2.0.21
+|    |    +--- org.jetbrains.kotlin:kotlin-stdlib:2.0.21 (*)
+|    |    \--- org.jetbrains.kotlin:kotlin-android-extensions-runtime:2.0.21
+|    |         \--- org.jetbrains.kotlin:kotlin-stdlib:2.0.21 (*)
+|    +--- androidx.exifinterface:exifinterface:1.0.0 -> 1.3.6
+|    |    \--- androidx.annotation:annotation:1.2.0 -> 1.8.1 (*)
+|    +--- androidx.security:security-crypto:1.0.0 -> 1.1.0-alpha03
+|    |    +--- androidx.annotation:annotation:1.1.0 -> 1.8.1 (*)
+|    |    +--- 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 -> 2.0.21 (*)
+|    |    +--- 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.1 (*)
+|    |    \--- org.wordpress.wellsql:wellsql-annotations:2.0.0
+|    +--- org.wordpress.fluxc:fluxc-annotations:3122-c8c9f508066c0c8165420dee180b7d10cafbfbb6
+|    +--- org.greenrobot:eventbus:3.3.1 (*)
+|    +--- 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.1 (*)
+|    |    |    \--- 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.8.7 (*)
+|    |    +--- androidx.lifecycle:lifecycle-livedata:2.0.0 -> 2.8.7 (*)
+|    |    \--- 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 -> 2.0.21 (*)
-\--- org.wordpress.fluxc.plugins:woocommerce:trunk-b329e031481c8653a57c0a41c4dedfc0ba6a510e
-     +--- org.wordpress:fluxc:trunk-b329e031481c8653a57c0a41c4dedfc0ba6a510e (*)
-     +--- 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-b329e031481c8653a57c0a41c4dedfc0ba6a510e
-     +--- androidx.room:room-ktx:2.6.1 (*)
-     \--- org.jetbrains.kotlin:kotlin-stdlib:1.9.25 -> 2.0.21 (*)
+\--- org.wordpress.fluxc.plugins:woocommerce:3122-c8c9f508066c0c8165420dee180b7d10cafbfbb6
+     +--- org.wordpress:fluxc:3122-c8c9f508066c0c8165420dee180b7d10cafbfbb6 (*)
+     +--- 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:3122-c8c9f508066c0c8165420dee180b7d10cafbfbb6
+     +--- androidx.room:room-ktx:2.6.1 (*)
+     \--- org.jetbrains.kotlin:kotlin-stdlib:1.9.25 -> 2.0.21 (*)

@JorgeMucientes JorgeMucientes added the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Dec 30, 2024
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Dec 30, 2024

📲 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
FlavorJalapeno
Build TypeDebug
Commit7b973cc
Direct Downloadwoocommerce-wear-prototype-build-pr13220-7b973cc.apk

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Dec 30, 2024

📲 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
FlavorJalapeno
Build TypeDebug
Commit7b973cc
Direct Downloadwoocommerce-prototype-build-pr13220-7b973cc.apk

Base automatically changed from issue/13163-logic-to-save-hidden-sites to trunk December 30, 2024 12:35
@JorgeMucientes JorgeMucientes marked this pull request as ready for review December 30, 2024 16:00
@JorgeMucientes JorgeMucientes requested review from irfano, hichamboushaba and hafizrahman and removed request for irfano December 30, 2024 16:00
@hafizrahman
Copy link
Contributor

Testing notes:

Both success and failure test cases work in my test. With success, I can confirm that testing with two devices (one Simulator and one real), the one with disabled notification did not receive it while the other received it.

With the failure I tested with API faker and it worked as well.

Additionally, I like the loading animation being put on the button (which conveniently hides the button too), and the retry dialog is simple but effective 👍🏼

testBlocking {
val event = viewModel.event.runAndCaptureValues {
viewModel.onSaveTapped()
}.last()

assertThat(event).isEqualTo(ExitWithResult(data = true))
}

@Test
fun `given updating notification settings fails, when tapping save, then exit with result`() =
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, should be then show error dialog.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Fixed!

Copy link
Contributor

@hafizrahman hafizrahman left a comment

Choose a reason for hiding this comment

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

code looks good, pre-approving 👍🏼 👍🏼

left a small comment about unit test name but all good otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: site picker Any task related to site picker screen status: do not merge Dependent on another PR, ready for review but not ready for merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ignore push notifications for hidden sites
4 participants