Using state for edit and delete messages as well as doing reply to message function by nicholascheww · Pull Request #45 · MakanMatch/Backend

nicholascheww · 2024-07-08T09:50:00Z

What was done since the last PR:

Changed edit message function to use state instead of reloading after each edit
Added a (edited) after a message if it has been edited
Messages that have been edited will have a boolean value inside the database to tell whether if the message is edited or not
Changed delete message function to use state instead of reloading after each delete
Added a replyToID, repliedMessage and edited to models chatMessage
Done reply message where the id and the message the user is replying to will be saved into the database
Removed console.log() for logging user's messages
Removed chat.js file as realised it was redundant

Sync-up PR (Nicholas)

… does not disappear anymore

…as edited which will ruin the sorting of messages

Prakhar896

A few things to make consistent and more efficient.

Prakhar896 · 2024-07-08T15:22:56Z


+    // Static user IDs for demonstration
+    const user1ID = "Jamie";
+    const user2ID = "James";


Use real user IDs of real host and guest accounts. Currently, in onDBSynchronise, Jamie Oliver record is being created as a Host and Susie Jones record is being created as Guest. Their primary key identifiers are stored in Universal.data["DUMMY_HOST_ID"] and Universal.data["DUMMY_GUEST_ID"] respectively. You can use those.

im going to be working on doing auth soon so i shall leave this behind first

Prakhar896 · 2024-07-08T15:25:26Z

+        handleEditMessage(parsedMessage, user1ID, user2ID, ws);
+      } else if (parsedMessage.action === "delete") {
+        handleDeleteMessage(parsedMessage);
+      } else {


Ensure that the message is indeed a normal message event. I recommend having an if condition in this if a normal message is being sent and then making the else reject the client's message event. Example:

... } else if (parsedMessage.action === "newMessage") { ... } else { broadcastMessage(JSON.stringify({"error": "Invalid event update"})) }

Prakhar896 · 2024-07-08T15:27:19Z

      const jsonMessage = {
        action: "error",
-        message: "Error occured on the server",
+        message: "Error occurred on the server",


Why is this a server error? The client just provided an invalid messageId.

its because the message its not in the database anymore as if someone went into our database and delete that message from the database hence i put the error on the server is that wrong?

Prakhar896 · 2024-07-08T15:33:35Z

+      };
+      broadcastMessage(JSON.stringify(jsonMessage));
+    } catch (error) {
+      console.error("Error deleting message:", error);


Logger.log this as well.

Prakhar896 · 2024-07-08T15:34:10Z

-  function broadcastMessage(message, sender) {
+
+
+  function broadcastMessage(message) {


Just realised, this broadcastMessage method broadcasts messages to all clients. So, won't client-specific error messages be unnecessarily broadcasted to all clients?

my point for this is because if the message has been already deleted in our database it should reload both the clients pages idk if i am right or wrong about this part

… nicholas

Prakhar896

Minor changes needed.

Prakhar896 · 2024-07-11T06:19:02Z

+                        sender: parsedMessage.sender,
+                        datetime: parsedMessage.datetime,
+                        chatID: chatID,
+                        replyToID: parsedMessage.replyToID || null,


Shouldn't you be checking that the replyToID is a valid and existing ChatMessage first?

Prakhar896 · 2024-07-11T06:19:21Z

+        ws.on("message", async (message) => {
+            const parsedMessage = JSON.parse(message);
+            if (parsedMessage.action === "edit") {
+                handleEditMessage(parsedMessage, user1ID, user2ID, ws);


handleEditMessage only takes in editedMessage, the rest of the arguments are unnecessary.

Prakhar896 · 2024-07-11T06:22:23Z

+
+                    let replyToMessage = null;
+                    if (parsedMessage.replyToID) {
+                        replyToMessage = await ChatMessage.findByPk(parsedMessage.replyToID);


For the earlier comment on checking the existence of the reply to message, you can move this line of code up above the ChatMessage creation and carry out the check in this if chain itself. Basically:

let replyToMessage = null; if (parsedMessage.replyToID) { replyToMessage = await ChatMessage.findByPk(parsedMessage.replyToID); if (!replyToMessage) { // carry out error handling and reporting here } } const createdMessage = await ChatMessage.create(....

Prakhar896 · 2024-07-11T06:25:43Z

-  }

+    function broadcastMessage(message) {
+        clients.forEach((client) => {


You still need to understand that this whole flow won't work at all the second you add a third client. Since you're broadcasting everything to all clients, event updates for one conversation will interfere other conversations on the frontend as well. This is for future PRs, but I did inform you of this issue much earlier.

Prakhar896

Please check your code before requesting a review.

Prakhar896 · 2024-07-11T08:49:24Z

            } else if (parsedMessage.action === "send") {
                try {
+                  let replyToMessages = await ChatMessage.findByPk(parsedMessage.replyToID);
+                  console.log(replyToMessages);


Remove console log

Prakhar896 · 2024-07-11T08:50:03Z

                handleDeleteMessage(parsedMessage);
            } else if (parsedMessage.action === "send") {
                try {
+                  let replyToMessages = await ChatMessage.findByPk(parsedMessage.replyToID);


This variable name is plural when you are actually storing a singular ChatMessage. Change to replyToMessage instead.

Prakhar896

LGTM

nicholascheww and others added 9 commits July 5, 2024 14:29

Merge pull request #36 from MakanMatch/main

beed058

Sync-up PR (Nicholas)

changed edit message to use state

441ba1e

fixed a bug where messages are not being stored into chathistory

eff4dba

added replyto parameter to sql

3062756

added a new field replied message to the model and also reply message…

aadea38

… does not disappear anymore

changed delete to use state

b6b974a

removed console.logs and removed unecessary file

776d2d6

added a new edit boolean to model to set it to be true

7bbefe2

re removed console.log messages

4c5c422

nicholascheww added the enhancement New feature or request label Jul 8, 2024

nicholascheww requested a review from Prakhar896 July 8, 2024 09:50

nicholascheww added 2 commits July 8, 2024 21:58

changed a bug where edit message will put the time that the message w…

72855bf

…as edited which will ruin the sorting of messages

changed indent to 4

1cf7b42

Prakhar896 requested changes Jul 8, 2024

View reviewed changes

nicholascheww added 3 commits July 9, 2024 14:09

removed repliedmessage from data models

e1402fc

fixed and added stuff according to comments

f2582ea

Merge branch 'nicholas' of https://github.com/MakanMatch/Backend into…

7664cb5

… nicholas

nicholascheww requested a review from Prakhar896 July 9, 2024 13:55

reindented

6f94ab2

Prakhar896 requested changes Jul 11, 2024

View reviewed changes

nicholascheww added 2 commits July 11, 2024 15:16

fixed replyToID to check if it exists

7737ccf

fixed a problem where message was not able to send

56ba2d7

nicholascheww requested a review from Prakhar896 July 11, 2024 07:42

Prakhar896 requested changes Jul 11, 2024

View reviewed changes

removed console.log and changed from plural to singluar

2328eaa

nicholascheww requested a review from Prakhar896 July 11, 2024 08:54

Prakhar896 approved these changes Jul 11, 2024

View reviewed changes

Prakhar896 merged commit 397f3c9 into main Jul 11, 2024

		function broadcastMessage(message, sender) {


		function broadcastMessage(message) {

Conversation

nicholascheww commented Jul 8, 2024

Uh oh!

Prakhar896 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Prakhar896 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Prakhar896 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Prakhar896 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants