Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: display local notification - Part 0 #248

Closed
wants to merge 1 commit into from

Conversation

ohassine
Copy link
Member

What's new in this PR?

Jira US : https://wearezeta.atlassian.net/browse/AR-438

In this PR, I added a new table to user database called notification.
Also I prepared some methods that will be used in next PRs.

We need to store messages (that have to be displayed in notification) in order to group notifications by conversation name.
If we don't store messages somewhere, older notifications will be erased once a newer one is received.

image

@vitorhugods
Copy link
Member

Do we really need to store the notifications?

Can't we do something like the following:

  1. Message comes in in conversation A
  2. Get most recent UNREAD messages from conversation A. Maybe the latest 10 or so, no need to fetch them all.
  3. Show a notification for conversation A based on these messages acquired in step 2.

@ohassine
Copy link
Member Author

Actually, I didn't think in that way. But I think it is a better solution.

The current app is saving notifications in separate table and there is no relation between notifications and unread/read messages. So if the user reads the message from somewhere(web, iOS or whatever), its related notification will not be canceled.

About implementation, should we add two columns (isRead and isDismissed) to the message table.
isDismissed will be used to not display again the notification if it is already dismissed by the user.

cc @mejdoo

@vitorhugods
Copy link
Member

I am not sure we need to worry about dismissing of notifications and the isDismissed field.

At least most chat applications that I see on Android will show the messages again in the notification if there's a new message.

For example:

  1. The user has 4 new messages in Conversation A.
  2. The notification is shown to the user.
  3. User dismisses it.
  4. A new message for Conversation A is received. Total of unread messages is now 5.
  5. A new notification with the 5 unread messages shows up.

At least is how I am used to see it. Isn't it how the app currently works?
There's no point in showing the same 4 again if there isn't any change. But we can trigger the new notification with all the content.

Still, the isRead is needed, of course.

One extra benefit of a similar approach over storing notifications in the database, is that we don't need to store and manage states of two things (notifications and messages), only worrying about messages and if they're read or not.

@ohassine
Copy link
Member Author

Ah yes, I didn't notice about this before with other messaging apps.
Fully agree, it's the right way to do it.

The current app is not doing this, once the notification is dismissed we will not display older unread messages.
And once a message is read, its related notification is not dismissed.

I don't know if we proceed in this way or not, maybe we should ask the product team.
What do you think ? @vitorhugods & @mejdoo

@mejdoo
Copy link
Contributor

mejdoo commented Aug 31, 2021

I am not sure we need to worry about dismissing of notifications and the isDismissed field.

At least most chat applications that I see on Android will show the messages again in the notification if there's a new message.

For example:

  1. The user has 4 new messages in Conversation A.
  2. The notification is shown to the user.
  3. User dismisses it.
  4. A new message for Conversation A is received. Total of unread messages is now 5.
  5. A new notification with the 5 unread messages shows up.

At least is how I am used to see it. Isn't it how the app currently works?
There's no point in showing the same 4 again if there isn't any change. But we can trigger the new notification with all the content.

Still, the isRead is needed, of course.

One extra benefit of a similar approach over storing notifications in the database, is that we don't need to store and manage states of two things (notifications and messages), only worrying about messages and if they're read or not.

It's pretty much clear we need a product decision to change the behaviour of the current app. We can discuss with Eva in the standup today @ohassine

I like this idea of adding isRead field but if there is way to fetch notifications without storing them in the db, that would be better. Otherwise, the content of a notification (message) will be stored twice in notification and message tables.

Another solution to avoid duplications, you can define a relationship between the tables message and notification and remove the relation betweenconversation and notification. But I guess you want to display all the notifications related to one conversation in a single channel right? @ohassine

@ohassine
Copy link
Member Author

Yes in the same group of Messages channel

@ohassine
Copy link
Member Author

Closed based on the discussion above

@ohassine ohassine closed this Aug 31, 2021
@vitorhugods vitorhugods deleted the local_notification_part0 branch September 22, 2021 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants