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

Fix Chat Room Bugs When Changing Rooms and in Direct Messages #160

Open
wants to merge 3 commits into
base: development
Choose a base branch
from
+21 −4
Open

Fix Chat Room Bugs When Changing Rooms and in Direct Messages #160

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2888ab3
Fix Chat Room Bugs When Changing Rooms and in Direct Messages
odkhang Jul 22, 2024
7013839
remove unused comment, add line eof
lcduong Jul 22, 2024
045ed61
Merge branch 'development' into feature-139
lcduong Jul 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Fix Chat Room Bugs When Changing Rooms and in Direct Messages
@odkhang
odkhang committed Jul 22, 2024
commit 2888ab3cb2232b29a131eecee4aaa17de0d50d95
14 changes: 13 additions & 1 deletion server/venueless/live/modules/chat.py
View file
Original file line number Diff line number Diff line change
@@ -349,6 +349,18 @@ async def publish_unread_pointers(self, body):

@event("notification")
async def publish_notification(self, body):
event_id = body.get("data", {}).get("event", {}).get("event_id")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (complexity): Consider refactoring the code to reduce nesting and encapsulate specific logic into helper methods.

The new code introduces increased complexity due to additional nested conditionals, asynchronous context management, multiple return points, and potential error handling issues. To improve readability and maintainability, consider refactoring the code to reduce nesting and encapsulate specific logic into helper methods. Here's a suggested refactor:

@event("notification")
async def publish_notification(self, body):
    event_data = body.get("data", {}).get("event", {})
    event_id = event_data.get("event_id")
    channel = event_data.get("channel")

    if event_id and channel:
        read_pointer = await self._get_read_pointer(channel)
        if read_pointer is not None and read_pointer >= event_id:
            if await self.service.remove_notifications(self.consumer.user.id, self.channel_id, read_pointer):
                await self._send_notification_counts()
                return

    await self.consumer.send_json(["chat.notification", body.get("data")])

async def _get_read_pointer(self, channel):
    async with aredis() as redis:
        read_pointer = await redis.hget(f"chat:read:{self.consumer.user.id}", channel)
        return int(read_pointer.decode()) if read_pointer else None

async def _send_notification_counts(self):
    notification_counts = await database_sync_to_async(
        self.service.get_notification_counts
    )(self.consumer.user.id)
    await self.consumer.send_json(["chat.notification_counts", notification_counts])

This refactor reduces complexity by introducing helper methods, making the main method cleaner and easier to follow. It also adheres to the Single Responsibility Principle, making future maintenance simpler.

if event_id:
async with aredis() as redis:
read_pointer = await redis.hget(f"chat:read:{self.consumer.user.id}", body["data"]["event"]["channel"])
read_pointer = int(read_pointer.decode()) if read_pointer else 0
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Handle potential decoding errors

Consider adding error handling for the decode method in case the data is not in the expected format.

if read_pointer >= event_id:
if await self.service.remove_notifications(self.consumer.user.id, self.channel_id, read_pointer):
notification_counts = await database_sync_to_async(
self.service.get_notification_counts
)(self.consumer.user.id)
Comment on lines +359 to +361
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Consider using a more descriptive variable name

The variable notification_counts could be more descriptive to indicate what kind of notifications it is counting, e.g., unread_notification_counts.

Suggested change
notification_counts = await database_sync_to_async(
self.service.get_notification_counts
)(self.consumer.user.id)
unread_notification_counts = await database_sync_to_async(
self.service.get_unread_notification_counts
)(self.consumer.user.id)

await self.consumer.send_json(["chat.notification_counts", notification_counts])
return
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: Unnecessary return statement

The return statement here is redundant since the function will exit after this block anyway.

await self.consumer.send_json(["chat.notification", body.get("data")])

@command("send")
@@ -638,7 +650,7 @@ async def publish_event(self, body):
user_profiles_required |= extract_mentioned_user_ids(
data["content"].get("body", "")
)

user_profiles_required -= self.users_known_to_client
data["users"] = {}

18 changes: 14 additions & 4 deletions webapp/src/store/chat.js
View file
Original file line number Diff line number Diff line change
@@ -51,8 +51,8 @@ export default {
},
channelName (state, getters, rootState) {
return function (channel) {
if (this.isDirectMessageChannel(channel)) {
return this.directMessageChannelName(channel)
if (getters.isDirectMessageChannel(channel)) {
return getters.directMessageChannelName(channel)
} else {
return rootState.rooms.find(room => room.modules.some(m => m.channel_id === channel.id)).name
}
@@ -90,6 +90,7 @@ export default {
state.usersLookup = members.reduce((acc, member) => { acc[member.id] = member; return acc }, {})
state.timeline = []
state.warnings = []
state.fetchingMessages = null
state.beforeCursor = beforeCursor
state.config = config
if (getters.activeJoinedChannel) {
@@ -352,10 +353,19 @@ export default {
},
async 'api::chat.notification' ({state, rootState, getters, dispatch}, data) {
const channelId = data.event.channel
const channel = state.joinedChannels.find(c => c.id === channelId) || getters.automaticallyJoinedChannels.includes(channelId) ? {id: channelId} : null
const channel = state.joinedChannels.find(c => c.id === channelId) || (getters.automaticallyJoinedChannels.includes(channelId) ? {id: channelId} : null)
const eventId = data.event.event_id
if (!channel) return
// Increment notification count
Vue.set(state.notificationCounts, channel.id, (state.notificationCounts[channel.id] || 0) + 1)
if (eventId > state.readPointers[channelId] && channelId === state.channel) {
// In volatile channels, markChannelRead does not advance the readPointer and does not send mark_read, unless
// notificationCounts is non-zero.
// However, if we receive a notification for the channel that is currently open, we do need to trigger a
// *after* we increased notificationCounts to make sure that other connected clients know that the notification
// has been read.
dispatch('markChannelRead')
}
// TODO show desktop notification when window in focus but route is somewhere else?
let body = i18n.t('DirectMessage:notification-unread:text')
if (data.event.content.type === 'text') {
@@ -393,4 +403,4 @@ export default {
state.warnings.push(data)
}
}
}
}