Moodle
  1. Moodle
  2. MDL-27164

In the Add/remove users to groups screen, users are listed under Student and Mutiple roles, yet none of the users listed under Mutiples roles have mutiple roles

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.2, 2.3.3, 2.4.1
    • Fix Version/s: 2.3.5, 2.4.2
    • Component/s: Groups
    • Environment:
      Moodle 2.0.2+ (Build: 20110330)
      php 5.3.3
      MySQL 5.5.9
    • Testing Instructions:
      Hide
      1. Create two courses, A and B
      2. Manually enrol 2 users (Bob and John) as students on both courses
      3. Manually enrol 2 users (Eve and Kate) as teachers on both courses
      4. Add a course meta link to course B on course A, so that the users have 2 enrolments on course A
      5. Create a group in course A and go to the add users screen
      6. Make sure none of the users appear under "Multiple roles"
      7. Make sure John and Bob are listed under "Student"
      8. Make sure Kate and Eve are listed under "Teacher"
      Show
      Create two courses, A and B Manually enrol 2 users (Bob and John) as students on both courses Manually enrol 2 users (Eve and Kate) as teachers on both courses Add a course meta link to course B on course A, so that the users have 2 enrolments on course A Create a group in course A and go to the add users screen Make sure none of the users appear under "Multiple roles" Make sure John and Bob are listed under "Student" Make sure Kate and Eve are listed under "Teacher"
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
      MDL-27164-MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-27164-master
    • Rank:
      16817

      Description

      When viewing the Add/remove users to a group page, there are two lists:
      -Group members
      -Potential members

      In both lists, users appear under two headings: 'Student' and 'Multiple roles'. This is despite the fact that none of the users have multiple roles, i.e. they all have the role of student in the course (and entire system) and no other roles.

      Note, this seems to only happen in courses that have 'Course meta link' as an enrollment method. Users appearing under the heading of 'Student' are members of no or one group. Users appearing under Multiple roles are members of multiple groups, although some are in no groups, but perhaps in multiple courses.

      It seems that the term 'role' is being confused with group or course membership.

      To replicate this issue:

      1. Create two courses, A and B, with a user enrolled manually as student on both
      2. Add a course meta link to course B on course A, so that the user has 2 enrolments on course A as a student
      3. Create a group in course A and go to the add users screen
      4. The user appears under "Multiple roles"

        Activity

        Hide
        Dan Poltawski added a comment -

        Hi, is this still na issue in the latest stable?

        Show
        Dan Poltawski added a comment - Hi, is this still na issue in the latest stable?
        Hide
        Michael Aherne added a comment - - edited

        This does seem to be happening in both 2.3 stable and master. I haven't worked out how to reproduce the data that's causing it yet, though.

        In any case, the attached patch seems to fix it.

        I'm not convinced that in_array is the most performance-friendly way to do it, but I'm not noticing any deterioration. Can anyone comment on whether this is a sensible patch?

        Show
        Michael Aherne added a comment - - edited This does seem to be happening in both 2.3 stable and master. I haven't worked out how to reproduce the data that's causing it yet, though. In any case, the attached patch seems to fix it. I'm not convinced that in_array is the most performance-friendly way to do it, but I'm not noticing any deterioration. Can anyone comment on whether this is a sensible patch?
        Hide
        Frédéric Massart added a comment -

        Hi Michael, thanks for your work on this. Could you please provide some replication steps? I tried multiple things but I was not able to replicate this bug.

        As per your patch, it looks good and if you're worried about the performance of in_array(), you could just set a key using the roleId:

        diff --git a/group/lib.php b/group/lib.php
        index a3f8fac..20c9555 100644
        --- a/group/lib.php
        +++ b/group/lib.php
        @@ -846,7 +846,7 @@ function groups_calculate_role_people($rs, $context) {
                         $roles[$roledata->id] = $roledata;
                     }
                     // Record that user has role
        -            $users[$rec->userid]->roles[] = $roles[$rec->roleid];
        +            $users[$rec->userid]->roles[$rec->roleid] = $roles[$rec->roleid];
                 }
             }
             $rs->close();
        @@ -876,7 +876,8 @@ function groups_calculate_role_people($rs, $context) {
                 } else if($rolecount > 1) {
                     $roleid = '*';
                 } else {
        -            $roleid = $userdata->roles[0]->id;
        +            $userrole = reset($userdata->roles);
        +            $roleid = $userrole->id;
                 }
                 $roles[$roleid]->users[$userid] = $userdata;
             }
        

        In order to push your patch for integration you will also need to provide some testing instructions, which will probably be almost identical to the replication steps.

        Many thanks!
        Fred

        Show
        Frédéric Massart added a comment - Hi Michael, thanks for your work on this. Could you please provide some replication steps? I tried multiple things but I was not able to replicate this bug. As per your patch, it looks good and if you're worried about the performance of in_array(), you could just set a key using the roleId: diff --git a/group/lib.php b/group/lib.php index a3f8fac..20c9555 100644 --- a/group/lib.php +++ b/group/lib.php @@ -846,7 +846,7 @@ function groups_calculate_role_people($rs, $context) { $roles[$roledata->id] = $roledata; } // Record that user has role - $users[$rec->userid]->roles[] = $roles[$rec->roleid]; + $users[$rec->userid]->roles[$rec->roleid] = $roles[$rec->roleid]; } } $rs->close(); @@ -876,7 +876,8 @@ function groups_calculate_role_people($rs, $context) { } else if($rolecount > 1) { $roleid = '*'; } else { - $roleid = $userdata->roles[0]->id; + $userrole = reset($userdata->roles); + $roleid = $userrole->id; } $roles[$roleid]->users[$userid] = $userdata; } In order to push your patch for integration you will also need to provide some testing instructions, which will probably be almost identical to the replication steps. Many thanks! Fred
        Hide
        Michael Aherne added a comment -

        Hi Fred, thanks for the feedback. I've worked out how to replicate this and added some instructions.

        I think I've worked out what might be happening. It seems that enrolment plugins where roles_protected returns true get their own entry in role_assignments, regardless of whether the same role has already been assigned on the course by another plugin. On the other hand, plugins where the roles are unprotected effectively share a single role_assignment entry. This problem only manifests itself where multiple plugins have assigned the same role and at least one of them returns true for roles_protected.

        I'll look at updating the patch to use your suggestion instead of in_array - it does look much better!

        Cheers, Michael.

        Show
        Michael Aherne added a comment - Hi Fred, thanks for the feedback. I've worked out how to replicate this and added some instructions. I think I've worked out what might be happening. It seems that enrolment plugins where roles_protected returns true get their own entry in role_assignments, regardless of whether the same role has already been assigned on the course by another plugin. On the other hand, plugins where the roles are unprotected effectively share a single role_assignment entry. This problem only manifests itself where multiple plugins have assigned the same role and at least one of them returns true for roles_protected. I'll look at updating the patch to use your suggestion instead of in_array - it does look much better! Cheers, Michael.
        Hide
        Michael Aherne added a comment -

        Changed patch to Fred's idea, and ported to 2.3 and 2.4.

        Show
        Michael Aherne added a comment - Changed patch to Fred's idea, and ported to 2.3 and 2.4.
        Hide
        Frédéric Massart added a comment -

        Thanks for the patch Michael. I have added some testing instructions based on your replication tests, usually you would have been asked to do it, but as it was fairly straight forward I've added them to push this issue for integration.

        Big thanks,
        Fred

        Show
        Frédéric Massart added a comment - Thanks for the patch Michael. I have added some testing instructions based on your replication tests, usually you would have been asked to do it, but as it was fairly straight forward I've added them to push this issue for integration. Big thanks, Fred
        Hide
        Michael Aherne added a comment -

        Thanks for adding the testing instructions, Fred.

        Show
        Michael Aherne added a comment - Thanks for adding the testing instructions, Fred.
        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 -

        Integrated to master, 24 and 23. Thanks Michael!

        Show
        Dan Poltawski added a comment - Integrated to master, 24 and 23. Thanks Michael!
        Hide
        Michael de Raadt added a comment -

        Test result: Success!

        Tested in 2.3, 2.4 and Master.

        Show
        Michael de Raadt added a comment - Test result: Success! Tested in 2.3, 2.4 and Master.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks!

        Closing as fixed, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks! Closing as fixed, ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: