Issue 7 Add room app #52

jasonkeo · 2025-12-03T09:17:27Z

Change Summary

This is the room app model (very rough draft)

I added fields based on model
i also didnt add the image bucket yet oof
i used the viewset library to make api endpoint as the api requirements looked like just CRUD operations and viewset provides CRUD operations already
per requirements i added custom with responses to the api tho
also i had to router for urls from restframe work cus it goes hand in hand with viewset
all in all this is very incomplete but i wanted to put it up just in case it is halting project progress

to test run python manage.py test

Change Form

Fill this up (NA if not available). If a certain criteria is not met, can you please give a reason.

The pull request title has an issue number
The change works by "Smoke testing" or quick testing
The change has tests
The change has documentation

Other Information

[Is there anything in particular in the review that I should be aware of?]

Resolves:
Issue #7

Copilot

Pull request overview

This PR introduces a new Room API for a room booking system, implementing CRUD operations with custom response formats and permission-based access control. The implementation uses Django REST Framework's ModelViewSet pattern to provide standardized API endpoints.

Key Changes:

Added Room model with fields for name, image URL, location/capacity IDs, datetime ranges, and recurrence rules
Implemented REST API with filtering by name, location_id, and capacity_id
Added admin-only permissions for create/update/delete operations while allowing public read access

Reviewed changes

Copilot reviewed 11 out of 13 changed files in this pull request and generated 19 comments.

Show a summary per file

File	Description
server/api/room/models.py	Defines the Room model with booking-related fields and timestamp tracking
server/api/room/serializers.py	Creates ModelSerializer exposing all Room fields
server/api/room/views.py	Implements RoomViewSet with custom update/delete responses and query filtering
server/api/room/urls.py	Configures DRF router for room endpoints
server/api/urls.py	Registers room API URLs under /api/rooms/
server/api/settings.py	Adds room app to INSTALLED_APPS
server/api/room/tests.py	Provides API tests for list, retrieve, update, and delete operations
server/api/room/admin.py	Registers Room model in Django admin with custom display fields
server/api/room/apps.py	Configures the room Django app
server/api/room/migrations/0001_initial.py	Initial migration creating the Room table
server/api/room/migrations/0002_alter_room_created_at_alter_room_updated_at.py	Updates created_at/updated_at fields to use auto timestamps

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-03T09:24:49Z

server/api/room/models.py

+    start_datetime = models.DateTimeField()
+    end_datetime = models.DateTimeField()
+    recurrence_rule = models.TextField()
+    created_at = models.DateTimeField(auto_now_add=True)
+    updated_at = models.DateTimeField(auto_now=True)
+
+    def __str__(self):
+        return self.name


The model design appears conceptually flawed for a room booking system. The start_datetime, end_datetime, and recurrence_rule fields seem to represent booking/availability information rather than room properties. A Room should typically contain static properties (name, location, capacity), while booking information should be in a separate related model (e.g., RoomBooking or RoomAvailability) with a ForeignKey to Room. This would allow multiple bookings per room.

Suggested change

start_datetime = models.DateTimeField()

end_datetime = models.DateTimeField()

recurrence_rule = models.TextField()

created_at = models.DateTimeField(auto_now_add=True)

updated_at = models.DateTimeField(auto_now=True)

def __str__(self):

return self.name

created_at = models.DateTimeField(auto_now_add=True)

updated_at = models.DateTimeField(auto_now=True)

def __str__(self):

return self.name

class RoomBooking(models.Model):

id = models.AutoField(primary_key=True)

room = models.ForeignKey(Room, on_delete=models.CASCADE, related_name='bookings')

start_datetime = models.DateTimeField()

end_datetime = models.DateTimeField()

recurrence_rule = models.TextField(blank=True, null=True)

created_at = models.DateTimeField(auto_now_add=True)

updated_at = models.DateTimeField(auto_now=True)

def __str__(self):

return f"Booking for {self.room.name} from {self.start_datetime} to {self.end_datetime}"

Copilot · 2025-12-03T09:24:50Z

server/api/room/views.py

+        return Response({
+            "id": instance.id,
+            "name": instance.name,
+            "updated_at": instance.updated_at
+        })


The custom update response doesn't follow REST conventions. The response returns only id, name, and updated_at but omits other fields that may have been updated. This could confuse API consumers who expect to see the full updated resource or at least all modified fields. Consider returning the complete serialized object or using the standard serializer response.

Suggested change

return Response({

"id": instance.id,

"name": instance.name,

"updated_at": instance.updated_at

})

return Response(serializer.data)

Copilot · 2025-12-03T09:24:50Z

server/api/room/models.py

+
+
+class Room(models.Model):
+    id = models.AutoField(primary_key=True)
+    name = models.TextField()
+    img_url = models.TextField()
+    location_id = models.IntegerField()
+    capacity_id = models.IntegerField()


The location_id and capacity_id fields use IntegerField without any validation constraints. These fields could accept negative values or zero, which likely don't make sense for IDs. Consider adding validation with PositiveIntegerField or adding validators like MinValueValidator(1) to ensure valid ID values.

Suggested change

class Room(models.Model):

id = models.AutoField(primary_key=True)

name = models.TextField()

img_url = models.TextField()

location_id = models.IntegerField()

capacity_id = models.IntegerField()

from django.core.validators import MinValueValidator

class Room(models.Model):

id = models.AutoField(primary_key=True)

name = models.TextField()

img_url = models.TextField()

location_id = models.PositiveIntegerField(validators=[MinValueValidator(1)])

capacity_id = models.PositiveIntegerField(validators=[MinValueValidator(1)])

Copilot · 2025-12-03T09:24:51Z

server/api/room/serializers.py

+class RoomSerializer(serializers.ModelSerializer):
+    class Meta:
+        model = Room
+        fields = "__all__"


Using fields = "__all__" exposes all model fields including auto-generated fields like created_at and updated_at which shouldn't typically be writable via the API. Consider explicitly listing fields or using read_only_fields = ['id', 'created_at', 'updated_at'] to prevent clients from modifying these fields.

Suggested change

fields = "__all__"

fields = "__all__"

read_only_fields = ['id', 'created_at', 'updated_at']

Copilot · 2025-12-03T09:24:51Z

server/api/room/models.py

+
+
+class Room(models.Model):
+    id = models.AutoField(primary_key=True)


The id field is explicitly defined but Django automatically creates an AutoField primary key named id for models. This redundant field definition should be removed unless there's a specific reason to override the default behavior.

Suggested change

id = models.AutoField(primary_key=True)

Copilot · 2025-12-03T09:24:54Z

server/api/room/tests.py

+    def test_retrieve_update_delete_room(self):
+        # Get a room
+        room = Room.objects.first()
+        response = self.client.get(f"/api/rooms/{room.id}/")
+        self.assertEqual(response.status_code, status.HTTP_200_OK)
+        self.assertEqual(response.data["name"], room.name)
+
+        # Update room
+        response = self.client.patch(f"/api/rooms/{room.id}/", {"name": "Updated Room"}, format="json")
+        self.assertEqual(response.status_code, status.HTTP_200_OK)
+        self.assertEqual(response.data["name"], "Updated Room")
+
+        # Check via GET
+        response = self.client.get(f"/api/rooms/{room.id}/")
+        self.assertEqual(response.data["name"], "Updated Room")
+
+        # Delete room
+        response = self.client.delete(f"/api/rooms/{room.id}/")
+        self.assertEqual(response.status_code, status.HTTP_200_OK)
+        self.assertEqual(response.data["message"], "Room deleted successfully")
+
+        # Check room count
+        response = self.client.get("/api/rooms/")
+        self.assertEqual(len(response.data), 4)


Missing test coverage for the create operation (POST). The tests verify list, retrieve, update, and delete operations, but don't test creating a new room through the API. Consider adding a test for POST requests to ensure the create functionality works correctly with admin permissions.

Copilot · 2025-12-03T09:24:54Z

server/api/room/views.py

+            qs = qs.filter(location_id=loc)
+
+        if cap := params.get("capacity_id"):
+            qs = qs.filter(capacity_id=cap)
+


No validation is performed on the location_id and capacity_id filter parameters. Invalid values (e.g., non-numeric strings) could cause database errors. Consider adding validation or using try-except blocks to handle ValueError when converting these parameters, or add error handling in the queryset filtering.

Suggested change

qs = qs.filter(location_id=loc)

if cap := params.get("capacity_id"):

qs = qs.filter(capacity_id=cap)

try:

loc_int = int(loc)

qs = qs.filter(location_id=loc_int)

except (ValueError, TypeError):

pass

if cap := params.get("capacity_id"):

try:

cap_int = int(cap)

qs = qs.filter(capacity_id=cap_int)

except (ValueError, TypeError):

pass

Copilot · 2025-12-03T09:24:55Z

server/api/room/models.py

+    start_datetime = models.DateTimeField()
+    end_datetime = models.DateTimeField()


The model lacks validation to ensure end_datetime is after start_datetime. This could lead to invalid room data where the end time is before the start time. Consider adding a clean() method or model validation to enforce this constraint, or add a CheckConstraint in the model's Meta class.

Copilot · 2025-12-03T09:24:55Z

server/api/room/models.py

+    id = models.AutoField(primary_key=True)
+    name = models.TextField()
+    img_url = models.TextField()
+    location_id = models.IntegerField()
+    capacity_id = models.IntegerField()
+    start_datetime = models.DateTimeField()
+    end_datetime = models.DateTimeField()
+    recurrence_rule = models.TextField()
+    created_at = models.DateTimeField(auto_now_add=True)
+    updated_at = models.DateTimeField(auto_now=True)
+


[nitpick] The Room model lacks a docstring explaining what it represents and the purpose of its fields. This is especially important given the potentially confusing design where booking/availability fields are mixed with room properties. Consider adding a class docstring and field-level comments for clarity.

Suggested change

id = models.AutoField(primary_key=True)

name = models.TextField()

img_url = models.TextField()

location_id = models.IntegerField()

capacity_id = models.IntegerField()

start_datetime = models.DateTimeField()

end_datetime = models.DateTimeField()

recurrence_rule = models.TextField()

created_at = models.DateTimeField(auto_now_add=True)

updated_at = models.DateTimeField(auto_now=True)

"""

Represents a room that can be booked in the system.

This model combines static room properties (such as name, image, location, and capacity)

with booking/availability fields (such as start/end datetime and recurrence rule).

Fields:

id (AutoField): Unique identifier for the room.

name (TextField): The name of the room.

img_url (TextField): URL to an image representing the room.

location_id (IntegerField): Identifier for the location of the room.

capacity_id (IntegerField): Identifier for the room's capacity.

start_datetime (DateTimeField): Start date and time for room availability or booking.

end_datetime (DateTimeField): End date and time for room availability or booking.

recurrence_rule (TextField): Recurrence rule for repeated bookings (e.g., weekly).

created_at (DateTimeField): Timestamp when the room entry was created.

updated_at (DateTimeField): Timestamp when the room entry was last updated.

"""

id = models.AutoField(primary_key=True) # Unique identifier for the room

name = models.TextField() # The name of the room

img_url = models.TextField() # URL to an image representing the room

location_id = models.IntegerField() # Identifier for the location of the room

capacity_id = models.IntegerField() # Identifier for the room's capacity

start_datetime = models.DateTimeField() # Start date and time for room availability or booking

end_datetime = models.DateTimeField() # End date and time for room availability or booking

recurrence_rule = models.TextField() # Recurrence rule for repeated bookings (e.g., weekly)

created_at = models.DateTimeField(auto_now_add=True) # Timestamp when the room entry was created

updated_at = models.DateTimeField(auto_now=True) # Timestamp when the room entry was last updated

Copilot · 2025-12-03T09:24:55Z

server/api/room/models.py

+    name = models.TextField()
+    img_url = models.TextField()


TextField is typically used for large text content. For a room name and image URL, CharField with a max_length parameter would be more appropriate. Consider using models.CharField(max_length=255) for name and models.URLField(max_length=500) for img_url to better represent the data type and add validation.

Suggested change

name = models.TextField()

img_url = models.TextField()

name = models.CharField(max_length=255)

img_url = models.URLField(max_length=500)

Fozzyack · 2025-12-03T12:47:44Z

@ErikaKK. Some considerations. I am unsure how consistent blooms booking schedule is but here are some cases to think about.

Different hours on weekends,
Public holidays or one-off closures,
Temporary special hours (renovation, summer schedule, event)

Also might be worth moving the capacity as being a field in the room model. Can handle the naming (small, medium, large etc.) on the frontend.

ErikaKK · 2025-12-05T17:06:09Z

Please rebase main

ErikaKK

Please have a look at the copilot suggestions. Some permissions and fields need to change a bit. pls use /swagger to test you api endpoints. and it seems like the post request body for /api/rooms/ doesn't have all the field we want and doesn't have the permission set up.

ErikaKK · 2025-12-07T16:08:35Z

server/api/room/models.py

+class Room(models.Model):
+    id = models.AutoField(primary_key=True)
+    name = models.TextField(blank=False)
+    img = models.ImageField(upload_to='') # change to upload s3 later for deployment?


img = models.ImageField(upload_to='rooms/')

ErikaKK · 2025-12-07T16:09:17Z

server/api/room/models.py

+    location = models.ForeignKey(Location, on_delete=models.CASCADE, blank=False)
+    capacity = models.IntegerField(blank=False)
+    amenities = models.ManyToManyField(Amenties, blank=True)
+    status = models.BooleanField(default=True)


pls add docs for this

ErikaKK · 2025-12-07T16:16:15Z

server/api/room/views.py

+            "updated_at": instance.updated_at
+        })
+
+    def destroy(self, request, *args, **kwargs):


Actually we don't need to delete the rooms, we'll just update the status of a room. So we won't need the delete method

ErikaKK · 2025-12-07T16:18:51Z

server/api/room/views.py

+        serializer.is_valid(raise_exception=True)
+        serializer.save()
+
+        return Response({


the patch method could just response "success" if it's 200, because after second thought, I don't think we need to use any of this fields after we send this request. Feel free to let me know your thoughts

ErikaKK · 2025-12-07T16:26:23Z

server/api/room/models.py

+
+class Location(models.Model):
+    id = models.AutoField(primary_key=True)
+    name = models.TextField(blank=False)


please use models.CharField(blank=False) for the string field and set set an appropriate for the field

ErikaKK · 2025-12-07T16:27:29Z

server/api/room/models.py

all the TextField need to be replaced with CharField

ErikaKK · 2025-12-07T16:30:08Z

server/api/room/models.py

+    status = models.BooleanField(default=True)
+    start_datetime = models.DateTimeField(blank=False)
+    end_datetime = models.DateTimeField(blank=False)
+    recurrence_rule = models.TextField(blank=False)


this is an optional field

ErikaKK · 2025-12-07T16:39:35Z

server/api/settings.py

 # STATIC_ROOT is where the static files get copied to when "collectstatic" is run.
 STATIC_ROOT = "static_files"

+MEDIA_ROOT = BASE_DIR / "room_images"


you can remove this, I already config this for s3

ErikaKK · 2025-12-07T16:42:40Z

server/api/room/views.py

+
+    def get_permissions(self):
+        if self.action in ["create", "update", "destroy"]:
+            return [permissions.IsAdminUser()]


here we should have permissions.IsAuthenticated() for user who has an account

ErikaKK · 2025-12-07T16:43:43Z

server/api/urls.py

 urlpatterns = [
    path("admin/", admin.site.urls),
    path("api/healthcheck/", include(("api.healthcheck.urls"))),
+<<<<<<< HEAD


please resolve conflict here

Copilot AI review requested due to automatic review settings December 3, 2025 09:17

Copilot started reviewing on behalf of jasonkeo December 3, 2025 09:17 View session

Copilot finished reviewing on behalf of jasonkeo December 3, 2025 09:22

Copilot AI reviewed Dec 3, 2025

View reviewed changes

Fozzyack changed the title ~~Add room app #7~~ Issue 7 Add room app Dec 3, 2025

rozza368 mentioned this pull request Dec 6, 2025

Create Comprehensive Tests for Rooms App #68

Open

jasonkeo added 19 commits December 7, 2025 15:20

Create & Migrate Room Model

8da87e8

Add Serializer

3d429db

rebase

a7d522c

Add more test cases

805e9bd

Add comments to explain api

94c9cf2

added location + amenties table

63aac92

Fixing chatgpted test cases

5138bab

Fixing Api responses to match requirement description

40e818f

fix merge errors

541e107

more fixes

adc116b

added location & capicity to django admin page

4ac62fd

fix add model to admin

4e29146

add amenties to admin

3609faa

add image upload to local file

17a7c6e

fix api response

aca4760

fix naming of fields from location_id to location? might cause errors?

08e9e08

add status & change capicity to int

01769cd

Make test better

62315a0

More fixes to test

490d725

jasonkeo force-pushed the Add-room-app-#7 branch from 2d52ad8 to 490d725 Compare December 7, 2025 15:30

ErikaKK reviewed Dec 7, 2025

View reviewed changes

	fields = "__all__"
	fields = "__all__"
	read_only_fields = ['id', 'created_at', 'updated_at']



		class Room(models.Model):
		id = models.AutoField(primary_key=True)

-            qs = qs.filter(location_id=loc)
-        if cap := params.get("capacity_id"):
-            qs = qs.filter(capacity_id=cap)
+            try:
+                loc_int = int(loc)
+                qs = qs.filter(location_id=loc_int)
+            except (ValueError, TypeError):
+                pass
+        if cap := params.get("capacity_id"):
+            try:
+                cap_int = int(cap)
+                qs = qs.filter(capacity_id=cap_int)
+            except (ValueError, TypeError):
+                pass

		start_datetime = models.DateTimeField()
		end_datetime = models.DateTimeField()

-    id = models.AutoField(primary_key=True)
-    name = models.TextField()
-    img_url = models.TextField()
-    location_id = models.IntegerField()
-    capacity_id = models.IntegerField()
-    start_datetime = models.DateTimeField()
-    end_datetime = models.DateTimeField()
-    recurrence_rule = models.TextField()
-    created_at = models.DateTimeField(auto_now_add=True)
-    updated_at = models.DateTimeField(auto_now=True)
+    """
+    Represents a room that can be booked in the system.
+    This model combines static room properties (such as name, image, location, and capacity)
+    with booking/availability fields (such as start/end datetime and recurrence rule).
+    Fields:
+        id (AutoField): Unique identifier for the room.
+        name (TextField): The name of the room.
+        img_url (TextField): URL to an image representing the room.
+        location_id (IntegerField): Identifier for the location of the room.
+        capacity_id (IntegerField): Identifier for the room's capacity.
+        start_datetime (DateTimeField): Start date and time for room availability or booking.
+        end_datetime (DateTimeField): End date and time for room availability or booking.
+        recurrence_rule (TextField): Recurrence rule for repeated bookings (e.g., weekly).
+        created_at (DateTimeField): Timestamp when the room entry was created.
+        updated_at (DateTimeField): Timestamp when the room entry was last updated.
+    """
+    id = models.AutoField(primary_key=True)  # Unique identifier for the room
+    name = models.TextField()  # The name of the room
+    img_url = models.TextField()  # URL to an image representing the room
+    location_id = models.IntegerField()  # Identifier for the location of the room
+    capacity_id = models.IntegerField()  # Identifier for the room's capacity
+    start_datetime = models.DateTimeField()  # Start date and time for room availability or booking
+    end_datetime = models.DateTimeField()  # End date and time for room availability or booking
+    recurrence_rule = models.TextField()  # Recurrence rule for repeated bookings (e.g., weekly)
+    created_at = models.DateTimeField(auto_now_add=True)  # Timestamp when the room entry was created
+    updated_at = models.DateTimeField(auto_now=True)  # Timestamp when the room entry was last updated

Issue 7 Add room app #52

Are you sure you want to change the base?

Issue 7 Add room app #52

Uh oh!

Conversation

jasonkeo commented Dec 3, 2025 • edited by Fozzyack Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Summary

Change Form

Other Information

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Fozzyack commented Dec 3, 2025

Uh oh!

ErikaKK commented Dec 5, 2025

Uh oh!

ErikaKK left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

jasonkeo commented Dec 3, 2025 •

edited by Fozzyack

Loading