Moodle
  1. Moodle
  2. MDL-29538

Conditional code based on user fields.

    Details

    • Testing Instructions:
      Hide

      1. Enable conditional activities on your Moodle site.
      2. Create some random custom profile fields.
      3. Create a course, and enrol a student into the course.
      4. Create an activity and restrict the activity using all the custom profile fields, and user fields in the drop down (make sure you use all the types of operators as well (equal, contains, is empty etc))
      5. Restrict a whole topic section as well
      6. Log in as the student and visit the course.
      7. Ensure the message explaining that the items being restricted makes sense.
      8. One by one fulfill all the requirements that you have put in place on the restriction for that particular user and check to see if the restriction disappears in the message.
      9. Once all have been done, make sure you can access the activity/section.
      10. Backup and restore the course!

      Just a note, we have a large client that has been using this code heavily for 8 months with no issues.

      Show
      1. Enable conditional activities on your Moodle site. 2. Create some random custom profile fields. 3. Create a course, and enrol a student into the course. 4. Create an activity and restrict the activity using all the custom profile fields, and user fields in the drop down (make sure you use all the types of operators as well (equal, contains, is empty etc)) 5. Restrict a whole topic section as well 6. Log in as the student and visit the course. 7. Ensure the message explaining that the items being restricted makes sense. 8. One by one fulfill all the requirements that you have put in place on the restriction for that particular user and check to see if the restriction disappears in the message. 9. Once all have been done, make sure you can access the activity/section. 10. Backup and restore the course! Just a note, we have a large client that has been using this code heavily for 8 months with no issues.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-29538_usercond
    • Rank:
      19015

      Description

      I have developed conditional activities based on user profile fields and custom user profile fields. This means it is possible to create a new activity and restrict it to only a certain group of individuals. You are able to specify whether a field "is empty", "contains", "does not contain", "is equal to", "starts with" or "ends with" a given value.

      Example: restrict an activity to user who's -

      Email ends with "moodle.com"
      Department does not contain "Temporary Staff"
      City/town is equal to "Perth"

      Please note, I copied the coding style of existing code conditionlib so that it was consistent but am happy to clean it up or for someone else to clean it up if people would rather it followed Moodle coding guidelines.

      1. code review.txt
        8 kB
        Sam Marshall
      1. courseview.jpg
        38 kB
      2. restrictaccess.jpg
        151 kB

        Issue Links

          Activity

          Hide
          Mark Nelson added a comment -

          This code works but still requires some fine tuning and more testing. I thought I would gage the Moodle community's interest before spending more time on it.

          Show
          Mark Nelson added a comment - This code works but still requires some fine tuning and more testing. I thought I would gage the Moodle community's interest before spending more time on it.
          Hide
          Julian Ridden added a comment -

          would be great to see the ability to have conditional based on group or cohort. Many use groupings for this, but a conditional would make it a lot easier and straight forward

          Show
          Julian Ridden added a comment - would be great to see the ability to have conditional based on group or cohort. Many use groupings for this, but a conditional would make it a lot easier and straight forward
          Hide
          Michael de Raadt added a comment -

          Sounds good, Mark.

          Show
          Michael de Raadt added a comment - Sounds good, Mark.
          Hide
          Andrea Bicciolo added a comment -

          The idea sounds very interesting. Also, the proposal of Julian (adding conditional support for cohort and groups) may lead to very interesting scenarios.My +1

          Show
          Andrea Bicciolo added a comment - The idea sounds very interesting. Also, the proposal of Julian (adding conditional support for cohort and groups) may lead to very interesting scenarios.My +1
          Hide
          Adi Tedjasaputra added a comment - - edited

          Good idea Mark!

          Show
          Adi Tedjasaputra added a comment - - edited Good idea Mark!
          Hide
          Mark Nelson added a comment -

          Julian, I am not sure about adding the ability to lock it by cohorts. Cohorts are a site wide group used to enrol users into courses and does not really apply to individual courses. I could limit the cohorts to those that apply to the individual course, but in that cause I may as well be using groups. However, if I did add the functionality to make it conditional via groups, it would make groupings redundant which I am not sure the Moodle HQ wants. Btw, I have only touched base on cohorts, groups and groupings so may be missing something.

          Show
          Mark Nelson added a comment - Julian, I am not sure about adding the ability to lock it by cohorts. Cohorts are a site wide group used to enrol users into courses and does not really apply to individual courses. I could limit the cohorts to those that apply to the individual course, but in that cause I may as well be using groups. However, if I did add the functionality to make it conditional via groups, it would make groupings redundant which I am not sure the Moodle HQ wants. Btw, I have only touched base on cohorts, groups and groupings so may be missing something.
          Hide
          Andrea Bicciolo added a comment -

          Mark, while having a way to assign users to cohort based on user profile filed could be a powerful tool in several scenarios, reflecting on this in my opinion you are right, as cohort are created in higher contexts than activity context and are intended to enroll user into courses.

          Show
          Andrea Bicciolo added a comment - Mark, while having a way to assign users to cohort based on user profile filed could be a powerful tool in several scenarios, reflecting on this in my opinion you are right, as cohort are created in higher contexts than activity context and are intended to enroll user into courses.
          Hide
          Julian Ridden added a comment -

          I can't see HQ having issues.

          Groupings was always meant for groups of groups. It is a silly hack that teachers use to create a group and stick it in a grouping just to enable a resource for just a group. I can't see HQ arguing usability on this one. ANyone from HQ able to comment. I would hate for a perceived fear of change to stop what could be a really useful development.

          Julian

          Show
          Julian Ridden added a comment - I can't see HQ having issues. Groupings was always meant for groups of groups. It is a silly hack that teachers use to create a group and stick it in a grouping just to enable a resource for just a group. I can't see HQ arguing usability on this one. ANyone from HQ able to comment. I would hate for a perceived fear of change to stop what could be a really useful development. Julian
          Hide
          Kay Patterson added a comment -

          This for me is absolutely brilliant and thank you Mark! Perfect for what I am needing at the moment.
          I'm going to go try as soon as I can and give you full feedback.
          Kay

          Show
          Kay Patterson added a comment - This for me is absolutely brilliant and thank you Mark! Perfect for what I am needing at the moment. I'm going to go try as soon as I can and give you full feedback. Kay
          Hide
          Kay Patterson added a comment -

          Actually is there anyway i can try this mod. I am at the start of specifying and testing viability of moodle for a project that I propose to use in a rather creative way and just today (while in the bath - a proper eureka moment!) realised that what I need to do is what you are doing here and then I found this.

          However I have come back to moodle after a lot of years away and i have no idea how this tracker area works etc.

          Short version of this ramble - is there anyway I can get hold of your work as you progress as I'd love to thrash it about for you.

          Kay

          Show
          Kay Patterson added a comment - Actually is there anyway i can try this mod. I am at the start of specifying and testing viability of moodle for a project that I propose to use in a rather creative way and just today (while in the bath - a proper eureka moment!) realised that what I need to do is what you are doing here and then I found this. However I have come back to moodle after a lot of years away and i have no idea how this tracker area works etc. Short version of this ramble - is there anyway I can get hold of your work as you progress as I'd love to thrash it about for you. Kay
          Hide
          Mark Drechsler added a comment -

          Agree with Julian that having to use groupings to perform a 'selective release' (in the old WebCT terminology) is one of the dumbest work arounds I have to explain to new users. It would make far more sense to be able to restrict availability based purely on a group, and if an extension of this patch lets that happen then it would get my vote.

          As for the 'true meaning' of groupings, I've always explained them as primarily existing to allow lecturers to run multiple types of groups within a course (tutorials and prac groups for example) without tripping up on the filtering of activities to the different group types. Using it to restrict to individual groups is, like Julian said, a bit of a silly functional hack to achieve something which should be able to be done much more simply I reckon.

          Locking by cohorts? I can't understand the argument that this shouldn't be done because cohorts are set in a different context, since user fields exist in a different context too - unless I'm missing something else in the argument

          Thanks to Mark N for the idea and code regardless of the nuances - nice work

          Show
          Mark Drechsler added a comment - Agree with Julian that having to use groupings to perform a 'selective release' (in the old WebCT terminology) is one of the dumbest work arounds I have to explain to new users. It would make far more sense to be able to restrict availability based purely on a group, and if an extension of this patch lets that happen then it would get my vote. As for the 'true meaning' of groupings, I've always explained them as primarily existing to allow lecturers to run multiple types of groups within a course (tutorials and prac groups for example) without tripping up on the filtering of activities to the different group types. Using it to restrict to individual groups is, like Julian said, a bit of a silly functional hack to achieve something which should be able to be done much more simply I reckon. Locking by cohorts? I can't understand the argument that this shouldn't be done because cohorts are set in a different context, since user fields exist in a different context too - unless I'm missing something else in the argument Thanks to Mark N for the idea and code regardless of the nuances - nice work
          Hide
          gavin henrick added a comment -

          Having seen systems like ELIS and Totara, I think this possibly would be best split into 2 aspects:

          a) having membership of a cohort depend on user fields
          b) having conditional access based on cohort (or better an automatic group created in the course for each cohort)

          I think that perhaps linking conditionals to a user related aspect than a course aspect is not the best approach for abstraction, but that is just how I think about it.

          I think it may be better approach to keep conditionals based on course related (like completion, grade, time, or a group (in this case)) than a user aspect (user field)

          Show
          gavin henrick added a comment - Having seen systems like ELIS and Totara, I think this possibly would be best split into 2 aspects: a) having membership of a cohort depend on user fields b) having conditional access based on cohort (or better an automatic group created in the course for each cohort) I think that perhaps linking conditionals to a user related aspect than a course aspect is not the best approach for abstraction, but that is just how I think about it. I think it may be better approach to keep conditionals based on course related (like completion, grade, time, or a group (in this case)) than a user aspect (user field)
          Hide
          Minh-Tam Nguyen added a comment -

          +1 to Julian and Mark D's comments on why it is important to be able to set conditionals for groups within a course without having to resort to the groupings/dimensions of groups hack.

          Show
          Minh-Tam Nguyen added a comment - +1 to Julian and Mark D's comments on why it is important to be able to set conditionals for groups within a course without having to resort to the groupings/dimensions of groups hack.
          Hide
          Mark Nelson added a comment -

          Thanks Mark!

          I have been off sick the past week, so have had no time to look into this any further. I will hopefully add the ability to make events conditional via cohorts and groups by the end of next week. I will keep you updated here.

          Show
          Mark Nelson added a comment - Thanks Mark! I have been off sick the past week, so have had no time to look into this any further. I will hopefully add the ability to make events conditional via cohorts and groups by the end of next week. I will keep you updated here.
          Hide
          Julian Ridden added a comment -

          Thanks for the update Mark and building in the suggested changes.

          Show
          Julian Ridden added a comment - Thanks for the update Mark and building in the suggested changes.
          Hide
          Andrea Bicciolo added a comment - - edited

          The comments appears all quite sensible to me, if cohort support can be added into the conditional activities, then OK. However, a separate feature allowing also to assign users to cohort according to user fields (either standard and custom) would be really useful too and satisfy broader usage scenarios. Maybe for this it is needed another tracker issue if not already present ?

          Show
          Andrea Bicciolo added a comment - - edited The comments appears all quite sensible to me, if cohort support can be added into the conditional activities, then OK. However, a separate feature allowing also to assign users to cohort according to user fields (either standard and custom) would be really useful too and satisfy broader usage scenarios. Maybe for this it is needed another tracker issue if not already present ?
          Hide
          Sam Marshall added a comment -

          Thanks for your work on this feature. It looks good. I haven't had time to check it yet, maybe ask me for review in a few weeks. At least based on the functionality/screenshots I would be happy to submit it for integration.

          Regarding coding standards, I would suggest that at least on the trivial level (whitespace, naming, etc.) new code should generally follow coding standards rather than try to match other existing code in a file.

          If you do implement cohort/group support (which I suspect is a bad idea) please do it separately. It's best to keep different changes apart.

          Show
          Sam Marshall added a comment - Thanks for your work on this feature. It looks good. I haven't had time to check it yet, maybe ask me for review in a few weeks. At least based on the functionality/screenshots I would be happy to submit it for integration. Regarding coding standards, I would suggest that at least on the trivial level (whitespace, naming, etc.) new code should generally follow coding standards rather than try to match other existing code in a file. If you do implement cohort/group support (which I suspect is a bad idea) please do it separately. It's best to keep different changes apart.
          Hide
          Mark Nelson added a comment -

          Hi Kay,

          Missed your question. This isn't exactly a mod, as it requires editing core code.

          If you visit the link https://github.com/markn86/moodle/compare/MOODLE_21_STABLE...userconditions you will be given a list of all the files that were edited to make this possible, as well as the list of changes on each file. Please make sure you know what you are doing before editing these files as inserting in the incorrect place could cause your site to fail.

          Show
          Mark Nelson added a comment - Hi Kay, Missed your question. This isn't exactly a mod, as it requires editing core code. If you visit the link https://github.com/markn86/moodle/compare/MOODLE_21_STABLE...userconditions you will be given a list of all the files that were edited to make this possible, as well as the list of changes on each file. Please make sure you know what you are doing before editing these files as inserting in the incorrect place could cause your site to fail.
          Hide
          Mark Nelson added a comment -

          Hi Sam,

          Any idea when the code will be reviewed for core submission?

          Regards,

          Mark

          Show
          Mark Nelson added a comment - Hi Sam, Any idea when the code will be reviewed for core submission? Regards, Mark
          Hide
          Matthew Cannings added a comment -

          +1 for Julian's suggestion as I have always found Groupings overly complicated. Having the condition... Member of group ...would be great for me. Could conditions be made into plugins? If they were plugins I would enable Group and Userfield but disable Cohort!

          Show
          Matthew Cannings added a comment - +1 for Julian's suggestion as I have always found Groupings overly complicated. Having the condition... Member of group ...would be great for me. Could conditions be made into plugins? If they were plugins I would enable Group and Userfield but disable Cohort!
          Hide
          Artem Andreev added a comment -

          Seems as different issue exists (MDL-30554) for adding conditions based on groups...

          +1 for simplification system of groups/grouping/cohorts.

          Show
          Artem Andreev added a comment - Seems as different issue exists ( MDL-30554 ) for adding conditions based on groups... +1 for simplification system of groups/grouping/cohorts.
          Hide
          Mark Nelson added a comment -

          Hi Guys, I created a separate tracker issue for groups. Please visit the link http://tracker.moodle.org/browse/MDL-30554 to show your interest.

          Show
          Mark Nelson added a comment - Hi Guys, I created a separate tracker issue for groups. Please visit the link http://tracker.moodle.org/browse/MDL-30554 to show your interest.
          Hide
          Sam Marshall added a comment -

          I'm sorry it took me so long but I have finally done a code review.

          Because the code review is long (I try to be thorough with these things, which may be why it takes me forever to get around to them) I am attaching it as a text file rather than inline.

          Some notes:

          1) The code is very high quality, well done!

          2) I did not actually try to run it yet

          3) While you fix the problems, could you also rebase on current master (this feature should go into 2.3).

          4) I didn't check - are there existing unit tests for conditionlib, like for the grade conditions for instance? If there are, could you add at least a basic unit test for this new functionality. If there aren't, I guess you're off the hook.

          5) Talking of testing, could you write a brief (eg ~10 point) step-by-step script for manual testing of the feature, these can go in the testing instructions field of this bug (uh if you can access it - I think 'request peer review' might give you that box) or just in a comment and I can move them.

          There are quite a lot of issues to address but really it looks like it can be a solid feature. If the problems identified in code review are fixed I will certainly submit for integration into 2.3.

          Thanks for all your work on this so far.

          Show
          Sam Marshall added a comment - I'm sorry it took me so long but I have finally done a code review. Because the code review is long (I try to be thorough with these things, which may be why it takes me forever to get around to them) I am attaching it as a text file rather than inline. Some notes: 1) The code is very high quality, well done! 2) I did not actually try to run it yet 3) While you fix the problems, could you also rebase on current master (this feature should go into 2.3). 4) I didn't check - are there existing unit tests for conditionlib, like for the grade conditions for instance? If there are, could you add at least a basic unit test for this new functionality. If there aren't, I guess you're off the hook. 5) Talking of testing, could you write a brief (eg ~10 point) step-by-step script for manual testing of the feature, these can go in the testing instructions field of this bug (uh if you can access it - I think 'request peer review' might give you that box) or just in a comment and I can move them. There are quite a lot of issues to address but really it looks like it can be a solid feature. If the problems identified in code review are fixed I will certainly submit for integration into 2.3. Thanks for all your work on this so far.
          Hide
          Sam Marshall added a comment -

          (ahem - just thought I should probably put the bug through the right stages for peer review...)

          Show
          Sam Marshall added a comment - (ahem - just thought I should probably put the bug through the right stages for peer review...)
          Hide
          Mark Nelson added a comment -

          Hi Sam,

          Thanks for that.

          I will get round to this eventually, however, currently my development time is spent on work and re-writing the certificate module. I will probably have to address this when I have finished with the certificate changes (hopefully by the end of the month).

          Great code review, maybe you could do it for the certificate module as well when it is ready for testing!

          Regards,

          Mark

          Show
          Mark Nelson added a comment - Hi Sam, Thanks for that. I will get round to this eventually, however, currently my development time is spent on work and re-writing the certificate module. I will probably have to address this when I have finished with the certificate changes (hopefully by the end of the month). Great code review, maybe you could do it for the certificate module as well when it is ready for testing! Regards, Mark
          Hide
          Julian Ridden added a comment -

          any movements on getting this into the integration stream for 2.3?

          Show
          Julian Ridden added a comment - any movements on getting this into the integration stream for 2.3?
          Hide
          Mark Nelson added a comment - - edited

          Yeh, I am in the process of doing it now actually.

          I just did a rebase on my branch with master - that was a headache in itself. I feel sorry for Eloy who has to manage the central repo all the time.

          Going to address the issues that Sam mentioned. Not sure when deadline for core code is coming up but I do want to make it (lets hope that Sam can test it with such little notice)

          Show
          Mark Nelson added a comment - - edited Yeh, I am in the process of doing it now actually. I just did a rebase on my branch with master - that was a headache in itself. I feel sorry for Eloy who has to manage the central repo all the time. Going to address the issues that Sam mentioned. Not sure when deadline for core code is coming up but I do want to make it (lets hope that Sam can test it with such little notice)
          Hide
          Mark Nelson added a comment - - edited

          Hi Sam,

          I believe I have finished the changes you suggested, are you able to re-review before the 2.3 release in the hopes it can be merged? Sorry for the late notice.

          Regarding the modinfolib.php file - I just copied the logic used by the conditionsgrade array, which states that declaring the availability and completion fields, even if these settings are turned off, saves database space. These are the only changes I made because that is all there were for the conditionsgrade array. Isn't the caching done in lib/conditionlib.php? There is a call in modinfolib.php to is_available, which uses the data (if it exists) in the usersfieldcache.

          Regards,

          Mark

          Show
          Mark Nelson added a comment - - edited Hi Sam, I believe I have finished the changes you suggested, are you able to re-review before the 2.3 release in the hopes it can be merged? Sorry for the late notice. Regarding the modinfolib.php file - I just copied the logic used by the conditionsgrade array, which states that declaring the availability and completion fields, even if these settings are turned off, saves database space. These are the only changes I made because that is all there were for the conditionsgrade array. Isn't the caching done in lib/conditionlib.php? There is a call in modinfolib.php to is_available, which uses the data (if it exists) in the usersfieldcache. Regards, Mark
          Hide
          Mark Nelson added a comment -

          Any chance of this making it in 2.3 Sam?

          Show
          Mark Nelson added a comment - Any chance of this making it in 2.3 Sam?
          Hide
          Martin Dougiamas added a comment -

          This looks really useful, has been peer reviewed, and has been tested in production for a long time.

          My +1 to integrate if possible ...

          Show
          Martin Dougiamas added a comment - This looks really useful, has been peer reviewed, and has been tested in production for a long time. My +1 to integrate if possible ...
          Hide
          Martin Dougiamas added a comment -

          Mark can you

          1) Link the user docs (some page in docs.moodle.org/dev)
          2) Write some QA tests we can add to MDLQA-1

          Show
          Martin Dougiamas added a comment - Mark can you 1) Link the user docs (some page in docs.moodle.org/dev) 2) Write some QA tests we can add to MDLQA-1
          Hide
          Martin Dougiamas added a comment -

          Moved docs link to http://docs.moodle.org/dev/Conditional_user_fields

          It's not a 2.2 feature and should not be in those docs!

          Show
          Martin Dougiamas added a comment - Moved docs link to http://docs.moodle.org/dev/Conditional_user_fields It's not a 2.2 feature and should not be in those docs!
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I've been discussing this issue with Mark, Eloy and just now Martin.
          The code is 99% perfect now, just one remaining backup and restore issue.
          Mark has done an awesome job giving this every opportunity available to get into 2.3 but unfortunately this will be delayed. Sorry all. Its been delayed in the interests of safety given this collided with other new features, introduced new database structure, and was looked at late in the picture for 2.3.
          Rest assured as soon as master is accepting changes again after the release of 2.3 this will be interated and will be one of the first new features we can be certain about for 2.4.

          A big thank you to Mark and everyone else involved here and I will see this integrated once possible (I believe we will be coming out of master freeze round the end of June).

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've been discussing this issue with Mark, Eloy and just now Martin. The code is 99% perfect now, just one remaining backup and restore issue. Mark has done an awesome job giving this every opportunity available to get into 2.3 but unfortunately this will be delayed. Sorry all. Its been delayed in the interests of safety given this collided with other new features, introduced new database structure, and was looked at late in the picture for 2.3. Rest assured as soon as master is accepting changes again after the release of 2.3 this will be interated and will be one of the first new features we can be certain about for 2.4. A big thank you to Mark and everyone else involved here and I will see this integrated once possible (I believe we will be coming out of master freeze round the end of June). Cheers Sam
          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
          Mark Nelson added a comment -

          All done

          Show
          Mark Nelson added a comment - All done
          Hide
          Dan Poltawski added a comment -

          Taking integration held issues out of integration (whilst we are keeping master and 23_STABLE in sync).

          Show
          Dan Poltawski added a comment - Taking integration held issues out of integration (whilst we are keeping master and 23_STABLE in sync).
          Hide
          Sam Hemelryk added a comment -

          Thanks Mark, I've integrated this now and its ready for testing.
          I'll proceed to create an issue to address the bug that exists at the moment with backup/restore. We should be comparing user field details before restoring conditions.

          Show
          Sam Hemelryk added a comment - Thanks Mark, I've integrated this now and its ready for testing. I'll proceed to create an issue to address the bug that exists at the moment with backup/restore. We should be comparing user field details before restoring conditions.
          Hide
          Ankit Agarwal added a comment -

          Hi Guys,

          1. I donot see a way of specifying if a checkbox "type" of custom field should be checked or not
          2. how do I specify if "Date" type of profile field, should be in a given date range?
          3. is not empty should be an option as well IMHO
          4. I created a "menu" type of field and selected it a condition that it should contain "4", still a student with a value of "2" was able to access the activity.

          Failing this.Sorry.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Guys, I donot see a way of specifying if a checkbox "type" of custom field should be checked or not how do I specify if "Date" type of profile field, should be in a given date range? is not empty should be an option as well IMHO I created a "menu" type of field and selected it a condition that it should contain "4", still a student with a value of "2" was able to access the activity. Failing this.Sorry. Thanks
          Hide
          Mark Nelson added a comment -

          Ankit,

          1. How do you propose we do this? This would only be an option for checkbox fields, it makes no sense to add this to the drop down for all types of fields. Do we use JS that dynamically changes the options to choose? I do not see why the user can not select 'isempty', we could specify that in the help pop-up.

          2. Again, this is a unique field. I don't expect the user to enter a unix timestamp, but I don't see how we can do this without JS changing the options to date selectors where you can set a range which only adds another layer of complexity. Maybe we should just skip adding these fields to the drop down list.

          3. Sure, easy enough to add if others agree.

          4. I could not replicate this issue. I created a menu custom field with the following -

          One
          1
          Two123
          4

          Selected the option 'Two123' for the user in that course, visited the course and could not see the activity I had set to only allow users with the menu custom field set to a value containing '4', which is the correct behaviour. What were your options and what value was chosen?

          Show
          Mark Nelson added a comment - Ankit, 1. How do you propose we do this? This would only be an option for checkbox fields, it makes no sense to add this to the drop down for all types of fields. Do we use JS that dynamically changes the options to choose? I do not see why the user can not select 'isempty', we could specify that in the help pop-up. 2. Again, this is a unique field. I don't expect the user to enter a unix timestamp, but I don't see how we can do this without JS changing the options to date selectors where you can set a range which only adds another layer of complexity. Maybe we should just skip adding these fields to the drop down list. 3. Sure, easy enough to add if others agree. 4. I could not replicate this issue. I created a menu custom field with the following - One 1 Two123 4 Selected the option 'Two123' for the user in that course, visited the course and could not see the activity I had set to only allow users with the menu custom field set to a value containing '4', which is the correct behaviour. What were your options and what value was chosen?
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          After reading through Ankit's report and Marks feedback I have the following thoughts.

          a) We should open a new feature issue to investigate allowing field types to specify additional custom conditions. I have no thoughts about the detail of this but certainly I would consider it a new feature, or improvement. One that I'm sure would be popular. If we open the issue then we've got somewhere to work on the details of that and somewhere for people to vote and work it onto our development lists. This deals with both points 1 and 2.

          b) Add "is not empty" as an option.

          c) We need to fix this issue either now (very easy fix?) or as another issue made a blocker. Because this is a master only change we can fix this as a new bug and that is perhaps the way to go. Before we decide I would like it to be investigated to ensure it is not a serious issue.

          As mentioned I could reproduce the bug and I think I can explain it in more detail.

          • Log in as an admin
          • Create a new Menu profile field with the following details:
            • Shortname: favouritecolour
            • Name: What is your favourite colour?
            • Required: Yes
            • Display on signup page: Yes
            • Options: Undecided, Red, Green, Blue, Yellow, Orange, Pink, Purple, Magenta, Cyan, Turquoise, Brown
            • Default: Undecided
          • Browse to a course
          • Add a forum activity called what ever you want and add the following condition:
            • User field: What is your favourite colour?
            • doesn't contain
            • Undecided
          • Save the forum and log out.
          • Log in as a student of the course.
          • Browse to the course.
          • Confirm: The activity is shown as available.
          • Go to the page to edit your profile
          • Click the save button without making any changes
          • Browse back to the course.
          • Confirm: The activity is now shown as greyed out.

          If someone could please run through that to confirm others are seeing what I saw that would be great.
          Ankit, it would be also good if you could confirm this sounds a bit like what you did or whether it sounds like something separate.
          If it looks the same and others can reproduce it then I think this is our error.
          User profile fields aren't being processed correctly. When created we don't set the default value for all users. It is assumed (so that if the default value changes we don't need to try to work out who has selected what). Essentially an empty value should mean use the default value. I'd bet that the code fetching the user fields isn't taking that into account (perhaps the wrong API method).
          As soon as the user edits there profile and saves the default value is saved as a value for the user and things work again.
          Hopefully its just a case of looking through the user profile API and selecting a better method to use.
          Mark please ping me when you are online so that we can have a chat about this.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, After reading through Ankit's report and Marks feedback I have the following thoughts. a) We should open a new feature issue to investigate allowing field types to specify additional custom conditions. I have no thoughts about the detail of this but certainly I would consider it a new feature, or improvement. One that I'm sure would be popular. If we open the issue then we've got somewhere to work on the details of that and somewhere for people to vote and work it onto our development lists. This deals with both points 1 and 2. b) Add "is not empty" as an option. c) We need to fix this issue either now (very easy fix?) or as another issue made a blocker. Because this is a master only change we can fix this as a new bug and that is perhaps the way to go. Before we decide I would like it to be investigated to ensure it is not a serious issue. As mentioned I could reproduce the bug and I think I can explain it in more detail. Log in as an admin Create a new Menu profile field with the following details: Shortname: favouritecolour Name: What is your favourite colour? Required: Yes Display on signup page: Yes Options: Undecided, Red, Green, Blue, Yellow, Orange, Pink, Purple, Magenta, Cyan, Turquoise, Brown Default: Undecided Browse to a course Add a forum activity called what ever you want and add the following condition: User field: What is your favourite colour? doesn't contain Undecided Save the forum and log out. Log in as a student of the course. Browse to the course. Confirm: The activity is shown as available. Go to the page to edit your profile Click the save button without making any changes Browse back to the course. Confirm: The activity is now shown as greyed out. If someone could please run through that to confirm others are seeing what I saw that would be great. Ankit, it would be also good if you could confirm this sounds a bit like what you did or whether it sounds like something separate. If it looks the same and others can reproduce it then I think this is our error. User profile fields aren't being processed correctly. When created we don't set the default value for all users. It is assumed (so that if the default value changes we don't need to try to work out who has selected what). Essentially an empty value should mean use the default value. I'd bet that the code fetching the user fields isn't taking that into account (perhaps the wrong API method). As soon as the user edits there profile and saves the default value is saved as a value for the user and things work again. Hopefully its just a case of looking through the user profile API and selecting a better method to use. Mark please ping me when you are online so that we can have a chat about this. Cheers Sam
          Hide
          Ankit Agarwal added a comment - - edited

          Hi Sam,
          I did some more testing and consolidated steps to replicate my issue. It is a bit different from what you suggested, which clearly is an issue in itself.

          1. Create a menu profile field and set options as 1,2,3,4
          2. Edit an activity and set the condition so that it is visible if the menu field contains 4
          3. Login as student and make sure it is activity is not available.
          4. Goto your profile and set the menu field to 4
          5. Return to course and activity is still not available.
          6. Logout and log back in, now you should be able to access the activity.

          That is the activity's status doesn't change unless student logsout and than logs back in.

          Also to note I guess the checkbox kind of fields be handled with "is empty" and "is not empty" options IMO.

          Thanks

          Show
          Ankit Agarwal added a comment - - edited Hi Sam, I did some more testing and consolidated steps to replicate my issue. It is a bit different from what you suggested, which clearly is an issue in itself. Create a menu profile field and set options as 1,2,3,4 Edit an activity and set the condition so that it is visible if the menu field contains 4 Login as student and make sure it is activity is not available. Goto your profile and set the menu field to 4 Return to course and activity is still not available. Logout and log back in, now you should be able to access the activity. That is the activity's status doesn't change unless student logsout and than logs back in. Also to note I guess the checkbox kind of fields be handled with "is empty" and "is not empty" options IMO. Thanks
          Hide
          Mark Nelson added a comment -

          Ah right, so there is a is not empty condition (should have looked to verify), so imo that is enough for checked profile field types.

          The other issues -

          1) If a user changes a custom profile field by editing their profile which then matches the criteria to access an activity, they still can not access it and vice versa. This issue lies in the function get_cached_user_profile_field which is responsible for caching the user values. I suggest creating another tracker issue for this. Basically just clearing $SESSION->userfieldcache when the user saves their profile would fix this.

          2) Restoring a backup of a course with user conditions to another site we need to check the shortname of the user profile field to see if it exists before restoring. Another tracker issue already exists for this.

          Sam, allowing fields to specify their own conditions sounds like a plugin system. This would be pretty difficult to program as it may require a more complex table structure to accommodate for all the possibilities.

          Show
          Mark Nelson added a comment - Ah right, so there is a is not empty condition (should have looked to verify), so imo that is enough for checked profile field types. The other issues - 1) If a user changes a custom profile field by editing their profile which then matches the criteria to access an activity, they still can not access it and vice versa. This issue lies in the function get_cached_user_profile_field which is responsible for caching the user values. I suggest creating another tracker issue for this. Basically just clearing $SESSION->userfieldcache when the user saves their profile would fix this. 2) Restoring a backup of a course with user conditions to another site we need to check the shortname of the user profile field to see if it exists before restoring. Another tracker issue already exists for this. Sam, allowing fields to specify their own conditions sounds like a plugin system. This would be pretty difficult to program as it may require a more complex table structure to accommodate for all the possibilities.
          Hide
          Mark Nelson added a comment -

          In summary, I think we should integrate this tracker issue as it works as expected minus the (minor) issues I mentioned which can be addressed in other tracker issues.

          Show
          Mark Nelson added a comment - In summary, I think we should integrate this tracker issue as it works as expected minus the (minor) issues I mentioned which can be addressed in other tracker issues.
          Hide
          Ankit Agarwal added a comment - - edited

          Sam will be creating and linking the 3 bugs and 1 improvement issues, that we encountered while testing this issue.
          As far as I am concerned this can be considered as passed.

          Thanks

          Show
          Ankit Agarwal added a comment - - edited Sam will be creating and linking the 3 bugs and 1 improvement issues, that we encountered while testing this issue. As far as I am concerned this can be considered as passed. Thanks
          Hide
          Sam Hemelryk added a comment -

          Awesome thanks Ankit, creating the bugs now.

          Show
          Sam Hemelryk added a comment - Awesome thanks Ankit, creating the bugs now.
          Hide
          Sam Hemelryk added a comment -

          Ok marking this passed now, the following issues have been created.

          • MDL-34285: User profile field conditions missing a "is not empty" option.
          • MDL-34286: Caching issue in user profile field conditions.
          • MDL-34287: Default value issue with user profile field conditions.

          and the following improvement request.

          • MDL-34288: Allow user profile fields to specify the possible conditions.
          Show
          Sam Hemelryk added a comment - Ok marking this passed now, the following issues have been created. MDL-34285 : User profile field conditions missing a "is not empty" option. MDL-34286 : Caching issue in user profile field conditions. MDL-34287 : Default value issue with user profile field conditions. and the following improvement request. MDL-34288 : Allow user profile fields to specify the possible conditions.
          Hide
          Mark Nelson added a comment -

          Doh, I read Sam's comment "b) Add "is not empty" as an option." as And, not Add, so thought he was stating that it already existed and Ankit missed it. I really should have checked the code then to see if it existed before writing my earlier comment.

          Show
          Mark Nelson added a comment - Doh, I read Sam's comment "b) Add "is not empty" as an option." as And, not Add, so thought he was stating that it already existed and Ankit missed it. I really should have checked the code then to see if it existed before writing my earlier comment.
          Hide
          Ankit Agarwal added a comment -

          Just found another un-related issue. If there is a "checkbox" custom field, and you mark it as "required". Students can never leave it unchecked in that case.

          Show
          Ankit Agarwal added a comment - Just found another un-related issue. If there is a "checkbox" custom field, and you mark it as "required". Students can never leave it unchecked in that case.
          Hide
          Mark Nelson added a comment -

          Surely that is human error, not an issue in the code. If the user wants to create a checkbox that is required, and then specifies that a user can only access the activity if it is empty then they are free to do that.

          Show
          Mark Nelson added a comment - Surely that is human error, not an issue in the code. If the user wants to create a checkbox that is required, and then specifies that a user can only access the activity if it is empty then they are free to do that.
          Hide
          Mark Nelson added a comment -

          On the other hand, I doubt the administrator would want this situation to arise, so if it did it would almost always be human error, so sure we could add some validation for that issue (creating another tracker for this), but that also brings up the issue of someone creating user conditions dependent on a checkbox being empty, then changing the checkbox settings later to be required. Do we prevent them from changing it to required if it is being used as a user condition?

          Show
          Mark Nelson added a comment - On the other hand, I doubt the administrator would want this situation to arise, so if it did it would almost always be human error, so sure we could add some validation for that issue (creating another tracker for this), but that also brings up the issue of someone creating user conditions dependent on a checkbox being empty, then changing the checkbox settings later to be required. Do we prevent them from changing it to required if it is being used as a user condition?
          Hide
          Ankit Agarwal added a comment -

          Well in my opinion there should not be any way to mark a checkbox as required. Clearly this has nothing to do with the issue at hand, I just stumbled upon it, while testing the current issue. I created MDL-34289 to discuss this further and to decide if we want to change the behavior or not.

          Show
          Ankit Agarwal added a comment - Well in my opinion there should not be any way to mark a checkbox as required. Clearly this has nothing to do with the issue at hand, I just stumbled upon it, while testing the current issue. I created MDL-34289 to discuss this further and to decide if we want to change the behavior or not.
          Hide
          Mark Nelson added a comment -

          The administrator may want a required checkbox so that the user can verify a statement, such as 'I verify these profile details are true' when a user saves their profile.

          Show
          Mark Nelson added a comment - The administrator may want a required checkbox so that the user can verify a statement, such as 'I verify these profile details are true' when a user saves their profile.
          Hide
          Dan Poltawski added a comment -

          Congratulations!

          You've made it into the weekly release!

          Thanks for your contribution - here are some random drummers to keep you inspired for the next week!
          http://www.youtube.com/watch?v=_QhpHUmVCmY

          Show
          Dan Poltawski added a comment - Congratulations! You've made it into the weekly release! Thanks for your contribution - here are some random drummers to keep you inspired for the next week! http://www.youtube.com/watch?v=_QhpHUmVCmY
          Hide
          Mary Cooch added a comment -

          Removing docs_required label as this is now documented here http://docs.moodle.org/24/en/Conditional_activities_settings

          Show
          Mary Cooch added a comment - Removing docs_required label as this is now documented here http://docs.moodle.org/24/en/Conditional_activities_settings
          Hide
          Mark Nelson added a comment -

          Hi Mary, I wrote http://docs.moodle.org/dev/Conditional_user_fields for this section. I will link to it on the conditional activities settings doc.

          Show
          Mark Nelson added a comment - Hi Mary, I wrote http://docs.moodle.org/dev/Conditional_user_fields for this section. I will link to it on the conditional activities settings doc.
          Hide
          Helen Foster added a comment -
          Show
          Helen Foster added a comment - Thanks Mark for writing docs http://docs.moodle.org/24/en/Conditional_user_fields

            People

            • Votes:
              26 Vote for this issue
              Watchers:
              26 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: