-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Display personal deadline extensions on a student’s points page #1216
Display personal deadline extensions on a student’s points page #1216
Conversation
53c24d5
to
2f81e60
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I have just one question.
Is using deepcopy
really necessary? Does the code not work without it?
Also, the commit message should include Fixes #1119 on separate line, with an additional empty line above it.
2f81e60
to
7387e59
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready for merge!
Fixes apluslms#1119 Co-authored-by: Markku Riekkinen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I will make minor changes myself to improve accessibility and then merge this.
data-toggle="tooltip" | ||
title="{% translate 'PERSONAL_DEADLINE_DEVIATION' %} {{ entry.personal_deadline }}" | ||
> | ||
<span class="glyphicon glyphicon-time"></span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decorative icons should have the aria-hidden="true"
attribute so that they do not confuse screen readers.
<span class="glyphicon glyphicon-time"></span> | |
<span class="glyphicon glyphicon-time" aria-hidden="true"></span> |
https://getbootstrap.com/docs/3.4/components/#glyphicons-how-to-use
{% if entry|deadline_extended_exercise_open:now %} | ||
<span | ||
data-toggle="tooltip" | ||
title="{% translate 'PERSONAL_DEADLINE_DEVIATION' %} {{ entry.personal_deadline }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concatenating the date manually after the translated string may break some languages. If some language requires that the date is written at the start or in the middle of the sentence, then it is not possible to move the date's position here. You would need to use blocktranslate
so that you could embed the date variable within the translated string. Then, each language version may freely use the date variable in the correct position inside the sentence.
@@ -113,6 +118,14 @@ <h3 class="panel-title"> | |||
<td> | |||
{% if exercise_accessible or is_course_staff %} | |||
<a href="{{ entry.link }}" class="{% if entry|is_in_maintenance %}maintenance{% endif %}">{{ entry.name|parse_localization }}</a> | |||
{% if entry|deadline_extended_exercise_open:now %} | |||
<span |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is not following accessibility guidelines. Screen readers need text (in the DOM, not only in the tooltip) for reading out aloud. The screen reader version of the description can be hidden with the class sr-only
.
|
||
@register.filter | ||
def deadline_extended_exercises_open(entry, now): | ||
entries = deepcopy(entry['flatted']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When all modules are rendered in the web page, this function and the deepcopy are called once for each module. It would be nice to avoid the deepcopy. The old cache class (CachedPoints
, CachedContent
etc.) looks tricky so modifying it or its iterators is maybe not recommended. I can't think of any other solution than to convert the iterator to a normal list when it is originally created in ContentMixin.flat_module()
. I'm not sure if that is really a good idea.
a-plus/exercise/cache/hierarchy.py
Line 30 in eb8933a
class NextIterator(HierarchyIterator): |
a-plus/exercise/cache/hierarchy.py
Lines 123 to 127 in eb8933a
def flat_module(self, module, enclosed=True): | |
modules = self.modules() | |
idx = self._model_idx(module) | |
tree = self._by_idx(modules, idx) | |
return NextIterator(tree[0]['children'], enclosed=enclosed) |
a-plus/exercise/cache/hierarchy.py
Lines 113 to 116 in eb8933a
def modules_flatted(self): | |
for module in self.data['modules']: | |
module['flatted'] = self.flat_module(module) | |
return self.data['modules'] |
a-plus/exercise/templatetags/exercise.py
Line 55 in eb8933a
'modules': points.modules_flatted(), |
a-plus/exercise/templatetags/exercise.py
Lines 73 to 87 in eb8933a
@register.inclusion_tag("exercise/_user_results.html", takes_context=True) | |
def user_results(context: Context, student: Optional[User] = None) -> Dict[str, Any]: | |
values = _get_toc(context, student) | |
values['total_json'] = json.dumps(values['total']) | |
if values['is_course_staff']: | |
instance = context['instance'] | |
values['student_count'] = instance.students.count() | |
counts = (instance.students | |
.filter(submissions__exercise__course_module__course_instance=instance) | |
.values('submissions__exercise_id') | |
.annotate(count=models.Count('submissions__submitters', distinct=True)) | |
.order_by() | |
) | |
values['exercise_submitter_counts'] = {row['submissions__exercise_id']: row['count'] for row in counts} | |
return values |
locale/en/LC_MESSAGES/django.po
Outdated
@@ -4051,6 +4055,10 @@ msgstr "There may be changes in the assignments before the module opens!" | |||
msgid "SUBMISSIONS" | |||
msgstr "Submissions" | |||
|
|||
#: exercise/templates/exercise/_user_results.html | |||
msgid "PERSONAL_DEADLINE_DEVIATION" | |||
msgstr "Personal deadline deviation" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should user-facing text be "deadline deviation" or "deadline extension"?
505ef61
to
f43f9cb
Compare
Description
What?
Displays deadline extensions in the points page with a tag in the module heading and a clock icon next to each exercise.
Why?
Students and staff members see deadline extensions more easily.
How?
Adds two custom template tags that check if an exercise/module has a personal deadline.
Fixes #1119
What type of test did you run?
The deadline deviations are only shown on the points page when the module is open and the student has time left to submit the exercise. Works for both module are exercise deviations. Both students and staff members see the deviations.
Did you test the changes in
Translation
Programming style
Template blocks are not indented.
Have you updated the README or other relevant documentation?
Is it Done?