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
    • 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?

        Gliffy Diagrams

        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 Skoda 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 Skoda 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 Skoda 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 Skoda 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 Skoda added a comment -

          Yes, thanks.

          Show
          Petr Skoda 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 Skoda 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 Skoda 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 Skoda added a comment - - edited

          +1

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

          Show
          Petr Skoda 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