Moodle
  1. Moodle
  2. MDL-32714

Assessment Form criteria not saved properly under Rubric grading strategy

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.2
    • Fix Version/s: 2.1.7, 2.2.4
    • Component/s: Workshop
    • Labels:
    • Testing Instructions:
      Hide

      Testing difficulty: easy

      1. Create a new workshop activity with grading strategy set to "Rubric".
      2. Choose Edit assessment form from the Settings block
      3. Enter unique strings in description fields of each criterion
      4. Leave criteria levels empty in at least two criteria
      5. Save the assessment form
      6. TEST: Make sure the order and the criteria description are saved correctly

      Show
      Testing difficulty: easy 1. Create a new workshop activity with grading strategy set to "Rubric". 2. Choose Edit assessment form from the Settings block 3. Enter unique strings in description fields of each criterion 4. Leave criteria levels empty in at least two criteria 5. Save the assessment form 6. TEST: Make sure the order and the criteria description are saved correctly
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-32714-rubric
    • Rank:
      39668

      Description

      Content is not saved correctly in the assessment form of the workshop activity when "grading strategy: rubric" is used.
      Content in the description fields should be saved where they are entered and not overwrite existing content in the criterion 1 description field.

      Content from whichever highest number criteria field is used gets entered in (or replaces existing content in) the criteria description field number 1 and all other description fields are blank. (Example: string entered only in description field for criterion 3 shows in criteria 1 description field.)

      1. lib.php
        25 kB
        David Mudrak
      1. criterion 4 save and preview.png
        38 kB
      2. criterion saved again.png
        69 kB
      3. criterion text entered.png
        92 kB
      4. criterion text saved.png
        102 kB

        Activity

        Hide
        David Mudrak added a comment -

        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?

        Show
        David Mudrak added a comment - 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?
        Hide
        Eric Strom added a comment - - edited

        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.

        Show
        Eric Strom added a comment - - edited 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.
        Hide
        David Mudrak added a comment -

        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.

        Show
        David Mudrak added a comment - 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.
        Hide
        Eric Strom added a comment -

        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.

        Show
        Eric Strom added a comment - 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.
        Hide
        Eric Strom added a comment -

        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()

        Show
        Eric Strom added a comment - 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()
        Hide
        Michael de Raadt added a comment -

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

        Show
        Michael de Raadt added a comment - It looks like there's action here already. Good stuff.
        Hide
        Eric Strom added a comment -

        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.

        Show
        Eric Strom added a comment - 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.
        Hide
        Eric Strom added a comment -

        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.

        Show
        Eric Strom added a comment - 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.
        Hide
        David Mudrak added a comment -

        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.

        Show
        David Mudrak added a comment - 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.
        Hide
        Eric Strom added a comment -

        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?

        Show
        Eric Strom added a comment - 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?
        Hide
        David Mudrak added a comment -

        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).

        Show
        David Mudrak added a comment - 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).
        Hide
        Dan Poltawski added a comment -

        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?

        Show
        Dan Poltawski added a comment - 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?
        Hide
        Dan Poltawski added a comment -

        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

        Show
        Dan Poltawski added a comment - 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
        Hide
        Dan Poltawski added a comment -

        Looks to be related to the simpletest mock

        Show
        Dan Poltawski added a comment - Looks to be related to the simpletest mock
        Hide
        Dan Poltawski added a comment -

        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

        Show
        Dan Poltawski added a comment - 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
        Hide
        David Mudrak added a comment -

        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.

        Show
        David Mudrak added a comment - 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.
        Hide
        Dan Poltawski added a comment -

        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)

        Show
        Dan Poltawski added a comment - 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)
        Hide
        Dan Poltawski added a comment -

        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.

        Show
        Dan Poltawski added a comment - 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.
        Hide
        Dan Poltawski added a comment -

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

        Show
        Dan Poltawski added a comment - (ps. david, of coruse shout if you want to fix it in a better way)
        Hide
        David Mudrak added a comment -

        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.

        Show
        David Mudrak added a comment - 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.
        Hide
        Adrian Greeve added a comment -

        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.

        Show
        Adrian Greeve added a comment - 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.
        Hide
        Eloy Lafuente (stronk7) added a comment -

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

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

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - U P S T R E A M I Z E D ! Many thanks for the hard work, closing this as fixed. Ciao

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: