-
Notifications
You must be signed in to change notification settings - Fork 146
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.
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
Main language for evaluations #2367
base: main
Are you sure you want to change the base?
Changes from all commits
9906c86
1e01a20
e044b8c
de1fab2
977a4ae
4e8a9cb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
# Generated by Django 5.1.3 on 2024-11-25 20:19 | ||
|
||
from django.db import migrations, models | ||
|
||
|
||
def _migrate(apps, schema_editor): | ||
Evaluation = apps.get_model("evaluation", "Evaluation") | ||
for evaluation in Evaluation.objects.filter(state__gte=40): | ||
evaluation.main_language = "de" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? I would have expected "undecided" (which is the default value -> no explicit migration code needed?) (If we keep this, can we use some named variable instead of the magic 40?) |
||
evaluation.save() | ||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
("evaluation", "0147_unusable_password_default"), | ||
] | ||
|
||
operations = [ | ||
migrations.AddField( | ||
model_name="evaluation", | ||
name="main_language", | ||
field=models.CharField( | ||
choices=[("en", "English"), ("de", "Deutsch"), ("x", "undecided")], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I want to note that now a new migration is needed when the user changes the languages in the settings. Is it okay that this is still a setting then? I can imagine that this is unexpected from a django setting If not, then this can probably become a |
||
default="x", | ||
max_length=2, | ||
verbose_name="main language", | ||
), | ||
), | ||
migrations.RunPython(_migrate), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -425,6 +425,14 @@ class State(models.IntegerChoices): | |
name_en = models.CharField(max_length=1024, verbose_name=_("name (english)"), blank=True) | ||
name = translate(en="name_en", de="name_de") | ||
|
||
# questionaire is shown in this language per default | ||
main_language = models.CharField( | ||
max_length=2, | ||
verbose_name=_("main language"), | ||
default="x", | ||
choices=settings.LANGUAGES + [("x", _("undecided"))], | ||
) | ||
|
||
# defines how large the influence of this evaluation's grade is on the total grade of its course | ||
weight = models.PositiveSmallIntegerField(verbose_name=_("weight"), default=1) | ||
|
||
|
@@ -624,6 +632,10 @@ def is_in_evaluation_period(self): | |
def general_contribution_has_questionnaires(self): | ||
return self.general_contribution and self.general_contribution.questionnaires.count() > 0 | ||
|
||
@property | ||
def has_valid_main_language(self): | ||
return self.main_language != "x" | ||
|
||
@property | ||
def all_contributions_have_questionnaires(self): | ||
if is_prefetched(self, "contributions"): | ||
|
@@ -759,15 +771,20 @@ def can_publish_rating_results(self): | |
def ready_for_editors(self): | ||
pass | ||
|
||
@transition(field=state, source=State.PREPARED, target=State.EDITOR_APPROVED) | ||
@transition( | ||
field=state, | ||
source=State.PREPARED, | ||
target=State.EDITOR_APPROVED, | ||
conditions=[lambda self: self.has_valid_main_language], | ||
) | ||
def editor_approve(self): | ||
pass | ||
|
||
@transition( | ||
field=state, | ||
source=[State.NEW, State.PREPARED, State.EDITOR_APPROVED], | ||
target=State.APPROVED, | ||
conditions=[lambda self: self.general_contribution_has_questionnaires], | ||
conditions=[lambda self: self.general_contribution_has_questionnaires and self.has_valid_main_language], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue says that staff users should be able to set "undecided". I think this change here would block that workflow? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In preparation, yes. But at the point of approval (not editor approval!), a language has to be determined. |
||
) | ||
def manager_approve(self): | ||
pass | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1920,6 +1920,38 @@ def test_evaluation_create(self): | |
form.submit() | ||
self.assertEqual(Evaluation.objects.get().name_de, "lfo9e7bmxp1xi") | ||
|
||
def _set_valid_form(self, form): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. factoring out the helper is nice -- but we should also use it in |
||
form["course"] = self.course.pk | ||
form["name_de"] = "lfo9e7bmxp1xi" | ||
form["name_en"] = "asdf" | ||
form["vote_start_datetime"] = "2014-01-01 00:00:00" | ||
form["vote_end_date"] = "2099-01-01" | ||
form["general_questionnaires"] = [self.q1.pk] | ||
form["wait_for_grade_upload_before_publishing"] = True | ||
|
||
form["contributions-TOTAL_FORMS"] = 1 | ||
form["contributions-INITIAL_FORMS"] = 0 | ||
form["contributions-MAX_NUM_FORMS"] = 5 | ||
form["contributions-0-evaluation"] = "" | ||
form["contributions-0-contributor"] = self.manager.pk | ||
form["contributions-0-questionnaires"] = [self.q2.pk] | ||
form["contributions-0-order"] = 0 | ||
form["contributions-0-role"] = Contribution.Role.EDITOR | ||
form["contributions-0-textanswer_visibility"] = Contribution.TextAnswerVisibility.GENERAL_TEXTANSWERS | ||
|
||
def test_evaluation_create_main_language(self): | ||
response = self.app.get(self.url_for_semester, user=self.manager, status=200) | ||
form = response.forms["evaluation-form"] | ||
self._set_valid_form(form) | ||
|
||
with self.assertRaises(ValueError): | ||
form["main_language"] = "some_wrong_value" | ||
form.submit() | ||
|
||
form["main_language"] = "x" | ||
form.submit() | ||
self.assertEqual(Evaluation.objects.get().main_language, "x") | ||
|
||
|
||
class TestEvaluationCopyView(WebTestStaffMode): | ||
@classmethod | ||
|
@@ -2349,6 +2381,19 @@ def test_state_change_log_translated(self, trans): | |
response = self.app.get(self.url, user=self.manager) | ||
self.assertContains(response, "TRANSLATED-state: TRANSLATED-new → TRANSLATED-prepared") | ||
|
||
def test_evaluation_confirm_without_main_language(self): | ||
response = self.app.get(self.url, user=self.manager, status=200) | ||
form = response.forms["evaluation-form"] | ||
|
||
form["main_language"] = "x" | ||
response = form.submit("operation", value="approve") | ||
self.assertContains(response, "You have to set a main language to approve this evaluation.") | ||
|
||
self.assertNotEqual(Evaluation.objects.first().state, self.evaluation.State.APPROVED) | ||
|
||
form["main_language"] = "en" | ||
form.submit("operation", value="approve") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably have to add an assertion that the response now doesn't contain the above error anymore (our views return 200 even if there is a business logic error / form validation failed) |
||
|
||
|
||
class TestEvaluationDeleteView(WebTestStaffMode): | ||
csrf_checks = False | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1330,17 +1330,19 @@ def notify_reward_points(grantings, **_kwargs): | |
Evaluation, Contribution, formset=ContributionFormset, form=ContributionForm, extra=1 if editable else 0 | ||
) | ||
|
||
evaluation_form = EvaluationForm(request.POST or None, instance=evaluation, semester=evaluation.course.semester) | ||
operation = request.POST.get("operation") | ||
|
||
if request.method == "POST" and operation not in ("save", "approve"): | ||
raise SuspiciousOperation("Invalid POST operation") | ||
|
||
evaluation_form = EvaluationForm( | ||
request.POST or None, instance=evaluation, semester=evaluation.course.semester, operation=operation | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. random consideration: A narrower, more precise interface would be passing |
||
) | ||
formset = InlineContributionFormset( | ||
request.POST or None, instance=evaluation, form_kwargs={"evaluation": evaluation} | ||
) | ||
|
||
operation = request.POST.get("operation") | ||
|
||
if evaluation_form.is_valid() and formset.is_valid(): | ||
if operation not in ("save", "approve"): | ||
raise SuspiciousOperation("Invalid POST operation") | ||
|
||
if not evaluation.can_be_edited_by_manager or evaluation.participations_are_archived: | ||
raise SuspiciousOperation("Modifying this evaluation is not allowed.") | ||
|
||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
Copyright (c) 2011-2013 Alexander Kaupanin https://github.com/simsalabim | ||
Copyright (c) 2024 Jonathan Weth <[email protected]> | ||
|
||
Permission is hereby granted, free of charge, to any person obtaining | ||
a copy of this software and associated documentation files (the | ||
"Software"), to deal in the Software without restriction, including | ||
without limitation the rights to use, copy, modify, merge, publish, | ||
distribute, sublicense, and/or sell copies of the Software, and to | ||
permit persons to whom the Software is furnished to do so, subject to | ||
the following conditions: | ||
|
||
The above copyright notice and this permission notice shall be | ||
included in all copies or substantial portions of the Software. | ||
|
||
THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, | ||
EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF | ||
MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND | ||
NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE | ||
LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION | ||
OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION | ||
WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like the "Invalid value doesn't work"-block in 303, I think this will already fail on setting the form field as django webtest throws when we try to set a value that is not in the choices list. I think testing that server-side validation works is a good idea here, not sure if it can be done with WebTest form framework though. I think in the past we always manually crafted POST requests to achieve this.