Assessment Form criteria not saved properly under Rubric grading strategy

Hi Eric. Thanks for the report. I just tried to reproduce and the editor works as expected to me. I must admit the description is not 100% clear to me. Could you please attach some screenshots that illustrate this?

Just tested again. Looks like the behavior isn't consistently the way I described but it certainly isn't saving correctly in any case for me so far. See screen shots. Notice the text when saved is flipped between the criterion 1 and criterion 2 description fields. Also, the level descriptions I entered under criterion 1 showed up in criterion 2 instead also.

Wow. I never saw that and it has never been reported yet. I just tried again but I can't get the same behavior. Here at my laptop, criteria are saved and displayed in the same order as in the editing mode...

Will be happy if you provide more findings. Please increase the debugging level to the maximum so we know all eventual error messages and warnings.

I entered data in criterion 3 and replaced data in criterion 2. Criterion 2 data saved accurately, but data in criterion description 3 went to criterion 1 description field instead.

Debugging output below.

When saving the form:
Did you remember to make the first column something unique in your call to get_records? Duplicate value '' found in column 'lid'.
line 818 of /lib/dml/mysqli_native_moodle_database.php: call to debugging()
line 428 of /mod/workshop/form/rubric/lib.php: call to mysqli_native_moodle_database->get_records_sql()
line 101 of /mod/workshop/form/rubric/lib.php: call to workshop_rubric_strategy->load_fields()
line 1031 of /mod/workshop/locallib.php: call to workshop_rubric_strategy->__construct()
line 50 of /mod/workshop/editform.php: call to workshop->grading_strategy_instance()
Did you remember to make the first column something unique in your call to get_records? Duplicate value '' found in column 'lid'.
line 818 of /lib/dml/mysqli_native_moodle_database.php: call to debugging()
line 428 of /mod/workshop/form/rubric/lib.php: call to mysqli_native_moodle_database->get_records_sql()
line 101 of /mod/workshop/form/rubric/lib.php: call to workshop_rubric_strategy->load_fields()
line 1031 of /mod/workshop/locallib.php: call to workshop_rubric_strategy->__construct()
line 50 of /mod/workshop/editform.php: call to workshop->grading_strategy_instance()
Did you remember to make the first column something unique in your call to get_records? Duplicate value '' found in column 'lid'.
line 818 of /lib/dml/mysqli_native_moodle_database.php: call to debugging()
line 428 of /mod/workshop/form/rubric/lib.php: call to mysqli_native_moodle_database->get_records_sql()
line 101 of /mod/workshop/form/rubric/lib.php: call to workshop_rubric_strategy->load_fields()
line 1031 of /mod/workshop/locallib.php: call to workshop_rubric_strategy->__construct()
line 50 of /mod/workshop/editform.php: call to workshop->grading_strategy_instance()
Skip to main content
This page should automatically redirect. If nothing is happening please use the continue link below.
(Continue)
Error output, so disabling automatic redirect.

After viewing the saved form:
Did you remember to make the first column something unique in your call to get_records? Duplicate value '' found in column 'lid'.
line 818 of /lib/dml/mysqli_native_moodle_database.php: call to debugging()
line 428 of /mod/workshop/form/rubric/lib.php: call to mysqli_native_moodle_database->get_records_sql()
line 101 of /mod/workshop/form/rubric/lib.php: call to workshop_rubric_strategy->load_fields()
line 1031 of /mod/workshop/locallib.php: call to workshop_rubric_strategy->__construct()
line 50 of /mod/workshop/editform.php: call to workshop->grading_strategy_instance()
Did you remember to make the first column something unique in your call to get_records? Duplicate value '' found in column 'lid'.
line 818 of /lib/dml/mysqli_native_moodle_database.php: call to debugging()
line 428 of /mod/workshop/form/rubric/lib.php: call to mysqli_native_moodle_database->get_records_sql()
line 101 of /mod/workshop/form/rubric/lib.php: call to workshop_rubric_strategy->load_fields()
line 1031 of /mod/workshop/locallib.php: call to workshop_rubric_strategy->__construct()
line 50 of /mod/workshop/editform.php: call to workshop->grading_strategy_instance()
Did you remember to make the first column something unique in your call to get_records? Duplicate value '' found in column 'lid'.
line 818 of /lib/dml/mysqli_native_moodle_database.php: call to debugging()
line 428 of /mod/workshop/form/rubric/lib.php: call to mysqli_native_moodle_database->get_records_sql()
line 101 of /mod/workshop/form/rubric/lib.php: call to workshop_rubric_strategy->load_fields()
line 1031 of /mod/workshop/locallib.php: call to workshop_rubric_strategy->__construct()
line 50 of /mod/workshop/editform.php: call to workshop->grading_strategy_instance()
Did you remember to make the first column something unique in your call to get_records? Duplicate value '' found in column 'lid'.
line 818 of /lib/dml/mysqli_native_moodle_database.php: call to debugging()
line 428 of /mod/workshop/form/rubric/lib.php: call to mysqli_native_moodle_database->get_records_sql()
line 101 of /mod/workshop/form/rubric/lib.php: call to workshop_rubric_strategy->load_fields()
line 1031 of /mod/workshop/locallib.php: call to workshop_rubric_strategy->__construct()
line 50 of /mod/workshop/editform.php: call to workshop->grading_strategy_instance()

It looks like there's action here already. Good stuff.

David, thanks for jumping into this so quickly from the get go. Is there any more details I can provide that might be helpful?
Added another screen shot documenting the behavior. Text in Criterion #4 description field gets saved to the description #1 field.

Just adding confirmation for me that this behavior is ONLY happening with the grading strategy set to rubric. Assessment forms when the workshop uses other grading strategies (Accumulative grading, Comments, and Number of errors) are saving appropriately.

Eric, thanks for providing the debugging message. It was really helpful and I believe I found the buggy code. I was finally able to reproduce this weird behaviour once there are some criteria left with no levels defined.

I am attaching a file containing the fix for this. Can you please try and put the attached lib.php to your mod/workshop/form/rubric/ folder (please make sure you have backup of the current lib.php).

Please let me know if this lib.php fixes the issue so we can put the patch upstream.

The lib replacement appears to fix the issue with saving text in criterion fields. Great work! Thank you, David.

Being new to the workshop activity I'm exploring the details as I have time. This morning it came to me that the rubric grading method process in the workshop should? be the same process as the Advanced grading rubric system used in other assignments. Do you have a sense as to why it uses something different? Is the rubric (advanced grading) method integration on the project road map for the workshop module?

Submitting for integration. Please see the commit message for the description and explanation.

Fix version to be set to 2.1.7 and 2.2.4 (not available in JIRA yet).

Thanks David, that has been integrated now.

Random piece of commentary which is unimportant: I prefer SQL in single quotes in sql because it makes it clearer there is no variable insertion into the string. Do you prefer double quotes?

Failing this.

Seems to be breaking the build with unit tests:

PHP Fatal error: Call to a member function close() on a non-object in /Users/Shared/Jenkins/Home/git_repositories/MOODLE_21_STABLE/mod/workshop/form/rubric/lib.php on line 448

Looks to be related to the simpletest mock

To be clear, its fine on master. Just seems to be a problem with the simpletest mock with get_recordset_sql.

Reversing the recordset change back to get_records_sql on the stable branches fixes this, but that is not ideal - I don't know how easy it is to fix the mock. So leaving this for the experts to come online

Hi Dan. Thanks for the review and the test. Do I read it correctly that there is a problem in the unit test and not in the code itself?

WRT double quotes - it was discussed at http://moodle.org/mod/forum/discuss.php?d=130249 and even though most folks (including yourself) were against it, I found it more comfortable for several reasons (subquery variables expansion and easy syntax highlighting). IIRC, most of our SQL statements actually use double quotes. Anyway, it's really just a matter of personal preference IMHO.

David: yes - seems like the DB mock is not supporting recordsets and failing.

(Double quotes, heh thanks that is a blast from the past! and agree about preference)

We wanted to fix the build to prevent other errors from being masked, so Eloy and I have come up with a hacky solution to this. The ugly part is mocking a recordset. completionlib does its own thing implementing in a more elaborate way.

But this is all gone in master, so I suppose we shouldn't spend too much time.

(ps. david, of coruse shout if you want to fix it in a better way)

Thanks Dan for fixing the $DB->expectOnce() test in unittest, I totally forgot to execute them prior to submitting the patch. I am completely OK with the hack. To be honest, things like $DB->expectOnce() should not be actually used in the unit test (unless there is good reason for it) as they are too tied to implementation details of the tested method.

Tested in master and observed the problem.
Tested in version 2.1, 2.2 and master integration and found no problems.
Everything is saving as it should.
Thanks.

U P S T R E A M I Z E D !

Many thanks for the hard work, closing this as fixed.

Ciao