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

Feat <back> <chat, websockets>: storing websockets and userIds + receiver joins DM Room when creating DM room #58

Merged
merged 7 commits into from
Sep 26, 2022

Conversation

NelMasr
Copy link
Collaborator

@NelMasr NelMasr commented Sep 24, 2022

Every time a user connects, we store his socket ID and associate it to its userID.
Every time a user disconnects, we delete the entry from the array.

Every time a user connects, we store his socket ID and associate it to
its userID.
Every time a user disconnects, we delete the entry from the array.
NelMasr and others added 3 commits September 24, 2022 23:56
Before, only the sender joined the room that was created, like a
channel.
But in a DM, both users have to join the channel.
Now that we stock websockets, it is possible.
To make the receiver join the room only when he is connected
@NelMasr
Copy link
Collaborator Author

NelMasr commented Sep 25, 2022

New feature in this PR (I decided to do it in this branch because it cannot work if this PR is not in main).
Now that we store websocket IDs for each user, we can make the receiver join the room if he is connected (if we find his websocket in our UsersWebsockets object)

@NelMasr NelMasr changed the title Feat <back> <chat, websockets>: storing websockets and userIds Feat <back> <chat, websockets>: storing websockets and userIds + receiver joins DM Room when creating DM room Sep 25, 2022
@Psycokwet
Copy link
Owner

If you want, when you have this kind of issue, create a branch, from the one you did a pr on, and so, you can start working on dependant feature like that :) Just don't do a pr for the child branch before the other one did pass ! :)

New feature in this PR (I decided to do it in this branch because it cannot work if this PR is not in main). Now that we store websocket IDs for each user, we can make the receiver join the room if he is connected (if we find his websocket in our UsersWebsockets object)

@thi-nguy
Copy link
Collaborator

great tip, I'm having the same concern :)\

@thi-nguy
Copy link
Collaborator

thi-nguy commented Sep 26, 2022

Sorry I have no idea how the codes work :), so I can not say it's good or not.

Just one thing comes up to my mind. Should we propose some ways to test what you guys have done in a PR? (you can put it in the comments, a guide, or a test. Show us how to test the work, which results should we have, in console or whatever).

Otherwise with people like me, I don't understand the code and I don't know which result it should give, I feel it's kind of wasting time to read blindly and don't get much knowledge. Maybe I don't understand the code, but at least I should be able to see the results.

For example, from pure front-end, you guys can test it easily because it's visual. But with work in fusion, I created some tests for you either on the site, or in the console to prove that it works (with backend). It takes more time for me, but it helps people who does not work on it have some ideas on how it should work.

NelMasr added a commit that referenced this pull request Sep 26, 2022
Copy link
Owner

@Psycokwet Psycokwet left a comment

Choose a reason for hiding this comment

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

I don't see anything wrong appart from my comment, but as thi said, didn't know how to test this one ^^

backend/src/chat/chat.gateway.ts Show resolved Hide resolved
Copy link
Owner

@Psycokwet Psycokwet left a comment

Choose a reason for hiding this comment

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

Aaah, I meant to approve, don't know what I did wrong sorry u_u'

Copy link
Collaborator

@DaiClement DaiClement left a comment

Choose a reason for hiding this comment

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

I approve, but if you can make this little change.

Comment on lines 398 to 405
// @UseGuards(JwtWsGuard)
// @SubscribeMessage(ROUTES_BASE.CHAT.BAN_USER_REQUEST)
// async banUser(
// @MessageBody() data: { message: string; channelId: number },
// @UserPayload() payload: any,
// ) {
//
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose this is the next PR.
Next time try to separate them.

export class UsersWebsockets {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you didn't change the file's name.
Can you change it ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah, i forgot.

@@ -49,7 +52,27 @@ export class ChatGateway {

@WebSocketServer()
server: Server;
async handleConnection(client: Socket) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you add the decorator ?
async handleConnection(@ConnectedSocket() client: Socket) {

}
}

handleDisconnect(client: Socket) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

same thing here

Copy link
Collaborator

@DaiClement DaiClement left a comment

Choose a reason for hiding this comment

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

sorry, i ask for change of interface/charRoom.ts

Copy link
Collaborator

@DaiClement DaiClement left a comment

Choose a reason for hiding this comment

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

Can you change the name of interfaces/ChatRoom.ts ?
Sorry for the multiple message. i am tired.

@NelMasr
Copy link
Collaborator Author

NelMasr commented Sep 26, 2022

Can you change the name of interfaces/ChatRoom.ts ? Sorry for the multiple message. i am tired.

It is done, thank you!

Copy link
Collaborator

@DaiClement DaiClement left a comment

Choose a reason for hiding this comment

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

I prefer all stuff concern user in user/something.
But i need your code, so i approve it 😅

@@ -24,7 +24,7 @@ export class ChatService {
private usersRepository: Repository<User>,
private userService: UsersService,
) {}
private static chatRoomList: ChatRoom[] = [];
public static userWebsockets: UsersWebsockets[] = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

As we said by oral, i prefer having a userGateway.
But i need your code, so a approve it 😅

@DaiClement DaiClement merged commit e86f28d into main Sep 26, 2022
@DaiClement DaiClement deleted the nad_back_getUserWebsockets branch September 26, 2022 19:37
@DaiClement DaiClement mentioned this pull request Sep 26, 2022
Psycokwet pushed a commit that referenced this pull request Oct 13, 2022
Psycokwet pushed a commit that referenced this pull request Oct 13, 2022
Feat <back> <chat, websockets>: storing websockets and userIds + receiver joins DM Room when creating DM room
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants