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.

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

Implement Read Status #4

Merged
merged 6 commits into from
Apr 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions client/cypress/integration/bug-fix-ticket.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ describe("Bug Fix: Sending Messages", () => {
cy.contains("Bob").click();

cy.get("input[name=text]").type("First message{enter}");
cy.wait(500)
Copy link
Owner Author

Choose a reason for hiding this comment

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

Referenced this in my PR description, but had to add these to prevent cypress from moving too quickly and dropping characters.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can safely ignore cypress tests for tickets other than your first ticket.

Copy link
Owner Author

Choose a reason for hiding this comment

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

ah, noted! Figured it'd be good practice to implement testing but will skip this in the future.

cy.get("input[name=text]").type("Second message{enter}");
cy.wait(500)
cy.get("input[name=text]").type("Third message{enter}");

cy.contains("First message");
Expand Down Expand Up @@ -60,7 +62,9 @@ describe("Bug Fix: Sending Messages", () => {
cy.contains("Bob").click();

cy.get("input[name=text]").type("Fourth message{enter}");
cy.wait(500)
cy.get("input[name=text]").type("Fifth message{enter}");
cy.wait(500)
cy.get("input[name=text]").type("Sixth message{enter}");

cy.contains("Fourth message");
Expand Down
55 changes: 55 additions & 0 deletions client/cypress/integration/read-status.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/// <reference types="cypress" />

const ringo = {
username: "Ringo",
email: "[email protected]",
password: "Z6#6%xfLTarZ9U",
};
const george = {
username: "George",
email: "[email protected]",
password: "L%e$xZHC4QKP@F",
};

describe("Read Status", () => {
it("setup", () => {
cy.signup(ringo.username, ringo.email, ringo.password);
cy.logout();
cy.signup(george.username, george.email, george.password);
cy.logout();
});

it("displays total unread", () => {
cy.login(ringo.username, ringo.password);

cy.get("input[name=search]").type("George");
cy.contains("George").click();

cy.get("input[name=text]").type("First message{enter}");
cy.wait(500)
cy.get("input[name=text]").type("Second message{enter}");
cy.wait(500)
cy.get("input[name=text]").type("Third message{enter}");

cy.logout();

cy.login(george.username, george.password);
cy.contains("3");
cy.logout();
});

it("displays last read message", () => {
cy.login(george.username, george.password);
cy.contains("Ringo").click();
cy.logout();

cy.login(ringo.username, ringo.password);
cy.contains("George").click();

cy.get("svg").should(($list) => {
expect($list).to.have.length(6)
})

cy.logout();
});
});
1 change: 1 addition & 0 deletions client/src/components/ActiveChat/ActiveChat.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ const ActiveChat = ({
{user && (
<>
<Messages
lastReadMessage={conversation.lastReadMessage}
messages={conversation.messages}
otherUser={conversation.otherUser}
userId={user.id}
Expand Down
12 changes: 9 additions & 3 deletions client/src/components/ActiveChat/Messages.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,21 @@ import { SenderBubble, OtherUserBubble } from '.';
import moment from 'moment';

const Messages = (props) => {
const { messages, otherUser, userId } = props;
const { lastReadMessage, messages, otherUser, userId } = props;

return (
<Box>
{messages.map((message) => {
const time = moment(message.createdAt).format('h:mm');

let isLastRead = lastReadMessage && lastReadMessage === message.id;
return message.senderId === userId ? (
<SenderBubble key={message.id} text={message.text} time={time} />
<SenderBubble
key={message.id}
isLastRead={isLastRead}
otherUser={otherUser}
text={message.text}
time={time}
/>
) : (
<OtherUserBubble
key={message.id}
Expand Down
16 changes: 14 additions & 2 deletions client/src/components/ActiveChat/SenderBubble.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,20 @@
import React from 'react';
import { makeStyles } from '@material-ui/core/styles';
import { Box, Typography } from '@material-ui/core';
import { Avatar, Box, Typography } from '@material-ui/core';

const useStyles = makeStyles(() => ({
root: {
display: 'flex',
flexDirection: 'column',
alignItems: 'flex-end',
},
avatar: {
height: 20,
width: 20,
marginRight: 1,
marginTop: 9,
marginBottom: 5,
},
date: {
fontSize: 11,
color: '#BECCE2',
Expand All @@ -27,7 +34,7 @@ const useStyles = makeStyles(() => ({
},
}));

const SenderBubble = ({ time, text }) => {
const SenderBubble = ({ isLastRead, otherUser, time, text }) => {
const classes = useStyles();

return (
Expand All @@ -36,6 +43,11 @@ const SenderBubble = ({ time, text }) => {
<Box className={classes.bubble}>
<Typography className={classes.text}>{text}</Typography>
</Box>
{isLastRead && <Avatar
alt={otherUser.username}
src={otherUser.photoUrl}
className={classes.avatar}
/>}
Comment on lines +46 to +50
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

</Box>
);
};
Expand Down
81 changes: 78 additions & 3 deletions client/src/components/Home.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,43 @@ const Home = ({ user, logout }) => {
}
};

const markMessagesRead = useCallback(async (conversationId, messages) => {
const readMessageIds = messages.map(message => message.id);
const readAt = new Date();

const body = {
conversationId,
readAt,
readMessages: readMessageIds
}

const { data } = await axios.patch("/api/conversations", body);

socket.emit("read-messages", { conversationId, readAt, readMessageIds: data.readMessageIds });
}, [socket]);

const updateReadMessages = useCallback((data) => {
setConversations((prev) =>
prev.map((convo) => {
if (convo.id === data.conversationId) {
const convoCopy = { ...convo, messages: [ ...convo.messages ]}
const newMessages = convoCopy.messages.map(message => {
if (data.readMessageIds.includes(message.id)) {
message.readAt = data.readAt;
}
return message;
});

convoCopy.messages = newMessages;
convoCopy.lastReadMessage = Math.max(...data.readMessageIds);
return convoCopy;
} else {
return convo;
}
}),
);
}, [setConversations]);

const addNewConvo = useCallback(
(recipientId, message) => {
setConversations((prev) => {
Expand Down Expand Up @@ -106,28 +143,64 @@ const Home = ({ user, logout }) => {
id: message.conversationId,
otherUser: sender,
messages: [message],
unreadMessageCount: 0,
};
newConvo.latestMessageText = message.text;
setConversations((prev) => [newConvo, ...prev]);
}

let readMessages = [];

setConversations((prev) => {
return prev.map((convo) => {
const convoCopy = { ...convo, messages: [ ...convo.messages ]}

if (convoCopy.id === message.conversationId) {
convoCopy.messages.push(message);
convoCopy.latestMessageText = message.text;

if (message.senderId !== user.id) {
if (activeConversation === convoCopy.otherUser.username) {
readMessages.push(message);
} else {
convoCopy.unreadMessageCount++;
}
}
}

return convoCopy;
});
});

if (readMessages.length > 0) {
markMessagesRead(message.conversationId, readMessages);
}
},
[setConversations],
[activeConversation, markMessagesRead, setConversations, user.id],
);

const setActiveChat = (username) => {
const conversation = conversations.find(conversation => {
return conversation.otherUser.username === username;
});

let readMessages = conversation.messages.filter(message => {
return !message.readAt && message.senderId !== user.id;
});

setConversations((prev) => {
return prev.map((convo) => {
const convoCopy = { ...convo }

if (convoCopy.id === conversation.id) {
convoCopy.unreadMessageCount = 0;
}

return convoCopy;
});
});

if (readMessages.length > 0) { markMessagesRead(conversation.id, readMessages); }
setActiveConversation(username);
};

Expand Down Expand Up @@ -166,15 +239,17 @@ const Home = ({ user, logout }) => {
socket.on("add-online-user", addOnlineUser);
socket.on("remove-offline-user", removeOfflineUser);
socket.on("new-message", addMessageToConversation);
socket.on("read-messages", updateReadMessages);

return () => {
// before the component is destroyed
// unbind all event handlers used in this component
socket.off("add-online-user", addOnlineUser);
socket.off("remove-offline-user", removeOfflineUser);
socket.off("new-message", addMessageToConversation);
socket.off("read-messages", updateReadMessages);
};
}, [addMessageToConversation, addOnlineUser, removeOfflineUser, socket]);
}, [addMessageToConversation, addOnlineUser, removeOfflineUser, socket, updateReadMessages]);

useEffect(() => {
// when fetching, prevent redirect
Expand Down
21 changes: 19 additions & 2 deletions client/src/components/Sidebar/Chat.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React from 'react';
import { Box } from '@material-ui/core';
import { Box, Chip } from '@material-ui/core';
import { BadgeAvatar, ChatContent } from '../Sidebar';
import { makeStyles } from '@material-ui/core/styles';

Expand All @@ -15,6 +15,12 @@ const useStyles = makeStyles((theme) => ({
cursor: 'grab',
},
},
unread: {
fontSize: 10,
fontWeight: 'bold',
height: 20,
marginRight: 20,
}
}));

const Chat = ({ conversation, setActiveChat }) => {
Expand All @@ -25,6 +31,8 @@ const Chat = ({ conversation, setActiveChat }) => {
await setActiveChat(conversation.otherUser.username);
};

const isUnread = conversation.unreadMessageCount > 0;

return (
<Box onClick={() => handleClick(conversation)} className={classes.root}>
<BadgeAvatar
Expand All @@ -33,7 +41,16 @@ const Chat = ({ conversation, setActiveChat }) => {
online={otherUser.online}
sidebar={true}
/>
<ChatContent conversation={conversation} />
<ChatContent
conversation={conversation}
isUnread={isUnread}
/>
{isUnread && <Chip
className={classes.unread}
color="primary"
label={conversation.unreadMessageCount}
size="small"
/>}
</Box>
);
};
Expand Down
11 changes: 9 additions & 2 deletions client/src/components/Sidebar/ChatContent.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,21 +18,28 @@ const useStyles = makeStyles((theme) => ({
color: "#9CADC8",
letterSpacing: -0.17,
},
unReadText: {
fontSize: 12,
fontWeight: 600,
color: "#000",
letterSpacing: -0.17,
}
}));

const ChatContent = ({ conversation }) => {
const ChatContent = ({ conversation, isUnread }) => {
const classes = useStyles();

const { otherUser } = conversation;
const latestMessageText = conversation.id && conversation.latestMessageText;
const previewClass = isUnread ? classes.unReadText : classes.previewText;

return (
<Box className={classes.root}>
<Box>
<Typography className={classes.username}>
{otherUser.username}
</Typography>
<Typography className={classes.previewText}>
<Typography className={previewClass}>
{latestMessageText}
</Typography>
</Box>
Expand Down
18 changes: 18 additions & 0 deletions server/messenger_backend/migrations/0002_message_readat.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Generated by Django 3.2.4 on 2022-04-18 12:43

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('messenger_backend', '0001_initial'),
]

operations = [
migrations.AddField(
model_name='message',
name='readAt',
field=models.DateTimeField(null=True),
),
]
1 change: 1 addition & 0 deletions server/messenger_backend/models/message.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,6 @@ class Message(utils.CustomModel):
related_name="messages",
related_query_name="message"
)
readAt = models.DateTimeField(null=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

While this works for the implementation you created, it adds a lot of complexity to the code, since now you're having to do more work to count the number of read messages and it makes it more difficult to also add the avatar image under the last read message that is present in the mocks.
A boolean field in the Message model that marks whether a message is read or not is better in this case.

Let's not update time implementation as it would require a lot of code changes throughout the PR. 🙂

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thank you for the feedback! Yeah, the time implementation was certainly more complex but I was trying to think ahead for future updates that may want to access when messages were read. Absolutely agreed though that for this particular implementation a boolean would have sufficed.

createdAt = models.DateTimeField(auto_now_add=True, db_index=True)
updatedAt = models.DateTimeField(auto_now=True)
Loading