diff --git a/csm_web/scheduler/models.py b/csm_web/scheduler/models.py index 7eb1b2be..9e367ab6 100644 --- a/csm_web/scheduler/models.py +++ b/csm_web/scheduler/models.py @@ -42,6 +42,7 @@ def day_to_number(day_of_week): def week_bounds(date): + """Compute the bounds of the week containing the date.""" week_start = date - datetime.timedelta(days=date.weekday()) week_end = week_start + datetime.timedelta(weeks=1) return week_start, week_end @@ -51,6 +52,7 @@ class User(AbstractUser): priority_enrollment = models.DateTimeField(null=True, blank=True) def can_enroll_in_course(self, course, bypass_enrollment_time=False): + """Determine whether this user is allowed to enroll in the given course.""" # check restricted first if course.is_restricted and not self.is_whitelisted_for(course): return False @@ -61,17 +63,18 @@ def can_enroll_in_course(self, course, bypass_enrollment_time=False): ) if bypass_enrollment_time: return not is_associated + + if self.priority_enrollment: + now = timezone.now().astimezone(timezone.get_default_timezone()) + is_valid_enrollment_time = ( + self.priority_enrollment < now < course.enrollment_end + ) else: - if self.priority_enrollment: - now = timezone.now().astimezone(timezone.get_default_timezone()) - is_valid_enrollment_time = ( - self.priority_enrollment < now < course.enrollment_end - ) - else: - is_valid_enrollment_time = course.is_open() - return is_valid_enrollment_time and not is_associated + is_valid_enrollment_time = course.is_open() + return is_valid_enrollment_time and not is_associated def is_whitelisted_for(self, course: "Course"): + """Determine whether this user is whitelisted for the given course.""" return not course.is_restricted or self.whitelist.filter(pk=course.pk).exists() class Meta: @@ -123,10 +126,12 @@ def __str__(self): @property def section(self): + """Retrieve the section associated with this attendance.""" return self.student.section @property def week_start(self): + """Retrieve the date corresonding to the beginning of the week for this attendance.""" return week_bounds(self.sectionOccurrence.date)[0] class Meta: @@ -189,6 +194,7 @@ def clean(self): raise ValidationError("word of the day limit must be in days") def is_open(self): + """Determine whether the course is open for enrollment.""" now = timezone.now().astimezone(timezone.get_default_timezone()) return self.enrollment_start < now < self.enrollment_end @@ -201,6 +207,7 @@ class Profile(ValidatingModel): @property def name(self): + """Retrieve the user's full name.""" return self.user.get_full_name() def __str__(self): @@ -232,10 +239,8 @@ class Student(Profile): def save(self, *args, **kwargs): super().save(*args, **kwargs) - """ - Create an attendance for this week for the student if their section hasn't already been held this week - and no attendance for this week already exists. - """ + # Create an attendance for this week for the student if their section + # hasn't already been held this week and no attendance for this week already exists. now = timezone.now().astimezone(timezone.get_default_timezone()) week_start = week_bounds(now.date())[0] for spacetime in self.section.spacetimes.all(): @@ -255,14 +260,22 @@ def save(self, *args, **kwargs): ): if settings.DJANGO_ENV != settings.DEVELOPMENT: logger.info( - " SO automatically created for student" - f" {self.user.email} in course {course.name} for date" - f" {now.date()}" + ( + " SO automatically created for student" + " %s in course %s for date %s" + ), + self.user.email, + course.name, + now.date(), ) logger.info( - " Attendance automatically created for student" - f" {self.user.email} in course {course.name} for date" - f" {now.date()}" + ( + " Attendance automatically created for student" + " %s in course %s for date %s" + ), + self.user.email, + course.name, + now.date(), ) so_qs = SectionOccurrence.objects.filter( section=self.section, @@ -336,6 +349,7 @@ class Section(ValidatingModel): @functional.cached_property def current_student_count(self): + """Query the number of students currently enrolled in this section.""" return self.students.filter(active=True).count() def delete(self, *args, **kwargs): @@ -348,26 +362,28 @@ def delete(self, *args, **kwargs): def clean(self): super().clean() - """ - Checking self.pk is checking if this is a creation (as opposed to an update) - We can't possibly have spacetimes at creation time (because it's a foreign-key field), - so we only validate this on updates - """ + # Checking self.pk is checking if this is a creation (as opposed to an update) + # We can't possibly have spacetimes at creation time (because it's a foreign-key field), + # so we only validate this on updates if self.pk and not self.spacetimes.exists(): raise ValidationError("Section must have at least one Spacetime") def __str__(self): - return "{course} section ({enrolled}/{cap}, {mentor}, {spacetimes})".format( - # pylint is unable to recognize the reverse accessor in the OneToOneOrNoneField - course=self.mentor.course.name, # pylint: disable=no-member - mentor="(no mentor)" if not self.mentor else self.mentor.name, - enrolled=self.current_student_count, - cap=self.capacity, - spacetimes="|".join(map(str, self.spacetimes.all())), + # pylint is unable to recognize the reverse accessor in the OneToOneOrNoneField + course_name = self.mentor.course.name # pylint: disable=no-member + enrolled_count = self.current_student_count + capacity = self.capacity + mentor_name = "(no mentor)" if not self.mentor else self.mentor.name + spacetimes = "|".join(map(str, self.spacetimes.all())) + + return ( + f"{course_name} section ({enrolled_count}/{capacity}, {mentor_name}," + f" {spacetimes})" ) def worksheet_path(instance, filename): + """Compute the full worksheet path for a worksheet file.""" # file will be uploaded to MEDIA_ROOT// course_name = str(instance.resource.course.name).replace(" ", "") return f"resources/{course_name}/{filename}" @@ -404,7 +420,7 @@ class Worksheet(ValidatingModel): @receiver(models.signals.post_delete, sender=Worksheet) -def auto_delete_file_on_delete(sender, instance, **kwargs): +def auto_delete_file_on_delete(instance, **kwargs): """ Deletes file from filesystem when corresponding `Worksheet` object is deleted. @@ -416,13 +432,13 @@ def auto_delete_file_on_delete(sender, instance, **kwargs): @receiver(models.signals.pre_save, sender=Worksheet) -def auto_delete_file_on_change(sender, instance, **kwargs): +def auto_delete_file_on_change(instance, **kwargs): """ Deletes old file from filesystem when corresponding `Worksheet` object is updated with a new file. """ if not instance.pk: - return False + return db_obj = Worksheet.objects.get(pk=instance.pk) exists = True @@ -465,6 +481,7 @@ class Spacetime(ValidatingModel): @property def override(self): + """Retrieve the override for this spacetime if it exists.""" # pylint is unable to recognize the reverse accessor # pylint: disable=no-member return ( @@ -475,6 +492,7 @@ def override(self): @property def end_time(self): + """Retrieve the end time for this spacetime.""" # Time does not support addition/subtraction, # so we have to create a datetime wrapper over start_time to add it to duration return ( @@ -490,6 +508,7 @@ def end_time(self): ).time() def day_number(self): + """Retrieve the day number corresponding to the day of week for this spacetime.""" return day_to_number(self.day_of_week) def __str__(self): @@ -524,6 +543,7 @@ def clean(self): ) def is_expired(self): + """Determine whether this override for the spacetime is expired.""" now = timezone.now().astimezone(timezone.get_default_timezone()) return self.date < now.date() diff --git a/csm_web/scheduler/views/spacetime.py b/csm_web/scheduler/views/spacetime.py index 0377e729..c96b169d 100644 --- a/csm_web/scheduler/views/spacetime.py +++ b/csm_web/scheduler/views/spacetime.py @@ -23,6 +23,11 @@ def get_queryset(self): ).distinct() def destroy(self, request, pk=None): + """ + Deletes a spacetime, if possible. + + Will not delete the spacetime if it will leave no spacetimes associated with a section. + """ spacetime = get_object_or_error(self.get_queryset(), pk=pk) section = spacetime.section course = section.mentor.course @@ -30,8 +35,11 @@ def destroy(self, request, pk=None): is_coordinator = course.coordinator_set.filter(user=request.user).exists() if not is_coordinator: logger.error( - " Could not delete spacetime, user" - f" {log_str(request.user)} does not have proper permissions" + ( + " Could not delete spacetime, user %s" + " does not have proper permissions" + ), + log_str(request.user), ) raise PermissionDenied( "You must be a coordinator to delete this spacetime!" @@ -40,8 +48,8 @@ def destroy(self, request, pk=None): has_multiple_spacetimes = section.spacetimes.count() > 1 if not has_multiple_spacetimes: logger.error( - f" Could not delete spacetime, only one" - f" spacetime exists" + " Could not delete spacetime, only one" + " spacetime exists" ) return Response( status=status.HTTP_400_BAD_REQUEST, @@ -50,17 +58,19 @@ def destroy(self, request, pk=None): now = timezone.now().astimezone(timezone.get_default_timezone()) - # FIX ME: Once issue #303 is resolved, this should delete the directly related section occurences - future_sectionOccurrences = section.sectionoccurrence_set.filter( + # FIX ME: Once issue #303 is resolved, this should delete + # the directly related section occurences + future_section_occurrences = section.sectionoccurrence_set.filter( date__gte=now.date(), date__week_day=spacetime.day_number() ) with transaction.atomic(): spacetime.delete() - future_sectionOccurrences.delete() + future_section_occurrences.delete() logger.info( - f" Deleted Spacetime {log_str(spacetime)}" + " Deleted Spacetime %s", + log_str(spacetime), ) return Response(status=status.HTTP_200_OK) @@ -86,17 +96,20 @@ def modify(self, request, pk=None): # validate and save spacetime.save() logger.info( - f" Modified Spacetime {log_str(spacetime)} (previously" - f" {old_spacetime_str})" + " Modified Spacetime %s (previously %s)", + log_str(spacetime), + old_spacetime_str, ) return Response(status=status.HTTP_202_ACCEPTED) @action(detail=True, methods=["put", "delete"]) def override(self, request, pk=None): + """Either update or delete a spacetime override.""" spacetime = get_object_or_error(self.get_queryset(), pk=pk) if request.method == "PUT": if hasattr(spacetime, "_override"): # update + # pylint: disable-next=protected-access serializer = OverrideSerializer(spacetime._override, data=request.data) status_code = status.HTTP_202_ACCEPTED else: # create @@ -107,19 +120,21 @@ def override(self, request, pk=None): if serializer.is_valid(): override = serializer.save() logger.info( - f" Overrode Spacetime {log_str(spacetime)} with" - f" Override {log_str(override)}" + " Overrode Spacetime %s with Override %s", + log_str(spacetime), + log_str(override), ) return Response(status=status_code) logger.error( - f" Could not override Spacetime {log_str(spacetime)}," - f" errors: {serializer.errors}" + " Could not override Spacetime %s, errors: %s", + log_str(spacetime), + serializer.errors, ) return Response( serializer.errors, status=status.HTTP_422_UNPROCESSABLE_ENTITY ) - elif request.method == "DELETE": + if request.method == "DELETE": mentor = spacetime.section.mentor course = mentor.course @@ -127,8 +142,11 @@ def override(self, request, pk=None): is_coordinator = course.coordinator_set.filter(user=request.user).exists() if not (is_mentor or is_coordinator): logger.error( - " Could not delete override, user" - f" {log_str(request.user)} does not have proper permissions" + ( + " Could not delete override, user %s" + " does not have proper permissions" + ), + log_str(request.user), ) raise PermissionDenied( "You must be a mentor or a coordinator to delete this spacetime" @@ -136,10 +154,13 @@ def override(self, request, pk=None): ) if hasattr(spacetime, "_override"): - override = spacetime._override + override = spacetime._override # pylint: disable=protected-access override.delete() logger.info( - f" Deleted override for {log_str(spacetime)}" + " Deleted override for %s", + log_str(spacetime), ) return Response(status=status.HTTP_200_OK) + + return Response(status=status.HTTP_404_NOT_FOUND) diff --git a/csm_web/scheduler/views/utils.py b/csm_web/scheduler/views/utils.py index cdbd8a15..00179c2f 100644 --- a/csm_web/scheduler/views/utils.py +++ b/csm_web/scheduler/views/utils.py @@ -1,10 +1,8 @@ import logging from operator import attrgetter -from typing import Any, Generic, Iterator, TypeVar +from typing import Any from django.core.exceptions import ObjectDoesNotExist, PermissionDenied -from django.db import models -from django.db.models.query import QuerySet from django.shortcuts import get_object_or_404 from rest_framework import mixins, viewsets from scheduler.models import ( @@ -21,6 +19,8 @@ def log_str(obj) -> str: + """Convert a Django model into a string for logging.""" + def log_format(*args) -> str: return ( "<" @@ -44,7 +44,9 @@ def log_format(*args) -> str: return log_format("pk", "date", "spacetime.pk") if isinstance(obj, Attendance): return log_format("pk", "sectionOccurrence.date", "presence") - except Exception as error: + except Exception as error: # pylint: disable=broad-exception-caught + # we want to catch all exceptions here, since logging shouldn't break the application; + # any exceptions raised during logging should just be logged and ignored logger.error(" Exception while logging: %s", error) return "" @@ -57,10 +59,10 @@ def get_object_or_error(specific_queryset, **kwargs): """ try: return specific_queryset.get(**kwargs) - except ObjectDoesNotExist: + except ObjectDoesNotExist as error: if get_object_or_404(specific_queryset.model.objects, **kwargs): - raise PermissionDenied() - raise ObjectDoesNotExist() + raise PermissionDenied() from error + raise ObjectDoesNotExist() from error METHOD_MIXINS = { @@ -74,6 +76,7 @@ def get_object_or_error(specific_queryset, **kwargs): def viewset_with(*permitted_methods: str) -> Any: + """Helper method to create a list of permitted methods for a viewset.""" assert all( method in METHOD_MIXINS for method in permitted_methods ), "Unrecognized method for ViewSet"