Skip to content

Commit

Permalink
Fix check_submission_allowed() message severity
Browse files Browse the repository at this point in the history 
Fixes #1226

Co-authored-by: Jimmy Ihalainen <jimmy.ihalainen@gmail.com>
  • Loading branch information
@khattam2 @ihalaij1
khattam2 and ihalaij1 committed Apr 11, 2024
1 parent b31e686 commit 8f265f5
Show file tree
Hide file tree
Showing 7 changed files with 130 additions and 94 deletions.
9 changes: 6 additions & 3 deletions exercise/api/views.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,9 @@ def grader_detail(self, request, *args, **kwargs):
)

# find out if student can submit new exercise and if ok create submission template
status, errors, students = self.exercise.check_submission_allowed(student)
status, warnings, students = self.exercise.check_submission_allowed(student)
errors = warnings['error_messages'] + warnings['warning_messages']

if status != self.exercise.SUBMIT_STATUS.ALLOWED:
return Response({'success': False, 'errors': errors})
submission = Submission.objects.create(exercise=self.exercise)
Expand Down Expand Up @@ -279,7 +281,7 @@ def submit(self, request, *args, **kwargs):
data = None
status_code = None
headers = None
submission_status, issues, students = (
submission_status, warnings, students = (
self.exercise.check_submission_allowed(request.user.userprofile, request)
)
if submission_status == self.exercise.SUBMIT_STATUS.ALLOWED:
Expand Down Expand Up @@ -315,7 +317,8 @@ def submit(self, request, *args, **kwargs):
if page.errors:
data = {'errors': page.errors}
else:
data = {'errors': issues}
errors = warnings['error_messages'] + warnings['warning_messages']
data = {'errors': errors}
status_code = status.HTTP_400_BAD_REQUEST

return Response(data, status=status_code, headers=headers)
Expand Down
128 changes: 78 additions & 50 deletions exercise/exercise_models.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -748,49 +748,60 @@ def one_has_access(self, students, when=None):
Checks if any of the users can submit taking the granted extra time
in consideration.
"""
warnings = {
'error_messages': [],
'warning_messages': [],
'info_messages': []
}
timing,d = self.get_timing(students, when or timezone.now())

formatted_time = date_format(timezone.localtime(d), "DATETIME_FORMAT")
if timing == self.TIMING.OPEN:
return True,[]
return True, warnings
if timing == self.TIMING.LATE:
return True,[
warnings['warning_messages'].append(
format_lazy(
# xgettext:no-python-format
_('EXERCISE_TIMING_LATE -- {date}, {percent:d}'),
date=formatted_time,
percent=self.course_module.get_late_submission_point_worth(),
)
]
)
return True, warnings
if timing == self.TIMING.UNOFFICIAL:
return True,[
warnings['warning_messages'].append(
format_lazy(
_('EXERCISE_TIMING_UNOFFICIAL -- {date}'),
date=formatted_time,
)
]
)
return True, warnings
if timing == self.TIMING.CLOSED_BEFORE:
return False,[
warnings['warning_messages'].append(
format_lazy(
_('EXERCISE_TIMING_CLOSED_BEFORE -- {date}'),
date=formatted_time,
)
]
)
return False, warnings
if timing == self.TIMING.CLOSED_AFTER:
return False,[
warnings['warning_messages'].append(
format_lazy(
_('EXERCISE_TIMING_CLOSED_AFTER -- {date}'),
date=formatted_time,
)
]
)
return False, warnings
if timing == self.TIMING.ARCHIVED:
return False,[
warnings['error_messages'].append(
format_lazy(
_('EXERCISE_TIMING_ARCHIVED -- {date}'),
date=formatted_time,
)
]
return False,["ERROR"]
)
return False, warnings
warnings['error_messages'].append(_('SOMETHING_WENT_WRONG'))
return False, warnings

def one_has_deadline_deviation(self, students):
deviation = None
Expand Down Expand Up @@ -825,31 +836,39 @@ def max_submissions_for_student(self, user_profile):
return self.max_submissions

def one_has_submissions(self, students: List[UserProfile]) -> Tuple[bool, List[str]]:
warnings = {
'error_messages': [],
'warning_messages': [],
'info_messages': []
}

if len(students) == 1 and self.status in (self.STATUS.ENROLLMENT, self.STATUS.ENROLLMENT_EXTERNAL):
enrollment = self.course_instance.get_enrollment_for(students[0].user)
if not enrollment or enrollment.status != Enrollment.ENROLLMENT_STATUS.ACTIVE:
return True, []
return True, warnings
submission_count = 0
for profile in students:
# The students are in the same group, therefore, each student should
# have the same submission count. However, max submission deviation
# may be set for only one group member.
submission_count = self.get_submissions_for_student(profile, True, True).count()
if submission_count < self.max_submissions_for_student(profile):
return True, []
return True, warnings
# Even in situations where the student could otherwise make an infinite
# number of submissions, there is still a hard limit.
max_unofficial_submissions = settings.MAX_UNOFFICIAL_SUBMISSIONS
if max_unofficial_submissions == 0 or submission_count < max_unofficial_submissions:
if self.max_submissions == 0:
# If max_submissions is 0, the submission limit is "infinite".
return True, []
return True, warnings
if self.category.accept_unofficial_submits:
# Note: time is not checked here, but unofficial submissions are
# not allowed if the course archive time has passed.
# The caller must check the time limits too.
return True, [_('EXERCISE_MAX_SUBMISSIONS_USED_UNOFFICIAL_ALLOWED')]
return False, [_('EXERCISE_MAX_SUBMISSIONS_USED')]
warnings['warning_messages'].append(_('EXERCISE_MAX_SUBMISSIONS_USED_UNOFFICIAL_ALLOWED'))
return True, warnings
warnings['error_messages'].append(_('EXERCISE_MAX_SUBMISSIONS_USED'))
return False, warnings

def no_submissions_left(self, students):
if len(students) == 1 and self.status in (self.STATUS.ENROLLMENT, self.STATUS.ENROLLMENT_EXTERNAL):
Expand All @@ -872,15 +891,19 @@ def check_submission_allowed(self, profile, request=None):
@return: (success_flag, warning_message_list)
"""
success, warnings, students = self._check_submission_allowed(profile, request)
return success, list(str(w) for w in warnings), students
return success, warnings, students

def _check_submission_allowed(self, profile, request=None): # noqa: MC0001
students = [profile]
warnings = []
warnings = {
'error_messages' : [],
'warning_messages': [],
'info_messages': [],
}

# Let course module settings decide submissionable state.
#if self.course_instance.ending_time < timezone.now():
# warnings.append(_('The course is archived. Exercises are offline.'))
# warnings['warning_messages'].append(_('The course is archived. Exercises are offline.'))
# return False, warnings, students

# Check enrollment requirements.
Expand All @@ -890,21 +913,17 @@ def _check_submission_allowed(self, profile, request=None): # noqa: MC0001
LearningObject.STATUS.ENROLLMENT_EXTERNAL,
):
if not self.course_instance.is_enrollment_open():
return (self.SUBMIT_STATUS.CANNOT_ENROLL,
[_('ENROLLMENT_ERROR_ENROLLMENT_NOT_OPEN')],
students)
warnings['error_messages'].append(_('ENROLLMENT_ERROR_ENROLLMENT_NOT_OPEN'))
return self.SUBMIT_STATUS.CANNOT_ENROLL, warnings, students
if not self.course_instance.is_enrollable(profile.user):
return (self.SUBMIT_STATUS.CANNOT_ENROLL,
[_('CANNOT_ENROLL_IN_COURSE')],
students)
warnings['error_messages'].append(_('CANNOT_ENROLL_IN_COURSE_ERROR'))
return self.SUBMIT_STATUS.CANNOT_ENROLL, warnings, students
elif not enrollment or enrollment.status != Enrollment.ENROLLMENT_STATUS.ACTIVE:
if self.course_instance.is_course_staff(profile.user):
return (self.SUBMIT_STATUS.ALLOWED,
[_('STAFF_CAN_SUBMIT_WITHOUT_ENROLLING')],
students)
return (self.SUBMIT_STATUS.NOT_ENROLLED,
[_('MUST_ENROLL_TO_SUBMIT_EXERCISES')],
students)
warnings['info_messages'].append(_('STAFF_CAN_SUBMIT_WITHOUT_ENROLLING'))
return self.SUBMIT_STATUS.ALLOWED, warnings, students
warnings['error_messages'].append(_('MUST_ENROLL_TO_SUBMIT_EXERCISES_ERROR'))
return self.SUBMIT_STATUS.NOT_ENROLLED, warnings, students

# Support group id from post or currently selected group.
group = None
Expand All @@ -913,7 +932,7 @@ def _check_submission_allowed(self, profile, request=None): # noqa: MC0001
try:
group_id = json.loads(request.POST.get('__aplus__', '{}')).get('group')
except json.JSONDecodeError:
warnings.append(_('EXERCISE_WARNING_CANNOT_SUBMIT_INVALID_JSON_IN_POST'))
warnings['error_messages'].append(_('EXERCISE_ERROR_CANNOT_SUBMIT_INVALID_JSON_IN_POST'))
return self.SUBMIT_STATUS.INVALID, warnings, students
if group_id is None:
group_id = request.POST.get("_aplus_group")
Expand All @@ -926,7 +945,7 @@ def _check_submission_allowed(self, profile, request=None): # noqa: MC0001
course_instance=self.course_instance,
id=gid).first()
if group is None:
warnings.append(_('EXERCISE_WARNING_NO_GROUP_WITH_ID'))
warnings['error_messages'].append(_('EXERCISE_ERROR_NO_GROUP_WITH_ID'))
return self.SUBMIT_STATUS.INVALID_GROUP, warnings, students
except ValueError:
pass
Expand All @@ -938,22 +957,22 @@ def _check_submission_allowed(self, profile, request=None): # noqa: MC0001
submission = self.get_submissions_for_student(profile).first()
if submission:
if self._detect_group_changes(profile, group, submission):
msg = _('EXERCISE_WARNING_GROUP_CANNOT_CHANGE_FOR_SAME_EXERCISE_MSG')
warning = _('EXERCISE_WARNING_HAS_PREVIOUSLY_SUBMITTED_EXERCISE -- {with_group}, {msg}')
msg = _('EXERCISE_ERROR_GROUP_CANNOT_CHANGE_FOR_SAME_EXERCISE_MSG')
error_msg = _('EXERCISE_ERROR_HAS_PREVIOUSLY_SUBMITTED_EXERCISE -- {with_group}, {msg}')
if submission.submitters.count() == 1:
warning = format_lazy(warning, with_group=_('ALONE'), msg=msg)
error_msg = format_lazy(error_msg, with_group=_('ALONE'), msg=msg)
else:
collaborators = StudentGroup.format_collaborator_names(
submission.submitters.all(), profile)
with_group = format_lazy(_('WITH -- {}'), collaborators)
warning = format_lazy(warning, with_group=with_group, msg=msg)
warnings.append(warning)
error_msg = format_lazy(error_msg, with_group=with_group, msg=msg)
warnings['error_messages'].append(error_msg)
return self.SUBMIT_STATUS.INVALID_GROUP, warnings, students

elif self._detect_submissions(profile, group):
warnings.append(
warnings['error_messages'].append(
format_lazy(
_('EXERCISE_WARNING_COLLABS_HAVE_SUBMITTED_EXERCISE_WITH_DIFF_GROUP -- {collaborators}'),
_('EXERCISE_ERROR_COLLABS_HAVE_SUBMITTED_EXERCISE_WITH_DIFF_GROUP -- {collaborators}'),
collaborators=group.collaborator_names(profile),
)
)
Expand All @@ -969,23 +988,27 @@ def _check_submission_allowed(self, profile, request=None): # noqa: MC0001
size = "{:d}".format(self.min_group_size)
else:
size = "{:d}-{:d}".format(self.min_group_size, self.max_group_size)
warnings.append(
warnings['error_messages'].append(
format_lazy(
_('EXERCISE_WARNING_REQUIRES_GROUP_SIZE -- {size}'),
_('EXERCISE_ERROR_REQUIRES_GROUP_SIZE -- {size}'),
size=size
)
)
if self.status in (self.STATUS.ENROLLMENT, self.STATUS.ENROLLMENT_EXTERNAL):
access_ok, access_warnings = True, []
access_ok, access_warnings = True, {'error_messages': [], 'warning_messages': [], 'info_messages': []}
else:
access_ok, access_warnings = self.one_has_access(students)
is_staff = all(self.course_instance.is_course_staff(p.user) for p in students)
ok = (access_ok and len(warnings) == 0) or is_staff # pylint: disable=consider-using-ternary
all_warnings = warnings + access_warnings
# pylint: disable-next=consider-using-ternary
ok = (access_ok and len(warnings['error_messages'] + warnings['warning_messages']) == 0) or is_staff
# Combine warnings dict with access_warnings dict
all_warnings = {
key: warnings.get(key, []) + access_warnings.get(key, [])
for key in set(warnings) | set(access_warnings)
}
if not ok:
if len(all_warnings) == 0:
all_warnings.append(_(
'EXERCISE_WARNING_CANNOT_SUBMIT_UNKNOWN_REASON'))
if len(all_warnings['error_messages'] + all_warnings['warning_messages']) == 0:
all_warnings['error_messages'].append(_('EXERCISE_ERROR_CANNOT_SUBMIT_UNKNOWN_REASON'))
return self.SUBMIT_STATUS.INVALID, all_warnings, students

submit_limit_ok, submit_limit_warnings = self.one_has_submissions(students)
Expand All @@ -995,7 +1018,12 @@ def _check_submission_allowed(self, profile, request=None): # noqa: MC0001
submit_limit_warnings,
students)

return self.SUBMIT_STATUS.ALLOWED, all_warnings + submit_limit_warnings, students
# Combine all_warnings dict with submit_limit_warnings dict
all_warnings = {
key: all_warnings.get(key, []) + submit_limit_warnings.get(key, [])
for key in set(all_warnings) | set(submit_limit_warnings)
}
return self.SUBMIT_STATUS.ALLOWED, all_warnings, students

def _detect_group_changes(self, profile, group, submission):
submitters = list(submission.submitters.all())
Expand Down
2 changes: 1 addition & 1 deletion exercise/tests.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,7 @@ def test_base_exercise_submission_allowed(self):
status, errors, _students = (
self.base_exercise.check_submission_allowed(self.user.userprofile))
self.assertNotEqual(status, self.base_exercise.SUBMIT_STATUS.ALLOWED)
self.assertEqual(len(errors), 1)
self.assertEqual(len(errors['error_messages'] + errors['warning_messages'] + errors['info_messages']), 1)
json.dumps(errors)
self.assertNotEqual(
self.static_exercise.check_submission_allowed(self.user.userprofile)[0],
Expand Down
22 changes: 14 additions & 8 deletions exercise/views.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ def post(self, request: HttpRequest, *args: Any, **kwargs: Any) -> HttpResponse:
new_submission = None
page = ExercisePage(self.exercise)
_submission_status, submission_allowed, _issues, students = (
self.submission_check(True, request)
self.submission_check(request)
)
if submission_allowed:
try:
Expand Down Expand Up @@ -212,22 +212,28 @@ def post(self, request: HttpRequest, *args: Any, **kwargs: Any) -> HttpResponse:
return self.render_to_response(self.get_context_data(
page=page, students=students, submission=new_submission))

def submission_check(self, error=False, request=None):
def submission_check(self, request=None):
if self.exercise.grading_mode == BaseExercise.GRADING_MODE.LAST:
# Add warning about the grading mode.
messages.warning(self.request, _('ONLY_YOUR_LAST_SUBMISSION_WILL_BE_GRADED'))
if not self.profile:
issue = _('SUBMISSION_MUST_SIGN_IN_AND_ENROLL_TO_SUBMIT_EXERCISES')
messages.error(self.request, issue)
return self.exercise.SUBMIT_STATUS.INVALID, False, [issue], []
submission_status, issues, students = (
submission_status, warnings, students = (
self.exercise.check_submission_allowed(self.profile, request)
)
if len(issues) > 0:
if error:
messages.error(self.request, "\n".join(issues))
else:
messages.warning(self.request, "\n".join(issues))

issues = []
for msg in warnings['error_messages']:
issues.append(msg)
messages.error(self.request, msg)
for msg in warnings['warning_messages']:
issues.append(msg)
messages.warning(self.request, msg)
for msg in warnings['info_messages']:
messages.info(self.request, msg)

submission_allowed = (
submission_status == self.exercise.SUBMIT_STATUS.ALLOWED
)
Expand Down
3 changes: 2 additions & 1 deletion external_services/api/views.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ def post(self, request, version=None): # version is the API version parameter fr
if serializer.validated_data['req_type'] == LTIOutcomeXMLParser.TYPE_REPLACE:
exercise = serializer.validated_data['exercise']
student = serializer.validated_data['submitter']
sbms_status, errors, _students = exercise.check_submission_allowed(student)
sbms_status, warnings, _students = exercise.check_submission_allowed(student)
errors = warnings['error_messages'] + warnings['warning_messages']
if sbms_status != exercise.SUBMIT_STATUS.ALLOWED:
return Response({'detail': errors}, status=status.HTTP_200_OK)
# the key "detail" is also used by raised exceptions in authentication and parsing
Expand Down
Loading

0 comments on commit 8f265f5

Please sign in to comment.