Skip to content
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

Test: export options #8207

Merged

Conversation

chlulei
Copy link
Contributor

@chlulei chlulei commented Oct 17, 2024

This PR reimplements the non standard export options of Test and TestQuestionPool to conform with the new export infrastructure. This also includes changes to the ilTestExportGUI and ilObjQuestionPoolGUI. For details see the documentation.

The class ilTestExportTableGUI is no longer in use, instead the Test export tab uses the Export's standard implementation with a new KS DataTable.

The export options for Test are "ARC", "XML", "XMLResults", "Plugin", and for TestQuestionPool "XML" and "XLSX". The implementation is a bit more involved because of file handling, but mainly consists of reshuffled previously existing code. An exception to this is the export option "Plugin" for Test. It is no longer possible to dynamically adapt the available export options according to what plugins are installed, so the export process now has a second step where the plugin is selected.

@schmitz-ilias schmitz-ilias added improvement php Pull requests that update Php code labels Oct 17, 2024
@fhelfer
Copy link
Contributor

fhelfer commented Oct 24, 2024

I left some comments. There was one error that occurred as I checked out the code.
Besides that, there are just some minor improvements, that I commented.

Code looks good and works (in case someone comments out the numerous bugs currently in trunk)
This is a good improvement for TestExport, and from the perspective of refactoring the Export itself, this would be a good help having it for ILIAS 10

@kergomard If you don't see any issues, this can be merged

Best @fhelfer

@kergomard
Copy link
Contributor

Please don't yet. There will be a rather biggish PR that also changes things in Exports, so maybe it works afterwards, maybe it doesn't and it will most probably need rebasing.

@kergomard
Copy link
Contributor

Thank you for the pr @chlulei !

If I'm getting this right, we need this to make the exports work again in ILIAS 10. As we did quite some restructuring in exports, could you please check, what we still need and update the pr accordingly? I will then review swiftly.

Thanks and best,
@kergomard

chlulei and others added 2 commits October 25, 2024 11:47
… from export tab. Removed test question pool export option (excel) from export tab. Fixed file paths of test archive export option. Removed corresponding code from gui classes.
@chlulei
Copy link
Contributor Author

chlulei commented Oct 25, 2024

Hi @kergomard,
I removed the export options to generate participant results from the export tab and removed the code related to the plugin export option i added to the ilTestExportGUI. As far as i can see from your changes, those are not supposed to be in the export tab anymore. I also reverted relevant changes i did in ilObjQuestionPoolGUI.

I was able to test the changes in the Test export tab, but not in the export tab of the TestQuestionPool due to an unrelated error.

Copy link
Contributor

@kergomard kergomard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much @chlulei !

There are still conflicts, could you please resolve them?

Then:

  • I don't think we need this new class property, right?
  • I think this here is wrong, too. We do not have a questionpool-specific ExportGUI anymore.
    This is just a question:
  • Does every object-type need to implement an ExportOption, even if it is only just the default xml-export?

Thanks again and best,
@kergomard

…ilquestionpoolexportgui' to 'ilexportgui' in ilObjQuestionPoolGUI.
@chlulei
Copy link
Contributor Author

chlulei commented Oct 25, 2024

@kergomard,
regarding your Question:

No, this is only the case for Test and TestQuestionPool. Both components do not return a xml representation with ilTestExporter::getXmlRepresentation or ilTestQuestionPoolExporter::getXmlRepresentation, instead another export is created and saved in the data directory of Test/TestQuestionPool. The implemented export options are used to display the export files stored in these directories and to use the export procedure of Test and TestQuestionPool to create the exports.

@kergomard
Copy link
Contributor

Thank you very much for your explanation @chlulei !

We can hopefully get rid of this soon, then!
I was wondering why I can "Squash and Merge", but not "Rebase and Merge", now I know: You have a merge commit in the middle of it. Might I suggest to change your git settings, so that in the future you do not create a merge commit on pull, but create a proper rebase? This gives far cleaner commit histories.

This, I will squash and merge.

Thanks again and have a good weekend,
@kergomard

@kergomard kergomard merged commit 99c3688 into ILIAS-eLearning:trunk Oct 25, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement php Pull requests that update Php code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants