Main language for evaluations #2367

hansegucker · 2025-01-13T18:46:31Z

No description provided.

richardebeling · 2025-01-19T15:12:54Z

evap/contributor/tests/test_views.py

+        with self.assertRaises(ValueError):
+            form["main_language"] = "some_other_wrong_value"
+            form.submit(name="operation", value="save")


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.

richardebeling · 2025-01-19T15:14:37Z

evap/evaluation/migrations/0148_evaluation_main_language.py

+def _migrate(apps, schema_editor):
+    Evaluation = apps.get_model("evaluation", "Evaluation")
+    for evaluation in Evaluation.objects.filter(state__gte=40):
+        evaluation.main_language = "de"


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?)

We want each published evaluation to have a decided main language. We check this during state transitions. So, we have to set a reasonable default for all evaluations which changing the main language is no longer possible.

Ok, sounds reasonable. Could we still replace 40 here with some named constant?

evap/evaluation/models.py

richardebeling · 2025-01-19T15:24:03Z

evap/staff/tests/test_views.py

@@ -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):


factoring out the helper is nice -- but we should also use it in test_evaluation_create then :D

richardebeling · 2025-01-19T15:27:12Z

evap/staff/tests/test_views.py

+        self.assertNotEqual(Evaluation.objects.first().state, self.evaluation.State.APPROVED)
+
+        form["main_language"] = "en"
+        form.submit("operation", value="approve")


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)

evap/staff/views.py

evap/student/templates/student_vote.html

Kakadus · 2025-01-27T21:38:17Z

evap/evaluation/migrations/0148_evaluation_main_language.py

+            model_name="evaluation",
+            name="main_language",
+            field=models.CharField(
+                choices=[("en", "English"), ("de", "Deutsch"), ("x", "undecided")],


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

I think that's fine for this feature. Dynamic language support in evap doesn't really exist, we hardcode "German and English" in many places.

But doesn't this then motivate a models.TextChoices instead of a setting?

Yes, but I would do that separately from this PR

niklasmohrin · 2025-02-03T22:59:39Z

As far as I understand, this waits for #2377, so should we mark this one here as draft for now?

@ts-ignore

# Conflicts: # evap/student/templates/student_vote.html diff --git a/evap/static/ts/src/sisyphus.LICENSE b/evap/static/ts/src/sisyphus.LICENSE new file mode 100644 index 00000000..d53be7e3 --- /dev/null +++ b/evap/static/ts/src/sisyphus.LICENSE @@ -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. \ No newline at end of file diff --git a/evap/static/ts/src/sisyphus.ts b/evap/static/ts/src/sisyphus.ts new file mode 100644 index 00000000..db049c46 --- /dev/null +++ b/evap/static/ts/src/sisyphus.ts @@ -0,0 +1,309 @@ +/** + * Plugin developed to save html forms data to LocalStorage to restore them after browser crashes, tabs closings + * and other disasters. + * + * @author Alexander Kaupanin <[email protected]> + * @author Jonathan Weth <[email protected]> + */ + +interface SisyphusOptions { + name?: string; + excludeFields: string[]; + customKeySuffix: string; + locationBased: boolean; + timeout: number; + autoRelease: boolean; + onSave: (arg0: Sisyphus) => void; + onBeforeRestore: (arg0: Sisyphus) => boolean | void; + onRestore: (arg0: Sisyphus) => void; + onRelease: (arg0: Sisyphus) => void; +} + +export class Sisyphus { + private readonly identifier: string; + private options: SisyphusOptions; + private readonly href: string; + private readonly targets: NodeListOf<HTMLFormElement>; + + constructor(identifier: string, options: SisyphusOptions) { + const defaults = { + excludeFields: [], + customKeySuffix: "", + locationBased: false, + timeout: 0, + autoRelease: true, + onSave: function () {}, + onBeforeRestore: function () {}, + onRestore: function () {}, + onRelease: function () {}, + }; + + this.identifier = identifier; + this.options = { ...defaults, ...options }; + + this.targets = document.querySelectorAll(this.identifier); + if (this.options.name) { + this.href = this.options.name; + } else { + this.href = location.hostname + location.pathname; + } + + const callback_result = this.options.onBeforeRestore(this); + if (callback_result === undefined || callback_result) { + this.restoreAllData(); + } + + if (this.options.autoRelease) { + this.bindReleaseData(); + } + + this.bindSaveData(); + } + + findFieldsToProtect(target: HTMLFormElement): Array<Element> { + return Array.of(...target.elements).filter((el: Element) => { + if ( + el instanceof HTMLInputElement && + ["submit", "reset", "button", "file", "password", "hidden"].includes(el.type.toLowerCase()) + ) { + return false; + } + if (["BUTTON", "FIELDSET", "OBJECT", "OUTPUT"].includes(el.tagName)) { + return false; + } + return true; + }); + } + + getExcludeFields(): Element[] { + return this.options.excludeFields.flatMap(selector => Array.from(document.querySelectorAll(selector))); + } + + getFormIdAndName(target: HTMLFormElement) { + return target.id + target.name; + } + + getPrefix(target: HTMLFormElement, field: Element) { + return ( + (this.options.locationBased ? this.href : "") + + this.getFormIdAndName(target) + + (field.getAttribute("name") || "") + + this.options.customKeySuffix + ); + } + + /** + * Bind saving data + */ + bindSaveData() { + if (this.options.timeout) { + this.saveDataByTimeout(); + } + + for (const target of this.targets) { + for (const field of this.findFieldsToProtect(target)) { + if (this.getExcludeFields().includes(field)) { + continue; + } + const prefix = this.getPrefix(target, field); + if ( + (field instanceof HTMLInputElement && field.type === "text") || + field instanceof HTMLTextAreaElement + ) { + if (!this.options.timeout) { + this.bindSaveDataImmediately(field, prefix); + } + } + this.bindSaveDataOnChange(field); + } + } + } + + /** + * Save all protected forms data to Local Storage. + * Common method, necessary to not lead astray user firing 'data is saved' when select/checkbox/radio + * is changed and saved, while text field data is saved only by timeout + */ + saveAllData() { + for (const target of this.targets) { + let multiCheckboxCache: { [key: string]: boolean } = {}; + + for (const field of this.findFieldsToProtect(target)) { + if (this.getExcludeFields().includes(field) || !field.getAttribute("name")) { + continue; + } + const prefix = this.getPrefix(target, field); + const fieldType = field.getAttribute("type"); + // @ts-ignore + let value: string | string[] | boolean = field.value; + + if (field instanceof HTMLInputElement && fieldType === "checkbox") { + if (field.name.indexOf("[") !== -1) { + if (multiCheckboxCache[field.name]) { + return; + } + let tempValue: string[] = []; + $("[name='" + field.name + "']:checked").each(function () { + tempValue.push(field.value); + }); + value = tempValue; + multiCheckboxCache[field.name] = true; + } else { + value = field.checked; + } + this.saveToBrowserStorage(prefix, value, false); + } else if (field instanceof HTMLInputElement && fieldType === "radio") { + if (field.checked) { + value = field.value; + this.saveToBrowserStorage(prefix, value, false); + } + } else { + this.saveToBrowserStorage(prefix, value, false); + } + } + } + this.options.onSave(this); + } + + /** + * Restore forms data from Local Storage + */ + restoreAllData() { + let restored = false; + + for (const target of this.targets) { + for (const field of this.findFieldsToProtect(target)) { + if (this.getExcludeFields().includes(field)) { + continue; + } + const resque = localStorage.getItem(this.getPrefix(target, field)); + if (resque !== null) { + this.restoreFieldsData(field, resque); + restored = true; + } + } + } + + if (restored) { + this.options.onRestore(this); + } + } + + /** + * Restore form field data from local storage + */ + restoreFieldsData(field: Element, resque: string) { + if (field.getAttribute("name") === undefined) { + return false; + } + if ( + field instanceof HTMLInputElement && + field.type === "checkbox" && + resque !== "false" && + field.name.indexOf("[") === -1 + ) { + field.checked = true; + } else if ( + field instanceof HTMLInputElement && + field.type === "checkbox" && + resque === "false" && + field.name.indexOf("[") === -1 + ) { + field.checked = false; + } else if (field instanceof HTMLInputElement && field.type === "radio") { + if (field.value === resque) { + field.checked = true; + } + } else if (field instanceof HTMLInputElement && field.name.indexOf("[") === -1) { + field.value = resque; + } else { + // @ts-ignore + field.value = resque.split(","); + } + } + + /** + * Bind immediate saving (on typing/checking/changing) field data to local storage when user fills it + */ + bindSaveDataImmediately(field: HTMLInputElement | HTMLTextAreaElement, prefix: string) { + field.addEventListener("input", () => { + this.saveToBrowserStorage(prefix, field.value); + }); + } + + /** + * Save data to Local Storage and fire callback if defined + */ + saveToBrowserStorage(key: string, value: any, fireCallback?: boolean) { + // if fireCallback is undefined it should be true + fireCallback = fireCallback === undefined ? true : fireCallback; + localStorage.setItem(key, value + ""); + if (fireCallback && value !== "") { + this.options.onSave(this); + } + } + + /** + * Bind saving field data on change + */ + bindSaveDataOnChange(field: Element) { + field.addEventListener("change", () => { + this.saveAllData(); + }); + } + + /** + * Saving (by timeout) field data to local storage when user fills it + */ + saveDataByTimeout() { + setTimeout(() => { + const timeout = () => { + this.saveAllData(); + setTimeout(timeout, this.options.timeout * 1000); + }; + + return timeout; + }, this.options.timeout * 1000); + } + + /** + * Bind release form fields data from local storage on submit/reset form + */ + bindReleaseData() { + for (const target of this.targets) { + const releaseHandler = () => { + this.releaseData(target); + }; + target.addEventListener("submit", releaseHandler); + target.addEventListener("reset", releaseHandler); + } + } + + /** + * Manually release form fields + */ + manuallyReleaseData() { + for (const target of this.targets) { + this.releaseData(target); + } + } + + /** + * Bind release form fields data from local storage on submit/resett form + */ + releaseData(target: HTMLFormElement) { + let released = false; + + for (const field of this.findFieldsToProtect(target)) { + if (this.getExcludeFields().includes(field)) { + continue; + } + localStorage.removeItem(this.getPrefix(target, field)); + released = true; + } + + if (released) { + this.options.onRelease(this); + } + } +}

janno42

Works almost as expected 👍

One thing though: If the language is still "undecided", contributors will see the language field in the evaluation form filled in with the first option ("English"). Instead, they should see that no language has been chosen yet, and should deliberately choose a language.

richardebeling linked an issue Jan 19, 2025 that may be closed by this pull request

Independent questionnaire language #1575

Open

richardebeling reviewed Jan 19, 2025

View reviewed changes

Kakadus reviewed Jan 27, 2025

View reviewed changes

janno42 self-requested a review February 10, 2025 18:29

niklasmohrin marked this pull request as draft February 17, 2025 16:29

hansegucker added 5 commits February 24, 2025 17:34

Introduce field for setting main language of evaluations

b0ccc10

Prevent approving evaluations without a valid main language

832b836

Add migration for setting main language for existing evaluations

1153013

Add select for questionnaire language on vote page

5ad70a3

hansegucker force-pushed the main-language-for-evaluations branch from 4e8a9cb to 0e751fa Compare February 24, 2025 16:36

Store selected questionnaire language

8bd5721

janno42 requested changes Mar 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Main language for evaluations #2367

Main language for evaluations #2367

hansegucker commented Jan 13, 2025

richardebeling Jan 19, 2025

richardebeling Jan 19, 2025

hansegucker Feb 17, 2025

richardebeling Mar 3, 2025

richardebeling Jan 19, 2025

richardebeling Jan 19, 2025

Kakadus Jan 27, 2025 •

edited

Loading

richardebeling Feb 3, 2025

Kakadus Mar 3, 2025

richardebeling Mar 3, 2025

niklasmohrin commented Feb 3, 2025

janno42 left a comment

Main language for evaluations #2367

Are you sure you want to change the base?

Main language for evaluations #2367

Conversation

hansegucker commented Jan 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kakadus Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasmohrin commented Feb 3, 2025

janno42 left a comment

Choose a reason for hiding this comment

Kakadus Jan 27, 2025 •

edited

Loading