Moodle
  1. Moodle
  2. MDL-38155

Add ability to suspend user enrolment on the User Upload Admin tool

    Details

    • Testing Instructions:
      Hide
      1. Bulk upload users with users2.txt (attached) and make sure you get no error
        1. student1 has active enrolment in course 1 and course 2 and suspended in course 3
        2. Student2 has active enrolment in course 1 and course 3 and suppended in course 2.
      2. Remove enrolstatus from users2.txt and upload file, make sure enrolment is not affected.
      Show
      Bulk upload users with users2.txt (attached) and make sure you get no error student1 has active enrolment in course 1 and course 2 and suspended in course 3 Student2 has active enrolment in course 1 and course 3 and suppended in course 2. Remove enrolstatus from users2.txt and upload file, make sure enrolment is not affected.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull Master Branch:
      wip-mdl-38155
    • Rank:
      47981
    • Sprint:
      BACKEND Sprint 6

      Description

      What I would like is the ability to suspend a users enrolment in certain courses using the Upload Users Admin tool.

      Currently the only way to stop a users enrolment is to manually remove them, set the enrolment period, or suspend them. The only one that can be done through the tool is setting the enrolment period to something greater than 0.

      I know there is a suspendeded column but that is for the entire site(right? its not in the docs). I don't know if another column should be added or if we could use the enrolX column. Currently anything above 0 sets the enrolment date. 0 removes the end date of enrolment. Could we have -1 equal suspended enrolment.

      Thoughts?

      1. enrolstatus.patch
        2 kB
        James Henestofel
      2. users.txt
        0.2 kB
        James Henestofel
      3. users2.txt
        0.2 kB
        James Henestofel

        Activity

        Hide
        Rajesh Taneja added a comment -

        Thanks for reporting this James.

        Rather then -1, we should have one more field (enrolstatus), which should set status of enrolment. Currently we don't pass that, as we are not expecting one.

        I've put that on the backlog.

        In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.

        Show
        Rajesh Taneja added a comment - Thanks for reporting this James. Rather then -1, we should have one more field (enrolstatus), which should set status of enrolment. Currently we don't pass that, as we are not expecting one. I've put that on the backlog. In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.
        Hide
        James Henestofel added a comment -

        I've attached a patch file because the fix was quite small.

        I've added another column enrolstatus that can be added to each course entry the same as enrolperiod

        To suspend a user enter 1 and to make them active you can enter anything other than 1 because it only checks if the entry is 1.

        I've also attached two example user files to upload to verify its working.

        users.txt will set the following

        user course1 status/entry course2 status/entry course3 status/entry
        student1 Active/0 Suspended/1 Active/blank
        student2 Active/blank Active/0 Suspended/1

        users2.txt will set the following

        user course1 status/entry course2 status/entry course3 status/entry
        student1 Active/blank Active/0 Suspended/1
        student2 Active/0 Suspended/1 Active/blank

        If possible would be nice to backport to older versions. I checked them the best I could and they all seemed to be the same as far as what I've changed.

        Show
        James Henestofel added a comment - I've attached a patch file because the fix was quite small. I've added another column enrolstatus that can be added to each course entry the same as enrolperiod To suspend a user enter 1 and to make them active you can enter anything other than 1 because it only checks if the entry is 1 . I've also attached two example user files to upload to verify its working. users.txt will set the following user course1 status/entry course2 status/entry course3 status/entry student1 Active/0 Suspended/1 Active/blank student2 Active/blank Active/0 Suspended/1 users2.txt will set the following user course1 status/entry course2 status/entry course3 status/entry student1 Active/blank Active/0 Suspended/1 student2 Active/0 Suspended/1 Active/blank If possible would be nice to backport to older versions. I checked them the best I could and they all seemed to be the same as far as what I've changed.
        Hide
        Rajesh Taneja added a comment -

        Thanks for the patch James,

        We will try to get to this soon.

        Show
        Rajesh Taneja added a comment - Thanks for the patch James, We will try to get to this soon.
        Hide
        Rajesh Taneja added a comment -

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

        If you have any information about this issue or a possible fix please post it here

        Show
        Rajesh Taneja added a comment - Hello. 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 If you have any information about this issue or a possible fix please post it here
        Hide
        James Henestofel added a comment -

        Any thoughts on this issue. I've run into an issue where I really do need this setting. I've started the following discussion detailing my issues with the disable course enrollment setting and how I need this setting to change a users status in a course https://moodle.org/mod/forum/discuss.php?d=238189

        Show
        James Henestofel added a comment - Any thoughts on this issue. I've run into an issue where I really do need this setting. I've started the following discussion detailing my issues with the disable course enrollment setting and how I need this setting to change a users status in a course https://moodle.org/mod/forum/discuss.php?d=238189
        Hide
        Rajesh Taneja added a comment -

        Thanks for providing the patch James,

        I have created git commit for this and pushing it for peer-review.

        Show
        Rajesh Taneja added a comment - Thanks for providing the patch James, I have created git commit for this and pushing it for peer-review.
        Hide
        Petr Škoda added a comment -

        hmm, I do not think the patch is going to work when you want to enable already suspended enrolments. I think it should be working both ways. Reopening...

        Show
        Petr Škoda added a comment - hmm, I do not think the patch is going to work when you want to enable already suspended enrolments. I think it should be working both ways. Reopening...
        Hide
        James Henestofel added a comment -

        Petr,
        This patch should be working both ways. With your testing did this not occur? In my comments above I explain how it should be working. The testing instructions that were entered did not include checking to see if users suspended enrolment was changed to active.

        Show
        James Henestofel added a comment - Petr, This patch should be working both ways. With your testing did this not occur? In my comments above I explain how it should be working. The testing instructions that were entered did not include checking to see if users suspended enrolment was changed to active.
        Hide
        Petr Škoda added a comment - - edited

        I never read comments before reading the code, if I did that I could not review the code any more.

        Anyway this breaks backwards compatibility, the null in enrol_user() call means "keep current" - we should keep this 'feature' imho. (I was wrong about the disabling.)

        There are two options:
        1/ enrolstatus column is present - '' means null, '0' active, '1' passive, everything else warning and use null (there might be other values in the future)
        2/ enrolstatus column is not present - I think we should keep current behaviour, that is pass null

        Show
        Petr Škoda added a comment - - edited I never read comments before reading the code, if I did that I could not review the code any more. Anyway this breaks backwards compatibility, the null in enrol_user() call means "keep current" - we should keep this 'feature' imho. (I was wrong about the disabling.) There are two options: 1/ enrolstatus column is present - '' means null, '0' active, '1' passive, everything else warning and use null (there might be other values in the future) 2/ enrolstatus column is not present - I think we should keep current behaviour, that is pass null
        Hide
        James Henestofel added a comment -

        Ok, I understand now. Both options could be accommodated.

        The $status should be defaulted to NULL. And the logic that checks the enrolstatus should check for '' = NULL, 0 = ACTIVE, or 1 = SUSPENDED and default to NULL again if none present. Correct?

        Rajesh, do you want to handle this or should I?

        Show
        James Henestofel added a comment - Ok, I understand now. Both options could be accommodated. The $status should be defaulted to NULL. And the logic that checks the enrolstatus should check for '' = NULL, 0 = ACTIVE, or 1 = SUSPENDED and default to NULL again if none present. Correct? Rajesh, do you want to handle this or should I?
        Hide
        Petr Škoda added a comment -

        Yes, thanks.

        Show
        Petr Škoda added a comment - Yes, thanks.
        Hide
        James Henestofel added a comment -

        I've went ahead and implemented this on my own. One thing I've run into with GIT though is I'm getting the message 'No newline at end of file' for the locallib file. If Either of you know how to get around this please let me know. My repo branch is below.

        https://github.com/henesnarfel/moodle/tree/wip-MDL-38155-m26-enrolstatususerupload

        Show
        James Henestofel added a comment - I've went ahead and implemented this on my own. One thing I've run into with GIT though is I'm getting the message 'No newline at end of file' for the locallib file. If Either of you know how to get around this please let me know. My repo branch is below. https://github.com/henesnarfel/moodle/tree/wip-MDL-38155-m26-enrolstatususerupload
        Hide
        Rajesh Taneja added a comment -

        Thanks for the patch James,

        locallib.php doesn't have a line after ending bracket "}". Your editor is smart enough to add one. Moodle doesn't have a hard-line on having newline at the end of file but we prefer to have one so that seems fine.

        On the other hand looking at your patch there were two minor things which need to be addressed:

        1. Check for not empty should be check for isset, else empty will consider '' and fail condition check. https://github.com/henesnarfel/moodle/compare/wip-MDL-38155-m26-enrolstatususerupload#diff-0717d612ef220391a755aa779e13ec15R952
        2. Null has to be lower case as per coding guidelines. http://docs.moodle.org/dev/Coding_style#Booleans_and_the_null_value

        I have fixed that in the branch and pushing for integration.

        Thanks Petr for looking at this.

        Show
        Rajesh Taneja added a comment - Thanks for the patch James, locallib.php doesn't have a line after ending bracket "}". Your editor is smart enough to add one. Moodle doesn't have a hard-line on having newline at the end of file but we prefer to have one so that seems fine. On the other hand looking at your patch there were two minor things which need to be addressed: Check for not empty should be check for isset, else empty will consider '' and fail condition check. https://github.com/henesnarfel/moodle/compare/wip-MDL-38155-m26-enrolstatususerupload#diff-0717d612ef220391a755aa779e13ec15R952 Null has to be lower case as per coding guidelines. http://docs.moodle.org/dev/Coding_style#Booleans_and_the_null_value I have fixed that in the branch and pushing for integration. Thanks Petr for looking at this.
        Hide
        James Henestofel added a comment -

        Rajesh,
        As for the newline. I just wanted to make sure that if you guys used the patch from my branch the newline wouldn't be an issue. The thing is, is that it didn't actually add a line but just showed the warning of no newline. Just something that GIT does I guess.

        Per your comments.
        1. I just looked at how they were checking the 'enrolperiod' here https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-38155#diff-0717d612ef220391a755aa779e13ec15R964 and copied that 'check' functionality.
        2. That's fine. I forgot to look at the coding standards. I did do a search on the file and found other places where they were use 'null' some were upper some were lower. So I chose upper. I know those may have occurred before standards or may have been overlooked

        One last thing is your commit message I think references the wrong tracker issue number. It's referencing mdl-31391 but should be mdl-38155

        Thanks to both for helping with this.

        Show
        James Henestofel added a comment - Rajesh, As for the newline. I just wanted to make sure that if you guys used the patch from my branch the newline wouldn't be an issue. The thing is, is that it didn't actually add a line but just showed the warning of no newline. Just something that GIT does I guess. Per your comments. 1. I just looked at how they were checking the 'enrolperiod' here https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-38155#diff-0717d612ef220391a755aa779e13ec15R964 and copied that 'check' functionality. 2. That's fine. I forgot to look at the coding standards. I did do a search on the file and found other places where they were use 'null' some were upper some were lower. So I chose upper. I know those may have occurred before standards or may have been overlooked One last thing is your commit message I think references the wrong tracker issue number. It's referencing mdl-31391 but should be mdl-38155 Thanks to both for helping with this.
        Hide
        Rajesh Taneja added a comment -

        Thanks James, fixed tracker number in commit msg.

        Show
        Rajesh Taneja added a comment - Thanks James, fixed tracker number in commit msg.
        Hide
        Petr Škoda added a comment - - edited

        reopening, the cast to int is not ok imo, also the == comparison to '' is not accurate, it might be better to trim the data first too

        1/ if trim() === ''
        2/ if trim() === (string)ENROL_xxx
        3/ if trim() === (string)ENROL_yyy
        4/ else debugging('unknown value')

        Show
        Petr Škoda added a comment - - edited reopening, the cast to int is not ok imo, also the == comparison to '' is not accurate, it might be better to trim the data first too 1/ if trim() === '' 2/ if trim() === (string)ENROL_xxx 3/ if trim() === (string)ENROL_yyy 4/ else debugging('unknown value')
        Hide
        Rajesh Taneja added a comment -

        Thanks Petr,

        I have modified the code as per your recommendation.
        I am not sure about trim, as we don't do it anywhere and expect no space in csv file, although have added that in the patch.

        Will wait for your +1, before pushing it forward.

        Show
        Rajesh Taneja added a comment - Thanks Petr, I have modified the code as per your recommendation. I am not sure about trim, as we don't do it anywhere and expect no space in csv file, although have added that in the patch. Will wait for your +1, before pushing it forward.
        Hide
        Petr Škoda added a comment - - edited

        +1

        maybe the debugging could be a bit more verbose, I gave just an example above

        Show
        Petr Škoda added a comment - - edited +1 maybe the debugging could be a bit more verbose, I gave just an example above
        Hide
        Marina Glancy added a comment -

        Thanks guys, it has been integrated in master

        Please add to testing instructions checks that user enrolment status can also be updated (both enabled and disabled).

        Show
        Marina Glancy added a comment - Thanks guys, it has been integrated in master Please add to testing instructions checks that user enrolment status can also be updated (both enabled and disabled).
        Hide
        Adrian Greeve added a comment -

        Tested on the master integration branch.
        Testing matched testing instructions.
        Test passed.

        Show
        Adrian Greeve added a comment - Tested on the master integration branch. Testing matched testing instructions. Test passed.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Yes, it's happening (somewhere in the French Polynesia, right now). And you did it, raising Moodle to new excellency levels.

        Or, if you prefer, yes, you fixed that boring issue.

        Thanks anyway! Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Yes, it's happening (somewhere in the French Polynesia, right now). And you did it, raising Moodle to new excellency levels. Or, if you prefer, yes, you fixed that boring issue. Thanks anyway! Ciao
        Hide
        Mary Cooch added a comment -

        QA test added for inclusion in 2.6 testing MDLQA-5739

        Show
        Mary Cooch added a comment - QA test added for inclusion in 2.6 testing MDLQA-5739
        Hide
        Mary Cooch added a comment -

        Removing docs_required label as this is documented here http://docs.moodle.org/26/en/Upload_users and here http://docs.moodle.org/26/en/Enrolment_FAQ

        Show
        Mary Cooch added a comment - Removing docs_required label as this is documented here http://docs.moodle.org/26/en/Upload_users and here http://docs.moodle.org/26/en/Enrolment_FAQ

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Agile