Moodle
  1. Moodle
  2. MDL-42167

Coding error on submitting the assessment of an example submission

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.5.2
    • Fix Version/s: 2.5.3
    • Component/s: Workshop
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide

      Testing difficulty: Easy

      Testing environment: A course with at least two students enrolled.

      1. Make a new workshop instance. In the workshop settings form, set the "Overall feedback mode" to "Disabled" and "Mode of examples assessment" to "Examples must be assessed before own submission".
      2. Define a simple assessment form (the grading strategy not relevant here).
      3. Provide at least one example submission and its reference assessment.
      4. Switch the workshop to the submission phase.
      5. Log in as a student.
      6. TEST: Make sure you can assess the example submission without an error thrown.
      7. Submit something.
      8. Log in as a teacher or manager or admin.
      9. Allocate the submission to be assessed by another student and switch the workshop to the assessment phase.
      10. Log in as the student who was assigned as the reviewer of the submission.
      11. TEST: Make sure you can assess the submission without the database error thrown.

      Thanks for testing this!

      Show
      Testing difficulty: Easy Testing environment: A course with at least two students enrolled. Make a new workshop instance. In the workshop settings form, set the "Overall feedback mode" to "Disabled" and "Mode of examples assessment" to "Examples must be assessed before own submission". Define a simple assessment form (the grading strategy not relevant here). Provide at least one example submission and its reference assessment. Switch the workshop to the submission phase. Log in as a student. TEST: Make sure you can assess the example submission without an error thrown. Submit something. Log in as a teacher or manager or admin. Allocate the submission to be assessed by another student and switch the workshop to the assessment phase. Log in as the student who was assigned as the reviewer of the submission. TEST: Make sure you can assess the submission without the database error thrown. Thanks for testing this!
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-42167-workshop-update-record

      Description

      Full details, description and replication steps: https://moodle.org/mod/forum/discuss.php?d=240735#p1045730.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Matteo Scaramuccia added a comment -

            Here is a first rough patch that could be applied to the assessment.php file too.

            I doubt if my patch proposal could be seen as a workaround or it really addresses the required fix with no need for refactoring the current code: the update_record_raw method, e.g. on MySQL, acts triggering an exception when providing nothing but the id column since a long time.

            I'm available to extend it to the other branches as well as to fix the assessment.php file too, if the proposal sounds reasonable.

            Show
            Matteo Scaramuccia added a comment - Here is a first rough patch that could be applied to the assessment.php file too. I doubt if my patch proposal could be seen as a workaround or it really addresses the required fix with no need for refactoring the current code: the update_record_raw method, e.g. on MySQL, acts triggering an exception when providing nothing but the id column since a long time. I'm available to extend it to the other branches as well as to fix the assessment.php file too, if the proposal sounds reasonable.
            Hide
            David Mudrak added a comment -

            Well spotted and analysed Matteo Scaramuccia! You are completely right. This is a regression of MDL-37602 where the previous $DB->set_field() call was replaced by $DB->update_record() for Workshop in Moodle 2.5 and higher. I agree with your solution and will use you patch. I just want to go through other places where $DB->update_record() is used in the Workshop code to be sure we are not suffering this issue elsewhere.

            The essential step to reproduce this is to disable the feedback in the workshop settings, which is why we have not discovered this while testing.

            Well done again!

            Show
            David Mudrak added a comment - Well spotted and analysed Matteo Scaramuccia ! You are completely right. This is a regression of MDL-37602 where the previous $DB->set_field() call was replaced by $DB->update_record() for Workshop in Moodle 2.5 and higher. I agree with your solution and will use you patch. I just want to go through other places where $DB->update_record() is used in the Workshop code to be sure we are not suffering this issue elsewhere. The essential step to reproduce this is to disable the feedback in the workshop settings, which is why we have not discovered this while testing. Well done again!
            Hide
            David Mudrak added a comment -

            Done. I have reviewed all other usages of update_record() in the workshop and they should be correct. So I just extended Matteo's patch to cover both assessment.php and exassessment.php and am submitting it for the integration.

            Thanks Matteo for providing the patch.

            Show
            David Mudrak added a comment - Done. I have reviewed all other usages of update_record() in the workshop and they should be correct. So I just extended Matteo's patch to cover both assessment.php and exassessment.php and am submitting it for the integration. Thanks Matteo for providing the patch.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            Dan Poltawski added a comment -

            Integrated to master and 25 - thanks David/Matteo

            Show
            Dan Poltawski added a comment - Integrated to master and 25 - thanks David/Matteo
            Hide
            Dan Poltawski added a comment -

            Testing, testing. 123?

            Show
            Dan Poltawski added a comment - Testing, testing. 123?
            Hide
            Jason Fowler added a comment -

            Sorry Dan, no idea how I missed this. Will do it first thing in the morning.

            Show
            Jason Fowler added a comment - Sorry Dan, no idea how I missed this. Will do it first thing in the morning.
            Hide
            Dan Poltawski added a comment -

            No worries, thanks

            Show
            Dan Poltawski added a comment - No worries, thanks
            Hide
            Jason Fowler added a comment -

            All good David!

            Show
            Jason Fowler added a comment - All good David!
            Hide
            Dan Poltawski added a comment -

            Hurrah! Thanks for your contribution - this fix is part of Moodle.

            Show
            Dan Poltawski added a comment - Hurrah! Thanks for your contribution - this fix is part of Moodle.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: