search

enBloc-org / kindly

GNU General Public License v3.0
12 stars 16 forks source link

feat: added additional validation to the messaging function newConvoS… #197

Closed YuraPetrovskyi closed 3 weeks ago

YuraPetrovskyi commented 1 month ago

…tart.ts

relates #141

Checklist:

Description

**Relates #141
before creating a new conversation, we check if such a conversation already exists in the database, if we find such a conversation, we return its ID that we can use further in the code, if such a conversation does not exist in the database, then we create a new conversation

Files changed

before the change this function was only doing updates in the database now It return null or conversation_id

Tests

after applying the changes I got the expected behavior and no errors were caught

netlify[bot] commented 1 month ago

Deploy request for cool-creponne-3e1272 pending review.

Visit the deploys page to approve it

Name Link
Latest commit 1b19e78126275a4b95190afcf5cbe416debe677b
YuraPetrovskyi commented 1 month ago

Great Alphonso! I completely agree with you, you can close #201. I will try to implement the following logic :)

пт, 17 трав. 2024 р. о 15:50 Alphonso @.***> пише:

@.**** commented on this pull request.

On supabase/models/messaging/newConvoStart.ts https://github.com/enBloc-org/kindly/pull/197#discussion_r1605134923:

Thanks Yura, good flag!

I think your second suggestion is probably the best here since it avoids us having to constantly check for existing conversations (sounds like an absolute nightmare for our performance) and if we think of it, a button saying "message" is not odd for the UI if it redirects you to the appropriate conversation.

We might want to close #201 https://github.com/enBloc-org/kindly/issues/201 as redundant really 🤔

In this case, what we could do is:

  • Leave the "Message" button as is
  • Run startNewConversation wen the button is clicked and either:
    • Receive a null and create a new conversation, then navigate to it
    • Receive an Id and navigate to it

Funnily enough I see you're assigned to issue #133 https://github.com/enBloc-org/kindly/issues/133, where this would fit, so if you agree with that plan I can close #201 https://github.com/enBloc-org/kindly/issues/201 with a comment explaining and you can try to implement it in another PR :)

— Reply to this email directly, view it on GitHub https://github.com/enBloc-org/kindly/pull/197#discussion_r1605134923, or unsubscribe https://github.com/notifications/unsubscribe-auth/AVBTHGOEHHTH7U7MD3HDAI3ZCYKJ7AVCNFSM6AAAAABHJGDGMGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANRTGY2DMMZRGM . You are receiving this because you modified the open/close state.Message ID: @.***>

YuraPetrovskyi commented 1 month ago

Hello. I also added a check to see if we are creating a conversation with ourselves. I think it is appropriate. What do you think about it?

YuraPetrovskyi commented 1 month ago

By the way, I forgot to delete the alert, I will do it later....

camelPhonso commented 1 month ago

Hello. I also added a check to see if we are creating a conversation with ourselves. I think it is appropriate. What do you think about it?

Yes, brilliant! I think you're spot on to add that. But I think we might be doing it in the wrong place right now.

I haven't reviewed in depth but in the item[id]/page.tsx file there's a check stored in the variable canMessage. That currently only checks if the user is a refugee but if we evaluate there whether the current user is the same as the item donor then we can just stop the "message" button from rendering at all.

That could save us calling on the database at all in these cases and fixes the unwanted behaviour.

The evaluation right now looks like this:

if (userProfile?.data.refugee === false){
 canMessage = false;
}

I think if it looked like this, it would fix it for us:

if (userProfile?.data.refugee === false || item?.profiles?.id === userProfile?.data.id) {
  canMessage = false;
}

What do you think?

YuraPetrovskyi commented 1 month ago

Hi Alphons Yes, you are right it simplifies the code a lot. I checked and everything seems to work. I will delete my commit. Should I add your fixes or does it apply to another task? Can I ask you for advice on the next one?: I want to get messaging in order, I have already thought about the logic of how it should be and worked out some code. The logic is as follows - mostly it concerns the use of the application on the phone, when the user presses the "message" button, he expects to be redirected immediately to the message exchange page, but when the user opened the application and immediately wants to go to all messages, it is necessary to display a list of all messages sorted from new to old ones, where unread messages will also be displayed. (by the way, I would change the visualization of the messaging button in the navigation panel so that the presence of new unread messages is displayed). Since we don't use dynamic paths to fetch messages, I used local storage to pass the conversationId from NewConversationButton down to CurrentConversation. Then we can redirect the user to the corresponding conversation by checking for the conversationId in local storage. What do you think about it?

чт, 23 трав. 2024 р. о 10:00 Alphonso @.***> пише:

Hello. I also added a check to see if we are creating a conversation with ourselves. I think it is appropriate. What do you think about it?

Yes, brilliant! I think you're spot on to add that. But I think we might be doing it in the wrong place right now.

I haven't reviewed in depth but in the item[id]/page.tsx file there's a check stored in the variable canMessage. That currently only checks if the user is a refugee but if we evaluate there whether the current user is the same as the item donor then we can just stop the "message" button from rendering at all.

That could save us calling on the database at all in these cases and fixes the unwanted behaviour.

The evaluation right now looks like this:

if (userProfile?.data.refugee === false){ canMessage = false;}

I think if it looked like this, it would fix it for us:

if (userProfile?.data.refugee === false || item?.profiles?.id === userProfile?.data.id) { canMessage = false;}

What do you think?

— Reply to this email directly, view it on GitHub https://github.com/enBloc-org/kindly/pull/197#issuecomment-2126591119, or unsubscribe https://github.com/notifications/unsubscribe-auth/AVBTHGPKC6NMDTAXARJS7I3ZDWV2HAVCNFSM6AAAAABHJGDGMGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRWGU4TCMJRHE . You are receiving this because you modified the open/close state.Message ID: @.***>

camelPhonso commented 1 month ago

Hi Alphons Yes, you are right it simplifies the code a lot. I checked and everything seems to work. I will delete my commit. Should I add your fixes or does it apply to another task?

I think go ahead and add this fix to your PR and we can merge it all together - it fits quite nicely in the scope of this issue 👍

Can I ask you for advice on the next one?: I want to get messaging in order, I have already thought about the logic of how it should be and worked out some code. The logic is as follows - mostly it concerns the use of the application on the phone, when the user presses the "message" button, he expects to be redirected immediately to the message exchange page, but when the user opened the application and immediately wants to go to all messages, it is necessary to display a list of all messages sorted from new to old ones, where unread messages will also be displayed. (by the way, I would change the visualization of the messaging button in the navigation panel so that the presence of new unread messages is displayed). Since we don't use dynamic paths to fetch messages, I used local storage to pass the conversationId from NewConversationButton down to CurrentConversation. Then we can redirect the user to the corresponding conversation by checking for the conversationId in local storage. What do you think about it? чт, 23 трав. 2024 р. о 10:00 Alphonso @.***> пише:

This one sounds like it might cross over with some work that's outstanding from Nich right now. Do you mind checking back in via Discord after this Friday once we've had a chance to review all current PR's and ongoing Issues?

YuraPetrovskyi commented 1 month ago

Yes of course. This is the best way to avoid misunderstandings :)

чт, 23 трав. 2024 р. о 12:51 Alphonso @.***> пише:

Hi Alphons Yes, you are right it simplifies the code a lot. I checked and everything seems to work. I will delete my commit. Should I add your fixes or does it apply to another task?

I think go ahead and add this fix to your PR and we can merge it all together - it fits quite nicely in the scope of this issue 👍

Can I ask you for advice on the next one?: I want to get messaging in order, I have already thought about the logic of how it should be and worked out some code. The logic is as follows - mostly it concerns the use of the application on the phone, when the user presses the "message" button, he expects to be redirected immediately to the message exchange page, but when the user opened the application and immediately wants to go to all messages, it is necessary to display a list of all messages sorted from new to old ones, where unread messages will also be displayed. (by the way, I would change the visualization of the messaging button in the navigation panel so that the presence of new unread messages is displayed). Since we don't use dynamic paths to fetch messages, I used local storage to pass the conversationId from NewConversationButton down to CurrentConversation. Then we can redirect the user to the corresponding conversation by checking for the conversationId in local storage. What do you think about it? чт, 23 трав. 2024 р. о 10:00 Alphonso @.***> пише:

This one sounds like it might cross over with some work that's outstanding from Nich right now. Do you mind checking back in via Discord after this Friday once we've had a chance to review all current PR's and ongoing Issues?

— Reply to this email directly, view it on GitHub https://github.com/enBloc-org/kindly/pull/197#issuecomment-2126917255, or unsubscribe https://github.com/notifications/unsubscribe-auth/AVBTHGKLZS5CP6OURSAQW2TZDXJ27AVCNFSM6AAAAABHJGDGMGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRWHEYTOMRVGU . You are receiving this because you modified the open/close state.Message ID: @.***>

YuraPetrovskyi commented 3 weeks ago

Thank you @camelPhonso for your help and tips!