Moodle
  1. Moodle
  2. MDL-16701

Bulk user enrol hard-codes the student role id as 5 and incorrectly shows teachers as enrolled

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Deferred
    • Affects Version/s: 2.0, 2.1.10, 2.2.10, 2.3.7, 2.4.4, 2.5
    • Fix Version/s: 2.0.10, BACKEND
    • Component/s: Administration
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE

      Description

      This was checked in as MDL-15449.

      This is BAD. The code should look for the role in the database according to some condition, for example shortname = 'student' or having capability legacy/student, with appropriate error handling.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Tim Hunt added a comment -

            It also shows teachers as if they were already enrolled, however this is misleading as they are enrolled as teachers, and this page only lets you enrol people as students.

            Show
            Tim Hunt added a comment - It also shows teachers as if they were already enrolled, however this is misleading as they are enrolled as teachers, and this page only lets you enrol people as students.
            Hide
            Anthony Borrow added a comment -

            Yep, hard coding the role id ~ line 93:

            if( role_unassign(5, $ids[0], 0, $context->id) ) {

            is not pretty although I confess doing something similar on one of my servers just because it was a quick/easy way to do it without having to look up the id.

            Show
            Anthony Borrow added a comment - Yep, hard coding the role id ~ line 93: if( role_unassign(5, $ids [0] , 0, $context->id) ) { is not pretty although I confess doing something similar on one of my servers just because it was a quick/easy way to do it without having to look up the id.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Looks really horrible, yup. Perhaps we could use $CFG->defaultcourseroleid as the role to be used when using that bulk operation? Both when detecting current enrolments and when assigning them.

            Though it's possible that this will be changed in 2.0 new enrolments... I think it's better to have it working properly (at least "old behaviour") since now, yup?

            Show
            Eloy Lafuente (stronk7) added a comment - Looks really horrible, yup. Perhaps we could use $CFG->defaultcourseroleid as the role to be used when using that bulk operation? Both when detecting current enrolments and when assigning them. Though it's possible that this will be changed in 2.0 new enrolments... I think it's better to have it working properly (at least "old behaviour") since now, yup?
            Hide
            Eloy Lafuente (stronk7) added a comment -

            This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue.

            For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment

            Show
            Eloy Lafuente (stronk7) added a comment - This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue. For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment
            Hide
            Anthony Borrow added a comment -

            Yikes, I see this is still in Moodle 2.5 so I have tagged 2.1-2.5 as being affected as well. Peace - Anthony

            Show
            Anthony Borrow added a comment - Yikes, I see this is still in Moodle 2.5 so I have tagged 2.1-2.5 as being affected as well. Peace - Anthony
            Hide
            Tim Hunt added a comment -

            Increasing severity, and putting on the BACKEND backlog (I hope that is right).

            Show
            Tim Hunt added a comment - Increasing severity, and putting on the BACKEND backlog (I hope that is right).
            Hide
            Rajesh Taneja added a comment -

            I think this can be closed as admin/user/user_bulk_enrol.php is not executable anymore.
            This should be fixed by MDL-24064

            Show
            Rajesh Taneja added a comment - I think this can be closed as admin/user/user_bulk_enrol.php is not executable anymore. This should be fixed by MDL-24064
            Hide
            Tim Hunt added a comment -

            If that code is never used, then it should be deleted from the codebase. Or, are you saying that is what DML-24064 is about?

            Show
            Tim Hunt added a comment - If that code is never used, then it should be deleted from the codebase. Or, are you saying that is what DML-24064 is about?
            Hide
            Rajesh Taneja added a comment -

            MDL-24064 is about rewriting this file to use new enrolment.

            Show
            Rajesh Taneja added a comment - MDL-24064 is about rewriting this file to use new enrolment.
            Hide
            moodle.com added a comment -

            Will be fixed by MDL-24064

            Show
            moodle.com added a comment - Will be fixed by MDL-24064

              People

              • Votes:
                3 Vote for this issue
                Watchers:
                10 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: