TA: move Questionlist into Slate

[nhaagen]

https://docu.ilias.de/goto.php?target=wiki_1357_Revamping_Exam_View_with_List_of_Questions

Move „List of Questions“ to Slate:

• Instead of displaying the “List of Questions” in the ‘tst_left’ div, it’s displayed wihtin the slate. Like with Learning Sequences, the slate is hidden by default but may be showed by participants by clicking a main bar item (here: “List of Questions”). The menu item shows the test icon.
• Remove “Hide/Show List of Questions” button:
  There is no need for this button anymore as the “List of Questions” can be displayed via the main bar item.
• Use the “WorkflowListing” UI element for the display of the “List of Questions”:
  “WorkflowListing” can identify the currently processed question and differentiate between questions which were processed and those which weren’t.
• Remove question flag from “List of Questions”:
  The only things “WorkflowListing” cannot do is to show the “flag” of questions that were “marked” by participants.As this would contribute to unclutter the view, we suppose it being a gain for the Exam View.
• Move test title, participant name and test pass ID to top whitespace not used by now:
  In doing so, we suggest to show the installation logo as well.

Settings

• Renaming “Dashboard” view in the test pass:
  The “Dashboard” term is used in three different ways in ILIAS (main bar item, Test object tab, “Dashboard” as referred to here). Thus, it would contribute to disambiguation to rename the “Dashboard” option for the test GUI. Proposal: “Test Pass Overview” (German: “Übersicht Testdurchlauf”)
• Separating activation of „List of Questions” and “Dashboard” (then: “Test Pass Overview”):
  Activation of „List of Questions” (Checkbox)
  Activation of “Test Pass Overview” (Checkbox) with sub-options (“Show prior to the first question” s.o.)

[nhaagen]

@kergomard is working on the settings (https://github.com/kergomard/ILIAS/commits/9/test/feature_use_ks_forms_for_general_settings);
I'll wait for that before splitting up settings for 'List of Questions” and ''Pass Overview"

[dsstrassner]

@nhaagen I plan to give @kergomard feedback on his PR before his vacation.

Also, I am sorry, but maybe we need to change "Test Pass Overview" after the feedback from @matthiaskunkel on this Mantis issue.

[nhaagen]

Hi @dsstrassner ,
I think, this is good to go; probably we have to re-touch translations, but that's (hopefully) a minor thing.

[dsstrassner]

Hi @nhaagen,

first remark. I got an error by trying to import the attached tst export. (I use doil with this branch: https://github.com/kergomard/ILIAS/tree/_9/TA/QuestionList because of other test and trunk problems.):
1693215355__4105__tst_27526.zip.

Stacktrace:

Error thrown with message "Call to undefined method ilObjTestSettingsParticipantFunctionality::withQuestionListMode()"

Stacktrace:
#24 Error in /var/www/html/Modules/Test/classes/class.ilObjTest.php:3425
#23 ilObjTest:fromXML in /var/www/html/Services/QTI/classes/class.ilQTIParser.php:565
#22 ilQTIParser:handlerParseEndTag in /var/www/html/Services/QTI/classes/class.ilQTIParser.php:545
#21 ilQTIParser:handlerEndTag in [internal]:0
#20 xml_parse in /var/www/html/Services/Xml/classes/class.ilSaxParser.php:162
#19 ilSaxParser:parse in /var/www/html/Services/Xml/classes/class.ilSaxParser.php:98
#18 ilSaxParser:startParsing in /var/www/html/Services/QTI/classes/class.ilQTIParser.php:253
#17 ilQTIParser:startParsing in /var/www/html/Modules/Test/classes/class.ilTestImporter.php:101
#16 ilTestImporter:importXmlRepresentation in /var/www/html/Services/Export/classes/class.ilImport.php:304
#15 ilImport:processItemXml in /var/www/html/Services/Export/classes/class.ilExportFileParser.php:109
#14 ilExportFileParser:handleEndTag in [internal]:0
#13 xml_parse in /var/www/html/Services/Xml/classes/class.ilSaxParser.php:162
#12 ilSaxParser:parse in /var/www/html/Services/Xml/classes/class.ilSaxParser.php:98
#11 ilSaxParser:startParsing in /var/www/html/Services/Export/classes/class.ilExportFileParser.php:67
#10 ilExportFileParser:startParsing in /var/www/html/Services/Export/classes/class.ilExportFileParser.php:49
#9 ilExportFileParser:__construct in /var/www/html/Services/Export/classes/class.ilImport.php:243
#8 ilImport:doImportObject in /var/www/html/Services/Export/classes/class.ilImport.php:136
#7 ilImport:importObject in /var/www/html/Modules/Test/classes/class.ilObjTestGUI.php:1436
#6 ilObjTestGUI:importVerifiedFileObject in /var/www/html/Modules/Test/classes/class.ilObjTestGUI.php:890
#5 ilObjTestGUI:executeCommand in /var/www/html/Services/UICore/classes/class.ilCtrl.php:118
#4 ilCtrl:forwardCommand in /var/www/html/Services/Repository/classes/class.ilRepositoryGUI.php:243
#3 ilRepositoryGUI:show in /var/www/html/Services/Repository/classes/class.ilRepositoryGUI.php:223
#2 ilRepositoryGUI:executeCommand in /var/www/html/Services/UICore/classes/class.ilCtrl.php:118
#1 ilCtrl:forwardCommand in /var/www/html/Services/UICore/classes/class.ilCtrl.php:91
#0 ilCtrl:callBaseClass in /var/www/html/ilias.php:24

I had no problems to import this test to the doil instance where I tested the Result-Presentation ;-)

[dsstrassner]

Second Remark:

I get no Button for the List of Questions? But for Test Pass Overview, en though that is not enabled at all:

And the setting for "Test pass Overview" has no functions except the sub-options could be activated and they work then.

[dsstrassner]

@matthiaskunkel has decided, how the translation and labels should be and changed that for trunk and ILIAS 8: https://mantis.ilias.de/view.php?id=26823#c95923

Please note this during implementation. Thanks in advance @nhaagen

[dsstrassner]

Hi @nhaagen could you please rebase and resolve the conflicts. After Max huge PR, I got now some errors, so I could not retest your changes. Thanks in advance.

[dsstrassner]

Opening the setting tab of a test:
ArgumentCountError thrown with message "Too few arguments to function ilTestQuestionSetConfigFactory::__construct(), 6 passed in /var/www/html/Modules/Test/classes/MainSettings/class.ilObjTestSettingsMainGUI.php on line 110 and exactly 7 expected"

Stacktrace:
#8 ArgumentCountError in /var/www/html/Modules/Test/classes/class.ilTestQuestionSetConfigFactory.php:33
#7 ilTestQuestionSetConfigFactory:__construct in /var/www/html/Modules/Test/classes/MainSettings/class.ilObjTestSettingsMainGUI.php:110
#6 ilObjTestSettingsMainGUI:__construct in /var/www/html/Modules/Test/classes/class.ilObjTestGUI.php:493
#5 ilObjTestGUI:executeCommand in /var/www/html/Services/UICore/classes/class.ilCtrl.php:118
#4 ilCtrl:forwardCommand in /var/www/html/Services/Repository/classes/class.ilRepositoryGUI.php:243
#3 ilRepositoryGUI:show in /var/www/html/Services/Repository/classes/class.ilRepositoryGUI.php:223
#2 ilRepositoryGUI:executeCommand in /var/www/html/Services/UICore/classes/class.ilCtrl.php:118
#1 ilCtrl:forwardCommand in /var/www/html/Services/UICore/classes/class.ilCtrl.php:91
#0 ilCtrl:callBaseClass in /var/www/html/ilias.php:24

And Create a question:

ArgumentCountError thrown with message "Too few arguments to function ilTestQuestionSetConfigFactory::__construct(), 6 passed in /var/www/html/Modules/TestQuestionPool/classes/class.assQuestionGUI.php on line 848 and exactly 7 expected"

Stacktrace:
#7 ArgumentCountError in /var/www/html/Modules/Test/classes/class.ilTestQuestionSetConfigFactory.php:33
#6 ilTestQuestionSetConfigFactory:__construct in /var/www/html/Modules/TestQuestionPool/classes/class.assQuestionGUI.php:848
#5 assQuestionGUI:saveReturn in /var/www/html/Modules/TestQuestionPool/classes/class.assQuestionGUI.php:241
#4 assQuestionGUI:executeCommand in /var/www/html/Services/UICore/classes/class.ilCtrl.php:118
#3 ilCtrl:forwardCommand in /var/www/html/Modules/TestQuestionPool/classes/class.ilObjQuestionPoolGUI.php:455
#2 ilObjQuestionPoolGUI:executeCommand in /var/www/html/Services/UICore/classes/class.ilCtrl.php:118
#1 ilCtrl:forwardCommand in /var/www/html/Services/UICore/classes/class.ilCtrl.php:91
#0 ilCtrl:callBaseClass in /var/www/html/ilias.php:24

I think here are also some commits missing.

[nhaagen]

With the latest commit, there is a super-small change to the UI-Interface: buttons->withUnavailableAction accepts a "false"-paramter as well.

[yvseiler]

I spoke briefly with @dsstrassner about the action menu of the question. I think it's right to change the dropdown into our UI dropdown component as you plan to do and live with both options being offered for now, even if only one can be executed.

I think it can be implemented temporarily in this way, because after executing the action, there is direct feedback (i.e. as a user, I see whether my answer or input has been deleted or not - if necessary, I repeat the step with the other action). If this problem can be eliminated as soon as possible afterwards, all the better (perhaps instead of greying out, the two actions should perhaps even replace each other).

Please consider the following suggestions:

• To make it clearer to users that the actions "Undo Editing" and "Delete Answer" are similar, I would change the order in the actions menu. 1. mark this question, 2. move question to the end, 3. TRENNLINIE, 4. Undo Editing, 5. Delete Answer

Best regards
Yvonne

[nhaagen]

[dsstrassner]

Hi @nhaagen, thank you for your patience. But now we have the situation, that "Delete Answers" does not work.
I have tested this with the following options activated / deactivated:

• Exam View
• Flagging Questions
• Show ‘Test Attempt Overview’

I could not delete a saved answer: Entering an answer, going to the next or previous question (to save the answer), going back and selecting "Delete Answer" has no effect. The answer(s) are not deleted.

The other things from this Feature Request work like a charm.

[nhaagen]

I rebased again and, for the sake of this PR, "fixed" the discardAnswer-action.
The action-handling, however, highly depends on JS - there probably will we more issues due to the recent changes of turning legacy buttons into UI Buttons (they lose classes/ids, so the JS will not find them), and the UI Buttons lack proper actions themeselves, e.g. here: https://github.com/ILIAS-eLearning/ILIAS/blame/22e1821d7cb03d9ab06afaa6e71006e25b1a9cca/Modules/Test/classes/class.ilTestPlayerAbstractGUI.php#L2311
This is something else, though, and I'd really like to tackle that detached from this PR/topic.

[dsstrassner]

hi @kergomard, from my point of view, you are ready to go ;)

[kergomard]

Thank you very much for this improvement @nhaagen !

Please answer the following questions:

• Do we really need withHighPriority() on all the modifications? For me this smells quite a bit.
• This looks overly complicated. I think this should be enough, right:

fn($id) => "document.getElementById('$id').addEventListener(
  'click',
  (event)=>{
    event.target.name = 'cmd[discardSolution]';
    event.target.form.requestSubmit(event.target);
  }
)"

I've a few small change request:

• Could you please rename the table row containing the settings for the Question List to something similar as we then use in the code? It took me a while to understand that the table column for 'usr_pass_overview_mode' is 'show summary'. I would suggest to really use the full name 'usr_pass_overview_mode'.
• Could you please use strict typing here
• Could you please move the namespaces here to a use statement.

This is just a note for the release notes (if not already on our radar): Question-List-Settings from previous ILIAS-Versions will not be imported correctly.

Thanks again and best!
@kergomard

[nhaagen]

Hi @kergomard ,
thanks for looking into this; I implemented the desire changes.
As for modification-priorities: since the provider checks on TEST_PLAYER_KIOSK_MODE_ENABLED, I do not really see negative side-effects; which ones would you feel to lower in priority? I'd imagine footer and title at most, but not really so.
Best regards,
Nils

[kergomard]

Hi @nhaagen
Thank you very much! I will look at it this afternoon.
For the Priorities: I think we shouldn't set them, if not absolutely necessary. ILIAS is extendable and the nice thing about the Modifications is that they allow so much flexibility. If we already remove this in our code, we make it harder for it to be extended and adapted to individual needs. ->withHighPriority() for me is the equivalent of !important in CSS: If you need it, it smells ;-).

A very real use case this caused me a lot of grieve is the SEB-Plugin.

Thanks and best,
@kergomard

[kergomard]

Hi @nhaagen

I just checked the rest (my afternoon started early ;-) ). Apart from the question of the Priorities, it is ready to be merged.

All checks fail, but this is unrelated.

Thanks again and best,
@kergomard

[nhaagen]

Unfortunately, we cannot drop Prios alltogether and lowPriority will already conflict; I used a const to go slightly above "low" and leave enough room for further modifications. This is certainly not ideal, but the best I can come up with.
ContributionalModifiers would be nice ;)

[kergomard]

Thank you very much @nhaagen !

I will merge this now as it is.

Best,
@kergomard

[yvseiler]

I'd like to add something to my comment above from Sep 27 that I've stumbled across and learned more about in the meantime. This has no effect on the merge and is just for the purpose of recording this in case it is useful later.

If this problem can be eliminated as soon as possible afterwards, all the better (perhaps instead of greying out, the two actions should perhaps even replace each other).

As a correction to my suggestion above: "Do not show" an option is not recommended for contextual menus, but "Disable" is. See https://www.nngroup.com/articles/contextual-menus/ : "Disable items that are irrelevant to the user's context. Instead of hiding irrelevant actions, disable them so that users don't have to try to figure out where menu items disappeared, or figure out how to get the system back into the right context to display a particular command."