From 1027c246b14841302faaa83bf86e6a9eced79579 Mon Sep 17 00:00:00 2001 From: kevinbu233 Date: Thu, 16 Feb 2023 20:32:11 -0800 Subject: [PATCH 01/21] Made model and starting views --- csm_web/scheduler/models.py | 3 +++ csm_web/scheduler/serializers.py | 6 +++++- csm_web/scheduler/views/swap.py | 10 ++++++++++ 3 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 csm_web/scheduler/views/swap.py diff --git a/csm_web/scheduler/models.py b/csm_web/scheduler/models.py index 828a50fa..02c1dc02 100644 --- a/csm_web/scheduler/models.py +++ b/csm_web/scheduler/models.py @@ -33,6 +33,9 @@ def week_bounds(date): week_end = week_start + datetime.timedelta(weeks=1) return week_start, week_end +class Swap(models.Model): + sender = models.ForeignKey(Student, on_delete=models.CASCADE) + receiver = models.ForeignKey(Student, on_delete=models.CASCADE) class User(AbstractUser): priority_enrollment = models.DateTimeField(null=True, blank=True) diff --git a/csm_web/scheduler/serializers.py b/csm_web/scheduler/serializers.py index bcb1636e..33f72aa0 100644 --- a/csm_web/scheduler/serializers.py +++ b/csm_web/scheduler/serializers.py @@ -1,7 +1,7 @@ from rest_framework import serializers from enum import Enum from django.utils import timezone -from .models import Attendance, Course, Link, SectionOccurrence, User, Student, Section, Mentor, Override, Spacetime, Coordinator, DayOfWeekField, Resource, Worksheet, Matcher, MatcherSlot, MatcherPreference +from .models import Attendance, Swap, Course, Link, SectionOccurrence, User, Student, Section, Mentor, Override, Spacetime, Coordinator, DayOfWeekField, Resource, Worksheet, Matcher, MatcherSlot, MatcherPreference class Role(Enum): @@ -58,6 +58,10 @@ class Meta: fields = ("spacetime", "date") read_only_fields = ("spacetime", "date") +class SwapSerializer(serializers.ModelSerializer): + class Meta: + model = Swap + fields = ("sender", "receiver") class SpacetimeSerializer(serializers.ModelSerializer): diff --git a/csm_web/scheduler/views/swap.py b/csm_web/scheduler/views/swap.py new file mode 100644 index 00000000..55999c0d --- /dev/null +++ b/csm_web/scheduler/views/swap.py @@ -0,0 +1,10 @@ +from rest_framework.exceptions import PermissionDenied +from rest_framework.response import Response +from rest_framework import status +from rest_framework.decorators import api_view + +from .utils import viewset_with +from ..models import Swap, Student +from scheduler.serializers import UserSerializer + + From 1442c47eca97dd2963ba0e8354ddd1aacfa7cd3c Mon Sep 17 00:00:00 2001 From: Kartavya Sharma Date: Mon, 27 Feb 2023 18:17:49 -0800 Subject: [PATCH 02/21] Created views for swap --- csm_web/scheduler/views/section.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/csm_web/scheduler/views/section.py b/csm_web/scheduler/views/section.py index c79ccde0..2eab4532 100644 --- a/csm_web/scheduler/views/section.py +++ b/csm_web/scheduler/views/section.py @@ -17,12 +17,14 @@ Spacetime, Student, User, + Swap, ) from scheduler.serializers import ( SectionOccurrenceSerializer, SectionSerializer, SpacetimeSerializer, StudentSerializer, + SwapSerializer, ) from .utils import get_object_or_error, log_str, logger, viewset_with @@ -779,3 +781,25 @@ def wotd(self, request, pk=None): return Response({}, status=status.HTTP_200_OK) raise PermissionDenied() + + @action(detail=True, methods=["GET", "POST"]) + def swap(self, request, pk=None): + """ + GET: Returns all relevant Swap objects for current user. + + POST: Handles creating a new Swap instance when a Swap a requested. + """ + section = get_object_or_error(Section.objects, pk=pk) + + if request.method == "GET": + student_id = request.data["student_id"] + if student_id == "": + raise PermissionDenied("The `student_id` field in the request cannot be empty, please specify student.") + student_swaps = Swap.objects.filter(sender__id=student_id) + return Response( + SwapSerializer( + student_swaps + ).data + ) + if request.method == "POST": + pass From ad5f33eb35c6f5e697fd0d833ba71b4f595e58bb Mon Sep 17 00:00:00 2001 From: Kartavya Sharma Date: Mon, 27 Feb 2023 18:20:53 -0800 Subject: [PATCH 03/21] Refactored Swap models --- csm_web/scheduler/models.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/csm_web/scheduler/models.py b/csm_web/scheduler/models.py index 02c1dc02..cb1e5657 100644 --- a/csm_web/scheduler/models.py +++ b/csm_web/scheduler/models.py @@ -33,9 +33,6 @@ def week_bounds(date): week_end = week_start + datetime.timedelta(weeks=1) return week_start, week_end -class Swap(models.Model): - sender = models.ForeignKey(Student, on_delete=models.CASCADE) - receiver = models.ForeignKey(Student, on_delete=models.CASCADE) class User(AbstractUser): priority_enrollment = models.DateTimeField(null=True, blank=True) @@ -117,7 +114,7 @@ def week_start(self): class Meta: unique_together = ("sectionOccurrence", "student") ordering = ("sectionOccurrence",) - #indexes = (models.Index(fields=("date",)),) + # indexes = (models.Index(fields=("date",)),) class SectionOccurrence(ValidatingModel): @@ -243,9 +240,18 @@ class Meta: unique_together = ("user", "section") +class Swap(models.Model): + """ + Represents a given "instance" of a swap. Every time a swap is requested between two users, + a Swap instance is initialized containing references to the respective students. + """ + sender = models.ForeignKey(Student, related_name="sender", on_delete=models.CASCADE) + receiver = models.ForeignKey(Student, related_name="receiver", on_delete=models.CASCADE) + + class Mentor(Profile): """ - Represents a given "instance" of a mentor. Every section a mentor teaches in every course should + Represents a given "instance" of a mentor. Every section a mentor teaches in every ccurse should have a new Mentor profile. """ From 9867af1026c588a2c41b20e18aa95d10a2fb2bd5 Mon Sep 17 00:00:00 2001 From: Kartavya Sharma Date: Mon, 10 Apr 2023 18:51:13 -0700 Subject: [PATCH 04/21] Added basic structure for GET and PUT for Swap requests, added test file test_swap.py to test directory. --- csm_web/scheduler/migrations/0033_swap.py | 24 ++++++++ csm_web/scheduler/tests/models/test_swap.py | 0 csm_web/scheduler/views/section.py | 62 ++++++++++++++++----- csm_web/scheduler/views/swap.py | 10 ---- 4 files changed, 72 insertions(+), 24 deletions(-) create mode 100644 csm_web/scheduler/migrations/0033_swap.py create mode 100644 csm_web/scheduler/tests/models/test_swap.py delete mode 100644 csm_web/scheduler/views/swap.py diff --git a/csm_web/scheduler/migrations/0033_swap.py b/csm_web/scheduler/migrations/0033_swap.py new file mode 100644 index 00000000..70996237 --- /dev/null +++ b/csm_web/scheduler/migrations/0033_swap.py @@ -0,0 +1,24 @@ +# Generated by Django 3.2.16 on 2023-04-11 01:40 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('scheduler', '0032_word_of_the_day'), + ] + + operations = [ + migrations.CreateModel( + name='Swap', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('receiver', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, + related_name='receiver', to='scheduler.student')), + ('sender', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, + related_name='sender', to='scheduler.student')), + ], + ), + ] diff --git a/csm_web/scheduler/tests/models/test_swap.py b/csm_web/scheduler/tests/models/test_swap.py new file mode 100644 index 00000000..e69de29b diff --git a/csm_web/scheduler/views/section.py b/csm_web/scheduler/views/section.py index 2eab4532..31d71c55 100644 --- a/csm_web/scheduler/views/section.py +++ b/csm_web/scheduler/views/section.py @@ -6,7 +6,7 @@ from django.utils import timezone from rest_framework import status from rest_framework.decorators import action -from rest_framework.exceptions import PermissionDenied +from rest_framework.exceptions import PermissionDenied, NotFound from rest_framework.response import Response from scheduler.models import ( Attendance, @@ -782,24 +782,58 @@ def wotd(self, request, pk=None): raise PermissionDenied() - @action(detail=True, methods=["GET", "POST"]) + @action(detail=True, methods=["GET", "PUT", "DELETE"]) def swap(self, request, pk=None): """ GET: Returns all relevant Swap objects for current user. + Request format: + { student_id: int } + Response format: + { + receiver: [`List of Swap objects`], + sender: [`List of Swap objects`] + } + Where each Swap object has the following format: + { + swap_id: int, + name: string, + mentor_name: string, + time: DateTime, + } POST: Handles creating a new Swap instance when a Swap a requested. + Request format: + { email: string } + `email` is the email of the student who will be receiving the swap request. + Response format: + {} + With status code of 201 if successful, 400 if not. """ - section = get_object_or_error(Section.objects, pk=pk) + student_id = 1 # Change to request.data["student_id"] + if student_id == "": + raise PermissionDenied("The `student_id` field in the request cannot be empty, please specify student.") + student = get_object_or_error(Student.objects, id=student_id) if request.method == "GET": - student_id = request.data["student_id"] - if student_id == "": - raise PermissionDenied("The `student_id` field in the request cannot be empty, please specify student.") - student_swaps = Swap.objects.filter(sender__id=student_id) - return Response( - SwapSerializer( - student_swaps - ).data - ) - if request.method == "POST": - pass + try: + outgoing_swaps = get_object_or_error(Swap.objects, sender=student) + incoming_swaps = get_object_or_error(Swap.objects, receiver=student) + except Exception as e: + raise NotFound("No swaps found for student with id: " + str(student_id)) + + student_swaps = { + "sender": SwapSerializer(outgoing_swaps).data, + "receiver": SwapSerializer(incoming_swaps).data, + } + + return Response(student_swaps, status=status.HTTP_200_OK) + + if request.method == "PUT": + receiver_email = request.data["email"] + receiver = get_object_or_error(Student.objects, email=receiver_email) + try: + swap = Swap.objects.create(sender=student, receiver=receiver) + except Exception as e: + raise NotFound("Could not create swap for student with id: " + str(student_id)) + + return Response(SwapSerializer(swap).data, status=status.HTTP_201_CREATED) diff --git a/csm_web/scheduler/views/swap.py b/csm_web/scheduler/views/swap.py deleted file mode 100644 index 55999c0d..00000000 --- a/csm_web/scheduler/views/swap.py +++ /dev/null @@ -1,10 +0,0 @@ -from rest_framework.exceptions import PermissionDenied -from rest_framework.response import Response -from rest_framework import status -from rest_framework.decorators import api_view - -from .utils import viewset_with -from ..models import Swap, Student -from scheduler.serializers import UserSerializer - - From ade355dc43a18ae3e02202934940473ba2e940d5 Mon Sep 17 00:00:00 2001 From: Kartavya Sharma Date: Tue, 11 Apr 2023 10:51:19 -0700 Subject: [PATCH 05/21] refactor: fixed typo in Mentor model --- csm_web/scheduler/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/csm_web/scheduler/models.py b/csm_web/scheduler/models.py index cb1e5657..37a5111d 100644 --- a/csm_web/scheduler/models.py +++ b/csm_web/scheduler/models.py @@ -251,7 +251,7 @@ class Swap(models.Model): class Mentor(Profile): """ - Represents a given "instance" of a mentor. Every section a mentor teaches in every ccurse should + Represents a given "instance" of a mentor. Every section a mentor teaches in every course should have a new Mentor profile. """ From 624b34f343127ea0b618fe749a32fb073b1e60fb Mon Sep 17 00:00:00 2001 From: Kartavya Sharma Date: Wed, 12 Apr 2023 11:36:33 -0700 Subject: [PATCH 06/21] Added Logging for when a Swap is successfully created. Added boilerplate code for testing swaps and tests covering basic swap functionality --- csm_web/scheduler/tests/models/test_swap.py | 68 +++++++++++++++++++++ csm_web/scheduler/views/section.py | 11 +++- 2 files changed, 77 insertions(+), 2 deletions(-) diff --git a/csm_web/scheduler/tests/models/test_swap.py b/csm_web/scheduler/tests/models/test_swap.py index e69de29b..1fb6efeb 100644 --- a/csm_web/scheduler/tests/models/test_swap.py +++ b/csm_web/scheduler/tests/models/test_swap.py @@ -0,0 +1,68 @@ +import pytest + +from django.core.exceptions import ValidationError +from scheduler.models import Student, User +from scheduler.factories import UserFactory, CourseFactory, SectionFactory, StudentFactory, MentorFactory + + +@pytest.fixture +def setup_section(db): + """ + Creates a pair of sections within the same course, each with a mentor and student. + """ + # Setup course + course = CourseFactory.create() + # Setup test section one + mentor_user_one, student_user_one = UserFactory.create_batch(2) + mentor_one = MentorFactory.create(course=course, user=mentor_user_one) + section_one = SectionFactory.create(mentor=mentor_one) + student_one = StudentFactory.create(user=student_user_one, course=course, section=section_one) + # Setup test section two + mentor_user_two, student_user_two = UserFactory.create_batch(2) + mentor_two = MentorFactory.create(course=course, user=mentor_user_two) + section_two = SectionFactory.create(mentor=mentor_two) + student_two = StudentFactory.create(user=student_user_two, course=course, section=section_two) + + return course, section_one, section_two, student_one, student_two + + +@pytest.mark.django_db +def test_basic_swap_request_success(client, setup_section): + """ + Tests that a student can successfully request a swap. + """ + # TODO: Add test for when a student successfully requests a swap from another student in a different section + + +@pytest.mark.django_db +@pytest.mark.parametrize( + ["email"], + [ + # empty email + (" ",), + # invalid email + ("invalid",), + # non-existent email + ("abcd@example.io",), + # valid email + (setup_section,) + ], + ids=["empty email", "invalid email", "non-existent email", "valid email"], +) +def test_request_swap_invalid_email(client, setup_section, email, request): + """ + Tests that a student cannot request a swap with an invalid email. + """ + course, section_one, section_two, student_one, student_two = setup_section + receiver_email = email + if (type(receiver_email) != str): + receiver_email = request.getfixturevalue(email.__name__)[4].user.email + # TODO Add test for when a student requests a swap with a invalid email + + +@pytest.mark.django_db +def test_swap_with_stale_receiver(client, setup_section): + """ + Tests weather Swaps are being invalidated/deleted when receiver has swapped with another student. + """ + # TODO Add test for when a student requests a swap with a stale receiver diff --git a/csm_web/scheduler/views/section.py b/csm_web/scheduler/views/section.py index 31d71c55..2dd4aa52 100644 --- a/csm_web/scheduler/views/section.py +++ b/csm_web/scheduler/views/section.py @@ -830,10 +830,17 @@ def swap(self, request, pk=None): if request.method == "PUT": receiver_email = request.data["email"] - receiver = get_object_or_error(Student.objects, email=receiver_email) + receiver = get_object_or_error(Student.objects, user__email=receiver_email) try: swap = Swap.objects.create(sender=student, receiver=receiver) except Exception as e: raise NotFound("Could not create swap for student with id: " + str(student_id)) - return Response(SwapSerializer(swap).data, status=status.HTTP_201_CREATED) + # Successful Swap + logger.info( + " User %s requested a swap with %s", + log_str(student.user), + log_str(receiver), + ) + + return Response({}, status=status.HTTP_201_CREATED) From 6e042acd4d18f4703630bf55a72063148b5a6ccc Mon Sep 17 00:00:00 2001 From: Kartavya Sharma Date: Mon, 17 Apr 2023 19:58:07 -0700 Subject: [PATCH 07/21] WIP: Working on atomic transactions after student accepts swap --- csm_web/scheduler/views/section.py | 47 ++++++++++++++++++------------ 1 file changed, 29 insertions(+), 18 deletions(-) diff --git a/csm_web/scheduler/views/section.py b/csm_web/scheduler/views/section.py index 2dd4aa52..ed9a4326 100644 --- a/csm_web/scheduler/views/section.py +++ b/csm_web/scheduler/views/section.py @@ -782,7 +782,7 @@ def wotd(self, request, pk=None): raise PermissionDenied() - @action(detail=True, methods=["GET", "PUT", "DELETE"]) + @action(detail=True, methods=["GET", "POST", "DELETE"]) def swap(self, request, pk=None): """ GET: Returns all relevant Swap objects for current user. @@ -790,7 +790,7 @@ def swap(self, request, pk=None): { student_id: int } Response format: { - receiver: [`List of Swap objects`], + receiver: [`List of Swap objects`], sender: [`List of Swap objects`] } Where each Swap object has the following format: @@ -811,7 +811,8 @@ def swap(self, request, pk=None): """ student_id = 1 # Change to request.data["student_id"] if student_id == "": - raise PermissionDenied("The `student_id` field in the request cannot be empty, please specify student.") + raise PermissionDenied("The `student_id` field in the request cannot be empty," + "please specify student.") student = get_object_or_error(Student.objects, id=student_id) if request.method == "GET": @@ -828,19 +829,29 @@ def swap(self, request, pk=None): return Response(student_swaps, status=status.HTTP_200_OK) - if request.method == "PUT": - receiver_email = request.data["email"] - receiver = get_object_or_error(Student.objects, user__email=receiver_email) - try: - swap = Swap.objects.create(sender=student, receiver=receiver) - except Exception as e: - raise NotFound("Could not create swap for student with id: " + str(student_id)) - - # Successful Swap - logger.info( - " User %s requested a swap with %s", - log_str(student.user), - log_str(receiver), - ) + if request.method == "POST": + if (pk is None): + '''If pk is None, create a new Swap object between the the current student + and the student with the email specified in the request. This is essentially + creating a swap invite.''' + receiver_email = request.data["email"] + receiver = get_object_or_error(Student.objects, user__email=receiver_email) + try: + # Create Swap request + swap = Swap.objects.create(sender=student, receiver=receiver) + except Exception as e: + raise NotFound("Could not create swap for student with id: " + str(student_id)) + + # Successfuly created swap + logger.info( + " User %s requested a swap with %s", + log_str(student.user), + log_str(receiver), + ) - return Response({}, status=status.HTTP_201_CREATED) + return Response({}, status=status.HTTP_201_CREATED) + else: + '''If pk is not None, initiate the swap between the two students. This should + be done in one atomic transaction. If either or both of the swaps are deleted, + then return a 404 error.''' + pass From 7550961f23975e8852fe3382d3b2bc078a0189c7 Mon Sep 17 00:00:00 2001 From: Kartavya Sharma Date: Mon, 17 Apr 2023 20:12:51 -0700 Subject: [PATCH 08/21] Fixed getting student id from request based on method --- csm_web/scheduler/views/section.py | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/csm_web/scheduler/views/section.py b/csm_web/scheduler/views/section.py index ed9a4326..c2afce78 100644 --- a/csm_web/scheduler/views/section.py +++ b/csm_web/scheduler/views/section.py @@ -809,13 +809,12 @@ def swap(self, request, pk=None): {} With status code of 201 if successful, 400 if not. """ - student_id = 1 # Change to request.data["student_id"] - if student_id == "": + student_id = 1 # Change to request.data["student_id"] ? request.method != "POST" and pk is not None : -1 + if student_id == -1: raise PermissionDenied("The `student_id` field in the request cannot be empty," "please specify student.") - student = get_object_or_error(Student.objects, id=student_id) - if request.method == "GET": + student = get_object_or_error(Student.objects, id=student_id) try: outgoing_swaps = get_object_or_error(Swap.objects, sender=student) incoming_swaps = get_object_or_error(Swap.objects, receiver=student) @@ -846,12 +845,21 @@ def swap(self, request, pk=None): logger.info( " User %s requested a swap with %s", log_str(student.user), - log_str(receiver), + log_str(receiver.user), ) return Response({}, status=status.HTTP_201_CREATED) else: '''If pk is not None, initiate the swap between the two students. This should - be done in one atomic transaction. If either or both of the swaps are deleted, - then return a 404 error.''' - pass + be done in one atomic transaction. If either or both of the swaps are already + deleted, then return a 404 error.''' + # Check if swap object with given id exists + try: + target_swap = get_object_or_error(Swap.objects, id=pk) + except Exception as e: + raise PermissionDenied("Swap with id: " + str(pk) + " does not exist.") + # Execute atomic transaction, and swap sections + sender = target_swap.sender + receiver = target_swap.receiver + with transaction.atomic(): + pass From 7972d99e7d21cf4941734502badfd1d5e8ae748b Mon Sep 17 00:00:00 2001 From: Kartavya Sharma Date: Mon, 17 Apr 2023 21:00:30 -0700 Subject: [PATCH 09/21] Added process for swapping sections after swap invite has been accepted --- csm_web/scheduler/views/section.py | 38 +++++++++++++++++++++++------- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/csm_web/scheduler/views/section.py b/csm_web/scheduler/views/section.py index c2afce78..50afeb44 100644 --- a/csm_web/scheduler/views/section.py +++ b/csm_web/scheduler/views/section.py @@ -809,15 +809,19 @@ def swap(self, request, pk=None): {} With status code of 201 if successful, 400 if not. """ - student_id = 1 # Change to request.data["student_id"] ? request.method != "POST" and pk is not None : -1 + section = get_object_or_error(Section.objects, pk=pk) + is_mentor = section.mentor.user == request.user + student_id = request.data.get("student_id", -1) if student_id == -1: raise PermissionDenied("The `student_id` field in the request cannot be empty," "please specify student.") if request.method == "GET": + if is_mentor: + raise PermissionDenied("Cannot `GET` swaps for a mentor.") student = get_object_or_error(Student.objects, id=student_id) try: - outgoing_swaps = get_object_or_error(Swap.objects, sender=student) - incoming_swaps = get_object_or_error(Swap.objects, receiver=student) + outgoing_swaps = Swap.objects.filter(sender=student) + incoming_swaps = Swap.objects.filter(receiver=student) except Exception as e: raise NotFound("No swaps found for student with id: " + str(student_id)) @@ -825,16 +829,19 @@ def swap(self, request, pk=None): "sender": SwapSerializer(outgoing_swaps).data, "receiver": SwapSerializer(incoming_swaps).data, } - return Response(student_swaps, status=status.HTTP_200_OK) - if request.method == "POST": + if is_mentor: + raise PermissionDenied("Cannot `POST` swaps for a mentor.") if (pk is None): '''If pk is None, create a new Swap object between the the current student and the student with the email specified in the request. This is essentially creating a swap invite.''' receiver_email = request.data["email"] receiver = get_object_or_error(Student.objects, user__email=receiver_email) + receiver_is_sender = receiver == student + if receiver_is_sender: + raise PermissionDenied("Cannot send a swap request to yourself.") try: # Create Swap request swap = Swap.objects.create(sender=student, receiver=receiver) @@ -847,7 +854,6 @@ def swap(self, request, pk=None): log_str(student.user), log_str(receiver.user), ) - return Response({}, status=status.HTTP_201_CREATED) else: '''If pk is not None, initiate the swap between the two students. This should @@ -859,7 +865,21 @@ def swap(self, request, pk=None): except Exception as e: raise PermissionDenied("Swap with id: " + str(pk) + " does not exist.") # Execute atomic transaction, and swap sections - sender = target_swap.sender - receiver = target_swap.receiver + try: + sender = get_object_or_error(Student.objects, id=target_swap.sender.id) + receiver = get_object_or_error(Student.objects, id=target_swap.receiver.id) + except Exception as e: + raise NotFound("Could not find swaps for student.") with transaction.atomic(): - pass + sender.section = target_swap.receiver.section + sender.save() + receiver.section = target_swap.sender.section + receiver.save() + # Delete all other swaps between the two students. Delete this swap object too. + target_swap.delete() + # Get outgoing and incoming swaps for sender and receiver + outgoing_swaps = Swap.objects.filter(sender=sender).values() + incoming_swaps = Swap.objects.filter(receiver=sender).values() + for expired_swap in outgoing_swaps + incoming_swaps: + expired_swap.delete() + return Response({}, status=status.HTTP_200_OK) From fdd5d32b7cb0390c71d6d56cd3ebc022b3d4715a Mon Sep 17 00:00:00 2001 From: Kartavya Sharma Date: Tue, 18 Apr 2023 09:18:49 -0700 Subject: [PATCH 10/21] Added concurrency checks for swap sequence, and added email validation checks --- csm_web/scheduler/views/section.py | 46 +++++++++++++++++------------- 1 file changed, 26 insertions(+), 20 deletions(-) diff --git a/csm_web/scheduler/views/section.py b/csm_web/scheduler/views/section.py index 50afeb44..eaf2b707 100644 --- a/csm_web/scheduler/views/section.py +++ b/csm_web/scheduler/views/section.py @@ -837,8 +837,14 @@ def swap(self, request, pk=None): '''If pk is None, create a new Swap object between the the current student and the student with the email specified in the request. This is essentially creating a swap invite.''' - receiver_email = request.data["email"] - receiver = get_object_or_error(Student.objects, user__email=receiver_email) + receiver_email = request.data("email", None) + if receiver_email is None: + raise PermissionDenied("The `email` field in the request cannot be empty," + "please specify a receiver.") + try: + receiver = get_object_or_error(Student.objects, user__email=receiver_email) + except Exception as e: + raise NotFound("Invalid email provided.") receiver_is_sender = receiver == student if receiver_is_sender: raise PermissionDenied("Cannot send a swap request to yourself.") @@ -859,27 +865,27 @@ def swap(self, request, pk=None): '''If pk is not None, initiate the swap between the two students. This should be done in one atomic transaction. If either or both of the swaps are already deleted, then return a 404 error.''' - # Check if swap object with given id exists - try: - target_swap = get_object_or_error(Swap.objects, id=pk) - except Exception as e: - raise PermissionDenied("Swap with id: " + str(pk) + " does not exist.") - # Execute atomic transaction, and swap sections - try: - sender = get_object_or_error(Student.objects, id=target_swap.sender.id) - receiver = get_object_or_error(Student.objects, id=target_swap.receiver.id) - except Exception as e: - raise NotFound("Could not find swaps for student.") with transaction.atomic(): + # Check if swap object with given id exists + try: + target_swap = get_object_or_error(Swap.objects, pk=pk).select_for_update() + except Exception as e: + raise PermissionDenied("Swap with id: " + str(pk) + " does not exist.") + # Execute atomic transaction, and swap sections + try: + sender = get_object_or_error(Student.objects, id=target_swap.sender.id) + receiver = get_object_or_error(Student.objects, id=target_swap.receiver.id) + except Exception as e: + raise NotFound("Could not find swaps for student.") sender.section = target_swap.receiver.section sender.save() receiver.section = target_swap.sender.section receiver.save() - # Delete all other swaps between the two students. Delete this swap object too. - target_swap.delete() - # Get outgoing and incoming swaps for sender and receiver - outgoing_swaps = Swap.objects.filter(sender=sender).values() - incoming_swaps = Swap.objects.filter(receiver=sender).values() - for expired_swap in outgoing_swaps + incoming_swaps: - expired_swap.delete() + # Delete all other swaps between the two students. Delete this swap object too. + target_swap.delete() + # Get outgoing and incoming swaps for sender and receiver + outgoing_swaps = Swap.objects.filter(sender=sender).select_for_update().values() + incoming_swaps = Swap.objects.filter(receiver=sender).select_for_update().values() + for expired_swap in outgoing_swaps + incoming_swaps: + expired_swap.delete() return Response({}, status=status.HTTP_200_OK) From e4a650377b8e6d55f90a813a4fe7548b6f806a8a Mon Sep 17 00:00:00 2001 From: Edward Lee Date: Mon, 24 Apr 2023 18:25:13 -0700 Subject: [PATCH 11/21] First attempt at delete swap method --- csm_web/scheduler/views/section.py | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/csm_web/scheduler/views/section.py b/csm_web/scheduler/views/section.py index eaf2b707..595e12fc 100644 --- a/csm_web/scheduler/views/section.py +++ b/csm_web/scheduler/views/section.py @@ -32,6 +32,8 @@ class SectionViewSet(*viewset_with("retrieve", "partial_update", "create")): serializer_class = SectionSerializer + lookup_field = 'pk' + lookup_url_kwarg = 'section_id' def get_object(self): """Retrieve section object""" @@ -833,8 +835,9 @@ def swap(self, request, pk=None): if request.method == "POST": if is_mentor: raise PermissionDenied("Cannot `POST` swaps for a mentor.") - if (pk is None): - '''If pk is None, create a new Swap object between the the current student + swap_id = self.kwargs.get('swap_id', None) + if (swap_id is None): + '''If swap_id is None, create a new Swap object between the the current student and the student with the email specified in the request. This is essentially creating a swap invite.''' receiver_email = request.data("email", None) @@ -862,15 +865,15 @@ def swap(self, request, pk=None): ) return Response({}, status=status.HTTP_201_CREATED) else: - '''If pk is not None, initiate the swap between the two students. This should + '''If swap_id is not None, initiate the swap between the two students. This should be done in one atomic transaction. If either or both of the swaps are already deleted, then return a 404 error.''' with transaction.atomic(): # Check if swap object with given id exists try: - target_swap = get_object_or_error(Swap.objects, pk=pk).select_for_update() + target_swap = get_object_or_error(Swap.objects, pk=swap_id).select_for_update() except Exception as e: - raise PermissionDenied("Swap with id: " + str(pk) + " does not exist.") + raise PermissionDenied("Swap with id: " + str(swap_id) + " does not exist.") # Execute atomic transaction, and swap sections try: sender = get_object_or_error(Student.objects, id=target_swap.sender.id) @@ -889,3 +892,20 @@ def swap(self, request, pk=None): for expired_swap in outgoing_swaps + incoming_swaps: expired_swap.delete() return Response({}, status=status.HTTP_200_OK) + + if request.method == "DELETE": + swap_id = self.kwargs.get('swap_id', None) + swap = get_object_or_error(Swap, pk=swap_id) + if student_id != swap.reciever.id and student_id != swap.sender.id: + logger.error( + f" Could not delete swap, user {log_str(request.user)} does not have proper permissions" + ) + raise PermissionDenied("You must be a student or a coordinator to delete this spacetime override!") + + swap.delete() + swap.save() + + logger.info( + f" Deleted swap {log_str(swap)}" + ) + return Response(status=status.HTTP_200_OK) From 3ae21497bfc87bcfec4795fbc2d9c9d63914c559 Mon Sep 17 00:00:00 2001 From: Kartavya Sharma Date: Tue, 25 Apr 2023 11:19:14 -0700 Subject: [PATCH 12/21] Updated docstring for swaps --- csm_web/scheduler/views/section.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/csm_web/scheduler/views/section.py b/csm_web/scheduler/views/section.py index 595e12fc..b971fbe3 100644 --- a/csm_web/scheduler/views/section.py +++ b/csm_web/scheduler/views/section.py @@ -803,7 +803,8 @@ def swap(self, request, pk=None): time: DateTime, } - POST: Handles creating a new Swap instance when a Swap a requested. + POST: Handles creating a new Swap instance when a Swap a requested or + processes an existing Swap request. Request format: { email: string } `email` is the email of the student who will be receiving the swap request. @@ -891,6 +892,11 @@ def swap(self, request, pk=None): incoming_swaps = Swap.objects.filter(receiver=sender).select_for_update().values() for expired_swap in outgoing_swaps + incoming_swaps: expired_swap.delete() + logger.info( + " User %s swapped with %s", + log_str(sender.user), + log_str(receiver.user), + ) return Response({}, status=status.HTTP_200_OK) if request.method == "DELETE": From ec9c6af5f6fc8245cee7dd9c8b5324590c8b2c4b Mon Sep 17 00:00:00 2001 From: Kartavya Sharma Date: Mon, 15 May 2023 14:42:37 -0700 Subject: [PATCH 13/21] Added tests for requesting basic section swap between two students --- csm_web/scheduler/tests/models/test_swap.py | 14 +++++++++-- csm_web/scheduler/views/section.py | 27 +++++++++++++++------ 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/csm_web/scheduler/tests/models/test_swap.py b/csm_web/scheduler/tests/models/test_swap.py index 1fb6efeb..b4f461c2 100644 --- a/csm_web/scheduler/tests/models/test_swap.py +++ b/csm_web/scheduler/tests/models/test_swap.py @@ -1,10 +1,11 @@ import pytest +import json +from django.urls import reverse from django.core.exceptions import ValidationError -from scheduler.models import Student, User +from scheduler.models import Student, User, Swap from scheduler.factories import UserFactory, CourseFactory, SectionFactory, StudentFactory, MentorFactory - @pytest.fixture def setup_section(db): """ @@ -32,6 +33,14 @@ def test_basic_swap_request_success(client, setup_section): Tests that a student can successfully request a swap. """ # TODO: Add test for when a student successfully requests a swap from another student in a different section + section_one, section_two, student_one, student_two = setup_section[1:] + client.force_login(student_one.user) + post_url = reverse("section-swap", args=[section_one.id]) + data = json.dumps({"receiver_email": student_two.user.email, "student_id": student_one.id}) + response = client.post(post_url, data=data, content_type="application/json") + + # Check that the swap request was created + assert Swap.objects.filter(receiver=student_two).exists() @pytest.mark.django_db @@ -58,6 +67,7 @@ def test_request_swap_invalid_email(client, setup_section, email, request): if (type(receiver_email) != str): receiver_email = request.getfixturevalue(email.__name__)[4].user.email # TODO Add test for when a student requests a swap with a invalid email + @pytest.mark.django_db diff --git a/csm_web/scheduler/views/section.py b/csm_web/scheduler/views/section.py index b971fbe3..93400cf3 100644 --- a/csm_web/scheduler/views/section.py +++ b/csm_web/scheduler/views/section.py @@ -5,7 +5,8 @@ from django.db.models import Prefetch, Q from django.utils import timezone from rest_framework import status -from rest_framework.decorators import action +from rest_framework import permissions +from rest_framework.decorators import action, permission_classes from rest_framework.exceptions import PermissionDenied, NotFound from rest_framework.response import Response from scheduler.models import ( @@ -32,8 +33,8 @@ class SectionViewSet(*viewset_with("retrieve", "partial_update", "create")): serializer_class = SectionSerializer - lookup_field = 'pk' - lookup_url_kwarg = 'section_id' + # lookup_field = 'pk' + # lookup_url_kwarg = 'section_id' def get_object(self): """Retrieve section object""" @@ -815,6 +816,7 @@ def swap(self, request, pk=None): section = get_object_or_error(Section.objects, pk=pk) is_mentor = section.mentor.user == request.user student_id = request.data.get("student_id", -1) + student = Student.objects.get(id=student_id) if student_id == -1: raise PermissionDenied("The `student_id` field in the request cannot be empty," "please specify student.") @@ -835,15 +837,16 @@ def swap(self, request, pk=None): return Response(student_swaps, status=status.HTTP_200_OK) if request.method == "POST": if is_mentor: + logger.error("Cannot `POST` swaps for a mentor.") raise PermissionDenied("Cannot `POST` swaps for a mentor.") swap_id = self.kwargs.get('swap_id', None) if (swap_id is None): '''If swap_id is None, create a new Swap object between the the current student and the student with the email specified in the request. This is essentially creating a swap invite.''' - receiver_email = request.data("email", None) + receiver_email = request.data.get("receiver_email", None) if receiver_email is None: - raise PermissionDenied("The `email` field in the request cannot be empty," + raise PermissionDenied("The `receiver_email` field in the request cannot be empty," "please specify a receiver.") try: receiver = get_object_or_error(Student.objects, user__email=receiver_email) @@ -860,7 +863,7 @@ def swap(self, request, pk=None): # Successfuly created swap logger.info( - " User %s requested a swap with %s", + " User %s requested a swap with %s", log_str(student.user), log_str(receiver.user), ) @@ -874,12 +877,22 @@ def swap(self, request, pk=None): try: target_swap = get_object_or_error(Swap.objects, pk=swap_id).select_for_update() except Exception as e: + logger.error( + " User %s could not swap with %s", + log_str(student.user), + log_str(receiver.user), + ) raise PermissionDenied("Swap with id: " + str(swap_id) + " does not exist.") # Execute atomic transaction, and swap sections try: sender = get_object_or_error(Student.objects, id=target_swap.sender.id) receiver = get_object_or_error(Student.objects, id=target_swap.receiver.id) except Exception as e: + logger.error( + " User %s could not swap with %s", + log_str(student.user), + log_str(receiver.user), + ) raise NotFound("Could not find swaps for student.") sender.section = target_swap.receiver.section sender.save() @@ -893,7 +906,7 @@ def swap(self, request, pk=None): for expired_swap in outgoing_swaps + incoming_swaps: expired_swap.delete() logger.info( - " User %s swapped with %s", + " User %s swapped with %s", log_str(sender.user), log_str(receiver.user), ) From 7ddd3b8bde8feae3ed15fc5e91421dad143ac68c Mon Sep 17 00:00:00 2001 From: Kartavya Sharma Date: Tue, 16 May 2023 00:27:42 -0700 Subject: [PATCH 14/21] Swap list GET endpoint now works, added test utils for Swapping, and added tests for getting user swap requests --- csm_web/scheduler/tests/models/test_swap.py | 140 +++++++++++++++----- csm_web/scheduler/views/section.py | 15 ++- 2 files changed, 119 insertions(+), 36 deletions(-) diff --git a/csm_web/scheduler/tests/models/test_swap.py b/csm_web/scheduler/tests/models/test_swap.py index b4f461c2..f5412ec6 100644 --- a/csm_web/scheduler/tests/models/test_swap.py +++ b/csm_web/scheduler/tests/models/test_swap.py @@ -3,44 +3,81 @@ from django.urls import reverse from django.core.exceptions import ValidationError -from scheduler.models import Student, User, Swap -from scheduler.factories import UserFactory, CourseFactory, SectionFactory, StudentFactory, MentorFactory +from scheduler.models import ( + Student, + User, + Swap +) +from scheduler.factories import ( + UserFactory, + CourseFactory, + SectionFactory, + StudentFactory, + MentorFactory +) + @pytest.fixture -def setup_section(db): +def setup_scheduler(db): """ Creates a pair of sections within the same course, each with a mentor and student. """ # Setup course course = CourseFactory.create() - # Setup test section one - mentor_user_one, student_user_one = UserFactory.create_batch(2) - mentor_one = MentorFactory.create(course=course, user=mentor_user_one) - section_one = SectionFactory.create(mentor=mentor_one) - student_one = StudentFactory.create(user=student_user_one, course=course, section=section_one) - # Setup test section two - mentor_user_two, student_user_two = UserFactory.create_batch(2) - mentor_two = MentorFactory.create(course=course, user=mentor_user_two) - section_two = SectionFactory.create(mentor=mentor_two) - student_two = StudentFactory.create(user=student_user_two, course=course, section=section_two) + # Setup sections + section_one = create_section(course) + section_two = create_section(course) + # Setup students + section_one_students = create_students(course, section_one, 3) + section_two_students = create_students(course, section_two, 3) - return course, section_one, section_two, student_one, student_two + return course, section_one, section_two, section_one_students, section_two_students @pytest.mark.django_db -def test_basic_swap_request_success(client, setup_section): +def test_basic_swap_request_success(client, setup_scheduler): """ Tests that a student can successfully request a swap. """ - # TODO: Add test for when a student successfully requests a swap from another student in a different section - section_one, section_two, student_one, student_two = setup_section[1:] - client.force_login(student_one.user) - post_url = reverse("section-swap", args=[section_one.id]) - data = json.dumps({"receiver_email": student_two.user.email, "student_id": student_one.id}) - response = client.post(post_url, data=data, content_type="application/json") + section_one, section_two, section_one_students, section_two_students = setup_scheduler[1:] + sender_student, reciever_student = section_one_students[0], section_two_students[0] + # Create a swap request + create_swap_request(client, sender_student, reciever_student) + # Make sure that the swap request was created + assert Swap.objects.filter(receiver=reciever_student).exists() + - # Check that the swap request was created - assert Swap.objects.filter(receiver=student_two).exists() +@pytest.mark.django_db +@pytest.mark.parametrize( + ["is_empty"], + [ + # expect empty swap list + (True,), + # expect non-empty swap list + (False,), + ], + ids=["empty swap list", "non-empty swap list"], +) +def test_basic_swap_get(client, setup_scheduler, is_empty): + """ + Tests getting a list of swaps for a student + """ + section_one, section_two, section_one_students, section_two_students = setup_scheduler[1:] + sender_student, reciever_student = section_one_students[0], section_two_students[0] + if not is_empty: + # Create a swap request + create_swap_request(client, sender_student, reciever_student) + # Get the list of swaps for the reciever + swaps = get_swap_requests(client, sender_student) + # Decode get response to UTF-8 + swaps = json.loads(swaps.content.decode("utf-8")) + if not is_empty: + # Make sure that the swap request was created + assert Swap.objects.filter(receiver=reciever_student).exists() + # Make sure that the swap request is in the list of swaps + assert len(swaps["sender"]) != 0 and swaps["sender"][0]["receiver"] == reciever_student.id + else: + assert len(swaps["sender"]) == 0 @pytest.mark.django_db @@ -54,25 +91,68 @@ def test_basic_swap_request_success(client, setup_section): # non-existent email ("abcd@example.io",), # valid email - (setup_section,) + (setup_scheduler,) ], ids=["empty email", "invalid email", "non-existent email", "valid email"], ) -def test_request_swap_invalid_email(client, setup_section, email, request): +def test_request_swap_invalid_email(client, setup_scheduler, email, request): """ Tests that a student cannot request a swap with an invalid email. """ - course, section_one, section_two, student_one, student_two = setup_section + course, section_one, section_two, section_one_students, section_two_students = setup_scheduler receiver_email = email if (type(receiver_email) != str): - receiver_email = request.getfixturevalue(email.__name__)[4].user.email - # TODO Add test for when a student requests a swap with a invalid email - + receiver_email = request.getfixturevalue(email.__name__)[4][0].user.email + # TODO Add test for when a student requests a swap with a invalid email @pytest.mark.django_db -def test_swap_with_stale_receiver(client, setup_section): +def test_swap_with_stale_receiver(client, setup_scheduler): """ Tests weather Swaps are being invalidated/deleted when receiver has swapped with another student. """ # TODO Add test for when a student requests a swap with a stale receiver + + +def create_students(course, section, quantity): + """ + Creates a given number of students for a given section. + """ + student_users = UserFactory.create_batch(quantity) + students = [] + for student_user in student_users: + student = StudentFactory.create(user=student_user, course=course, section=section) + students.append(student) + return students + + +def create_section(course): + """ + Creates a section for a given course. + """ + mentor_user = UserFactory.create() + mentor = MentorFactory.create(user=mentor_user, course=course) + section = SectionFactory.create(mentor=mentor) + return section + + +def create_swap_request(client, sender, reciever): + """ + Creates a swap request between two students. + """ + client.force_login(sender.user) + post_url = reverse("section-swap", args=[sender.section.id]) + data = json.dumps({"receiver_email": reciever.user.email, "student_id": sender.id}) + response = client.post(post_url, data=data, content_type="application/json") + return response + + +def get_swap_requests(client, sender): + """ + Gets a list of swap requests for a given student. + """ + client.force_login(sender.user) + get_url = reverse("section-swap", args=[sender.section.id]) + body = {"student_id": sender.id} + response = client.get(get_url, body, content_type="application/json") + return response \ No newline at end of file diff --git a/csm_web/scheduler/views/section.py b/csm_web/scheduler/views/section.py index 93400cf3..fc42412c 100644 --- a/csm_web/scheduler/views/section.py +++ b/csm_web/scheduler/views/section.py @@ -815,24 +815,27 @@ def swap(self, request, pk=None): """ section = get_object_or_error(Section.objects, pk=pk) is_mentor = section.mentor.user == request.user - student_id = request.data.get("student_id", -1) - student = Student.objects.get(id=student_id) + student_id = request.data.get("student_id", -1) if request.method != "GET" \ + else request.query_params.get("student_id", -1) if student_id == -1: raise PermissionDenied("The `student_id` field in the request cannot be empty," "please specify student.") + try: + student = get_object_or_error(Student.objects, id=student_id) + except Exception: + raise NotFound("No student found with id: " + str(student_id)) if request.method == "GET": if is_mentor: raise PermissionDenied("Cannot `GET` swaps for a mentor.") - student = get_object_or_error(Student.objects, id=student_id) try: outgoing_swaps = Swap.objects.filter(sender=student) incoming_swaps = Swap.objects.filter(receiver=student) except Exception as e: raise NotFound("No swaps found for student with id: " + str(student_id)) - + student_swaps = { - "sender": SwapSerializer(outgoing_swaps).data, - "receiver": SwapSerializer(incoming_swaps).data, + "sender": SwapSerializer(outgoing_swaps, many=True).data, + "receiver": SwapSerializer(incoming_swaps, many=True).data, } return Response(student_swaps, status=status.HTTP_200_OK) if request.method == "POST": From a7cac708fb842184a681f62995a29019cf115e5c Mon Sep 17 00:00:00 2001 From: Kartavya Sharma Date: Tue, 16 May 2023 01:55:16 -0700 Subject: [PATCH 15/21] Added test for checking request email validity, also added possible testing scenarios (pending implementation) --- csm_web/scheduler/tests/models/test_swap.py | 36 ++++++++++++++++++--- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/csm_web/scheduler/tests/models/test_swap.py b/csm_web/scheduler/tests/models/test_swap.py index f5412ec6..3d7c95a4 100644 --- a/csm_web/scheduler/tests/models/test_swap.py +++ b/csm_web/scheduler/tests/models/test_swap.py @@ -103,15 +103,43 @@ def test_request_swap_invalid_email(client, setup_scheduler, email, request): receiver_email = email if (type(receiver_email) != str): receiver_email = request.getfixturevalue(email.__name__)[4][0].user.email - # TODO Add test for when a student requests a swap with a invalid email + if type(email) == str: + with pytest.raises(Exception): + # Create a swap request + create_swap_request(client, section_one_students[0], email) + # Get the list of swaps for the reciever + swaps = get_swap_requests(client, section_one_students[0]) + else: + # Create a swap request + create_swap_request(client, section_one_students[0], section_two_students[0]) + # Check that the swap request was created + assert Swap.objects.filter(receiver=section_two_students[0]).exists() + + +@pytest.mark.django_db +def test_accept_swap_request(client, setup_scheduler): + """ + Tests that a student can accept a swap request. + """ + # TODO Add test for when a student accepts a swap request + + +@pytest.mark.django_db +def test_reject_swap_request(client, setup_scheduler): + """ + Tests that a student can reject a swap request. + """ + # TODO Add test for when a student rejects a swap request @pytest.mark.django_db -def test_swap_with_stale_receiver(client, setup_scheduler): +def test_swap_conflict_clear(client, setup_scheduler): """ - Tests weather Swaps are being invalidated/deleted when receiver has swapped with another student. + Test for when a student has multiple swap requests pending and one of them + is accepted. In this case, all other swap requests from that user should be + invalidated and cleared. """ - # TODO Add test for when a student requests a swap with a stale receiver + # TODO Add test for when a student accepts a swap in a conflict setup def create_students(course, section, quantity): From 5950a3ccff5075b5a0e6aba994fdb5855ab2e0bc Mon Sep 17 00:00:00 2001 From: Kartavya Sharma Date: Tue, 16 May 2023 16:52:33 -0700 Subject: [PATCH 16/21] Swap processing is now working, implemented new swap tests, updated swap serializer to include swap_id --- csm_web/scheduler/serializers.py | 2 +- csm_web/scheduler/tests/models/test_swap.py | 55 +++++++--- csm_web/scheduler/views/section.py | 112 +++++++++++--------- 3 files changed, 104 insertions(+), 65 deletions(-) diff --git a/csm_web/scheduler/serializers.py b/csm_web/scheduler/serializers.py index 33f72aa0..c8c25979 100644 --- a/csm_web/scheduler/serializers.py +++ b/csm_web/scheduler/serializers.py @@ -61,7 +61,7 @@ class Meta: class SwapSerializer(serializers.ModelSerializer): class Meta: model = Swap - fields = ("sender", "receiver") + fields = ("id", "sender", "receiver") class SpacetimeSerializer(serializers.ModelSerializer): diff --git a/csm_web/scheduler/tests/models/test_swap.py b/csm_web/scheduler/tests/models/test_swap.py index 3d7c95a4..fd1972a9 100644 --- a/csm_web/scheduler/tests/models/test_swap.py +++ b/csm_web/scheduler/tests/models/test_swap.py @@ -3,6 +3,7 @@ from django.urls import reverse from django.core.exceptions import ValidationError +from rest_framework.exceptions import NotFound from scheduler.models import ( Student, User, @@ -39,7 +40,7 @@ def test_basic_swap_request_success(client, setup_scheduler): """ Tests that a student can successfully request a swap. """ - section_one, section_two, section_one_students, section_two_students = setup_scheduler[1:] + section_one_students, section_two_students = setup_scheduler[3:] sender_student, reciever_student = section_one_students[0], section_two_students[0] # Create a swap request create_swap_request(client, sender_student, reciever_student) @@ -62,7 +63,7 @@ def test_basic_swap_get(client, setup_scheduler, is_empty): """ Tests getting a list of swaps for a student """ - section_one, section_two, section_one_students, section_two_students = setup_scheduler[1:] + section_one_students, section_two_students = setup_scheduler[3:] sender_student, reciever_student = section_one_students[0], section_two_students[0] if not is_empty: # Create a swap request @@ -99,21 +100,19 @@ def test_request_swap_invalid_email(client, setup_scheduler, email, request): """ Tests that a student cannot request a swap with an invalid email. """ - course, section_one, section_two, section_one_students, section_two_students = setup_scheduler + section_one_students, section_two_students = setup_scheduler[3:] + sender, receiver = section_one_students[0], section_two_students[0] receiver_email = email if (type(receiver_email) != str): - receiver_email = request.getfixturevalue(email.__name__)[4][0].user.email + receiver_email = receiver.user.email + receiver.user.email = receiver_email + response = create_swap_request(client, sender, receiver) if type(email) == str: - with pytest.raises(Exception): - # Create a swap request - create_swap_request(client, section_one_students[0], email) - # Get the list of swaps for the reciever - swaps = get_swap_requests(client, section_one_students[0]) + assert response.status_code == 404 else: - # Create a swap request - create_swap_request(client, section_one_students[0], section_two_students[0]) + assert response.status_code == 201 # Check that the swap request was created - assert Swap.objects.filter(receiver=section_two_students[0]).exists() + assert Swap.objects.filter(receiver=receiver).exists() @pytest.mark.django_db @@ -121,7 +120,20 @@ def test_accept_swap_request(client, setup_scheduler): """ Tests that a student can accept a swap request. """ - # TODO Add test for when a student accepts a swap request + section_one_students, section_two_students = setup_scheduler[3:] + sender, receiver = section_one_students[0], section_two_students[0] + create_swap_request(client, sender, receiver) + # Get the swap_id + swap_id = json.loads(get_swap_requests(client, sender).content.decode("utf-8"))["sender"][0]["id"] + accept_swap_request(client, receiver, swap_id) + # Record old section ids + old_sender_section_id, old_receiver_section_id = sender.section.id, receiver.section.id + # Get the new section ids + sender.refresh_from_db() + receiver.refresh_from_db() + # Make sure that the swap was accepted + assert sender.section.id == old_receiver_section_id + assert receiver.section.id == old_sender_section_id @pytest.mark.django_db @@ -169,7 +181,7 @@ def create_swap_request(client, sender, reciever): Creates a swap request between two students. """ client.force_login(sender.user) - post_url = reverse("section-swap", args=[sender.section.id]) + post_url = reverse("section-swap-no-id", args=[sender.section.id]) data = json.dumps({"receiver_email": reciever.user.email, "student_id": sender.id}) response = client.post(post_url, data=data, content_type="application/json") return response @@ -180,7 +192,18 @@ def get_swap_requests(client, sender): Gets a list of swap requests for a given student. """ client.force_login(sender.user) - get_url = reverse("section-swap", args=[sender.section.id]) + get_url = reverse("section-swap-no-id", args=[sender.section.id]) body = {"student_id": sender.id} response = client.get(get_url, body, content_type="application/json") - return response \ No newline at end of file + return response + + +def accept_swap_request(client, receiver, swap_id): + """ + Accepts a swap request between two students. + """ + client.force_login(receiver.user) + post_url = reverse("section-swap-with-id", kwargs={"section_id": receiver.section.id, "swap_id": swap_id}) + data = json.dumps({"student_id": receiver.id}) + response = client.post(post_url, data, content_type="application/json") + return response diff --git a/csm_web/scheduler/views/section.py b/csm_web/scheduler/views/section.py index fc42412c..83b0d74b 100644 --- a/csm_web/scheduler/views/section.py +++ b/csm_web/scheduler/views/section.py @@ -33,8 +33,8 @@ class SectionViewSet(*viewset_with("retrieve", "partial_update", "create")): serializer_class = SectionSerializer - # lookup_field = 'pk' - # lookup_url_kwarg = 'section_id' + lookup_field = 'pk' + lookup_url_kwarg = 'section_id' def get_object(self): """Retrieve section object""" @@ -785,8 +785,9 @@ def wotd(self, request, pk=None): raise PermissionDenied() - @action(detail=True, methods=["GET", "POST", "DELETE"]) - def swap(self, request, pk=None): + + @action(detail=True, methods=["GET", "POST"], url_path='swap') + def swap_no_id(self, request, section_id=None, swap_id=None): """ GET: Returns all relevant Swap objects for current user. Request format: @@ -813,7 +814,7 @@ def swap(self, request, pk=None): {} With status code of 201 if successful, 400 if not. """ - section = get_object_or_error(Section.objects, pk=pk) + section = get_object_or_error(Section.objects, pk=section_id) is_mentor = section.mentor.user == request.user student_id = request.data.get("student_id", -1) if request.method != "GET" \ else request.query_params.get("student_id", -1) @@ -871,49 +872,64 @@ def swap(self, request, pk=None): log_str(receiver.user), ) return Response({}, status=status.HTTP_201_CREATED) - else: - '''If swap_id is not None, initiate the swap between the two students. This should - be done in one atomic transaction. If either or both of the swaps are already - deleted, then return a 404 error.''' - with transaction.atomic(): - # Check if swap object with given id exists - try: - target_swap = get_object_or_error(Swap.objects, pk=swap_id).select_for_update() - except Exception as e: - logger.error( - " User %s could not swap with %s", - log_str(student.user), - log_str(receiver.user), - ) - raise PermissionDenied("Swap with id: " + str(swap_id) + " does not exist.") - # Execute atomic transaction, and swap sections - try: - sender = get_object_or_error(Student.objects, id=target_swap.sender.id) - receiver = get_object_or_error(Student.objects, id=target_swap.receiver.id) - except Exception as e: - logger.error( - " User %s could not swap with %s", - log_str(student.user), - log_str(receiver.user), - ) - raise NotFound("Could not find swaps for student.") - sender.section = target_swap.receiver.section - sender.save() - receiver.section = target_swap.sender.section - receiver.save() - # Delete all other swaps between the two students. Delete this swap object too. - target_swap.delete() - # Get outgoing and incoming swaps for sender and receiver - outgoing_swaps = Swap.objects.filter(sender=sender).select_for_update().values() - incoming_swaps = Swap.objects.filter(receiver=sender).select_for_update().values() - for expired_swap in outgoing_swaps + incoming_swaps: - expired_swap.delete() - logger.info( - " User %s swapped with %s", - log_str(sender.user), - log_str(receiver.user), - ) - return Response({}, status=status.HTTP_200_OK) + + + @action(detail=True, methods=["POST", "DELETE"], url_path='swap/(?P[^/.]+)') + def swap_with_id(self, request, section_id=None, swap_id=None): + '''If swap_id is not None, initiate the swap between the two students. This should + be done in one atomic transaction. If either or both of the swaps are already + deleted, then return a 404 error.''' + student_id = request.data.get("student_id", -1) if request.method != "GET" \ + else request.query_params.get("student_id", -1) + if student_id == -1: + raise PermissionDenied("The `student_id` field in the request cannot be empty," + "please specify student.") + try: + student = get_object_or_error(Student.objects, id=student_id) + except Exception: + raise NotFound("No student found with id: " + str(student_id)) + if request.method == "POST": + with transaction.atomic(): + # Check if swap object with given id exists + try: + target_swap = Swap.objects.select_for_update().get(id=swap_id) + except Exception as e: + logger.info(e) + logger.error( + " User %s could not complete swap. Swap does not exist.", + log_str(student.user), + ) + raise NotFound("Swap with id: " + str(swap_id) + " does not exist.") + # Execute atomic transaction, and swap sections + try: + sender = get_object_or_error(Student.objects, id=target_swap.sender.id) + receiver = get_object_or_error(Student.objects, id=target_swap.receiver.id) + except Exception as e: + logger.error( + " User %s could not swap with %s", + log_str(student.user), + log_str(receiver.user), + ) + raise NotFound("Could not find swaps for student.") + sender.section = target_swap.receiver.section + sender.save() + receiver.section = target_swap.sender.section + receiver.save() + # Delete all other swaps between the two students. Delete this swap object too. + target_swap.delete() + # Get outgoing and incoming swaps for sender and receiver + outgoing_swaps = Swap.objects.filter(sender=sender).select_for_update().values() + incoming_swaps = Swap.objects.filter(receiver=sender).select_for_update().values() + for expired_swap in outgoing_swaps: + expired_swap.delete() + for expired_swap in incoming_swaps: + expired_swap.delete() + logger.info( + " User %s swapped with %s", + log_str(sender.user), + log_str(receiver.user), + ) + return Response({}, status=status.HTTP_200_OK) if request.method == "DELETE": swap_id = self.kwargs.get('swap_id', None) From fb03a04f2d74092f5238f9f4f3d31e64247bc2be Mon Sep 17 00:00:00 2001 From: Kartavya Sharma Date: Sun, 4 Jun 2023 09:41:15 -0700 Subject: [PATCH 17/21] Fixed invalidation process for swaps and added more tests. --- csm_web/scheduler/tests/models/test_swap.py | 35 +++++++- csm_web/scheduler/views/section.py | 94 +++++++++++++-------- 2 files changed, 90 insertions(+), 39 deletions(-) diff --git a/csm_web/scheduler/tests/models/test_swap.py b/csm_web/scheduler/tests/models/test_swap.py index fd1972a9..cbd81127 100644 --- a/csm_web/scheduler/tests/models/test_swap.py +++ b/csm_web/scheduler/tests/models/test_swap.py @@ -134,6 +134,9 @@ def test_accept_swap_request(client, setup_scheduler): # Make sure that the swap was accepted assert sender.section.id == old_receiver_section_id assert receiver.section.id == old_sender_section_id + # Make sure that all swaps for both students are cleared + assert len(json.loads(get_swap_requests(client, sender).content.decode("utf-8"))["sender"]) == 0 + assert len(json.loads(get_swap_requests(client, receiver).content.decode("utf-8"))["sender"]) == 0 @pytest.mark.django_db @@ -141,7 +144,14 @@ def test_reject_swap_request(client, setup_scheduler): """ Tests that a student can reject a swap request. """ - # TODO Add test for when a student rejects a swap request + section_one_students, section_two_students = setup_scheduler[3:] + sender, receiver = section_one_students[0], section_two_students[0] + create_swap_request(client, sender, receiver) + # Get the swap_id + swap_id = json.loads(get_swap_requests(client, sender).content.decode("utf-8"))["sender"][0]["id"] + reject_swap_request(client, receiver, swap_id) + # Make sure that the swap was rejected + assert len(json.loads(get_swap_requests(client, receiver).content.decode("utf-8"))["sender"]) == 0 @pytest.mark.django_db @@ -151,8 +161,17 @@ def test_swap_conflict_clear(client, setup_scheduler): is accepted. In this case, all other swap requests from that user should be invalidated and cleared. """ - # TODO Add test for when a student accepts a swap in a conflict setup - + section_one_students, section_two_students = setup_scheduler[3:] + sender = section_one_students[0] + receiver_one, receiver_two = section_two_students[0], section_two_students[1] + create_swap_request(client, sender, receiver_one) + create_swap_request(client, sender, receiver_two) + # Get first swap_id + swap_id_one = json.loads(get_swap_requests(client, sender).content.decode("utf-8"))["sender"][0]["id"] + # Accept first swap request + accept_swap_request(client, receiver_one, swap_id_one) + # Make sure that the second swap request was cleared + assert len(json.loads(get_swap_requests(client, receiver_two).content.decode("utf-8"))["receiver"]) == 0 def create_students(course, section, quantity): """ @@ -207,3 +226,13 @@ def accept_swap_request(client, receiver, swap_id): data = json.dumps({"student_id": receiver.id}) response = client.post(post_url, data, content_type="application/json") return response + + +def reject_swap_request(client, receiver, swap_id): + """ + Receiver rejects a swap request from a sender. + """ + client.force_login(receiver.user) + delete_url = reverse("section-swap-with-id", kwargs={"section_id": receiver.section.id, "swap_id": swap_id}) + response = client.delete(delete_url) + return response \ No newline at end of file diff --git a/csm_web/scheduler/views/section.py b/csm_web/scheduler/views/section.py index 83b0d74b..8544f9f9 100644 --- a/csm_web/scheduler/views/section.py +++ b/csm_web/scheduler/views/section.py @@ -815,18 +815,21 @@ def swap_no_id(self, request, section_id=None, swap_id=None): With status code of 201 if successful, 400 if not. """ section = get_object_or_error(Section.objects, pk=section_id) - is_mentor = section.mentor.user == request.user student_id = request.data.get("student_id", -1) if request.method != "GET" \ else request.query_params.get("student_id", -1) + + # check if student_id is specified in request if student_id == -1: raise PermissionDenied("The `student_id` field in the request cannot be empty," "please specify student.") + + # check if student exists try: student = get_object_or_error(Student.objects, id=student_id) except Exception: raise NotFound("No student found with id: " + str(student_id)) if request.method == "GET": - if is_mentor: + if section.mentor.user == request.user: raise PermissionDenied("Cannot `GET` swaps for a mentor.") try: outgoing_swaps = Swap.objects.filter(sender=student) @@ -839,39 +842,56 @@ def swap_no_id(self, request, section_id=None, swap_id=None): "receiver": SwapSerializer(incoming_swaps, many=True).data, } return Response(student_swaps, status=status.HTTP_200_OK) + if request.method == "POST": - if is_mentor: + if section.mentor.user == request.user: logger.error("Cannot `POST` swaps for a mentor.") raise PermissionDenied("Cannot `POST` swaps for a mentor.") - swap_id = self.kwargs.get('swap_id', None) - if (swap_id is None): - '''If swap_id is None, create a new Swap object between the the current student - and the student with the email specified in the request. This is essentially - creating a swap invite.''' - receiver_email = request.data.get("receiver_email", None) - if receiver_email is None: - raise PermissionDenied("The `receiver_email` field in the request cannot be empty," - "please specify a receiver.") - try: - receiver = get_object_or_error(Student.objects, user__email=receiver_email) - except Exception as e: - raise NotFound("Invalid email provided.") - receiver_is_sender = receiver == student - if receiver_is_sender: - raise PermissionDenied("Cannot send a swap request to yourself.") - try: - # Create Swap request - swap = Swap.objects.create(sender=student, receiver=receiver) - except Exception as e: - raise NotFound("Could not create swap for student with id: " + str(student_id)) - # Successfuly created swap - logger.info( - " User %s requested a swap with %s", - log_str(student.user), - log_str(receiver.user), - ) - return Response({}, status=status.HTTP_201_CREATED) + '''Create a new Swap object between the the current student + and the student with the email specified in the request. This is essentially + creating a swap invite.''' + receiver_email = request.data.get("receiver_email", None) + + # Check if receiver_email is empty + if receiver_email is None: + raise PermissionDenied("The `receiver_email` field in the request cannot be empty," + "please specify a receiver.") + + # Check if receiver_email is valid + try: + receiver = get_object_or_error(Student.objects, user__email=receiver_email) + except Exception as e: + raise NotFound("Invalid email provided.") + + # Check if receiver is the sender + receiver_is_sender = receiver == student + if receiver_is_sender: + raise PermissionDenied("Cannot send a swap request to yourself.") + + # Check if receiver has already sent a swap request to the sender + try: + incoming_swaps = Swap.objects.filter(receiver=student) + except Exception as e: + logger.error(e) + raise PermissionDenied("Could not fetch incoming swaps for student with id: " + + str(student_id)) + for swap in incoming_swaps: + if swap.sender == receiver: + raise PermissionDenied("Cannot send a swap request to someone who has already" + "sent you a swap request.") + # Create Swap request + try: + swap = Swap.objects.create(sender=student, receiver=receiver) + except Exception as e: + raise NotFound("Could not create swap for student with id: " + str(student_id)) + # Successfuly created swap + logger.info( + " User %s requested a swap with %s", + log_str(student.user), + log_str(receiver.user), + ) + return Response({}, status=status.HTTP_201_CREATED) @action(detail=True, methods=["POST", "DELETE"], url_path='swap/(?P[^/.]+)') @@ -900,6 +920,7 @@ def swap_with_id(self, request, section_id=None, swap_id=None): log_str(student.user), ) raise NotFound("Swap with id: " + str(swap_id) + " does not exist.") + # Execute atomic transaction, and swap sections try: sender = get_object_or_error(Student.objects, id=target_swap.sender.id) @@ -918,8 +939,8 @@ def swap_with_id(self, request, section_id=None, swap_id=None): # Delete all other swaps between the two students. Delete this swap object too. target_swap.delete() # Get outgoing and incoming swaps for sender and receiver - outgoing_swaps = Swap.objects.filter(sender=sender).select_for_update().values() - incoming_swaps = Swap.objects.filter(receiver=sender).select_for_update().values() + outgoing_swaps = Swap.objects.filter(sender=sender).select_for_update() + incoming_swaps = Swap.objects.filter(receiver=sender).select_for_update() for expired_swap in outgoing_swaps: expired_swap.delete() for expired_swap in incoming_swaps: @@ -932,13 +953,14 @@ def swap_with_id(self, request, section_id=None, swap_id=None): return Response({}, status=status.HTTP_200_OK) if request.method == "DELETE": - swap_id = self.kwargs.get('swap_id', None) swap = get_object_or_error(Swap, pk=swap_id) if student_id != swap.reciever.id and student_id != swap.sender.id: logger.error( - f" Could not delete swap, user {log_str(request.user)} does not have proper permissions" + f" Could not delete swap, " + f"user {log_str(request.user)} does not have proper permissions" ) - raise PermissionDenied("You must be a student or a coordinator to delete this spacetime override!") + raise PermissionDenied("You must be a student or a coordinator" + "to delete this spacetime override!") swap.delete() swap.save() From bd2df336592fc21abb902500789374b0ba138fba Mon Sep 17 00:00:00 2001 From: Kartavya Sharma Date: Sun, 4 Jun 2023 10:31:34 -0700 Subject: [PATCH 18/21] Fixed error where only the sender's expired swaps were being deleted when a swap went through. --- csm_web/scheduler/tests/models/test_swap.py | 9 +++++++++ csm_web/scheduler/views/section.py | 19 +++++++++++++------ 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/csm_web/scheduler/tests/models/test_swap.py b/csm_web/scheduler/tests/models/test_swap.py index cbd81127..7cb32e5b 100644 --- a/csm_web/scheduler/tests/models/test_swap.py +++ b/csm_web/scheduler/tests/models/test_swap.py @@ -173,6 +173,15 @@ def test_swap_conflict_clear(client, setup_scheduler): # Make sure that the second swap request was cleared assert len(json.loads(get_swap_requests(client, receiver_two).content.decode("utf-8"))["receiver"]) == 0 + +@pytest.mark.django_db +def test_swap_conflict_concurrent_fail(client, setup_scheduler): + """ + Test for when two receivers accept a swap request from the same sender concurrently. + """ + # TODO implement this test + + def create_students(course, section, quantity): """ Creates a given number of students for a given section. diff --git a/csm_web/scheduler/views/section.py b/csm_web/scheduler/views/section.py index 8544f9f9..4d182a10 100644 --- a/csm_web/scheduler/views/section.py +++ b/csm_web/scheduler/views/section.py @@ -933,18 +933,25 @@ def swap_with_id(self, request, section_id=None, swap_id=None): ) raise NotFound("Could not find swaps for student.") sender.section = target_swap.receiver.section - sender.save() - receiver.section = target_swap.sender.section - receiver.save() - # Delete all other swaps between the two students. Delete this swap object too. - target_swap.delete() - # Get outgoing and incoming swaps for sender and receiver + # Get outgoing and incoming swaps for sender outgoing_swaps = Swap.objects.filter(sender=sender).select_for_update() incoming_swaps = Swap.objects.filter(receiver=sender).select_for_update() for expired_swap in outgoing_swaps: expired_swap.delete() for expired_swap in incoming_swaps: expired_swap.delete() + sender.save() + receiver.section = target_swap.sender.section + # Get outgoing and incoming swaps for receiver + outgoing_swaps = Swap.objects.filter(sender=receiver).select_for_update() + incoming_swaps = Swap.objects.filter(receiver=receiver).select_for_update() + for expired_swap in outgoing_swaps: + expired_swap.delete() + for expired_swap in incoming_swaps: + expired_swap.delete() + receiver.save() + # Finall, delete the swap object + target_swap.delete() logger.info( " User %s swapped with %s", log_str(sender.user), From 03e34c987d829672df2bb2cefe0e570d88191e6a Mon Sep 17 00:00:00 2001 From: Kartavya Sharma Date: Fri, 30 Jun 2023 22:19:49 -0700 Subject: [PATCH 19/21] Fixed locked rows for sender and receiver and removed a concurrency test --- csm_web/scheduler/tests/models/test_swap.py | 16 +--------------- csm_web/scheduler/views/section.py | 11 +++++++---- 2 files changed, 8 insertions(+), 19 deletions(-) diff --git a/csm_web/scheduler/tests/models/test_swap.py b/csm_web/scheduler/tests/models/test_swap.py index 7cb32e5b..68e1b9fc 100644 --- a/csm_web/scheduler/tests/models/test_swap.py +++ b/csm_web/scheduler/tests/models/test_swap.py @@ -2,13 +2,7 @@ import json from django.urls import reverse -from django.core.exceptions import ValidationError -from rest_framework.exceptions import NotFound -from scheduler.models import ( - Student, - User, - Swap -) +from scheduler.models import Swap from scheduler.factories import ( UserFactory, CourseFactory, @@ -174,14 +168,6 @@ def test_swap_conflict_clear(client, setup_scheduler): assert len(json.loads(get_swap_requests(client, receiver_two).content.decode("utf-8"))["receiver"]) == 0 -@pytest.mark.django_db -def test_swap_conflict_concurrent_fail(client, setup_scheduler): - """ - Test for when two receivers accept a swap request from the same sender concurrently. - """ - # TODO implement this test - - def create_students(course, section, quantity): """ Creates a given number of students for a given section. diff --git a/csm_web/scheduler/views/section.py b/csm_web/scheduler/views/section.py index 4d182a10..4f296e27 100644 --- a/csm_web/scheduler/views/section.py +++ b/csm_web/scheduler/views/section.py @@ -912,19 +912,22 @@ def swap_with_id(self, request, section_id=None, swap_id=None): with transaction.atomic(): # Check if swap object with given id exists try: - target_swap = Swap.objects.select_for_update().get(id=swap_id) + target_swap = get_object_or_error(Swap.objects.select_for_update(), id=swap_id) except Exception as e: logger.info(e) logger.error( - " User %s could not complete swap. Swap does not exist.", + " User %s could not complete swap. " + "Swap does not exist.", log_str(student.user), ) raise NotFound("Swap with id: " + str(swap_id) + " does not exist.") # Execute atomic transaction, and swap sections try: - sender = get_object_or_error(Student.objects, id=target_swap.sender.id) - receiver = get_object_or_error(Student.objects, id=target_swap.receiver.id) + sender = get_object_or_error(Student.objects.select_for_update(), + id=target_swap.sender.id) + receiver = get_object_or_error(Student.objects.select_for_update(), + id=target_swap.receiver.id) except Exception as e: logger.error( " User %s could not swap with %s", From bab07a4ef95a5bf045ad445adbff74a099b55455 Mon Sep 17 00:00:00 2001 From: Kartavya Sharma Date: Fri, 30 Jun 2023 22:22:34 -0700 Subject: [PATCH 20/21] Reverted comment change --- csm_web/scheduler/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/csm_web/scheduler/models.py b/csm_web/scheduler/models.py index 37a5111d..9737efc7 100644 --- a/csm_web/scheduler/models.py +++ b/csm_web/scheduler/models.py @@ -114,7 +114,7 @@ def week_start(self): class Meta: unique_together = ("sectionOccurrence", "student") ordering = ("sectionOccurrence",) - # indexes = (models.Index(fields=("date",)),) + #indexes = (models.Index(fields=("date",)),) class SectionOccurrence(ValidatingModel): From 59f467d65e4e96638400d1c581037d388f10ffdd Mon Sep 17 00:00:00 2001 From: Kartavya Sharma Date: Mon, 3 Jul 2023 19:03:05 -0700 Subject: [PATCH 21/21] Resolved requested changes, added specific exceptions, and split tests --- csm_web/scheduler/tests/models/test_swap.py | 57 ++++++------- csm_web/scheduler/views/section.py | 91 ++++++++++----------- 2 files changed, 74 insertions(+), 74 deletions(-) diff --git a/csm_web/scheduler/tests/models/test_swap.py b/csm_web/scheduler/tests/models/test_swap.py index 68e1b9fc..fa51367b 100644 --- a/csm_web/scheduler/tests/models/test_swap.py +++ b/csm_web/scheduler/tests/models/test_swap.py @@ -40,39 +40,40 @@ def test_basic_swap_request_success(client, setup_scheduler): create_swap_request(client, sender_student, reciever_student) # Make sure that the swap request was created assert Swap.objects.filter(receiver=reciever_student).exists() - + @pytest.mark.django_db -@pytest.mark.parametrize( - ["is_empty"], - [ - # expect empty swap list - (True,), - # expect non-empty swap list - (False,), - ], - ids=["empty swap list", "non-empty swap list"], -) -def test_basic_swap_get(client, setup_scheduler, is_empty): +def test_empty_swap_list(client, setup_scheduler): """ - Tests getting a list of swaps for a student + Test getting an empty list of swaps for a student. """ section_one_students, section_two_students = setup_scheduler[3:] - sender_student, reciever_student = section_one_students[0], section_two_students[0] - if not is_empty: - # Create a swap request - create_swap_request(client, sender_student, reciever_student) - # Get the list of swaps for the reciever + sender_student = section_one_students[0] + # Get the list of swaps for the sender swaps = get_swap_requests(client, sender_student) # Decode get response to UTF-8 swaps = json.loads(swaps.content.decode("utf-8")) - if not is_empty: - # Make sure that the swap request was created - assert Swap.objects.filter(receiver=reciever_student).exists() - # Make sure that the swap request is in the list of swaps - assert len(swaps["sender"]) != 0 and swaps["sender"][0]["receiver"] == reciever_student.id - else: - assert len(swaps["sender"]) == 0 + # Make sure that the list of swaps is empty + assert len(swaps["sender"]) == 0 + + +@pytest.mark.django_db +def test_non_empty_swap_list(client, setup_scheduler): + """ + Test getting a non-empty list of swaps for a student. + """ + section_one_students, section_two_students = setup_scheduler[3:] + sender_student, receiver_student = section_one_students[0], section_two_students[0] + # Create a swap request + create_swap_request(client, sender_student, receiver_student) + # Get the list of swaps for the sender + swaps = get_swap_requests(client, sender_student) + # Decode get response to UTF-8 + swaps = json.loads(swaps.content.decode("utf-8")) + # Make sure that the swap request was created + assert Swap.objects.filter(receiver=receiver_student).exists() + # Make sure that the swap request is in the list of swaps + assert len(swaps["sender"]) != 0 and swaps["sender"][0]["receiver"] == receiver_student.id @pytest.mark.django_db @@ -217,7 +218,7 @@ def accept_swap_request(client, receiver, swap_id): Accepts a swap request between two students. """ client.force_login(receiver.user) - post_url = reverse("section-swap-with-id", kwargs={"section_id": receiver.section.id, "swap_id": swap_id}) + post_url = reverse("section-swap-with-id", kwargs={"pk": receiver.section.id, "swap_id": swap_id}) data = json.dumps({"student_id": receiver.id}) response = client.post(post_url, data, content_type="application/json") return response @@ -228,6 +229,6 @@ def reject_swap_request(client, receiver, swap_id): Receiver rejects a swap request from a sender. """ client.force_login(receiver.user) - delete_url = reverse("section-swap-with-id", kwargs={"section_id": receiver.section.id, "swap_id": swap_id}) + delete_url = reverse("section-swap-with-id", kwargs={"pk": receiver.section.id, "swap_id": swap_id}) response = client.delete(delete_url) - return response \ No newline at end of file + return response diff --git a/csm_web/scheduler/views/section.py b/csm_web/scheduler/views/section.py index 4f296e27..45784a28 100644 --- a/csm_web/scheduler/views/section.py +++ b/csm_web/scheduler/views/section.py @@ -1,13 +1,14 @@ import datetime import re -from django.db import transaction +from django.core.exceptions import ValidationError, ObjectDoesNotExist +from django.db import transaction, IntegrityError, DatabaseError from django.db.models import Prefetch, Q from django.utils import timezone from rest_framework import status from rest_framework import permissions -from rest_framework.decorators import action, permission_classes -from rest_framework.exceptions import PermissionDenied, NotFound +from rest_framework.decorators import action +from rest_framework.exceptions import PermissionDenied, NotFound, ValidationError as BadRequestError from rest_framework.response import Response from scheduler.models import ( Attendance, @@ -33,8 +34,6 @@ class SectionViewSet(*viewset_with("retrieve", "partial_update", "create")): serializer_class = SectionSerializer - lookup_field = 'pk' - lookup_url_kwarg = 'section_id' def get_object(self): """Retrieve section object""" @@ -787,7 +786,7 @@ def wotd(self, request, pk=None): @action(detail=True, methods=["GET", "POST"], url_path='swap') - def swap_no_id(self, request, section_id=None, swap_id=None): + def swap_no_id(self, request, pk=None, swap_id=None): """ GET: Returns all relevant Swap objects for current user. Request format: @@ -805,7 +804,7 @@ def swap_no_id(self, request, section_id=None, swap_id=None): time: DateTime, } - POST: Handles creating a new Swap instance when a Swap a requested or + POST: Handles creating a new Swap instance when a Swap is requested or processes an existing Swap request. Request format: { email: string } @@ -814,27 +813,29 @@ def swap_no_id(self, request, section_id=None, swap_id=None): {} With status code of 201 if successful, 400 if not. """ - section = get_object_or_error(Section.objects, pk=section_id) - student_id = request.data.get("student_id", -1) if request.method != "GET" \ - else request.query_params.get("student_id", -1) + section = get_object_or_error(Section.objects, pk=pk) + if request.method != "GET": + student_id = request.data.get("student_id", -1) + else: + student_id = request.query_params.get("student_id", -1) # check if student_id is specified in request if student_id == -1: - raise PermissionDenied("The `student_id` field in the request cannot be empty," + raise BadRequestError("The `student_id` field in the request cannot be empty," "please specify student.") # check if student exists try: student = get_object_or_error(Student.objects, id=student_id) - except Exception: + except ObjectDoesNotExist as e: + logger.info(e) raise NotFound("No student found with id: " + str(student_id)) if request.method == "GET": if section.mentor.user == request.user: - raise PermissionDenied("Cannot `GET` swaps for a mentor.") - try: - outgoing_swaps = Swap.objects.filter(sender=student) - incoming_swaps = Swap.objects.filter(receiver=student) - except Exception as e: + raise BadRequestError("Cannot `GET` swaps for a mentor.") + outgoing_swaps = Swap.objects.filter(sender=student) + incoming_swaps = Swap.objects.filter(receiver=student) + if outgoing_swaps is None or incoming_swaps is None: raise NotFound("No swaps found for student with id: " + str(student_id)) student_swaps = { @@ -846,7 +847,7 @@ def swap_no_id(self, request, section_id=None, swap_id=None): if request.method == "POST": if section.mentor.user == request.user: logger.error("Cannot `POST` swaps for a mentor.") - raise PermissionDenied("Cannot `POST` swaps for a mentor.") + raise BadRequestError("Cannot `POST` swaps for a mentor.") '''Create a new Swap object between the the current student and the student with the email specified in the request. This is essentially @@ -855,35 +856,40 @@ def swap_no_id(self, request, section_id=None, swap_id=None): # Check if receiver_email is empty if receiver_email is None: - raise PermissionDenied("The `receiver_email` field in the request cannot be empty," + raise BadRequestError("The `receiver_email` field in the request cannot be empty," "please specify a receiver.") # Check if receiver_email is valid try: receiver = get_object_or_error(Student.objects, user__email=receiver_email) - except Exception as e: + except ObjectDoesNotExist as e: raise NotFound("Invalid email provided.") # Check if receiver is the sender receiver_is_sender = receiver == student if receiver_is_sender: - raise PermissionDenied("Cannot send a swap request to yourself.") + raise BadRequestError("Cannot send a swap request to yourself.") # Check if receiver has already sent a swap request to the sender - try: - incoming_swaps = Swap.objects.filter(receiver=student) - except Exception as e: - logger.error(e) - raise PermissionDenied("Could not fetch incoming swaps for student with id: " + + incoming_swaps = Swap.objects.filter(receiver=student) + if incoming_swaps is None: + logger.error("Could not fetch incoming swaps for student with id: %s", student_id) + raise NotFound("Could not fetch incoming swaps for student with id: " + str(student_id)) for swap in incoming_swaps: if swap.sender == receiver: - raise PermissionDenied("Cannot send a swap request to someone who has already" + raise NotFound("Cannot send a swap request to someone who has already" "sent you a swap request.") # Create Swap request try: swap = Swap.objects.create(sender=student, receiver=receiver) - except Exception as e: + except (IntegrityError, ValidationError, DatabaseError) as e: + logger.info(e) + logger.error( + " User %s requested a swap with %s", + log_str(student.user), + log_str(receiver.user) + ) raise NotFound("Could not create swap for student with id: " + str(student_id)) # Successfuly created swap logger.info( @@ -895,25 +901,24 @@ def swap_no_id(self, request, section_id=None, swap_id=None): @action(detail=True, methods=["POST", "DELETE"], url_path='swap/(?P[^/.]+)') - def swap_with_id(self, request, section_id=None, swap_id=None): + def swap_with_id(self, request, pk=None, swap_id=None): '''If swap_id is not None, initiate the swap between the two students. This should be done in one atomic transaction. If either or both of the swaps are already deleted, then return a 404 error.''' - student_id = request.data.get("student_id", -1) if request.method != "GET" \ - else request.query_params.get("student_id", -1) + student_id = request.data.get("student_id", -1) if student_id == -1: - raise PermissionDenied("The `student_id` field in the request cannot be empty," + raise BadRequestError("The `student_id` field in the request cannot be empty," "please specify student.") try: student = get_object_or_error(Student.objects, id=student_id) - except Exception: + except ObjectDoesNotExist as e: raise NotFound("No student found with id: " + str(student_id)) if request.method == "POST": with transaction.atomic(): # Check if swap object with given id exists try: target_swap = get_object_or_error(Swap.objects.select_for_update(), id=swap_id) - except Exception as e: + except ObjectDoesNotExist as e: logger.info(e) logger.error( " User %s could not complete swap. " @@ -928,7 +933,7 @@ def swap_with_id(self, request, section_id=None, swap_id=None): id=target_swap.sender.id) receiver = get_object_or_error(Student.objects.select_for_update(), id=target_swap.receiver.id) - except Exception as e: + except ObjectDoesNotExist as e: logger.error( " User %s could not swap with %s", log_str(student.user), @@ -938,22 +943,18 @@ def swap_with_id(self, request, section_id=None, swap_id=None): sender.section = target_swap.receiver.section # Get outgoing and incoming swaps for sender outgoing_swaps = Swap.objects.filter(sender=sender).select_for_update() + outgoing_swaps.delete() incoming_swaps = Swap.objects.filter(receiver=sender).select_for_update() - for expired_swap in outgoing_swaps: - expired_swap.delete() - for expired_swap in incoming_swaps: - expired_swap.delete() + incoming_swaps.delete() sender.save() receiver.section = target_swap.sender.section # Get outgoing and incoming swaps for receiver outgoing_swaps = Swap.objects.filter(sender=receiver).select_for_update() + outgoing_swaps.delete() incoming_swaps = Swap.objects.filter(receiver=receiver).select_for_update() - for expired_swap in outgoing_swaps: - expired_swap.delete() - for expired_swap in incoming_swaps: - expired_swap.delete() + incoming_swaps.delete() receiver.save() - # Finall, delete the swap object + # Finally, delete the swap object target_swap.delete() logger.info( " User %s swapped with %s", @@ -971,9 +972,7 @@ def swap_with_id(self, request, section_id=None, swap_id=None): ) raise PermissionDenied("You must be a student or a coordinator" "to delete this spacetime override!") - swap.delete() - swap.save() logger.info( f" Deleted swap {log_str(swap)}"