#140: Question UI Renderer soll gelöschte Antworten erkennen

[MHajoha]

Closes #140

[MHajoha]

@MartinGauk ist jetzt rebased. Möchtest du dir noch anschauen, ob die Integration mit dem Iframe sinnvoll ist?

[MartinGauk]

Das Name Mangling besteht nicht mehr.
Welche Implikationen hat die doppelte Benutzung von Namen in Bezug auf #159?

[MHajoha]

Guter Punkt und gute Frage. Ersterer ist in a2c1a80 gefixt.

Was Wechselwirkungen mit #159 betrifft:

• Dieser PR dürfte feat: handle duplicate field names and select elements with multiple attribute #159 nicht beeinflussen, solange populate_duplicate_field_names vor check_for_and_preserve_unknown_options aufgerufen wird (aktuell wäre das so).
• feat: handle duplicate field names and select elements with multiple attribute #159 wird diesen PR beeinflussen, weil check_for_and_preserve_unknown_options dann auch beigebracht werden muss, mit den JSON-Arrays in den multi-valued fields zu arbeiten. Im simpelsten Fall wäre das vermutlich nicht allzu schwer, idealerweise sollten wir aber noch eine Variante der Warnung dafür hinzufügen. (z.B. The last submission set the field choice to the value(s) A, B, D, but the option(s) D are no longer available. The available options are: A, B, C.)

[MartinGauk]

Das wird mir zu aufwändig. Um die Dinge zu vereinfachen, sollten wir NICHT versuchen, die Werte zu erhalten (außer bei selects).

[MHajoha]

Durch das Erhalten der Werte kommt hier wirklich wenig Komplexität hinzu, die nicht auch für die Warnung nötig ist. Ich denke der Aufwand, die beiden PRs aufeinander zu rebasen ist echt überschaubar. (Auf jeden Fall weniger als mein letzter Rebase) Aber vielleicht sprechen wir da morgen mal drüber

[MartinGauk]

Warum ist dieser Marker notwendig?

[MHajoha]

Es ist ja leider inzwischen etwas her, aber ich glaube es schien mir der einfachste Weg, den neuesten Step mit submitteten Daten zu finden. $qa->get_last_qt_data ist für uns unpassend, weil z.B. ein Step mit _scoringstate hinter dem letzten Save/Submit-State liegen könne (und das oft auch tut).

Es wäre vielleicht ausreichend, den neuesten Step mit präfixlosen QT-Vars zu suchen, oder den neusten Step mit bestimmten States, oder eine Kombination, aber beides schien mir unsicherer und hätte mehr recherche bedurft als der Marker.

[MHajoha]

Wenn du eine bessere Möglichkeit kennst, setze ich die natürlich gern um. Falls nicht, würde ich die Erklärung noch als Code-Kommentar hinzufügen.

[MartinGauk]

Das verstehe ich nicht, weil Moodle ja nur mit $qa->get_last_qt_data arbeitet. Hat sich der Marker eh erledigt, wenn (wie im anderen Kommentar gewünscht) nicht mehr versucht wird, die Werte zu erhalten?

[MHajoha]

Wenn ich das gerade richtig überblicke, setzt auch kein Standard-Moodle-QType eine mit _ beginnende QT-Var in einem Step außer dem ersten. D.h. die laufen gar nicht in dieses Problem.

Vielleicht nochmal anders beschrieben, sehen Attempts bei uns typischerweise so aus:

• $qa->steps[0]->data: [ "_attemptstate" => "abcdefg", "-_behaviour" => "deferredfeedback" ]
• $qa->steps[1]->data: [ "mein_input_feld" => "studi_antwort" ]
• $qa->steps[2]->data: [ "_scoringstate" => "hijklmnop", "-finish" => 1 ]

$qa->get_last_qt_data() gibt hier [ "_scoringstate" => "hijklmnop" ] zurück, also eben nicht die neuste Antwort.

get_last_qt_data filtert Felder heraus, die mit - (Behaviour-Vars) oder : (Metadaten) beginnen, und gibt diese gefilterten Daten dann für den neuesten Step zurück, wo sie nicht leer sind. Dem am nächsten würde es kommen, eine Variante davon zu schreiben, die zusätzlich Felder beginnend mit _ ("cached values that were created during processing") herausfiltert. In den meisten Fällen wäre das auch korrekt, würde aber Fragepakete brechen, die (aus welchem Grund auch immer) eine leere Submission generieren.

Der robusteste Ansatz schien mir daher der Marker. Alternativ könnte man wie erwähnt vielleicht auch anhand von $step->state arbeiten, da wäre ich aber unsicher, dass der State bei neuen Steps schon rechtzeitig korrekt gesetzt ist und dass die States auch tatsächlich so konsistent verwendet werden, dass ein Rückschluss überhaupt möglich ist.

Der Marker hat also tendenziell wenig mit dem Erhalten der invaliden Werte zu tun. Ich würde perspektivisch sogar unsere Verwendung von get_last_qt_var($name) durch get_last_response()[$name] ersetzen wollen, da aktuell keine Möglichkeit besteht, effektiv ein Feld in einer späteren Submission zu entfernen. (Komischer Use-Case, sollten wir aber mMn supporten.)

[MartinGauk]

Was wäre denn eigentlich, wenn wir alle Formulardaten im iframe in ein einzelnes JSON packen würden? Würde dadurch #159 (deutlich?) vereinfacht werden können, @janbritz ?

Mir fällt hier nur ein Problem ein in Verbindung mit Uploadfeldern. Die müssten schon noch getrennt übermittelt werden, damit der Moodle-Mechanismus zum Abspeichern der Dateien greift.

[janbritz]

Falls wir das machen und Entries mit doppeltem Key als Listen im JSON unterbringen, würde das #159 vereinfachen. Vor allem, wenn es für uns vertretbar ist, dass wenn z.B. nur eine von mehreren Checkboxen mit dem selben Namen gechecked ist oder nur eine <option> in einem <select multiple> selektiert ist, nur einen Wert und keine Liste mit einem Wert zurückgibt, würde das den kompletten (neuen) Code in classes/question_ui_metadata_extractor.php überflüssig machen. Allerdings finde ich es, so wie es aktuell ist, dev-freundlicher, da keine Fallunterscheidung (einzelner Wert oder Array) im Paket vorgenommen werden muss, sondern in solchen Fällen immer ein Array ankommt.

Im classes/question_ui_renderer.php würde sich aber vermutlich nicht viel ändern.

[MHajoha]

Alles in JSON zu packen würde auch #142 lösen, das wäre vielleicht ganz sinnvoll.

Einzelwerte können wir FormModel-seitig in eine Liste packen, davon würden die meisten Paketentwickler:innen vermutlich gar nichts mitbekommen.

[MHajoha]

Ich habe den PR jetzt rebased, und an die duplicate fields angepasst. Ich habe mich dabei dann doch dafür entschieden, invalide Werte nur für Selects beizubehalten. Der Grund dafür ist in einem Kommentar in check_for_and_preserve_unknown_options erklärt:

// We don't preserve values for any type other than selects because it would be difficult to then remove the
// invalid value:
// For multi-valued fields, there would be no way for us to know whether the intention was to replace the
// invalid value or add a new one.
// For single-valued fields, while we can assume that any valid value should overwrite the invalid value, that
// would add a fair bit of complexity for little benefit.

D.h. für Einzelwertfelder wäre es schon gut möglich, aber etwas komplexer als ich zunächst gehofft hatte. Da du @MartinGauk schon meintest, wir sollten das weglassen, habe ich das jetzt auch getan.

Ein paar Beispiele, wie das jetzt aussieht:

• Multi-Select:
  
• Radios
  
• Checkboxes
  

Gibt es mehrere invalide Werte, werden einfach separate Warnungen untereinander angezeigt: