Moodle
  1. Moodle
  2. MDL-35175

Lesson shows no attempts if associated with a grouping

    Details

    • Testing Instructions:
      Hide
      1. Enable experimental admin setting enablegroupmembersonly
      2. Create a group/grouping with students
      3. Create lesson and set that lesson to only be visible to users in the created grouping
      4. As a student in the created grouping, attempt the lesson activity
      5. As a teacher, VERIFY that viewing the report page will display the attempt that was made.
      Show
      Enable experimental admin setting enablegroupmembersonly Create a group/grouping with students Create lesson and set that lesson to only be visible to users in the created grouping As a student in the created grouping, attempt the lesson activity As a teacher, VERIFY that viewing the report page will display the attempt that was made.
    • Workaround:
      Hide

      Do not use groupings with lessons.

      Show
      Do not use groupings with lessons.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
    • Pull Master Branch:
    • Rank:
      43808

      Description

      A lesson associated with a grouping will have the report.php display that no attempts were made. But there are results in the lesson_attempts table and the gradebook.

      The problem is because of an incorrect query for groupings and groups.

      Replication steps:

      1. Create a group/grouping with students
      2. Create lesson and associate that lesson with the created grouping
      3. As a student in the created grouping, attempt the lesson activity
      4. View the report page for the lesson

      Expected result: The report page will display the attempt that was made.

      Actual result: A message is displayed "No attempts have been made on this lesson."

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Hi, Rex.

          I've just added you to the jira-developers group. I don't think this changes what you are capable of in Tracker, but it recognises the work you are doing. You can still edit issues, assign them and push to peer review.

          I've assigned this issue to you. Please push it to peer review. If you'd rather not do this, please reassign to moodle.com and we will take care of it at some point.

          I've noticed that you have recently reported a number of issues and provided patches. I will process them in the same way.

          Show
          Michael de Raadt added a comment - Hi, Rex. I've just added you to the jira-developers group. I don't think this changes what you are capable of in Tracker, but it recognises the work you are doing. You can still edit issues, assign them and push to peer review. I've assigned this issue to you. Please push it to peer review. If you'd rather not do this, please reassign to moodle.com and we will take care of it at some point. I've noticed that you have recently reported a number of issues and provided patches. I will process them in the same way.
          Hide
          Rex Lorenzo added a comment - - edited

          Michael, I appreciate the comments. I am reassigning this to moodle.com, because I don't have a clue as to who to ask for a peer review.

          Show
          Rex Lorenzo added a comment - - edited Michael, I appreciate the comments. I am reassigning this to moodle.com, because I don't have a clue as to who to ask for a peer review.
          Hide
          Dan Poltawski added a comment -

          Hi Rex,

          You can keep yourself assigned and just change the status to 'waiting for peer review' and we'll pick it up. (Eventually, not always as quick as we'd like..)

          Show
          Dan Poltawski added a comment - Hi Rex, You can keep yourself assigned and just change the status to 'waiting for peer review' and we'll pick it up. (Eventually, not always as quick as we'd like..)
          Hide
          Dan Poltawski added a comment -

          The fix makes sense to me, Thanks Rex. I'm pushing this for integration review.

          It'd be great to have a few more testing steps, particularly testing groups vs groupings if possible.

          Show
          Dan Poltawski added a comment - The fix makes sense to me, Thanks Rex. I'm pushing this for integration review. It'd be great to have a few more testing steps, particularly testing groups vs groupings if possible.
          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
          Eloy Lafuente (stronk7) added a comment -

          Hi Rex,

          I know this sounds ridiculous... but "gg.groupingid = :groupingid" is not a JOIN condition, but a WHERE one. I know results are the same in this case (inner join), but I think it's better to put it in the correct place.

          Also, the correct JOIN condition "gm.groupid=gg.groupid", please put some spaces around the "=", give you are going to edit that line.

          Finally, note that 21_STABLE is under security fixes support only, so this won't be integrated there.

          So reopening, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Rex, I know this sounds ridiculous... but "gg.groupingid = :groupingid" is not a JOIN condition, but a WHERE one. I know results are the same in this case (inner join), but I think it's better to put it in the correct place. Also, the correct JOIN condition "gm.groupid=gg.groupid", please put some spaces around the "=", give you are going to edit that line. Finally, note that 21_STABLE is under security fixes support only, so this won't be integrated there. So reopening, thanks!
          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
          Chris Follin added a comment -

          Rex's fix in this issue is nearly exactly the same as the fix in MDL-35358, but in a different file. To Eloy's suggestion of moving "gg.groupingid = :groupingid" to WHERE instead of JOIN, it was added to JOIN in 35358 and it passed integration.

          We'd like to move forward with this but we will need clarification of whether the fix should be consistent with MDL-35358 or should deviate from it as suggested by Eloy.

          Show
          Chris Follin added a comment - Rex's fix in this issue is nearly exactly the same as the fix in MDL-35358 , but in a different file. To Eloy's suggestion of moving "gg.groupingid = :groupingid" to WHERE instead of JOIN, it was added to JOIN in 35358 and it passed integration. We'd like to move forward with this but we will need clarification of whether the fix should be consistent with MDL-35358 or should deviate from it as suggested by Eloy.
          Hide
          Michael de Raadt added a comment -

          I'm bumping this issue. It seems to be close to a fix. Rex, could you have another look in light of Chris's comments?

          Show
          Michael de Raadt added a comment - I'm bumping this issue. It seems to be close to a fix. Rex, could you have another look in light of Chris's comments?
          Hide
          Rex Lorenzo added a comment -

          Sorry for the really long delay in addressing to the issues in the integration review. I have made the changes as specified and added a new branch for Moodle 2.4 and dropped 2.1.

          Show
          Rex Lorenzo added a comment - Sorry for the really long delay in addressing to the issues in the integration review. I have made the changes as specified and added a new branch for Moodle 2.4 and dropped 2.1.
          Hide
          Dan Poltawski added a comment -

          Sorry Rex, somehow this was missed by me. I'm pushing for integration - if could rebase since it seems to have beens ome time.

          Show
          Dan Poltawski added a comment - Sorry Rex, somehow this was missed by me. I'm pushing for integration - if could rebase since it seems to have beens ome time.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Silly question, which should be the behavior for attempts made before the grouping was applied? I bet that attempts belonging to users out from the grouping will not be shown.... uhm...

          .. that leads me to the problem of controlling when one activity grouping can be set or no. Sure we should perform some checks (or warn) when doing so.

          But that's another issue, your fix seems correct. +1

          Show
          Eloy Lafuente (stronk7) added a comment - Silly question, which should be the behavior for attempts made before the grouping was applied? I bet that attempts belonging to users out from the grouping will not be shown.... uhm... .. that leads me to the problem of controlling when one activity grouping can be set or no. Sure we should perform some checks (or warn) when doing so. But that's another issue, your fix seems correct. +1
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sorry, have to fail this:

          • 24_STABLE does not apply clear. Seems it can be cherry-picked from master, but, there is also
          • 23_STABLE is wrong. The SQL sentence is not ended and also the "ORDER BY u.lastname" has been "eaten".

          Reopening... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Sorry, have to fail this: 24_STABLE does not apply clear. Seems it can be cherry-picked from master, but, there is also 23_STABLE is wrong. The SQL sentence is not ended and also the "ORDER BY u.lastname" has been "eaten". Reopening... ciao
          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
          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
          Rex Lorenzo added a comment -

          Rebased all branches and fixed issue with 22/23 branches that were mentioned.

          Show
          Rex Lorenzo added a comment - Rebased all branches and fixed issue with 22/23 branches that were mentioned.
          Hide
          Dan Poltawski added a comment -

          Hi Rex, Feel free to keep yourself assignee - I get notified as peer reviewer

          Show
          Dan Poltawski added a comment - Hi Rex, Feel free to keep yourself assignee - I get notified as peer reviewer
          Hide
          Dan Poltawski added a comment -

          Hi Rex,

          Eloy's comment about the ORDER BY is still there on 2.3: "The SQL sentence is not ended and also the "ORDER BY u.lastname" has been "eaten"."

          The order by clause seems to be lost in this branch.

          (BTW we are only supporting security fixes on 2.2, so a fix wont be integrated for that).

          Show
          Dan Poltawski added a comment - Hi Rex, Eloy's comment about the ORDER BY is still there on 2.3: "The SQL sentence is not ended and also the "ORDER BY u.lastname" has been "eaten"." The order by clause seems to be lost in this branch. (BTW we are only supporting security fixes on 2.2, so a fix wont be integrated for that).
          Hide
          Rex Lorenzo added a comment -

          Dan, so very terribly sorry. Was very sloppy in trying to fix problems with my cherry-picking my fix for 2.4+ into 2.3-.

          I fixed the issue with the 2.3 patch and removed the 2.2 pull URL.

          Show
          Rex Lorenzo added a comment - Dan, so very terribly sorry. Was very sloppy in trying to fix problems with my cherry-picking my fix for 2.4+ into 2.3-. I fixed the issue with the 2.3 patch and removed the 2.2 pull URL.
          Hide
          Dan Poltawski added a comment -

          No problem at all, thanks!

          Show
          Dan Poltawski added a comment - No problem at all, thanks!
          Hide
          Damyon Wiese added a comment -

          I'm a little confused by this - the lesson module does not support groupings - is this a local customisation you have made?

          Regards, Damyon

          Show
          Damyon Wiese added a comment - I'm a little confused by this - the lesson module does not support groupings - is this a local customisation you have made? Regards, Damyon
          Hide
          Damyon Wiese added a comment -

          Hi Rex,

          I'm taking this out of the integration queue this week - can you please respond to my question about the lesson not supporting groupings?

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - Hi Rex, I'm taking this out of the integration queue this week - can you please respond to my question about the lesson not supporting groupings? Thanks, Damyon
          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
          Chris Follin added a comment -

          Hi, Damyon. Lessons can be restricted to groupings in the Lesson settings. The experimental "enablegroupmembersonly" setting needs to be enabled and then when editing the Lesson settings, you need to have advanced features shown to see the Grouping option under Common Module Settings.

          Show
          Chris Follin added a comment - Hi, Damyon. Lessons can be restricted to groupings in the Lesson settings. The experimental "enablegroupmembersonly" setting needs to be enabled and then when editing the Lesson settings, you need to have advanced features shown to see the Grouping option under Common Module Settings.
          Hide
          Rex Lorenzo added a comment -

          Chris, thanks for answering Damyon's question. I would like to have this issue pushed back into integration, unless there is another question?

          Show
          Rex Lorenzo added a comment - Chris, thanks for answering Damyon's question. I would like to have this issue pushed back into integration, unless there is another question?
          Hide
          Damyon Wiese 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.

          Thanks!

          Show
          Damyon Wiese 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. Thanks!
          Hide
          Damyon Wiese added a comment -

          Thanks Rex - sorry the delay last week, the testing instructions threw me. Have made them a bit clearer - the patch looks good.

          This has been pushed to master, 24 and 23 branches.

          Show
          Damyon Wiese added a comment - Thanks Rex - sorry the delay last week, the testing instructions threw me. Have made them a bit clearer - the patch looks good. This has been pushed to master, 24 and 23 branches.
          Hide
          Frédéric Massart added a comment -

          Failing for consideration. If there is not question in my lesson, then there won't be any report. If that's unrelated, or expected, please pass the test as it worked as described.

          Show
          Frédéric Massart added a comment - Failing for consideration. If there is not question in my lesson, then there won't be any report. If that's unrelated, or expected, please pass the test as it worked as described.
          Hide
          Rex Lorenzo added a comment -

          I would say that is expected behavior and not the bug this patch is fixing.

          Show
          Rex Lorenzo added a comment - I would say that is expected behavior and not the bug this patch is fixing.
          Hide
          Damyon Wiese added a comment -

          I think that this has nothing to do with the patch and it should pass if this is the only issue.

          Show
          Damyon Wiese added a comment - I think that this has nothing to do with the patch and it should pass if this is the only issue.
          Hide
          Frédéric Massart added a comment -

          Thanks, passing.

          Show
          Frédéric Massart added a comment - Thanks, passing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Because

          A
          MARVELOUS
          A       U
          Z  YOU  P
          I  ARE  E
          N  PPL  R
          G       B
            TNKS! 
          

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Because A MARVELOUS A U Z YOU P I ARE E N PPL R G B TNKS! Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: