Moodle
  1. Moodle
  2. MDL-12083

Choice should implement 'choice_role_unassign()' function in lib

    Details

    • Type: Bug Bug
    • Status: Development in progress
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.8.3, 1.9, 2.0
    • Fix Version/s: None
    • Component/s: Choice
    • Labels:
      None
    • Affected Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE, MOODLE_20_STABLE
    • Rank:
      1913

      Description

      Choice should implement this function (called by the role_unassign function in accesslib if it exists) to correctly remove choices when somebody is removed from the course. Otherwise they remain as 'ghost' choices and take up spaces that other cannot use.

        Issue Links

          Activity

          Hide
          Dan Marsden added a comment -

          I've been thinking about this for a while - (there are also several tracker items that elude to this issue), and this is partly different to the expected behaviour of Moodle courses when a user is unenrolled from the course - the data stays there, but as the user is unenrolled, it is no longer displayed....

          But this causes problems. - specifically with the Choices.

          Since there is a role_unassign function - I presume there is also a Role_assign function we can leverage too?

          perhaps a better solution would be to leverage the role_assign function, and check if the user has submitted to any choices which impose a limit, then check to see if their selected choice puts the choice over the limit, and if so, remove their currently selected choice?

          any thoughts?

          Dan

          Show
          Dan Marsden added a comment - I've been thinking about this for a while - (there are also several tracker items that elude to this issue), and this is partly different to the expected behaviour of Moodle courses when a user is unenrolled from the course - the data stays there, but as the user is unenrolled, it is no longer displayed.... But this causes problems. - specifically with the Choices. Since there is a role_unassign function - I presume there is also a Role_assign function we can leverage too? perhaps a better solution would be to leverage the role_assign function, and check if the user has submitted to any choices which impose a limit, then check to see if their selected choice puts the choice over the limit, and if so, remove their currently selected choice? any thoughts? Dan
          Hide
          Dan Marsden added a comment - - edited

          Adding MartinL to this - I've got a function here that might work (haven't tested it completely) but it should do all the right stuff - is this function too heavy on performance to add to role_assign?

          function choice_role_assign($userid, $context, $roleid) {
          $responses = get_records("choice_answers", "userid", $aa->userid); //get all choice answers for this user
          foreach ($responses as $responses) { // check each answer - if no answers, we don't need to do anything!
          $choice_instance = get_record("choice", "choiceid", $response->choiceid); // get the details on the choice for this answer
          if ($choice_instance->limitanswers==true) { //if this choice uses the limit function, we need to check to make sure there is still a free space.
          $option = get_record("choice_options", "choiceid", $response->choiceid, "id", $response->optionid);

          $all_responses = get_records("choice_answers", "choiceid", $response->choiceid);
          $numresponses = 0;
          foreach ($all_responses as $res) {
          if (has_capability('mod/choice:choose', $context))

          { $numresponses++; }

          }
          if ($option->maxanswers > $numresponses)

          { //re-enabling this answer for this user will cause the limit to be exceeded. so delete this option. delete_records('choice_answers', 'id', $response->id); }

          }
          }
          }

          Show
          Dan Marsden added a comment - - edited Adding MartinL to this - I've got a function here that might work (haven't tested it completely) but it should do all the right stuff - is this function too heavy on performance to add to role_assign? function choice_role_assign($userid, $context, $roleid) { $responses = get_records("choice_answers", "userid", $aa->userid); //get all choice answers for this user foreach ($responses as $responses) { // check each answer - if no answers, we don't need to do anything! $choice_instance = get_record("choice", "choiceid", $response->choiceid); // get the details on the choice for this answer if ($choice_instance->limitanswers==true) { //if this choice uses the limit function, we need to check to make sure there is still a free space. $option = get_record("choice_options", "choiceid", $response->choiceid, "id", $response->optionid); $all_responses = get_records("choice_answers", "choiceid", $response->choiceid); $numresponses = 0; foreach ($all_responses as $res) { if (has_capability('mod/choice:choose', $context)) { $numresponses++; } } if ($option->maxanswers > $numresponses) { //re-enabling this answer for this user will cause the limit to be exceeded. so delete this option. delete_records('choice_answers', 'id', $response->id); } } } }
          Hide
          Martín Langhoff added a comment -

          Hi Dan - would it be possible to set things so that ...?

          • choice_role_assign() does something very cheap, like marking a "we have a new role-assignment for user X" somewhere, like cached_flags for example.
          • the rest of the job is done by choice_cron(), which checks if there's anything "interesting" in cached_flags

          The tricky thing IMHO would be that cached_flags entries are not considered sacred, so they may be lost / timeout if cron.php isn't being run / etc – so a low-frequency cronjob that does a wider sanity check on those choice slots would be good, OR a more reliable storage place.

          Show
          Martín Langhoff added a comment - Hi Dan - would it be possible to set things so that ...? choice_role_assign() does something very cheap, like marking a "we have a new role-assignment for user X" somewhere, like cached_flags for example. the rest of the job is done by choice_cron(), which checks if there's anything "interesting" in cached_flags The tricky thing IMHO would be that cached_flags entries are not considered sacred, so they may be lost / timeout if cron.php isn't being run / etc – so a low-frequency cronjob that does a wider sanity check on those choice slots would be good, OR a more reliable storage place.
          Hide
          Dan Marsden added a comment -

          hmm what about something like this:

          add a new field to choice_responses something like "newenrol".

          When choice_role_assign runs, it finds all responses from that user and sets the newenrol flag to "1"

          as the user has enrolled, until the cron runs, the user will show up in the choice as selecting the option, and it will exceed the limit..

          in choice_cron, we check all responses with the newenrol flag set to 1 and make sure the limit isn't exceeded.

          If the limit is exceeded, we send an e-mail to all teachers notifying them that a user has re-enrolled, but their choice response has been removed as it exceeds the limit currently set.

          ....we could even make this a setting in the choice config.....
          admins could set "what to do with responses that tip the limit over the edge." - email admin, email teacher, email student,

          and also whether or not to "remove" the existing response....

          I'm liking that solution better myself - what do you think Howard?

          Dan

          Show
          Dan Marsden added a comment - hmm what about something like this: add a new field to choice_responses something like "newenrol". When choice_role_assign runs, it finds all responses from that user and sets the newenrol flag to "1" as the user has enrolled, until the cron runs, the user will show up in the choice as selecting the option, and it will exceed the limit.. in choice_cron, we check all responses with the newenrol flag set to 1 and make sure the limit isn't exceeded. If the limit is exceeded, we send an e-mail to all teachers notifying them that a user has re-enrolled, but their choice response has been removed as it exceeds the limit currently set. ....we could even make this a setting in the choice config..... admins could set "what to do with responses that tip the limit over the edge." - email admin, email teacher, email student, and also whether or not to "remove" the existing response.... I'm liking that solution better myself - what do you think Howard? Dan
          Hide
          Wen Hao Chuang added a comment -

          Hi Dan and Howard, any update on this? Thanks!

          Show
          Wen Hao Chuang added a comment - Hi Dan and Howard, any update on this? Thanks!
          Hide
          Dan Marsden added a comment -

          as mentioned in my last comment, I'd like some feedback on this before I make any changes - Wen, what are your thoughts on the way to implement this?

          Show
          Dan Marsden added a comment - as mentioned in my last comment, I'd like some feedback on this before I make any changes - Wen, what are your thoughts on the way to implement this?
          Hide
          Miro Goepel added a comment -

          Hi,

          I was able to reproduce a bug in the choice module which may be connected to this issue: http://moodle.org/mod/forum/discuss.php?d=118433#p606048

          Show
          Miro Goepel added a comment - Hi, I was able to reproduce a bug in the choice module which may be connected to this issue: http://moodle.org/mod/forum/discuss.php?d=118433#p606048
          Hide
          Dan Marsden added a comment -

          yep - this is a known issue, but no-one has given any feedback on what they think "should" happen in this situation.

          Show
          Dan Marsden added a comment - yep - this is a known issue, but no-one has given any feedback on what they think "should" happen in this situation.
          Hide
          Paul Holden added a comment -

          Hi Dan,

          I've created a branch on my github that adds an event handler to the choice module that removes response data from a user when they are unenrolled from a course; https://github.com/paulholden/moodle/compare/master...MDL-12083

          Show
          Paul Holden added a comment - Hi Dan, I've created a branch on my github that adds an event handler to the choice module that removes response data from a user when they are unenrolled from a course; https://github.com/paulholden/moodle/compare/master...MDL-12083
          Hide
          Dan Marsden added a comment -

          Thanks Paul - my problem is that I'm not sure that removing response data when a user is unenrolled is correct behaviour.

          Show
          Dan Marsden added a comment - Thanks Paul - my problem is that I'm not sure that removing response data when a user is unenrolled is correct behaviour.
          Hide
          Dan Marsden added a comment -

          still waiting on feedback - some people I talk to think this is really just expected behaviour but I suppose we could improve documentation?

          Show
          Dan Marsden added a comment - still waiting on feedback - some people I talk to think this is really just expected behaviour but I suppose we could improve documentation?
          Hide
          Howard Miller added a comment - - edited

          Just backtracking and possibly stating the obvious...

          • this always turned up for me were choice was being used for things like assigning tutorial group lists and suchlike. so...
          • the limit on the choice cannot be violated (it could be the number of seats in the room for example)
          • the owner expects that if a student is removed from the course the 'space' will become available, especially as it looks like it should be.

          For me, I don't have a big problem with breaking the 'keep the data' rule here. If the user is unenrolled they are removed from the choice. I suspect this could be an admin option? Again, I wouldn't go mad. What about

          • delete unenrolled users
          • keep data and allow additional choices (re-enrolled students may tip it over)
          • keep data and do not allow additional choices (the status quo)

          None of it is entirely ideal but it's better than it is.

          OR...

          Just make the 'unenrolled' slots stay visible (but styled and annotated in a way that doesn't cause additional confusion) and allow the teacher to remove them. That would also satisfy me.

          OR...

          The above and notify teachers if someone drops out of the course who is in a choice (with a limit)

          Show
          Howard Miller added a comment - - edited Just backtracking and possibly stating the obvious... this always turned up for me were choice was being used for things like assigning tutorial group lists and suchlike. so... the limit on the choice cannot be violated (it could be the number of seats in the room for example) the owner expects that if a student is removed from the course the 'space' will become available, especially as it looks like it should be. For me, I don't have a big problem with breaking the 'keep the data' rule here. If the user is unenrolled they are removed from the choice. I suspect this could be an admin option? Again, I wouldn't go mad. What about delete unenrolled users keep data and allow additional choices (re-enrolled students may tip it over) keep data and do not allow additional choices (the status quo) None of it is entirely ideal but it's better than it is. OR... Just make the 'unenrolled' slots stay visible (but styled and annotated in a way that doesn't cause additional confusion) and allow the teacher to remove them. That would also satisfy me. OR... The above and notify teachers if someone drops out of the course who is in a choice (with a limit)

            People

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

              Dates

              • Created:
                Updated: