Skip to content
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

Jump to bottom

reset evaluation after it started #2136

Merged
merged 30 commits into from
Oct 14, 2024
Merged

reset evaluation after it started #2136

merged 30 commits into from
Oct 14, 2024

Conversation

fekoch
Copy link
Collaborator

@fekoch fekoch commented Mar 5, 2024 •
edited
Loading

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

@fekoch 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 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
Copy link
Collaborator Author

fekoch commented Apr 22, 2024

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

@fekoch fekoch marked this pull request as ready for review April 22, 2024 19:34
@Kakadus Kakadus mentioned this pull request Apr 22, 2024
Merged
@fekoch fekoch requested a review from niklasmohrin April 23, 2024 08:02
niklasmohrin
niklasmohrin reviewed Apr 24, 2024
View reviewed changes
Copy link
Member

@niklasmohrin niklasmohrin left a comment

Choose a reason for hiding this comment

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

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

@niklasmohrin
Copy link
Member

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 fekoch force-pushed the 1991/reset_evaluation_after_it_started branch from 909c5a7 to f3670fb Compare April 29, 2024 15:33
@fekoch fekoch marked this pull request as draft April 29, 2024 18:32
@fekoch fekoch mentioned this pull request Apr 29, 2024
Merged
@fekoch 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
Copy link
Collaborator Author

fekoch commented Jun 10, 2024 •
edited
Loading

@fekoch some TODO's:

  • type annotation in test helper function
  • remove empty newline

@fekoch fekoch force-pushed the 1991/reset_evaluation_after_it_started branch from 8eabcbe to 2e67d03 Compare June 24, 2024 17:55
@fekoch fekoch marked this pull request as ready for review June 24, 2024 17:57
richardebeling
richardebeling reviewed Jun 24, 2024
View reviewed changes
@fekoch fekoch requested a review from richardebeling June 24, 2024 19:18
janno42
janno42 requested changes Jul 8, 2024
View reviewed changes
janno42
janno42 requested changes Jul 8, 2024
View reviewed changes
Copy link
Member

@janno42 janno42 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  • 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).

fekoch added 20 commits October 7, 2024 17:54
@fekoch
require delete_previous_answers to be specified explicitly
89c9711
@fekoch
specify delete_previous_answers directly
f18b5dc
@fekoch
use multiline comment syntax
cf0e34d
@fekoch
cleanup html code style
5dd7b05
@fekoch
add None to accepted argument types
1593d2b
@fekoch
disallow resetting of single results
0ebf5ed
@fekoch
delete unused html
08fc10a
@fekoch
apply translation suggestions
1a49a91
@fekoch
only show confirmation modal with delete answers checkbox
3fc8efc
@fekoch
narrow reset_to_new to only take bools
db5f42b
@fekoch
color delete previous answers button with danger
d92c8d5
@fekoch
change 'can_reset_to_new' to do all checks in one function
fda6ed8
@fekoch
consistently leave out the slash for the "self-closing" tags
33bcab1
@fekoch
favor nesting inputs in labels instead of id/for
b281d88
@fekoch
remove unused for attribute
7d802a6
@fekoch
cleanup modal-skip-hack
0c68ae6
@fekoch
use closing slashes for self closing tags
f3b2a9c
@fekoch
fix text
7b78414
@fekoch
change can_reset_to_new to @Property to keep in line with other condi…
b97e12e 
…tions
@fekoch
convert parameter to bool
316553b
@fekoch fekoch force-pushed the 1991/reset_evaluation_after_it_started branch from 5d9d71e to 316553b Compare October 7, 2024 16:04
@fekoch fekoch requested a review from niklasmohrin October 7, 2024 17:13
niklasmohrin
niklasmohrin approved these changes Oct 14, 2024
View reviewed changes
Copy link
Member

@niklasmohrin niklasmohrin left a comment

Choose a reason for hiding this comment

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

Very cool, thanks!

@niklasmohrin niklasmohrin merged commit cd259e8 into e-valuation:main Oct 14, 2024
12 checks passed
@fekoch fekoch deleted the 1991/reset_evaluation_after_it_started branch October 14, 2024 16:59
Kakadus pushed a commit to Kakadus/EvaP that referenced this pull request Jan 6, 2025
@fekoch @Kakadus
Reset evaluation after it started (e-valuation#2136)
5201b4c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janno42 janno42 janno42 approved these changes

@richardebeling richardebeling richardebeling approved these changes

@niklasmohrin niklasmohrin niklasmohrin approved these changes

@Kakadus Kakadus Kakadus approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reset evaluation after it started
5 participants
@fekoch @niklasmohrin @janno42 @richardebeling @Kakadus