-
-
Notifications
You must be signed in to change notification settings - Fork 44
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
#2392 - No need to send a notification when the chatroom is open #2394
Conversation
group-income Run #3351
Run Properties:
|
Project |
group-income
|
Branch Review |
sebin/task/#2392-dm-notification-issue
|
Run status |
Passed #3351
|
Run duration | 09m 05s |
Commit |
33dda1cb66 ℹ️: Merge 885e49082269884e516faeae5931de6e8a0b6824 into 1bc76ebde99551c7e9d478bfca90...
|
Committer | Sebin Song |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
10
|
Skipped |
0
|
Passing |
111
|
View all changes introduced in this branch ↗︎ |
const currentRoute = sbp('controller/router').history.current || '' | ||
const isTargetChatroomCurrentlyActive = currentRoute.path.includes('/group-chat') && | ||
getters.currentChatRoomId === contractID // when the target chatroom is currently open/active on the browser, No need to send a notification. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two potential issues we need to consider here:
- Should we send a notification anyway when the user is idle but on the page? (We currently use
idleVue
to check for this, search the project for it) - Are we able to, when using the service worker approach, tell what page we're on in the actions (and whether we're idle or not)? This question is for @corrideat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to approve this after creating an issue to deal with this in a different manner later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Approving this and to deal with the service-worker and idleVue
considerations I created issue #2397 and assigned it to @corrideat!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooops... I think I approved too quickly.
messageReceivePostEffect
is supposed to always be called, because it calls 'gi.actions/identity/kv/addChatRoomUnreadMessage'
which is always supposed to be called (as it's not notification related).
…sebin/task/#2396-non-monetary-bug
closes #2392
It appears the fix works well when tested with multiple containers of Firefox Dev Edition.