Closed ericchu94 closed 9 years ago
@ericchu94 Definitely needs review. I'd like to make sure our visions of the project are the same. I want the modules to progress in a good fashion, e.g. we don't have to go back and redo a lot of work to implement new changes.
@FlipEnergy
The chat room thing feels weird. I can see it working, but originally I thought to have chat rooms strongly coupled with users, but this seems to work better.
@ericchu94
Well, chat rooms are how I'm understanding what we want to achieve. Especially as it is in the beginning in Module 1. Afterwards, I'm not so sure either. But I was thinking that having multiple chat rooms can have the same users, and the names for the chat rooms would default as a list of usernames of people in it. Later on, we can have the option to change the chat room name. I can see why it may be confusing but these "chat rooms" are eventually just going to be instances between two or more people, like facebook messenger. Not really like a lounge-type where people just join and leave to chat with randoms.
As for administrating chat rooms, I'm not too sure. I'm just not entirely sure how your approach would be.
Your description of chat rooms from the comment is inconsistent to the BRD. You cannot simply say that you want these chat rooms to be similar to facebook messenger. Define the attributes associated to facebook messenger in the BRD.
Add/modify/remove friends from contact list
How can I modify a friend from the contact list?
join the room the contact is in from contact list
The requirement does not make sense. A contact does not have a one-to-one mapping to a room
If a chatroom consisting of a group of users already exists, users can rejoin chatroom after signing off
This requirement does not make sense. This works to enforce that only chat rooms are unique on the participants field, whilst it is defined that chat rooms are not. It raises the question: which chatroom is the user rejoining into?
That's a good point. I didn't think of that. I want to know how you would describe what you had in mind. If not a chat room type setup, what instead? Should we restrict contacts to only one room or whatever?
I don't know. As long as you can figure out something that works and makes sense.
The first question I would like to solve is whether there can be multiple chats between the same 2 people.
Pros of multiple chats:
Cons of multiple chats:
The use case I'm having trouble with is the following:
Well I don't think multiple chats between the same 2 people should be allowed. It's just unnecessary.
I'm going to edit the the document first. Then let you review it further.
@ericchu94 Please review again.
There needs to be a way for users to leave group chats since a user should be able to opt-out of chatting.
Added comments on commit
@ericchu94
@FlipEnergy You mentioned:
Well I don't think multiple chats between the same 2 people should be allowed. It's just unnecessary.
But it is not in the brd?
Added comments to commit
@ericchu94
Made some changes but as for this comment in Module 5:
Why should this behavior be different than the one-on-one behavior mentioned below?
This way a group chat can be expanded to include more people but if two people are chatting, their chat history is not sacrificed.
I played around with FB messenger and it seems this is the way they do it. There could be a better way. Any ideas? Also any more feature suggestions to add?
Okay I see the benefits of having 2 different behaviors. It seems reasonable
@ericchu94 One more total review please.
@FlipEnergy There is a requirement that each message has a associated timestamp. More comments on commit
@ericchu94 Thanks for the feedback. Please review again.
@ericchu94 Made a new commit based on your comments. Please check again and tell me what you think about deleting a chat instance if all participants left. Is there any point to keep it around? Should we add a rejoin option for a future module?
@FlipEnergy I think we are almost there...
@ericchu94 Review again please.
Added comments
@ericchu94 I thought keeping a one person group chat could be okay as well in case they want to read the chat history. But from last review, I thought you said you you want to disallow single person chats. Did you change your mind?
no
@ericchu94 Then I don't understand what you mean by the most recent commit comment. Please elaborate? Or are you actually asking?
@ericchu94 Made another update. Should be a bit more clear now. Review once more please.
You still have not fixed the previously mentioned issue
A group chat should not be removed if there is only 1 user
There seems to be a contradiction. I'm not sure what you're trying to say. You want to disallow one person chats, but you want a person to be able to still read the chat history if they are the only one left in a group chat.
Similar to below, disallow single user group chats
Why shouldn't the remaining person be able to read the chat history?
Sorry for my wording The following is more clear:
- A user should not be able to chat in a group chat if it is the sole user
- A user must be able to see the history of a group chat if the user did not leave
So you're saying, if a user is the sole participant in a group chat, he/she cannot chat but should still be able to view the chat history? So you want to disable the chatting but not remove the room?
That is a possible solution
Do you think there is a better way?
I don't have any other ideas at the moment
@ericchu94 Once more please.
Looks good to me.
@allenh:
@allenh It has been 4 days. Do you have an update?
@ericchu94 No, not yet. I am still thinking what in the BRD needs to be changed to fit better with FSD
@allenh we are not following a Waterfall model; there can and will be reiterations.
If it is taking so long, it'll be a good idea to just finish this review and get started on your own ticket. If problems arrive, simply open up another issue to address it.
@allenh is there a status update on this issue?
@FlipEnergy @ericchu94 Sorry for the delay of the review, I was quite busy for the past few days.
For the BRD I have some suggestions and question and they are as follows:
@ericchu94 Thoughts?
Changing display name (user name?) is nice, abuse of the system is not. Setup a control system I guess? (Timeouts, absolute limit, limit per duration, etc). Are the display names unique?
Removing is nice as well (that was what I was talking about when I mentioned chat room administration).
I think a leaver should be able to read chat history. How it is implemented is not relevant here; only if it is feasible.
@allenh Added your suggested features. Check again please.
@FlipEnergy It looks good to me.
What happens when a user can't change their username?
What happens when a user has left the room (kicked/voluntary)?
I suppose we should have (popup?) messages for those to tell the user why they can't change their username or if they're kicked or left.
This okay? The message could actually just be added in the chat as like a system message.
Commented on commit
@ericchu94 Made the quick fixes.
I really do not believe a popup is a good user experience. Besides, the mechanism of delivery should not be specified here, it is the requirement of a delivery that should be. Hence business requirements document and functional specifications document.
Store in /documentation
Determine what is the problem that is to be solved, and the solution to the problem. This documentation should be of a very high level.
Examples: Things to include (BRD stuff):
Things not to include (FSD stuff):