fix(messenger): make sure chats have an unread count of 0 for channels you can't view #5062
Conversation
Jenkins BuildsClick to see older builds (21)
|
There was a problem hiding this comment.
Thanks for precise description.
As far as I can see, current fix proposal will not work at runtime. Meaning, if the app is already opened and the permission to channel is lost, unview counts will stay as they are.
Also, keeping the DB value means that if the channel goes back to visible to us, we'll have a badge for the previous unread messages we had.
I am not sure about that assumption. The cache you modified will be eventually saved to the db and the values will be zeroed there as well.
Another solution would be to change the DB value of the chat when we receive a new permission and detect that we no longer have view access. It's cleaner, but it doesn't fix previous account having the issue.
I think the issue is more complicated than assumed. unviewed_message_count and unviewed_mentions_count are denormalized fields and can be modified in BlockContact for instance. So if you block contact that wrote a message on channel you have no permissions to, the unread counts will be back there.
I am thinking about the following solution:
- Add
canViewcolumn to chats' db - Modify the logic of denormalized fields recalculation, so that it zeroes when
!chat.canView- this will ensure no surprises as described above - Whenever
chat.canViewis changed, trigger denormalized fields recalculation - Keep changes in
Initas you did + updatechat.canView
Ideally point 4) should be done through migration but since we keep community description marshaled I don't think it is even possible.
The points 1-3 are good candidates to be addressed in another PR I suppose.
You're right.
Good point, I hadn't thought about it.
Indeed, I was thinking to see if there was a possible migration that would fix this in one pass and not think about it anymore, but it's impossible with how the Community description is marshalled.
I think to correctly fix the issue, I'll need to implement some of the points from 1-3 right away. Otherwise, like you said, we'll be missing the update on runtime. Thanks for the great insight! |
jrainville
force-pushed
the
fix/unread-msgs-in-hidden-chats
branch
from
0e244ed
to
a8000ed
Compare
|
Alright, I pushed a new commit that addresses the problem that when the channel becomes unavailable, the count wouldn't update. I fixed it by checking if we are removed from a community channel, then we reset the counts to 0. I'll keep the changes in Init too to fix people who had the problem happen to them before this fix (since we can't easily do a migration). These changes don't address putting back the old count when the channel is back available, but IMO it's an edge case plus not that useful, so it's not worth recalculating. As for the other points to improve, I think they will be mostly addressed in this refactor to align CommunityChats and Chats (adding canView, canPost, etc to the Chat) status-im/status-desktop#14219 |
jrainville
force-pushed
the
fix/unread-msgs-in-hidden-chats
branch
3 times, most recently
from
6f6ab1c
to
804c8f8
Compare
|
Now that you're back @osmaczko you can review again 😄 |
protocol/messenger_communities.go
Outdated
| // Check if we have been removed from a chat (ie no longer have access) | ||
| for id, changes := range communityResponse.Changes.ChatsModified { | ||
| if _, ok := changes.MembersRemoved[common.PubkeyToHex(&m.identity.PublicKey)]; ok { | ||
| chatID := community.IDString() + id |
There was a problem hiding this comment.
| chatID := community.IDString() + id | |
| chatID := community.ChatID(id) |
protocol/messenger_communities.go
Outdated
| @@ -3089,6 +3089,24 @@ func (m *Messenger) handleCommunityResponse(state *ReceivedMessageState, communi | |||
| } | |||
| } | |||
|
|
|||
| // Check if we have been removed from a chat (ie no longer have access) | |||
| for id, changes := range communityResponse.Changes.ChatsModified { | |||
There was a problem hiding this comment.
| for id, changes := range communityResponse.Changes.ChatsModified { | |
| for channelID, changes := range communityResponse.Changes.ChatsModified { |
…ls you can't view Fixes status-im/status-desktop#14421 The problem is that you can receive messages to a channel, then later, before marking them as read, a permission is added to them, so you no longer have access. Then, you can't even mark it as read if it's hidden. Here, I fix it by setting the unread count on Init at 0 if the user doesn't have view access to it. And I make sure we update the counts when we are removed from a channel
jrainville
force-pushed
the
fix/unread-msgs-in-hidden-chats
branch
from
804c8f8
to
b7e045e
Compare
Fixes status-im/status-desktop#14421
The problem is that you can receive messages to a channel, then later, before marking them as read, a permission is added to them, so you no longer have access. Then, you can't even mark it as read if it's hidden. Here, I fix it by setting the unread count on Init at 0 if the user doesn't have view access to it.
It's not the cleanest solution, but it's the simplest one to fix previous accounts that had that issue.
Another solution would be to change the DB value of the chat when we receive a new permission and detect that we no longer have view access. It's cleaner, but it doesn't fix previous account having the issue.
Also, keeping the DB value means that if the channel goes back to visible to us, we'll have a badge for the previous unread messages we had.
Let me know what you guys think.