Cloudkibo / KiboPush

0 stars 1 forks source link

Push Notifications for Live Chat #8666

Open AnishaChhatwani opened 4 years ago

AnishaChhatwani commented 4 years ago

This task is to write logic to show notifications on mobile whenever a subscriber messages

jekram commented 4 years ago

why no update? Every task on your deck should have an update

arveenkumar55 commented 4 years ago

On last working day(Thursday) I didn't work on this task.

arveenkumar55 commented 4 years ago

This is in Progress.

arveenkumar55 commented 4 years ago

I have created a small document(https://docs.google.com/document/d/1oDqA5aQ393kKZYQdGDJSYe99sJmfTbzsCYYYSJPYpMI/edit?usp=sharing) of push Notification. I will discuss with @ImranBinShoukat and @sojharo. Once they will approve i will start to implement.

sojharo commented 4 years ago

I had a discussion on this with Arveen and reviewed his document. I think, from our previous experience of mobile push notifications, there should not be any need of storing token on server. I have asked arveen to give me some time and I will read the documentation Expo framework that they are using in react native mobile development.

The push notifications come from our server using APNS or Firebase and expo is using these in backend. Previously, when we worked on mobile, we had not stored any token on server, but client used to listen for push on user's id by subscribing to it. Same thing must be done in this mobile app as well and we should not require to add a new field in the database. I am reading on it now and will update here and discuss with arveen.

sojharo commented 4 years ago

I have read up on this in detail and have found that when we are using expo framework for creating our react-native apps then in this case we can only receive push notifications from expo server which behind the scenes is using firebase and apns. So this is forcing us to use their own token for each device. Therefore, we will have to store this token in our database.

However, please don't store it in company table and store the token in user table for each user who is using device because he might be using our app on multiple devices. It is better to store token array in user table and when there is any update in the company then our server gets list of team members of that company from user's table and send the push notification to them using their tokens.

Please make sure that we keep updating the tokens array for that user in case we receive any new token or previous one gets expired.

Go ahead and do the change on server and send me the pull request. Also, update your document accordingly

ImranBinShoukat commented 4 years ago

Please start with the following flow diagrams first:

  1. How will you register user for push notification
  2. If a company has 5 users and a message comes in from a subscriber, then how will you send push notifications. This should handle the following:
    • If a session is unassigned, all agents should receive pull notifications
    • If a session is assigned to agent, then how will you make sure only assigned agent gets the push notification
    • If a session is assigned to team, then how will you make sure only assigned team agents get the push notifications
  3. If a user has logged in from multiple devices, then how will you send push notifications to all of them
  4. How will you send push notification for resolve/reopen session if session is assigned to team
  5. How will you send push notification when session gets assigned to either team or agent
arveenkumar55 commented 4 years ago

I have covered the all use-cases in document as Imran said above comment. I will discuss with @ImranBinShoukat and @sojharo. Once they will approve i will start to implement.

ImranBinShoukat commented 4 years ago
  1. Please define the data type of listToken field. Show the updated schema.
  2. Please make the diagrams more clear. I have put some comments, please have a look
arveenkumar55 commented 4 years ago

I have edited the document. kindly @sojharo and @ImranBinShoukat please review. https://docs.google.com/document/d/1oDqA5aQ393kKZYQdGDJSYe99sJmfTbzsCYYYSJPYpMI/edit

ImranBinShoukat commented 4 years ago

I have reviewed it. Once, sir @jekram and @sojharo review it, then you can start working on it

arveenkumar55 commented 4 years ago

OK @ImranBinShoukat.

arveenkumar55 commented 4 years ago

I have created the I/O digram. I have put diagram at the end of document. @sojharo and @ImranBinShoukat please review the document let me know if you want any changes. https://docs.google.com/document/d/1oDqA5aQ393kKZYQdGDJSYe99sJmfTbzsCYYYSJPYpMI/edit

sojharo commented 4 years ago

I have reviewed the diagrams and they look good to me.

arveenkumar55 commented 4 years ago

worked on the server side task i) create an endpoint to update the companyUserTable (done) ii) create a generic send notification function (done) iii) Usecases (To send a notification on the client-side) (Progress)

arveenkumar55 commented 4 years ago

Task Update: Work on server side Task i) Write logic to remove the useless expo token from the database ii) Use Cases (Progress)

  1. If a user has logged in from multiple devices, then how will you send push notifications to all of them (done)
  2. If a company has 5 users and a message comes in from a subscriber, then how will you send push notifications. This should handle the following: (done) i. If a session is unassigned, all agents should receive pull notifications(done) ii. If a session is assigned to an agent, then how will you make sure only assigned agent gets the push notification(done) iii. If a session is assigned to a team, then how will you make sure only assigned team agents get the push notifications(done)
  3. How will you send a push notification when the session gets assigned to either team or agent(Progress) i. For Agent (Done) ii. Team (Not Done)
  4. How will you send push notification for resolve/reopen session if a session is assigned to the team (Not done)

Remaining server-side work complete tomorrow.

jekram commented 4 years ago

@arveenkumar55 what is the ETA to complete this task? This task has been open by six weeks

@sojharo

arveenkumar55 commented 4 years ago

It will take around 2 days because i) There is some work on server-side remaining ii) Work on Client-side remaining

jekram commented 4 years ago

What server-side task?

arveenkumar55 commented 4 years ago

There was 4 uses cases which I had written above. I have completed 2 and half on server side. remaining server-side task will complete today.

jekram commented 4 years ago

Five days ago this was the status

worked on the server side task i) create an endpoint to update the companyUserTable (done) ii) create a generic send notification function (done) iii) Usecases (To send a notification on the client-side) (Progress)

Which status is correct?

sojharo commented 4 years ago

I will review this with arveen today.

arveenkumar55 commented 4 years ago

Task Update: Work on server side Task (done)

  1. How will you send a push notification when the session gets assigned to either team or agent(Progress) ii. Team (Done)
  2. How will you send push notification for resolve/reopen session if a session is assigned to the team (done) i. Resolve session (done) ii. Reopen session (done)

Client side Work (Progress)

Remaining client-side works complete tomorrow.

arveenkumar55 commented 4 years ago

Complete the client side work. My pull request in under review when it will merge i will assign for testing

arveenkumar55 commented 4 years ago

@AnishaChhatwani please test on staging

AnishaChhatwani commented 4 years ago

Tested. I am not getting any push notifications. Getting this warning. @arveenkumar55 please look into it Screenshot_20200612-165815_Expo

arveenkumar55 commented 4 years ago

I worked with @AnishaChhatwani to setup an expo client on her Laptop. Basically the issue was to get an expo token on local enviroment we need to login first on the terminal. She was not log in to the terminal because this show was getting an error form client-side.

@AnishaChhatwani please now test

AnishaChhatwani commented 4 years ago

Tested on staging

AnishaChhatwani commented 4 years ago

push notifications are being displayed correctly. Just a suggestion: I think we should show profile picture too in the notification. @arveenkumar55 please see if we can do that too

arveenkumar55 commented 4 years ago

@AnishaChhatwani Basically we are using expo framework and it didn't support per-notification icons right now. This question already asked in 2017 https://forums.expo.io/t/push-notification-dynamic-icon/1684/2 test we can only define a single icon for all push notifications. Also, I have also asked from the expo community if they implement dynamic push notification in the three years. https://forums.expo.io/t/handle-dynamic-push-notification-icon-in-expo/39216 I am waiting for the expo community reply.

AnishaChhatwani commented 4 years ago

Found the following issues during team testing on android:

  1. Push notification does not show KiboPush icon.
  2. Clicking on push notification does not open live chat. Instead it redirected me to dashboard
  3. It sent me push notification even when I was logged out
arveenkumar55 commented 4 years ago

@AnishaChhatwani please test locally

arveenkumar55 commented 4 years ago

There was some profile picture issue in chat session I have fixed also. Once my pull request will merge I will assign for testing.

jekram commented 4 years ago

Merge is complete. What we have not submitted for testing?

arveenkumar55 commented 4 years ago

@AnishaChhatwani please test locally

AnishaChhatwani commented 4 years ago

Tested locally

AnishaChhatwani commented 4 years ago

Working fine on Android

AnishaChhatwani commented 4 years ago

Found following issues during testing on IOS:

arveenkumar55 commented 4 years ago

I have fixed these issues.

I have looked this Notification remains even after viewing the message issue but couldn't find solution. I will look at this issue on Monday.

arveenkumar55 commented 4 years ago

I have looked at this issue Notification remains even after viewing the message and studied expo documentation and try to implement the code but getting some error from the expo notification. test1

I have asked @bjafri5 for help but we couldn't find a solution. After then I have posted question in expo community form. Now i am waiting for reply. https://forums.expo.io/t/getpresentednotificationsasync-function-is-not-working-in-expo-notification/40860

arveenkumar55 commented 4 years ago

I got response from community and I followed the steps but still getting the same issue. I have asked again from him. https://forums.expo.io/t/getpresentednotificationsasync-function-is-not-working-in-expo-notification/40860 I am waiting for response.

jekram commented 4 years ago

This is not rocket science. Hundreds of companies have implemented this including KiboPush. @sojharo This has been open for 4 months. @arveenkumar55 If you are struggling why this is not getting escalated.

jekram commented 4 years ago

why are we not using Google Andriod Servies? Did we not used Google Firebase services last time?

arveenkumar55 commented 4 years ago

Basically we are using Material starter kit in our project and it is using expo frame work.On backed expo is using google services to send push notification.

arveenkumar55 commented 4 years ago

I couldn't work on this issue today as you said in meeting i need to discuss first with @sojharo . As @sojharo was not available today. I will discuss with @sojharo on next working days and will try to finish this task.

sojharo commented 4 years ago

I will discuss this with @arveenkumar55 on Monday. We have been using android push notifications on android java using google firebase services. From above comments, it looks like there is some issue in the expo framework that is being used in our react native code. I will work with arveen to understand why the problem is occurring.

jekram commented 4 years ago

@arveenkumar55 If a task is open for 2 months it is not acceptable. You need to resolve it or escalate it. A given task should NOT take more than 3 days

arveenkumar55 commented 4 years ago

First I discussed this issue with @sojharo and after i have upgraded the expo version and facing some Issues. I will fix these issues tommorrow.

jekram commented 4 years ago

What version of expo we were using before (hen it was released), and what version of expo we are upgrading to and when it was released? normally they have specified what has been fixed? What investigation we have done?

arveenkumar55 commented 4 years ago

Basically Previously we were using expo sdk version 36.0.0 and its released date 10 December 2019. https://blog.expo.io/expo-sdk-36-is-now-available-b91897b437fe We have updated this version into expo sdk version 38.0.0 and its released date 25 June 2020. https://dev.to/expo/expo-sdk-38-is-now-available-5aa0 In expo 38.0.0 they have added new expo-notification functions like getpresentedNotificationsAsync(): and which were are using and these functions are not supported in expo 36.0.0 Because of this we have updated the expo version.