Fix new notifications broken while notification center is open

changelog/unreleased/bugfix-incoming-notifications-broken-while-notification-center-is-open

@@ -0,0 +1,7 @@
+Bugfix: Incoming notifications broken while notification center is open
+
+We've fixed a bug where the visual representation of new incoming notifications were broken,
+while the notification center was opened.
+
+https://github.com/owncloud/web/pull/10760
+https://github.com/owncloud/web/issues/10602

packages/web-client/src/helpers/call.ts

@@ -1,3 +1,3 @@
-export const call = function* <T>(p: Promise<T>): Generator<Promise<T>, T, T> {
-  return yield p
+export const call = function* <T>(p: T | Promise<T>): Generator<Promise<T>, T, T> {
+  return yield Promise.resolve(p)
 }

packages/web-runtime/src/components/Topbar/Notifications.vue

@@ -17,7 +17,7 @@
           v-if="notifications.length"
           class="oc-notifications-mark-all"
           appearance="raw"
-          @click="deleteNotifications(notifications.map((n) => n.notification_id))"
+          @click="deleteNotificationsTask.perform(notifications.map((n) => n.notification_id))"
           ><span v-text="$gettext('Mark all as read')"
         /></oc-button>
       </div>
@@ -75,20 +75,18 @@
   </div>
 </template>
 <script lang="ts">
-import { onMounted, onUnmounted, ref, unref } from 'vue'
+import { computed, onMounted, onUnmounted, ref, unref } from 'vue'
 import isEmpty from 'lodash-es/isEmpty'
-import { useCapabilityStore, useSpacesStore } from '@ownclouders/web-pkg'
+import { useCapabilityStore, useSpacesStore, createFileRouteOptions } from '@ownclouders/web-pkg'
 import NotificationBell from './NotificationBell.vue'
 import { Notification } from '../../helpers/notifications'
 import {
   formatDateFromISO,
   formatRelativeDateFromISO,
-  useClientService,
-  useRoute
+  useClientService
 } from '@ownclouders/web-pkg'
 import { useGettext } from 'vue3-gettext'
 import { useTask } from 'vue-concurrency'
-import { createFileRouteOptions } from '@ownclouders/web-pkg'
 import { MESSAGE_TYPE } from '@ownclouders/web-client/src/sse'
 
 const POLLING_INTERVAL = 30000
@@ -102,12 +100,14 @@ export default {
     const capabilityStore = useCapabilityStore()
     const clientService = useClientService()
     const { current: currentLanguage } = useGettext()
-    const route = useRoute()
 
     const notifications = ref<Notification[]>([])
-    const loading = ref(false)
     const notificationsInterval = ref()
-    const dropdownOpened = ref(false)
+    const dropdownOpen = ref(false)
+
+    const loading = computed(() => {
+      return fetchNotificationsTask.isRunning || deleteNotificationsTask.isRunning
+    })
 
     const formatDate = (date) => {
       return formatDateFromISO(date, currentLanguage)
@@ -122,7 +122,7 @@ export default {
       { name: 'space', labelAttribute: 'name' },
       { name: 'virus', labelAttribute: 'name' }
     ]
-    const getMessage = ({ message, messageRich, messageRichParameters }: Notification) => {
+    const getMessage = ({ message, messageRich, messageRichParameters }: Notification): string => {
       if (messageRich && !isEmpty(messageRichParameters)) {
         let interpolatedMessage = messageRich
         for (const param of messageParameters) {
@@ -171,17 +171,7 @@ export default {
       return null
     }
 
-    const setAdditionalData = () => {
-      loading.value = true
-      for (const notification of unref(notifications)) {
-        notification.computedMessage = getMessage(notification)
-        notification.computedLink = getLink(notification)
-      }
-      loading.value = false
-    }
-
     const fetchNotificationsTask = useTask(function* (signal) {
-      loading.value = true
       try {
         const response = yield clientService.httpAuthenticated.get(
           'ocs/v2.php/apps/notifications/api/v1/notifications'
@@ -196,29 +186,28 @@ export default {
         } = yield response.data
         notifications.value =
           data?.sort((a, b) => (new Date(b.datetime) as any) - (new Date(a.datetime) as any)) || []
+        unref(notifications).forEach((notification) => setAdditionalNotificationData(notification))
       } catch (e) {
         console.error(e)
-      } finally {
-        if (unref(dropdownOpened)) {
-          setAdditionalData()
-        }
-        loading.value = false
       }
     }).restartable()
 
-    const deleteNotifications = async (ids: string[]) => {
-      loading.value = true
+    const deleteNotificationsTask = useTask(function* (signal, ids) {
       try {
-        await clientService.httpAuthenticated.delete(
+        yield clientService.httpAuthenticated.delete(
           'ocs/v2.php/apps/notifications/api/v1/notifications',
           { data: { ids } }
         )
       } catch (e) {
         console.error(e)
       } finally {
         notifications.value = unref(notifications).filter((n) => !ids.includes(n.notification_id))
-        loading.value = false
       }
+    }).restartable()
+
+    const setAdditionalNotificationData = (notification: Notification) => {
+      notification.computedMessage = getMessage(notification)
+      notification.computedLink = getLink(notification)
     }
 
     const onSSENotificationEvent = (event) => {
@@ -227,18 +216,18 @@ export default {
         if (!notification || !notification.notification_id) {
           return
         }
+        setAdditionalNotificationData(notification)
         notifications.value = [notification, ...unref(notifications)]
       } catch (_) {
         console.error('Unable to parse sse notification data')
       }
     }
 
     const hideDrop = () => {
-      dropdownOpened.value = false
+      dropdownOpen.value = false
     }
     const showDrop = () => {
-      dropdownOpened.value = true
-      setAdditionalData()
+      dropdownOpen.value = true
     }
 
     onMounted(() => {
@@ -270,8 +259,7 @@ export default {
       notifications,
       fetchNotificationsTask,
       loading,
-      dropdownOpened,
-      deleteNotifications,
+      deleteNotificationsTask,
       formatDate,
       formatDateRelative,
       getMessage,

packages/web-runtime/tests/unit/components/Topbar/Notifications.spec.ts

@@ -20,7 +20,8 @@ const selectors = {
 
 vi.mock('@ownclouders/web-pkg', async (importOriginal) => ({
   ...(await importOriginal<any>()),
-  useServerSentEvents: vi.fn()
+  queryItemAsString: vi.fn(),
+  useAppDefaults: vi.fn()
 }))
 
 describe('Notification component', () => {
@@ -31,26 +32,27 @@ describe('Notification component', () => {
     expect(wrapper.find(selectors.markAll).exists()).toBeFalsy()
   })
   it('renders a set of notifications', async () => {
-    const notifications = [mock<Notification>({ messageRich: undefined })]
+    const notifications = [
+      mock<Notification>({
+        messageRich: undefined,
+        computedMessage: undefined,
+        computedLink: undefined
+      })
+    ]
     const { wrapper } = getWrapper({ notifications })
-    await wrapper.vm.fetchNotificationsTask.perform()
     await wrapper.vm.fetchNotificationsTask.last
     expect(wrapper.find(selectors.noNewNotifications).exists()).toBeFalsy()
     expect(wrapper.findAll(selectors.notificationItem).length).toBe(notifications.length)
   })
   it('renders the loading state', async () => {
     const notifications = [mock<Notification>({ messageRich: undefined })]
     const { wrapper } = getWrapper({ notifications })
-    await wrapper.vm.fetchNotificationsTask.perform()
-    await wrapper.vm.fetchNotificationsTask.last
-    wrapper.vm.loading = true
     await wrapper.vm.$nextTick()
     expect(wrapper.find(selectors.notificationsLoading).exists()).toBeTruthy()
   })
   it('marks all notifications as read', async () => {
     const notifications = [mock<Notification>({ messageRich: undefined })]
     const { wrapper, mocks } = getWrapper({ notifications })
-    await wrapper.vm.fetchNotificationsTask.perform()
     await wrapper.vm.fetchNotificationsTask.last
     await wrapper.find(selectors.markAll).trigger('click')
     await wrapper.vm.$nextTick()
@@ -64,7 +66,6 @@ describe('Notification component', () => {
         user: 'einstein'
       })
       const { wrapper } = getWrapper({ notifications: [notification] })
-      await wrapper.vm.fetchNotificationsTask.perform()
       await wrapper.vm.fetchNotificationsTask.last
       const avatarImageStub = wrapper.findComponent<any>(selectors.avatarImageStub)
       expect(avatarImageStub.attributes('userid')).toEqual(notification.user)
@@ -78,7 +79,6 @@ describe('Notification component', () => {
         messageRichParameters: { user: { displayname, name } }
       })
       const { wrapper } = getWrapper({ notifications: [notification] })
-      await wrapper.vm.fetchNotificationsTask.perform()
       await wrapper.vm.fetchNotificationsTask.last
       const avatarImageStub = wrapper.findComponent<any>(selectors.avatarImageStub)
       expect(avatarImageStub.attributes('userid')).toEqual(name)
@@ -89,10 +89,11 @@ describe('Notification component', () => {
     it('displays if no message given', async () => {
       const notification = mock<Notification>({
         messageRich: undefined,
-        message: undefined
+        message: undefined,
+        computedMessage: undefined,
+        computedLink: undefined
       })
       const { wrapper } = getWrapper({ notifications: [notification] })
-      await wrapper.vm.fetchNotificationsTask.perform()
       await wrapper.vm.fetchNotificationsTask.last
       expect(wrapper.find(selectors.notificationSubject).exists()).toBeTruthy()
     })
@@ -101,12 +102,13 @@ describe('Notification component', () => {
     it('displays simple message if messageRich not given', async () => {
       const notification = mock<Notification>({
         messageRich: undefined,
-        message: 'some message'
+        message: 'some message',
+        computedMessage: undefined,
+        computedLink: undefined
       })
       const { wrapper } = getWrapper({ notifications: [notification] })
-      await wrapper.vm.fetchNotificationsTask.perform()
-      await wrapper.vm.fetchNotificationsTask.last
       wrapper.vm.showDrop()
+      await wrapper.vm.fetchNotificationsTask.last
       await wrapper.vm.$nextTick()
       expect(wrapper.find(selectors.notificationMessage).text()).toEqual(notification.message)
     })
@@ -116,12 +118,13 @@ describe('Notification component', () => {
         messageRichParameters: {
           user: { displayname: 'Albert Einstein' },
           resource: { name: 'someFile.txt' }
-        }
+        },
+        computedMessage: undefined,
+        computedLink: undefined
       })
       const { wrapper } = getWrapper({ notifications: [notification] })
-      await wrapper.vm.fetchNotificationsTask.perform()
-      await wrapper.vm.fetchNotificationsTask.last
       wrapper.vm.showDrop()
+      await wrapper.vm.fetchNotificationsTask.last
       await wrapper.vm.$nextTick()
       expect(wrapper.find(selectors.notificationMessage).text()).toEqual(
         'Albert Einstein shared someFile.txt with you'
@@ -132,10 +135,11 @@ describe('Notification component', () => {
     it('displays if given directly', async () => {
       const notification = mock<Notification>({
         messageRich: undefined,
+        computedMessage: undefined,
+        computedLink: undefined,
         link: 'http://some-link.com'
       })
       const { wrapper } = getWrapper({ notifications: [notification] })
-      await wrapper.vm.fetchNotificationsTask.perform()
       await wrapper.vm.fetchNotificationsTask.last
       expect(wrapper.find(selectors.notificationLink).exists()).toBeTruthy()
     })
@@ -148,13 +152,15 @@ describe('Notification component', () => {
             user: { displayname: 'Albert Einstein' },
             resource: { name: 'someFile.txt' },
             share: { id: '1' }
-          }
+          },
+          computedMessage: undefined,
+          computedLink: undefined
         })
         const { wrapper } = getWrapper({ notifications: [notification] })
-        await wrapper.vm.fetchNotificationsTask.perform()
-        await wrapper.vm.fetchNotificationsTask.last
         wrapper.vm.showDrop()
+        await wrapper.vm.fetchNotificationsTask.last
         await wrapper.vm.$nextTick()
+
         const routerLink = wrapper.findComponent<typeof RouterLink>(
           `${selectors.notificationItem} router-link-stub`
         )
@@ -177,12 +183,13 @@ describe('Notification component', () => {
           messageRichParameters: {
             user: { displayname: 'Albert Einstein' },
             space: { name: 'someFile.txt', id: `${spaceMock.fileId}!2` }
-          }
+          },
+          computedMessage: undefined,
+          computedLink: undefined
         })
         const { wrapper } = getWrapper({ notifications: [notification], spaces: [spaceMock] })
-        await wrapper.vm.fetchNotificationsTask.perform()
-        await wrapper.vm.fetchNotificationsTask.last
         wrapper.vm.showDrop()
+        await wrapper.vm.fetchNotificationsTask.last
         await wrapper.vm.$nextTick()
         const routerLink = wrapper.findComponent<typeof RouterLink>(
           `${selectors.notificationItem} router-link-stub`