-
Notifications
You must be signed in to change notification settings - Fork 0
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
Draft: Model Updates #6
base: main
Are you sure you want to change the base?
Conversation
Updated models to reflected suggested structure for allowing multiple users per conversation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liamptiernan Good job on the PR 👍
Nice work with the database design! 💯
Consider this PR approved ✔️
Please don't merge this PR as it's supposed to be just a draft.
However, please respond to the below questions before closing this request 🙂
- How would you update the socket events in order to reflect these changes in the models?
When we GET conversations now, we must return an array of users instead of the singular
otherUser
property.
- Is there a better way to represent
otherUsers
other than an array that will give us O(1) lookup time? - If we had several developers working on this feature, how would you divide the tasks in order to avoid introducing merge conflicts?
user2 = models.ForeignKey( | ||
User, on_delete=models.CASCADE, db_column="user2Id", related_name="+", | ||
) | ||
users = models.ManyToManyField(User) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Thanks @Fire-Hound! Responses to your questions below
Within the backend group, it would likely be best to have someone solely focused on the database updates and migrations and another person solely focused on API updates. If you have multiple people working on the backend, giving each a separate endpoint to modify would likely be best. Within the frontend group, someone should focus on the API requests and how the front end consumes them. Another person or people should focus on the design updates (how multiple usernames are displayed, add user button, etc). |
Description
Resolves #5
Updated models to reflect suggested structure for allowing multiple users per conversation.
Notes on your approach and thought process
Please note, I reference changes from #4 which implements the read status in the answers to my questions. However, since this PR hasn't been approved and merged yet, those changes won't be included in this branch yet. I'll rebase as soon as #4 is merged into main.
What additional (non-database) changes are needed to implement this feature (we want this to be high level but thorough)?
To complete the implementation of this feature, we need to make a handful of updates to both the API and UI.
API
The conversations endpoint needs to be updated for both the POST and GET methods. When we GET conversations now, we must return an array of users instead of the singular
otherUser
property. When we POST to conversations with read message data, we must use the user ID to appropriately mark which user read the message on the Message object, in addition to when the message was read on the Read object.The messages endpoint also needs to be updated to properly create new conversations with all users added.
Finally, we need to add functionality to the conversations endpoint to allow for the addition of new users to existing conversations, most likely with a PATCH method.
UI
We would need to update a few displays in the UI. The username associated with the conversation would now be a list of usernames. For each of these users, we would need to display their online status. Additionally, the read message feature would need to be updated to mark messages when each user had read them. I.e. there would be multiple avatars marking the last read message for each individual user.
Finally, we would need to allow users to add multiple people to both new and existing conversations. This could be in the form of selecting multiple users when creating a new conversation, as well as an "Add User" button when in an existing conversation.
If our app were already deployed, what steps would we need to take to move to this new feature without disrupting service for current users?
If this app was in production, when we the database and existing conversations to use the new many-to-many users field, we would need to populate that new field with the legacy user IDs from
user1
anduser2
. This would ensure that users still have access to their conversations after the migration.As well, to preserve the message read functionality on messages that have already been read, we'd need to populate the new
readBy
field with the user who read the message (in the previous structure, this would just be the user who didn't send the message). Then, we'd need to update the intermediary model with the time that the message was read by that user.Notes on Other Potential Approaches
I believe that the method I chose allows us to keep the most amount of the existing app intact, but there are other ways we could handle implementing multiple users per conversation. For one, instead of having the relationship be between users and conversations, we could have a many-to-many relationship between messages and users and remove the users foreign key from conversations altogether.
Using this method would allow for a slightly different behavior, where any message could be sent to any user(s). We would then "group" those messages that are sent to the same set of users together into conversations. This would mean that adding a user to an existing conversation would just create a new conversation altogether, rather than adding them to the conversation object. This could provide benefits for privacy / security as adding a user to an existing conversation wouldn't give them access to previous messages, as they would have needed to be on those particular messages in order to have seen them.
Alternatively, we could introduce a 'group' model that would allow for the grouping together of multiple users. In this scenario, we would leave the conversations model untouched and the app would continue to function as is for one on one conversations. However, with groups, users could create collections of users.
Making an entirely new model with a many-to-many relationship to users has a few benefits. For one, we can implement group specific actions such as the ability for users to add or remove other users from a group. This type of functionality is very specific to a group, so we wouldn't want this option to be available in a one on one conversation. Additionally, setting this up as a new model allows us to leave our existing app almost completely intact. There would be a lot less work to be done updating existing database tables, endpoints, etc. thus reducing the likelihood that users would experience issues. Instead we could simply introduce this as a new feature that's available to be used. However, there is a bit more work to be done as we would be starting from scratch in a lot of ways. Not only would we need to create this new model, but we'd need to add new endpoints and extensive UI updates to handle this large addition.
As mentioned before, I believe that the method I chose stays closer to our current structure and functionality, meaning we could implement this change faster, and in a way that would feel familiar to current users.