Allow users to force notifications to be marked as read

[Silver-IT]

Summary of Changes

• Closes Notifications unread status not being sync #2235

[cypress]

Passing run #2819 ↗︎

0

112

10

0

0

Details:

Merge 09f3b3e into eea332d...
Project: group-income	Commit: c894e4dd07 ℹ️
Status: Passed	Duration: 10:37 💡
Started: Jul 26, 2024 7:51 PM	Ended: Jul 26, 2024 8:01 PM

Review all test suite changes for PR #2248 ↗︎

[taoeffect]

Question: I don't see how this PR will help sync the existing status of the notification back to the client. This seems to be modifying the action of marking all notifications as read, but where is the code for grabbing the latest value of a notification's read status? Is that what should be fixed?

[Silver-IT]

Question: I don't see how this PR will help sync the existing status of the notification back to the client. This seems to be modifying the action of marking all notifications as read, but where is the code for grabbing the latest value of a notification's read status? Is that what should be fixed?

Actually, the syncing notifications status is already implemented. I guess the reason of the difference status between two devices is because that there could be a network issue in one device. So it could fail to listen the KV event and to update notification status on this device.
Actually, the syncing contracts could also be failed when there is a network issue. I also faced this kind of problem while I was testing on my side. Syncing an identity contract was failed, and can not display username (or displayname) only for one member. I investigated this issue and noticed that it was because that his identity contract was not fully synced.

So what I did in this PR is to allow users to force updating the notifications read status. When the user clicks Mark all as read, 'gi.actions/identity/kv/markNotificationStatusRead' function will be executed even though the values of KV store are already updated. Actually, this function call won't make any changes on KV store side, but an event will be emitted on backend side which will update the notification status of all the devices where the event is being listened.

Actually, there could be two choices.
One is to allow users to force updating notifications status and to get updated to the latest KV store values
And the other is to call 'gi.actions/identity/kv/loadNotificationStatus' function every time users open the notification card.

I thought the first one is better in terms of performance, so I worked on it.

@taoeffect, do you think there is a real bug in listening KV event? If not, which option do you prefer?

[taoeffect]

And the other is to call 'gi.actions/identity/kv/loadNotificationStatus' function every time users open the notification card.

I don't see how that would help, because this is already called on login by 'gi.actions/identity/kv/load', and yet the notification status isn't updated on page refresh. So can you investigate why that is?

[Silver-IT]

I don't see how that would help, because this is already called on login by 'gi.actions/identity/kv/load', and yet the notification status isn't updated on page refresh.

That makes sense.

So can you investigate why that is?

As I have already said in here, I couldn't have any chance to encounter this issue on my side. I tried many times, but never encountered. So, it is difficult to investigate.

[taoeffect]

As I have already said in #2235 (comment), I couldn't have any chance to encounter this issue on my side. I tried many times, but never encountered. So, it is difficult to investigate.

Here's something you can try:

• use https://localhost.run/
• device 1 (desktop) invites device 2 (phone) to group
• add 3rd user and create proposal
• device 1 has its internet connection turned off
• device 2 marks notification as read
• device 1 comes back online

Then do that again if the problem doesn't reproduce, but switch device 1 and device 2 (e.g. if device 1 was desktop, make it phone).

[Silver-IT]

@taoeffect, with your recommended workflow, I can see the the second device doesn't get updated with the notifications status even though refresh the browser. After some investigation, I have noticed that we should call sbp('gi.actions/identity/kv/load') in other place in main.js. Please have a look at the updates.

[taoeffect]

Great work @Silver-IT !!!

Minor request for error handling please:

[taoeffect]

Actually, one more change request: we want to call sbp('gi.actions/identity/kv/load') when we come back online, so could you please do the following:

1. Add an event to events.js called ONLINE
2. emit this event in main.js in the online handler EDIT: and the 'reconnection-succeeded' handler too
3. listen for this event in identity-kv.js and call sbp('gi.actions/identity/kv/load') in the event listener (please copy the same .catch handler, but modify the error message to make it clear that the error happened in the online handler, instead of during login)

[taoeffect]

When I run grunt test locally on this branch I get this heisenbug error as well: #2257

So this PR seems to result in two common heisenbugs:

• Heisenbug in group-chat.spec.js #2256 on Github
• Heisenbug in group-proposals.spec.js #2257 locally on my machine

[taoeffect]

Great work @Silver-IT ! (And @taoeffect). To deal with the heisenbug I've commented out the problematic line for now and updated issue #2256.