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

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
Open

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

Show file tree
Hide file tree
Changes from 2 commits
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
14 changes: 13 additions & 1 deletion server/venueless/live/modules/chat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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"] = {}

Expand Down
11 changes: 8 additions & 3 deletions webapp/src/store/chat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -352,10 +353,14 @@ 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) {
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') {
Expand Down
Loading