Merge pull request #20649 from wordpress-mobile/refactor/mark-all-not…

…es-read

Refactor Mark all as read in notifications

WordPress/src/main/java/org/wordpress/android/models/Note.kt

@@ -224,9 +224,13 @@ class Note {
      * Setters
      */
 
-    fun setRead() {
+    fun setRead() = updateReadState(1)
+
+    fun setUnread() = updateReadState(0)
+
+    private fun updateReadState(read: Int) {
         try {
-            mNoteJSON?.putOpt("read", 1)
+            mNoteJSON?.putOpt("read", read)
         } catch (e: JSONException) {
             AppLog.e(AppLog.T.NOTIFS, "Failed to set 'read' property", e)
         }

WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsListViewModel.kt

@@ -6,6 +6,8 @@ import androidx.lifecycle.MutableLiveData
 import dagger.hilt.android.lifecycle.HiltViewModel
 import kotlinx.coroutines.CoroutineDispatcher
 import kotlinx.coroutines.flow.MutableSharedFlow
+import kotlinx.coroutines.withContext
+import org.wordpress.android.R
 import org.wordpress.android.datasets.wrappers.NotificationsTableWrapper
 import org.wordpress.android.datasets.wrappers.ReaderPostTableWrapper
 import org.wordpress.android.fluxc.model.SiteModel
@@ -16,6 +18,7 @@ import org.wordpress.android.fluxc.utils.AppLogWrapper
 import org.wordpress.android.models.Note
 import org.wordpress.android.models.Notification.PostLike
 import org.wordpress.android.modules.BG_THREAD
+import org.wordpress.android.modules.UI_THREAD
 import org.wordpress.android.push.GCMMessageHandler
 import org.wordpress.android.ui.jetpackoverlay.JetpackFeatureRemovalOverlayUtil
 import org.wordpress.android.ui.jetpackoverlay.JetpackOverlayConnectedFeature.NOTIFICATIONS
@@ -28,6 +31,8 @@ import org.wordpress.android.ui.reader.actions.ReaderActions
 import org.wordpress.android.ui.reader.actions.ReaderPostActionsWrapper
 import org.wordpress.android.util.AppLog
 import org.wordpress.android.util.EventBusWrapper
+import org.wordpress.android.util.NetworkUtilsWrapper
+import org.wordpress.android.util.ToastUtilsWrapper
 import org.wordpress.android.viewmodel.Event
 import org.wordpress.android.viewmodel.ScopedViewModel
 import javax.inject.Inject
@@ -36,9 +41,12 @@ import javax.inject.Named
 @HiltViewModel
 class NotificationsListViewModel @Inject constructor(
     @Named(BG_THREAD) bgDispatcher: CoroutineDispatcher,
+    @Named(UI_THREAD) private val mainDispatcher: CoroutineDispatcher,
     private val appPrefsWrapper: AppPrefsWrapper,
     private val jetpackFeatureRemovalOverlayUtil: JetpackFeatureRemovalOverlayUtil,
     private val gcmMessageHandler: GCMMessageHandler,
+    private val networkUtilsWrapper: NetworkUtilsWrapper,
+    private val toastUtilsWrapper: ToastUtilsWrapper,
     private val notificationsUtilsWrapper: NotificationsUtilsWrapper,
     private val appLogWrapper: AppLogWrapper,
     private val siteStore: SiteStore,
@@ -81,16 +89,34 @@ class NotificationsListViewModel @Inject constructor(
         appPrefsWrapper.notificationPermissionsWarningDismissed = false
     }
 
-    fun markNoteAsRead(context: Context, notes: List<Note>) {
+    fun markNoteAsRead(context: Context, notes: List<Note>) = launch {
+        if (networkUtilsWrapper.isNetworkAvailable().not()) {
+            withContext(mainDispatcher) {
+                toastUtilsWrapper.showToast(R.string.error_network_connection)
+            }
+            return@launch
+        }
         notes.filter { it.isUnread }
             .map {
                 gcmMessageHandler.removeNotificationWithNoteIdFromSystemBar(context, it.id)
-                notificationsActionsWrapper.markNoteAsRead(it)
-                it.setRead()
-                it
-            }.takeIf { it.isNotEmpty() }?.let {
-                notificationsTableWrapper.saveNotes(it, false)
+                it.apply { setRead() }
+            }.takeIf { it.isNotEmpty() }?.let { notes ->
+                // update the UI before the API request
+                notificationsTableWrapper.saveNotes(notes, false)
                 eventBusWrapper.post(NotificationsChanged())
+                // mark notes as read, this might wait for a long time
+                notificationsActionsWrapper.markNoteAsRead(notes)?.let { result ->
+                    if (result.isError) {
+                        appLogWrapper.e(AppLog.T.NOTIFS, "Failed to mark notes as read: ${result.error}")
+                        // revert the UI changes and display the error message
+                        val revertedNotes = notes.map { it.apply { setUnread() } }
+                        notificationsTableWrapper.saveNotes(revertedNotes, false)
+                        eventBusWrapper.post(NotificationsChanged())
+                        withContext(mainDispatcher) {
+                            toastUtilsWrapper.showToast(R.string.error_generic)
+                        }
+                    }
+                }
             }
     }
 

WordPress/src/main/java/org/wordpress/android/ui/notifications/utils/NotificationsActionsWrapper.kt

@@ -1,13 +1,18 @@
 package org.wordpress.android.ui.notifications.utils
 
 import dagger.Reusable
+import org.wordpress.android.fluxc.model.notification.NotificationModel
+import org.wordpress.android.fluxc.store.NotificationStore
 import org.wordpress.android.models.Note
+import org.wordpress.android.util.AppLog
 import javax.inject.Inject
 import kotlin.coroutines.resume
 import kotlin.coroutines.suspendCoroutine
 
 @Reusable
-class NotificationsActionsWrapper @Inject constructor() {
+class NotificationsActionsWrapper @Inject constructor(
+    private val notificationStore: NotificationStore
+) {
     suspend fun downloadNoteAndUpdateDB(noteId: String): Boolean =
         suspendCoroutine { continuation ->
             NotificationsActions.downloadNoteAndUpdateDB(
@@ -16,7 +21,21 @@ class NotificationsActionsWrapper @Inject constructor() {
                 { continuation.resume(true) })
         }
 
-    fun markNoteAsRead(note: Note?) {
-        NotificationsActions.markNoteAsRead(note)
+    @Suppress("TooGenericExceptionCaught")
+    suspend fun markNoteAsRead(notes: List<Note>): NotificationStore.OnNotificationChanged? {
+        val noteIds = notes.map {
+            try {
+                it.id.toLong()
+            } catch (ex: Exception) {
+                // id might be empty
+                AppLog.e(AppLog.T.NOTIFS, "Error parsing note id: ${it.id}", ex)
+                -1L
+            }
+        }.filter { it != -1L }
+        if (noteIds.isEmpty()) return null
+
+        return notificationStore.markNotificationsRead(
+            NotificationStore.MarkNotificationsReadPayload(noteIds.map { NotificationModel(remoteNoteId = it) })
+        )
     }
 }

WordPress/src/main/java/org/wordpress/android/util/ToastUtilsWrapper.kt

@@ -0,0 +1,12 @@
+package org.wordpress.android.util
+
+import androidx.annotation.StringRes
+import dagger.Reusable
+import org.wordpress.android.WordPress
+import javax.inject.Inject
+
+@Reusable
+class ToastUtilsWrapper @Inject constructor() {
+    fun showToast(@StringRes messageRes: Int) =
+        ToastUtils.showToast(WordPress.getContext(), messageRes)
+}

WordPress/src/test/java/org/wordpress/android/ui/notifications/NotificationsListViewModelTest.kt

@@ -21,6 +21,7 @@ import org.wordpress.android.fluxc.model.SiteModel
 import org.wordpress.android.fluxc.store.AccountStore
 import org.wordpress.android.fluxc.store.CommentStore
 import org.wordpress.android.fluxc.store.CommentsStore
+import org.wordpress.android.fluxc.store.NotificationStore
 import org.wordpress.android.fluxc.store.SiteStore
 import org.wordpress.android.fluxc.utils.AppLogWrapper
 import org.wordpress.android.models.Note
@@ -33,6 +34,8 @@ import org.wordpress.android.ui.prefs.AppPrefsWrapper
 import org.wordpress.android.ui.reader.actions.ReaderActions
 import org.wordpress.android.ui.reader.actions.ReaderPostActionsWrapper
 import org.wordpress.android.util.EventBusWrapper
+import org.wordpress.android.util.NetworkUtilsWrapper
+import org.wordpress.android.util.ToastUtilsWrapper
 
 private const val REQUEST_BLOG_LISTENER_PARAM_POSITION = 2
 
@@ -76,7 +79,13 @@ class NotificationsListViewModelTest : BaseUnitTest() {
     private lateinit var commentStore: CommentsStore
 
     @Mock
-    private lateinit var  accountStore: AccountStore
+    private lateinit var accountStore: AccountStore
+
+    @Mock
+    private lateinit var toastUtilsWrapper: ToastUtilsWrapper
+
+    @Mock
+    private lateinit var networkUtilsWrapper: NetworkUtilsWrapper
 
     @Mock
     private lateinit var action: ActionHandler
@@ -86,10 +95,13 @@ class NotificationsListViewModelTest : BaseUnitTest() {
     @Before
     fun setup() {
         viewModel = NotificationsListViewModel(
+            testDispatcher(),
             testDispatcher(),
             appPrefsWrapper,
             jetpackFeatureRemovalOverlayUtil,
             gcmMessageHandler,
+            networkUtilsWrapper,
+            toastUtilsWrapper,
             notificationsUtilsWrapper,
             appLogWrapper,
             siteStore,
@@ -104,33 +116,37 @@ class NotificationsListViewModelTest : BaseUnitTest() {
     }
 
     @Test
-    fun `WHEN marking a note as read THEN the note is marked as read and the notification removed from system bar`() {
-        // Given
-        val noteId = "1"
-        val context: Context = mock()
-        val note = mock<Note>()
-        whenever(note.id).thenReturn(noteId)
-        whenever(note.isUnread).thenReturn(true)
-
-        // When
-        viewModel.markNoteAsRead(context, listOf(note))
-
-        // Then
-        verify(gcmMessageHandler, times(1)).removeNotificationWithNoteIdFromSystemBar(context, noteId)
-        verify(notificationsActionsWrapper, times(1)).markNoteAsRead(note)
-        verify(note, times(1)).setRead()
-        verify(notificationsTableWrapper, times(1)).saveNotes(listOf(note), false)
-        verify(eventBusWrapper, times(1)).post(any())
-    }
+    fun `WHEN marking a note as read THEN the note is marked as read and the notification removed from system bar`() =
+        test {
+            // Given
+            val noteId = "1"
+            val context: Context = mock()
+            val note = mock<Note>()
+            val notes = listOf(note)
+            whenever(note.id).thenReturn(noteId)
+            whenever(note.isUnread).thenReturn(true)
+            whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(true)
+
+            // When
+            viewModel.markNoteAsRead(context, notes)
+
+            // Then
+            verify(gcmMessageHandler, times(1)).removeNotificationWithNoteIdFromSystemBar(context, noteId)
+            verify(note, times(1)).setRead()
+            verify(notificationsActionsWrapper).markNoteAsRead(notes)
+            verify(notificationsTableWrapper, times(1)).saveNotes(notes, false)
+            verify(eventBusWrapper, times(1)).post(any())
+        }
 
     @Test
-    fun `WHEN marking a note as read THEN the read note is saved`() {
+    fun `WHEN marking a note as read THEN the read note is saved`() = test {
         // Given
         val noteId = "1"
         val context: Context = mock()
         val note = mock<Note>()
         whenever(note.id).thenReturn(noteId)
         whenever(note.isUnread).thenReturn(true)
+        whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(true)
 
         // When
         viewModel.markNoteAsRead(context, listOf(note))
@@ -141,7 +157,7 @@ class NotificationsListViewModelTest : BaseUnitTest() {
     }
 
     @Test
-    fun `WHEN marking all as read THEN only the unread notes are marked as read and saved`() {
+    fun `WHEN marking all as read THEN only the unread notes are marked as read and saved`() = test {
         // Given
         val noteId1 = "1"
         val noteId2 = "2"
@@ -151,19 +167,57 @@ class NotificationsListViewModelTest : BaseUnitTest() {
         whenever(note1.id).thenReturn(noteId1)
         whenever(note1.isUnread).thenReturn(true)
         whenever(note2.isUnread).thenReturn(false)
+        whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(true)
 
         // When
         viewModel.markNoteAsRead(context, listOf(note1, note2))
 
         // Then
         verify(gcmMessageHandler, times(1)).removeNotificationWithNoteIdFromSystemBar(context, noteId1)
-        verify(notificationsActionsWrapper, times(1)).markNoteAsRead(note1)
         verify(note1, times(1)).setRead()
         verify(gcmMessageHandler, times(0)).removeNotificationWithNoteIdFromSystemBar(context, noteId2)
-        verify(notificationsActionsWrapper, times(0)).markNoteAsRead(note2)
         verify(note2, times(0)).setRead()
         verify(notificationsTableWrapper, times(1)).saveNotes(listOf(note1), false)
         verify(eventBusWrapper, times(1)).post(any())
+        verify(notificationsActionsWrapper, times(1)).markNoteAsRead(listOf(note1))
+    }
+
+    @Test
+    fun `GIVEN a interrupted network WHEN marking a note as read THEN show a network error toast`() = test {
+        // Given
+        val note = mock<Note>()
+        whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(false)
+
+        // When
+        viewModel.markNoteAsRead(mock(), listOf(note))
+
+        // Then
+        verify(toastUtilsWrapper, times(1)).showToast(any())
+    }
+
+    @Test
+    fun `GIVEN a stable network WHEN making a note as read fails THEN show a generic error toast`() = test {
+        // Given
+        val note = mock<Note>()
+        whenever(note.id).thenReturn("123")
+        whenever(note.isUnread).thenReturn(true)
+        whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(true)
+        whenever(notificationsActionsWrapper.markNoteAsRead(listOf(note))).thenReturn(
+            NotificationStore.OnNotificationChanged(1).apply {
+                error = NotificationStore.NotificationError(
+                    NotificationStore.NotificationErrorType.GENERIC_ERROR, "error"
+                )
+            }
+        )
+
+        // When
+        viewModel.markNoteAsRead(mock(), listOf(note))
+
+        // Then
+        verify(gcmMessageHandler).removeNotificationWithNoteIdFromSystemBar(any(), eq("123"))
+        verify(notificationsTableWrapper, times(2)).saveNotes(any(), eq(false))
+        verify(eventBusWrapper, times(2)).post(any())
+        verify(toastUtilsWrapper, times(1)).showToast(any())
     }
 
     @Test
@@ -230,7 +284,8 @@ class NotificationsListViewModelTest : BaseUnitTest() {
         whenever(note.commentId).thenReturn(commentId)
         whenever(commentStore.likeComment(site, commentId, null, true)).thenReturn(
             CommentsStore.CommentsActionPayload(
-                CommentStore.CommentError(CommentStore.CommentErrorType.GENERIC_ERROR,"error"), null)
+                CommentStore.CommentError(CommentStore.CommentErrorType.GENERIC_ERROR, "error"), null
+            )
         )
 
         // When