Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-32714

Assessment Form criteria not saved properly under Rubric grading strategy

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

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

        Gliffy Diagrams

        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
          mudrd8mz 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
          mudrd8mz 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
          strom@augsburg.edu 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
          strom@augsburg.edu 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
          mudrd8mz 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
          mudrd8mz 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
          strom@augsburg.edu 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
          strom@augsburg.edu 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
          strom@augsburg.edu 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
          strom@augsburg.edu 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
          salvetore Michael de Raadt added a comment -

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

          Show
          salvetore Michael de Raadt added a comment - It looks like there's action here already. Good stuff.
          Hide
          strom@augsburg.edu 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
          strom@augsburg.edu 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
          strom@augsburg.edu 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
          strom@augsburg.edu 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
          mudrd8mz 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
          mudrd8mz 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
          strom@augsburg.edu 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
          strom@augsburg.edu 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
          mudrd8mz 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
          mudrd8mz 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
          poltawski 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
          poltawski 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
          poltawski 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
          poltawski 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
          poltawski Dan Poltawski added a comment -

          Looks to be related to the simpletest mock

          Show
          poltawski Dan Poltawski added a comment - Looks to be related to the simpletest mock
          Hide
          poltawski 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
          poltawski 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
          mudrd8mz 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
          mudrd8mz 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
          poltawski 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
          poltawski 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
          poltawski 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
          poltawski 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
          poltawski Dan Poltawski added a comment -

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

          Show
          poltawski Dan Poltawski added a comment - (ps. david, of coruse shout if you want to fix it in a better way)
          Hide
          mudrd8mz 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
          mudrd8mz 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
          abgreeve 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
          abgreeve 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
          stronk7 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
          stronk7 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:
                Fix Release Date:
                9/Jul/12