Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.7, 2.5.2, 2.6
    • Fix Version/s: 2.5.5, 2.6.2
    • Component/s: Lesson
    • Labels:
    • Testing Instructions:
      Hide
      1. Create a lesson with "Available for group members only" unset. Add an essay to the lesson.
      2. Add 2 users, "User 1" and "User 2" to the course. Login as both users and submit an assignment.
      3. Log back in as admin. Create a group with only User 1 in it, and add to a grouping. Change the lesson settings to "Available for group members only" and use this grouping.
      4. Go to the "Grade essays" tab. You should not see the error.
      Show
      Create a lesson with "Available for group members only" unset. Add an essay to the lesson. Add 2 users, "User 1" and "User 2" to the course. Login as both users and submit an assignment. Log back in as admin. Create a group with only User 1 in it, and add to a grouping. Change the lesson settings to "Available for group members only" and use this grouping. Go to the "Grade essays" tab. You should not see the error.
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull 2.6 Branch:
      MDL-42495-m26
    • Pull Master Branch:

      Description

      On going to the "Grade essays" tab on the lesson, it is possible to get a coding error under certain circumstances. The error is accompanied by the following notice:

      Notice: Undefined offset: 73371 in /moodle/web/mod/lesson/essay.php on line 330 Call Stack: 0.0026 968608 1. {main}() /moodle/web/mod/lesson/essay.php:0 Notice: Undefined offset: 73371 in /moodle/web/mod/lesson/essay.php on line 375 Call Stack: 0.0026 968608 1. {main}() /moodle/web/mod/lesson/essay.php:0
      

      The circumstances seem to be

      1. the lesson is limited to group members only
      2. there is an essay submission from someone who is not a group member in the target grouping

      To reproduce:

      1. Create a lesson with "Available for group members only" unset
      2. Add 2 users, "User 1" and "User 2" to the course. Login as both users and submit an assignment.
      3. Log back in as admin. Create a group with only User 1 in it, and add to a grouping. Change the lesson settings to "Available for group members only" and use this grouping.
      4. Go to the "Grade essays" tab. You should see the error.

      (NB: This error does not appear unless there is at least one submission by someone who is in a group in the chosen grouping)

      Even though the steps above describe a not particularly sensible scenario, where someone changes the target grouping after submission has started, I think (although I haven't tested this) that the same should happen if someone submits an essay assignment and then is removed from a group, which is a more reasonable scenario.

        Gliffy Diagrams

          Activity

          Hide
          Michael Aherne added a comment -

          I have 2 possible patches for this, but am checking with one of our academic users to see what behaviour he would expect in the given scenario.

          Show
          Michael Aherne added a comment - I have 2 possible patches for this, but am checking with one of our academic users to see what behaviour he would expect in the given scenario.
          Hide
          Michael de Raadt added a comment -

          Thanks for working on this Michael.

          Show
          Michael de Raadt added a comment - Thanks for working on this Michael.
          Hide
          Michael Aherne added a comment -

          Added a patch for this.

          Show
          Michael Aherne added a comment - Added a patch for this.
          Hide
          Andrew Davis added a comment -

          Looks like a good change.

          The only alteration I would suggest you consider is adding a debugging() call within that if block. That will cause a debug message to be displayed on development sites without affecting production sites. I am just worried about what will happen if someone somehow breaks the loading of users. This safety check will give the appearance of the code working but with potentially catastrophic performance on large sites. If this safety check is going to the database it would be beneficial to inform the developer.

          Oh, one more very picky thing. Add a full-stop to the end of the comment. http://docs.moodle.org/dev/Coding_style#Inline_comments

          Submit for integration when you are ready.

          Show
          Andrew Davis added a comment - Looks like a good change. The only alteration I would suggest you consider is adding a debugging() call within that if block. That will cause a debug message to be displayed on development sites without affecting production sites. I am just worried about what will happen if someone somehow breaks the loading of users. This safety check will give the appearance of the code working but with potentially catastrophic performance on large sites. If this safety check is going to the database it would be beneficial to inform the developer. Oh, one more very picky thing. Add a full-stop to the end of the comment. http://docs.moodle.org/dev/Coding_style#Inline_comments Submit for integration when you are ready.
          Hide
          Michael Aherne added a comment - - edited

          Thanks for the feedback Andrew. I've made the changes you suggested, but I don't have enough Tracker privileges to submit for integration. Would you mind doing it?

          Show
          Michael Aherne added a comment - - edited Thanks for the feedback Andrew. I've made the changes you suggested, but I don't have enough Tracker privileges to submit for integration. Would you mind doing it?
          Hide
          Damyon Wiese added a comment -

          Hi Michael,

          Thanks for the patch - but I don't think this is the correct solution. It would be far better in this case to just remove the " if (!empty($cm->groupingid))

          { ...}

          " block a few lines above, than re-fetching users that were excluded by that query. This will show attempts by users not in this group - (the same as what your patch does).

          Please update the patch and re-submit it.

          Thankyou!

          Show
          Damyon Wiese added a comment - Hi Michael, Thanks for the patch - but I don't think this is the correct solution. It would be far better in this case to just remove the " if (!empty($cm->groupingid)) { ...} " block a few lines above, than re-fetching users that were excluded by that query. This will show attempts by users not in this group - (the same as what your patch does). Please update the patch and re-submit it. Thankyou!
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Michael Aherne added a comment -

          Hi Damyon, thanks for the feedback! I've changed the patch to do as you suggested and it's working well. It's much better than having a lot of extra database calls, which was bothering me a bit with the original patch.

          Show
          Michael Aherne added a comment - Hi Damyon, thanks for the feedback! I've changed the patch to do as you suggested and it's working well. It's much better than having a lot of extra database calls, which was bothering me a bit with the original patch.
          Hide
          CiBoT added a comment -

          Results for MDL-42495

          • Remote repository: git://github.com/micaherne/moodle.git
          Show
          CiBoT added a comment - Results for MDL-42495 Remote repository: git://github.com/micaherne/moodle.git Remote branch MDL-42495 -m26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/344 Warning: The MDL-42495 -m26 branch at git://github.com/micaherne/moodle.git has not been rebased recently. Details: http://integration.moodle.org/job/Precheck%20remote%20branch/344/artifact/work/smurf.html Remote branch MDL-42495 -m to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/345 Warning: The MDL-42495 -m branch at git://github.com/micaherne/moodle.git has not been rebased recently. Details: http://integration.moodle.org/job/Precheck%20remote%20branch/345/artifact/work/smurf.html
          Hide
          Andrew Davis added a comment -

          Submitting for integration.

          Show
          Andrew Davis added a comment - Submitting for integration.
          Hide
          Michael Aherne added a comment -

          Thanks, Andrew!

          Show
          Michael Aherne added a comment - Thanks, Andrew!
          Hide
          Damyon Wiese added a comment -

          Thanks Michael, this has been integrated to 25, 26 and master (I cherry-picked to 25 after confirming the bug exists there as well).

          Show
          Damyon Wiese added a comment - Thanks Michael, this has been integrated to 25, 26 and master (I cherry-picked to 25 after confirming the bug exists there as well).
          Hide
          Jérôme Mouneyrac added a comment -

          All good thanks.

          Show
          Jérôme Mouneyrac added a comment - All good thanks.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I won't be saying "Thanks!" this week. I'm tired of it.

          For the good (and the bad), your code is now part of Moodle, the best LMS in the world. Hope you are contributing for that to continue being a fact (and not the opposite), sincerely.

          Just closing this as fixed, ciao

          PS: Just a bit of black/cruel humor, sorry, LOL!

          Show
          Eloy Lafuente (stronk7) added a comment - I won't be saying "Thanks!" this week. I'm tired of it. For the good (and the bad), your code is now part of Moodle, the best LMS in the world. Hope you are contributing for that to continue being a fact (and not the opposite), sincerely. Just closing this as fixed, ciao PS: Just a bit of black/cruel humor, sorry, LOL!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: