Skip to content

Conversation

@hansegucker
Copy link
Collaborator

Close #2585

Copy link
Member

@richardebeling richardebeling left a comment

Choose a reason for hiding this comment

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

lgtm, but I think we shouldn't spend resources to remove the breadcrumb link

Comment on lines +7 to +11
{% if disable_breadcrumb_questionnaire %}
<li class="breadcrumb-item">{{ questionnaire.name }}</li>
{% else %}
<li class="breadcrumb-item"><a href="{% url 'staff:questionnaire_edit' questionnaire.id %}">{{ questionnaire.name }}</a></li>
{% endif %}
Copy link
Member

Choose a reason for hiding this comment

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

Why? I don't think the link is a problem, on the contrary, I think not having the link can only make it harder for people.

Not having the link requires additional logic in the template here, we need to add a new template argument, which we need to find a name for ("disable_breadcrumb_questionnaire" is rather misleading).

I'd say let's not go this way, just keep the link, or am I missing something?

"-course__semester__created_at", "vote_start_datetime"
)

evaluations_grouped_by_semester = unordered_groupby((e.course.semester, e) for e in evaluations)
Copy link
Member

Choose a reason for hiding this comment

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

N+1 queries pattern -- let's prefetch the semester above (should be relatively simple)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Questionnaire usage overview

2 participants