add room and chat classes #30 #39

jbesraa · 2017-02-22T08:00:29Z

Add chat and room classes.
relates to #30

Ok, so Ive been working on this issue for last days.

Here is where Iam currently:

When i think about the whole process, I can see that we should have two classes:

chat-room (or channel) class
users class

class chatRoom {

  constructor (roomName) {
    this.roomName = roomName;
    this.endpoints = {};
  }

  addEndpoint (endpointID) {
    this.endpoints[endpointID] = {
      permission: 'CHAT'
    }
  }

}


//mock
FACN = new chatRoom('FACN')
}

let users = [];

class newUser {
  constructor(username){
    this.username = username;
    users.push(username)
  }

  isValid() { //should edit
    if(indexOf(comms.roomid.endpoints[username]) == -1)
    {
      successUsername(this.username);
      showActiveUsers();
    }
    else{
      ErrorUsernameTaken();
    }
  }

}

}

couple of things about these classes:

i mocked a room, but this room should be created when the mentor create it and then we can know which room it is from the url the mentor gives to the students
deleteendpoint function should be added to the chatroom class(useable when user disconnects)
user obj added at the newuser class to keep track on who is currently in the room. though, we can do that differently (on the server):
io[username]=username (we can move username with a socket from the client side to the server side). im not sure which approach we should follow.
isValid function should check if the username is already exist at the specific room. It worked fine with me while i used the socket.username approach(see point 2), i'm not sure about the implementation now.
successUsername should:

add user endpoint to the room endpoints list
remove name field
remove register button(or enter button)
show chat window
show input field
show send button

const successUsername = (username) => {
  comms.roomid.addEndpoint(username);
  username.style.display = 'none';
  register.style.display = 'none';
  chat.style.display = 'block' ;
  message.style.display = 'block';
  send.style.display = 'block';
};

showActiveusers should show the current users in the chat room.
usernametaken should return a message with error about username is not available

CHAT HANDLERS:

Client side:

let register = document.getElementById('register');
let message = document.getElementsByClassName('message');
let username = document.getElementById('username');
let send = document.getElementById('send');

register.addEventListener('click', () => {
  //registration process(creating new user, making sure the username is not exist in the room, adding the user endpoint to the specific room endpoints list)
})

comms.registerHandler('CHATROOM', 'REGISTER', //..)
comms.registerHandler('CHATROOM', 'MESSAGE', //..)
comms.registerHandler('CHATROOM', 'DISCONNECT', //..)

send.addEventListener('click', () => {
  comms.send('CHATROOM', 'TEXTMESSAGE', this.comms[roomid], message.value);
  displayTextMessage(data);
  value='';
})

A) the CB function at the registerHandler. i didnt really know what i should implement in it. in one hand i would redirect to the router at the backend, but because we are using websockets, im doing that at the router file (see example below).
B) im not sure how to start the registration process

Server Side:

const chatHandler = (route, data) => {
  switch (route) {
    case ('CHATROOM.REGISTER') : {
      newuser = new newUser(data)
      newuser.isValid();
      break;
    }

    case ('CHATROOM.MESSAGE') : {
      io.emit('message', data)
      break;
    }

    case ('CHATROOM.DISCONNECT') : {
      //user close window
      break;
    }
  }
};

io.on('connection', (socket) => {
  socket.on('message', (message) => {
    chatHandler('CHATROOM.MESSAGE', message) // point A
  })
  //TODO
  //socket.one('disconnect'..)
})

I) Im not sure if i can chatHandler('CHATROOM.REGISTRATION', data) after io.on('connection, (scoket) =>{
help with that would be appreciated (this issue also refers to point B)

I didnt have time to setup a server and work with it.

at the beginning i build the whole chat with websockets and express server and i could implement alot of things, when i switched and started working with the comms file things got a little bit more complicated.

would love to hear what you think and help with it :)

jbesraa · 2017-02-22T08:04:26Z

useful resources :

https://github.com/dwyl/hapi-socketio-redis-chat-example
https://github.com/smilli/nodejs-chat-complete

njsfield

Nice work :) the changes generally outline that the Server should deal with storing the global Room object (for Server Side Rendering). Please review the client-server model that should help you when making the changes

njsfield · 2017-02-22T12:41:36Z

public/js/chat-channel.js

@@ -0,0 +1,17 @@
+class ChatRoom {


Perhaps change 'ChatRoom' to just Room (as this will hold endpoints that are sending messages to each-other, as well as sending ChatRoom messages).

Perhaps the logic of this function should be built into our current client side Comms file

@esraajb
A rooms object is now initialised in server.js - check the latest version of master
The /create route will be responsible for everything you are doing here:

putting a new room inside the rooms object, based on the roomId

creating a new endpoint

adding "chat" to the permissions

(see no. 4 in https://github.com/foundersandcoders/Live-Peers/blob/2545e0358926c863e5293e147874e3822a5f4c63/docs/sprint1/client-server-model/1.%20Mentor%20View.png)

njsfield · 2017-02-22T12:45:43Z

public/js/chat.js

+// let username = document.getElementById('username');
+
+register.addEventListener('click', () => {
+  // registeration proccess(creating new user, making sure the username is not exist in the room, adding the user endpoint to the specific room endpoints list)


I love this logic! Though it may be tricky to implement in this sprint (instead of the previously suggested method of Server Side rendering), as we'd have to deal with a lot of DOM manipulation

njsfield · 2017-02-22T15:01:59Z

public/js/helpers.js

+const ErrorUsernameTaken = () => {
+  let error = '<p>username taken</p>';
+  document.getElementById('message').insertAdjacentHTML('beforebegin', error);
+  return;


Again, this would be a great implementation if we were opting for a client side rendering approach (single page app) for this sprint

njsfield · 2017-02-22T15:07:23Z

public/js/new-user.js

@@ -0,0 +1,18 @@
+let users = [];
+
+class NewUser {


Class could just be called 'Users', and addUser could be a method inside this users class

njsfield · 2017-02-22T15:14:57Z

public/js/new-user.js

+    users.push(username)
+  }
+
+  isValid() { //should edit


Awesome! Though instead of calling seperate functions, this isValid function should probably just return true or false, then the successUserName function will check whether the isValid function has returned true or false

njsfield · 2017-02-22T15:32:22Z

src/chat-handler.js

+const chatHandler = (route, data) => {
+  switch (route) {
+    case ('CHATROOM.REGISTER') : {
+      newuser = new NewUser(data);


This switch case will be called when a new socket is registering itself (against an existing endpoint in our data store). So a student journey could look like this;

Student registers themselves when first visiting the site (post request to /join)

Server registers the user/endpoint in rooms module

Server sends client view

Client side js fires Websocket for the first time (msg with chatroom.register)

Server adds the sockets id to the rooms object

Can we call this join rather than register?

@njsfield 's point 5 above refers to no. 11 in the mentor journey of the client/server model. So the server-side chat.js needs to contain method like addSocket, which puts the appropriate socketId into the endpoints object.

njsfield · 2017-02-22T15:34:42Z

src/chat-handler.js

+};
+
+io.on('connection', (socket) => {
+  socket.on('message', (message) => {


Awesome! Perhaps on 'disconnect', a message is sent into the above switch statement with 'chat.disconnect' message? Then the clientid will get removed from the room?

jsms90 · 2017-02-22T14:49:52Z

public/js/new-user.js

@@ -0,0 +1,18 @@
+let users = [];
+
+class NewUser {


I reckon the class should be 'users' rather than 'NewUser'? In the same way that we have class 'ChatRoom', not 'NewChatRoom'

a new user is just an instance of 'users'

jsms90 · 2017-02-26T16:35:38Z

public/js/chat-channel.js

@@ -0,0 +1,17 @@
+class ChatRoom {


@esraajb
A rooms object is now initialised in server.js - check the latest version of master
The /create route will be responsible for everything you are doing here:

putting a new room inside the rooms object, based on the roomId

creating a new endpoint

adding "chat" to the permissions

(see no. 4 in https://github.com/foundersandcoders/Live-Peers/blob/2545e0358926c863e5293e147874e3822a5f4c63/docs/sprint1/client-server-model/1.%20Mentor%20View.png)

jsms90 · 2017-02-26T16:38:38Z

public/js/chat.js

+
+comms.registerHandler('CHATROOM', 'REGISTER', /*TODO*/);
+comms.registerHandler('CHATROOM', 'MESSAGE', /*TODO*/);
+comms.registerHandler('CHATROOM', 'DISCONNECT', /*TODO*/);


comms.js comes in at no. 8 & 9 (of latest docs for mentor journey https://github.com/foundersandcoders/Live-Peers/blob/2545e0358926c863e5293e147874e3822a5f4c63/docs/sprint1/client-server-model/1.%20Mentor%20View.png).

I reckon registerHandler is better than send (see no. 9), but addApp might be better?

jsms90 · 2017-02-26T17:24:19Z

src/chat-handler.js

+const chatHandler = (route, data) => {
+  switch (route) {
+    case ('CHATROOM.REGISTER') : {
+      newuser = new NewUser(data);


Can we call this join rather than register?

jsms90 · 2017-02-26T17:26:42Z

src/chat-handler.js

+const chatHandler = (route, data) => {
+  switch (route) {
+    case ('CHATROOM.REGISTER') : {
+      newuser = new NewUser(data);


@njsfield 's point 5 above refers to no. 11 in the mentor journey of the client/server model. So the server-side chat.js needs to contain method like addSocket, which puts the appropriate socketId into the endpoints object.

jbesraa · 2017-02-26T19:55:35Z

Instead of chatHandler, I created chat class

const appendP = (output, comms, from, message) => {
  output.appendChild(elt('p', from + ' : ' + message));
};

class Chat {
  constructor (comms, input, output, form) {
    this.comms = comms;
    this.input = input;
    this.output = output;
    this.form = form;
    this.app = 'CHATROOM';
    this.sys = 'SYSTEM';

    comms.send(this.app, 'REGISTER', this.sys, 'JOINED');
    comms.registerHandler(this.app, 'REGISTER', appendP.bind(null, output));
    comms.registerHandler(this.app, 'MESSAGE', appendP.bind(null, output));
    comms.registerHandler(this.app, 'DISCONNECT', appendP.bind(null, output));

    this.form.onsubmit = (e) => {
      e.preventDefault();
      if (this.input.value) {
        this.comms.send(this.app, 'MESSAGE', this.sys, this.input.value);
        this.input.value = '';
      }
    };

    this.input.onkeydown = (e) => {
      if (e.which === 13 && this.input.value) {
        this.comms.send(this.app, 'MESSAGE', this.sys, this.input.value);
        this.input.value = '';
      }
    };
  }
}

const elt = (type, msg) => {
  const newElt = document.createElement(type);
  newElt.innerHTML = msg;
  return newElt;
};

room class

// Global room class
class Room {

  constructor (roomname) {
    this.roomname = roomname;
    this.endpoints = {};
  }
  addEndpoint (endpointId) {
    this.endpoints[endpointId] = {
      name: '',
      permissions: 'CHAT',
      commsid: ''
    };
  }
  updateEndpointName (endpointId, Name) {
    this.endpoints[endpointId].name = Name;
  }
  updateEndpointPermissions (endpointId, permissions) {
    this.endpoints[endpointId].permissions = permissions;
  }
  updateEndpointCommsID (endpointId, commsid) {
    this.endpoints[endpointId].commsid = commsid;
  }
  removeEndpoint (endpointId) {
    delete this.endpoints[endpointId];
  }
  getRoomName () {
    return this.roomname;
  }
  getEndpointNameFromCommsID (commsid) {
    for (let props in this.endpoints) {
      if (this.endpoints[props].commsid === commsid) {
        return props;
      }
    }
  }
}
module.exports = Room;

big credit to nick

jsms90

Really cool @esraajb

I started writing this class myself, because I needed to call its methods with the routes. But I got myself in a few knots. This looks really clean 😄

Just a few changes before we can merge

jsms90 · 2017-02-27T11:18:56Z

src/room.js

+    };
+  }
+  updateEndpointName (endpointId, Name) {
+    this.endpoints[endpointId].name = Name;


any reason why Name starts with a capital?

jsms90 · 2017-02-27T11:20:24Z

src/room.js

+    };
+  }
+  updateEndpointName (endpointId, Name) {
+    this.endpoints[endpointId].name = Name;


this is not the "endpoint name", this is the user's username. Can we change this variable to username @njsfield?

any reason why the parameter Name starts with a capital letter?

jsms90 · 2017-02-27T11:22:10Z

src/room.js

+    this.endpoints[endpointId] = {
+      name: '',
+      permissions: 'CHAT',
+      commsid: ''


Rather than initialising endpoints with some empty strings, we could either have a few more methods:

addUsername

addPermissions

addCommsId

Or change the "update" methods so that they can also be used for adding username etc when they don't yet exist

jsms90 · 2017-02-27T11:22:59Z

src/room.js

+  updateEndpointName (endpointId, Name) {
+    this.endpoints[endpointId].name = Name;
+  }
+  updateEndpointPermissions (endpointId, permissions) {


This naming is slightly redundant. Can we just change these to:

updateUsername

updatePermissions

updateCommsId

?

jsms90 · 2017-02-27T11:23:05Z

src/room.js

+    };
+  }
+  updateEndpointName (endpointId, Name) {
+    this.endpoints[endpointId].name = Name;


this is not the "endpoint name", this is the user's username. @njsfield @esraajb Can we change this variable to username?

@esraajb any reason why the parameter Name starts with a capital letter?

jsms90 · 2017-02-27T11:23:10Z

src/room.js

+    this.endpoints[endpointId] = {
+      name: '',
+      permissions: 'CHAT',
+      commsid: ''


Rather than initialising endpoints with some empty strings, we could either:

have a few more methods:

addUsername

addPermissions

addCommsId

change the "update" methods so that they can also be used for adding new username/permissions/commsId. For example:

updateUsername (endpointId, username) { this.endpoints[endpointId][username] = username; }

jsms90 · 2017-02-27T11:34:29Z

src/room.js

+  updateEndpointPermissions (endpointId, permissions) {
+    this.endpoints[endpointId].permissions = permissions;
+  }
+  updateEndpointCommsID (endpointId, commsid) {


I know this sounds really pedantic 😅, but please can we be consistent with capitalising: commsID vs commsId vs commsid.

Everywhere else, we have capital I, lowercase d 👍

njsfield · 2017-02-27T11:49:55Z

public/js/chat.js

@@ -0,0 +1,34 @@
+const appendP = (output, comms, from, message) => {


Remove 'comms' argument. This will be the implementation of Comms.js on the client side...

The chatroom/webrtc module constructors will take the comms instance, so for our comms.js module instead of this-

self.handlers[msg.app + '.' + msg.method](this, msg.from, msg.params);

The comms.js will be calling its handlers like this-

self.handlers[msg.app + '.' + msg.method](msg.from, msg.params);

jsms90 · 2017-02-27T11:51:09Z

@esraajb Can you change the name of this pull to be descriptive about what you're actually asking to be merged, and update the body to say that it relates to the relevant issue, please?

jbesraa · 2017-02-27T12:16:00Z

@jsms90 please see last two commits
and let me know if i missed something ?

njsfield · 2017-02-27T12:25:56Z

src/room.js

+      commsId: ''
+    };
+  }
+  updateUsernameame (endpointId, username) {


spelling should be updateUsername

njsfield · 2017-02-27T12:36:46Z

src/room.js

+      if (this.endpoints[props].commsId === commsId) {
+        return props;
+      }
+    }


We should also have a getCommsId method, that takes an endpointId as its argument and returns the endpoints comms id

jsms90 · 2017-02-27T15:13:22Z

public/js/chat.js

+    this.input = input;
+    this.output = output;
+    this.form = form;
+    this.app = 'CHATROOM';


CHAT instead of CHATROOM now

jsms90 · 2017-02-27T15:18:01Z

src/room.js

+    this.endpoints = {};
+  }
+  addEndpoint (endpointId) {
+    this.endpoints[endpointId] = {


Still got this issue from before:

Rather than initialising endpoints with some empty strings, we should just have this.endpoints = {}

Whereas the addEndpoint method should create a new endpointId, like I have done here:

addEndpoint () { let endpointId = // function to create a random string this.endpoints[endpointId] = {}; }

And then we need a few more methods:

addUsername

addPermissions

addCommsId

Im not sure we need to add these new functions.
I think the update functions will do the job as they will create the property if its not already exist.

all we need to do is use brackets as you said.

updateUsername (endpointId, username) { this.endpoints[endpointId]['username'] = username; }

jsms90 · 2017-02-27T15:30:16Z

src/room.js

+    };
+  }
+  updateUsername (endpointId, username) {
+    this.endpoints[endpointId].username = username;


I think you need to use bracket syntax, if username is not already defined? So:

this.endpoints[endpointId].username = username

(Same for the other update functions)

@njsfield @esraajb I might be wrong about that? But I think it wasn't working for me when I used dot syntax like this elsewhere.

jsms90 · 2017-02-27T21:48:17Z

src/room.js

+    this.endpoints = {};
+  }
+  addEndpoint (endpointId) {
+    this.endpoints[endpointId] = {};


I still think it would be better to have an the endpointId string being created by this addEndpoint method. That way, the apps themselves aren't left to declare an endpointId in whatever form they choose. @njsfield @esraajb Do you disagree?

See my last comment:

the addEndpoint method should create a new endpointId, like I have done here:

addEndpoint () { let endpointId = // function to create a random string this.endpoints[endpointId] = {}; }

Or you could separate it further, if you wanted to be pure with your functions:

createEndpoint () { let endpointId = Math.random().toString(36).slice(2); return endpointId; } addEndpoint (endpointId) { this.endpoints[endpointId] = {}; }

But there's little point, since you'll never create an endpoint, separately from adding it to the endpoints object

jsms90

I feel like some of the changes I suggested earlier still stand, but it's probably just worth merging this now and dealing with it later, if we need to 👍

jsms90 · 2017-02-27T22:27:00Z

src/room.js

+    this.endpoints = {};
+  }
+  addEndpoint (endpointId) {
+    this.endpoints[endpointId] = {};


Or you could separate it further, if you wanted to be pure with your functions:

createEndpoint () { let endpointId = Math.random().toString(36).slice(2); return endpointId; } addEndpoint (endpointId) { this.endpoints[endpointId] = {}; }

But there's little point, since you'll never create an endpoint, separately from adding it to the endpoints object

related to #30

c9879bb

jbesraa requested review from jsms90 and njsfield February 22, 2017 08:01

njsfield reviewed Feb 22, 2017

View reviewed changes

jsms90 suggested changes Feb 26, 2017

View reviewed changes

jbesraa added 3 commits February 27, 2017 08:58

func that create element wiht the the message as innerhtml

8b653b9

add chat class

6c2d77d

add room class

4402ac3

jsms90 suggested changes Feb 27, 2017

View reviewed changes

jsms90 mentioned this pull request Feb 27, 2017

add create route #56

Merged

rename functions and consistent naming for commsId

afe2b06

njsfield reviewed Feb 27, 2017

View reviewed changes

njsfield mentioned this pull request Feb 27, 2017

Update client side comms #60

Merged

jbesraa changed the title ~~related to #30~~ add room and chat classes #30 Feb 27, 2017

remove comms from appendP

68f7664

njsfield reviewed Feb 27, 2017

View reviewed changes

src/room.js Outdated

commsId: ''

};

}

updateUsernameame (endpointId, username) {

Copy link

Collaborator

njsfield Feb 27, 2017

Choose a reason for hiding this comment

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

spelling should be updateUsername

njsfield reviewed Feb 27, 2017

View reviewed changes

jbesraa added 2 commits February 27, 2017 16:57

fix typo

066d724

add getcommsid func

3224cbe

jbesraa requested review from njsfield and jsms90 February 27, 2017 15:08

jsms90 reviewed Feb 27, 2017

View reviewed changes

jbesraa added 2 commits February 27, 2017 17:18

change chatroom to chat

0b2dd57

remove username and commsid from addendpoint

4e6a353

jsms90 suggested changes Feb 27, 2017

View reviewed changes

fix update functions

89b293d

fix commsId

b8fd91a

njsfield approved these changes Feb 27, 2017

View reviewed changes

jsms90 suggested changes Feb 27, 2017

View reviewed changes

jsms90 reviewed Feb 27, 2017

View reviewed changes

jsms90 merged commit 80175be into master Feb 27, 2017

		@@ -0,0 +1,34 @@
		const appendP = (output, comms, from, message) => {

add room and chat classes #30 #39

add room and chat classes #30 #39

Conversation

jbesraa commented Feb 22, 2017 • edited Loading

Add chat and room classes. relates to #30

jbesraa commented Feb 22, 2017

njsfield left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbesraa commented Feb 26, 2017

jsms90 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsms90 commented Feb 27, 2017

jbesraa commented Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsms90 Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

jbesraa Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsms90 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbesraa commented Feb 22, 2017 •

edited

Loading

Add chat and room classes.
relates to #30

jbesraa commented Feb 27, 2017 •

edited

Loading

jsms90 Feb 27, 2017 •

edited

Loading

jbesraa Feb 27, 2017 •

edited

Loading