From a8def01e4e163e225429d0b957f82e035d33182f Mon Sep 17 00:00:00 2001 From: Markku Riekkinen Date: Fri, 6 May 2022 13:44:36 +0300 Subject: [PATCH 1/2] WIP Approve late/unofficial submissions when adding deviations TODO: * +1 max submissions deviation button in the inspect submission view: should this also check if any submission should be approved immediately? * test all the changes * deviations/viewbase.py approve_unofficial_submissions(): implementation unfinished * clean up the code * success messages to the user in the user interface? When deviations are added, the teacher may choose to also immediately approve such existing submissions that are covered by the deviation. For example, if the submission was late, but with the new personal deadline it is not late, then it would be approved. Fixes #892. "The second pull request" in the comment: https://github.com/apluslms/a-plus/issues/892#issuecomment-1048656166 This work started in Muhammad's pull request #991. Markku has refactored the code, improved the database queries and the details overall. Co-authored-by: Muhammad Wahjoe --- deviations/forms.py | 10 ++ deviations/viewbase.py | 189 +++++++++++++++++++++++++++++++- deviations/views.py | 65 ++++++++++- locale/en/LC_MESSAGES/django.po | 12 ++ locale/fi/LC_MESSAGES/django.po | 12 ++ 5 files changed, 281 insertions(+), 7 deletions(-) diff --git a/deviations/forms.py b/deviations/forms.py index ad5454804..2f5bf364e 100644 --- a/deviations/forms.py +++ b/deviations/forms.py @@ -89,6 +89,11 @@ class DeadlineRuleDeviationForm(BaseDeviationForm): initial=True, label=_('LABEL_WITHOUT_LATE_PENALTY'), ) + approve_late_submissions = forms.BooleanField( + required=False, + initial=True, + label=_('LABEL_APPROVE_LATE_SUBMISSIONS'), + ) def __init__(self, *args: Any, **kwargs: Any) -> None: super().__init__(*args, **kwargs) @@ -113,6 +118,11 @@ class MaxSubmissionRuleDeviationForm(BaseDeviationForm): min_value=1, label=_('LABEL_EXTRA_SUBMISSIONS'), ) + approve_unofficial_submissions = forms.BooleanField( + required=False, + initial=True, + label=_('LABEL_APPROVE_UNOFFICIAL_SUBMISSIONS'), + ) def __init__(self, *args, **kwargs) -> None: super().__init__(*args, **kwargs) diff --git a/deviations/viewbase.py b/deviations/viewbase.py index 184d1194f..276cbea6a 100644 --- a/deviations/viewbase.py +++ b/deviations/viewbase.py @@ -1,11 +1,11 @@ +import datetime from itertools import groupby -from typing import Any, Dict, Iterable, List, Optional, Tuple, Type +from typing import AbstractSet, Any, Dict, Iterable, List, Optional, Tuple, Type from django.db import models from django.http import HttpRequest, HttpResponse from django.contrib import messages from django import forms -from django.shortcuts import get_object_or_404 from django.utils.text import format_lazy from django.utils.translation import ugettext_lazy as _, ngettext @@ -14,7 +14,7 @@ from deviations.models import SubmissionRuleDeviation from lib.viewbase import BaseFormView, BaseRedirectView from authorization.permissions import ACCESS -from exercise.models import BaseExercise +from exercise.models import BaseExercise, Submission from userprofile.models import UserProfile @@ -66,6 +66,11 @@ def form_valid(self, form: forms.BaseForm) -> HttpResponse: ) new_deviation.update_by_form(form.cleaned_data) new_deviation.save() + submission_count = self.approve_submissions( + exercises, + submitters, + form.cleaned_data, + )#TODO return super().form_valid(form) @@ -79,6 +84,21 @@ def serialize_session_data(self, form_data: Dict[str, Any]) -> Dict[str, Any]: result[key] = [i.id for i in form_data.get(key, [])] return result + def approve_submissions( + self, + exercises: models.QuerySet[BaseExercise], + submitters: models.QuerySet[UserProfile], + form_data: Dict[str, Any], + ) -> Optional[int]: + """Approve existing late and/or unofficial submissions + that are covered by the new deviations. + + If the form_data disables the approval, then no submissions are changed + and this method returns None. + Otherwise, return the number of approved submissions. + """ + raise NotImplementedError("Child classes must override the method approve_submissions().") + class OverrideDeviationsView(CourseInstanceMixin, BaseFormView): access_mode = ACCESS.TEACHER @@ -123,14 +143,18 @@ def form_valid(self, form: forms.BaseForm) -> HttpResponse: existing_deviations = {(d.submitter_id, d.exercise_id): d for d in self.existing_deviations} + excluded_deviations = set() for exercise in self.exercises: for submitter in self.submitters: - existing_deviation = existing_deviations.get((submitter.id, exercise.id)) + deviation_pair = (submitter.id, exercise.id) + existing_deviation = existing_deviations.get(deviation_pair) if existing_deviation is not None: - if (submitter.id, exercise.id) in override_deviations: + if deviation_pair in override_deviations: existing_deviation.granter = self.request.user.userprofile existing_deviation.update_by_form(self.session_data) existing_deviation.save() + else: + excluded_deviations.add(deviation_pair) else: new_deviation = self.deviation_model( exercise=exercise, @@ -141,6 +165,13 @@ def form_valid(self, form: forms.BaseForm) -> HttpResponse: new_deviation.save() del self.request.session[self.session_key] + #TODO approve late/unofficial submissions + submission_count = self.approve_submissions( + self.exercises, + self.submitters, + self.session_data, + excluded_deviations, + ) return super().form_valid(form) def deserialize_session_data(self, session_data: Dict[str, Any]) -> Dict[str, Any]: @@ -155,6 +186,20 @@ def deserialize_session_data(self, session_data: Dict[str, Any]) -> Dict[str, An } return result + def approve_submissions( + self, + exercises: models.QuerySet[BaseExercise], + submitters: models.QuerySet[UserProfile], + form_data: Dict[str, Any], + excluded_deviations: AbstractSet[Tuple[int, int]], + ) -> int: + """Approve existing late and/or unofficial submissions + that are covered by the new deviations. + + Return the number of approved submissions. + """ + raise NotImplementedError("Child classes must override the method approve_submissions().") + class RemoveDeviationsByIDView(CourseInstanceMixin, BaseRedirectView): access_mode = ACCESS.TEACHER @@ -196,7 +241,7 @@ def form_valid(self, form: forms.BaseForm) -> HttpResponse: if number_of_removed == 0: messages.warning(self.request, _("NOTHING_REMOVED")) else: - message = format_lazy( + message = format_lazy(#TODO the string is not lazy ngettext( 'REMOVED_DEVIATION -- {count}', 'REMOVED_DEVIATIONS -- {count}', @@ -301,3 +346,135 @@ def get_submitters(form_data: Dict[str, Any]) -> models.QuerySet[UserProfile]: models.Q(id__in=form_data.get('submitter', [])) | models.Q(taggings__tag__in=form_data.get('submitter_tag', [])) ).distinct() + + +def approve_late_submissions( + exercises: models.QuerySet[BaseExercise], + submitters: models.QuerySet[UserProfile], + excluded_deviations: Optional[AbstractSet[Tuple[int, int]]] = None, + extra_minutes: Optional[int] = None, + new_deadline: Optional[datetime.datetime] = None, + ) -> int: + exercises_by_module = {} + for exercise in exercises: + exercises_by_module.setdefault(exercise.course_module_id, []).append(exercise) + + for module_id, exercise_list in exercises_by_module.items(): + if extra_minutes is not None: + dl = exercise_list[0].course_module.closing_time + datetime.timedelta(minutes=extra_minutes) + else: + dl = new_deadline + submissions = (Submission.objects + .exclude_errors() + .defer_text_fields() + .filter( + # Late submissions do not have any late penalty when late submissions are disallowed, + # but unofficial submissions are allowed. + # The submission becomes then unofficial without any late penalty. + # Note: the exercise max_submissions limit is not checked here. + # Some of the unofficial submissions may have exceeded the submission attempt limit. + # Those submissions are approved here as well with the assumption that + # the teacher intended that when he/she added new deadline deviations. + models.Q(status=Submission.STATUS.UNOFFICIAL) | models.Q(late_penalty_applied__isnull=False), + exercise__in=exercise_list, + submitters__in=submitters, + submission_time__lte=dl, + )) + submission_count = 0 + for submission in submissions: + may_approve = True + for submitter in submission.submitters.all(): + if excluded_deviations and (submitter.id, submission.exercise_id) in excluded_deviations: + may_approve = False + break + if may_approve: + submission.approve_penalized_submission() + submission.save() + submission_count += 1 + + return submission_count + + +def approve_unofficial_submissions( + exercises: models.QuerySet[BaseExercise], + submitters: models.QuerySet[UserProfile], + excluded_deviations: Optional[AbstractSet[Tuple[int, int]]] = None, + extra_submissions: int = 0, + ) -> int: + #TODO fix this function + # Fetch submissions that exceeded the max submissions limit of the exercise and have status UNOFFICIAL. + # submissions = (Submission.objects + # .exclude_errors() + # .defer_text_fields() + # .filter( + # exercise__in=exercises, + # submitters__in=submitters, + # ) + # .annotate(rank=models.Window(#Window disallowed in filter() + # DenseRank(), + # partition_by=[models.F('exercise'), models.F('submitters')], + # order_by=models.F('submission_time').asc(), + # )) + # .filter( + # status=Submission.STATUS.UNOFFICIAL, + # rank__gt=models.F('exercise__max_submissions'), + # rank__lte=models.F('exercise__max_submissions') + extra_submissions, + # #exercise__in=exercises, + # #submitters__in=submitters, + # ) + # .order_by('exercise', 'submitters', 'submission_time') + # ) + + submission_counts = (Submission.objects + .filter( + exercise__in=exercises, + submitters__in=submitters, + ) + .values( + 'exercise', + 'submitters', + ) + .annotate( + count=models.Count('id'), + max_submissions=models.F('exercise__max_submissions'), + ) + .order_by() + ) + exercise_submitter_pairs = set() + for c in submission_counts: + if c['count'] > c['max_submissions']: + exercise_submitter_pairs.add((c['exercise'], c['submitters'])) + # fetch submissions for exercise_submitter_pairs and check if some submissions should be approved + # (over max submissions and within extra submissions, status unofficial) + + submissions = (Submission.objects + .exclude_errors() + .defer_text_fields() + .filter( + exercise__in=exercises, + submitters__in=submitters, + ) + .order_by('exercise', 'submitters', 'submission_time') + ) + #TODO itertools groupby ?? + submission_count = 0 + for submission in submissions: + # groupby? get one student's submissions in one exercise + # count the number of submissions and approve unofficial submissions above the exercise.max_submissions + # (extra_submissions define how many are approved) + submission_count += 1 + + ######################################### old below + submission_count = 0 + for submission in submissions: + may_approve = True + for submitter in submission.submitters.all(): + if excluded_deviations and (submitter.id, submission.exercise_id) in excluded_deviations: + may_approve = False + break + if may_approve: + submission.approve_penalized_submission() + submission.save() + submission_count += 1 + + return submission_count diff --git a/deviations/views.py b/deviations/views.py index ad96788aa..8eb244b92 100644 --- a/deviations/views.py +++ b/deviations/views.py @@ -1,7 +1,10 @@ -from typing import Any, Dict +from typing import AbstractSet, Any, Dict, Optional, Tuple +from django.db import models from django.utils.dateparse import parse_datetime +from exercise.models import BaseExercise +from userprofile.models import UserProfile from .forms import ( DeadlineRuleDeviationForm, RemoveDeviationForm, @@ -9,6 +12,8 @@ ) from .viewbase import ( AddDeviationsView, + approve_late_submissions, + approve_unofficial_submissions, ListDeviationsView, OverrideDeviationsView, RemoveDeviationsByIDView, @@ -37,6 +42,21 @@ def serialize_session_data(self, form_data: Dict[str, Any]) -> Dict[str, Any]: }) return result + def approve_submissions( + self, + exercises: models.QuerySet[BaseExercise], + submitters: models.QuerySet[UserProfile], + form_data: Dict[str, Any], + ) -> Optional[int]: + if form_data.get('approve_late_submissions', False): + return approve_late_submissions( + exercises, + submitters, + extra_minutes=form_data.get('minutes'),#TODO int or datetime? + new_deadline=form_data.get('new_date'), + ) + return None + class OverrideDeadlinesView(OverrideDeviationsView): template_name = "deviations/override_dl.html" @@ -52,6 +72,21 @@ def deserialize_session_data(self, session_data: Dict[str, Any]) -> Dict[str, An }) return result + def approve_submissions( + self, + exercises: models.QuerySet[BaseExercise], + submitters: models.QuerySet[UserProfile], + form_data: Dict[str, Any], + excluded_deviations: AbstractSet[Tuple[int, int]], + ) -> int: + return approve_late_submissions( + exercises, + submitters, + excluded_deviations=excluded_deviations, + extra_minutes=form_data.get('minutes'), + new_deadline=form_data.get('new_date'), + ) + class RemoveDeadlinesByIDView(RemoveDeviationsByIDView): deviation_model = DeadlineRuleDeviation @@ -79,6 +114,20 @@ def serialize_session_data(self, form_data: Dict[str, Any]) -> Dict[str, Any]: result['extra_submissions'] = form_data['extra_submissions'] return result + def approve_submissions( + self, + exercises: models.QuerySet[BaseExercise], + submitters: models.QuerySet[UserProfile], + form_data: Dict[str, Any], + ) -> Optional[int]: + if form_data.get('approve_unofficial_submissions', False): + return approve_unofficial_submissions( + exercises, + submitters, + extra_submissions=form_data['extra_submissions'], + ) + return None + class OverrideSubmissionsView(OverrideDeviationsView): template_name = "deviations/override_submissions.html" @@ -90,6 +139,20 @@ def deserialize_session_data(self, session_data: Dict[str, Any]) -> Dict[str, An result['extra_submissions'] = session_data['extra_submissions'] return result + def approve_submissions( + self, + exercises: models.QuerySet[BaseExercise], + submitters: models.QuerySet[UserProfile], + form_data: Dict[str, Any], + excluded_deviations: AbstractSet[Tuple[int, int]], + ) -> int: + return approve_unofficial_submissions( + exercises, + submitters, + excluded_deviations=excluded_deviations, + extra_submissions=form_data['extra_submissions'], + ) + class RemoveSubmissionsByIDView(RemoveDeviationsByIDView): deviation_model = MaxSubmissionsRuleDeviation diff --git a/locale/en/LC_MESSAGES/django.po b/locale/en/LC_MESSAGES/django.po index 176c7a782..c80b37865 100644 --- a/locale/en/LC_MESSAGES/django.po +++ b/locale/en/LC_MESSAGES/django.po @@ -1486,6 +1486,12 @@ msgstr "New submission deadline." msgid "LABEL_WITHOUT_LATE_PENALTY" msgstr "Do not apply late penalty during extra time." +#: deviations/forms.py +msgid "LABEL_APPROVE_LATE_SUBMISSIONS" +msgstr "" +"Approve existing late and/or unofficial submissions that are covered by the " +"new personal deadline." + #: deviations/forms.py msgid "DEVIATION_MODULE_ADD_HELPTEXT" msgstr "" @@ -1521,6 +1527,12 @@ msgstr "You have to provide either the extra time or a date in the future." msgid "LABEL_EXTRA_SUBMISSIONS" msgstr "Extra submissions" +#: deviations/forms.py +msgid "LABEL_APPROVE_UNOFFICIAL_SUBMISSIONS" +msgstr "" +"Approve existing unofficial submissions that are covered by the new personal " +"submission attempt limit." + #: deviations/forms.py msgid "DEVIATION_MODULE_REMOVE_HELPTEXT" msgstr "" diff --git a/locale/fi/LC_MESSAGES/django.po b/locale/fi/LC_MESSAGES/django.po index c4e3acba9..c54838985 100644 --- a/locale/fi/LC_MESSAGES/django.po +++ b/locale/fi/LC_MESSAGES/django.po @@ -1495,6 +1495,12 @@ msgstr "Uusi määräaika." msgid "LABEL_WITHOUT_LATE_PENALTY" msgstr "Älä käytä myöhästymissakkoa ylimääräisen palautusajan aikana." +#: deviations/forms.py +msgid "LABEL_APPROVE_LATE_SUBMISSIONS" +msgstr "" +"Hyväksy olemassaolevat myöhästyneet ja/tai epäviralliset palautukset, jotka " +"tehtiin uuden henkilökohtaisen määräajan sisällä." + #: deviations/forms.py msgid "DEVIATION_MODULE_ADD_HELPTEXT" msgstr "" @@ -1530,6 +1536,12 @@ msgstr "Syötä joko ylimääräinen palautusaika tai uusi määräaika." msgid "LABEL_EXTRA_SUBMISSIONS" msgstr "Ylimääräisiä palautuskertoja" +#: deviations/forms.py +msgid "LABEL_APPROVE_UNOFFICIAL_SUBMISSIONS" +msgstr "" +"Hyväksy olemassaolevat epäviralliset palautukset, jotka tehtiin uuden " +"henkilökohtaisen palautuskertarajan sisällä." + #: deviations/forms.py msgid "DEVIATION_MODULE_REMOVE_HELPTEXT" msgstr "" From e42030e9e3719564a41208d7ed90c41ffff7d74a Mon Sep 17 00:00:00 2001 From: Markku Riekkinen Date: Fri, 6 May 2022 16:22:42 +0300 Subject: [PATCH 2/2] WIP clarify TODOs --- deviations/viewbase.py | 47 +++++++++++------------------------------- 1 file changed, 12 insertions(+), 35 deletions(-) diff --git a/deviations/viewbase.py b/deviations/viewbase.py index 276cbea6a..9a04d0933 100644 --- a/deviations/viewbase.py +++ b/deviations/viewbase.py @@ -70,7 +70,8 @@ def form_valid(self, form: forms.BaseForm) -> HttpResponse: exercises, submitters, form.cleaned_data, - )#TODO + ) + #TODO success message to the user? return super().form_valid(form) @@ -165,13 +166,13 @@ def form_valid(self, form: forms.BaseForm) -> HttpResponse: new_deviation.save() del self.request.session[self.session_key] - #TODO approve late/unofficial submissions submission_count = self.approve_submissions( self.exercises, self.submitters, self.session_data, excluded_deviations, ) + #TODO success message to the user? return super().form_valid(form) def deserialize_session_data(self, session_data: Dict[str, Any]) -> Dict[str, Any]: @@ -402,29 +403,7 @@ def approve_unofficial_submissions( extra_submissions: int = 0, ) -> int: #TODO fix this function - # Fetch submissions that exceeded the max submissions limit of the exercise and have status UNOFFICIAL. - # submissions = (Submission.objects - # .exclude_errors() - # .defer_text_fields() - # .filter( - # exercise__in=exercises, - # submitters__in=submitters, - # ) - # .annotate(rank=models.Window(#Window disallowed in filter() - # DenseRank(), - # partition_by=[models.F('exercise'), models.F('submitters')], - # order_by=models.F('submission_time').asc(), - # )) - # .filter( - # status=Submission.STATUS.UNOFFICIAL, - # rank__gt=models.F('exercise__max_submissions'), - # rank__lte=models.F('exercise__max_submissions') + extra_submissions, - # #exercise__in=exercises, - # #submitters__in=submitters, - # ) - # .order_by('exercise', 'submitters', 'submission_time') - # ) - + # Find exercises in which students have exceeded the max submissions limit. submission_counts = (Submission.objects .filter( exercise__in=exercises, @@ -444,29 +423,27 @@ def approve_unofficial_submissions( for c in submission_counts: if c['count'] > c['max_submissions']: exercise_submitter_pairs.add((c['exercise'], c['submitters'])) - # fetch submissions for exercise_submitter_pairs and check if some submissions should be approved - # (over max submissions and within extra submissions, status unofficial) + # Fetch submissions for exercise_submitter_pairs and check if some submissions should be approved + # (over max submissions and within extra submissions, status unofficial). + #TODO submissions = (Submission.objects .exclude_errors() .defer_text_fields() .filter( - exercise__in=exercises, + exercise__in=exercises,#TODO use exercise_submitter_pairs + # should there be a separate query for each pair since we don't want to query unused pairs? + # not all students have exceeding submissions in each exercise. submitters__in=submitters, ) .order_by('exercise', 'submitters', 'submission_time') ) - #TODO itertools groupby ?? + submission_count = 0 for submission in submissions: - # groupby? get one student's submissions in one exercise + #TODO # count the number of submissions and approve unofficial submissions above the exercise.max_submissions # (extra_submissions define how many are approved) - submission_count += 1 - - ######################################### old below - submission_count = 0 - for submission in submissions: may_approve = True for submitter in submission.submitters.all(): if excluded_deviations and (submitter.id, submission.exercise_id) in excluded_deviations: