This input does not support the option 'mul'.

[aharjula]

Back when units were migrated and the form lived a bit there were some existing questions that required an option named 'mul' for the inputs to return "unwrapped" values. However, it appears that the new input option system is unaware of this option (which is largely obsolete) and breaks questions which have it.

Could we maybe add that option to the allowed input options list? So we do not need to fix the materials as there really does not seem to be a simple way to do that...

For units input that is. Basically, this was the problems solution back then. And there should be a test for this still in the tests...

[aharjula]

Also these kinds of obsolete things would be nice to drop when saving/exporting/importting...

[sangwinc]

Sorry about that Matti.

I have to confess to having lost track of this a little...

I think the "mul" option has actually been factored out (and the units input no longer sends a stackunits object to the PRT) I missed this. Do we really want the above fix, or do we actually want legacy questions to break, so we can completely weed out this option?

[aharjula]

Well the option has been dead for quite some time indeed but it has been lingering in quite a few questions for quite some time and this sudden disabling of all inputs that have it came as a bit of a shock. Especially, as now we had questions loading up in quizzes (luckily not in live systems that I know of) that have parts of them disabled which makes this somewhat sneaky an issue.

Basically, we need a deprecation process with easy identification and preferably automatic fixing of questions that this applies to. So sure lets make "mul" go away but maybe give people some system level warning before it goes away? And if it can be handled automatically, fix questions during import/export and during STACK upgrades. Basically, this is the same issue as the versions in questions in that matrix-function case.

[aharjula]

Also are the questions broken by this even detectable by the question test system? I don't think it cares about seeing the input at all? Or does it actually care about the errors in input fields?

Luckily, in the case of Abacus I can probably just run UPDATE mdl_qtype_stack_inputs SET options = '' WHERE options = 'mul'; (in the case of Abacus only 255 questions and 555 inputs are affected by this) or some such but it does not fix materials being inputted from XML-files or other older systems.

[sangwinc]

Given the API/YAML things floating about in my mind, this might be a very good opportunity to refactor "question validaiton" code into a single method. At the same time, we need to have both an "error" and a "warning" system. Given a "question object" of raw types, we return two arrays, "errors" and "warnings", mapped to the question fields. This will be used by

1. standard moodle form (as now, with no change in beheviour for errors, but a warning block somewhere perhaps).
2. import of XML & YAML.
3. bulk check scripts over a whole collection of questions, just as the "fix delimiter" scripts.

I think this would be a very useful refactoring.

[timhunt]

That last part sounds good to me. Will also make it easier to unit test the validation logic.

[aharjula]

Single method validation would be nice, just to avoid having to find out or remember what needs to be validated, but do note that:

1. While validation of a question being saved/imported should naturally validate everything including settings related to inputs and question level CAS commands. Those are static things and once valid can be considered valid until STACK upgrades happen.
2. Do not ever do that when validating the students input as you are only wasting cycles to check things that have not changed. When working with student input validation you should only validate the input. Especially, as STACK is stateless you only need to deal with the input, not with any of the other CAS code in the question.

Currently, we validate and re-validate all the CAS commands (and now the input options too) in the question every time we get something to evaluate if we could just stop doing that we could save some cycles and maybe invest them to more complex validations of the actually changing content i.e. the student input. Also big bulk actions would surely complete faster e.g. re-grading of a quiz and so on.

So all fine to have that single question-model validation method, but I will be very disappointed if it ever appears somewhere in the call graph of input evaluation.

[aharjula]

I know that what I described would lead to a situation where an author side security hole like the ones closed last year would simply go through unless question-model validation get executed and acted on. I believe that as long as we provide a way of running that validation easily over everything we do not need to keep running it everywhere and can leave the responsibility of fixing questions to the users.

However, as that might be a bit too optimistic viewpoint I would suggest that we add a database field "valid" in the qtype_stack_options which will have three states "true"/"false"/"NULL" and if a question gets loaded with "NULL" it will be validated and the value will be stored to the database. "false" (errors present) will obviously lead to re-validation to generate the error messages for display and "true" (warnings may be present but we do not show them to the student) means that no validation is needed. The important bit is that we add an database call to the end of db/upgrade.php to the same place where we clear the cache to reset all STACK questions to valid = 'NULL' when STACK gets upgraded.

You may also use a four or even five value field for that to also keep track of questions that have warnings for fast listings of questions needing attention.

[sangwinc]

Thanks Matti,
You've raised a very important point here, and I will pause to consider it. Currently we "trust" the Moodle database because authors can't save invalid questions.
We won't be able to trust a stateless API, so these things will be validated.
I'll need to investigate what happens when you import XML etc. and think about how we fit all this together.

[aharjula]

Importing XML goes past the validation, of that I have experience. You can even bypass some things through that, nothing CAS security related though as all CAS commands are currently validated multiple times...

In any case "mul" and other things that have no effect should be warnings and should not affect the questions use.

[aharjula]

Back to the "mul" issue. What bothers me more (after some thinking) is that you are basically now rendering an error over just the input field and not triggering a real error that would kill the whole question and preferably the whole quiz. If it were a warning and in this case it probably should be then it should not block the use of the input and in the case of a warning not related to the student it should not be visible to the student.

Basically, disabling a part of a question makes no sense just kill the question if it has something broken or let it be if not. Throw an exception or something so that Moodle can catch it and switch to some error reporting mode, currently the error appears quite silently. When throwing exceptions try to add the name of the question in the exception so that finding it in the broken quiz does not require binary searching.

[sangwinc]

Over the weekend I've refactored the question validation code out of the edit_form file and into the questiontype.php class, using the $fromform datatype as discussed with Tim last week by email.

I've also started to add in some separate native multi-language support, so that the [[input:ans1]] tags can be included within the multlang spans. This will be a big improvement I think for question authors. The native language support will also be useful for the work I'm doing in the API branch, as will this separate validation code. I really hope I've not broken anything (like the database) here. It think this should have no significant differences.

I've also added in a $warnings system, as suggested by Matti above. We can use this to flag up issues like the "mul", and build a bulk testing script which issues errors and warnings without breaking questions.

[aharjula]

Nice to have things collected like that and once there is caching of that question validation result I'll be even happier. Especially, as validating that multilang feature will essentially require validation of all the CASText fields in the question to check if all of them have the same languages (maybe some logic to note xx vs. xx_simple, the latter might exist to expand the first in some cases) present. And that validation will not be cheap if you basically need to evaluate each lang-code found anywhere in the question for each segment including PRT node feedback to see what if anything comes out.

$fromform seems like a data-structure defining STACK although somewhat oddly in some places but translating to that from YAML/XML/JSON/DB/qtype_stack_question should be trivial and if we get caching no one will care whether they have to run that translation every now and then. Just decide that validate_fromform() is the validator and lets build some mappings (e.g. sans/tans in nodes will probably need to be moved around in the structure and so on) around that.

[sangwinc]

This latest commit returns error messages on each input field, which I think will be a substantial improvement now I'm moving away from form-based input options with dropdowns we need more substantial validation. This will also be essential for any YAML-based validation.