Moodle
  1. Moodle
  2. MDL-24419

Allow Conditional Settings for Topic/Week

    Details

    • Rank:
      6338

      Description

      It would be nice to allow conditional settings not only for individual Activities/Resources, but for Topics/Weeks as well.
      This would be quite a time saver.
      Most users look for the some kind of automated release of one week worth of materials after another.
      Setting this for 15-20 individual activities/resources is time consuming. )
      Thank you!

      1. custom-release-criteria-for-topics-weeks-m205.patch
        69 kB
        Kirill Astashov
      2. custom-release-criteria-for-topics-weeks-m23dev-2011120500.02.patch
        62 kB
        Kirill Astashov
      3. custom-release-criteria-topics-weeks-m203.patch
        69 kB
        Kirill Astashov
      4. MDL-24419 test script.txt
        5 kB
        Sam Marshall

        Issue Links

          Activity

          Hide
          Sam Marshall added a comment -

          Yes, this would be nice!

          I don't have time to develop this at the moment, and it is probably too close to put in 2.0 - I think this is more a 2.1 feature and needs thought, possibly alongside other related features that should probably be added to topics/weeks - ie stuff you would want to add to 'edit week' page - such as insert and delete for example [shifting everything else along] which is something we have in the OU custom stuff.

          If you know anyone who's willing to develop this, that would be great. Otherwise I will check to see if the OU will sponsor it in which case we can develop it for a 2.x release.

          Show
          Sam Marshall added a comment - Yes, this would be nice! I don't have time to develop this at the moment, and it is probably too close to put in 2.0 - I think this is more a 2.1 feature and needs thought, possibly alongside other related features that should probably be added to topics/weeks - ie stuff you would want to add to 'edit week' page - such as insert and delete for example [shifting everything else along] which is something we have in the OU custom stuff. If you know anyone who's willing to develop this, that would be great. Otherwise I will check to see if the OU will sponsor it in which case we can develop it for a 2.x release.
          Hide
          Tamara Snyder added a comment -

          This is a feature our users have asked for as well. Sounds like it's a common feature in other LMSs.

          Show
          Tamara Snyder added a comment - This is a feature our users have asked for as well. Sounds like it's a common feature in other LMSs.
          Hide
          Sam Marshall added a comment -

          Yes it is a good feature and if you can get somebody to develop it (to Moodle code quality standards) that would be great. I will provide code review for any such contribution.

          Unfortunately, to update on my earlier comment, nobody here has requested the feature as a priority. That means I won't be able to develop it personally (for at least the rest of this year).

          Show
          Sam Marshall added a comment - Yes it is a good feature and if you can get somebody to develop it (to Moodle code quality standards) that would be great. I will provide code review for any such contribution. Unfortunately, to update on my earlier comment, nobody here has requested the feature as a priority. That means I won't be able to develop it personally (for at least the rest of this year).
          Hide
          Geoff Young added a comment -

          This would be a significant time saver if you had the option to either allow individual conditions or essentially 'group' conditions. This type of setting needs to be as flexible as possible and by having Topic/Week conditions it would allow for much better course by course delivery. 'Vote early and vote often!'

          Show
          Geoff Young added a comment - This would be a significant time saver if you had the option to either allow individual conditions or essentially 'group' conditions. This type of setting needs to be as flexible as possible and by having Topic/Week conditions it would allow for much better course by course delivery. 'Vote early and vote often!'
          Hide
          Ray Lawrence added a comment -

          Indeed lots of useful applications for this.

          Show
          Ray Lawrence added a comment - Indeed lots of useful applications for this.
          Hide
          Isabelle Langeveld added a comment -

          Same here, allthough it can be a bit user UNfriendly not to see all topics in advance, for some groups it can be very usefull to pace their learning path. It would also be great to send an automated message once a topice has been added.

          Show
          Isabelle Langeveld added a comment - Same here, allthough it can be a bit user UNfriendly not to see all topics in advance, for some groups it can be very usefull to pace their learning path. It would also be great to send an automated message once a topice has been added.
          Hide
          Shane added a comment -

          Bring it on.

          would be great to have rather than having to add a condition to every item in a topic. Massive time saver

          Show
          Shane added a comment - Bring it on. would be great to have rather than having to add a condition to every item in a topic. Massive time saver
          Hide
          Shane added a comment -

          need some options like these: http://imageshack.us/photo/my-images/715/moodletopicoptions.gif/

          (added a delete topic as that would be handy as well)

          Show
          Shane added a comment - need some options like these: http://imageshack.us/photo/my-images/715/moodletopicoptions.gif/ (added a delete topic as that would be handy as well)
          Hide
          Kirill Astashov added a comment -

          Please find attached custom release criteria for sections (topics/weeks). Hope it may be useful.

          Show
          Kirill Astashov added a comment - Please find attached custom release criteria for sections (topics/weeks). Hope it may be useful.
          Hide
          Glenn Sisson added a comment -

          This would be a very welcome addition. As stated by others, it is very time consuming to do it by activity.

          Please, please please add to core soon.

          Show
          Glenn Sisson added a comment - This would be a very welcome addition. As stated by others, it is very time consuming to do it by activity. Please, please please add to core soon.
          Hide
          Kirill Astashov added a comment - - edited

          Please find attached an improved version of the feature - custom-release-criteria-for-topics-weeks-m205.patch
          Previous version should work, but it won't apply gracefully because it also patches custom course navigation block, and also lib/navigationlib.php has been amended in core since 2.0.3.

          The updated patch should now apply to vanilla Moodle, tested on 2.0.4 and 2.0.5, it won't apply to 2.0.3 or earlier.
          Note that $CFG->enableavailability must be enabled for the feature to work.
          The patch has been also pushed to github.com/netspotau, branch MDL-24419_M20.

          Feedbacks are welcome.

          Show
          Kirill Astashov added a comment - - edited Please find attached an improved version of the feature - custom-release-criteria-for-topics-weeks-m205.patch Previous version should work, but it won't apply gracefully because it also patches custom course navigation block, and also lib/navigationlib.php has been amended in core since 2.0.3. The updated patch should now apply to vanilla Moodle, tested on 2.0.4 and 2.0.5, it won't apply to 2.0.3 or earlier. Note that $CFG->enableavailability must be enabled for the feature to work. The patch has been also pushed to github.com/netspotau, branch MDL-24419 _M20. Feedbacks are welcome.
          Hide
          Sam Marshall added a comment -

          Thanks. I'm not sure if you (Kirill) are trying to get it into core moodle yourself - if you aren't, but are just providing the patch for anyone who's interested, then that's fine - but I did a code review anyway as somebody might be interested in getting it into core!

          Note: there is SOME prospect that 'somebody' might be me, but not in the short term. I think we do have this as a locally requested feature but it's some way down the list.

          Review based on github view of commit https://github.com/netspotau/moodle/commit/991be828565d0219c01d6691d045a035c73861e5

          This looks pretty good but in order to be included in core moodle, it would need the following changes.

          Major changes (not in order):

          A1) Rebase against current master.

          A2) Move code from local plugin (eg install.xml needs to go into core lib/db/install.xml & also upgrade.php)

          A3) The table names are a bit horrible but also, in order to go into core moodle, the fields in the first table _dt should go into the main course_sections table (analagous to how it works in course_modules) and the _cg table could then be called course_sections_availability or whatever (by 'whatever' I mean, the same name as the equivalent table for course_modules).

          A4) The code in conditionlib_section is based on conditionlib. Instead, new functions should be integrated into conditionlib rather than creating a new file. Where the functions are exactly/almost exactly the same but with $cs instead of $cm, hopefully it should be possible to reduce duplication by changing the existing function into a private function that takes either a $cm or $cs parameter and making two public wrappers for the different uses. (Or something like that.)

          A5) There is some code duplicated in the two formats (and would need to be duplicated in other custom formats too). This should probably be replaced by a new ->uservisible parameter in course_sections when you get them from modinfo (oh, except I don't think sections work that way) or a new function or something - as you can tell, I haven't really thought this through and it might require more consideration.

          A6) I wasn't quite entirely sure if the new section data gets stored in course modinfo. If it does, that's great because it means this system doesn't add any database queries on course view. If it doesn't, that's bad. Also, if it does, I think there might be need for some extra code in course/lib.php to save space in modinfo by making sure not to store the values of those fields if they are default .

          And some minor notes:

          B1) If calling get_record where it is ok for it to return false, I prefer if it explicitly includes the IGNORE_MISSING strictness parameter. (Also btw, in editsection.php there is a brace in wrong place in this if.)

          B2) There are some cases of incorrect whitespace etc - Tim's codechecker plugin will find most of these (when submitting code for moodle core, it should not have any codechecker errors/warnings on the lines you change; obviously there may be errors on other lines in the same file that you haven't touched). Some the codechecker doesn't find at the moment, like there should be spaces around =>. NOTE: I am aware that many of these are wrong because the code was copy/pasted from code I originally wrote, which was wrong But we didn't have the detailed code style guidelines back then.

          B3) Local variable names aren't supposed to have underlines, e.g. $data_dt should be renamed something like $datetimedata or $datadatetime or, at a pinch, $datadt).

          B4) What's the reason for using insert_record_raw instead of insert_record? If this is actually needed there should be a comment to say why.

          B5) You've included changes to modedit, moodleform_mod which are not needed in this patch (regarding change to support times instead of just date, which is already in master) - kind of covered by the 'rebase' thing above but.

          B6) In conditionlib, "if (!defined('CONDITION_STUDENTVIEW_HIDE')) {..." is wrong - this script should only be included with require_once so it should never be defined so no condition is necessary. You've probably done this because of conditionlib_section above.

          Show
          Sam Marshall added a comment - Thanks. I'm not sure if you (Kirill) are trying to get it into core moodle yourself - if you aren't, but are just providing the patch for anyone who's interested, then that's fine - but I did a code review anyway as somebody might be interested in getting it into core! Note: there is SOME prospect that 'somebody' might be me, but not in the short term. I think we do have this as a locally requested feature but it's some way down the list. Review based on github view of commit https://github.com/netspotau/moodle/commit/991be828565d0219c01d6691d045a035c73861e5 This looks pretty good but in order to be included in core moodle, it would need the following changes. Major changes (not in order): A1) Rebase against current master. A2) Move code from local plugin (eg install.xml needs to go into core lib/db/install.xml & also upgrade.php) A3) The table names are a bit horrible but also, in order to go into core moodle, the fields in the first table _dt should go into the main course_sections table (analagous to how it works in course_modules) and the _cg table could then be called course_sections_availability or whatever (by 'whatever' I mean, the same name as the equivalent table for course_modules). A4) The code in conditionlib_section is based on conditionlib. Instead, new functions should be integrated into conditionlib rather than creating a new file. Where the functions are exactly/almost exactly the same but with $cs instead of $cm, hopefully it should be possible to reduce duplication by changing the existing function into a private function that takes either a $cm or $cs parameter and making two public wrappers for the different uses. (Or something like that.) A5) There is some code duplicated in the two formats (and would need to be duplicated in other custom formats too). This should probably be replaced by a new ->uservisible parameter in course_sections when you get them from modinfo (oh, except I don't think sections work that way) or a new function or something - as you can tell, I haven't really thought this through and it might require more consideration. A6) I wasn't quite entirely sure if the new section data gets stored in course modinfo. If it does, that's great because it means this system doesn't add any database queries on course view. If it doesn't, that's bad. Also, if it does, I think there might be need for some extra code in course/lib.php to save space in modinfo by making sure not to store the values of those fields if they are default . And some minor notes: B1) If calling get_record where it is ok for it to return false, I prefer if it explicitly includes the IGNORE_MISSING strictness parameter. (Also btw, in editsection.php there is a brace in wrong place in this if.) B2) There are some cases of incorrect whitespace etc - Tim's codechecker plugin will find most of these (when submitting code for moodle core, it should not have any codechecker errors/warnings on the lines you change; obviously there may be errors on other lines in the same file that you haven't touched). Some the codechecker doesn't find at the moment, like there should be spaces around =>. NOTE: I am aware that many of these are wrong because the code was copy/pasted from code I originally wrote, which was wrong But we didn't have the detailed code style guidelines back then. B3) Local variable names aren't supposed to have underlines, e.g. $data_dt should be renamed something like $datetimedata or $datadatetime or, at a pinch, $datadt). B4) What's the reason for using insert_record_raw instead of insert_record? If this is actually needed there should be a comment to say why. B5) You've included changes to modedit, moodleform_mod which are not needed in this patch (regarding change to support times instead of just date, which is already in master) - kind of covered by the 'rebase' thing above but. B6) In conditionlib, "if (!defined('CONDITION_STUDENTVIEW_HIDE')) {..." is wrong - this script should only be included with require_once so it should never be defined so no condition is necessary. You've probably done this because of conditionlib_section above.
          Hide
          Kirill Astashov added a comment -

          Sam, thanks a lot for the review, I'll consider these issues when developing the feature further.
          As you noticed, the code is based on conditionlib for resources/activities as I thought it was the quickest way to do it.

          The feature has been developed for University of NSW and is currently on production.
          I've not been asked to push the feature into core, this is quite a big task, you know, so at this stage I'm just sharing with the community.

          The university is keen to port the feature to 2.1 and 2.2 to keep it on their future upgrades, I'm likely to take care of it in the upcoming months.

          Show
          Kirill Astashov added a comment - Sam, thanks a lot for the review, I'll consider these issues when developing the feature further. As you noticed, the code is based on conditionlib for resources/activities as I thought it was the quickest way to do it. The feature has been developed for University of NSW and is currently on production. I've not been asked to push the feature into core, this is quite a big task, you know, so at this stage I'm just sharing with the community. The university is keen to port the feature to 2.1 and 2.2 to keep it on their future upgrades, I'm likely to take care of it in the upcoming months.
          Hide
          Kirill Astashov added a comment -

          Currently busy rebasing to Moodle 2.2 and implementing changes so that the feature could go into core.

          Show
          Kirill Astashov added a comment - Currently busy rebasing to Moodle 2.2 and implementing changes so that the feature could go into core.
          Hide
          Mark Drechsler added a comment -

          Hey Sam,
          Has this got a chance of making it into Core?
          Mark.

          Show
          Mark Drechsler added a comment - Hey Sam, Has this got a chance of making it into Core? Mark.
          Hide
          Sam Marshall added a comment -

          Yes it has got a chance!

          Once finished I can review it again - if you fixed the issues identified in my earlier code review (& any new ones I might spot based on the revised code!), I will submit it for integration into core. I think it should be accepted; I can't say this for certain as I don't make the final decisions but as maintainer for this area I do have some influence, and this is a pretty important feature...

          Some of those issues were a bit complicated i.e. I didn't fully know the right answer straight off, so I'm not sure if there might be a need for some iteration. Also, bear in mind the integration reviewer might find problems that I missed. I.e. this could still be some work but I think it should be possible to get it in.

          By the way, as this is a new feature I would expect this to probably go into 2.3 (master) branch only. (2.3 is currently almost identical to 2.2.)

          Show
          Sam Marshall added a comment - Yes it has got a chance! Once finished I can review it again - if you fixed the issues identified in my earlier code review (& any new ones I might spot based on the revised code!), I will submit it for integration into core. I think it should be accepted; I can't say this for certain as I don't make the final decisions but as maintainer for this area I do have some influence, and this is a pretty important feature... Some of those issues were a bit complicated i.e. I didn't fully know the right answer straight off, so I'm not sure if there might be a need for some iteration. Also, bear in mind the integration reviewer might find problems that I missed. I.e. this could still be some work but I think it should be possible to get it in. By the way, as this is a new feature I would expect this to probably go into 2.3 (master) branch only. (2.3 is currently almost identical to 2.2.)
          Hide
          Kirill Astashov added a comment -

          Hi Sam,

          Please take a look at the latest version of custom release criteria for topics/weeks patch. This has been rebased against current m23 master, most of your notices were considered and implemented. If you're interested in further discussion or comments please feel free to contact me.

          Show
          Kirill Astashov added a comment - Hi Sam, Please take a look at the latest version of custom release criteria for topics/weeks patch. This has been rebased against current m23 master, most of your notices were considered and implemented. If you're interested in further discussion or comments please feel free to contact me.
          Hide
          Sam Marshall added a comment -

          Thanks Kiril, looking good. Review points:

          1) I don't like use of new public form fields $mform->cs and ->showavailability; could this be included in the customdata variable that is passed to constructor instead please? (the array just above). Same as the way 'course' is handled. Doing it 2 different ways for one form doesn't make sense, and the customdata approach is the standard one.

          2) Trivial whitespace points from Moodle coding standard:
          a) Space around => please
          b) Space after comma.
          c) Space before the bracket on 'foreach' and 'if' please.
          d) When wrapping lines, please use eight spaces (there are some that use 4 or a random amount), except for associative arrays one per line, then it's still 4 (already correct).
          e) Space around = (assignment operator)
          f) Except for between classes, you should not have more than one blank line in a row (classes have two blank lines between)
          It will be easy to find most of these if you run the local codechecker plugin on files you changed (when editing existing files, obviously you should ignore any problems that are on lines you didn't change - the idea is to make sure new code is correct, old code is not your problem and changing it would make the patch harder to read).

          4) There are lines that set up $grouparray using =& for object variables - please change these just to use = as =& does not do anything for objects.

          5) Are you sure about this change?

          • $showsection = (has_capability('moodle/course:viewhiddensections', $context) or $thissection->visible or !$course->hiddensections);
            + $showsection = !$course->hiddensections && (has_capability('moodle/course:viewhiddensections', $context) || ($thissection->visible && ($thissection->is_available || $thissection->showavailability)));

          This one means that if $course->hiddensections (wtf is that anyway... nm) is false, even if you have viewhiddensections you will not see it, that's probably an unintentional difference...

          ALSO

          As this condition is complex and is shared between the two course formats, perhaps it would be worth making it shared - perhaps as a static function inside condition_info_section? Not sure, what do you think about that?

          Regarding the rest of it, I understand it's difficult to change the code for the two course formats to actually share this logic in a generic way because of all the branches and stuff. I don't really like this but as it is no worse than present, and would be difficult to fix, I decided it's OK. This is one point I can imagine HQ reviewer might not like, but let's see.

          6) echo '<div class="availabilityinfo">';

          Could you use html_writer for things like this please - it's better practice for new code (also, shockingly for html_writer, will actually make code slightly shorter b/c of the way it was laid out before as you can output the tag all at once instead of separate lines for open/content/close).

          7) New language strings 'completionconditionsection', 'gradeconditionsection' - is there any possibility these would need to be different? Seems to me these could be deleted and use the existing ones. Still needs different ones for help as you have done.

          8) Language string:
          +$string['groupingnoaccess'] = 'You have to become a member of the group which has access to this section.';

          A better way of writing this would probably be 'You do not currently belong to a group which has access to this section'. The current phrasing might be problematic because it suggests people can 'become' a member of that group - depending on how the teacher is running the course, it may or may not be possible to 'become' a member, so it might be misleading.

          9) This is my most serious concern - there is way, way too much duplication in conditionlib. There's no way this code could be accepted. We need to kill all the duplication. Luckily unless I'm missing something it should be fairly easy, here's how:

          a. Create a base abstract class condition_info_base which takes the following constructor param $tableprefix (stored in a member variable), and has a second protected member variable $item.

          b. Change existing condition_info to extends condition_info_base, calling the base constructor with parameter 'course_modules'. The constructor should be changed to use $this->item instead of $this->cm.

          c. Move most of the functions (listed below), out of condition_info and into condition_info_base. While moving, change them so that (a) the variable name is changed from $cmid to $itemid, (b) the phpdoc for this variable and all mentions of course-module is changed to 'item', (c) the insert/update/etc database instructions are changed to use $this->tableprefix . '_availability' (or whatever) instead of hardcoded to course_modules_availability:

          fill_availability_conditions
          add_completion_condition
          add_grade_condition
          wipe_conditions
          get_full_information
          is_available*
          show_availability
          require_data
          get_cached_grade_score
          wipe_session_cache
          update_cs_from_form
          completion_value_used_as_condition

          • I think is_available includes an extra grouping condition for sections which is not there for modules - you will need to override this function in your base class but I think you can call the parent function and then just do the groupings bit on top.

          I think this is pretty much all the functions except the constructor!

          e. Make the private functions protected.

          f. Reimplement your condition_info_section as another subclass of condition_info_base. I think it will only need a few functions, the constructor and the overridden is_available and get_full_course_section.

          That's it. So overall, looking great but that duplication is a big problem, I think that definitely needs fixing - sorry but we really can't have two copies of all that code from condition_info (possibly with subtle, hard-to-spot differences).

          Show
          Sam Marshall added a comment - Thanks Kiril, looking good. Review points: 1) I don't like use of new public form fields $mform->cs and ->showavailability; could this be included in the customdata variable that is passed to constructor instead please? (the array just above). Same as the way 'course' is handled. Doing it 2 different ways for one form doesn't make sense, and the customdata approach is the standard one. 2) Trivial whitespace points from Moodle coding standard: a) Space around => please b) Space after comma. c) Space before the bracket on 'foreach' and 'if' please. d) When wrapping lines, please use eight spaces (there are some that use 4 or a random amount), except for associative arrays one per line, then it's still 4 (already correct). e) Space around = (assignment operator) f) Except for between classes, you should not have more than one blank line in a row (classes have two blank lines between) It will be easy to find most of these if you run the local codechecker plugin on files you changed (when editing existing files, obviously you should ignore any problems that are on lines you didn't change - the idea is to make sure new code is correct, old code is not your problem and changing it would make the patch harder to read). 4) There are lines that set up $grouparray using =& for object variables - please change these just to use = as =& does not do anything for objects. 5) Are you sure about this change? $showsection = (has_capability('moodle/course:viewhiddensections', $context) or $thissection->visible or !$course->hiddensections); + $showsection = !$course->hiddensections && (has_capability('moodle/course:viewhiddensections', $context) || ($thissection->visible && ($thissection->is_available || $thissection->showavailability))); This one means that if $course->hiddensections (wtf is that anyway... nm) is false, even if you have viewhiddensections you will not see it, that's probably an unintentional difference... ALSO As this condition is complex and is shared between the two course formats, perhaps it would be worth making it shared - perhaps as a static function inside condition_info_section? Not sure, what do you think about that? Regarding the rest of it, I understand it's difficult to change the code for the two course formats to actually share this logic in a generic way because of all the branches and stuff. I don't really like this but as it is no worse than present, and would be difficult to fix, I decided it's OK. This is one point I can imagine HQ reviewer might not like, but let's see. 6) echo '<div class="availabilityinfo">'; Could you use html_writer for things like this please - it's better practice for new code (also, shockingly for html_writer, will actually make code slightly shorter b/c of the way it was laid out before as you can output the tag all at once instead of separate lines for open/content/close). 7) New language strings 'completionconditionsection', 'gradeconditionsection' - is there any possibility these would need to be different? Seems to me these could be deleted and use the existing ones. Still needs different ones for help as you have done. 8) Language string: +$string ['groupingnoaccess'] = 'You have to become a member of the group which has access to this section.'; A better way of writing this would probably be 'You do not currently belong to a group which has access to this section'. The current phrasing might be problematic because it suggests people can 'become' a member of that group - depending on how the teacher is running the course, it may or may not be possible to 'become' a member, so it might be misleading. 9) This is my most serious concern - there is way, way too much duplication in conditionlib. There's no way this code could be accepted. We need to kill all the duplication. Luckily unless I'm missing something it should be fairly easy, here's how: a. Create a base abstract class condition_info_base which takes the following constructor param $tableprefix (stored in a member variable), and has a second protected member variable $item. b. Change existing condition_info to extends condition_info_base, calling the base constructor with parameter 'course_modules'. The constructor should be changed to use $this->item instead of $this->cm. c. Move most of the functions (listed below), out of condition_info and into condition_info_base. While moving, change them so that (a) the variable name is changed from $cmid to $itemid, (b) the phpdoc for this variable and all mentions of course-module is changed to 'item', (c) the insert/update/etc database instructions are changed to use $this->tableprefix . '_availability' (or whatever) instead of hardcoded to course_modules_availability: fill_availability_conditions add_completion_condition add_grade_condition wipe_conditions get_full_information is_available* show_availability require_data get_cached_grade_score wipe_session_cache update_cs_from_form completion_value_used_as_condition I think is_available includes an extra grouping condition for sections which is not there for modules - you will need to override this function in your base class but I think you can call the parent function and then just do the groupings bit on top. I think this is pretty much all the functions except the constructor! e. Make the private functions protected. f. Reimplement your condition_info_section as another subclass of condition_info_base. I think it will only need a few functions, the constructor and the overridden is_available and get_full_course_section. That's it. So overall, looking great but that duplication is a big problem, I think that definitely needs fixing - sorry but we really can't have two copies of all that code from condition_info (possibly with subtle, hard-to-spot differences).
          Hide
          Kirill Astashov added a comment -

          Hi Sam, long time no see.

          I've redeveloped the feature as per your review and rebased it against current 2.3dev (master).
          Updated version of the feature is available at
          https://github.com/kastashov/moodle/commits/crctw/

          I tried to consider all your concerns mentioned in the review, some comments follow.

          > 7) New language strings 'completionconditionsection', 'gradeconditionsection' - is there
          > any possibility these would need to be different? Seems to me these could be deleted and
          > use the existing ones. Still needs different ones for help as you have done.

          Yes, these need to be different, for addHelpButton() function takes 'completionconditionsection'
          as argument and searches for "completionconditionsection_help" as help, this is all automatic.
          So as to reduce the number of strings to edit for translators, I've changed it to
          $string['completionconditionsection'] = $string['completioncondition'];
          Same for "gradecondition". Not sure if this dependancy is appropriate though.

          > 9) This is my most serious concern - there is way, way too much duplication in conditionlib.
          > There's no way this code could be accepted. We need to kill all the duplication. Luckily
          > unless I'm missing something it should be fairly easy, here's how:
          > a. Create a base abstract class condition_info_base which takes the following constructor
          > param $tableprefix (stored in a member variable), and has a second protected member variable $item.
          > ... etc

          As I had started killing the duplication before I noticed this review, I just incorporated it all
          in the existing class, a new property "objtype" defines whether an object should be treated as a module
          or a section. Default is module for backward compatibility.

          Please have a look, I hope we're pretty close.

          Show
          Kirill Astashov added a comment - Hi Sam, long time no see. I've redeveloped the feature as per your review and rebased it against current 2.3dev (master). Updated version of the feature is available at https://github.com/kastashov/moodle/commits/crctw/ I tried to consider all your concerns mentioned in the review, some comments follow. > 7) New language strings 'completionconditionsection', 'gradeconditionsection' - is there > any possibility these would need to be different? Seems to me these could be deleted and > use the existing ones. Still needs different ones for help as you have done. Yes, these need to be different, for addHelpButton() function takes 'completionconditionsection' as argument and searches for "completionconditionsection_help" as help, this is all automatic. So as to reduce the number of strings to edit for translators, I've changed it to $string ['completionconditionsection'] = $string ['completioncondition'] ; Same for "gradecondition". Not sure if this dependancy is appropriate though. > 9) This is my most serious concern - there is way, way too much duplication in conditionlib. > There's no way this code could be accepted. We need to kill all the duplication. Luckily > unless I'm missing something it should be fairly easy, here's how: > a. Create a base abstract class condition_info_base which takes the following constructor > param $tableprefix (stored in a member variable), and has a second protected member variable $item. > ... etc As I had started killing the duplication before I noticed this review, I just incorporated it all in the existing class, a new property "objtype" defines whether an object should be treated as a module or a section. Default is module for backward compatibility. Please have a look, I hope we're pretty close.
          Hide
          Sam Marshall added a comment -

          Thanks Kirill. I've put it in my calendar to review this again on Monday (assuming there isn't a crisis first).

          Re 7, sorry I think you are right and I was wrong, it is probably not possible to use a different lang string (which is kind of stupid). However, I don't think the code in lang.php option is a good one, it will confuse the translation editor. Instead there is an AMOS script (commit message) thing that can be done (to automatically generate new string for all the existing translations), while this leaves the duplication in place for any new translations I think this is the best option. You can leave it to me to sort this part out, code will just have the copy/paste duplicate.

          Re 9, I bet that after review I'm going to ask you to split it into separate classes, but with the variable you've already defined it should be pretty easy to do this...! Anyway we can wait until I do the review.

          Show
          Sam Marshall added a comment - Thanks Kirill. I've put it in my calendar to review this again on Monday (assuming there isn't a crisis first). Re 7, sorry I think you are right and I was wrong, it is probably not possible to use a different lang string (which is kind of stupid). However, I don't think the code in lang.php option is a good one, it will confuse the translation editor. Instead there is an AMOS script (commit message) thing that can be done (to automatically generate new string for all the existing translations), while this leaves the duplication in place for any new translations I think this is the best option. You can leave it to me to sort this part out, code will just have the copy/paste duplicate. Re 9, I bet that after review I'm going to ask you to split it into separate classes, but with the variable you've already defined it should be pretty easy to do this...! Anyway we can wait until I do the review.
          Hide
          Sam Marshall added a comment -

          New review. I do think we're getting closer but there are still quite a lot of things - sorry. To avoid confusion, I've started numbering from 10 so as not to reuse the same numbers, but this obviously supercedes the old review.

          10) You left in the public form fields $mform->cs and ->showavailability although I think you already changed the code to not use them

          11) As noted above, sorry but please continue to duplicate the lang strings instead of using '$string['gradeconditionsection'] = $string['gradecondition'];' logic.

          12) (Same as last time) Language string:
          +$string['groupingnoaccess'] = 'You have to become a member of the group which has access to this section.';

          A better way of writing this would probably be 'You do not currently belong to a group which has access to this section'. The current phrasing might be problematic because it suggests people can 'become' a member of that group - depending on how the teacher is running the course, it may or may not be possible to 'become' a member, so it might be misleading.

          13) You lost the <?php from the start of conditionlib.php?

          14) The reduced duplication in conditionlib makes this much much much better than previous change but I really think it ought to be turned into a set of three classes (condition_info_base, condition_info, and condition_info_section) as per my original comment 9, and remove the 'objtype' field. Basically, classes are supposed to define the type of the object, so having a variable called objtype is not a good sign.

          As far as I can see the code you've already written should make this a pretty easy change to carry out: any function that doesn't refer to the objtype variable goes into the base class, anything which only uses it for a table name prefix you can also do in base class (if you have a $tableprefix variable, see my original comment) while other functions might need to be abstract and implemented in the two classes. You could search replace $cmorcs with $item.

          15) Regarding the secinfo code. This is good but I think it needs a slightly different approach. It's a difficult area and I had a discussion with Petr (skodak) about it because I wasn't sure.

          Basically I see a few problems with the current code:

          a) It seems like lots of places that need to call rebuild_course_cache will also need to call rebuild_secinfo, including in existing code (custom modules etc).

          b) Unlike rebuild_course_cache there isn't a lazy/deferred rebuild option (where it just sets the value to null and lets it sort it out later).

          c) The modinfo code was moved into a nice separate class, OOP-style, in modinfolib.php. But this code is still in functions which seems like a step backward.

          d) The static caching has been removed from get_all_sections (agree this cache is not needed if using secinfo cache) but there is no indication for existing callers that this might be a bad function to use now.

          e) I'm not sure the availability value/uservisible for sections actually works correctly from a security perspective. (I.e. supposing your section is not available until 16 June - it is currently 1 June - the section contains a forum - a student guesses the URL to that forum, which is easy to do on Moodle - the student should be refused access, but does this happen or do they get in?)

          Basically my suggestion is to integrate this code with the existing modinfo cache. It is all 'really' the same thing, i.e. a course cache, and the function of preventing access is also related. I think after this changes it should also improve backward compatibility meaning that fewer changes are needed in unrelated files (like the mod/forum one at least). This would involve:

          i) Remove rebuild_course_secinfo, and make rebuild_course_cache do that work (but also including the existing rebuild_course_cache capability to just set the value to null if requested). So if you rebuild course cache, that includes the section cache, and works in the same way.

          ii) Remove get_all_sections_secinfo, and add the equivalent to course_modinfo class in modinfolib.php. That would include code in the constructor to make sure it is set and call rebuild_course_cache if necessary (actually that code basically exists, just need to include secinfo when testing the condition), and a new function course_modinfo::get_all_sections that basically just returns the value. Maybe also a course_modinfo::get_section_by_number convenience function that calls get_all_sections then returns the relevant part of the array.

          iii) Put get_all_sections back the way it was (for backward compatibility). This might mean duplicating the code rather than calling this function directly from inside rebuild_course_cache (since we don't want the static cache in that case). Maybe this function (get_all_sections) should be moved into deprecatedlib to warn people that there is now a more performant way to do it?

          iv) Modify the function cm_info::obtain_dynamic_data, inside the if statement about availability being enabled, so that it makes the check about section availability too.

          v) You can see that 'really' it would be good if we turned these section objects into a proper class and set certain fields for it as well (similar to cm_info class), but I suggest that we do NOT include that improvement in this change as it's already big enough. Anyway, putting the code in the right place makes it easier/clearer to do that later.

          16) The code at the start of editsection.php where it does complicated if statements and the rebuild_course_secinfo calls; seems to me this should just call (in your code) get_all_sections_secinfo which already contains this logic. Or in the new code, $modinfo->get_section_by_number($number).

          17) Your upgrade script sets the savepoint twice for the same number - probably the two blocks should just be combined into a single block.

          18) Trivia but there are a few points where double quotes are used for strings that should be single quotes.

          Show
          Sam Marshall added a comment - New review. I do think we're getting closer but there are still quite a lot of things - sorry. To avoid confusion, I've started numbering from 10 so as not to reuse the same numbers, but this obviously supercedes the old review. 10) You left in the public form fields $mform->cs and ->showavailability although I think you already changed the code to not use them 11) As noted above, sorry but please continue to duplicate the lang strings instead of using '$string ['gradeconditionsection'] = $string ['gradecondition'] ;' logic. 12) (Same as last time) Language string: +$string ['groupingnoaccess'] = 'You have to become a member of the group which has access to this section.'; A better way of writing this would probably be 'You do not currently belong to a group which has access to this section'. The current phrasing might be problematic because it suggests people can 'become' a member of that group - depending on how the teacher is running the course, it may or may not be possible to 'become' a member, so it might be misleading. 13) You lost the <?php from the start of conditionlib.php? 14) The reduced duplication in conditionlib makes this much much much better than previous change but I really think it ought to be turned into a set of three classes (condition_info_base, condition_info, and condition_info_section) as per my original comment 9, and remove the 'objtype' field. Basically, classes are supposed to define the type of the object, so having a variable called objtype is not a good sign. As far as I can see the code you've already written should make this a pretty easy change to carry out: any function that doesn't refer to the objtype variable goes into the base class, anything which only uses it for a table name prefix you can also do in base class (if you have a $tableprefix variable, see my original comment) while other functions might need to be abstract and implemented in the two classes. You could search replace $cmorcs with $item. 15) Regarding the secinfo code. This is good but I think it needs a slightly different approach. It's a difficult area and I had a discussion with Petr (skodak) about it because I wasn't sure. Basically I see a few problems with the current code: a) It seems like lots of places that need to call rebuild_course_cache will also need to call rebuild_secinfo, including in existing code (custom modules etc). b) Unlike rebuild_course_cache there isn't a lazy/deferred rebuild option (where it just sets the value to null and lets it sort it out later). c) The modinfo code was moved into a nice separate class, OOP-style, in modinfolib.php. But this code is still in functions which seems like a step backward. d) The static caching has been removed from get_all_sections (agree this cache is not needed if using secinfo cache) but there is no indication for existing callers that this might be a bad function to use now. e) I'm not sure the availability value/uservisible for sections actually works correctly from a security perspective. (I.e. supposing your section is not available until 16 June - it is currently 1 June - the section contains a forum - a student guesses the URL to that forum, which is easy to do on Moodle - the student should be refused access, but does this happen or do they get in?) Basically my suggestion is to integrate this code with the existing modinfo cache. It is all 'really' the same thing, i.e. a course cache, and the function of preventing access is also related. I think after this changes it should also improve backward compatibility meaning that fewer changes are needed in unrelated files (like the mod/forum one at least). This would involve: i) Remove rebuild_course_secinfo, and make rebuild_course_cache do that work (but also including the existing rebuild_course_cache capability to just set the value to null if requested). So if you rebuild course cache, that includes the section cache, and works in the same way. ii) Remove get_all_sections_secinfo, and add the equivalent to course_modinfo class in modinfolib.php. That would include code in the constructor to make sure it is set and call rebuild_course_cache if necessary (actually that code basically exists, just need to include secinfo when testing the condition), and a new function course_modinfo::get_all_sections that basically just returns the value. Maybe also a course_modinfo::get_section_by_number convenience function that calls get_all_sections then returns the relevant part of the array. iii) Put get_all_sections back the way it was (for backward compatibility). This might mean duplicating the code rather than calling this function directly from inside rebuild_course_cache (since we don't want the static cache in that case). Maybe this function (get_all_sections) should be moved into deprecatedlib to warn people that there is now a more performant way to do it? iv) Modify the function cm_info::obtain_dynamic_data, inside the if statement about availability being enabled, so that it makes the check about section availability too. v) You can see that 'really' it would be good if we turned these section objects into a proper class and set certain fields for it as well (similar to cm_info class), but I suggest that we do NOT include that improvement in this change as it's already big enough. Anyway, putting the code in the right place makes it easier/clearer to do that later. 16) The code at the start of editsection.php where it does complicated if statements and the rebuild_course_secinfo calls; seems to me this should just call (in your code) get_all_sections_secinfo which already contains this logic. Or in the new code, $modinfo->get_section_by_number($number). 17) Your upgrade script sets the savepoint twice for the same number - probably the two blocks should just be combined into a single block. 18) Trivia but there are a few points where double quotes are used for strings that should be single quotes.
          Hide
          Martin Dougiamas added a comment -

          Hi Sam!

          Thanks for all the detailed reviewing! Unfortunately I think Netspot may have run out of dev time for this.

          Are you able to fix those last things and submit it for integration? (Just asking because you are so familiar with it now and it would be easiest for you).

          Show
          Martin Dougiamas added a comment - Hi Sam! Thanks for all the detailed reviewing! Unfortunately I think Netspot may have run out of dev time for this. Are you able to fix those last things and submit it for integration? (Just asking because you are so familiar with it now and it would be easiest for you).
          Hide
          Mark Drechsler added a comment -

          Hey Sam,
          Kirill is going to run through the 'quick' changes and post a reply here, but I think some help on 14 & 15 would be good.
          Thanks,
          Mark.

          Show
          Mark Drechsler added a comment - Hey Sam, Kirill is going to run through the 'quick' changes and post a reply here, but I think some help on 14 & 15 would be good. Thanks, Mark.
          Hide
          Kirill Astashov added a comment -

          I have implemented fixes as per items 10...13 and 16...18 of the review,
          this has been pushed to https://github.com/kastashov/moodle/commits/crctw/

          Show
          Kirill Astashov added a comment - I have implemented fixes as per items 10...13 and 16...18 of the review, this has been pushed to https://github.com/kastashov/moodle/commits/crctw/
          Hide
          Sam Marshall added a comment -

          Thanks for the fixes.

          Martin/Mark: Yes I can do remaining changes and am happy to, except just like Netspot, I don't have any time budgeted for it... so far as I'm aware we don't have an internal need for this at present.

          I will raise this with my project manager to see if I can do this here. It's possible that people have requested this feature here but it just hasn't got to the top of any lists yet, in which case surely they can let me have a few days to get it submitted.

          Show
          Sam Marshall added a comment - Thanks for the fixes. Martin/Mark: Yes I can do remaining changes and am happy to, except just like Netspot, I don't have any time budgeted for it... so far as I'm aware we don't have an internal need for this at present. I will raise this with my project manager to see if I can do this here. It's possible that people have requested this feature here but it just hasn't got to the top of any lists yet, in which case surely they can let me have a few days to get it submitted.
          Hide
          Kirill Astashov added a comment -

          Sam,

          While testing the feature, a small but nasty bug was found and fixed.
          I've pushed the fix to https://github.com/kastashov/moodle/commits/crctw

          Regards,
          Kirill

          Show
          Kirill Astashov added a comment - Sam, While testing the feature, a small but nasty bug was found and fixed. I've pushed the fix to https://github.com/kastashov/moodle/commits/crctw Regards, Kirill
          Hide
          Sam Marshall added a comment -

          Thanks. By the way, I haven't heard back from my manager yet about whether I can spend a couple days finishing coding this - will try again when I see them next.

          Show
          Sam Marshall added a comment - Thanks. By the way, I haven't heard back from my manager yet about whether I can spend a couple days finishing coding this - will try again when I see them next.
          Hide
          Kirill Astashov added a comment -

          Sam,

          This is now being tested by client, so... another commit.
          From now on in case a fix should be applied, I'd just push it without commenting on it here unless you advise that you've pulled and started working on the feature.

          Regards,
          Kirill

          Show
          Kirill Astashov added a comment - Sam, This is now being tested by client, so... another commit. From now on in case a fix should be applied, I'd just push it without commenting on it here unless you advise that you've pulled and started working on the feature. Regards, Kirill
          Hide
          Sam Marshall added a comment -

          Thanks Kiril. Good news - I now have approval to do this and hopefully in time for Moodle 2.3 - I will comment when I start working on it.

          As part of that I have had an internal request to make it so you can restrict sections by grouping as well (haven't checked but I think the current implementation only does the stuff that's in the usual 'availability' area of the form) - I think that should probably be easy to add so I'll do it.

          Show
          Sam Marshall added a comment - Thanks Kiril. Good news - I now have approval to do this and hopefully in time for Moodle 2.3 - I will comment when I start working on it. As part of that I have had an internal request to make it so you can restrict sections by grouping as well (haven't checked but I think the current implementation only does the stuff that's in the usual 'availability' area of the form) - I think that should probably be easy to add so I'll do it.
          Hide
          Ray Lawrence added a comment -

          Sam/Krill, I've been watching this with interest as it will be hugely useful for lots of our clients.

          I was delighted to see the additional request regarding restricting sections to groupings and then I remembered.... MDL-28225

          Would this depend on the Experimental setting being enabled and, if so, would it open the can of worms described by Petr. Or would it resolve them?

          Show
          Ray Lawrence added a comment - Sam/Krill, I've been watching this with interest as it will be hugely useful for lots of our clients. I was delighted to see the additional request regarding restricting sections to groupings and then I remembered.... MDL-28225 Would this depend on the Experimental setting being enabled and, if so, would it open the can of worms described by Petr. Or would it resolve them?
          Hide
          Sam Marshall added a comment -

          Ray: Well that question is a bit complicated. I don't really agree there is a problem in the first place and my preference is that the experimental setting should be removed (as if it is always-on). HOWEVER that's not directly related to this issue and I don't want to get into it.

          So in other words - yes I will make it so that the grouping option only appears if you have enabled the experimental setting.

          Show
          Sam Marshall added a comment - Ray: Well that question is a bit complicated. I don't really agree there is a problem in the first place and my preference is that the experimental setting should be removed (as if it is always-on). HOWEVER that's not directly related to this issue and I don't want to get into it. So in other words - yes I will make it so that the grouping option only appears if you have enabled the experimental setting.
          Hide
          Ray Lawrence added a comment -

          Hi Sam,

          Thanks for that clarification.

          Cheers

          Show
          Ray Lawrence added a comment - Hi Sam, Thanks for that clarification. Cheers
          Hide
          Kirill Astashov added a comment -

          Sam,

          I confirm that you already can restrict section by grouping in the current implementation of the feature.

          Regards,
          Kirill

          Show
          Kirill Astashov added a comment - Sam, I confirm that you already can restrict section by grouping in the current implementation of the feature. Regards, Kirill
          Hide
          Bryan Woerner added a comment -

          For those of us who do not speak code, will any of the above patches allow us to set up a topic so it becomes active on a specified date? This would be great because in case I am not around to physically unblink each topic and it would save a lot of time.

          Show
          Bryan Woerner added a comment - For those of us who do not speak code, will any of the above patches allow us to set up a topic so it becomes active on a specified date? This would be great because in case I am not around to physically unblink each topic and it would save a lot of time.
          Hide
          Sam Marshall added a comment -

          Kiril - Awesome. Thanks.
          Bryan - Yes this will provide that feature.

          Show
          Sam Marshall added a comment - Kiril - Awesome. Thanks. Bryan - Yes this will provide that feature.
          Hide
          Ray Lawrence added a comment -

          Hi,

          I was just reviewing a client site and a potential option to improve this immensely leapt out at me. It would be great if the revealing of the sections could be set to reveal from a specified duration from the commencement of the course rather than dates being set individually. This would reduce administration effort required for courses that are re-used.

          Whilst typing it also occurs that if if the conditional settings for sections could be managed for groups and individuals in the same way as in the quiz module that would fantastic (bout probably too much to ask for?).

          Show
          Ray Lawrence added a comment - Hi, I was just reviewing a client site and a potential option to improve this immensely leapt out at me. It would be great if the revealing of the sections could be set to reveal from a specified duration from the commencement of the course rather than dates being set individually. This would reduce administration effort required for courses that are re-used. Whilst typing it also occurs that if if the conditional settings for sections could be managed for groups and individuals in the same way as in the quiz module that would fantastic (bout probably too much to ask for?).
          Hide
          Sam Marshall added a comment -

          Ray:

          Regarding your first point, I'll check it has been included in this, but in general all of Moodle already supports that first feature, albeit in a slightly different manner. When backup/restoring a Moodle course, if you set a different start date it will automatically adjust all the other dates. So if you reuse a course, just back it up and restore it with the new date (then rename it and the old one if you want it to have the same name).

          Moodle has chosen this approach (set as 'absolute' date, adjust on restore) for all dates in the system, rather than defining dates to be based on the course start date, so it would be a bad idea to implement any one part differently.

          Regarding your second part, yes that would be too much to ask for This feature is specifically about providing the same facilities for sections that are already available to control the conditional availability of individual activities (which don't have per-user modifications).

          Show
          Sam Marshall added a comment - Ray: Regarding your first point, I'll check it has been included in this, but in general all of Moodle already supports that first feature, albeit in a slightly different manner. When backup/restoring a Moodle course, if you set a different start date it will automatically adjust all the other dates. So if you reuse a course, just back it up and restore it with the new date (then rename it and the old one if you want it to have the same name). Moodle has chosen this approach (set as 'absolute' date, adjust on restore) for all dates in the system, rather than defining dates to be based on the course start date, so it would be a bad idea to implement any one part differently. Regarding your second part, yes that would be too much to ask for This feature is specifically about providing the same facilities for sections that are already available to control the conditional availability of individual activities (which don't have per-user modifications).
          Hide
          Sam Marshall added a comment -

          I'm actually starting work on this, woohoo. Will pull from Kiril's repo as of now.

          Show
          Sam Marshall added a comment - I'm actually starting work on this, woohoo. Will pull from Kiril's repo as of now.
          Hide
          Ray Lawrence added a comment -

          1st point above: Yup, if this will work in that way too that would be useful (although last time I looked at this using an account with course creator rights the date re-alignment option wasn't available, I can't check this atm as restore for CCs is broken again).

          2nd point: I guessed that.

          Show
          Ray Lawrence added a comment - 1st point above: Yup, if this will work in that way too that would be useful (although last time I looked at this using an account with course creator rights the date re-alignment option wasn't available, I can't check this atm as restore for CCs is broken again). 2nd point: I guessed that.
          Hide
          Sam Marshall added a comment -

          Haven't changed anything yet (got called away to fix/triage other problems), just merged Kirill's branch against current master and resolved the conflicts. This is now on my GitHub (diff url link just added to bug).

          Show
          Sam Marshall added a comment - Haven't changed anything yet (got called away to fix/triage other problems), just merged Kirill's branch against current master and resolved the conflicts. This is now on my GitHub (diff url link just added to bug).
          Hide
          Kirill Astashov added a comment - - edited

          Hi Sam, that's good news!
          From now on, if we catch a bug, I'll let you know here in comments.
          Good luck.

          P.S. Now I really think that rebuild_course_secinfo should be called from rebuild_course_cache (from somewhere close to end of rebuild_course_cache code), for they are pretty much always called one after the other, and then all those diffs like

          rebuild_course_cache($course->id);
          + rebuild_course_secinfo($course->id);

          could be just reverted.

          Show
          Kirill Astashov added a comment - - edited Hi Sam, that's good news! From now on, if we catch a bug, I'll let you know here in comments. Good luck. P.S. Now I really think that rebuild_course_secinfo should be called from rebuild_course_cache (from somewhere close to end of rebuild_course_cache code), for they are pretty much always called one after the other, and then all those diffs like rebuild_course_cache($course->id); + rebuild_course_secinfo($course->id); could be just reverted.
          Hide
          Sam Marshall added a comment -

          I managed to grab a few hours to work on this today.

          Update on progress. So far I have:

          • changed it to use classes instead of the object variable (my review #14)
          • fixed a bug where it didn't work properly for grade completion

          (these are both in repo now)

          Still to do:

          • rework secinfo code (my review #15)
          • check it integrates with modinfo to prevent access for students who guess url to activities (also from #15)
          • finish and document testing procedure
          • final checking/tidying
          Show
          Sam Marshall added a comment - I managed to grab a few hours to work on this today. Update on progress. So far I have: changed it to use classes instead of the object variable (my review #14) fixed a bug where it didn't work properly for grade completion (these are both in repo now) Still to do: rework secinfo code (my review #15) check it integrates with modinfo to prevent access for students who guess url to activities (also from #15) finish and document testing procedure final checking/tidying
          Hide
          Sam Marshall added a comment -

          Next commit: I reworked secinfo to move all the necessary code into modinfolib. Because this defines a class for sections, it can now produce better information for IDEs, and better in-code documentation. I also added code similar to the existing code for modinfo, so that it does not store fields in the cache if they have default values; this reduced the cache field size by a factor of 4 (from about 4KB to 1KB) in my test course.

          In addition to the other remaining 'to do' list items above, I think it would probably be beneficial (it would save one db query, and some memory) to switch all/most existing core uses of get_all_sections function to use the new cache, and move get_all_sections to deprecatedlib.

          Show
          Sam Marshall added a comment - Next commit: I reworked secinfo to move all the necessary code into modinfolib. Because this defines a class for sections, it can now produce better information for IDEs, and better in-code documentation. I also added code similar to the existing code for modinfo, so that it does not store fields in the cache if they have default values; this reduced the cache field size by a factor of 4 (from about 4KB to 1KB) in my test course. In addition to the other remaining 'to do' list items above, I think it would probably be beneficial (it would save one db query, and some memory) to switch all/most existing core uses of get_all_sections function to use the new cache, and move get_all_sections to deprecatedlib.
          Hide
          Sam Marshall added a comment -

          Now integrated with modinfo so that it actually works to prevent access to activities within unavailable sections. I made the API the same as it is for course-modules, using the same variable names (available, availableinfo, and uservisible).

          While testing this, I noticed that there is a problem related to the navigation cache. Because the navigation system caches information it probably shouldn't (I think in the user session), when there are changes to session availability, this does not show up in navigation. For example, say you have a session set to depend on a certain activity being completed; if you complete that activity, you'll have to wait ten minutes before the section shows up in the navigation menu, even though it will show up immediately in the course page. This is MDL-31631. I'll add a 'related' marker.

          Show
          Sam Marshall added a comment - Now integrated with modinfo so that it actually works to prevent access to activities within unavailable sections. I made the API the same as it is for course-modules, using the same variable names (available, availableinfo, and uservisible). While testing this, I noticed that there is a problem related to the navigation cache. Because the navigation system caches information it probably shouldn't (I think in the user session), when there are changes to session availability, this does not show up in navigation. For example, say you have a session set to depend on a certain activity being completed; if you complete that activity, you'll have to wait ten minutes before the section shows up in the navigation menu, even though it will show up immediately in the course page. This is MDL-31631 . I'll add a 'related' marker.
          Hide
          Sam Marshall added a comment -

          Latest commit ('tidying'):

          • Tidied whitespace, wrapping, etc.
          • Removed impossible code branches.
          • Removed duplicated code in editsection and refactored update_cm_from_form so it could be shared.
          • Made it so grouping option only appears if you've enabled groupmembersonly (consistent with activity behaviour)
          • Removed double-checks in formats for section already existing due to cache being wrong (this shouldn't happen - in case it does, I filed MDL-32215 so it can be detected).
          • Removed course/modedit change about using a variable instead of just passing a temp array, and course/moodleform_mod.php whitespace correction, because there doesn't seem really to be a need to touch either file.
          • There was a possible serious performance problem with changes to completion_value_used_as_condition, which I believe may be called multiple times per page, but the new code makes a database query. I have changed it to use entirely cached data without any queries.
          • There was no index on the course_sections_availability table in install.xml (but there was one in the upgrade). I changed it to have the same set of keys/indexes as course_modules_availability. Also generally updated install.xml changes to match current 2.3 format (this was changed slightly).

          Using performance info on a test course, logged in as student, with multiple requests to ensure all data is cached, and switching between master and this branch, the number of DB queries required to display a course that uses section completion goes from 48 (master) to 47 (this change), so overall (because of the section caching) this change should slightly improve performance.

          Still to do: test script.

          Show
          Sam Marshall added a comment - Latest commit ('tidying'): Tidied whitespace, wrapping, etc. Removed impossible code branches. Removed duplicated code in editsection and refactored update_cm_from_form so it could be shared. Made it so grouping option only appears if you've enabled groupmembersonly (consistent with activity behaviour) Removed double-checks in formats for section already existing due to cache being wrong (this shouldn't happen - in case it does, I filed MDL-32215 so it can be detected). Removed course/modedit change about using a variable instead of just passing a temp array, and course/moodleform_mod.php whitespace correction, because there doesn't seem really to be a need to touch either file. There was a possible serious performance problem with changes to completion_value_used_as_condition, which I believe may be called multiple times per page, but the new code makes a database query. I have changed it to use entirely cached data without any queries. There was no index on the course_sections_availability table in install.xml (but there was one in the upgrade). I changed it to have the same set of keys/indexes as course_modules_availability. Also generally updated install.xml changes to match current 2.3 format (this was changed slightly). Using performance info on a test course, logged in as student, with multiple requests to ensure all data is cached, and switching between master and this branch, the number of DB queries required to display a course that uses section completion goes from 48 (master) to 47 (this change), so overall (because of the section caching) this change should slightly improve performance. Still to do: test script.
          Hide
          Sam Marshall added a comment -

          Attached backup file for testing. Test script to follow.

          Show
          Sam Marshall added a comment - Attached backup file for testing. Test script to follow.
          Hide
          Sam Marshall added a comment -

          More commits:

          • There was no backup/restore support! I have written this.
          • Entries in the new table were not deleted when the course is deleted; fixed.
          Show
          Sam Marshall added a comment - More commits: There was no backup/restore support! I have written this. Entries in the new table were not deleted when the course is deleted; fixed.
          Hide
          Sam Marshall added a comment -

          Attaching test script (it's long, so neater here than in the test script section of the bug, probably).

          Show
          Sam Marshall added a comment - Attaching test script (it's long, so neater here than in the test script section of the bug, probably).
          Hide
          Sam Marshall added a comment -

          I have moved the old branch to here for reference:

          https://github.com/sammarshallou/moodle/compare/master...MDL-24419-original

          And created a new branch by rebasing so that it is a small number of logical commits. Actually, I couldn't really split stuff out that much, so basically there are just two minor core changes that I had added, as separate commits, then the rest as one big wodge.

          The new branch is substantially unchanged from the end of the old branch, but I've rebased it on current master.

          Basically the old branch might be useful if anyone following development (e.g. Kirill) wants to check the individual commits of what I changed.

          Show
          Sam Marshall added a comment - I have moved the old branch to here for reference: https://github.com/sammarshallou/moodle/compare/master...MDL-24419-original And created a new branch by rebasing so that it is a small number of logical commits. Actually, I couldn't really split stuff out that much, so basically there are just two minor core changes that I had added, as separate commits, then the rest as one big wodge. The new branch is substantially unchanged from the end of the old branch, but I've rebased it on current master. Basically the old branch might be useful if anyone following development (e.g. Kirill) wants to check the individual commits of what I changed.
          Hide
          Sam Marshall added a comment -

          This is now 'finished' as far as I'm concerned, so requesting peer review please. Yes it is a large change, sorry.

          Note 1: Regarding 'secinfo' cache - this was discussed with Petr beforehand, although he has not seen the implementation so not saying it's certain he likes it.

          Note 2: Regarding performance - as commented above, comparing this against master on a repeated (second) student view of the test course page which uses the feature results in 47 db queries after this change vs. 48 queries after it, so a one-query performance improvement.

          Note 3: Once this is implemented, a potential further enhancement would be to remove the 'visibleold' logic about hidden sections (where if you hide a section it also hides all the activities in it, storing the old state in 'visibleold'). My new code will make it easy to automatically 'hide' activities (including preventing access) if sections are hidden. I didn't include the change in this commit because it's big enough already, but I think removing that rather ugly aspect of the system will be a nice future cleanup.

          Show
          Sam Marshall added a comment - This is now 'finished' as far as I'm concerned, so requesting peer review please. Yes it is a large change, sorry. Note 1: Regarding 'secinfo' cache - this was discussed with Petr beforehand, although he has not seen the implementation so not saying it's certain he likes it. Note 2: Regarding performance - as commented above, comparing this against master on a repeated (second) student view of the test course page which uses the feature results in 47 db queries after this change vs. 48 queries after it, so a one-query performance improvement. Note 3: Once this is implemented, a potential further enhancement would be to remove the 'visibleold' logic about hidden sections (where if you hide a section it also hides all the activities in it, storing the old state in 'visibleold'). My new code will make it easy to automatically 'hide' activities (including preventing access) if sections are hidden. I didn't include the change in this commit because it's big enough already, but I think removing that rather ugly aspect of the system will be a nice future cleanup.
          Hide
          Sam Marshall added a comment -

          Martin suggests that the field 'secinfo' should be renamed 'sectioncache' (i.e. go for a meaningful name similar to the name of the course_sections table, rather than making it analogous to the legacy name modinfo). I'll make that change along with other changes based on review.

          Show
          Sam Marshall added a comment - Martin suggests that the field 'secinfo' should be renamed 'sectioncache' (i.e. go for a meaningful name similar to the name of the course_sections table, rather than making it analogous to the legacy name modinfo). I'll make that change along with other changes based on review.
          Hide
          Itamar Tzadok added a comment -

          I've looked at the code and if I understand it correctly, the gist of the matter is in conditionlib. I think that until the concept of the course section entity is revisited in an orderly fashion, this enhancement could be done with much less code changes by using a designated module to provide the section not only conditions but also a context and further settings. The designated module instance can be automatically created for a section when the section is created or better when the course designer wants to set availability conditions (in which case DB resources are allocated only where needed and not always as with the current solution; that is, if you have 50 sections and you want to control access in only 4 of them you would need only 4 instances of the designated module with their allocated resources). The designated module is minimal, that is, nothing but name and common module settings so while it is registered in the section sequence it does not show up on display. In conditionlib it would stand for the section and provide the condition info. The availability of context could allow attaching contextual things to sections such as blocks. Just my 2 cents.

          Show
          Itamar Tzadok added a comment - I've looked at the code and if I understand it correctly, the gist of the matter is in conditionlib. I think that until the concept of the course section entity is revisited in an orderly fashion, this enhancement could be done with much less code changes by using a designated module to provide the section not only conditions but also a context and further settings. The designated module instance can be automatically created for a section when the section is created or better when the course designer wants to set availability conditions (in which case DB resources are allocated only where needed and not always as with the current solution; that is, if you have 50 sections and you want to control access in only 4 of them you would need only 4 instances of the designated module with their allocated resources). The designated module is minimal, that is, nothing but name and common module settings so while it is registered in the section sequence it does not show up on display. In conditionlib it would stand for the section and provide the condition info. The availability of context could allow attaching contextual things to sections such as blocks. Just my 2 cents.
          Hide
          Dan Poltawski added a comment -

          Hi Sam,

          First look comments in random order:

          1. I'm a bit concerned about the 'caches' (groups and CONDITIONLIB_PRIVATE) being touched here and their interaction with the mythical MUC. (just a comment we should note/put it in list of things to convert to MUC)
          2. I agree with martin that I prefer the explicit sectioncache name
          3. There is trailing whitespace in condition_info_section::is_available() near the SQL (I think that SQL should be indented inline with the rest of the code FWIW)
          4. Strict Standards: Non-static method course_modinfo::build_secinfo() should not be called statically in /Users/danp/git/moodle/lib/modinfolib.php on line 1244
          5. When I first installed cleanly and went into a course (with conditional settings disabled) it broke all the sections with
             Notice: Undefined property: stdClass::$uservisible in /Users/danp/git/moodle/course/format/weeks/format.php on line 156 Notice: Undefined property: stdClass::$available in /Users/danp/git/moodle/course/format/weeks/format.php on line 157
            

            I chose topics format and it started working and then couldn't reproduce..

          6. Why is build_secinfo indexing by section number rather than ID? Could that be less robust if sections are reordered?
          7. It needs rebasing against master
          Show
          Dan Poltawski added a comment - Hi Sam, First look comments in random order: I'm a bit concerned about the 'caches' (groups and CONDITIONLIB_PRIVATE) being touched here and their interaction with the mythical MUC. (just a comment we should note/put it in list of things to convert to MUC) I agree with martin that I prefer the explicit sectioncache name There is trailing whitespace in condition_info_section::is_available() near the SQL (I think that SQL should be indented inline with the rest of the code FWIW) Strict Standards: Non-static method course_modinfo::build_secinfo() should not be called statically in /Users/danp/git/moodle/lib/modinfolib.php on line 1244 When I first installed cleanly and went into a course (with conditional settings disabled) it broke all the sections with Notice: Undefined property: stdClass::$uservisible in /Users/danp/git/moodle/course/format/weeks/format.php on line 156 Notice: Undefined property: stdClass::$available in /Users/danp/git/moodle/course/format/weeks/format.php on line 157 I chose topics format and it started working and then couldn't reproduce.. Why is build_secinfo indexing by section number rather than ID? Could that be less robust if sections are reordered? It needs rebasing against master
          Hide
          Sam Marshall added a comment -

          1. Agree these caches should be considered for the unified caching architecture but that is a future API; for now, having the caches is helpful and neither cache should run out of memory or anything.

          2. Yes, I agreed with Martin too Renamed appropriately in field and function names.

          3. Sorry about that, I do check but probably edited after.
          3/fwiw. Also fixed - personally I think my SQL formatting is the most readable of any in Moodle (not the actual queries, just the way I format them) so I've kept the same structure but just indented it.

          4. Ooops! Added missing static keyword.

          5. There are two related problem in this area:

          i. The error you saw occurs if you have a section that doesn't exist in database.
          ii. When that happens it can add multiple sections...

          You could reproduce by creating a new course with 1 section. That might give the error already; if it doesn't, edit it and change setting so it has 2 sections. Reload the page a bunch, it might add duplicate sections to database. Anyway I fixed it so this will work now.

          I fixed both problems. Also see MDL-32215 which I'm going to look at implementing soon (independently of this change) that should make sure this doesn't happen.

          6. I see your point here but some of the existing section code was already based around section numbers, not ids - in particular, I designed this stuff to replace get_all_sections which stores the array in that format (indexed by section number). It should be equally 'robust' assuming there are never duplicates (which there shouldn't be, and in future definitely won't be if I get MDL-32215 done).

          7. Of course, sorry, done. (There was a conflict in navigationlib due to a separate change I was already aware of, now fixed).

          Show
          Sam Marshall added a comment - 1. Agree these caches should be considered for the unified caching architecture but that is a future API; for now, having the caches is helpful and neither cache should run out of memory or anything. 2. Yes, I agreed with Martin too Renamed appropriately in field and function names. 3. Sorry about that, I do check but probably edited after. 3/fwiw. Also fixed - personally I think my SQL formatting is the most readable of any in Moodle (not the actual queries, just the way I format them) so I've kept the same structure but just indented it. 4. Ooops! Added missing static keyword. 5. There are two related problem in this area: i. The error you saw occurs if you have a section that doesn't exist in database. ii. When that happens it can add multiple sections... You could reproduce by creating a new course with 1 section. That might give the error already; if it doesn't, edit it and change setting so it has 2 sections. Reload the page a bunch, it might add duplicate sections to database. Anyway I fixed it so this will work now. I fixed both problems. Also see MDL-32215 which I'm going to look at implementing soon (independently of this change) that should make sure this doesn't happen. 6. I see your point here but some of the existing section code was already based around section numbers, not ids - in particular, I designed this stuff to replace get_all_sections which stores the array in that format (indexed by section number). It should be equally 'robust' assuming there are never duplicates (which there shouldn't be, and in future definitely won't be if I get MDL-32215 done). 7. Of course, sorry, done. (There was a conflict in navigationlib due to a separate change I was already aware of, now fixed).
          Hide
          Sam Marshall added a comment -

          Review comments addressed, hopefully - re-requesting in case you want to look at it further or disagree with any of the rationale. (For instance I'm sure I can change it to make the array index by id if you definitely think that's better.)

          Show
          Sam Marshall added a comment - Review comments addressed, hopefully - re-requesting in case you want to look at it further or disagree with any of the rationale. (For instance I'm sure I can change it to make the array index by id if you definitely think that's better.)
          Hide
          Sam Marshall added a comment -

          Two more notes: first, I removed the bit with a 'TODO' that said it was only needed until navigation stops cacheing section data, because Sam did that already now; and second, I forgot to say, thank you for the review!!

          Show
          Sam Marshall added a comment - Two more notes: first, I removed the bit with a 'TODO' that said it was only needed until navigation stops cacheing section data, because Sam did that already now; and second, I forgot to say, thank you for the review!!
          Hide
          Dan Poltawski added a comment -

          Hi Sam,

          This looks good for integration to me now.

          However - it'd be really great if you could expand the unit tests we currently have in lib/tests/conditionlib_test.php

          thanks,
          Dan

          Show
          Dan Poltawski added a comment - Hi Sam, This looks good for integration to me now. However - it'd be really great if you could expand the unit tests we currently have in lib/tests/conditionlib_test.php thanks, Dan
          Hide
          Sam Marshall added a comment -

          Great - thanks.

          I will look into adding unit tests for the section features hopefully tomorrow (unfortunately I am under a lot of time pressure because of various problems with OU systems so it might not really be tomorrow but at least this week, I hope). After that will submit for integration.

          Show
          Sam Marshall added a comment - Great - thanks. I will look into adding unit tests for the section features hopefully tomorrow (unfortunately I am under a lot of time pressure because of various problems with OU systems so it might not really be tomorrow but at least this week, I hope). After that will submit for integration.
          Hide
          Sam Marshall added a comment -

          Now submitting for integration. Includes unit tests - I copied the existing unit tests for activities, modified them to work, and added extra tests for the grouping support which is done differently. (And fixed the bug I found along the way!)

          Show
          Sam Marshall added a comment - Now submitting for integration. Includes unit tests - I copied the existing unit tests for activities, modified them to work, and added extra tests for the grouping support which is done differently. (And fixed the bug I found along the way!)
          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
          Sam Marshall added a comment -

          Rebased to master (no conflicts except version number). I also ran phpunit tests for entire project. There are still failures but none are related to this code.

          Show
          Sam Marshall added a comment - Rebased to master (no conflicts except version number). I also ran phpunit tests for entire project. There are still failures but none are related to this code.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Getting on this, not sure if it will be reviewed this week or next one, but getting on it.

          BTW, is there any Docs page available to know a bit more about the goals/plan of this? Any reading would help I guess.

          Show
          Eloy Lafuente (stronk7) added a comment - Getting on this, not sure if it will be reviewed this week or next one, but getting on it. BTW, is there any Docs page available to know a bit more about the goals/plan of this? Any reading would help I guess.
          Hide
          Petr Škoda added a comment - - edited

          Hi, btw there should be no need to store previous USER and CFG in tests, it resets automatically if you do "$this->resetAfterTest()", but of course you can still do it if you really want to. Also do not assign to $USER directly, use $this->setUser($user_or_id); oh, please do not reinvent $this->getDataGenerator()->create_user().

          Show
          Petr Škoda added a comment - - edited Hi, btw there should be no need to store previous USER and CFG in tests, it resets automatically if you do "$this->resetAfterTest()", but of course you can still do it if you really want to. Also do not assign to $USER directly, use $this->setUser($user_or_id); oh, please do not reinvent $this->getDataGenerator()->create_user().
          Hide
          Petr Škoda added a comment -

          1/ hmm, every new caching such as $GROUPLIB_CACHE should be added to the phpunit test reset code. New globals MUST be also defined in global scope - in case of core it is lib/setup.php, plugins should not add any globals.

          2/ Each new format_string() should have context parameter too.

          3/ we need to lower memory consumption by removing unnecessary includes, this from lib/navigationlib.php is going exactly the opposite way:
          +require_once($CFG->libdir.'/conditionlib.php');
          +require_once($CFG->libdir.'/completionlib.php');

          4/ PARAM_FLOAT should not be used in forms, you need to deal with floating points/commas there

          5/ why <FIELD NAME="groupingid" TYPE="int" LENGTH="?11?"

          Show
          Petr Škoda added a comment - 1/ hmm, every new caching such as $GROUPLIB_CACHE should be added to the phpunit test reset code. New globals MUST be also defined in global scope - in case of core it is lib/setup.php, plugins should not add any globals. 2/ Each new format_string() should have context parameter too. 3/ we need to lower memory consumption by removing unnecessary includes, this from lib/navigationlib.php is going exactly the opposite way: +require_once($CFG->libdir.'/conditionlib.php'); +require_once($CFG->libdir.'/completionlib.php'); 4/ PARAM_FLOAT should not be used in forms, you need to deal with floating points/commas there 5/ why <FIELD NAME="groupingid" TYPE="int" LENGTH="?11?"
          Hide
          Eloy Lafuente (stronk7) added a comment -

          (failing based on Petr's review, thanks!)

          Show
          Eloy Lafuente (stronk7) added a comment - (failing based on Petr's review, thanks!)
          Hide
          Sam Marshall added a comment -

          thanks Petr, will look at this asap.

          regarding storing previous $USER and $CFG in tests - before adding the code there, I actually tested this with print_object and found that the values I set seemed to stay around - was maybe using older version of test infrastructure though. Will remove.

          Will do 1, 2, 3 (don't understand why the requires are there anyway), 5; I have a query about 4, will ask in chat.

          Show
          Sam Marshall added a comment - thanks Petr, will look at this asap. regarding storing previous $USER and $CFG in tests - before adding the code there, I actually tested this with print_object and found that the values I set seemed to stay around - was maybe using older version of test infrastructure though. Will remove. Will do 1, 2, 3 (don't understand why the requires are there anyway), 5; I have a query about 4, will ask in chat.
          Hide
          Sam Marshall added a comment -

          Resubmitting for integration after fixing all reported issues (and one other).

          0. Fixed unit tests:

          • removed unnecessary code to clear global changes
          • changed to use setUser
          • removed custom make_user function and switched to use generator one

          1. Added GROUPLIB_CACHE to phpunit reset and added it to lib/setup.php globals list.

          2. There were 2 new format_string calls; changed both to supply context.

          3. Removed the two unnecessary includes.

          4. Replaced existing code, including existing use in the old per-activity grade condition form, with format_float and unformat_float calls (same function used in gradebook).

          As part of doing this I had to add a new parameter to format_float so that it can format numbers nicely (i.e. limit to 5 decimals but show fewer than that if possible) in order to preserve the existing behaviour of that activity form; it is necessary for usability because of limited space in form fields and the fact that you probably nearly always use whole numbers anyhow. I added a new unit test to format_float to verify the new and existing behaviour. This part is done as a separate commit in the branch.

          5. Presumably they used 11 digits by accident, I changed it to 10.

          Additionally, I found and fixed a PHP strict warning with the grouplib cache stuff.

          After doing this I reran both the unit tests and the manual test script to verify that it still works!

          Show
          Sam Marshall added a comment - Resubmitting for integration after fixing all reported issues (and one other). 0. Fixed unit tests: removed unnecessary code to clear global changes changed to use setUser removed custom make_user function and switched to use generator one 1. Added GROUPLIB_CACHE to phpunit reset and added it to lib/setup.php globals list. 2. There were 2 new format_string calls; changed both to supply context. 3. Removed the two unnecessary includes. 4. Replaced existing code, including existing use in the old per-activity grade condition form, with format_float and unformat_float calls (same function used in gradebook). As part of doing this I had to add a new parameter to format_float so that it can format numbers nicely (i.e. limit to 5 decimals but show fewer than that if possible) in order to preserve the existing behaviour of that activity form; it is necessary for usability because of limited space in form fields and the fact that you probably nearly always use whole numbers anyhow. I added a new unit test to format_float to verify the new and existing behaviour. This part is done as a separate commit in the branch. 5. Presumably they used 11 digits by accident, I changed it to 10. Additionally, I found and fixed a PHP strict warning with the grouplib cache stuff. After doing this I reran both the unit tests and the manual test script to verify that it still works!
          Hide
          Kirill Astashov added a comment -

          > Will do 1, 2, 3 (don't understand why the requires are there anyway)

          As far as I remember, requires are there because navigation block should also apply section release criteria so as not to display unavailable sections.

          Show
          Kirill Astashov added a comment - > Will do 1, 2, 3 (don't understand why the requires are there anyway) As far as I remember, requires are there because navigation block should also apply section release criteria so as not to display unavailable sections.
          Hide
          Sam Marshall added a comment -

          Thanks Kirill. I think that explains it - previously it used to directly call into conditionlib, etc. from within the navigation, but we changed it so that modinfolib provides information about section visibility, which means it no longer needs to directly call into those conditionlib functions.

          So when I changed it, I failed to remove the includes even though they are now unnecessary in that file.

          Show
          Sam Marshall added a comment - Thanks Kirill. I think that explains it - previously it used to directly call into conditionlib, etc. from within the navigation, but we changed it so that modinfolib provides information about section visibility, which means it no longer needs to directly call into those conditionlib functions. So when I changed it, I failed to remove the includes even though they are now unnecessary in that file.
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Sam Marshall added a comment -

          Rebased and fixed minor conflict

          Show
          Sam Marshall added a comment - Rebased and fixed minor conflict
          Hide
          Martin Dougiamas added a comment -

          Noting possible conflict with MDL-32508

          Show
          Martin Dougiamas added a comment - Noting possible conflict with MDL-32508
          Hide
          Dan Poltawski added a comment -

          Going to try and rebase this on top of my change in MDL-32508 to resolve that conflict.

          Show
          Dan Poltawski added a comment - Going to try and rebase this on top of my change in MDL-32508 to resolve that conflict.
          Hide
          Dan Poltawski added a comment -

          Sam, my changes have been integrated now unfortunately!

          I did do this rebase, although got interrupted, so only half sorted out the course format changes - I put the work I did at:
          https://github.com/danpoltawski/moodle/compare/9fdbd69e22f4c7806f950e32e04b14289dd2a6aa...MDL-24419-master

          Like I didn't properly port across the the renderer.php changes so will need some work..

          Show
          Dan Poltawski added a comment - Sam, my changes have been integrated now unfortunately! I did do this rebase, although got interrupted, so only half sorted out the course format changes - I put the work I did at: https://github.com/danpoltawski/moodle/compare/9fdbd69e22f4c7806f950e32e04b14289dd2a6aa...MDL-24419-master Like I didn't properly port across the the renderer.php changes so will need some work..
          Hide
          Sam Marshall added a comment -

          This was totally submitted first. ;p

          How do I get the integration branch to rebase against? (From your URL given, I can't see what you did or what needs doing so I think I will need to start again from scratch...)

          Although I may be able to change the code I will probably not have time to retest this.

          Show
          Sam Marshall added a comment - This was totally submitted first. ;p How do I get the integration branch to rebase against? (From your URL given, I can't see what you did or what needs doing so I think I will need to start again from scratch...) Although I may be able to change the code I will probably not have time to retest this.
          Hide
          Dan Poltawski added a comment -

          Sorry, its git://github.com/danpoltawski/moodle.git MDL-24419-master

          And integration is git://git.moodle.org/integration.git, but by the time you get this it'll probably be in moodle.git

          Show
          Dan Poltawski added a comment - Sorry, its git://github.com/danpoltawski/moodle.git MDL-24419 -master And integration is git://git.moodle.org/integration.git, but by the time you get this it'll probably be in moodle.git
          Hide
          Sam Marshall added a comment -

          Thanks. Currently I hope to rebase this on Monday, time permitting.

          Show
          Sam Marshall added a comment - Thanks. Currently I hope to rebase this on Monday, time permitting.
          Hide
          Sam Marshall added a comment -

          This feature is now rebased. I did a brief test, it appears to still work roughly the same as (and/or better than) it did before.

          Show
          Sam Marshall added a comment - This feature is now rebased. I did a brief test, it appears to still work roughly the same as (and/or better than) it did before.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this has been delayed again. Personal apologizes, I ran out of time.

          I'll try to make it my very-first issue to integrate next cycle.

          Thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this has been delayed again. Personal apologizes, I ran out of time. I'll try to make it my very-first issue to integrate next cycle. Thanks and ciao
          Hide
          Sam Marshall added a comment -

          Rebased again (minor conflict in course/format/renderer.php, not a problem).

          Show
          Sam Marshall added a comment - Rebased again (minor conflict in course/format/renderer.php, not a problem).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Both fixed on merge commit:

          • not using the xmldb editor. (install.xml)
          • course->sectioncache added twice. (upgrade.php)
          Show
          Eloy Lafuente (stronk7) added a comment - Both fixed on merge commit: not using the xmldb editor. (install.xml) course->sectioncache added twice. (upgrade.php)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Note for Docs: this change requires Moodle to be upgraded from /admin/index.php or CLI, because the site becomes inaccessible until upgrade is performed.

          Show
          Eloy Lafuente (stronk7) added a comment - Note for Docs: this change requires Moodle to be upgraded from /admin/index.php or CLI, because the site becomes inaccessible until upgrade is performed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi, here there are some modifications I've done on top of Sam's commits:
          (forget the NOBUG: Minibump is one unrelated leftover)

          https://github.com/stronk7/moodle/compare/integration...MDL-24419

          It includes:

          • The merge itself where some fixes to install and upgrade have been applied.
          • Code style cleanup.
          • Backup and restore fixes and improvements (more work needed).

          Apart from that:

          • Code looks ok.
          • Unit tests are passing.
          • Brief functional testing seems to work (UI, grouping...)

          So, we need to decide if we land this now, in order to be able to be tested and continue with detected problems later (new MDL issue(s)).

          Integrators votes required. Here it's my one: +1.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi, here there are some modifications I've done on top of Sam's commits: (forget the NOBUG: Minibump is one unrelated leftover) https://github.com/stronk7/moodle/compare/integration...MDL-24419 It includes: The merge itself where some fixes to install and upgrade have been applied. Code style cleanup. Backup and restore fixes and improvements (more work needed). Apart from that: Code looks ok. Unit tests are passing. Brief functional testing seems to work (UI, grouping...) So, we need to decide if we land this now, in order to be able to be tested and continue with detected problems later (new MDL issue(s)). Integrators votes required. Here it's my one: +1. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sam, regarding the pending work in restore (that will be handled in a different issue if this gets integrated), take a look to the TODOs I've introduced in my commit above.

          We need to know which records are existing ones and which have been introduced by the restore. And also we need to detect invalid duplicates.

          All that makes me thing if really the introduction of the after_restore() in steps is a good idea. They are only available at task level because only modules not being auto-contained should use them (bad citizens). And also, they are supported by restore_plugins, because some of them require access to the whole thing (plagiarism...). But bringing support to steps... could lead to abuse, so my proposal would be about to:

          1) drop support for restore_steps->after_restore()
          2) implement the section_availability restore as a new step, much like modules availability is handled, in 2 steps:

          a) reading section_availability information simply store it in backupids table
          b) later one new step "restore_process_course_sections_availability" executed by the restore_final_task.

          Just my thoughts, let's see if this land...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Sam, regarding the pending work in restore (that will be handled in a different issue if this gets integrated), take a look to the TODOs I've introduced in my commit above. We need to know which records are existing ones and which have been introduced by the restore. And also we need to detect invalid duplicates. All that makes me thing if really the introduction of the after_restore() in steps is a good idea. They are only available at task level because only modules not being auto-contained should use them (bad citizens). And also, they are supported by restore_plugins, because some of them require access to the whole thing (plagiarism...). But bringing support to steps... could lead to abuse, so my proposal would be about to: 1) drop support for restore_steps->after_restore() 2) implement the section_availability restore as a new step, much like modules availability is handled, in 2 steps: a) reading section_availability information simply store it in backupids table b) later one new step "restore_process_course_sections_availability" executed by the restore_final_task. Just my thoughts, let's see if this land...ciao
          Hide
          Sam Hemelryk added a comment -

          +1 to land this and continue testing/finding/fixing issues as they rise. Obviously getting restore working should be a priority (I think anyway)

          Show
          Sam Hemelryk added a comment - +1 to land this and continue testing/finding/fixing issues as they rise. Obviously getting restore working should be a priority (I think anyway)
          Hide
          Dan Poltawski added a comment -

          +1 here too

          Show
          Dan Poltawski added a comment - +1 here too
          Hide
          Aparup Banerjee added a comment -

          +1 to get this in (front of lots of testers)

          Show
          Aparup Banerjee added a comment - +1 to get this in (front of lots of testers)
          Hide
          Dan Poltawski added a comment - - edited

          OK, as we're all in agreement (and as discussed) i've pushed this now so we can get testers on it!

          Show
          Dan Poltawski added a comment - - edited OK, as we're all in agreement (and as discussed) i've pushed this now so we can get testers on it!
          Hide
          Dan Poltawski added a comment -

          I think we can use redirect_if_major_upgrade_required to fix the problem with needing to go to /admin/index.php

          I've created a patch for this (also incrementing the version number away from a microincrement because i have an irrational aversion from hardcoding that in file..)

          https://github.com/danpoltawski/moodle/commit/b24d1dbbad07cea80e36441215dcb5290ab1db2f

          Show
          Dan Poltawski added a comment - I think we can use redirect_if_major_upgrade_required to fix the problem with needing to go to /admin/index.php I've created a patch for this (also incrementing the version number away from a microincrement because i have an irrational aversion from hardcoding that in file..) https://github.com/danpoltawski/moodle/commit/b24d1dbbad07cea80e36441215dcb5290ab1db2f
          Hide
          Dan Poltawski added a comment -

          I've done a new install, created new course and getting:

          Notice: Undefined property: stdClass::$uservisible in /Users/danp/git/integration/course/format/renderer.php on line 553 Call Stack: 0.0001 637544 1.

          {main}() /Users/danp/git/integration/course/view.php:0 0.6618 17264464 2. require('/Users/danp/git/integration/course/format/weeks/format.php') /Users/danp/git/integration/course/view.php:239 0.6622 17291960 3. format_section_renderer_base->print_multiple_section_page() /Users/danp/git/integration/course/format/weeks/format.php:45 Notice: Undefined property: stdClass::$available in /Users/danp/git/integration/course/format/renderer.php on line 554 Call Stack: 0.0001 637544 1. {main}

          () /Users/danp/git/integration/course/view.php:0 0.6618 17264464 2.

          At guess, I think the section cache has cached a newly created section badly.

          Show
          Dan Poltawski added a comment - I've done a new install, created new course and getting: Notice: Undefined property: stdClass::$uservisible in /Users/danp/git/integration/course/format/renderer.php on line 553 Call Stack: 0.0001 637544 1. {main}() /Users/danp/git/integration/course/view.php:0 0.6618 17264464 2. require('/Users/danp/git/integration/course/format/weeks/format.php') /Users/danp/git/integration/course/view.php:239 0.6622 17291960 3. format_section_renderer_base->print_multiple_section_page() /Users/danp/git/integration/course/format/weeks/format.php:45 Notice: Undefined property: stdClass::$available in /Users/danp/git/integration/course/format/renderer.php on line 554 Call Stack: 0.0001 637544 1. {main} () /Users/danp/git/integration/course/view.php:0 0.6618 17264464 2. At guess, I think the section cache has cached a newly created section badly.
          Hide
          Dan Poltawski added a comment -

          Yes, in format_section_renderer_base::print_multiple_section_page()
          print_object($sections); was not retrieving any sections, so they were retrievied from:

          $thissection = get_course_section($section, $course->id);

          This function creates a section if it doesn't exist (the way the formats do this is horrible btw) without that data.

          Now I tried switching course format in course settings and the problem went away - I have a feeling this action will have triggered a rebuild of the cache.

          Show
          Dan Poltawski added a comment - Yes, in format_section_renderer_base::print_multiple_section_page() print_object($sections); was not retrieving any sections, so they were retrievied from: $thissection = get_course_section($section, $course->id); This function creates a section if it doesn't exist (the way the formats do this is horrible btw) without that data. Now I tried switching course format in course settings and the problem went away - I have a feeling this action will have triggered a rebuild of the cache.
          Hide
          Ankit Agarwal added a comment -

          I am getting these notices when trying to restore a course:-
          Notice: Undefined property: stdClass::$idnumber in /var/www/int/master/moodle/backup/moodle2/restore_stepslib.php on line 723 Notice: Undefined property: stdClass::$idnumber in /var/www/int/master/moodle/backup/moodle2/restore_stepslib.php on line 783

          Also I cannot see the availability information associated with sections as mentioned in the testing instructions (1.4 and 1.5)

          Waiting for these to be fixed.
          Thanks

          Show
          Ankit Agarwal added a comment - I am getting these notices when trying to restore a course:- Notice: Undefined property: stdClass::$idnumber in /var/www/int/master/moodle/backup/moodle2/restore_stepslib.php on line 723 Notice: Undefined property: stdClass::$idnumber in /var/www/int/master/moodle/backup/moodle2/restore_stepslib.php on line 783 Also I cannot see the availability information associated with sections as mentioned in the testing instructions (1.4 and 1.5) Waiting for these to be fixed. Thanks
          Hide
          Dan Poltawski added a comment -

          So to report my findings, to reproduce:
          1/ Create new course with 10 sections
          2/ Visit course
          3/ Boom!

          (and continues to boom until you do something that rebuilds course cache).

          We need to review anywhre where inserts a record to course_sections I guess to ensure the course cache is rebuilt? Furthermore I think we shouldn't leave it up to formats to create the course_section records. That was an issue I noticed when doing the course format rework.

          To stop the continual booming (only once) I added:

          rebuild_course_cache($courseid);
          

          to get_coruse_section

          Show
          Dan Poltawski added a comment - So to report my findings, to reproduce: 1/ Create new course with 10 sections 2/ Visit course 3/ Boom! (and continues to boom until you do something that rebuilds course cache). We need to review anywhre where inserts a record to course_sections I guess to ensure the course cache is rebuilt? Furthermore I think we shouldn't leave it up to formats to create the course_section records. That was an issue I noticed when doing the course format rework. To stop the continual booming (only once) I added: rebuild_course_cache($courseid); to get_coruse_section
          Hide
          Dan Poltawski added a comment -

          Note the $idnumber issue reported above is from MDL-32005

          Show
          Dan Poltawski added a comment - Note the $idnumber issue reported above is from MDL-32005
          Hide
          Sam Marshall added a comment -

          Thanks to all for your work on this.

          Eloy: I agree with your proposals re how to make the restore work correctly when restoring partial course that already has some availability data. I don't offhand know how to do those though!

          Dan: Ugh. Yes my intention was that anywhere which inserts/modifies course sections should call rebuild_course_cache. I thought I did a search for all places that do that and fixed them, but that may have been before the course formats rewrite (or else I just screwed up).

          Please let me know if you need me to act on any of this, I'm not sure of process at this point.

          Show
          Sam Marshall added a comment - Thanks to all for your work on this. Eloy: I agree with your proposals re how to make the restore work correctly when restoring partial course that already has some availability data. I don't offhand know how to do those though! Dan: Ugh. Yes my intention was that anywhere which inserts/modifies course sections should call rebuild_course_cache. I thought I did a search for all places that do that and fixed them, but that may have been before the course formats rewrite (or else I just screwed up). Please let me know if you need me to act on any of this, I'm not sure of process at this point.
          Hide
          Sam Marshall added a comment -

          Regarding the 'create course' problem, I have done a new commit for this which Dan is going to add on. (While rebasing this code to work with the rewritten course formats, one change got lost and I failed to spot it.)

          Show
          Sam Marshall added a comment - Regarding the 'create course' problem, I have done a new commit for this which Dan is going to add on. (While rebasing this code to work with the rewritten course formats, one change got lost and I failed to spot it.)
          Hide
          Dan Poltawski added a comment -

          I've pulled in Sam's fix for new course and also added the major_version_upgrade_required bump

          Show
          Dan Poltawski added a comment - I've pulled in Sam's fix for new course and also added the major_version_upgrade_required bump
          Hide
          Ankit Agarwal added a comment - - edited

          I still cannot see the availability information associated with sections as mentioned in the testing instructions (1.4 and 1.5).
          I carried on with the testing instructions ignoring the above issues and all rest things seem to work perfectly.
          Thanks

          Show
          Ankit Agarwal added a comment - - edited I still cannot see the availability information associated with sections as mentioned in the testing instructions (1.4 and 1.5). I carried on with the testing instructions ignoring the above issues and all rest things seem to work perfectly. Thanks
          Hide
          Ankit Agarwal added a comment -

          Sorry guys,
          Failing this as advised by Dan.

          Show
          Ankit Agarwal added a comment - Sorry guys, Failing this as advised by Dan.
          Hide
          Dan Poltawski added a comment -

          OK, I looked at what the old code was doing before sam had to radically rebase this, and have pushed a patch which I think restores the intended behaviour.

          The problem was that in the original code Sam was showing the availability info to people it was visible to if it was set even if the section was uservisible.

          So I chatted with Ankit and have patched this

          Sam, it'd be great if you could review this:
          http://git.moodle.org/gw?p=integration.git;a=blobdiff;f=course/format/renderer.php;h=45977d452a43725e89e67839fd97a6eca3742ae2;hp=63f3bdc199cbe6099b82cec160056cc097984a1a;hb=5316007e080327f1547fe017d0af588e16c4aee9;hpb=1edff8c7ed08339e0e42e9fab64aa909de627e71

          Ankit, it'd be great if you could try the tests again.

          Show
          Dan Poltawski added a comment - OK, I looked at what the old code was doing before sam had to radically rebase this, and have pushed a patch which I think restores the intended behaviour. The problem was that in the original code Sam was showing the availability info to people it was visible to if it was set even if the section was uservisible. So I chatted with Ankit and have patched this Sam, it'd be great if you could review this: http://git.moodle.org/gw?p=integration.git;a=blobdiff;f=course/format/renderer.php;h=45977d452a43725e89e67839fd97a6eca3742ae2;hp=63f3bdc199cbe6099b82cec160056cc097984a1a;hb=5316007e080327f1547fe017d0af588e16c4aee9;hpb=1edff8c7ed08339e0e42e9fab64aa909de627e71 Ankit, it'd be great if you could try the tests again.
          Hide
          Ankit Agarwal added a comment -

          Finally working as expected!
          I feel like I have already memorized the testing instructions

          Passing.
          Thanks

          Show
          Ankit Agarwal added a comment - Finally working as expected! I feel like I have already memorized the testing instructions Passing. Thanks
          Hide
          Sam Marshall added a comment -

          Dan, thanks for doing that patch - I am not quite 100% certain that is correct behaviour (it's difficult to tell just by thinking about the code) but I think it is at least 'good enough'. How about we go ahead with this and once it is integrated, I'll do another check of how it behaves and if I think there's anything wrong I'll file another issue regarding this minor problem?

          (also thanks for testing ankit, sorry it is so complicated

          Show
          Sam Marshall added a comment - Dan, thanks for doing that patch - I am not quite 100% certain that is correct behaviour (it's difficult to tell just by thinking about the code) but I think it is at least 'good enough'. How about we go ahead with this and once it is integrated, I'll do another check of how it behaves and if I think there's anything wrong I'll file another issue regarding this minor problem? (also thanks for testing ankit, sorry it is so complicated
          Hide
          Eloy Lafuente (stronk7) added a comment -

          U P S T R E A M I Z E D !

          Many thanks for the hard work, closing this as fixed.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - U P S T R E A M I Z E D ! Many thanks for the hard work, closing this as fixed. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Note to self/anyofus: Create new issue about the changes in backup & restore pending to get this information properly handled:

          http://tracker.moodle.org/browse/MDL-24419?focusedCommentId=157931&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-157931

          Show
          Eloy Lafuente (stronk7) added a comment - Note to self/anyofus: Create new issue about the changes in backup & restore pending to get this information properly handled: http://tracker.moodle.org/browse/MDL-24419?focusedCommentId=157931&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-157931
          Hide
          Tim Barker added a comment -

          Ankit has added the QA tests for this already so removed teh QA tests requd flag.

          Show
          Tim Barker added a comment - Ankit has added the QA tests for this already so removed teh QA tests requd flag.
          Hide
          Sam Marshall added a comment -

          Awesome Thanks all.

          Eloy - I have created issue MDL-33133 about the backup/restore issue. Am working on test case at least, may also try to fix it.

          Show
          Sam Marshall added a comment - Awesome Thanks all. Eloy - I have created issue MDL-33133 about the backup/restore issue. Am working on test case at least, may also try to fix it.
          Show
          Mary Cooch added a comment - This wonderful new feature is now documented - http://docs.moodle.org/23/en/Conditional_activities_settings and also http://docs.moodle.org/23/en/Conditional_activities_FAQ#Can_I_set_conditional_activities_for_a_whole_section.2C_not_just_individual_activities.3F (so removing label)
          Hide
          Tamara Snyder added a comment -

          Thanks very much for adding this greatly-desired feature. We are in the process of completing an upgrade to Moodle 2.2 and won't be able to upgrade to 2.3 for a while. Do you know if we can patch 2.2 with these changes?

          Show
          Tamara Snyder added a comment - Thanks very much for adding this greatly-desired feature. We are in the process of completing an upgrade to Moodle 2.2 and won't be able to upgrade to 2.3 for a while. Do you know if we can patch 2.2 with these changes?
          Hide
          Kirill Astashov added a comment -

          Tamara, I doubt that.

          Show
          Kirill Astashov added a comment - Tamara, I doubt that.
          Hide
          Steve Bailey added a comment -

          Does this allow you to restrict availability to a particular grouping, i.e. available to group members only? If not, does this new functionality make it easier to develop this feature?

          Show
          Steve Bailey added a comment - Does this allow you to restrict availability to a particular grouping, i.e. available to group members only? If not, does this new functionality make it easier to develop this feature?
          Hide
          Kirill Astashov added a comment -

          Steve, this feature was finally integrated and is in core as of Moodle 2.3.
          In regards to your question - yes, it allows section restriction for a designated grouping only.

          Show
          Kirill Astashov added a comment - Steve, this feature was finally integrated and is in core as of Moodle 2.3. In regards to your question - yes, it allows section restriction for a designated grouping only.
          Hide
          Ray Lawrence added a comment -

          @Krill

          I just looked at 2.4 and I can't see this option i.e. restrict section by grouping. Can you explain how it is enabled?

          Show
          Ray Lawrence added a comment - @Krill I just looked at 2.4 and I can't see this option i.e. restrict section by grouping. Can you explain how it is enabled?
          Hide
          Sam Marshall added a comment -

          Ray: There is a setting, which I think may be in the experimental section, called 'enable group members only' or something like that. You need to have this turned on in order to control access to anything by grouping, including sections.

          This setting should probably not exist, in other words it should be always on for all Moodle instances - there is never any reason to turn it off as the group members only feature is reliable, certainly not experimental, and has been used in large sites like ours for over five years. However, I believe it still exists and defaults to off.

          If that's not the case then maybe something is missing in 2.4, please file a new issue if concerned.

          Show
          Sam Marshall added a comment - Ray: There is a setting, which I think may be in the experimental section, called 'enable group members only' or something like that. You need to have this turned on in order to control access to anything by grouping, including sections. This setting should probably not exist, in other words it should be always on for all Moodle instances - there is never any reason to turn it off as the group members only feature is reliable, certainly not experimental, and has been used in large sites like ours for over five years. However, I believe it still exists and defaults to off. If that's not the case then maybe something is missing in 2.4, please file a new issue if concerned.
          Hide
          Ray Lawrence added a comment -

          Thanks. Yes, I'm aware of that setting and it's experimental status. Petr and I engaged in a long discussion about this here MDL-28225

          We can't recommend this as an option to clients in good faith as it may disappear at anytime, without replacement. So it's not really viable in the current state. Is didn't detect any desire by Petr to move this on.

          If the non-experimental aspect of groupings is left as is to manage the user interactions and the conditional access is move to the, um, conditional access functionality then it would be more logical IMO. I'll raise a feature request for this when I have a moment.

          Show
          Ray Lawrence added a comment - Thanks. Yes, I'm aware of that setting and it's experimental status. Petr and I engaged in a long discussion about this here MDL-28225 We can't recommend this as an option to clients in good faith as it may disappear at anytime, without replacement. So it's not really viable in the current state. Is didn't detect any desire by Petr to move this on. If the non-experimental aspect of groupings is left as is to manage the user interactions and the conditional access is move to the, um, conditional access functionality then it would be more logical IMO. I'll raise a feature request for this when I have a moment.
          Hide
          Sam Marshall added a comment -

          Ray: I wouldn't bother raising that request. I don't see any benefit to moving the feature. (It doesn't make a difference either way for topics, but for activities, if you select a grouping which may be used in order to choose a set of groups, then it obviously makes sense to use that same grouping to restrict access. I don't think there's a use case for having two separate settings, at least I haven't encountered one. And for consistency in both code and UI, the option should clearly be the same for activities and topics.)

          IMO there is no chance that the grouping/group members only feature will ever be removed (without providing an alternative that achieves the same thing). I know Petr doesn't like it but it is vital for the OU and given that there is no logical reason to remove it, it works well and performs well and has no problems and isn't even remotely experimental, I'm pretty certain we can argue against any attempt to get rid of it.

          We have been relying absolutely on the feature for the past five years, as noted. I'm pretty sure there are other large sites using it. I think you should recommend it wholeheartedly to all your clients. The more people using it, the more likely it is that one day, somebody will finally win the argument and get the admin setting removed in the proper way, by which I mean, so that it no longer needs an option because it is permanently enabled.

          And just by the way - the arguments as to why the groupmembersonly feature should be experimental apply to the other types of conditional access too! It's the same logic. (I.e. yes gradebook is not really able to indicate that a particular activity wasn't available to a particular user, which is perhaps less than ideal, but that applies whether it's a grouping groupmembersonly restriction, date restriction, something-else-being-complete restriction, or grade restriction.) There isn't any difference between groupmembersonly and other types of conditional availability, it just happens to appear in a different place on the activity settings form.

          Show
          Sam Marshall added a comment - Ray: I wouldn't bother raising that request. I don't see any benefit to moving the feature. (It doesn't make a difference either way for topics, but for activities, if you select a grouping which may be used in order to choose a set of groups, then it obviously makes sense to use that same grouping to restrict access. I don't think there's a use case for having two separate settings, at least I haven't encountered one. And for consistency in both code and UI, the option should clearly be the same for activities and topics.) IMO there is no chance that the grouping/group members only feature will ever be removed (without providing an alternative that achieves the same thing). I know Petr doesn't like it but it is vital for the OU and given that there is no logical reason to remove it, it works well and performs well and has no problems and isn't even remotely experimental, I'm pretty certain we can argue against any attempt to get rid of it. We have been relying absolutely on the feature for the past five years, as noted. I'm pretty sure there are other large sites using it. I think you should recommend it wholeheartedly to all your clients. The more people using it, the more likely it is that one day, somebody will finally win the argument and get the admin setting removed in the proper way, by which I mean, so that it no longer needs an option because it is permanently enabled. And just by the way - the arguments as to why the groupmembersonly feature should be experimental apply to the other types of conditional access too! It's the same logic. (I.e. yes gradebook is not really able to indicate that a particular activity wasn't available to a particular user, which is perhaps less than ideal, but that applies whether it's a grouping groupmembersonly restriction, date restriction, something-else-being-complete restriction, or grade restriction.) There isn't any difference between groupmembersonly and other types of conditional availability, it just happens to appear in a different place on the activity settings form.

            People

            • Votes:
              45 Vote for this issue
              Watchers:
              33 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: