Chat refactorization

[omgdory]

Profile Models Extended

Refactors existing Chat and Message models and views to more appropriately match the given issue.

---

Overview

What does this PR do?

Refactors existing Chat and Message models and views to more appropriately match the given issue. Associated files have been changed.

---

Key Features & Changes

What has been added or changed?

✔ Chat and Message models have been changed
✔ Database querying responsibility yielded from models.py to managers.py
✔ Views: Reworked the CRUD endpoints by using Django's existing ModelViewSet.
✔ Serializers for the corresponding models

---

Why This Is Needed

What problem does this solve?

To more appropriately handle the Chat and Message models, as the previous iteration was rudimentary. This new implementation is more streamlined.

---

Related Issue

🔗 This PR addresses Issue #26

---

Implementation Details

How was this feature developed?

Django

---

How to Test This PR

Step-by-step testing guide

1. Navigate to the backend folder.
2. Enable your virtual environment. For Mac/Linux (bash/zsh) users, this is env/Scripts/activate. For Windows (PowerShell) users, this is .\env\Scripts\Activate.ps1.
3. Run pip install -r requirements.txt
4. Run python manage.py migrate
5. Run python manage.py test apps.chat
6. Ensure that all tests pass.
7. Navigate to apps/chat/tests.py and ensure that the tests make sense, and reflect appropriate usage.

---

Files Added/Modified

📂 List of important files changed in this PR

File Name	Description
backend/apps/chat/managers.py	Database querying for the models
backend/apps/chat/models.py	Refactored for managers.py
backend/apps/chat/serializers.py	Serializers to translate between models and JSON
backend/apps/chat/tests.py	Testing use cases of implemented endpoints
backend/apps/chat/urls.py	URLs for the API endpoints
backend/apps/chat/views.py	The actual API endpoints (heavily refactored)

---

AI Code Usage

🤖 Was AI-generated code used in this PR?

• Yes (For outlining the structure of the implementation and for handling boilerplate Django code)
• No

---

Checklist for Reviewers

Things to check before approving this PR:

• Does the feature work as expected?
• Is the code clean, modular, and well-documented?
• Do the tests pass?
• Do the tests make sense?

---

Next Steps & Future Enhancements

What improvements can be made after merging?

• Implement frontend requests and work with frontend teammates to ensure that everything works appropriately
• Refactor views for different/new API endpoint functionality if needed

---

[cj-ballesteros]

I'm not assigned to this, but I just wanna say, this PR looks great!

[EthanZ23]

🔍 Peer Review: Amazing Work on This PR!

Hey Dorian 👋 — I just finished reviewing your PR titled "Chat refactorization #149".

Here’s my feedback:

---

✅ What Looks Great

• The code is indeed modular, readable, and maintainable, great job!
• The core implementation of the API endpoints + CRUD for enabling chat functionality works as expected.
• All tests passed successfully.
• The tests are logical and well structured. I reviewed apps/chat/tests.py and can confirm that they reflect appropriate usage and expected behavior.
• Nice utilization of AI to assist you with refactoring your models.py, and with code structure in other areas.

From my review:

• The move to custom managers (ChatManager, MessageManager) is clean and follows Django best practices.
• The serializers are well-structured and make good use of nested serializers and read_only fields.
• Including tests for chat creation, duplicate prevention, message posting, and permission enforcement adds great value and shows strong test coverage.

---

💡 Suggestions for Improvement (if any)

These are ideas for future improvements.

• Consider the few comments provided, many can be fixed or added in the next PR.
• (Optional suggestion for views.py) Implement pagination in messages method of ChatViewSet. Namely because in ChatViewSet.messages(), if chats grow large, pagination might be worth considering to avoid performance hits.
• You could add a few negative test cases in a future PR. For instance, sending a message in a chat you're not part of, or creating a chat with yourself.

---

🧪 Tested This Feature

I ran the following test steps:

• ✅ Reviewed and verified each modified file: managers.py, models.py, serializers.py, tests.py, urls.py, and views.py.
• ✅ Followed the step-by-step testing guide: All tests passed successfully.
• ✅ Checked the Chat model in the Django admin panel for correct behavior.

---

🔄 Next Steps

• Coordinate with frontend teammates to integrate this with the frontend chat UI.
• Add validation-focused and permission-based negative test cases in the next iteration.

---

🚀 Final Thoughts

Awesome work, Dorian! The PR is in great shape to be merged. You're making strong contributions to the project. If anything is unclear, please let me know. Aside of the few comments/suggestions I left, this PR will be approved. Keep up the great work.

-- Ethan

[alliekameda]

🔍 Peer Review: Splendid Work on This PR!

Dory, hi!! 👋 :wcatjam: — just finished reviewing your PR "Chat refactorization #149". Here’s my feedback:

---

✅ What Looks Great

• The core chat and messaging features are implemented cleanly.
• Excellent use of ModelViewSet and token-based authentication.
• Clear separation of business logic into managers.py.
• Meta, related_name, and serializers are all nicely structured.
• Thorough test coverage. Both success cases and interactions between components are covered.

---

💡 Suggestions for Improvement

models.py

• Consider enforcing the user1 < user2 constraint via a clean() method to ensure consistent ordering beyond get_or_create_chat.

def clean(self):
    if self.user1.id > self.user2.id:
        self.user1, self.user2 = self.user2, self.user1

• Clean up commented-out code like the legacy save() method for brevity (can always retrieve from Git history).

managers.py

• Use a helper like normalize_users() to clarify the user ordering logic.

def normalize_users(self, user1, user2):
    return (user2, user1) if user1.id > user2.id else (user1, user2)

• Wrap message creation and chat update inside a transaction.atomic() block to avoid partial updates if something fails.

from django.db import transaction

def create_message(self, chat, sender, content):
    with transaction.atomic():
        message = self.create(chat=chat, sender=sender, content=content)
        chat.updatedAt = now()
        chat.save()
    return message

views.py

• You might override create() instead of perform_create() for more control and consistent return handling.
• Add a validation check to prevent users from creating chats with themselves.

if user1 == user2:
    raise serializers.ValidationError("Cannot create chat with yourself. :(")

• Paginate message histories like Ethan suggested with PageNumberPagination.

from rest_framework.pagination import PageNumberPagination

class MessagePagination(PageNumberPagination):
    page_size = 20

serializers.py

• Add validation for empty or whitespace-only messages in validate_content.

def validate_content(self, value):
    if not value.strip():
        raise serializers.ValidationError("Message cannot be empty.")
    return value

• Use PrimaryKeyRelatedField(read_only=True) for user fields to avoid exposing full user objects unnecessarily.

user1 = serializers.PrimaryKeyRelatedField(read_only=True)

tests.py

• Like Ethan suggested, consider adding edge case tests for:
  • Empty or invalid message content.
  • Creating chats with yourself.
  • Accessing a chat the user isn’t part of.
• You could also DRY up test setup using a user factory or helper method to reduce duplication.

def create_user(self, username):
    return User.objects.create_user(username=username, password='testpass')

urls.py

• The explicit send_message and chat_list/chat_detail endpoints might be redundant given ModelViewSet, consider simplifying.
• Adding an app_name = 'chat' helps with reverse URL resolution and namespacing down the line.

---

🧪 Tested This Feature

I ran the following test steps:

• ✅ Created chats between different users and verified uniqueness logic.
• ✅ Sent messages and verified timestamps and order.
• ✅ Hit edge cases (e.g., duplicate chat, chat with self).
• ✅ Reviewed API response structures for clarity and correctness.

---

🔄 Next Steps

• Let me know if you want help implementing the optional suggestions (pagination, validation helpers, cleanup).
• Once updated or confirmed, I’ll gladly re-review and approve for merge!

---

🚀 Final Thoughts

This PR is solid, readable, testable, and will scale well. Thanks for all your effort, Dory!! ≽(•⩊ •マ≼ ✨

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@94d0176). Learn more about missing BASE report.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #149   +/-   ##
=======================================
  Coverage        ?   57.05%           
=======================================
  Files           ?       14           
  Lines           ?      475           
  Branches        ?       37           
=======================================
  Hits            ?      271           
  Misses          ?      196           
  Partials        ?        8           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[alliekameda]

Everything still looks good :)

[EthanZ23]

✅ Approved

Thanks for quickly addressing the feedback! The updates/additions were made efficiently, and I appreciate how you clearly answered all my questions. Everything looks great, solid work and clean implementation throughout.