feat: implement item movement request workflow with admin approval #27

tharjiha · 2025-12-21T00:07:56Z

Summary

This PR implements the full workflow for item movement requests, allowing volunteers to request item moves and admins to approve or reject them. Admin decisions are reflected in both the movement request records and item history.

Related Issue

Fixes #12

Changes

Volunteers can create item movement requests
Admins can view all pending movement requests
Admins can approve or reject movement requests
ItemHistory records are automatically generated on approval/rejection

Acceptance Criteria Checklist

Volunteers can request moves when authorized
Admin approvals/rejections update movement request status
ItemHistory entries match final request status

Testing

Manually tested via Django Admin and API endpoints
Verified database updates for:
- ItemMovementRequest
- ItemHistory

…izers, views, and URLs for move request workflow

Copilot

Pull request overview

This PR implements a workflow for item movement requests where volunteers can request item moves and admins can approve or reject them. However, there are critical issues that need to be addressed before merging, particularly around duplicate model creation and security concerns.

Key Changes:

Adds ItemMovementRequestViewSet with approve/reject actions for admin users
Implements URL routing for movement request endpoints
Creates serializers for movement request data handling

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 15 comments.

Show a summary per file

File	Description
backend/requests/models.py	Attempts to add ItemHistory model (duplicates existing inventory model)
backend/requests/serializers.py	Adds ItemMovementRequestSerializer with username fields
backend/requests/views.py	Implements viewset with create, list, approve, and reject actions
backend/requests/urls.py	Registers movement-requests router endpoint
backend/requests/migrations/0003_itemhistory.py	Migration to create ItemHistory (conflicts with existing model)
backend/core/urls.py	Integrates requests app URLs into main routing

Critical Issues Found:

Duplicate Model Creation: The PR attempts to create a new ItemHistory model in the requests app, but this model already exists in the inventory app with different field names (event_type vs item_type). This will cause database conflicts and import errors.
Security Vulnerability: The requested_by field in the serializer is not marked as read-only, allowing potential user impersonation during request creation.
Status Validation: The approve/reject actions don't validate whether a request has already been processed, potentially creating duplicate history entries.

Comments suppressed due to low confidence (1)

backend/requests/views.py:4

Import of 'ItemHistory' is not used.

from .models import ItemMovementRequest, ItemHistory

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

Copilot · 2025-12-22T15:41:31Z

backend/requests/serializers.py

+        validated_data["requested_by"] = self.context["request"].user
+        return super().create(validated_data)


The 'requested_by' field is being set redundantly. It's already set in the viewset's perform_create method (line 73 in views.py), so this assignment here is unnecessary and will be overwritten. Remove this line and let the viewset handle setting the requested_by field.

Suggested change

validated_data["requested_by"] = self.context["request"].user

return super().create(validated_data)

return super().create(validated_data)

Copilot · 2025-12-22T15:41:31Z

backend/requests/views.py

 from rest_framework import viewsets, permissions, status
 from rest_framework.decorators import action
 from rest_framework.response import Response
+from .models import ItemMovementRequest, ItemHistory


ItemHistory is imported but never used in this file. Since ItemHistory operations are handled within the model's approve/reject methods, this import should be removed.

Suggested change

from .models import ItemMovementRequest, ItemHistory

from .models import ItemMovementRequest

Copilot · 2025-12-22T15:41:32Z

backend/requests/views.py

+        # volunteers see only their own requests
+        user = self.request.user
+        if user.is_staff:
+            return ItemMovementRequest.objects.all()  # admins see all


The comment should start with a capital letter: "Admins see all". Following consistent capitalization in comments improves code readability.

Suggested change

# volunteers see only their own requests

user = self.request.user

if user.is_staff:

return ItemMovementRequest.objects.all() # admins see all

# Volunteers see only their own requests

user = self.request.user

if user.is_staff:

return ItemMovementRequest.objects.all() # Admins see all

Copilot · 2025-12-22T15:41:32Z

backend/requests/views.py

+class ItemMovementRequestViewSet(viewsets.ModelViewSet):
+    """
+    viewSet for managing item movement requests.
+    supports creation, listing, retrieval, approval, and rejection.
+    """
+
+    queryset = ItemMovementRequest.objects.all()
+    serializer_class = ItemMovementRequestSerializer
+    permission_classes = [permissions.IsAuthenticated]
+
+    def get_queryset(self):
+        # volunteers see only their own requests
+        user = self.request.user
+        if user.is_staff:
+            return ItemMovementRequest.objects.all()  # admins see all
+        return ItemMovementRequest.objects.filter(requested_by=user)
+
+    def perform_create(self, serializer):
+        serializer.save(requested_by=self.request.user)
+
+    @action(detail=True, methods=["post"], permission_classes=[permissions.IsAdminUser])
+    def approve(self, request, pk=None):
+        move_request = self.get_object()
+        comment = request.data.get("comment", "")
+        move_request.approve(admin_user=request.user, comment=comment)
+        serializer = self.get_serializer(move_request)
+        return Response(serializer.data, status=status.HTTP_200_OK)
+
+    @action(detail=True, methods=["post"], permission_classes=[permissions.IsAdminUser])
+    def reject(self, request, pk=None):
+        move_request = self.get_object()
+        comment = request.data.get("comment", "")
+        move_request.reject(admin_user=request.user, comment=comment)
+        serializer = self.get_serializer(move_request)
+        return Response(serializer.data, status=status.HTTP_200_OK)


The new ItemMovementRequestViewSet and its approve/reject actions lack test coverage. Since the users app has pytest tests, similar test coverage should be added for the movement request workflow, including testing creation, approval, rejection, permission checks, and the resulting ItemHistory entries.

Copilot · 2025-12-22T15:41:32Z

backend/requests/serializers.py

+            "created_at",
+            "updated_at",
+        ]
+        read_only_fields = ["id", "status", "admin", "admin_comment", "created_at", "updated_at"]


The 'requested_by' field should be included in the read_only_fields list since it's automatically set by the viewset's perform_create method. Without this protection, a malicious user could potentially set this field to another user's ID during request creation, impersonating other users.

Suggested change

read_only_fields = ["id", "status", "admin", "admin_comment", "created_at", "updated_at"]

read_only_fields = ["id", "requested_by", "status", "admin", "admin_comment", "created_at", "updated_at"]

Copilot · 2025-12-22T15:41:34Z

backend/requests/views.py

+    @action(detail=True, methods=["post"], permission_classes=[permissions.IsAdminUser])
+    def reject(self, request, pk=None):
+        move_request = self.get_object()
+        comment = request.data.get("comment", "")
+        move_request.reject(admin_user=request.user, comment=comment)
+        serializer = self.get_serializer(move_request)
+        return Response(serializer.data, status=status.HTTP_200_OK)


The reject action doesn't validate the current status of the movement request before processing. A request that has already been approved or rejected could be processed again, potentially creating duplicate ItemHistory entries. Consider adding a check to ensure the request is in "WAITING_APPROVAL" status before proceeding, and return an appropriate error response if not.

Copilot · 2025-12-22T15:41:34Z

backend/requests/models.py

+
+
+class ItemHistory(models.Model):
+    """
+    Records actions performed on items, including moves.
+    """
+
+    ITEM_CHOICES = [
+        ("MOVE_REQUESTED", "Move Requested"),
+        ("MOVE_APPROVED", "Move Approved"),
+        ("MOVE_REJECTED", "Move Rejected"),
+        ("MOVE_CANCELLED", "Move Cancelled"),
+    ]
+
+    item = models.ForeignKey(CollectionItem, on_delete=models.CASCADE)
+    item_type = models.CharField(max_length=50, choices=ITEM_CHOICES)
+    from_location = models.ForeignKey(Location, on_delete=models.SET_NULL, null=True, blank=True, related_name="+")
+    to_location = models.ForeignKey(Location, on_delete=models.SET_NULL, null=True, blank=True, related_name="+")
+    movement_request = models.ForeignKey("ItemMovementRequest", on_delete=models.SET_NULL, null=True, blank=True)
+    acted_by = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.SET_NULL, null=True, blank=True)
+    notes = models.TextField(blank=True)
+    timestamp = models.DateTimeField(auto_now_add=True)
+
+    class Meta:
+        ordering = ["-timestamp"]
+
+    def __str__(self):
+        return f"{self.item_type} for {self.item.item_code} at {self.timestamp}"


The ItemHistory model already exists in inventory/models.py with an 'event_type' field. Creating a duplicate model here with 'item_type' will cause conflicts and database schema issues. The approve() and reject() methods in ItemMovementRequest already correctly import ItemHistory from inventory.models and use 'event_type'. This duplicate model definition should be removed entirely.

Suggested change

class ItemHistory(models.Model):

"""

Records actions performed on items, including moves.

"""

ITEM_CHOICES = [

("MOVE_REQUESTED", "Move Requested"),

("MOVE_APPROVED", "Move Approved"),

("MOVE_REJECTED", "Move Rejected"),

("MOVE_CANCELLED", "Move Cancelled"),

]

item = models.ForeignKey(CollectionItem, on_delete=models.CASCADE)

item_type = models.CharField(max_length=50, choices=ITEM_CHOICES)

from_location = models.ForeignKey(Location, on_delete=models.SET_NULL, null=True, blank=True, related_name="+")

to_location = models.ForeignKey(Location, on_delete=models.SET_NULL, null=True, blank=True, related_name="+")

movement_request = models.ForeignKey("ItemMovementRequest", on_delete=models.SET_NULL, null=True, blank=True)

acted_by = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.SET_NULL, null=True, blank=True)

notes = models.TextField(blank=True)

timestamp = models.DateTimeField(auto_now_add=True)

class Meta:

ordering = ["-timestamp"]

def __str__(self):

return f"{self.item_type} for {self.item.item_code} at {self.timestamp}"

Copilot · 2025-12-22T15:41:35Z

backend/requests/serializers.py

+        read_only_fields = ["id", "status", "admin", "admin_comment", "created_at", "updated_at"]
+
+    def create(self, validated_data):
+        # volunteer = logged-in user


The comment should start with a capital letter and be more descriptive: "Volunteer is the logged-in user". This makes the code's intent clearer.

Suggested change

# volunteer = logged-in user

# Volunteer is the logged-in user.

Copilot · 2025-12-22T15:41:35Z

backend/requests/views.py

+    @action(detail=True, methods=["post"], permission_classes=[permissions.IsAdminUser])
+    def approve(self, request, pk=None):
+        move_request = self.get_object()
+        comment = request.data.get("comment", "")
+        move_request.approve(admin_user=request.user, comment=comment)
+        serializer = self.get_serializer(move_request)
+        return Response(serializer.data, status=status.HTTP_200_OK)


The approve and reject actions don't validate the current status of the movement request before processing. A request that has already been approved or rejected could be processed again, potentially creating duplicate ItemHistory entries. Consider adding a check to ensure the request is in "WAITING_APPROVAL" status before proceeding, and return an appropriate error response if not.

Copilot · 2025-12-22T15:41:35Z

backend/requests/serializers.py

@@ -1,4 +1,5 @@
 from rest_framework import serializers
+from .models import ItemMovementRequest, ItemHistory


Import of 'ItemHistory' is not used.

Suggested change

from .models import ItemMovementRequest, ItemHistory

from .models import ItemMovementRequest

tharjiha added 4 commits November 29, 2025 12:58

feat: add ItemHistory model to support move request tracking

fa87f1e

feat(backend): add ItemMovementRequest and ItemHistory models, serial…

e5ebeca

…izers, views, and URLs for move request workflow

feat: add item movement request workflow with admin approval

180f100

chore: format code with black

383b7ea

vivjd requested a review from Copilot December 22, 2025 15:36

Copilot started reviewing on behalf of vivjd December 22, 2025 15:36 View session

Copilot AI reviewed Dec 22, 2025

View reviewed changes

tharjiha added 3 commits December 22, 2025 10:51

chore: changed formatting and import suggestions

926416d

chore: format code with black

93d032f

fix(requests): prevent double approve/reject

9837bae

tharjiha requested review from qiuethan and vivjd January 2, 2026 17:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: implement item movement request workflow with admin approval #27

feat: implement item movement request workflow with admin approval #27

Uh oh!

tharjiha commented Dec 21, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 22, 2025

Uh oh!

Copilot AI Dec 22, 2025

Uh oh!

Copilot AI Dec 22, 2025

Uh oh!

Copilot AI Dec 22, 2025

Uh oh!

Copilot AI Dec 22, 2025

Uh oh!

Copilot AI Dec 22, 2025

Uh oh!

Copilot AI Dec 22, 2025

Uh oh!

Copilot AI Dec 22, 2025

Uh oh!

Copilot AI Dec 22, 2025

Uh oh!

Copilot AI Dec 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		validated_data["requested_by"] = self.context["request"].user
		return super().create(validated_data)

	from .models import ItemMovementRequest, ItemHistory
	from .models import ItemMovementRequest

	read_only_fields = ["id", "status", "admin", "admin_comment", "created_at", "updated_at"]
	read_only_fields = ["id", "requested_by", "status", "admin", "admin_comment", "created_at", "updated_at"]

	# volunteer = logged-in user
	# Volunteer is the logged-in user.

		@@ -1,4 +1,5 @@
		from rest_framework import serializers
		from .models import ItemMovementRequest, ItemHistory

feat: implement item movement request workflow with admin approval #27

Are you sure you want to change the base?

feat: implement item movement request workflow with admin approval #27

Uh oh!

Conversation

tharjiha commented Dec 21, 2025

Summary

Related Issue

Changes

Acceptance Criteria Checklist

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants