reset evaluation after it started #2136

fekoch · 2024-03-05T17:54:36Z

Closes #1991
Replaces the revert_to_new evaluation operation with reset_to_new. It now allows resetting from any State except NEW and PUBLISHED. It also allows deleting all previously received answers.

Also, it makes Evaluation.State an IntEnum.

Only show confirmation modal when deleting previous answers
Finish Unit-Tests
Cleanup leftover TODO's

evap/staff/templates/staff_evaluation_operation.html

fekoch · 2024-04-22T19:29:40Z

Two Unit Tests are missing. I do not have any idea on how to practically test for ui popups without selenium tests

niklasmohrin

I started reviewing your changes and I find it a bit tricky to separate the "revert -> reset" and "make State an IntEnum" changes. Do you think you could split these changes into two PRs? If not, we will have to see how we manage :D

The extra PR would also be a good place to discuss the motivation for the move to IntEnum - it sounds like a good idea, but it's hard to see the benefits when they are mixed in with the other changes here

evap/evaluation/migrations/0143_alter_evaluation_state.py

niklasmohrin · 2024-04-24T19:00:29Z

I do not have any idea on how to practically test for ui popups without selenium tests

I don't think it is possible with our current setup. However, since the javascript deciding whether or not to show the modal is pretty clear, I think we don't urgently need a test for that

fekoch · 2024-06-10T17:37:39Z

@fekoch some TODO's:

type annotation in test helper function
remove empty newline

evap/evaluation/models.py

evap/staff/templates/staff_evaluation_operation.html

evap/evaluation/models.py

evap/staff/templates/staff_evaluation_operation.html

evap/staff/views.py

janno42

Forgot to specify before: It should not be possible to reset single results. They should be removed from the selection and the operation should not be applicable_to them.
Submitting other operations (e.g., start evaluation, publish, unpublish) is not possible anymore. The submit button does not send a request in these cases (document.getElementById("delete-previous-answers") fails).

…tions

richardebeling

thanks :)

niklasmohrin

Nice job, I have some final remarks below.

Since all reviews are green now, but you still may want to change something, just write a quick comment once you are okay with merging

niklasmohrin · 2024-08-30T20:09:33Z

evap/evaluation/models.py

+        allowed_sources = [
+            Evaluation.State.PREPARED,
+            Evaluation.State.EDITOR_APPROVED,
+            Evaluation.State.APPROVED,
+            Evaluation.State.IN_EVALUATION,
+            Evaluation.State.EVALUATED,
+            Evaluation.State.REVIEWED,
+        ]
+        return self.state in allowed_sources and not self.is_single_result


Any reason to prefer this over the following?

Suggested change

allowed_sources = [

Evaluation.State.PREPARED,

Evaluation.State.EDITOR_APPROVED,

Evaluation.State.APPROVED,

Evaluation.State.IN_EVALUATION,

Evaluation.State.EVALUATED,

Evaluation.State.REVIEWED,

]

return self.state in allowed_sources and not self.is_single_result

return Evaluation.State.NEW < self.state < Evaluation.State.PUBLISHED and not self.is_single_result

niklasmohrin · 2024-08-30T20:11:01Z

evap/evaluation/models.py

+    def reset_to_new(self, *, delete_previous_answers: bool):
+        if delete_previous_answers:
+            for answer_class in Answer.__subclasses__():
+                answer_class._default_manager.filter(contribution__evaluation_id=self.id).delete()


Does the following also work?

Suggested change

answer_class._default_manager.filter(contribution__evaluation_id=self.id).delete()

answer_class._default_manager.filter(contribution__evaluation=self).delete()

niklasmohrin · 2024-08-30T20:16:20Z

evap/staff/views.py

+        if request.POST.get("delete-previous-answers") == "on":
+            delete_previous_answers = True


Couldn't we do delete_previous_answers = request.POST.get("delete-previous-answers") == "on" from the start? I think this would also fix the bool | None "issue", although I don't care so much about that

niklasmohrin · 2024-08-30T20:25:58Z

evap/staff/templates/staff_evaluation_operation.html

+                    {% comment %}
+                        The modal needs an element in the slot, otherwise it throws an error.
+                        No button is needed, as the modal is conditionally shown via JS.
+                    {% endcomment %}
+                    <span slot="show-button"></span>


Good catch, we should change the modal so that this is not needed, you or me can fix it in a follow up :)

niklasmohrin · 2024-08-30T20:31:00Z

evap/staff/templates/staff_evaluation_operation.html

+                        const data = new FormData(submitEvent.target);
+                        if (data.get("delete-previous-answers") === "on" && !submitEvent.submitter?.hasAttribute("data-confirm-answer-deletion")) {
+                            submitEvent.preventDefault()
+                            submitEvent.target.querySelector("confirmation-modal[data-allow-answer-deletion]").dialog.showModal();


With the following, we can get rid of the data-allow-answer-deletion on the modal:

Suggested change

submitEvent.target.querySelector("confirmation-modal[data-allow-answer-deletion]").dialog.showModal();

submitEvent.target.querySelector("confirmation-modal:has([data-confirm-answer-deletion])").dialog.showModal();

You can decide which version you prefer, I think both are fine

fekoch commented Mar 5, 2024

View reviewed changes

evap/staff/templates/staff_evaluation_operation.html Outdated Show resolved Hide resolved

niklasmohrin reviewed Mar 5, 2024

View reviewed changes

evap/staff/templates/staff_evaluation_operation.html Outdated Show resolved Hide resolved

fekoch force-pushed the 1991/reset_evaluation_after_it_started branch 2 times, most recently from c353503 to 5ee1e27 Compare April 15, 2024 15:51

fekoch force-pushed the 1991/reset_evaluation_after_it_started branch 3 times, most recently from cb79677 to 909c5a7 Compare April 22, 2024 19:28

fekoch marked this pull request as ready for review April 22, 2024 19:34

Kakadus mentioned this pull request Apr 22, 2024

Reraise exceptions when debugging #2167

Merged

fekoch requested a review from niklasmohrin April 23, 2024 08:02

niklasmohrin reviewed Apr 24, 2024

View reviewed changes

evap/evaluation/migrations/0143_alter_evaluation_state.py Outdated Show resolved Hide resolved

fekoch force-pushed the 1991/reset_evaluation_after_it_started branch from 909c5a7 to f3670fb Compare April 29, 2024 15:33

fekoch marked this pull request as draft April 29, 2024 18:32

fekoch mentioned this pull request Apr 29, 2024

Make Evaluation.State an IntEnum #2174

Merged

fekoch force-pushed the 1991/reset_evaluation_after_it_started branch 3 times, most recently from 6c0cf01 to 8eabcbe Compare June 3, 2024 16:00

fekoch force-pushed the 1991/reset_evaluation_after_it_started branch from 8eabcbe to 2e67d03 Compare June 24, 2024 17:55

fekoch marked this pull request as ready for review June 24, 2024 17:57

richardebeling reviewed Jun 24, 2024

View reviewed changes

evap/evaluation/models.py Outdated Show resolved Hide resolved

evap/evaluation/models.py Outdated Show resolved Hide resolved

evap/evaluation/models.py Outdated Show resolved Hide resolved

evap/staff/templates/staff_evaluation_operation.html Outdated Show resolved Hide resolved

fekoch requested a review from richardebeling June 24, 2024 19:18

richardebeling reviewed Jun 24, 2024

View reviewed changes

evap/evaluation/models.py Outdated Show resolved Hide resolved

richardebeling approved these changes Jun 24, 2024

View reviewed changes

richardebeling requested review from niklasmohrin and janno42 June 24, 2024 20:02

janno42 requested changes Jul 8, 2024

View reviewed changes

fekoch added 26 commits August 12, 2024 19:23

implement delete answers checkbox in view

24ddbf0

type helper function

42851fb

remove empty newline

fe2ea9a

correctly nest span element

113034d

incorporate feeback

eb2ace1

require delete_previous_answers to be specified explicitly

76c7492

specify delete_previous_answers directly

755f730

use multiline comment syntax

947230a

cleanup html code style

95a5047

add None to accepted argument types

2ef31ba

disallow resetting of single results

c7143ee

delete unused html

464a901

apply translation suggestions

54c1322

only show confirmation modal with delete answers checkbox

a17ae00

narrow reset_to_new to only take bools

8a2feb3

color delete previous answers button with danger

813fac5

change 'can_reset_to_new' to do all checks in one function

b6e57d7

consistently leave out the slash for the "self-closing" tags

e06216d

favor nesting inputs in labels instead of id/for

7694ba4

remove unused for attribute

11b793d

cleanup modal-skip-hack

70b4611

use closing slashes for self closing tags

5a5df94

fix text

5797446

run formatter

1e16353

change can_reset_to_new to @Property to keep in line with other condi…

9ab450e

…tions

convert parameter to bool

5d9d71e

fekoch force-pushed the 1991/reset_evaluation_after_it_started branch from cecbc89 to 5d9d71e Compare August 12, 2024 17:24

richardebeling approved these changes Aug 12, 2024

View reviewed changes

Kakadus approved these changes Aug 26, 2024

View reviewed changes

niklasmohrin approved these changes Aug 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reset evaluation after it started #2136

reset evaluation after it started #2136

fekoch commented Mar 5, 2024 •

edited

Loading

fekoch commented Apr 22, 2024

niklasmohrin left a comment

niklasmohrin commented Apr 24, 2024

fekoch commented Jun 10, 2024 •

edited

Loading

janno42 left a comment •

edited

Loading

richardebeling left a comment

niklasmohrin left a comment

niklasmohrin Aug 30, 2024

niklasmohrin Aug 30, 2024

niklasmohrin Aug 30, 2024

niklasmohrin Aug 30, 2024

niklasmohrin Aug 30, 2024

	answer_class._default_manager.filter(contribution__evaluation_id=self.id).delete()
	answer_class._default_manager.filter(contribution__evaluation=self).delete()

		if request.POST.get("delete-previous-answers") == "on":
		delete_previous_answers = True

	submitEvent.target.querySelector("confirmation-modal[data-allow-answer-deletion]").dialog.showModal();
	submitEvent.target.querySelector("confirmation-modal:has([data-confirm-answer-deletion])").dialog.showModal();

reset evaluation after it started #2136

Are you sure you want to change the base?

reset evaluation after it started #2136

Conversation

fekoch commented Mar 5, 2024 • edited Loading

fekoch commented Apr 22, 2024

niklasmohrin left a comment

Choose a reason for hiding this comment

niklasmohrin commented Apr 24, 2024

fekoch commented Jun 10, 2024 • edited Loading

janno42 left a comment • edited Loading

Choose a reason for hiding this comment

richardebeling left a comment

Choose a reason for hiding this comment

niklasmohrin left a comment

Choose a reason for hiding this comment

niklasmohrin Aug 30, 2024

Choose a reason for hiding this comment

niklasmohrin Aug 30, 2024

Choose a reason for hiding this comment

niklasmohrin Aug 30, 2024

Choose a reason for hiding this comment

niklasmohrin Aug 30, 2024

Choose a reason for hiding this comment

niklasmohrin Aug 30, 2024

Choose a reason for hiding this comment

fekoch commented Mar 5, 2024 •

edited

Loading

fekoch commented Jun 10, 2024 •

edited

Loading

janno42 left a comment •

edited

Loading