Main language for evaluations #2367
Conversation
| with self.assertRaises(ValueError): | ||
| form["main_language"] = "some_other_wrong_value" | ||
| form.submit(name="operation", value="save") |
There was a problem hiding this comment.
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.
| 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.
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?)
| 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.
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.
In preparation, yes. But at the point of approval (not editor approval!), a language has to be determined.
| @@ -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.
factoring out the helper is nice -- but we should also use it in test_evaluation_create then :D
| 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.
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)
| 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.
random consideration: A narrower, more precise interface would be passing require_main_language: bool instead of passing an operation string that implicitly allows to infer that bool
| data-bs-placement="bottom" | ||
| title="{{ language_name }}" | ||
| role="button" | ||
| href="{% url 'student:vote' evaluation.pk %}?language={{ language_code }}" |
There was a problem hiding this comment.
This should probably be a POST-form, not a GET-anchor. (Currently, this wouldn't need the CSRF token)
There was a problem hiding this comment.
Why should that need a CSRF token? As this won't set the language anywhere in the backend, it's only display logic determined by the GET parameter. @janno42 was OK with this.
There was a problem hiding this comment.
Ah, I assumed that this would save the language so that if a user came back later to the same evaluation, they would still go back to the language they last selected.
If we keep the language stateless, the anchor is fine here, but then we don't need the csrf_token above.
| model_name="evaluation", | ||
| name="main_language", | ||
| field=models.CharField( | ||
| choices=[("en", "English"), ("de", "Deutsch"), ("x", "undecided")], |
There was a problem hiding this comment.
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 models.TextChoices
No description provided.