Skip to content

2604 preparation reminder for internal users #2688

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.

Sign up for GitHub

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

Open
ybrnr wants to merge 6 commits into
base: main
Choose a base branch
from
Open

2604 preparation reminder for internal users #2688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ea812b1
chore: add external responsible to course 92 in test data
ybrnr Feb 16, 2026
b0508fb
feat: add reminder for internal users only
ybrnr Feb 16, 2026
12bf155
fix: fix external responsible filtering
ybrnr Mar 30, 2026
51bcd33
test: add test for internal reminders
ybrnr Mar 30, 2026
03a99f0
fix: filter responsibles as queryset instead of as a list
ybrnr Mar 30, 2026
128b933
chore: restore test_data.json
ybrnr Mar 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 28 additions & 10 deletions evap/staff/templates/staff_semester_preparation_reminder.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,37 @@ <h3>
<div class="d-flex mb-3">
<div class="me-auto">
{% if responsible_list and interactive %}
<form reload-on-success method="POST" action="{% url 'staff:semester_preparation_reminder' semester_id=semester.id %}">
{% csrf_token %}
<div class="d-flex align-items-center gap-1">
<form reload-on-success method="POST" action="{% url 'staff:semester_preparation_reminder' semester_id=semester.id %}">
{% csrf_token %}

<input type="hidden" name="semester_id" value="{{ semester.id }}" />
<input type="hidden" name="semester_id" value="{{ semester.id }}" />

<confirmation-modal type="submit">
<span slot="title">{% translate 'Remind all' %}</span>
<span slot="action-text">{% translate 'Remind all' %}</span>
<span slot="question">{% translate 'Do you really want to remind everyone?' %}</span>
<confirmation-modal type="submit">
<span slot="title">{% translate 'Remind all' %}</span>
<span slot="action-text">{% translate 'Remind all' %}</span>
<span slot="question">{% translate 'Do you really want to remind everyone?' %}</span>

<button slot="show-button" type="button" id="remindAllButton" class="btn btn-sm btn-light">{% translate 'Remind all' %}</button>
</confirmation-modal>
</form>

<form reload-on-success method="POST" action="{% url 'staff:semester_preparation_reminder' semester_id=semester.id %}">
{% csrf_token %}

<input type="hidden" name="semester_id" value="{{ semester.id }}" />
<input type="hidden" name="internal_only" value="true">

<confirmation-modal type="submit">
<span slot="title">{% translate 'Remind all internal users' %}</span>
<span slot="action-text">{% translate 'Remind all internal users' %}</span>
<span slot="question">{% translate 'Do you really want to remind all internal users?' %}</span>

<button slot="show-button" type="button" id="remindAllInternalUsersButton" class="btn btn-sm btn-light">{% translate 'Remind all internal users' %}</button>

@richardebeling richardebeling Apr 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The id here, remindAllInternalUsersButton, is never used. Let's remove it.

While you're at it, the same is true for remindAllButton above.

</confirmation-modal>
</form>
</div>

<button slot="show-button" type="button" id="remindAllButton" class="btn btn-sm btn-light">{% translate 'Remind all' %}</button>
</confirmation-modal>
</form>
{% endif %}
</div>
<div class="btn-switch btn-switch-light ms-2 d-print-none h-">
Expand Down
21 changes: 21 additions & 0 deletions evap/staff/tests/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1064,6 +1064,27 @@ def test_remind_all(self, email_template_mock) -> None:
self.assertEqual(kwargs["body_params"], {"user": self.user, "evaluations": [self.evaluation]})
self.assertEqual(kwargs["use_cc"], True)

@patch("evap.staff.views.EmailTemplate")
def test_remind_all_internal_only(self, email_template_mock) -> None:
self.user.email = "responsible@institution.example.com"
self.user.save()

external_responsible = baker.make(UserProfile, email="external@external.example.com")
baker.make(
Evaluation,
course=baker.make(Course, semester=self.semester, responsibles=[external_responsible]),
state=Evaluation.State.PREPARED,
)

email_template_mock.objects.get.return_value = email_template_mock
email_template_mock.EDITOR_REVIEW_REMINDER = EmailTemplate.EDITOR_REVIEW_REMINDER

self.app.post(self.url, params={"internal_only": "true"}, user=self.manager, status=200)

recipients = [call[0][0] for call in email_template_mock.send_to_user.call_args_list]
self.assertIn(self.user, recipients)
self.assertNotIn(external_responsible, recipients)
Comment on lines +1079 to +1086

@richardebeling richardebeling Apr 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be less code to not patch the email templates and just check mail.outbox afterwards? If so, let's do that (otherwise, ignore).


def test_invalid_mode(self) -> None:
self.app.get(self.url, params={"mode": "invalid"}, user=self.manager, status=400)

Expand Down
5 changes: 4 additions & 1 deletion evap/staff/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -959,13 +959,16 @@ def semester_questionnaire_assign(request, semester_id):
@manager_required
def semester_preparation_reminder(request: HttpRequest, semester_id: int) -> HttpResponse:
semester = get_object_or_404(Semester, id=semester_id)
internal_only = request.POST.get("internal_only") == "true"

evaluations = semester.evaluations.filter(
state__in=[Evaluation.State.PREPARED, Evaluation.State.EDITOR_APPROVED]
).prefetch_related("course__programs")

prepared_evaluations = semester.evaluations.filter(state=Evaluation.State.PREPARED)
responsibles = UserProfile.objects.filter(courses_responsible_for__evaluations__in=prepared_evaluations).distinct()
if internal_only:
responsibles = responsibles.exclude(pk__in=[r.pk for r in responsibles if r.is_external])
Comment on lines +970 to +971

@richardebeling richardebeling Apr 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_external is implemented in python logic, so django cannot convert it into a SQL query. That means we don't "win" anything by adding this filter to the query early. Note that the code here currently fetches responsibles, computes is_external for each of them, and then amends the original query with "where id is not in [list of external IDs]" -- this is just one extra database round trip without real benefit.

Above, you access request.POST, but outside the if request.method == 'POST' block below.

I think for maintainability, it makes sense to put this check inside this block. That way, we keep the queries simple, save one database roundtrip, don't build large SQL, and the world is greener.

The code change would then just be

if request.POST.get("internal_only") == "true" and responsible.is_external:
    continue


responsible_list = [
(
Expand All @@ -981,7 +984,7 @@ def semester_preparation_reminder(request: HttpRequest, semester_id: int) -> Htt
for responsible, evaluations, __ in responsible_list:
body_params = {"user": responsible, "evaluations": evaluations}
template.send_to_user(responsible, subject_params={}, body_params=body_params, use_cc=True, request=request)
messages.success(request, _("Successfully sent reminders to everyone."))
messages.success(request, _("Successfully sent reminders."))
Comment on lines -984 to +987

@janno42 janno42 Apr 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two different success messages for the two cases would be nice

return HttpResponse()
mode = request.GET.get("mode", "interactive")
if mode not in ["interactive", "text"]:
Expand Down
Loading