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.

Sign up for GitHub

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

Jump to bottom

feat(notifications): Implement notifications for Group Chats #3383

Merged
merged 17 commits into from
Jun 8, 2022
Merged

feat(notifications): Implement notifications for Group Chats #3383

merged 17 commits into from
Jun 8, 2022

Conversation

Jekrimo
Copy link
Contributor

@Jekrimo Jekrimo commented May 31, 2022

What this PR does 📖

Implement notifications for Group Chats

Which issue(s) this PR fixes 🔨

AP-1501

@netlify
Copy link

netlify bot commented May 31, 2022 •
edited
Loading

✅ Yeeeehaw, deploy preview is ready!

Name Link
🔨 Latest commit 491a13d
🔍 Latest deploy log https://app.netlify.com/sites/adoring-edison-dbcef8/deploys/62a0da756740d100087dfb0a
😎 Deploy Preview https://deploy-preview-3383--adoring-edison-dbcef8.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@Jekrimo Jekrimo added the Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA label May 31, 2022
@josephmcg josephmcg added Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. and removed Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA labels Jun 1, 2022
@Jekrimo Jekrimo added Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA and removed Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. labels Jun 1, 2022
aewing
aewing reviewed Jun 2, 2022
View reviewed changes
libraries/ui/Alerts.ts Outdated Show resolved Hide resolved
@Jekrimo Jekrimo added Ready for QA Ready for QA team to test, Devs approved. and removed Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA labels Jun 2, 2022
@phillsatellite
Copy link
Contributor

phillsatellite commented Jun 2, 2022 •
edited
Loading

Hi Jeff! Awesome pr 💯 Few small things I found though

  1. Once each user has sent a message in the group chat, whenever any future alerts from groups come in they will come in twice as much so like if User A sent the group a message User B would get 2 of the same alerts saying User A messaged the group. The easiest way to replicate is start a new group, send 1 message from each user after that the next message you send you should get x2 alerts
x2.alerts.mov
  1. Another issue I found was if a user hasn't entered the group chat yet they won't receive any alerts from it. They have to go into the group chat then navigate to another chat to start receiving notifications for it. I tested dev's behavior with how it is if you have a 1on1 chat you haven't entered yet and was still getting alerts for it
enter.before.alerts.mov
  1. And last one seems the group name is missing form the alert

Screen Shot 2022-06-02 at 2 29 47 PM

@phillsatellite phillsatellite added Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. and removed Ready for QA Ready for QA team to test, Devs approved. labels Jun 2, 2022
@stavares843 stavares843 force-pushed the AP-1501 branch 4 times, most recently from f30b1ac to f3ddac6 Compare June 3, 2022 17:39
@stavares843 stavares843 added Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA and removed Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. labels Jun 3, 2022
molimauro
molimauro reviewed Jun 6, 2022
View reviewed changes
store/textile/actions.ts Outdated Show resolved Hide resolved
@molimauro molimauro added Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. and removed Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA labels Jun 6, 2022
@Jekrimo Jekrimo added Ready for QA Ready for QA team to test, Devs approved. and removed Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. labels Jun 6, 2022
@stavares843 stavares843 added testing blocked There is an issue outside of the app that prevents testing. EG Infrastructure issues. and removed Ready for QA Ready for QA team to test, Devs approved. labels Jun 6, 2022
molimauro
molimauro reviewed Jun 6, 2022
View reviewed changes
store/textile/actions.ts Outdated Show resolved Hide resolved
@phillsatellite
Copy link
Contributor

Tested: I think something broke since the last changes no notifications from groups seem to appear. I also tested this with Sara to see if it would appear on her side and nothing 😢

no.notifications.mov

@phillsatellite phillsatellite added Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. and removed Ready for QA Ready for QA team to test, Devs approved. labels Jun 7, 2022
@stavares843
Copy link
Member

true 🔨 i also didn't get the group notifications

@Jekrimo
Copy link
Contributor Author

Jekrimo commented Jun 8, 2022

Tested: I think something broke since the last changes no notifications from groups seem to appear. I also tested this with Sara to see if it would appear on her side and nothing 😢

no.notifications.mov

pushed fix. PRAY FOR MEEEE

@Jekrimo Jekrimo added Ready for QA Ready for QA team to test, Devs approved. and removed Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. labels Jun 8, 2022
@phillsatellite phillsatellite added QA tested One QA Person has tested - Needs QA Lead review still and removed Ready for QA Ready for QA team to test, Devs approved. labels Jun 8, 2022
@stavares843 stavares843 merged commit 6ea270a into dev Jun 8, 2022
@stavares843 stavares843 deleted the AP-1501 branch June 8, 2022 20:45
@github-actions github-actions bot removed the QA tested One QA Person has tested - Needs QA Lead review still label Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@molimauro molimauro molimauro left review comments

@josephmcg josephmcg josephmcg left review comments

@WanderingHogan WanderingHogan WanderingHogan left review comments

@phillsatellite phillsatellite phillsatellite approved these changes

@stavares843 stavares843 stavares843 approved these changes

@aewing aewing aewing approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Jekrimo @phillsatellite @stavares843 @WanderingHogan @aewing @josephmcg @molimauro