Issue #1985 - notifications reader

[maxceem]

Issue #1985 - notifications reader

• Implemented general NotificationsReader which marks notifications as read by providing a criteria:
  • NotificationReader is not connected to store directly
  • NotifcationsReader only dispatches action when there are any notifications to be marked as read, otherwise it doesn't dispatch any actions
  • notifications are being marked as read, only at the time when NotifcationsReader is mounted. This apeears to cover all our current cases. There is no need to listen for new notifications.
• NotificationsReader marks notifications as read per this table https://docs.google.com/spreadsheets/d/1olFRCgXyZy6DTBWmFCJF-MXuhxh80y5JWOKvlSpgguU/edit#gid=1005864840. Note, that there are notifications marked by red color. They don't come to Connect App currently.
• Important. The way how we store notifications in the redux store was refactored.
  • before: we stored already bundled notifications with rendered texts
  • after: we store notifications as they come from the server (with preprocessing) but without bundling and without texts rendered.
  • Reasons:
    • NotificationReader mark notifications as per notification NOT per bundle. If we don't have original notifications it becomes complicated to properly updated state.
    • We wanted to have ALL notifications in the store, not only notifications which are visible. The new implementation separates notifications storing and notifications displaying. Now we store all the notifications in the store, while displaying only the notifications which has rules of how to render their texts.
• added missed goTo for some notifications

[RishiRajSahu]

Overall I liked the idea of putting NotificationsReader inside each container with some criteria to mark the notifications as read. Correct Me if I am wrong, So when user opens the project plan page, along with project plan notifications, notifications for timeline and phase-card will also be marked as read ?

[maxceem]

In general, NotificationReader markes notifications as read when it's rendered i. e. visible for user.

In particular for project plan page, when user opens it, notifications related to project plan and phase cards are marked as read, but not for timelines as timelines are not visible initially. Only when user open timeline tab related notifications will be marked as read.

Note, that timeline tab suppose to have red cue shown if there are some unread notifications related to timeline, so if we mark such notifications before user openes timeline, he will never see a red cue.

[RishiRajSahu]

Note, that timeline tab suppose to have red cue shown if there are some unread notifications related to timeline, so if we mark such notifications before user opens timeline, he will never see a red cue.

Correct we should not mark them as read on load of project-plan. I am just thinking how the cue will be handled/behave when we open timeline, will it disappear immediately, anyway we can take care that in our other issue

In particular for project plan page, when user opens it, notifications related to project plan and phase cards are marked as read

Actually why i raised the first review comment is, I am curious how we are going to handle visual cues for phases due to notifications regarding phase-cards ?

Cue on phase-card will be result of both the phase-card and timeline notification

[maxceem]

I am just thinking how the cue will be handled/behave when we open timeline, will it disappear immediately, anyway we can take care that in our other issue

So far cue will disappear NOT immediately, but after server request is finished. But it will be fixed after the issue #2649 - Hide dismissed notifications without waiting for server response is done. During that issue I'm going to slightly refactor actions, so we will get this behavior for all the cases. I mean everywhere notifications will be marked as read with immediate effect in Connect App. And with reverting read state if an error happened during server request.

Actually why i raised the first review comment is, I am curious how we are going to handle visual cues for phases due to notifications regarding phase-cards ?

So far per current implementation it should work as expected, i. e.:

• notifications regarding phase card will be marked as read on page render - no red cue for such cases
• red cue for a phase will be shown if any of tabs have unread notifications: timeline, discussions or specification this will be completed in the issue #2455 - Add notification visual cues to phase timeline and specifications tabs. And it already works for discussions.

[RishiRajSahu]

Okay sounds good to me, could we create a separate issue to keep track of phase specific notifications ?

notifications regarding phase card will be marked as read on page render - no red cue for such cases

[maxceem]

Okay sounds good to me, could we create a separate issue to keep track of phase specific notifications ?

Sure, what is the issue there?

[RishiRajSahu]

red cue for a phase will be shown if any of tabs have unread notifications: timeline, discussions or specification

EVENT_TYPE.PROJECT_PLAN.PHASE_PAYMENT_UPDATED 
EVENT_TYPE.PROJECT_PLAN.PHASE_PROGRESS_UPDATED 

should we include the above notifications as well to show red-cue or not ? Also have to check whether the above progress event always coincides with the timeline progress event or not ? if it coincides we can ignore phase progress updated event.

[maxceem]

should we include the above notifications as well to show red-cue or not ?

So far there were not such requirements in the related issues:

• issue #2455 - Add notification visual cues to phase timeline and specifications tabs
• issue #2410 - Add notification visual cues to project plan cards

Note, that per issue #2401 only notifications inside tabs affect red cue on the phase card. As per requirements, when we open phase, red cue on the phase disappears and is shown on the particular tab instead. Which implies that phase card by itself doesn't have a red cue.

should we include the above notifications as well to show red-cue or not ?

Coming back to your question. I understand your intention to show red cue for a phase due to phase related notifications. But I don't have a strong opinon on whether we should show it here or no.
I think this can be general question of how we higlight places which were updated but user actually see them immediately when he comes to the page.

[RishiRajSahu]

@maxceem thanks, I have created an issue to discuss further on this #2660