Moodle
  1. Moodle
  2. MDL-21415

Course Participation Report doesn't abide by separate groups

    Details

    • Story Points:
      20
    • Rank:
      173
    • Sprint:
      BACKEND Sprint 4

      Description

      In 1.97 (haven't tested other versions), the participation report shows all students to a non-editing teacher in a group.

      My courses are set to separate groups, and permissions for Non-editing Teacher for showallgroups are set to prevent. When I log in as a non-editing teacher I set up, and go to the Course reports, click the participation report, choose an activity, and then set "Show only" to Student, all student show up.

      Shouldn't the participation report also respect separate groups?

      1. index.php.patch
        4 kB
        Patrick Pollet
      2. index.php.patch
        4 kB
        Patrick Pollet
      1. screenshot_023.png
        26 kB

        Issue Links

          Activity

          Hide
          Neusa Buss added a comment -

          My Moodle 2.0.2 has this problem...
          How I can solve it ?
          Thanks, Neusa

          Show
          Neusa Buss added a comment - My Moodle 2.0.2 has this problem... How I can solve it ? Thanks, Neusa
          Hide
          Patrick Pollet added a comment -

          patch to course/report/participation/index.php

          Show
          Patrick Pollet added a comment - patch to course/report/participation/index.php
          Hide
          Patrick Pollet added a comment -

          patch to course/report/participation/index.php

          Show
          Patrick Pollet added a comment - patch to course/report/participation/index.php
          Hide
          Patrick Pollet added a comment -

          please find enclosed a patch to course/report/participation/index.php that add IN MOODLE 1.9.X a group selector in course participation reports

          Cheers.

          Show
          Patrick Pollet added a comment - please find enclosed a patch to course/report/participation/index.php that add IN MOODLE 1.9.X a group selector in course participation reports Cheers.
          Hide
          James Pengelly added a comment -

          This is still an issue in 2.2.1 and probably also affects the activity report (/report/outline/index.php) and live log (/report/loglive/index.php). The code at /report/participation/index.php is very different to the patch so I'm hoping someone will take this on and ideally incorporate it into a standard version.

          Show
          James Pengelly added a comment - This is still an issue in 2.2.1 and probably also affects the activity report (/report/outline/index.php) and live log (/report/loglive/index.php). The code at /report/participation/index.php is very different to the patch so I'm hoping someone will take this on and ideally incorporate it into a standard version.
          Hide
          Helen Foster added a comment - - edited

          Adding 2.3.1 as affects version and noting recent discussion about not being able to filter the participation report by groups http://moodle.org/mod/forum/discuss.php?d=208919

          Show
          Helen Foster added a comment - - edited Adding 2.3.1 as affects version and noting recent discussion about not being able to filter the participation report by groups http://moodle.org/mod/forum/discuss.php?d=208919
          Hide
          Ankit Agarwal added a comment -

          Sending for review.
          Will cherry-pick to other branches after the review.
          Thanks

          Show
          Ankit Agarwal added a comment - Sending for review. Will cherry-pick to other branches after the review. Thanks
          Hide
          Rossiani Wijaya added a comment -

          Hi Ankit,

          The patch looks great.

          I had discussion with Ankit regarding the necessity to re-set $currentgroup to null (line 74-76).

          In my opinion, the check and resetting $currentgroup is unnecessary. However the same check is also done in /user/index.php line 124-126, and it seems it is used there to avoid query failure.

          For this issue, the check is not breaking anything. So I will leave it to Ankit/Integrator to decide.

          [Y] Syntax
          [Y] Output
          [Y] Whitespace
          [-] Language
          [-] Databases
          [Y] Testing
          [-] Security
          [-] Documentation
          [Y] Git
          [Y] Sanity check

          Show
          Rossiani Wijaya added a comment - Hi Ankit, The patch looks great. I had discussion with Ankit regarding the necessity to re-set $currentgroup to null (line 74-76). In my opinion, the check and resetting $currentgroup is unnecessary. However the same check is also done in /user/index.php line 124-126, and it seems it is used there to avoid query failure. For this issue, the check is not breaking anything. So I will leave it to Ankit/Integrator to decide. [Y] Syntax [Y] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check
          Hide
          Ankit Agarwal added a comment -

          Thanks for the review Rosie,
          I have removed those two lines as indeed they were not required.
          sending for integration.
          Thanks

          Show
          Ankit Agarwal added a comment - Thanks for the review Rosie, I have removed those two lines as indeed they were not required. sending for integration. Thanks
          Hide
          Dan Poltawski 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
          Dan Poltawski 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 -

          Hi Ankit,

          I am no groups expert, but there are a few things which don't seem right to me:

          • The course group setting is more of a template for activity settings. If the group mode is not forced at the course level, then this setting could effectively be meaningless. (i.e. no activites are in a group mode)
          • So the important point seems to be the ability to display the groups dropdown if required (i.e. limit by the different course groups if you have accessallgroups), or only see the groups you have access to if you don't have the permission to view all groups.
          • Looking at groups_get_course_group(), I don't think that your $isseparategroups check is required, because groups_get_course_group() does the groupmode/capability check for us
          • In fact the main point seems to be whether the groups mode is enabled or not?

          Anyway i'm reopening this so that we can further discuss and clarify what is expected behaviour here, and because it looks to me like your $isseperategroups check is redundant.

          thanks

          Dan

          Show
          Dan Poltawski added a comment - Hi Ankit, I am no groups expert, but there are a few things which don't seem right to me: The course group setting is more of a template for activity settings. If the group mode is not forced at the course level, then this setting could effectively be meaningless. (i.e. no activites are in a group mode) So the important point seems to be the ability to display the groups dropdown if required (i.e. limit by the different course groups if you have accessallgroups), or only see the groups you have access to if you don't have the permission to view all groups. Looking at groups_get_course_group(), I don't think that your $isseparategroups check is required, because groups_get_course_group() does the groupmode/capability check for us In fact the main point seems to be whether the groups mode is enabled or not? Anyway i'm reopening this so that we can further discuss and clarify what is expected behaviour here, and because it looks to me like your $isseperategroups check is redundant. thanks Dan
          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
          Ankit Agarwal added a comment - - edited
          Capability(access all groups) Course Group mode forced user part of a group What to show
          TRUE sep TRUE TRUE show everything
          TRUE sep TRUE FALSE show everything
          TRUE sep FALSE TRUE show everything
          TRUE sep FALSE FALSE show everything
          TRUE visible TRUE TRUE show everything
          TRUE visible TRUE FALSE show everything
          TRUE visible FALSE TRUE show everything
          TRUE visible FALSE FALSE show everything
          TRUE nogroup TRUE TRUE show everything
          TRUE nogroup TRUE FALSE show everything
          TRUE nogroup FALSE TRUE show everything
          TRUE nogroup FALSE FALSE show everything
          FALSE sep TRUE TRUE show group only
          FALSE sep TRUE FALSE show nothing
          FALSE sep FALSE TRUE show group only
          FALSE sep FALSE FALSE show nothing
          FALSE visible TRUE TRUE show everything
          FALSE visible TRUE FALSE show everything
          FALSE visible FALSE TRUE show nothing
          FALSE visible FALSE FALSE show nothing
          FALSE nogroup TRUE TRUE show everything
          FALSE nogroup TRUE FALSE show everything
          FALSE nogroup FALSE TRUE show nothing
          FALSE nogroup FALSE FALSE show nothing
          Show
          Ankit Agarwal added a comment - - edited Capability(access all groups) Course Group mode forced user part of a group What to show TRUE sep TRUE TRUE show everything TRUE sep TRUE FALSE show everything TRUE sep FALSE TRUE show everything TRUE sep FALSE FALSE show everything TRUE visible TRUE TRUE show everything TRUE visible TRUE FALSE show everything TRUE visible FALSE TRUE show everything TRUE visible FALSE FALSE show everything TRUE nogroup TRUE TRUE show everything TRUE nogroup TRUE FALSE show everything TRUE nogroup FALSE TRUE show everything TRUE nogroup FALSE FALSE show everything FALSE sep TRUE TRUE show group only FALSE sep TRUE FALSE show nothing FALSE sep FALSE TRUE show group only FALSE sep FALSE FALSE show nothing FALSE visible TRUE TRUE show everything FALSE visible TRUE FALSE show everything FALSE visible FALSE TRUE show nothing FALSE visible FALSE FALSE show nothing FALSE nogroup TRUE TRUE show everything FALSE nogroup TRUE FALSE show everything FALSE nogroup FALSE TRUE show nothing FALSE nogroup FALSE FALSE show nothing
          Hide
          Martin Dougiamas added a comment -

          Thanks Ankit. I just went through the table and that makes sense to me as a good compromise between functionality and complexity.

          Show
          Martin Dougiamas added a comment - Thanks Ankit. I just went through the table and that makes sense to me as a good compromise between functionality and complexity.
          Hide
          Helen Foster added a comment -

          Just noting from a chat with Ankit, that we need an error message for the cases

          FALSE visible FALSE TRUE show nothing
          FALSE visible FALSE FALSE show nothing

          FALSE nogroup FALSE TRUE show nothing
          FALSE nogroup FALSE FALSE show nothing

          We could re-use the error message:

          'Sorry, but you do not currently have permissions to do that (View report).'

          and add info the docs about how to solve it (either allow the access all groups capability or force group mode in the course settings).

          Alternatively, we could have a more specific error message, such as

          'The course settings for groups, together with your current permissions, mean that you cannot view this report.'

          with a 'More information about this error' link to the docs.

          I don't suggest that we mention forcing group mode in the error message, as this seems a very weird way to solve the problem.

          Show
          Helen Foster added a comment - Just noting from a chat with Ankit, that we need an error message for the cases FALSE visible FALSE TRUE show nothing FALSE visible FALSE FALSE show nothing FALSE nogroup FALSE TRUE show nothing FALSE nogroup FALSE FALSE show nothing We could re-use the error message: 'Sorry, but you do not currently have permissions to do that (View report).' and add info the docs about how to solve it (either allow the access all groups capability or force group mode in the course settings). Alternatively, we could have a more specific error message, such as 'The course settings for groups, together with your current permissions, mean that you cannot view this report.' with a 'More information about this error' link to the docs. I don't suggest that we mention forcing group mode in the error message, as this seems a very weird way to solve the problem.
          Hide
          Ankit Agarwal added a comment -

          Thanks for the feedback Helen, Martin.
          I have update the patch. Requesting another peer-review.
          Thanks

          Show
          Ankit Agarwal added a comment - Thanks for the feedback Helen, Martin. I have update the patch. Requesting another peer-review. Thanks
          Hide
          Rossiani Wijaya added a comment -

          Hi Ankit,

          The patch looks great.

          [y] Syntax
          [-] Output
          [y] Whitespace
          [-] Language
          [y] Databases
          [-] Testing
          [-] Security
          [-] Documentation
          [y] Git
          [y] Sanity check

          Feel free to submit for integration review.

          Show
          Rossiani Wijaya added a comment - Hi Ankit, The patch looks great. [y] Syntax [-] Output [y] Whitespace [-] Language [y] Databases [-] Testing [-] Security [-] Documentation [y] Git [y] Sanity check Feel free to submit for integration review.
          Hide
          Ankit Agarwal added a comment -

          Thanks for the review Rosie.
          Submitting for integration
          Thanks

          Show
          Ankit Agarwal added a comment - Thanks for the review Rosie. Submitting for integration Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sorry I'm reopening this.

          Or I'm wrong or this change will lead to problems easily. Just imagine this situation, I think it's really, really common:

          • course without groups at all, nor groupmodeforce, obviously.
          • role xxx with 'moodle/site:accessallgroups' not allowed.

          Under that situation, it's clear that this piece of code:

          if (($course->groupmode == VISIBLEGROUPS || $course->groupmode == NOGROUPS) &&  $course->groupmodeforce == 0
                  && !has_capability('moodle/site:accessallgroups', $context)) {
              // Things are too complicated, we can't decide what to show and what not. So better show nothing.
              // Refer MDL-21415.
              print_error('badgroupsetting', 'error', new moodle_url('/course/view.php', array('id' => $PAGE->course->id)));
          }
          

          Is leading to a wrong error message. How the hell is that incorrect/complex?

          Also, we need constant ways to show the errors/messages to the user. It has no sense to use $OUTPUT->heading() for some errors and print_error() for others.

          Note I've not looked deeply. The use case above was enough to switch on all the alarms in my head. Don't we have other places (participants list...) where we are handling all these combinations properly?

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Sorry I'm reopening this. Or I'm wrong or this change will lead to problems easily. Just imagine this situation, I think it's really, really common: course without groups at all, nor groupmodeforce, obviously. role xxx with 'moodle/site:accessallgroups' not allowed. Under that situation, it's clear that this piece of code: if (($course->groupmode == VISIBLEGROUPS || $course->groupmode == NOGROUPS) && $course->groupmodeforce == 0 && !has_capability('moodle/site:accessallgroups', $context)) { // Things are too complicated, we can't decide what to show and what not. So better show nothing. // Refer MDL-21415. print_error('badgroupsetting', 'error', new moodle_url('/course/view.php', array('id' => $PAGE->course->id))); } Is leading to a wrong error message. How the hell is that incorrect/complex? Also, we need constant ways to show the errors/messages to the user. It has no sense to use $OUTPUT->heading() for some errors and print_error() for others. Note I've not looked deeply. The use case above was enough to switch on all the alarms in my head. Don't we have other places (participants list...) where we are handling all these combinations properly? 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
          Eloy Lafuente (stronk7) added a comment -

          Summary, after chatting with Ankit.

          1) The goal(s) of this issue is to:

          • Add one "group-selector" drop-down to the report, allowing to pick all or individual groups. Only if there are groups.
          • Prevent disclosing information not allowed to the current user.

          2) So this involves handling available groups at 2 different levels (course and activity) and, if done together, can lead to problematic combinations.

          3) There are 2 ways to solve this:

          a) Make the "group-selector" to be recalculated each time the "Activity selector" is picked.
          b) Continue showing the course-level groups in the selector and delay the "recalculation" after the form is submitted.

          While a) is the optimal solution (the user won't be able to pick an incorrect activity/group combination), it also will need to be verified after the form is sent, so surely b) is enough for now, leading to error, when the combination is not allowed.

          So I think the correct way is to take rid of the trouble-check currently evaluated only at course level, and create one new check once the form has been submitted, so we know both the picked activity and group and we can verify if the user has perms to see that group information there (groups_get_activity_allowed_groups() perhaps)

          Hope this helps, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Summary, after chatting with Ankit. 1) The goal(s) of this issue is to: Add one "group-selector" drop-down to the report, allowing to pick all or individual groups. Only if there are groups. Prevent disclosing information not allowed to the current user. 2) So this involves handling available groups at 2 different levels (course and activity) and, if done together, can lead to problematic combinations. 3) There are 2 ways to solve this: a) Make the "group-selector" to be recalculated each time the "Activity selector" is picked. b) Continue showing the course-level groups in the selector and delay the "recalculation" after the form is submitted. While a) is the optimal solution (the user won't be able to pick an incorrect activity/group combination), it also will need to be verified after the form is sent, so surely b) is enough for now, leading to error, when the combination is not allowed. So I think the correct way is to take rid of the trouble-check currently evaluated only at course level, and create one new check once the form has been submitted, so we know both the picked activity and group and we can verify if the user has perms to see that group information there (groups_get_activity_allowed_groups() perhaps) Hope this helps, ciao
          Hide
          Martin Dougiamas added a comment -

          Surely that bug mentioned above is easy to fix, but I would agree that if it's not TOOO much effort to get a better result as you described here, Eloy, then it's worth doing.

          I'm just a little worried that most of the exceptions are extremely rare cases and it's going to take a long time to get it perfect. But I'll leave an estimate to Ankit who knows the code better.

          Show
          Martin Dougiamas added a comment - Surely that bug mentioned above is easy to fix, but I would agree that if it's not TOOO much effort to get a better result as you described here, Eloy, then it's worth doing. I'm just a little worried that most of the exceptions are extremely rare cases and it's going to take a long time to get it perfect. But I'll leave an estimate to Ankit who knows the code better.
          Hide
          Ankit Agarwal added a comment -

          I have put this issue in my backlog for next sprint, as am going on a vacation. Will work on this after I come back.
          Thanks

          Show
          Ankit Agarwal added a comment - I have put this issue in my backlog for next sprint, as am going on a vacation. Will work on this after I come back. Thanks
          Hide
          Ankit Agarwal added a comment - - edited

          I have implement the new patch with the correct approach as discussed (point 1-3b from Eloy's comment). It was not very complicated for participation report as here we deal only with one activity at a time. I might need a bit of effort in other reports where multiple activities are handled at the same time.
          Here is the chart showing current approach.

          Capability(access all groups) Course Group mode forced user part of a group What to show
          TRUE sep TRUE TRUE show everything
          TRUE sep TRUE FALSE show everything
          TRUE sep FALSE TRUE show everything
          TRUE sep FALSE FALSE show everything
          TRUE visible TRUE TRUE show everything
          TRUE visible TRUE FALSE show everything
          TRUE visible FALSE TRUE show everything
          TRUE visible FALSE FALSE show everything
          TRUE nogroup TRUE TRUE show everything
          TRUE nogroup TRUE FALSE show everything
          TRUE nogroup FALSE TRUE show everything
          TRUE nogroup FALSE FALSE show everything
          FALSE sep TRUE TRUE show course level user groups only, After selection of activity, report displayed for the selected group if the user still have access to it based on activity group settings
          FALSE sep TRUE FALSE show nothing
          FALSE sep FALSE TRUE show course level user groups only, After selection of activity, report displayed for the selected group if the user still have access to it based on activity group settings
          FALSE sep FALSE FALSE show nothing
          FALSE visible TRUE TRUE show course level user groups only, After selection of activity, report displayed for the selected group if the user still have access to it based on activity group settings
          FALSE visible TRUE FALSE show nothing
          FALSE visible FALSE TRUE show course level user groups only, After selection of activity, report displayed for the selected group if the user still have access to it based on activity group settings
          FALSE visible FALSE FALSE show nothing
          FALSE nogroup TRUE TRUE show course level user groups only, After selection of activity, report displayed for the selected group if the user still have access to it based on activity group settings
          FALSE nogroup TRUE FALSE show nothing
          FALSE nogroup FALSE TRUE show course level user groups only, After selection of activity, report displayed for the selected group if the user still have access to it based on activity group settings
          FALSE nogroup FALSE FALSE show nothing

          I think the patch covers all of these situations. Requesting a review.

          Show
          Ankit Agarwal added a comment - - edited I have implement the new patch with the correct approach as discussed (point 1-3b from Eloy's comment). It was not very complicated for participation report as here we deal only with one activity at a time. I might need a bit of effort in other reports where multiple activities are handled at the same time. Here is the chart showing current approach. Capability(access all groups) Course Group mode forced user part of a group What to show TRUE sep TRUE TRUE show everything TRUE sep TRUE FALSE show everything TRUE sep FALSE TRUE show everything TRUE sep FALSE FALSE show everything TRUE visible TRUE TRUE show everything TRUE visible TRUE FALSE show everything TRUE visible FALSE TRUE show everything TRUE visible FALSE FALSE show everything TRUE nogroup TRUE TRUE show everything TRUE nogroup TRUE FALSE show everything TRUE nogroup FALSE TRUE show everything TRUE nogroup FALSE FALSE show everything FALSE sep TRUE TRUE show course level user groups only, After selection of activity, report displayed for the selected group if the user still have access to it based on activity group settings FALSE sep TRUE FALSE show nothing FALSE sep FALSE TRUE show course level user groups only, After selection of activity, report displayed for the selected group if the user still have access to it based on activity group settings FALSE sep FALSE FALSE show nothing FALSE visible TRUE TRUE show course level user groups only, After selection of activity, report displayed for the selected group if the user still have access to it based on activity group settings FALSE visible TRUE FALSE show nothing FALSE visible FALSE TRUE show course level user groups only, After selection of activity, report displayed for the selected group if the user still have access to it based on activity group settings FALSE visible FALSE FALSE show nothing FALSE nogroup TRUE TRUE show course level user groups only, After selection of activity, report displayed for the selected group if the user still have access to it based on activity group settings FALSE nogroup TRUE FALSE show nothing FALSE nogroup FALSE TRUE show course level user groups only, After selection of activity, report displayed for the selected group if the user still have access to it based on activity group settings FALSE nogroup FALSE FALSE show nothing I think the patch covers all of these situations. Requesting a review.
          Hide
          Rossiani Wijaya added a comment -

          Hi Ankit,

          Patch looks fine.

          [y] Syntax
          [-] Output
          [y] Whitespace
          [-] Language
          [-] Databases
          [-] Testing
          [-] Security
          [-] Documentation
          [y] Git
          [y] Sanity check

          Feel free to resubmit.

          Show
          Rossiani Wijaya added a comment - Hi Ankit, Patch looks fine. [y] Syntax [-] Output [y] Whitespace [-] Language [-] Databases [-] Testing [-] Security [-] Documentation [y] Git [y] Sanity check Feel free to resubmit.
          Hide
          Ankit Agarwal added a comment -

          Pushing for integration.
          Thanks

          Show
          Ankit Agarwal added a comment - Pushing for integration. Thanks
          Hide
          Dan Poltawski 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
          Dan Poltawski 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
          Damyon Wiese added a comment - - edited

          Thanks Ankit,

          This code is duplicating logic from the groups library and will eventually lead to bugs.

          For example:

          // Group security checks.
          +    $cmgroups = groups_get_activity_allowed_groups($cm);
          +    $cmgroupmode = groups_get_activity_groupmode($cm, $course);
          +    $isseparategroups = ($cmgroupmode == SEPARATEGROUPS and !has_capability('moodle/site:accessallgroups', context_module::instance($cm->id)));
          +    if ($isseparategroups && !array_key_exists($currentgroup, $cmgroups)) {
          +        // The user is not in the group so show message and exit.
          +        echo $OUTPUT->heading(get_string("notingroup"));
          +        echo $OUTPUT->footer();
          +        exit;
          +    }
          

          Should just be (tweaked to handle NO_GROUPS):

          // Group security checks.
          +    if (groups_get_activity_groupmode($cm)) {
          +        $cmgroups = groups_get_activity_allowed_groups($cm);
          +        if (!array_key_exists($currentgroup, $cmgroups)) {
          +            // The user is not in the group so show message and exit.
          +            echo $OUTPUT->heading(get_string("notingroup"));
          +            echo $OUTPUT->footer();
          +            exit;
          +        }
          +    }
          

          groups_get_activity_allowed_groups already checks the activity group mode and the accessallgroups capability - and really any code that is checking "accessallgroups" capability outside of lib/grouplib.php is incorrect.

          Even the check for "notingroup" at the top of the page should be removed IMO - if the user is not in the group, there will just be nothing in the report and the flexible table will print the 'nothingtodisplay' message.

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - - edited Thanks Ankit, This code is duplicating logic from the groups library and will eventually lead to bugs. For example: // Group security checks. + $cmgroups = groups_get_activity_allowed_groups($cm); + $cmgroupmode = groups_get_activity_groupmode($cm, $course); + $isseparategroups = ($cmgroupmode == SEPARATEGROUPS and !has_capability('moodle/site:accessallgroups', context_module::instance($cm->id))); + if ($isseparategroups && !array_key_exists($currentgroup, $cmgroups)) { + // The user is not in the group so show message and exit. + echo $OUTPUT->heading(get_string( "notingroup" )); + echo $OUTPUT->footer(); + exit; + } Should just be (tweaked to handle NO_GROUPS): // Group security checks. + if (groups_get_activity_groupmode($cm)) { + $cmgroups = groups_get_activity_allowed_groups($cm); + if (!array_key_exists($currentgroup, $cmgroups)) { + // The user is not in the group so show message and exit. + echo $OUTPUT->heading(get_string( "notingroup" )); + echo $OUTPUT->footer(); + exit; + } + } groups_get_activity_allowed_groups already checks the activity group mode and the accessallgroups capability - and really any code that is checking "accessallgroups" capability outside of lib/grouplib.php is incorrect. Even the check for "notingroup" at the top of the page should be removed IMO - if the user is not in the group, there will just be nothing in the report and the flexible table will print the 'nothingtodisplay' message. Thanks, Damyon
          Hide
          Ankit Agarwal added a comment -

          Hi Damyon,
          Summarising our discussions:-

          1. The first "notingroup" check was to bail out early to prevent fetching all the module info. But as you suggested it doesn't make much difference in performance I have removed it.
          2. I have changed the check to groups_get_activity_groupmode($cm) != NOGroups and removed the cap check. Although I still think that groups_get_activity_allowed_groups is not designed properly to indicate NOGROUPS mode. Atleast the phpdocs must explain clearly the behaviour and say that it should never be called in No Groups mode.
            Pushing back for integration.
            Thanks
          Show
          Ankit Agarwal added a comment - Hi Damyon, Summarising our discussions:- The first "notingroup" check was to bail out early to prevent fetching all the module info. But as you suggested it doesn't make much difference in performance I have removed it. I have changed the check to groups_get_activity_groupmode($cm) != NOGroups and removed the cap check. Although I still think that groups_get_activity_allowed_groups is not designed properly to indicate NOGROUPS mode. Atleast the phpdocs must explain clearly the behaviour and say that it should never be called in No Groups mode. Pushing back for integration. Thanks
          Hide
          Damyon Wiese added a comment -

          +1 for backport - the current behaviour sounds like a bug to me.

          Show
          Damyon Wiese added a comment - +1 for backport - the current behaviour sounds like a bug to me.
          Hide
          Ankit Agarwal added a comment -

          backported.
          Thanks

          Show
          Ankit Agarwal added a comment - backported. Thanks
          Hide
          Damyon Wiese added a comment -

          Looking again, thanks Ankit.

          Show
          Damyon Wiese added a comment - Looking again, thanks Ankit.
          Hide
          Damyon Wiese added a comment -

          Actually on reflection - this is a ui_change and so is more of an improvement so should not be backported - will integrate just the master branch (sorry for the wrong call above).

          Show
          Damyon Wiese added a comment - Actually on reflection - this is a ui_change and so is more of an improvement so should not be backported - will integrate just the master branch (sorry for the wrong call above).
          Hide
          Damyon Wiese added a comment -

          I found another problem.

          Using "groups_print_course_menu" does not work for this issue.

          E.g. Course is set to NO_GROUPS - but activity is set to SEPARATE_GROUPS. Using the current code $currentgroup will get set to false and errors are shown on the page - I think you need to call groups_get_all_groups($course->id) and build your own menu (or add an api call to the groupslib just for this case). A similar issue exists when the default groupingid for the course is set differently to the groupingid for an activity.

          Show
          Damyon Wiese added a comment - I found another problem. Using "groups_print_course_menu" does not work for this issue. E.g. Course is set to NO_GROUPS - but activity is set to SEPARATE_GROUPS. Using the current code $currentgroup will get set to false and errors are shown on the page - I think you need to call groups_get_all_groups($course->id) and build your own menu (or add an api call to the groupslib just for this case). A similar issue exists when the default groupingid for the course is set differently to the groupingid for an activity.
          Hide
          Ankit Agarwal added a comment -

          If we use groups_get_all_groups($course->id), that will clearly be leak in information, as the user can see all groups which he didn’t even know existed.
          We cannot do groups_get_all_groups($course->id, $userid) for the same reasons as we cannot use "groups_print_course_menu".

          Show
          Ankit Agarwal added a comment - If we use groups_get_all_groups($course->id), that will clearly be leak in information, as the user can see all groups which he didn’t even know existed. We cannot do groups_get_all_groups($course->id, $userid) for the same reasons as we cannot use "groups_print_course_menu".
          Hide
          Damyon Wiese added a comment -

          Sure Ankit we can keep the groups_print_course_menu (that is consistent with Eloys comment above) - but current code still is throwing an error when the course group mode is NOGROUPS.

          Show
          Damyon Wiese added a comment - Sure Ankit we can keep the groups_print_course_menu (that is consistent with Eloys comment above) - but current code still is throwing an error when the course group mode is NOGROUPS.
          Hide
          Damyon Wiese added a comment -

          Hi Ankit,

          I got Dans opinion on this also and we agree that it is worth adding a function to grouplib for this case as it will be useful elsewhere. Inside grouplib - you don't have to stick to the API - so you can do whatever $course->groupmode checks etc you like - but the point is that we do this in one place instead of spread out through the code.

          Feel free to work out the best function for the job: My suggestion is something like:

          function groups_group_visible($course, $cm=null, $userid=null)

          because it matches the style of:

          function groups_course_module_visible($cm, $userid=null)

          Sorry for the back and forth - but improving the groups api is worthwhile.

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Hi Ankit, I got Dans opinion on this also and we agree that it is worth adding a function to grouplib for this case as it will be useful elsewhere. Inside grouplib - you don't have to stick to the API - so you can do whatever $course->groupmode checks etc you like - but the point is that we do this in one place instead of spread out through the code. Feel free to work out the best function for the job: My suggestion is something like: function groups_group_visible($course, $cm=null, $userid=null) because it matches the style of: function groups_course_module_visible($cm, $userid=null) Sorry for the back and forth - but improving the groups api is worthwhile. Regards, 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
          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
          Ankit Agarwal added a comment - - edited

          Summary:-

          1. I have implemented 3(b) as agreed(refer Eloy's comment above).
          2. 3(a) can be implemented once we resolve MDL-20589
          3. All group visiblity checks are now done in groups_group_visible() that I created as part of MDL-40354
          4. group_print_course_menu() couldn't be used as it didn't print all the groups, and changing the api using a fourth parameter was making the api code very hacky. so I have added a new api groups_print_course_menu_allgroups().
          5. submitting for review to get more feedback on the approach, I will put up some unit tests after the initial review.
          Show
          Ankit Agarwal added a comment - - edited Summary:- I have implemented 3(b) as agreed(refer Eloy's comment above). 3(a) can be implemented once we resolve MDL-20589 All group visiblity checks are now done in groups_group_visible() that I created as part of MDL-40354 group_print_course_menu() couldn't be used as it didn't print all the groups, and changing the api using a fourth parameter was making the api code very hacky. so I have added a new api groups_print_course_menu_allgroups(). submitting for review to get more feedback on the approach, I will put up some unit tests after the initial review.
          Hide
          Damyon Wiese added a comment -

          Thanks Ankit,

          This latest patch looks like a good solution. I agree with the approach and the code looks nice.

          If you can add some unit tests this will be +1 for integration from me.

          Cheers, Damyon

          Show
          Damyon Wiese added a comment - Thanks Ankit, This latest patch looks like a good solution. I agree with the approach and the code looks nice. If you can add some unit tests this will be +1 for integration from me. Cheers, Damyon
          Hide
          Ankit Agarwal added a comment -

          Hi Damyon,
          Thanks for the review. Added tests. And also created MDL-41465.

          Thanks

          Show
          Ankit Agarwal added a comment - Hi Damyon, Thanks for the review. Added tests. And also created MDL-41465 . Thanks
          Hide
          Damyon Wiese added a comment -

          Just reviewing the tests - one minor point is that I don't like "assertNotEmpty(strpos(..."

          • assertContains is cleaner and will report better errors if there is a fail.

          Everything else looks great.

          Show
          Damyon Wiese added a comment - Just reviewing the tests - one minor point is that I don't like "assertNotEmpty(strpos(..." assertContains is cleaner and will report better errors if there is a fail. Everything else looks great.
          Hide
          Ankit Agarwal added a comment -

          Thanks Damyon, updated. Submitting for integration now.

          Show
          Ankit Agarwal added a comment - Thanks Damyon, updated. Submitting for integration now.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Uhm… so with this patch:

          1) All the groups are shown always in the menu?
          2) The "notingroup" error is shown if the teacher picks an "incorrect" one?

          I don't think 1) is correct, really (groups should not be public always IMO). But I'm not strong enough to reject this because of that. Please discuss if 1) is acceptable always.

          And also, tiny detail… I don't like the optional_param() within the function at all. It should be passed via parameter from the caller.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Uhm… so with this patch: 1) All the groups are shown always in the menu? 2) The "notingroup" error is shown if the teacher picks an "incorrect" one? I don't think 1) is correct, really (groups should not be public always IMO). But I'm not strong enough to reject this because of that. Please discuss if 1) is acceptable always. And also, tiny detail… I don't like the optional_param() within the function at all. It should be passed via parameter from the caller. Ciao
          Hide
          Chris Baldwin added a comment -

          I agree with Eloy - in our context we don't want all teachers to be able to see all groups - only those that they are members of. We have hundreds of groups per course so it makes it very hard to scroll through. Not a great user journey if there's the option to click on a group that they're not in & get an error.

          Chris

          Show
          Chris Baldwin added a comment - I agree with Eloy - in our context we don't want all teachers to be able to see all groups - only those that they are members of. We have hundreds of groups per course so it makes it very hard to scroll through. Not a great user journey if there's the option to click on a group that they're not in & get an error. Chris
          Hide
          Dan Poltawski added a comment -

          Sorry Ankit!

          I think Eloy makes a good point and so here is another bit of influence that we want to send this round for another iteration.

          Show
          Dan Poltawski added a comment - Sorry Ankit! I think Eloy makes a good point and so here is another bit of influence that we want to send this round for another iteration.
          Hide
          Damyon Wiese added a comment -

          Thanks Eloy - it looks like we could check for accessallgroups and if not then pass the user id to groups_get_all_groups to only get the users groups.

          Can you look at this please Ankit?

          Show
          Damyon Wiese added a comment - Thanks Eloy - it looks like we could check for accessallgroups and if not then pass the user id to groups_get_all_groups to only get the users groups. Can you look at this please Ankit?
          Hide
          Ankit Agarwal added a comment - - edited

          We cannot do that. Since the course group mode can be Visible, in which case the user should be able to see all groups. Or even if course groupmode is separate but not forced, the activity groupmode can be visible, in which case again the user should be allowed to see all groups. That was the reason we decided not to go with groups_print_course_menu() in the first place.

          Optional_param can be made as an extra param if needed. I tried to keep the signature of groups_print_course_menu_allgroups() close to groups_print_course_menu(), but if that is not what we want, I can change the api.

          Thanks

          Show
          Ankit Agarwal added a comment - - edited We cannot do that. Since the course group mode can be Visible, in which case the user should be able to see all groups. Or even if course groupmode is separate but not forced, the activity groupmode can be visible, in which case again the user should be allowed to see all groups. That was the reason we decided not to go with groups_print_course_menu() in the first place. Optional_param can be made as an extra param if needed. I tried to keep the signature of groups_print_course_menu_allgroups() close to groups_print_course_menu(), but if that is not what we want, I can change the api. 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
          Ankit Agarwal added a comment - - edited

          After having long discussion with Eloy, we concluded the following:-

          1. The ideal solution is groups_print_activity_menu(), but we cannot use that until MDL-20589 is resolved.
          2. The other alternatives are using one of the below:-
            1. groups_print_activity_menu(), which will break a lot of comibinations, but won't show any extra groups to the user which they cannot see.
            2. groups_print_course_menu_allgroups() as proposed in the patch. Will work for all combinations, however does show extra group names, which the user donot have access to.
            3. Amend groups_print_course_menu_allgroups() to check for "accessallgroups" and if the user doesn't has the capability show only user groups. This will block some combinations from working as well, however won't show any groups that the user cannot see.
          Show
          Ankit Agarwal added a comment - - edited After having long discussion with Eloy, we concluded the following:- The ideal solution is groups_print_activity_menu(), but we cannot use that until MDL-20589 is resolved. The other alternatives are using one of the below:- groups_print_activity_menu(), which will break a lot of comibinations, but won't show any extra groups to the user which they cannot see. groups_print_course_menu_allgroups() as proposed in the patch. Will work for all combinations, however does show extra group names, which the user donot have access to. Amend groups_print_course_menu_allgroups() to check for "accessallgroups" and if the user doesn't has the capability show only user groups. This will block some combinations from working as well, however won't show any groups that the user cannot see.
          Hide
          Dan Poltawski added a comment -

          It seems to me like 3) doesn't over-expose any information a but then we're also loosing information from now that we can see now, so that is a net loss.

          In which case 2) does seem like the only way.. I don't know, what do we think is worse

          Show
          Dan Poltawski added a comment - It seems to me like 3) doesn't over-expose any information a but then we're also loosing information from now that we can see now, so that is a net loss. In which case 2) does seem like the only way.. I don't know, what do we think is worse
          Hide
          Dan Poltawski added a comment -

          I vote for 3) for the short term improvement (this issue has been going on too long) and solve the selector as the major priority.

          That way we make progress, ackownedge we've not solved it properly.

          Show
          Dan Poltawski added a comment - I vote for 3) for the short term improvement (this issue has been going on too long) and solve the selector as the major priority. That way we make progress, ackownedge we've not solved it properly.
          Hide
          Ankit Agarwal added a comment -

          After discussions with Dan and Eloy, we decided to go with option 3. Pushing back for integration.
          Thanks

          Show
          Ankit Agarwal added a comment - After discussions with Dan and Eloy, we decided to go with option 3. Pushing back for integration. Thanks
          Hide
          Dan Poltawski added a comment -

          Hi Ankit,

          Well you are going to hate me, but..

          1. This is a new function. Why are we making this a 'print' style function. Thats Moodle 1.9 style mistakes. I don't believe any functions should be directly echoing like that any more. Traditionally those functions were <1.9 functions when we later realised that we don't always want to echo directly. I think it'd be better to never echo - all it takes is an echo from the calling code. (Happy to be overruled if you can get someone else to vote for keeping this)

          2. This statement is misleading. Don't use the word capability if you are not dealing with capabilities
          Note: This api does not do any group mode check use groups_print_course_menu() instead if you want capability checks.

          3. lib/tests/grouplib_test.php

          Now user can access one of the group. We can't assert an exact match here because of random ids.

          Urm, you have the ids in $group1 and $group2 above, surely you can?

          Show
          Dan Poltawski added a comment - Hi Ankit, Well you are going to hate me, but.. 1. This is a new function. Why are we making this a 'print' style function. Thats Moodle 1.9 style mistakes. I don't believe any functions should be directly echoing like that any more. Traditionally those functions were <1.9 functions when we later realised that we don't always want to echo directly. I think it'd be better to never echo - all it takes is an echo from the calling code. (Happy to be overruled if you can get someone else to vote for keeping this) 2. This statement is misleading. Don't use the word capability if you are not dealing with capabilities Note: This api does not do any group mode check use groups_print_course_menu() instead if you want capability checks. 3. lib/tests/grouplib_test.php Now user can access one of the group. We can't assert an exact match here because of random ids. Urm, you have the ids in $group1 and $group2 above, surely you can?
          Hide
          Ankit Agarwal added a comment -

          Hi Dan,

          1. We already have apis of this style in core.
            groups_print_course_menu()
            groups_print_activity_menu()
            Diverting the new api makes things inconsistent. If it was decided this is not what we want, we should have deprecated the $return for these apis as well. I am against this, however Damyon tend to agree with you as well, so I have made the changes.
          2. updated.
          3. ids = yui generated ids which are part of the html.
          Show
          Ankit Agarwal added a comment - Hi Dan, We already have apis of this style in core. groups_print_course_menu() groups_print_activity_menu() Diverting the new api makes things inconsistent. If it was decided this is not what we want, we should have deprecated the $return for these apis as well. I am against this, however Damyon tend to agree with you as well, so I have made the changes. updated. ids = yui generated ids which are part of the html.
          Hide
          Dan Poltawski added a comment -

          Hi Ankit,

          If it was decided this is not what we want, we should have deprecated the $return for these apis as well.

          No, there is a difference between modernising our apis and just deprecating things (breaking third party) code to make things prettier. This is a new modern creation, if we carried on being inspired by existing code we'd never improve.

          About your patch - just removing the return param isn't sufficient. You need to update the phpdoc of the function and it also needs to be renamed, its not printing anything, so the word print is not correct. My of the top of my head suggestion: groups_allgroups_course_menu

          Show
          Dan Poltawski added a comment - Hi Ankit, If it was decided this is not what we want, we should have deprecated the $return for these apis as well. No, there is a difference between modernising our apis and just deprecating things (breaking third party) code to make things prettier. This is a new modern creation, if we carried on being inspired by existing code we'd never improve. About your patch - just removing the return param isn't sufficient. You need to update the phpdoc of the function and it also needs to be renamed, its not printing anything, so the word print is not correct. My of the top of my head suggestion: groups_allgroups_course_menu
          Hide
          Ankit Agarwal added a comment -

          No, there is a difference between modernising our apis and just deprecating things (breaking third party) code to make things prettier.

          I completely agree with you that we should not be printing html directly. Just trying to understand if, such a decision was made, why was it not implemented in core atleast.

          api name updated.
          Thanks

          Show
          Ankit Agarwal added a comment - No, there is a difference between modernising our apis and just deprecating things (breaking third party) code to make things prettier. I completely agree with you that we should not be printing html directly. Just trying to understand if, such a decision was made, why was it not implemented in core atleast. api name updated. Thanks
          Hide
          Dan Poltawski added a comment -

          Just trying to understand if, such a decision was made, why was it not implemented in core atleast.

          It was ( e.g. http://docs.moodle.org/dev/Outputting_HTML_in_2.0), but there are quite a number of lines in Moodle and we can't do them all at once!

          Show
          Dan Poltawski added a comment - Just trying to understand if, such a decision was made, why was it not implemented in core atleast. It was ( e.g. http://docs.moodle.org/dev/Outputting_HTML_in_2.0 ), but there are quite a number of lines in Moodle and we can't do them all at once!
          Hide
          Dan Poltawski added a comment -

          Thanks Ankit, i've integrated it to master.

          I changed your commit message to match the new name

          Show
          Dan Poltawski added a comment - Thanks Ankit, i've integrated it to master. I changed your commit message to match the new name
          Hide
          David Monllaó added a comment -

          Pasting php error messages I receive in some cases when refreshing report/participation/index.php after changing course settings; this one was after changing the course settings to:

          • Group mode: visible groups
          • Force group mode: no

          And about the user:

          • no access all groups capability
          • user part of the group

          I have seen these error after changing to different combinations of course settings.

          ( ! ) Notice: Undefined offset: 2 in /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/grouplib.php on line 920
          Call Stack
          #	Time	Memory	Function	Location
          1	0.0007	366360	{main}( )	../index.php:0
          2	0.0616	4754848	bootstrap_renderer->header( )	../index.php:84
          3	0.0616	4755032	bootstrap_renderer->__call( )	../index.php:84
          4	0.0659	4952064	call_user_func_array ( )	../setuplib.php:1552
          5	0.0659	4952232	core_renderer->header( )	../setuplib.php:1552
          6	0.0721	5122944	core_renderer->render_page_layout( )	../outputrenderers.php:790
          7	0.0722	5140920	include( '/home/davidm/Desktop/moodlecode/INTEGRATION/master/theme/base/layout/general.php' )	../outputrenderers.php:860
          8	0.0722	5142424	moodle_page->has_navbar( )	../general.php:4
          9	0.0724	5144952	navbar->has_items( )	../pagelib.php:784
          10	0.0725	5153336	global_navigation->initialise( )	../navigationlib.php:2982
          11	0.0778	5404472	global_navigation->add_course_essentials( )	../navigationlib.php:1158
          12	0.0781	5408648	groups_get_course_group( )	../navigationlib.php:2435
          13	0.0785	5421104	_group_verify_activegroup( )	../grouplib.php:730
          
          ( ! ) Warning: array_key_exists() expects parameter 2 to be array, null given in /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/grouplib.php on line 920
          Call Stack
          #	Time	Memory	Function	Location
          1	0.0007	366360	{main}( )	../index.php:0
          2	0.0616	4754848	bootstrap_renderer->header( )	../index.php:84
          3	0.0616	4755032	bootstrap_renderer->__call( )	../index.php:84
          4	0.0659	4952064	call_user_func_array ( )	../setuplib.php:1552
          5	0.0659	4952232	core_renderer->header( )	../setuplib.php:1552
          6	0.0721	5122944	core_renderer->render_page_layout( )	../outputrenderers.php:790
          7	0.0722	5140920	include( '/home/davidm/Desktop/moodlecode/INTEGRATION/master/theme/base/layout/general.php' )	../outputrenderers.php:860
          8	0.0722	5142424	moodle_page->has_navbar( )	../general.php:4
          9	0.0724	5144952	navbar->has_items( )	../pagelib.php:784
          10	0.0725	5153336	global_navigation->initialise( )	../navigationlib.php:2982
          11	0.0778	5404472	global_navigation->add_course_essentials( )	../navigationlib.php:1158
          12	0.0781	5408648	groups_get_course_group( )	../navigationlib.php:2435
          13	0.0785	5421104	_group_verify_activegroup( )	../grouplib.php:730
          14	0.0788	5422168	array_key_exists ( )	../grouplib.php:920
          
          ( ! ) Notice: Undefined offset: 2 in /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/grouplib.php on line 929
          Call Stack
          #	Time	Memory	Function	Location
          1	0.0007	366360	{main}( )	../index.php:0
          2	0.0616	4754848	bootstrap_renderer->header( )	../index.php:84
          3	0.0616	4755032	bootstrap_renderer->__call( )	../index.php:84
          4	0.0659	4952064	call_user_func_array ( )	../setuplib.php:1552
          5	0.0659	4952232	core_renderer->header( )	../setuplib.php:1552
          6	0.0721	5122944	core_renderer->render_page_layout( )	../outputrenderers.php:790
          7	0.0722	5140920	include( '/home/davidm/Desktop/moodlecode/INTEGRATION/master/theme/base/layout/general.php' )	../outputrenderers.php:860
          8	0.0722	5142424	moodle_page->has_navbar( )	../general.php:4
          9	0.0724	5144952	navbar->has_items( )	../pagelib.php:784
          10	0.0725	5153336	global_navigation->initialise( )	../navigationlib.php:2982
          11	0.0778	5404472	global_navigation->add_course_essentials( )	../navigationlib.php:1158
          12	0.0781	5408648	groups_get_course_group( )	../navigationlib.php:2435
          13	0.0785	5421104	_group_verify_activegroup( )	../grouplib.php:730
          
          ( ! ) Warning: array_key_exists() expects parameter 2 to be array, null given in /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/grouplib.php on line 929
          Call Stack
          #	Time	Memory	Function	Location
          1	0.0007	366360	{main}( )	../index.php:0
          2	0.0616	4754848	bootstrap_renderer->header( )	../index.php:84
          3	0.0616	4755032	bootstrap_renderer->__call( )	../index.php:84
          4	0.0659	4952064	call_user_func_array ( )	../setuplib.php:1552
          5	0.0659	4952232	core_renderer->header( )	../setuplib.php:1552
          6	0.0721	5122944	core_renderer->render_page_layout( )	../outputrenderers.php:790
          7	0.0722	5140920	include( '/home/davidm/Desktop/moodlecode/INTEGRATION/master/theme/base/layout/general.php' )	../outputrenderers.php:860
          8	0.0722	5142424	moodle_page->has_navbar( )	../general.php:4
          9	0.0724	5144952	navbar->has_items( )	../pagelib.php:784
          10	0.0725	5153336	global_navigation->initialise( )	../navigationlib.php:2982
          11	0.0778	5404472	global_navigation->add_course_essentials( )	../navigationlib.php:1158
          12	0.0781	5408648	groups_get_course_group( )	../navigationlib.php:2435
          13	0.0785	5421104	_group_verify_activegroup( )	../grouplib.php:730
          14	0.0793	5422168	array_key_exists ( )	../grouplib.php:929
          
          Show
          David Monllaó added a comment - Pasting php error messages I receive in some cases when refreshing report/participation/index.php after changing course settings; this one was after changing the course settings to: Group mode: visible groups Force group mode: no And about the user: no access all groups capability user part of the group I have seen these error after changing to different combinations of course settings. ( ! ) Notice: Undefined offset: 2 in /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/grouplib.php on line 920 Call Stack # Time Memory Function Location 1 0.0007 366360 {main}( ) ../index.php:0 2 0.0616 4754848 bootstrap_renderer->header( ) ../index.php:84 3 0.0616 4755032 bootstrap_renderer->__call( ) ../index.php:84 4 0.0659 4952064 call_user_func_array ( ) ../setuplib.php:1552 5 0.0659 4952232 core_renderer->header( ) ../setuplib.php:1552 6 0.0721 5122944 core_renderer->render_page_layout( ) ../outputrenderers.php:790 7 0.0722 5140920 include( '/home/davidm/Desktop/moodlecode/INTEGRATION/master/theme/base/layout/general.php' ) ../outputrenderers.php:860 8 0.0722 5142424 moodle_page->has_navbar( ) ../general.php:4 9 0.0724 5144952 navbar->has_items( ) ../pagelib.php:784 10 0.0725 5153336 global_navigation->initialise( ) ../navigationlib.php:2982 11 0.0778 5404472 global_navigation->add_course_essentials( ) ../navigationlib.php:1158 12 0.0781 5408648 groups_get_course_group( ) ../navigationlib.php:2435 13 0.0785 5421104 _group_verify_activegroup( ) ../grouplib.php:730 ( ! ) Warning: array_key_exists() expects parameter 2 to be array, null given in /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/grouplib.php on line 920 Call Stack # Time Memory Function Location 1 0.0007 366360 {main}( ) ../index.php:0 2 0.0616 4754848 bootstrap_renderer->header( ) ../index.php:84 3 0.0616 4755032 bootstrap_renderer->__call( ) ../index.php:84 4 0.0659 4952064 call_user_func_array ( ) ../setuplib.php:1552 5 0.0659 4952232 core_renderer->header( ) ../setuplib.php:1552 6 0.0721 5122944 core_renderer->render_page_layout( ) ../outputrenderers.php:790 7 0.0722 5140920 include( '/home/davidm/Desktop/moodlecode/INTEGRATION/master/theme/base/layout/general.php' ) ../outputrenderers.php:860 8 0.0722 5142424 moodle_page->has_navbar( ) ../general.php:4 9 0.0724 5144952 navbar->has_items( ) ../pagelib.php:784 10 0.0725 5153336 global_navigation->initialise( ) ../navigationlib.php:2982 11 0.0778 5404472 global_navigation->add_course_essentials( ) ../navigationlib.php:1158 12 0.0781 5408648 groups_get_course_group( ) ../navigationlib.php:2435 13 0.0785 5421104 _group_verify_activegroup( ) ../grouplib.php:730 14 0.0788 5422168 array_key_exists ( ) ../grouplib.php:920 ( ! ) Notice: Undefined offset: 2 in /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/grouplib.php on line 929 Call Stack # Time Memory Function Location 1 0.0007 366360 {main}( ) ../index.php:0 2 0.0616 4754848 bootstrap_renderer->header( ) ../index.php:84 3 0.0616 4755032 bootstrap_renderer->__call( ) ../index.php:84 4 0.0659 4952064 call_user_func_array ( ) ../setuplib.php:1552 5 0.0659 4952232 core_renderer->header( ) ../setuplib.php:1552 6 0.0721 5122944 core_renderer->render_page_layout( ) ../outputrenderers.php:790 7 0.0722 5140920 include( '/home/davidm/Desktop/moodlecode/INTEGRATION/master/theme/base/layout/general.php' ) ../outputrenderers.php:860 8 0.0722 5142424 moodle_page->has_navbar( ) ../general.php:4 9 0.0724 5144952 navbar->has_items( ) ../pagelib.php:784 10 0.0725 5153336 global_navigation->initialise( ) ../navigationlib.php:2982 11 0.0778 5404472 global_navigation->add_course_essentials( ) ../navigationlib.php:1158 12 0.0781 5408648 groups_get_course_group( ) ../navigationlib.php:2435 13 0.0785 5421104 _group_verify_activegroup( ) ../grouplib.php:730 ( ! ) Warning: array_key_exists() expects parameter 2 to be array, null given in /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/grouplib.php on line 929 Call Stack # Time Memory Function Location 1 0.0007 366360 {main}( ) ../index.php:0 2 0.0616 4754848 bootstrap_renderer->header( ) ../index.php:84 3 0.0616 4755032 bootstrap_renderer->__call( ) ../index.php:84 4 0.0659 4952064 call_user_func_array ( ) ../setuplib.php:1552 5 0.0659 4952232 core_renderer->header( ) ../setuplib.php:1552 6 0.0721 5122944 core_renderer->render_page_layout( ) ../outputrenderers.php:790 7 0.0722 5140920 include( '/home/davidm/Desktop/moodlecode/INTEGRATION/master/theme/base/layout/general.php' ) ../outputrenderers.php:860 8 0.0722 5142424 moodle_page->has_navbar( ) ../general.php:4 9 0.0724 5144952 navbar->has_items( ) ../pagelib.php:784 10 0.0725 5153336 global_navigation->initialise( ) ../navigationlib.php:2982 11 0.0778 5404472 global_navigation->add_course_essentials( ) ../navigationlib.php:1158 12 0.0781 5408648 groups_get_course_group( ) ../navigationlib.php:2435 13 0.0785 5421104 _group_verify_activegroup( ) ../grouplib.php:730 14 0.0793 5422168 array_key_exists ( ) ../grouplib.php:929
          Hide
          David Monllaó added a comment -

          Few things I see that does not match what I understand from the testing instructions:

          • No data displayed and there is data
            • Without all groups capability
            • User that belongs to a group
            • There are logs of that group users
            • Course group mode to separate groups
            • Course force group mode to yes
            • Activity is forced to use separate groups
            • Result: I see Sorry, but you need to be part of a group to see this activity.
          • Always only the user group
            • Without all groups capability
            • User that belongs to a group
            • There are logs of that group users
            • Activity using separate, visible or no groups
            • All sort of combinations of 'course group mode' and 'forced group mode' settings BUT the case explained above
            • Result: I always see only the user group
          Show
          David Monllaó added a comment - Few things I see that does not match what I understand from the testing instructions: No data displayed and there is data Without all groups capability User that belongs to a group There are logs of that group users Course group mode to separate groups Course force group mode to yes Activity is forced to use separate groups Result: I see Sorry, but you need to be part of a group to see this activity. Always only the user group Without all groups capability User that belongs to a group There are logs of that group users Activity using separate, visible or no groups All sort of combinations of 'course group mode' and 'forced group mode' settings BUT the case explained above Result: I always see only the user group
          Hide
          David Monllaó added a comment -

          Failed after talking with Ankit about it

          Show
          David Monllaó added a comment - Failed after talking with Ankit about it
          Hide
          Ankit Agarwal added a comment - - edited

          Hi,

          1. We were not able to replicate it at will. This could have happened because of changes in setting making a report page url no longer valid. Since the user doesn't have access to a particular group or not. In practical situations this should never happen (when you go to reports from navigation and not hack away the url directly). Any ways I still have added some code to init various session arrays so that it might not happen again.
          2. This effected a particular case when the user had access to only one group exactly. So there was no drop down shown, hence the group param was never set. We should checking $activegroup before setting it as $currentgroup. Also $currentgroup should be fetched from session not page var, while doing the final checks.
          3. This is expected behaviour. My apologises, the table was not reflecting it. updating it now.

          Dan, can you please pull this change in ?
          https://github.com/ankitagarwal/moodle/commit/1adae53e732bd6cd6d2ac4b3ec587573a3eace85

          Thanks

          Show
          Ankit Agarwal added a comment - - edited Hi, We were not able to replicate it at will. This could have happened because of changes in setting making a report page url no longer valid. Since the user doesn't have access to a particular group or not. In practical situations this should never happen (when you go to reports from navigation and not hack away the url directly). Any ways I still have added some code to init various session arrays so that it might not happen again. This effected a particular case when the user had access to only one group exactly. So there was no drop down shown, hence the group param was never set. We should checking $activegroup before setting it as $currentgroup. Also $currentgroup should be fetched from session not page var, while doing the final checks. This is expected behaviour. My apologises, the table was not reflecting it. updating it now. Dan, can you please pull this change in ? https://github.com/ankitagarwal/moodle/commit/1adae53e732bd6cd6d2ac4b3ec587573a3eace85 Thanks
          Hide
          Dan Poltawski added a comment -

          Done, thanks Ankit.

          Back to you David.

          Show
          Dan Poltawski added a comment - Done, thanks Ankit. Back to you David.
          Hide
          David Monllaó added a comment -

          Yes! it passes

          Show
          David Monllaó added a comment - Yes! it passes
          Hide
          Ankit Agarwal added a comment -

          Thanks David, for going through this monstrous test!

          Show
          Ankit Agarwal added a comment - Thanks David, for going through this monstrous test!
          Hide
          Damyon Wiese added a comment -

          This issue along with 77 of it's friends has been sent upstream and released to the world.

          Thankyou for your contribution.

          Show
          Damyon Wiese added a comment - This issue along with 77 of it's friends has been sent upstream and released to the world. Thankyou for your contribution.
          Hide
          Mary Cooch added a comment - - edited

          Just housekeeping - removing docs_required because I think if something is now working as expected, we shouldn't need to document that -but please if you disagree then shout (or document it!)

          Show
          Mary Cooch added a comment - - edited Just housekeeping - removing docs_required because I think if something is now working as expected, we shouldn't need to document that -but please if you disagree then shout (or document it!)
          Hide
          Nick Varney added a comment - - edited

          Just taken a look at this with some anticipation in the 2.6 beta. It's great that the Gradebook now filters students by group. Have I missed something though? It still doesn't filter the activities assigned to groups! As a teacher of group 1 I do not wish to see all the other grade-able activities in the course. There could be hundreds of them that are not relevant to the selected group.

          Not filtering activities when the group filter is being used just doesn't seem to make sense to me and they take up precious screen estate in already data-heavy user area of Moodle.

          This filtering is really needed in my opinion to make group activities and tracking them a feasible prospect, as requested in https://tracker.moodle.org/browse/MDL-40367, otherwise the Gradebook remains a huge horizontal scrolling exercise.

          Show
          Nick Varney added a comment - - edited Just taken a look at this with some anticipation in the 2.6 beta. It's great that the Gradebook now filters students by group. Have I missed something though? It still doesn't filter the activities assigned to groups! As a teacher of group 1 I do not wish to see all the other grade-able activities in the course. There could be hundreds of them that are not relevant to the selected group. Not filtering activities when the group filter is being used just doesn't seem to make sense to me and they take up precious screen estate in already data-heavy user area of Moodle. This filtering is really needed in my opinion to make group activities and tracking them a feasible prospect, as requested in https://tracker.moodle.org/browse/MDL-40367 , otherwise the Gradebook remains a huge horizontal scrolling exercise.

            People

            • Votes:
              8 Vote for this issue
              Watchers:
              13 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Agile