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 Master Branch:

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

        Gliffy Diagrams

          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: