Non-counting questions by jooooosef · Pull Request #2450 · e-valuation/EvaP

jooooosef · 2025-05-26T19:35:15Z

closes #2391
fixes #2539

TODO:

add tests

richardebeling

Looking pretty good, thanks :)

niklasmohrin

okay some more concerns; this is not an easy issue :^)

niklasmohrin

great stuff, some clean up work but I think we are close to done :)

richardebeling · 2026-06-08T19:13:52Z

The changes in this file seem to be 100% about "allows additional text answers", not about "counts_for_grade". Is this intended, or just a rebase f-up?

Yes, it is now only about the allows additional text answers, since we moved counts_for_grade into the QuestionAssignment (I think I originally wrote this). The first part (with test_locked_contributor_questionnaire) seems to be from the rebase.
But the part with the update_fields from the second test is new, I cant find anything testing that. So I think I would like to keep this part, I have removed the rest.

matches the behaviour from the allows_additional_text_answer in QuestionForm

niklasmohrin

Very cool! I am pretty happy with the state of this and with what I was reading, good job. I have a handful of mostly nits, except for the typescript code which I think can use another touch.

I believe that our current plan is to make another release soon with the fixes for the recently discovered bugs, and I think it makes sense to hold this PR back until then, but after that I don't see anything in the way of merging.

niklasmohrin · 2026-06-24T21:11:18Z

+            self.assertEqual(
+                additional_textanswers,
+                row.find_element(By.CSS_SELECTOR, "input[id$='-allows_additional_textanswers']").is_enabled(),
+            )
+            self.assertEqual(
+                counts_for_grade,
+                row.find_element(By.CSS_SELECTOR, "input[id$='-counts_for_grade']").is_enabled(),
+            )


nit: we usually put the expected value second

Suggested change

self.assertEqual(

additional_textanswers,

row.find_element(By.CSS_SELECTOR, "input[id$='-allows_additional_textanswers']").is_enabled(),

)

self.assertEqual(

counts_for_grade,

row.find_element(By.CSS_SELECTOR, "input[id$='-counts_for_grade']").is_enabled(),

)

self.assertEqual(

row.find_element(By.CSS_SELECTOR, "input[id$='-allows_additional_textanswers']").is_enabled(),

additional_textanswers,

)

self.assertEqual(

row.find_element(By.CSS_SELECTOR, "input[id$='-counts_for_grade']").is_enabled(),

counts_for_grade,

)

niklasmohrin · 2026-06-24T21:15:08Z

+        new_row = self.selenium.find_elements(By.CSS_SELECTOR, "#question_table tbody tr")[
+            -2
+        ]  # the last row is the add row button


Suggested change

new_row = self.selenium.find_elements(By.CSS_SELECTOR, "#question_table tbody tr")[

-2

] # the last row is the add row button

new_row = self.selenium.find_elements(By.CSS_SELECTOR, "#question_table tbody tr")[1]

niklasmohrin · 2026-06-24T21:18:51Z

niklasmohrin · 2026-06-24T21:37:33Z

+
+            const staffQuestionnaireForm = new StaffQuestionnaireForm(document.getElementById("question_table"));
+


there is trailing whitespace on the empty lines here

niklasmohrin · 2026-06-24T21:41:18Z

+        this.questionTable.addEventListener("change", this.handleQuestionTypeChange);
+        this.questionnaireTypeSelect.addEventListener("change", this.handleQuestionnaireTypeChange);
+
+        this.initialize();


I would either inline this method, or not call it here and instead let the caller run this. We have this semi pattern, for example in the contact modal, that constructor just queries the DOM and a second method, in this case attach, must be called to activate the component. This would also include the addEventListener` above.

I don't have a strong opinion, we can do everything in the constructor (without the extra method), or we move the event listeners to initialize and rename this accordingly and let the caller call this second function too.

niklasmohrin · 2026-06-24T22:17:06Z

+                assert not any(
+                    isinstance(result, RatingResult) and result.counts_for_grade
+                    for result in questionnaire_result.question_results
+                )


If this ever fails, we will be happy to have added a message like

Suggested change

)

), f"Dropout questionnaire {questionnaire_result.questionnaire.id} has results that count"

niklasmohrin · 2026-06-24T22:17:33Z

        for questionnaire_result in contribution_result.questionnaire_results:
-            if not questionnaire_result.questionnaire.is_dropout:  # dropout questionnaires are not counted
-                grouped_results[contribution_result.contributor].extend(questionnaire_result.question_results)
+            if questionnaire_result.questionnaire.is_dropout:  # dropout questionnaires are not counted


Suggested change

if questionnaire_result.questionnaire.is_dropout: # dropout questionnaires are not counted

if questionnaire_result.questionnaire.is_dropout:

niklasmohrin · 2026-06-24T22:18:00Z

                    ]
                ),
+                # The weight of this contributors grade is supposed to represent the number of students the
+                # contributor interacted with, which we derive from the max answer count, independently of counts_for_grades.


Suggested change

# contributor interacted with, which we derive from the max answer count, independently of counts_for_grades.

# contributor interacted with, which we derive from the max answer count, independently of counts_for_grade.

niklasmohrin · 2026-06-24T22:23:01Z

-        calculated_grade = distribution_to_grade(calculate_average_distribution(self.evaluation))
-        self.assertAlmostEqual(calculated_grade, 1.5)


Can you add the removed lines here back to the existing test?

niklasmohrin · 2026-06-24T22:32:26Z

+        if (questionnaireType === QUESTIONNAIRE_TYPE_DROPOUT) {
+            checkboxes.forEach(checkbox => {
+                if (checkbox.classList.contains("counts-for-grade-checkbox")) {
+                    this.disableAndUncheck(checkbox);


(out of scope for this PR)

@janno42 It would probably be a nice feature to add a tooltip to the disabled checkboxes that display the reason for why the checkbox is disabled ("Questions of a dropout questionnaire cannot count towards the evaluation's grade" or so)). I think this is slightly more complicated than "very small" or maybe even "small" because this typescript logic here has to interact with bootstrap tooltips and changing labels, and the knowledge of when things are disabled leaks out of this file, but it should be doable.

jooooosef marked this pull request as draft May 26, 2025 19:35

jooooosef force-pushed the iss2391 branch from 893abd4 to 82c4659 Compare June 16, 2025 16:30

richardebeling reviewed Jun 16, 2025

View reviewed changes

Comment thread evap/staff/templates/staff_questionnaire_form.html Outdated

Comment thread evap/student/templates/student_vote_questionnaire_group.html Outdated

jooooosef marked this pull request as ready for review June 17, 2025 09:11

jooooosef requested a review from richardebeling June 23, 2025 18:40

richardebeling reviewed Jun 30, 2025

View reviewed changes

Comment thread evap/staff/templates/staff_questionnaire_form.html Outdated

Comment thread evap/evaluation/tests/test_models.py Outdated

jooooosef force-pushed the iss2391 branch from e4ada51 to 230869a Compare July 14, 2025 18:40

niklasmohrin reviewed Jul 14, 2025

View reviewed changes

jooooosef force-pushed the iss2391 branch 2 times, most recently from 7386f67 to 6b24166 Compare July 21, 2025 19:59

jooooosef mentioned this pull request Jul 21, 2025

Do not implicitly set values in clean function #2482

Open

jooooosef requested review from niklasmohrin and richardebeling July 21, 2025 20:05

niklasmohrin requested changes Jul 28, 2025

View reviewed changes

richardebeling removed their request for review August 4, 2025 15:27

jooooosef force-pushed the iss2391 branch 2 times, most recently from 3eb3935 to 3f26948 Compare August 11, 2025 20:38

janno42 force-pushed the main branch 2 times, most recently from 73e348c to 3577b6a Compare August 12, 2025 11:21

jooooosef force-pushed the iss2391 branch from cafc398 to c62e873 Compare August 18, 2025 16:33

niklasmohrin reviewed Aug 23, 2025

View reviewed changes

jooooosef force-pushed the iss2391 branch from cf9ef56 to 65403d2 Compare October 20, 2025 20:05

niklasmohrin mentioned this pull request Oct 27, 2025

Cannot change question type and allows_additional_textanswers of text/heading questions simultaneously #2539

Closed

Kakadus linked an issue Oct 27, 2025 that may be closed by this pull request

Cannot change question type and allows_additional_textanswers of text/heading questions simultaneously #2539

Closed

Kakadus reviewed Oct 27, 2025

View reviewed changes

Comment thread evap/staff/forms.py Outdated

Kakadus mentioned this pull request Nov 4, 2025

Separate questions and questionnaires #2513

Merged

richardebeling requested changes Nov 4, 2025

View reviewed changes

jooooosef force-pushed the iss2391 branch 2 times, most recently from 77f11b9 to d360f52 Compare November 24, 2025 22:33

jooooosef requested a review from janno42 December 1, 2025 16:40

niklasmohrin reviewed May 18, 2026

View reviewed changes

richardebeling removed their request for review May 25, 2026 12:17

janno42 removed their request for review June 1, 2026 15:38

jooooosef requested review from niklasmohrin and richardebeling June 1, 2026 20:29

richardebeling reviewed Jun 8, 2026

View reviewed changes

jooooosef added 19 commits June 22, 2026 18:13

Add non-counting questions to grade calculation

07a1f64

Expose questionnaire checkbox controls

82b80d2

fix model and migration for counts_for_grade

32d62ba

fix the grade calculation logic to use counts_for_grade

9196ab3

fix frontend and logic to use counts_for_grade from question assignment

6b7a8d9

fix checkbox disabled bug and add tests

0807084

use assignment instead of question for student frontend

2d05eed

change question to question_assignment when needed in tests

8ef8b79

already disbale the counts_for_grade checkbox before rendering

3cee189

matches the behaviour from the allows_additional_text_answer in QuestionForm

fix tests

dd73b36

fix question assignment creation in frontendtests

66cf9ee

update test_data

c62d4c7

update minimal test_data

97c4376

some typing and cleaner code

bd52175

linter unhappy

93d627a

frontend things made better

b15b3cb

fix tests

88212b4

unnessessary typecast

9cd582a

remove double testing

0a993de

jooooosef force-pushed the iss2391 branch from 5ac3c4c to 0a993de Compare June 22, 2026 17:49

jooooosef requested review from janno42 and richardebeling June 22, 2026 17:49

fix eslint

5fce07d

niklasmohrin reviewed Jun 24, 2026

View reviewed changes


		const staffQuestionnaireForm = new StaffQuestionnaireForm(document.getElementById("question_table"));

	)
	), f"Dropout questionnaire {questionnaire_result.questionnaire.id} has results that count"

	if questionnaire_result.questionnaire.is_dropout: # dropout questionnaires are not counted
	if questionnaire_result.questionnaire.is_dropout:

	# contributor interacted with, which we derive from the max answer count, independently of counts_for_grades.
	# contributor interacted with, which we derive from the max answer count, independently of counts_for_grade.

		calculated_grade = distribution_to_grade(calculate_average_distribution(self.evaluation))
		self.assertAlmostEqual(calculated_grade, 1.5)

Uh oh!

Conversation

jooooosef commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

richardebeling left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

niklasmohrin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

niklasmohrin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niklasmohrin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jooooosef commented May 26, 2025 •

edited

Loading