Moodle
  1. Moodle
  2. MDL-44070

Conditional availability: Enhancements including OR conditions, plugins

    Details

    • Testing Instructions:
      Hide

      UPGRADE TESTING

      The upgrade cannot easily be included in automated tests (the key function is included in a unit test, but that doesn't test the full process) and it is very important that it works, so I am writing a manual test script.

      1. You need a Moodle 2.6 install.

      • This could be a fresh install e.g. using new prefix.
      • Or it could be an existing install if you don't mind upgrading it to 2.7.
      • Alternatively, you can also use a Moodle 2.7 install prior to this change
        landing. Key point is that you must not have yet run the database update
        that comes with this change.

      2. In this Moodle 2.6 install, turn on the following options:

      • enablecompletion
      • enableavailability
      • enablegroupmembersonly (this one is on the experimental settings page)

      3. Restore the attached backup 'revised-26-backup.mbz' to a new course using default settings.

      4. Upgrade your Moodle 2.6 install to Moodle 2.7 including this change.

      EXPECTED: As part of the System upgrade, you should see a progress bar as it updates availability settings.

      5. Check the availability settings for each activity and topic as follows. You can do some of this just by looking at the page with editing on, but for at least a few of them, I suggest double-checking on the editing screen.

      In each case the top-level conditions should be an AND group (which doesn't display in the interface except for cases when there's more than one). The 'shown/hidden' in brackets refers to the action taken when that condition fails, i.e. 'hidden' = hidden completely from users who don't meet the condition and 'shown' = shown greyed out with a message.

      NOTE: Times are from Europe/London timezone, may vary if user has other timezone.

      Page with FROM date (2024)

      • Date >= 10 March 2024 00:00 (shown)

      Page with UNTIL date (March 5)

      • Date < 5 March 2014 00:00(hidden)

      Page with FROM and UNTIL (not shown)

      • Date >= 10 December 2015 00:00 (shown)
      • Date < 4 March 2020 00:00 (hidden)

      Quiz grade >= 75%

      • 'Quiz for grade' score is >= 75%. (shown)

      Quiz grade < 25%

      • 'Quiz for grade' score is < 25%. (shown)

      Quiz grade 50-60%

      • 'Quiz for grade' score is >=50 and <60%. (shown)

      Email contains frog

      • Email address contains frog. (shown)

      Tickbox ticked

      • Page with tickbox is marked complete. (shown)

      Quiz complete but failed

      • Quiz for grade is complete and failed. (shown)

      Quiz complete and passed

      • Quiz for grade is complete and passed. (shown)

      Quiz not complete

      • Quiz for grade is incomplete (shown)

      Grouping Page

      • (Not in availability system, but: groupmembersonly is turned on and 'Example grouping' selected)

      All the options!

      • Date >= 4 March 2017 00:00 (shown)
      • Date < 4 March 2024 00:00 (hidden)
      • Score in Quiz for grade >= 30 and <35% (shown)
      • Score in Course total >= 5 and <10% (shown)
      • Page with tickbox is complete (shown)
      • Quiz for grade is incomplete (shown)
      • City/town contains Frogtown. (shown)
      • Email address contains @ (shown)
      • (Not in availability system, but: groupmembersonly is turned on and 'Example grouping' selected)

      Group forum (group members only)

      • (Not in availability system, but: groupmembersonly is turned on, no grouping)

      Topic with lots of conditions

      • Date >= 5 December 2014 00:00 (hidden)
      • Page with tickbox is complete (hidden)
      • Quiz for grade >= 20% (hidden)
      • Email address contains @ (hidden)

      Topic with grouping (works slightly differently)

      • Grouping: Example grouping (hidden)

      PERFORMANCE

      • Use the "Make a test course feature" to create an M sized course
      • Add conditional activities to 50 activities
      • Login as a student and time the course page loading time
      • Disable the conditional access feature and compare the course page load time
      Show
      UPGRADE TESTING The upgrade cannot easily be included in automated tests (the key function is included in a unit test, but that doesn't test the full process) and it is very important that it works, so I am writing a manual test script. 1. You need a Moodle 2.6 install. This could be a fresh install e.g. using new prefix. Or it could be an existing install if you don't mind upgrading it to 2.7. Alternatively, you can also use a Moodle 2.7 install prior to this change landing. Key point is that you must not have yet run the database update that comes with this change. 2. In this Moodle 2.6 install, turn on the following options: enablecompletion enableavailability enablegroupmembersonly (this one is on the experimental settings page) 3. Restore the attached backup 'revised-26-backup.mbz' to a new course using default settings. 4. Upgrade your Moodle 2.6 install to Moodle 2.7 including this change. EXPECTED: As part of the System upgrade, you should see a progress bar as it updates availability settings. 5. Check the availability settings for each activity and topic as follows. You can do some of this just by looking at the page with editing on, but for at least a few of them, I suggest double-checking on the editing screen. In each case the top-level conditions should be an AND group (which doesn't display in the interface except for cases when there's more than one). The 'shown/hidden' in brackets refers to the action taken when that condition fails, i.e. 'hidden' = hidden completely from users who don't meet the condition and 'shown' = shown greyed out with a message. NOTE: Times are from Europe/London timezone, may vary if user has other timezone. Page with FROM date (2024) Date >= 10 March 2024 00:00 (shown) Page with UNTIL date (March 5) Date < 5 March 2014 00:00(hidden) Page with FROM and UNTIL (not shown) Date >= 10 December 2015 00:00 (shown) Date < 4 March 2020 00:00 (hidden) Quiz grade >= 75% 'Quiz for grade' score is >= 75%. (shown) Quiz grade < 25% 'Quiz for grade' score is < 25%. (shown) Quiz grade 50-60% 'Quiz for grade' score is >=50 and <60%. (shown) Email contains frog Email address contains frog. (shown) Tickbox ticked Page with tickbox is marked complete. (shown) Quiz complete but failed Quiz for grade is complete and failed. (shown) Quiz complete and passed Quiz for grade is complete and passed. (shown) Quiz not complete Quiz for grade is incomplete (shown) Grouping Page (Not in availability system, but: groupmembersonly is turned on and 'Example grouping' selected) All the options! Date >= 4 March 2017 00:00 (shown) Date < 4 March 2024 00:00 (hidden) Score in Quiz for grade >= 30 and <35% (shown) Score in Course total >= 5 and <10% (shown) Page with tickbox is complete (shown) Quiz for grade is incomplete (shown) City/town contains Frogtown. (shown) Email address contains @ (shown) (Not in availability system, but: groupmembersonly is turned on and 'Example grouping' selected) Group forum (group members only) (Not in availability system, but: groupmembersonly is turned on, no grouping) Topic with lots of conditions Date >= 5 December 2014 00:00 (hidden) Page with tickbox is complete (hidden) Quiz for grade >= 20% (hidden) Email address contains @ (hidden) Topic with grouping (works slightly differently) Grouping: Example grouping (hidden) PERFORMANCE Use the "Make a test course feature" to create an M sized course Add conditional activities to 50 activities Login as a student and time the course page loading time Disable the conditional access feature and compare the course page load time
    • Affected Branches:
      MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_27_STABLE
    • Pull Master Branch:
      MDL-44070-master

      Description

      I propose to make major improvements to the conditional availability system.

      Key improvements:

      • Support for OR and NOT conditions (current system only does AND) and condition trees.
      • Improved user interface using JavaScript.
      • Support for pluggable restrictions instead of the current hardcoded options (date, grade, etc).
      • Tidy up 'grouping' feature - currently used both for grouped activities (to select a set of groups) and also as a restriction - to separate these functions.
      • Behaviour is intended to be fully backward-compatible via database upgrade. I also intend for public API functions to continue working (although possibly with deprecation/name changes).

      Out of scope:

      • This enhancement is strictly related to conditional activity/section availability (the system controlled by $CFG->enableavailability). For example, there will be no changes to the activity or course completion system at this point.

      Please see the attached Word document for a detailed explanation of planned changes.

        Gliffy Diagrams

        1. Conditional activity availability - community version.docx
          220 kB
          Sam Marshall
        1. How error appeared on restored course.jpg
          211 kB
        2. missingstrings.png
          70 kB

          Issue Links

            Activity

            Hide
            Sam Marshall added a comment -
            Show
            Sam Marshall added a comment - Forum thread: https://moodle.org/mod/forum/discuss.php?d=253958
            Hide
            Adam Morris added a comment -

            This is a really great idea.

            Consider that hiding an item is also a way to restrict access. It's essentially making the restriction "Restrict if their role is not "Teachers" or "Managers". It would be nice if, when the user hides an activity, that it essentially adds this restriction into the conditional availability thingie.

            Show
            Adam Morris added a comment - This is a really great idea. Consider that hiding an item is also a way to restrict access. It's essentially making the restriction "Restrict if their role is not "Teachers" or "Managers". It would be nice if, when the user hides an activity, that it essentially adds this restriction into the conditional availability thingie.
            Hide
            Alexander Bias added a comment -

            Dear Sam,

            thank you for going ahead to improve the conditional availability.

            We would be very grateful if you could have a look at MDL-38058 along the way and hopefully resolve it. MDL-38058 prevents us from using the conditional availability at all.

            Thanks in advance,
            Alex

            Show
            Alexander Bias added a comment - Dear Sam, thank you for going ahead to improve the conditional availability. We would be very grateful if you could have a look at MDL-38058 along the way and hopefully resolve it. MDL-38058 prevents us from using the conditional availability at all. Thanks in advance, Alex
            Hide
            Sam Marshall added a comment -

            Alexander: Thanks for comment. If there's time, I'll try to resolve that issue while converting the user profile field code into the new plugin. By the way, under my proposed system based on plugins, even if I didn't fix it, you could potentially just delete the user profile condition plugin (so that users could use other types of condition) and the system would still work.

            Adam: Thanks for comment! I take your point about the 'visible' option, which is something I hadn't thought about - you're right, it is just one more way to restrict access. That's been there for a very, very long time, though - and users expect to be able to have that option even if they haven't enabled the availability system (in features page). My immediate opinion is that I'd prefer to leave it alone during this change; there is clearly a potential to merge it in later (or merge in the back-end and leave it with separate UI).

            Show
            Sam Marshall added a comment - Alexander: Thanks for comment. If there's time, I'll try to resolve that issue while converting the user profile field code into the new plugin. By the way, under my proposed system based on plugins, even if I didn't fix it, you could potentially just delete the user profile condition plugin (so that users could use other types of condition) and the system would still work. Adam: Thanks for comment! I take your point about the 'visible' option, which is something I hadn't thought about - you're right, it is just one more way to restrict access. That's been there for a very, very long time, though - and users expect to be able to have that option even if they haven't enabled the availability system (in features page). My immediate opinion is that I'd prefer to leave it alone during this change; there is clearly a potential to merge it in later (or merge in the back-end and leave it with separate UI).
            Hide
            Adam Morris added a comment -

            Sam, I would probably concur with your initial response, however allow me to talk out loud if nothing else and finish the thought. Before I do that, yes, the eye and visibility toggle should stay as the same UI as it's always been.

            If Moodle could restrict things by roles in a course context, then you'd essentially be giving it a more robust CMSy feature. Now, Moodle is supposed to be a VLE and not yet another CMS package, but hear me out. It's used by schools. You have administers and coordinators and then teaching staff and parents and support staff, and if schools are like most of the ones I worked at they wouldn't be adverse to use their Moodle for things a bit beyond the classroom. Especially if it meant not having to use a whole nother package.

            The visibility toggle actually restricts access by role, the problem is that it hardcodes which roles... $access = $userrole != $studentrole

            Also, once you start restricting access in one place, users are going to expect to do all such restriction in that one place. However, you can also restrict access by toggling the item or toggling the section even.

            Visibility turns the item grey, but the restrictions outputs a message. That doesn't seem right, either.

            It's a lot to ask the user to jump through hoops like this, and it's one of the main problems, as I see it, with the restrictions setup as implemented now.

            Show
            Adam Morris added a comment - Sam, I would probably concur with your initial response, however allow me to talk out loud if nothing else and finish the thought. Before I do that, yes, the eye and visibility toggle should stay as the same UI as it's always been. If Moodle could restrict things by roles in a course context, then you'd essentially be giving it a more robust CMSy feature. Now, Moodle is supposed to be a VLE and not yet another CMS package, but hear me out. It's used by schools. You have administers and coordinators and then teaching staff and parents and support staff, and if schools are like most of the ones I worked at they wouldn't be adverse to use their Moodle for things a bit beyond the classroom. Especially if it meant not having to use a whole nother package. The visibility toggle actually restricts access by role, the problem is that it hardcodes which roles... $access = $userrole != $studentrole Also, once you start restricting access in one place, users are going to expect to do all such restriction in that one place. However, you can also restrict access by toggling the item or toggling the section even. Visibility turns the item grey, but the restrictions outputs a message. That doesn't seem right, either. It's a lot to ask the user to jump through hoops like this, and it's one of the main problems, as I see it, with the restrictions setup as implemented now.
            Hide
            Tim Hunt added a comment -

            You can restrict things by role. All activities have a mod/whatever:view capability. Roles without that capability can't see the activity. And you can override roles for just one activity.

            Show
            Tim Hunt added a comment - You can restrict things by role. All activities have a mod/whatever:view capability. Roles without that capability can't see the activity. And you can override roles for just one activity.
            Hide
            Sam Marshall added a comment -

            Adam: Yes I see the point. However, a few notes in addition to Tim's:

            1. This change will at least reduce the number of different places you can restrict access (at present you can restrict it with the eye, with grouping, and in 'Restrict access' - I'm moving grouping into 'Restrict access') so it's progress.

            2. There are actually 2 ways to restrict access by role at present, both of which will still work in the new system. One is the way Tim outlined; the other is to hide an activity using the eye icon and then override 'viewhiddenactivities' for that activity, to allow specific roles (not just the default one) to view it. Which you do depends on whether you want it grey or not. (You said the list of roles was hard-coded; it isn't, it uses that capability. So you can perfectly well change the list at system level across your site as well, e.g. if you don't want teachers to be able to view hidden activities.)

            3. It would be possible to add a new access restriction plugin to directly restrict access by role - with an easy dropdown so you could add restrictions like '(has role Teacher OR has role Student) AND NOT (has role Malingerer)' - after this new API is implemented. That might result in a simple UI and I can see the attraction to it, BUT it may well not be a good idea given we already have two ways to do it, at least one of which isn't a hack. So I can imagine that might not get into core. (Also there will be a performance cost to this restriction, compared to capabilities which are already optimised/cached.) There will be nothing to stop people making a contrib plugin that does exactly that though!

            The real reason I don't want to move the eye icon into the same back-end is that it will be easier to do the front-end if I am displaying to the user (for editing using the new JS interface) all the things that are in this system, and I don't have to preprocess it first to remove a hidden node that's handling the eye icon, or something like that. It wouldn't be impossible or anything. But I think if we did put the eye icon into the same back-end we would leave it with the same functionality, i.e. if set to hidden, it means you must have viewhiddenactivities capability.

            Show
            Sam Marshall added a comment - Adam: Yes I see the point. However, a few notes in addition to Tim's: 1. This change will at least reduce the number of different places you can restrict access (at present you can restrict it with the eye, with grouping, and in 'Restrict access' - I'm moving grouping into 'Restrict access') so it's progress. 2. There are actually 2 ways to restrict access by role at present, both of which will still work in the new system. One is the way Tim outlined; the other is to hide an activity using the eye icon and then override 'viewhiddenactivities' for that activity, to allow specific roles (not just the default one) to view it. Which you do depends on whether you want it grey or not. (You said the list of roles was hard-coded; it isn't, it uses that capability. So you can perfectly well change the list at system level across your site as well, e.g. if you don't want teachers to be able to view hidden activities.) 3. It would be possible to add a new access restriction plugin to directly restrict access by role - with an easy dropdown so you could add restrictions like '(has role Teacher OR has role Student) AND NOT (has role Malingerer)' - after this new API is implemented. That might result in a simple UI and I can see the attraction to it, BUT it may well not be a good idea given we already have two ways to do it, at least one of which isn't a hack. So I can imagine that might not get into core. (Also there will be a performance cost to this restriction, compared to capabilities which are already optimised/cached.) There will be nothing to stop people making a contrib plugin that does exactly that though! The real reason I don't want to move the eye icon into the same back-end is that it will be easier to do the front-end if I am displaying to the user (for editing using the new JS interface) all the things that are in this system, and I don't have to preprocess it first to remove a hidden node that's handling the eye icon, or something like that. It wouldn't be impossible or anything. But I think if we did put the eye icon into the same back-end we would leave it with the same functionality, i.e. if set to hidden, it means you must have viewhiddenactivities capability.
            Hide
            Sam Marshall added a comment -

            Note: I've added information about my development branch just in case anyone wants to follow the development, but mainly for later use. Obviously development has barely started so I don't recommend that anyone else uses any of the code there yet!

            Show
            Sam Marshall added a comment - Note: I've added information about my development branch just in case anyone wants to follow the development, but mainly for later use. Obviously development has barely started so I don't recommend that anyone else uses any of the code there yet!
            Hide
            Ray Lawrence added a comment -

            The current capability restricts availability by role for activity types not per activity instance.

            A per instance role based restriction would be a great future addition as in 3 above provided the performance cost isn't prohibitive.

            Really looking forward to see this phase progress.

            Show
            Ray Lawrence added a comment - The current capability restricts availability by role for activity types not per activity instance. A per instance role based restriction would be a great future addition as in 3 above provided the performance cost isn't prohibitive. Really looking forward to see this phase progress.
            Hide
            Adam Morris added a comment -

            Wow, okay, wait...

            So you can actually already do what I am trying to describe: per instance access by role. Hide it, and then manually override which role can view that item, but the "view" capability is a bot of a misnomer then, because if a user doesn't have view capability they don't have no capability? Therefore it's really an "access" capability? (He says as he reaches for my dev computer to try this...)

            I would concur that letting a plugin do this is probably best, given all the points above.

            Awesome. +++1

            Show
            Adam Morris added a comment - Wow, okay, wait... So you can actually already do what I am trying to describe: per instance access by role. Hide it, and then manually override which role can view that item, but the "view" capability is a bot of a misnomer then, because if a user doesn't have view capability they don't have no capability? Therefore it's really an "access" capability? (He says as he reaches for my dev computer to try this...) I would concur that letting a plugin do this is probably best, given all the points above. Awesome. +++1
            Hide
            Tim Hunt added a comment -

            Ray, it will work perinstance. The mod/forum:view capability is checked in the context of that forum, not in the course. You can override the permission in just that one forum.

            Adam, it is not that bad a misnomer. If you cannot see a particuarl forum at all, how can you do anything else to it?

            Show
            Tim Hunt added a comment - Ray, it will work perinstance. The mod/forum:view capability is checked in the context of that forum, not in the course. You can override the permission in just that one forum. Adam, it is not that bad a misnomer. If you cannot see a particuarl forum at all, how can you do anything else to it?
            Hide
            Ray Lawrence added a comment -

            Good point. The issue with that approach is that it's not that obvious. Per instance role based restriction would be better in the Restrict access interface IMO. That's one for debate at a later date..

            Show
            Ray Lawrence added a comment - Good point. The issue with that approach is that it's not that obvious. Per instance role based restriction would be better in the Restrict access interface IMO. That's one for debate at a later date..
            Hide
            Sam Marshall added a comment -

            I've written manual test instructions for the upgrade step. (This is obviously not finished yet and the test might not pass, just recording here as I've written it.)

            Show
            Sam Marshall added a comment - I've written manual test instructions for the upgrade step. (This is obviously not finished yet and the test might not pass, just recording here as I've written it.)
            Hide
            Sam Marshall added a comment -

            Attached Moodle 2.6 backup (name includes course-3) for use when testing upgrade (and, later, restore).

            Show
            Sam Marshall added a comment - Attached Moodle 2.6 backup (name includes course-3) for use when testing upgrade (and, later, restore).
            Hide
            Sam Marshall added a comment -

            I have now finished (I think) the back-end part of this change. You can find it at the Git repository linked in this issue.

            Note that there is no front-end yet, so if you were to try this code, although it is supposed to work, you would find it unusable (unless you fancy typing in conditions as obscure JSON strings!)

            There are extensive unit tests for all significant areas of new code, and the entire Moodle unit tests do still pass. (Except the three or four which also fail on my master checkout, unrelated to this.)

            This is a large change (+8k, -3k lines) & includes 'tidying up' changes over many files) so reviewing will be a pain. If any HQ developer wants to make a start on reviewing this code before the front-end is finished, so as to spread out the workload, please do. The code is fairly logically divided into commits (with useful messages), so that you can review one commit at a time. By the way, the branch includes the commit for MDL-44141 because it relies on that change, ignore that one, it will disappear when integrated.

            There are two minor functionality changes (to obscure parts of quiz and workshop) as a result of the removal of the previous experimental groupmembersonly option. I have raised these with the relevant maintainers; Tim has okayed the quiz one, David has not yet had time to review the workshop change.

            I am going to start work on the front-end part of the change (which should be much simpler and almost entirely contained to JavaScript and CSS changes, unless I spot something wrong with the API as a result of the JS work) tomorrow and will try to complete it as quickly as possible, but it might take quite some days as there's a fair bit to do. Plus after that is done I will have to learn how to write Behat tests, which is apparently a nightmare.

            Show
            Sam Marshall added a comment - I have now finished (I think) the back-end part of this change. You can find it at the Git repository linked in this issue. Note that there is no front-end yet, so if you were to try this code, although it is supposed to work, you would find it unusable (unless you fancy typing in conditions as obscure JSON strings!) There are extensive unit tests for all significant areas of new code, and the entire Moodle unit tests do still pass. (Except the three or four which also fail on my master checkout, unrelated to this.) This is a large change (+8k, -3k lines) & includes 'tidying up' changes over many files) so reviewing will be a pain. If any HQ developer wants to make a start on reviewing this code before the front-end is finished, so as to spread out the workload, please do. The code is fairly logically divided into commits (with useful messages), so that you can review one commit at a time. By the way, the branch includes the commit for MDL-44141 because it relies on that change, ignore that one, it will disappear when integrated. There are two minor functionality changes (to obscure parts of quiz and workshop) as a result of the removal of the previous experimental groupmembersonly option. I have raised these with the relevant maintainers; Tim has okayed the quiz one, David has not yet had time to review the workshop change. I am going to start work on the front-end part of the change (which should be much simpler and almost entirely contained to JavaScript and CSS changes, unless I spot something wrong with the API as a result of the JS work) tomorrow and will try to complete it as quickly as possible, but it might take quite some days as there's a fair bit to do. Plus after that is done I will have to learn how to write Behat tests, which is apparently a nightmare.
            Hide
            Damyon Wiese added a comment -

            I commented on the forum post - just cross linking it here: https://moodle.org/mod/forum/discuss.php?d=253958#p1113483

            Show
            Damyon Wiese added a comment - I commented on the forum post - just cross linking it here: https://moodle.org/mod/forum/discuss.php?d=253958#p1113483
            Hide
            Sam Marshall added a comment -

            CHANGE TO SCOPE

            Following the discussion I have removed all groupmembersonly changes from this feature so that there is a chance it will be included into Moodle 2.7. See forum post here:

            https://moodle.org/mod/forum/discuss.php?d=253958#p1113663

            I have filed MDL-44725 about removing the (fairly broken) groupmembersonly feature and replacing it with availability conditions - this will not happen until Moodle 2.8.

            Show
            Sam Marshall added a comment - CHANGE TO SCOPE Following the discussion I have removed all groupmembersonly changes from this feature so that there is a chance it will be included into Moodle 2.7. See forum post here: https://moodle.org/mod/forum/discuss.php?d=253958#p1113663 I have filed MDL-44725 about removing the (fairly broken) groupmembersonly feature and replacing it with availability conditions - this will not happen until Moodle 2.8.
            Hide
            Sam Marshall added a comment -

            For info, the current branch on GitHub (still not finished) now incorporates the change to stop modifying groupmembersonly, as well as additional API features/improvements/refactoring.

            The availability unit tests all now pass again, but I think some other unit tests may fail, and I haven't retried the frontend, that may well be completely broken (and still isn't finished). So I wouldn't really advise anyone tries this out just yet...

            Show
            Sam Marshall added a comment - For info, the current branch on GitHub (still not finished) now incorporates the change to stop modifying groupmembersonly, as well as additional API features/improvements/refactoring. The availability unit tests all now pass again, but I think some other unit tests may fail, and I haven't retried the frontend, that may well be completely broken (and still isn't finished). So I wouldn't really advise anyone tries this out just yet...
            Hide
            Sam Marshall added a comment -

            For info: I made a blog post and screencast (which you have to download instead of playing directly, because reasons) demonstrating the UI for this feature: http://learn1.open.ac.uk/mod/oublog/viewpost.php?post=143752 - I'm going to post this on the forum but thought it might be useful to have it here as well.

            Show
            Sam Marshall added a comment - For info: I made a blog post and screencast (which you have to download instead of playing directly, because reasons) demonstrating the UI for this feature: http://learn1.open.ac.uk/mod/oublog/viewpost.php?post=143752 - I'm going to post this on the forum but thought it might be useful to have it here as well.
            Hide
            Sam Marshall added a comment -

            OK, this feature is finally finished, so I'm submitting for peer review. I don't know whether this is now too late to make 2.7 (even though it's not the code freeze yet, and I've removed the slightly scarier groupmembersonly changes); Martin suggested it might be possible to make 2.7 if I removed the groupmembersonly stuff, so here goes!

            If this doesn't make 2.7 I hope it can be considered as the first priority for 2.8 once that branch opens.

            Notes for peer reviewer

            1. Commit structure

            This change is broken into separate logical commits that make it easier to read and understand, but they cannot be separated (i.e. Moodle will not work properly if you only apply 3 of them). It wasn't possible to achieve this. They generally do not modify the same file in more than one commit (one or two exceptions where it's different areas of the file).

            2. Test script

            There are ample unit tests on all parts of the back-end (part of the reason why there are so many lines of code here), but there is only a manual test script for upgrade (above).

            I'm definitely going to write a Behat test that covers this feature, but I didn't want that to hold up peer review. Any peer review would be conditional on me getting the Behat test in.

            Please review the rest of it regardless of this change; the test script can be reviewed separately.

            3. Date selector

            This branch includes one commit which won't be going into Moodle, the one labelled as MDL-44814. Basically this is a tweak to the current date selector, but Andrew is about to replace that.

            Please review the rest of it regardless of this issue; I'll need to redo that small part, but it can be reviewed separately.

            Show
            Sam Marshall added a comment - OK, this feature is finally finished, so I'm submitting for peer review. I don't know whether this is now too late to make 2.7 (even though it's not the code freeze yet, and I've removed the slightly scarier groupmembersonly changes); Martin suggested it might be possible to make 2.7 if I removed the groupmembersonly stuff, so here goes! If this doesn't make 2.7 I hope it can be considered as the first priority for 2.8 once that branch opens. Notes for peer reviewer 1. Commit structure This change is broken into separate logical commits that make it easier to read and understand, but they cannot be separated (i.e. Moodle will not work properly if you only apply 3 of them). It wasn't possible to achieve this. They generally do not modify the same file in more than one commit (one or two exceptions where it's different areas of the file). 2. Test script There are ample unit tests on all parts of the back-end (part of the reason why there are so many lines of code here), but there is only a manual test script for upgrade (above). I'm definitely going to write a Behat test that covers this feature, but I didn't want that to hold up peer review. Any peer review would be conditional on me getting the Behat test in. Please review the rest of it regardless of this change; the test script can be reviewed separately. 3. Date selector This branch includes one commit which won't be going into Moodle, the one labelled as MDL-44814 . Basically this is a tweak to the current date selector, but Andrew is about to replace that. Please review the rest of it regardless of this issue; I'll need to redo that small part, but it can be reviewed separately.
            Show
            CiBoT added a comment - Results for MDL-44070 Remote repository: https://github.com/sammarshallou/moodle.git Remote branch MDL-44070 -master to be integrated into upstream master Executed job http://social.srv.in.moodle.com/job/Precheck%20remote%20branch/2479 Details: http://social.srv.in.moodle.com/job/Precheck%20remote%20branch/2479/artifact/work/smurf.html
            Hide
            Dan Poltawski added a comment -

            I'm just recording my thoughts here to be on record after private discussion with Sam.

            To be clear, its very close to the freeze date, but I don't see a problem with this landing at the last possible moment (& have confirmed the same with Eloy). It is OK for this to land in this last week (although obviously we'd like to avoid that in general). The bigger problem is going to be getting it peer reviewed in time, hopefully we can manage to achieve that in Moodle HQ.

            You've told me that at this point the main thing missing is behat tests:

            • We might be able to support you with that - just shout if you need it.
            • We can definitely accept more behat tests after the freeze.
            • Also, given your excellent track record, the fact this change has been well communicated far in advance and the fact you've adapted to our suggested changes v.late in the game - IMO we can afford to be a bit flexible about accepting the behat tests later.
            Show
            Dan Poltawski added a comment - I'm just recording my thoughts here to be on record after private discussion with Sam. To be clear, its very close to the freeze date, but I don't see a problem with this landing at the last possible moment (& have confirmed the same with Eloy). It is OK for this to land in this last week (although obviously we'd like to avoid that in general). The bigger problem is going to be getting it peer reviewed in time, hopefully we can manage to achieve that in Moodle HQ. You've told me that at this point the main thing missing is behat tests: We might be able to support you with that - just shout if you need it. We can definitely accept more behat tests after the freeze. Also, given your excellent track record, the fact this change has been well communicated far in advance and the fact you've adapted to our suggested changes v.late in the game - IMO we can afford to be a bit flexible about accepting the behat tests later.
            Hide
            Sam Marshall added a comment -

            OK, update for end of week (I'm off for the weekend, back Monday).

            1. I've fixed most of the cibot problems (except the ones which are correct, mostly no doc for functions that implement base class). Will request CI again.
            2. I've implemented comprehensive behat tests - note that this requires a couple of extra things adding to behat, included as extra commits in this branch. The behat tests all worked. (Note past tense.)
            3. I've rebased on latest master.

            The behat tests don't work on my system since rebasing on master. The reason is that the Moodle admin Behat step "I set the following administration settings values" has stopped working - you can see that all my tests fail because they need to turn on enableavailability. I looked at the code but can't actually see anything wrong with it, so I wonder if there's something more general broken with behat, or what. Anyhow, the tests are there and ought to work if somebody fixes that behat step...

            Show
            Sam Marshall added a comment - OK, update for end of week (I'm off for the weekend, back Monday). 1. I've fixed most of the cibot problems (except the ones which are correct, mostly no doc for functions that implement base class). Will request CI again. 2. I've implemented comprehensive behat tests - note that this requires a couple of extra things adding to behat, included as extra commits in this branch. The behat tests all worked. (Note past tense.) 3. I've rebased on latest master. The behat tests don't work on my system since rebasing on master. The reason is that the Moodle admin Behat step "I set the following administration settings values" has stopped working - you can see that all my tests fail because they need to turn on enableavailability. I looked at the code but can't actually see anything wrong with it, so I wonder if there's something more general broken with behat, or what. Anyhow, the tests are there and ought to work if somebody fixes that behat step...
            Show
            CiBoT added a comment - Results for MDL-44070 Remote repository: https://github.com/sammarshallou/moodle.git Remote branch MDL-44070 -master to be integrated into upstream master Executed job http://social.srv.in.moodle.com/job/Precheck%20remote%20branch/2513 Details: http://social.srv.in.moodle.com/job/Precheck%20remote%20branch/2513/artifact/work/smurf.html
            Hide
            Sam Marshall added a comment -

            Expected codechecker issues

            These are all the issues which I believe do not need correcting.

            1. mod/forum/discuss.php & course/lib.php - incorrect indents

            These changes are only a few lines in a larger file - I'm matching the legacy indent.

            2. (many cases) - 'is not documented'

            Codechecker cannot spot the case where there is no phpdoc for a function because it is implementing an API that is already defined in a parent class. This is definitely permitted, I checked with Eloy.

            (Sorry I am going to have to run ci another time to check I managed to fix the other ones that aren't supposed to happen...)

            Show
            Sam Marshall added a comment - Expected codechecker issues These are all the issues which I believe do not need correcting. 1. mod/forum/discuss.php & course/lib.php - incorrect indents These changes are only a few lines in a larger file - I'm matching the legacy indent. 2. (many cases) - 'is not documented' Codechecker cannot spot the case where there is no phpdoc for a function because it is implementing an API that is already defined in a parent class. This is definitely permitted, I checked with Eloy. (Sorry I am going to have to run ci another time to check I managed to fix the other ones that aren't supposed to happen...)
            Hide
            Sam Marshall added a comment -

            Update: My previous problems with Behat testing were caused because Selenium 2.41.0 (current version) running on my Windows 7 / Firefox 28 system causes failures in a variety of Moodle unit tests, not just mine. For example, --tags=block_notification will also fail.

            As a result, all the Behat tests are now passing.

            Show
            Sam Marshall added a comment - Update: My previous problems with Behat testing were caused because Selenium 2.41.0 (current version) running on my Windows 7 / Firefox 28 system causes failures in a variety of Moodle unit tests, not just mine. For example, --tags=block_notification will also fail. As a result, all the Behat tests are now passing.
            Hide
            CiBoT added a comment -
            Show
            CiBoT added a comment - Results for MDL-44070 Remote repository: https://github.com/sammarshallou/moodle.git Remote branch MDL-44070 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2644 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2644/artifact/work/smurf.html
            Hide
            Sam Marshall added a comment -

            Just fixed a few more codechecker issues (sigh), I hope it's all done now.

            Show
            Sam Marshall added a comment - Just fixed a few more codechecker issues (sigh), I hope it's all done now.
            Hide
            CiBoT added a comment -
            Show
            CiBoT added a comment - Results for MDL-44070 Remote repository: https://github.com/sammarshallou/moodle.git Remote branch MDL-44070 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2646 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2646/artifact/work/smurf.html
            Hide
            Sam Marshall added a comment -

            I have modified this to remove the dependency on the date selector (as a result, the date selector popup is temporarily disabled - you can use dropdowns). This can be resolved after it is integrated, e.g. I can make the code work with the new date selector if it lands.

            Show
            Sam Marshall added a comment - I have modified this to remove the dependency on the date selector (as a result, the date selector popup is temporarily disabled - you can use dropdowns). This can be resolved after it is integrated, e.g. I can make the code work with the new date selector if it lands.
            Hide
            Sam Marshall added a comment -

            This has been rebased again to take into account review comments (thanks Eloy!) on my Behat steps, basically by changing my JavaScript slightly I was able to use an existing Behat step instead of having to make a new one in one case.

            Think that means that this now only depends on something already submitted for integration, so is not blocked. Still needs reviewing though!

            I realise there is a lot of code here (+13k lines, -3k lines). If necessary I can get somebody here to do a review - I don't think Tim has the time, so it'll have to be a developer from my team - although that would probably not be ideal. Please let me know if doing that would help.

            Show
            Sam Marshall added a comment - This has been rebased again to take into account review comments (thanks Eloy!) on my Behat steps, basically by changing my JavaScript slightly I was able to use an existing Behat step instead of having to make a new one in one case. Think that means that this now only depends on something already submitted for integration, so is not blocked. Still needs reviewing though! I realise there is a lot of code here (+13k lines, -3k lines). If necessary I can get somebody here to do a review - I don't think Tim has the time, so it'll have to be a developer from my team - although that would probably not be ideal. Please let me know if doing that would help.
            Hide
            Sam Marshall added a comment -

            I've rebased this again to latest master (some minor changes were necessary to fix modified core code in the same areas this touches). Have rerun phpunit tests for whole system, and the Behat tests for this feature (--tags=availability,core_availability), they pass.

            Regarding the size of the code review, it may be of interest that if you exclude the completely new API code (/availability), there are 'only' about 2k added lines total between the other commits, most of which are in backup and many of which are in PHPunit tests. Not sure it is practical for somebody to review only that part, as obviously you might need to know how the new API works as the other changes are going to call into it, but it might be a way forward, for example if I get a developer on my team to review the /availability part.

            Show
            Sam Marshall added a comment - I've rebased this again to latest master (some minor changes were necessary to fix modified core code in the same areas this touches). Have rerun phpunit tests for whole system, and the Behat tests for this feature (--tags=availability,core_availability), they pass. Regarding the size of the code review, it may be of interest that if you exclude the completely new API code (/availability), there are 'only' about 2k added lines total between the other commits, most of which are in backup and many of which are in PHPunit tests. Not sure it is practical for somebody to review only that part, as obviously you might need to know how the new API works as the other changes are going to call into it, but it might be a way forward, for example if I get a developer on my team to review the /availability part.
            Hide
            Marina Glancy added a comment - - edited

            Hi, Sam. This is mega-work. I'm looking forward to see it in Moodle.
            I just did a very quick code review and just want to point out some little errors:

            • behat step regex should look different
            • deprecated global functions should be moved in lib/deprecatedlib.php
            • I don't see any .dir-rtl conditions in CSS which means that this interface was never tested in RTL language
            • there is a weird namespace and package name "availability_group" - those are not allowed.
            • rebuild_course_cache() is called from inside upgrade process

            This was extra-quick, I'm sure there is much more.

            Show
            Marina Glancy added a comment - - edited Hi, Sam. This is mega-work. I'm looking forward to see it in Moodle. I just did a very quick code review and just want to point out some little errors: behat step regex should look different deprecated global functions should be moved in lib/deprecatedlib.php I don't see any .dir-rtl conditions in CSS which means that this interface was never tested in RTL language there is a weird namespace and package name "availability_group" - those are not allowed. rebuild_course_cache() is called from inside upgrade process This was extra-quick, I'm sure there is much more.
            Hide
            Marina Glancy added a comment - - edited

            and very important - this fix misses version bump and updating function redirect_if_major_upgrade_required() - after pulling your branch I have three screens of errors.

            Show
            Marina Glancy added a comment - - edited and very important - this fix misses version bump and updating function redirect_if_major_upgrade_required() - after pulling your branch I have three screens of errors.
            Hide
            Marina Glancy added a comment -

            Here is the upgrade screenshot. Strings are missing and plugins are listed as "Add-ons" instead of "Standard" - see core_plugin_manager::standard_plugins_list()

            Show
            Marina Glancy added a comment - Here is the upgrade screenshot. Strings are missing and plugins are listed as "Add-ons" instead of "Standard" - see core_plugin_manager::standard_plugins_list()
            Hide
            Marina Glancy added a comment -

            Also strings missing for the new plugin type (lang/en/plugin.php), as well as settings page for enabling/disabling them

            $string['type_availability'] = 'Availability restriction';
            $string['type_availability_plural'] = 'Availability restrictions';
            

            See Administration>Plugins>Plugins overview

            Show
            Marina Glancy added a comment - Also strings missing for the new plugin type (lang/en/plugin.php), as well as settings page for enabling/disabling them $string['type_availability'] = 'Availability restriction'; $string['type_availability_plural'] = 'Availability restrictions'; See Administration>Plugins>Plugins overview
            Hide
            Marina Glancy added a comment -

            Leaving the review to Dan now

            Show
            Marina Glancy added a comment - Leaving the review to Dan now
            Hide
            Dan Poltawski added a comment - - edited

            (Sorry, I will try and group up the comments a bit rather than drip feed them, but i'm worried i'll loose these stack traces, so posting now).

            I've seen phpunit failures (OSX/Postgres):

            2) availability_date_condition_testcase::test_in_tree
            Failed asserting that 'Available from <strong>18 February 2014, 7:20 pm</strong>' matches PCRE pattern "~from.*18 February 2014, 7:20 PM~".
             
            /Users/danp/moodles/pm/moodle/availability/condition/date/tests/condition_test.php:74
            /Users/danp/moodles/pm/moodle/lib/phpunit/classes/advanced_testcase.php:80
             
            To re-run:
             vendor/bin/phpunit availability_date_condition_testcase availability/condition/date/tests/condition_test.php
             
            3) availability_date_condition_testcase::test_get_description
            Failed asserting that 'It is after <strong>18 February 2014, 2:55 pm</strong>' matches PCRE pattern "~after.*18 February 2014, 2:55 PM~".
             
            /Users/danp/moodles/pm/moodle/availability/condition/date/tests/condition_test.php:188
            /Users/danp/moodles/pm/moodle/lib/phpunit/classes/advanced_testcase.php:80
             
            To re-run:
             vendor/bin/phpunit availability_date_condition_testcase availability/condition/date/tests/condition_test.php
            

            Behat too fails in two ways in my runs so far:

            1. There is a memory exhaustion problem ( https://gist.github.com/danpoltawski/dde6484012295ce26624 ). Could be completely unrelated to this change - its gonna be hard to debug this because its killing the behat executable and likely exposing a bug in the framework.. Or it might be transient, going to run the whole suite again and Raj is having a look. For info the apache log suggests the last thing hit was forum post page after posting.
            2. This is the first error on availability (using --stop-on-failure because of the above issue).

              01. Exception thrown by (//html/.//a[./@href][(((./@id = 'C1' or contains(normalize-space(string(.)), 'C1')) or contains(./@title, 'C1') or contains(./@rel, 'C1')) or .//img[contains(./@alt, 'C1')])] | .//*[./@role = 'link'][((./@id = 'C1' or contains(./@value, 'C1')) or contains(./@title, 'C1') or contains(normalize-space(string(.)), 'C1'))])[1]
                  unknown error: Element is not clickable at point (325, 8). Other element would receive the click: <div class="container-fluid">...</div>
                    (Session info: chrome=33.0.1750.152)
                    (Driver info: chromedriver=2.8.241036,platform=Mac OS X 10.9.2 x86_64) (WARNING: The server did not provide any stacktrace information)
                  Command duration or timeout: 18 milliseconds
                  Build info: version: '2.39.0', revision: 'ff23eac', time: '2013-12-16 16:11:15'
                  System info: host: 'bart.local', ip: '192.168.100.17', os.name: 'Mac OS X', os.arch: 'x86_64', os.version: '10.9.2', java.version: '1.6.0_65'
                  Session ID: dfe937d55009bd0ad00368930d4e6fa3
                  Driver info: org.openqa.selenium.chrome.ChromeDriver
                  Capabilities [{platform=MAC, acceptSslCerts=true, javascriptEnabled=true, browserName=chrome, chrome={userDataDir=/var/folders/y9/tr0d97bs7jjcdh12jcqbs5dr0000gn/T/.org.chromium.Chromium.dj9l7w}, rotatable=false, locationContextEnabled=true, version=33.0.1750.152, takesHeapSnapshot=true, cssSelectorsEnabled=true, databaseEnabled=false, handlesAlerts=true, browserConnectionEnabled=false, nativeEvents=true, webStorageEnabled=true, applicationCacheEnabled=false, takesScreenshot=true}]
                  In step `And I follow "C1"'.     # behat_general::click_link()
                  From scenario `Test condition'.  # /Users/danp/moodles/pm_behating/moodle/availability/condition/group/tests/behat/availability_group.feature:25
                  Of feature `availability_group'. # /Users/danp/moodles/pm_behating/moodle/availability/condition/group/tests/behat/availability_group.feature
              

            Show
            Dan Poltawski added a comment - - edited (Sorry, I will try and group up the comments a bit rather than drip feed them, but i'm worried i'll loose these stack traces, so posting now). I've seen phpunit failures (OSX/Postgres): 2) availability_date_condition_testcase::test_in_tree Failed asserting that 'Available from <strong>18 February 2014, 7:20 pm</strong>' matches PCRE pattern "~from.*18 February 2014, 7:20 PM~".   /Users/danp/moodles/pm/moodle/availability/condition/date/tests/condition_test.php:74 /Users/danp/moodles/pm/moodle/lib/phpunit/classes/advanced_testcase.php:80   To re-run: vendor/bin/phpunit availability_date_condition_testcase availability/condition/date/tests/condition_test.php   3) availability_date_condition_testcase::test_get_description Failed asserting that 'It is after <strong>18 February 2014, 2:55 pm</strong>' matches PCRE pattern "~after.*18 February 2014, 2:55 PM~".   /Users/danp/moodles/pm/moodle/availability/condition/date/tests/condition_test.php:188 /Users/danp/moodles/pm/moodle/lib/phpunit/classes/advanced_testcase.php:80   To re-run: vendor/bin/phpunit availability_date_condition_testcase availability/condition/date/tests/condition_test.php Behat too fails in two ways in my runs so far: There is a memory exhaustion problem ( https://gist.github.com/danpoltawski/dde6484012295ce26624 ). Could be completely unrelated to this change - its gonna be hard to debug this because its killing the behat executable and likely exposing a bug in the framework.. Or it might be transient, going to run the whole suite again and Raj is having a look. For info the apache log suggests the last thing hit was forum post page after posting. This is the first error on availability (using --stop-on-failure because of the above issue). 01. Exception thrown by (//html/.//a[./@href][(((./@id = 'C1' or contains(normalize-space(string(.)), 'C1')) or contains(./@title, 'C1') or contains(./@rel, 'C1')) or .//img[contains(./@alt, 'C1')])] | .//*[./@role = 'link'][((./@id = 'C1' or contains(./@value, 'C1')) or contains(./@title, 'C1') or contains(normalize-space(string(.)), 'C1'))])[1] unknown error: Element is not clickable at point (325, 8). Other element would receive the click: <div class="container-fluid">...</div> (Session info: chrome=33.0.1750.152) (Driver info: chromedriver=2.8.241036,platform=Mac OS X 10.9.2 x86_64) (WARNING: The server did not provide any stacktrace information) Command duration or timeout: 18 milliseconds Build info: version: '2.39.0', revision: 'ff23eac', time: '2013-12-16 16:11:15' System info: host: 'bart.local', ip: '192.168.100.17', os.name: 'Mac OS X', os.arch: 'x86_64', os.version: '10.9.2', java.version: '1.6.0_65' Session ID: dfe937d55009bd0ad00368930d4e6fa3 Driver info: org.openqa.selenium.chrome.ChromeDriver Capabilities [{platform=MAC, acceptSslCerts=true, javascriptEnabled=true, browserName=chrome, chrome={userDataDir=/var/folders/y9/tr0d97bs7jjcdh12jcqbs5dr0000gn/T/.org.chromium.Chromium.dj9l7w}, rotatable=false, locationContextEnabled=true, version=33.0.1750.152, takesHeapSnapshot=true, cssSelectorsEnabled=true, databaseEnabled=false, handlesAlerts=true, browserConnectionEnabled=false, nativeEvents=true, webStorageEnabled=true, applicationCacheEnabled=false, takesScreenshot=true}] In step `And I follow "C1"'. # behat_general::click_link() From scenario `Test condition'. # /Users/danp/moodles/pm_behating/moodle/availability/condition/group/tests/behat/availability_group.feature:25 Of feature `availability_group'. # /Users/danp/moodles/pm_behating/moodle/availability/condition/group/tests/behat/availability_group.feature
            Hide
            Dan Poltawski added a comment - - edited

            Urgh, i'm not getting very far because I am spending time on testing things. It seems like there is a phpunit reset problem triggered when I test against this branch on mssql and oracle.

            15:30:13 Eloy Lafuente: and, about the date failures. the mac is your culprit, lol. it's unable to do upper am/pm 

            We've fixed that in two ways in the past: 9be8583ac557835e19a6eba4e94ef037dc675a39 and e03ed0e1341b3abea9f035d6cd245e329b1cee91

            Show
            Dan Poltawski added a comment - - edited Urgh, i'm not getting very far because I am spending time on testing things. It seems like there is a phpunit reset problem triggered when I test against this branch on mssql and oracle. 15:30:13 Eloy Lafuente: and, about the date failures. the mac is your culprit, lol. it's unable to do upper am/pm  We've fixed that in two ways in the past: 9be8583ac557835e19a6eba4e94ef037dc675a39 and e03ed0e1341b3abea9f035d6cd245e329b1cee91
            Hide
            Dan Poltawski added a comment -

            I'm stopping peer review of this issue, because I am not going to complete it today and want it available for other volunteers.

            (I will continue to add comments)

            Show
            Dan Poltawski added a comment - I'm stopping peer review of this issue, because I am not going to complete it today and want it available for other volunteers. (I will continue to add comments)
            Hide
            Dan Poltawski added a comment -

            Sorry Sam for starting and stopping this. My head has not been in the right space for this today, this list of things is trivial and not really considering the wider picture, but:

            • I find the manually writing and searching of json a bit strange, but I suppose this is about performance.
            • core_availability\capability_checker looks like its using static caches without reset
            • \core_availability\info_module::is_user_visible() is mistakenly using $cm
            • \core_availability\info is using debugging() calls that need to be DEBUG_DEVELOPER
            • I have to say I find the naming of 'core_availability::node' not quite natural, however I don't think there is anything inherently wrong with it
            • asort -> core_collator::asort
            Show
            Dan Poltawski added a comment - Sorry Sam for starting and stopping this. My head has not been in the right space for this today, this list of things is trivial and not really considering the wider picture, but: I find the manually writing and searching of json a bit strange, but I suppose this is about performance. core_availability\capability_checker looks like its using static caches without reset \core_availability\info_module::is_user_visible() is mistakenly using $cm \core_availability\info is using debugging() calls that need to be DEBUG_DEVELOPER I have to say I find the naming of 'core_availability::node' not quite natural, however I don't think there is anything inherently wrong with it asort -> core_collator::asort
            Show
            CiBoT added a comment - Results for MDL-44070 Remote repository: https://github.com/sammarshallou/moodle.git Remote branch MDL-44070 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2717 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2717/artifact/work/smurf.html
            Hide
            Sam Marshall added a comment -

            Thank you for review comments! I'm investigating these now and will update when done.

            Show
            Sam Marshall added a comment - Thank you for review comments! I'm investigating these now and will update when done.
            Hide
            Sam Marshall added a comment -

            (Sorry for several updates to test instructions. There were some minor errors in the instructions which I only spotted while going through.)

            I've addressed all the review comments (these are listed basically in the order they were above):

            FIXED - Behat step regex (actually in MDL-44891):
            Assume you mean there should be an indication of which of these are selectors etc, rather than just using the output straight from Behat. I didn't spot this before.

            FIXED - Deprecated global functions in wrong place:

            • Moved coursemodule_visible_for_user from datalib -> deprecatedlib
            • This is the only global function I deprecated, the others are class functions

            FIXED - Right-to-left support:
            You are right, I didn't think to test in an RTL language. I've now tested in Hebrew (not that I know Hebrew of course) and added rules flipping the CSS in both themes.

            NO CHANGE - availability_group namespace and package name:
            This is correct because of new plugin type/prefix availability_. (I checked, doesn't seem to be used anywhere it shouldn't be. Note: I think cibot will tell you if you use an invalid package name, at least in some cases)

            FIXED - rebuild_course_cache in upgrade:
            Presumably this is no longer needed in upgrade now that course cache is in MUC,. I've removed it.

            FIXED - version bump and redirect_if_major_upgrade_required
            I had avoided doing the version bump because it makes rebasing every week even more annoying - and I didn't actually know about function. Have done version bump and changed version in function.

            FIXED - Missing change to core_plugin_manager::standard_plugins_list

            FIXED - Missing strings pluginname on some of the availability plugins
            As well as adding the missing string, I also standardised the format of these strings and made it a bit shorter.

            FIXED - Missing strings for plugin type - lang/en/plugin.php
            Thanks Marina for writing the strings btw!

            FIXED - Missing settings page to enable/disable plugins
            This was the largest code change. I have added a settings page with enable/disable feature. (Also tested that it works. Seems to!)

            FIXED - Mac PHPunit failures with 'PM'
            I did a search and these strings only appear in the PHPunit test. They were all in regex checks which I have changed to allow either case, with a comment about the Mac bug.

            FIXED - Behat failures in Chrome
            This occurs in Chrome but not in Firefox. I had not tested in Chrome previously (seems unreasonably difficult to change browser in Behat, incidentally).
            1. For some reason, at a particular point it tries to click the course shortname 'C1' in breadcrumbs but Chrome thinks it is not clickable. I have changed it to go to the course page via the homepage instead in the 2 cases where this caused a problem. (Note: There are other places where it clicks C1 in breadcrumbs and works fine.)
            2. In another test the 'I add a page to section' step failed. I was able to workaround this by going back to homepage and course page again. Have added MDL-44959 about this issue so that it can ideally be fixed properly later.
            The --tags=core_availability,availability Behat test now completely passes in Chrome as well as Firefox.

            NO CHANGE - Behat out of memory error
            This is either not related to my changes (I'm betting), or is a bug in Behat system. Running only my features "--tags=availability,core_availability" it works successfully, and features are supposed to be handled independently.

            NO CHANGE - Manually writing/searching JSON:
            Think this is okay - JSON strings are created manually (a) in test scripts, (b) in the upgrade of legacy data, and (c) in restore when updating legacy data. But the main API is all using json_encode/decode in PHP and equivalents in YUI, and not doing it manually.

            NO CHANGE - core_availability\capability_checker looks like its using static caches without reset
            This is not a static cache - it's an ordinary member variable, and members of the class are not kept in static variables either, so I think it's okay. The cache is only used within 'who can access this' availability checks on an item (basically to stop it having to do two get_users_by_capability calls for accessallgroups, if there are group conditions like 'belongs to group X OR to group Y')

            FIXED - \core_availability\info_module::is_user_visible() is mistakenly using $cm
            Oops - this case was supposed to be covered by phpunit but the code didn't actually run because of a missing backslash.

            NO CHANGE - \core_availability\info is using debugging() calls that need to be DEBUG_DEVELOPER
            These are intentionally visible under lower debugging levels because they indicate that the availability condition is not working and you are going to need to edit the settings for the activity to fix it. Shouldn't really happen but if it does, would be worth displaying to ordinary users as it indicates a real problem that something isn't working.

            NO CHANGE - Dan said 'I have to say I find the naming of 'core_availability::node' not quite natural, however I don't think there is anything inherently wrong with it'
            If anyone thinks of a better name I'll rename it, but I think 'node' (for a node in a tree) is good enough.

            FIXED - asort -> core_collator::asort
            Didn't know about this before. Comedy fact: while fixing this I searched all my commits for asort and found a bunch from the old code I am deleting; of these, 6 out of 7 were not using the core_collator version....

            In addition to these review changes there are some things I did/spotted:

            1. I made a fresh Moodle install with the weekly build, and retested the full upgrade (I had done this before but probably in pieces which is why I didn't spot the problems Marina found). This includes going through the test instructions for this issue again since those are about testing the upgrade (they worked already, though).

            2. FIXED - There was a bug with setting up the 'until' dropdown for a date condition when you edit activity.

            3. I reran Behat tests for this area on both Chrome and Firefox (--tags=availability,core_availability). Cannot run full Behat tests as no time.

            I rebased all the changes into the existing commits and pushed it again.

            Show
            Sam Marshall added a comment - (Sorry for several updates to test instructions. There were some minor errors in the instructions which I only spotted while going through.) I've addressed all the review comments (these are listed basically in the order they were above): FIXED - Behat step regex (actually in MDL-44891 ): Assume you mean there should be an indication of which of these are selectors etc, rather than just using the output straight from Behat. I didn't spot this before. FIXED - Deprecated global functions in wrong place: Moved coursemodule_visible_for_user from datalib -> deprecatedlib This is the only global function I deprecated, the others are class functions FIXED - Right-to-left support: You are right, I didn't think to test in an RTL language. I've now tested in Hebrew (not that I know Hebrew of course) and added rules flipping the CSS in both themes. NO CHANGE - availability_group namespace and package name: This is correct because of new plugin type/prefix availability_. (I checked, doesn't seem to be used anywhere it shouldn't be. Note: I think cibot will tell you if you use an invalid package name, at least in some cases) FIXED - rebuild_course_cache in upgrade: Presumably this is no longer needed in upgrade now that course cache is in MUC,. I've removed it. FIXED - version bump and redirect_if_major_upgrade_required I had avoided doing the version bump because it makes rebasing every week even more annoying - and I didn't actually know about function. Have done version bump and changed version in function. FIXED - Missing change to core_plugin_manager::standard_plugins_list FIXED - Missing strings pluginname on some of the availability plugins As well as adding the missing string, I also standardised the format of these strings and made it a bit shorter. FIXED - Missing strings for plugin type - lang/en/plugin.php Thanks Marina for writing the strings btw! FIXED - Missing settings page to enable/disable plugins This was the largest code change. I have added a settings page with enable/disable feature. (Also tested that it works. Seems to!) FIXED - Mac PHPunit failures with 'PM' I did a search and these strings only appear in the PHPunit test. They were all in regex checks which I have changed to allow either case, with a comment about the Mac bug. FIXED - Behat failures in Chrome This occurs in Chrome but not in Firefox. I had not tested in Chrome previously (seems unreasonably difficult to change browser in Behat, incidentally). 1. For some reason, at a particular point it tries to click the course shortname 'C1' in breadcrumbs but Chrome thinks it is not clickable. I have changed it to go to the course page via the homepage instead in the 2 cases where this caused a problem. (Note: There are other places where it clicks C1 in breadcrumbs and works fine.) 2. In another test the 'I add a page to section' step failed. I was able to workaround this by going back to homepage and course page again. Have added MDL-44959 about this issue so that it can ideally be fixed properly later. The --tags=core_availability,availability Behat test now completely passes in Chrome as well as Firefox. NO CHANGE - Behat out of memory error This is either not related to my changes (I'm betting), or is a bug in Behat system. Running only my features "--tags=availability,core_availability" it works successfully, and features are supposed to be handled independently. NO CHANGE - Manually writing/searching JSON: Think this is okay - JSON strings are created manually (a) in test scripts, (b) in the upgrade of legacy data, and (c) in restore when updating legacy data. But the main API is all using json_encode/decode in PHP and equivalents in YUI, and not doing it manually. NO CHANGE - core_availability\capability_checker looks like its using static caches without reset This is not a static cache - it's an ordinary member variable, and members of the class are not kept in static variables either, so I think it's okay. The cache is only used within 'who can access this' availability checks on an item (basically to stop it having to do two get_users_by_capability calls for accessallgroups, if there are group conditions like 'belongs to group X OR to group Y') FIXED - \core_availability\info_module::is_user_visible() is mistakenly using $cm Oops - this case was supposed to be covered by phpunit but the code didn't actually run because of a missing backslash. NO CHANGE - \core_availability\info is using debugging() calls that need to be DEBUG_DEVELOPER These are intentionally visible under lower debugging levels because they indicate that the availability condition is not working and you are going to need to edit the settings for the activity to fix it. Shouldn't really happen but if it does, would be worth displaying to ordinary users as it indicates a real problem that something isn't working. NO CHANGE - Dan said 'I have to say I find the naming of 'core_availability::node' not quite natural, however I don't think there is anything inherently wrong with it' If anyone thinks of a better name I'll rename it, but I think 'node' (for a node in a tree) is good enough. FIXED - asort -> core_collator::asort Didn't know about this before. Comedy fact: while fixing this I searched all my commits for asort and found a bunch from the old code I am deleting; of these, 6 out of 7 were not using the core_collator version.... In addition to these review changes there are some things I did/spotted: 1. I made a fresh Moodle install with the weekly build, and retested the full upgrade (I had done this before but probably in pieces which is why I didn't spot the problems Marina found). This includes going through the test instructions for this issue again since those are about testing the upgrade (they worked already, though). 2. FIXED - There was a bug with setting up the 'until' dropdown for a date condition when you edit activity. 3. I reran Behat tests for this area on both Chrome and Firefox (--tags=availability,core_availability). Cannot run full Behat tests as no time. I rebased all the changes into the existing commits and pushed it again.
            Hide
            Sam Marshall added a comment -

            I'm submitting this for integration since I've addressed all the review comments, and so that it doesn't fall off the radar just yet. However, I do understand that the review wasn't really finished, so if it needs to be removed from integration for that reason, then OK.

            I normally don't work weekends but in case anyone else does, I'll try to check in to this tracker issue once each day and look through any further review comments.

            If integration goes ahead, I'll also be available all next week to work/assist with any necessary fixes. (I've got local work to do as well, but nothing urgent, so this will be my top priority.)

            Show
            Sam Marshall added a comment - I'm submitting this for integration since I've addressed all the review comments, and so that it doesn't fall off the radar just yet. However, I do understand that the review wasn't really finished, so if it needs to be removed from integration for that reason, then OK. I normally don't work weekends but in case anyone else does, I'll try to check in to this tracker issue once each day and look through any further review comments. If integration goes ahead, I'll also be available all next week to work/assist with any necessary fixes. (I've got local work to do as well, but nothing urgent, so this will be my top priority.)
            Hide
            CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            Damyon Wiese added a comment -

            Thanks Sam,

            This is a huge patch, and obviously you have put heaps of work into this new system - thanks!

            I have had a good look at this now - but haven't referenced the above comments to see if there are more issues that need fixing. This is the list of issues I found (coded with letters so they are separate to any comments above):

            Blocker issues:

            (A) We are trying not to add new admin pages directly - instead we are adding admin tools so they can be uninstalled/replaced if required. So admin/availabilityconditions.php should all be in a new tool "admin/tool/availabilityconditions".

            (B) This API needs docs on http://docs.moodle.org/dev/Core_APIs. (See lock which was added for 2.7)

            (C) Calling "$PAGE->requires->yui_module(...)" for each plugin is a slow way to setup the page because it prevents combo loading. In Atto we did this by passing an array of modules to $PAGE->requires->yui_module() along with the init params for each plugin - then calling the init function for each plugin from the init function for Atto. This change made a huge difference to page load times. (See Atto for more info).

            (D) All the strings "requires_0|1|2|3" etc need better names (for translators).

            (E) Please do this: (you have these still in the code)
            // TODO Update this to release version.

            (F) Whitespace error in "availability/tests/behat/edit_availability.feature"

            (G) Upgrade step does not handle 0 things to update:

            Warning: Division by zero in /home/damyonw/Documents/Moodle/integration/master/moodle/lib/weblib.php on line 3103
            

            (H) Restoring an activity with a condition set to require 50% for "Course Total" results in broken condition with "Not available unless: You achieve a required score in (missing activity)".

            Should fix issues:

            (I) /availability/tests/mock_.php should be in /availability/tests/fixtures/mock_.php

            (J) Inline styles should not be added by javascript - add a class and put the styles in the plugin styles.css. (style="width: 3em")

            (K) availability/condition/grade does not work with scales (but neither did the old system - there should be a follow-up issue for this at least).

            (L) There are lots of event handlers added by the plugins that should use delegate instead of 'on' (super low priority)

            (M) Core should not be directly calling functions from plugins - even if it is conditional. This should be designed around:

            +                        class_exists('availability_completion\condition') &&
            +                        availability_completion\condition::completion_value_used
            

            (N) Is there a follow up issue to remove deprecated functions in 2.9 ?

            That's all.

            I'm leaving this in the review state while I wait for fixes. If the blockers are fixed and behat + phpunit is passing - this looks ready to me.

            Show
            Damyon Wiese added a comment - Thanks Sam, This is a huge patch, and obviously you have put heaps of work into this new system - thanks! I have had a good look at this now - but haven't referenced the above comments to see if there are more issues that need fixing. This is the list of issues I found (coded with letters so they are separate to any comments above): Blocker issues: (A) We are trying not to add new admin pages directly - instead we are adding admin tools so they can be uninstalled/replaced if required. So admin/availabilityconditions.php should all be in a new tool "admin/tool/availabilityconditions". (B) This API needs docs on http://docs.moodle.org/dev/Core_APIs . (See lock which was added for 2.7) (C) Calling "$PAGE->requires->yui_module(...)" for each plugin is a slow way to setup the page because it prevents combo loading. In Atto we did this by passing an array of modules to $PAGE->requires->yui_module() along with the init params for each plugin - then calling the init function for each plugin from the init function for Atto. This change made a huge difference to page load times. (See Atto for more info). (D) All the strings "requires_0|1|2|3" etc need better names (for translators). (E) Please do this: (you have these still in the code) // TODO Update this to release version. (F) Whitespace error in "availability/tests/behat/edit_availability.feature" (G) Upgrade step does not handle 0 things to update: Warning: Division by zero in /home/damyonw/Documents/Moodle/integration/master/moodle/lib/weblib.php on line 3103 (H) Restoring an activity with a condition set to require 50% for "Course Total" results in broken condition with "Not available unless: You achieve a required score in (missing activity)". Should fix issues: (I) /availability/tests/mock_ .php should be in /availability/tests/fixtures/mock_ .php (J) Inline styles should not be added by javascript - add a class and put the styles in the plugin styles.css. (style="width: 3em") (K) availability/condition/grade does not work with scales (but neither did the old system - there should be a follow-up issue for this at least). (L) There are lots of event handlers added by the plugins that should use delegate instead of 'on' (super low priority) (M) Core should not be directly calling functions from plugins - even if it is conditional. This should be designed around: + class_exists('availability_completion\condition') && + availability_completion\condition::completion_value_used (N) Is there a follow up issue to remove deprecated functions in 2.9 ? That's all. I'm leaving this in the review state while I wait for fixes. If the blockers are fixed and behat + phpunit is passing - this looks ready to me.
            Hide
            Damyon Wiese added a comment -

            Some distracting further comments to some discussions above:

            I think tree_node is a better name than just node here. Thats subjective - so do whatever.

            "These are intentionally visible under lower debugging levels because they indicate that the availability condition is not working and you are going to need to edit the settings for the activity to fix it." - if they are informative make them real strings. Showing english debugging messages to real users is sloppy.

            Show
            Damyon Wiese added a comment - Some distracting further comments to some discussions above: I think tree_node is a better name than just node here. Thats subjective - so do whatever. "These are intentionally visible under lower debugging levels because they indicate that the availability condition is not working and you are going to need to edit the settings for the activity to fix it." - if they are informative make them real strings. Showing english debugging messages to real users is sloppy.
            Hide
            Sam Marshall added a comment -

            Thanks for these review comments! I have addressed all of them, as follows:

            FIXED - (A) Should not add new admin pages directly, use tool instead.
            Have moved code into admin tool as suggested.

            FIXED - (B) Needs docs on http://docs.moodle.org/dev/Core_APIs.
            Have added doc page for the overall API, and finished the existing page about how to write a new plugin.
            http://docs.moodle.org/dev/Availability_API
            http://docs.moodle.org/dev/Availability_conditions

            FIXED - (C) Calling "$PAGE->requires->yui_module(...)" for each plugin is slow, should use single call
            Changed as suggested.

            FIXED - (D) Strings "requires_0|1|2|3" etc need better names (for translators).
            This was the same situation as the strings in the previous system, however there is no reason not to make it better along with the new system, so I've made it use textual strings.

            FIXED - (E) Update 'requires' version number in plugins to current
            I updated them all to the version.php value from my branch.

            FIXED - (F) Whitespace error (tabs) in "availability/tests/behat/edit_availability.feature"
            Have got rid of the tabs.

            FIXED - (G) Upgrade step does not handle 0 things to update
            Ooops! Sorry. I added an if statement.

            FIXED - (H) Restoring single activity with grade condition for "Course Total" results in broken condition
            (Note: The report wasn't specific, but this problem occurs if you backup and restore a single activity rather than the whole course. It also occurs if you use 'Duplicate' option.)

            This behaviour is expected when restoring to a different course. The activity backup does not include the grade, so if you backup an activity that relies on any grade on the course - not just 'course total', anything - this will lose the reference to grades. Existing 2.6 behaviour is be the same - if you back up an individual activity with dependencies and do not include the dependencies, the dependencies will be lost in the restored activity.

            However it ought to work when restoring to the same course. This also applies to completion and group/grouping dependencies. I've added a new phpunit test for the 'duplicate' case (in the backup commit) which verifies that this works as expected.

            FIXED - (I) /availability/tests/mock_.php should be in /availability/tests/fixtures/mock_.php
            I moved these files. Also moved a fixture (data file) I had added in a backup unit test, similarly.

            FIXED - (J) Inline styles should not be added by javascript
            As suggested I put styles in styles.css. I also changed the JS so that it automatically adds a suitable class (frankenstyle name) to the .availability-plugincontrols DIV, which meant that I didn't need to add individual classes to the grade inputs.

            ISSUE ADDED - (K) availability/condition/grade does not work with scales (but neither did the old system).
            I've created a separate issue MDL-44983; as this works the same way in the old system I don't believe it is necessary as part of this change, however it is clearly an issue with the system which should ideally be improved at some point.

            FIXED - (L) Plugin event handlers should use delegate instead of 'on'
            I have changed it so all the plugin events are delegates on the single field element. (Note: I didn't change the events in the core JS as those are a bit more complicated, but there should probably be fewer of them. Can be changed later if required.)

            FIXED - (M) Core should not be directly calling functions from plugins (completion_value_used)
            I have added this function to \core_availability\info instead, it now gives all plugins a chance to respond to this (in practice I doubt any would need to, but I agree it is bad practice to directly call plugin anyhow).

            ISSUE ADDED - (N) Is there a follow up issue to remove deprecated functions in 2.9 ?
            There wasn't, sorrry - I now added MDL-44985 which lists everything deprecated.

            FIXED - Rename node to tree_node.
            Agree, tree_node is fine, have renamed it.

            FIXED - Debugging message that appear to real users should use translated strings
            OK, on considering this I have changed them to developer warning level (as Dan originally suggested), since they should not occur if the code (JavaScript + PHP) is working correctly.

            Other issues fixed:

            FIXED - Due to incorrect HTML code, in Firefox when you clicked on the 'must be complete' dropdown it focused a different dropdown. I corrected the code (missing </label>).

            FIXED - Some of the deprecation warnings in modinfolib had the wrong text (the ones for section referred to cm).

            FIXED - The one Behat scenario that doesn't require JavaScript was failing after I did 'behat init' and it updated composer and dependencies, so this is probably due to a bug in those systems. There was an error 'Notice: Undefined index: host in [...]\vendor\symfony\browser-kit\Symfony\Component\BrowserKit\CookieJar.php line 217' - I fixed this by temporarily changing one scenario that didn't need javascript to add the @javascript tag, this fixes the problem. There is a comment documenting this. If the Behat thing is resolved then this can be changed back later.

            Retesting:

            PHPunit: Rerun (entire system) - passed
            Behat: Rerun (--tags=availability,core_availability, Firefox) - passed
            Upgrade: I retested the upgrade from weekly master, this time from an empty site (to check the case with 0 items)

            Note:

            Not sure if I mentioned it already, but the 10 'thematic' commits here won't result in a working Moodle system between two commits. If that is a concern, it might be best to squash it into a single commit before actually integrating.

            Show
            Sam Marshall added a comment - Thanks for these review comments! I have addressed all of them, as follows: FIXED - (A) Should not add new admin pages directly, use tool instead. Have moved code into admin tool as suggested. FIXED - (B) Needs docs on http://docs.moodle.org/dev/Core_APIs . Have added doc page for the overall API, and finished the existing page about how to write a new plugin. http://docs.moodle.org/dev/Availability_API http://docs.moodle.org/dev/Availability_conditions FIXED - (C) Calling "$PAGE->requires->yui_module(...)" for each plugin is slow, should use single call Changed as suggested. FIXED - (D) Strings "requires_0|1|2|3" etc need better names (for translators). This was the same situation as the strings in the previous system, however there is no reason not to make it better along with the new system, so I've made it use textual strings. FIXED - (E) Update 'requires' version number in plugins to current I updated them all to the version.php value from my branch. FIXED - (F) Whitespace error (tabs) in "availability/tests/behat/edit_availability.feature" Have got rid of the tabs. FIXED - (G) Upgrade step does not handle 0 things to update Ooops! Sorry. I added an if statement. FIXED - (H) Restoring single activity with grade condition for "Course Total" results in broken condition (Note: The report wasn't specific, but this problem occurs if you backup and restore a single activity rather than the whole course. It also occurs if you use 'Duplicate' option.) This behaviour is expected when restoring to a different course. The activity backup does not include the grade, so if you backup an activity that relies on any grade on the course - not just 'course total', anything - this will lose the reference to grades. Existing 2.6 behaviour is be the same - if you back up an individual activity with dependencies and do not include the dependencies, the dependencies will be lost in the restored activity. However it ought to work when restoring to the same course. This also applies to completion and group/grouping dependencies. I've added a new phpunit test for the 'duplicate' case (in the backup commit) which verifies that this works as expected. FIXED - (I) /availability/tests/mock_.php should be in /availability/tests/fixtures/mock_.php I moved these files. Also moved a fixture (data file) I had added in a backup unit test, similarly. FIXED - (J) Inline styles should not be added by javascript As suggested I put styles in styles.css. I also changed the JS so that it automatically adds a suitable class (frankenstyle name) to the .availability-plugincontrols DIV, which meant that I didn't need to add individual classes to the grade inputs. ISSUE ADDED - (K) availability/condition/grade does not work with scales (but neither did the old system). I've created a separate issue MDL-44983 ; as this works the same way in the old system I don't believe it is necessary as part of this change, however it is clearly an issue with the system which should ideally be improved at some point. FIXED - (L) Plugin event handlers should use delegate instead of 'on' I have changed it so all the plugin events are delegates on the single field element. (Note: I didn't change the events in the core JS as those are a bit more complicated, but there should probably be fewer of them. Can be changed later if required.) FIXED - (M) Core should not be directly calling functions from plugins (completion_value_used) I have added this function to \core_availability\info instead, it now gives all plugins a chance to respond to this (in practice I doubt any would need to, but I agree it is bad practice to directly call plugin anyhow). ISSUE ADDED - (N) Is there a follow up issue to remove deprecated functions in 2.9 ? There wasn't, sorrry - I now added MDL-44985 which lists everything deprecated. FIXED - Rename node to tree_node. Agree, tree_node is fine, have renamed it. FIXED - Debugging message that appear to real users should use translated strings OK, on considering this I have changed them to developer warning level (as Dan originally suggested), since they should not occur if the code (JavaScript + PHP) is working correctly. Other issues fixed: FIXED - Due to incorrect HTML code, in Firefox when you clicked on the 'must be complete' dropdown it focused a different dropdown. I corrected the code (missing </label>). FIXED - Some of the deprecation warnings in modinfolib had the wrong text (the ones for section referred to cm). FIXED - The one Behat scenario that doesn't require JavaScript was failing after I did 'behat init' and it updated composer and dependencies, so this is probably due to a bug in those systems. There was an error 'Notice: Undefined index: host in [...] \vendor\symfony\browser-kit\Symfony\Component\BrowserKit\CookieJar.php line 217' - I fixed this by temporarily changing one scenario that didn't need javascript to add the @javascript tag, this fixes the problem. There is a comment documenting this. If the Behat thing is resolved then this can be changed back later. Retesting: PHPunit: Rerun (entire system) - passed Behat: Rerun (--tags=availability,core_availability, Firefox) - passed Upgrade: I retested the upgrade from weekly master, this time from an empty site (to check the case with 0 items) Note: Not sure if I mentioned it already, but the 10 'thematic' commits here won't result in a working Moodle system between two commits. If that is a concern, it might be best to squash it into a single commit before actually integrating.
            Hide
            Damyon Wiese added a comment -

            Thanks Sam, the updates look perfect. I'm ready to push this as soon as MDL-44891 is sorted.

            I did notice one other tiny problem when checking the accessibility. Note: you have done a great job of keyboard support and hidden labels for inputs etc. There is just one tiny bug when tabbing through the restrictions: tabbing to the hide icon changes it from hide to show. I'll add a follow up issue for that when this is integrated if you haven't fixed it already by then.

            Show
            Damyon Wiese added a comment - Thanks Sam, the updates look perfect. I'm ready to push this as soon as MDL-44891 is sorted. I did notice one other tiny problem when checking the accessibility. Note: you have done a great job of keyboard support and hidden labels for inputs etc. There is just one tiny bug when tabbing through the restrictions: tabbing to the hide icon changes it from hide to show. I'll add a follow up issue for that when this is integrated if you haven't fixed it already by then.
            Hide
            Damyon Wiese added a comment -

            Also - don't worry about the upgrade path for the 10 separate commits - they will be collected by my merge commit.

            Show
            Damyon Wiese added a comment - Also - don't worry about the upgrade path for the 10 separate commits - they will be collected by my merge commit.
            Hide
            Marina Glancy added a comment -

            Please don't forget to create an issue under MDL-41330 with the list of all functions and methods that need to be removed (Damyon has mentioned this already above)

            Show
            Marina Glancy added a comment - Please don't forget to create an issue under MDL-41330 with the list of all functions and methods that need to be removed (Damyon has mentioned this already above)
            Hide
            Damyon Wiese added a comment -

            I converted MDL-44985 to a subtask of MDL-41330.

            Show
            Damyon Wiese added a comment - I converted MDL-44985 to a subtask of MDL-41330 .
            Hide
            Damyon Wiese added a comment -

            Thanks Sam,

            Integrated to master only. Really good job with this - well done!

            I'll add an issue for that keyboard nav thing now.

            Show
            Damyon Wiese added a comment - Thanks Sam, Integrated to master only. Really good job with this - well done! I'll add an issue for that keyboard nav thing now.
            Hide
            Damyon Wiese added a comment -

            Keyboard nav issue added and linked.

            Also I had to add a whitespace fix for: backup/moodle2/tests/moodle2_test.php

            Show
            Damyon Wiese added a comment - Keyboard nav issue added and linked. Also I had to add a whitespace fix for: backup/moodle2/tests/moodle2_test.php
            Hide
            Sam Marshall added a comment -

            Thanks for all your work on this too! Sorry about the whitespace issue and the keyboard problem, I should've noticed that one. (Fix submitted in other issue.)

            Show
            Sam Marshall added a comment - Thanks for all your work on this too! Sorry about the whitespace issue and the keyboard problem, I should've noticed that one. (Fix submitted in other issue.)
            Hide
            Michael de Raadt added a comment - - edited

            I ran into some errors when accessing the provided course after upgrading.

            This was produced twice before page output...

            Error processing availability data for ‘’: Missing or invalid ->t for date condition
             
                line 201 of \availability\classes\info.php: call to debugging()
                line 1766 of \lib\modinfolib.php: call to core_availability\info->is_available()
                line 1299 of \lib\modinfolib.php: call to cm_info->obtain_dynamic_data()
                line 1176 of \lib\modinfolib.php: call to cm_info->get_name()
                line 1857 of \lib\navigationlib.php: call to cm_info->__get()
                line 1896 of \lib\navigationlib.php: call to global_navigation->generate_sections_and_activities()
                line 443 of \course\format\lib.php: call to global_navigation->load_generic_course_sections()
                line 145 of \course\format\topics\lib.php: call to format_base->extend_course_navigation()
                line 1811 of \lib\navigationlib.php: call to format_topics->extend_course_navigation()
                line 1181 of \lib\navigationlib.php: call to global_navigation->load_course_sections()
                line 222 of \blocks\navigation\block_navigation.php: call to global_navigation->initialise()
                line 178 of \blocks\navigation\block_navigation.php: call to block_navigation->get_navigation()
                line 294 of \blocks\moodleblock.class.php: call to block_navigation->get_content()
                line 236 of \blocks\moodleblock.class.php: call to block_base->formatted_contents()
                line 988 of \lib\blocklib.php: call to block_base->get_content_for_output()
                line 1040 of \lib\blocklib.php: call to block_manager->create_block_contents()
                line 361 of \lib\outputrenderers.php: call to block_manager->ensure_content_created()
                line 45 of \theme\clean\layout\columns3.php: call to core_renderer->standard_head_html()
                line 877 of \lib\outputrenderers.php: call to include()
                line 807 of \lib\outputrenderers.php: call to core_renderer->render_page_layout()
                line 250 of \course\view.php: call to core_renderer->header()
            

            This appeared in the course, in the Topic 1 section...

            Debug info:
            Error code: codingerror
             
            Stack trace:
                line 71 of \availability\condition\date\classes\condition.php: coding_exception thrown
                line 234 of \availability\classes\tree.php: call to availability_date\condition->__construct()
                line 141 of \availability\classes\info.php: call to core_availability\tree->__construct()
                line 111 of \availability\classes\info.php: call to core_availability\info->decode_availability()
                line 233 of \availability\condition\completion\classes\condition.php: call to core_availability\info->get_availability_tree()
                line 622 of \availability\classes\info.php: call to availability_completion\condition::completion_value_used()
                line 668 of \course\renderer.php: call to core_availability\info::completion_value_used()
                line 1021 of \course\renderer.php: call to core_course_renderer->course_section_cm_completion()
                line 919 of \course\renderer.php: call to core_course_renderer->course_section_cm()
                line 1088 of \course\renderer.php: call to core_course_renderer->course_section_cm_list_item()
                line 767 of \course\format\renderer.php: call to core_course_renderer->course_section_cm_list()
                line 56 of \course\format\topics\format.php: call to format_section_renderer_base->print_multiple_section_page()
                line 286 of \course\view.php: call to require()
            

            The error appeared on subsequent visits to the course.

            As the activities weren't presented, I wasn't able to check that the availability and conditional access setting were upgraded.

            Show
            Michael de Raadt added a comment - - edited I ran into some errors when accessing the provided course after upgrading. This was produced twice before page output... Error processing availability data for ‘’: Missing or invalid ->t for date condition   line 201 of \availability\classes\info.php: call to debugging() line 1766 of \lib\modinfolib.php: call to core_availability\info->is_available() line 1299 of \lib\modinfolib.php: call to cm_info->obtain_dynamic_data() line 1176 of \lib\modinfolib.php: call to cm_info->get_name() line 1857 of \lib\navigationlib.php: call to cm_info->__get() line 1896 of \lib\navigationlib.php: call to global_navigation->generate_sections_and_activities() line 443 of \course\format\lib.php: call to global_navigation->load_generic_course_sections() line 145 of \course\format\topics\lib.php: call to format_base->extend_course_navigation() line 1811 of \lib\navigationlib.php: call to format_topics->extend_course_navigation() line 1181 of \lib\navigationlib.php: call to global_navigation->load_course_sections() line 222 of \blocks\navigation\block_navigation.php: call to global_navigation->initialise() line 178 of \blocks\navigation\block_navigation.php: call to block_navigation->get_navigation() line 294 of \blocks\moodleblock.class.php: call to block_navigation->get_content() line 236 of \blocks\moodleblock.class.php: call to block_base->formatted_contents() line 988 of \lib\blocklib.php: call to block_base->get_content_for_output() line 1040 of \lib\blocklib.php: call to block_manager->create_block_contents() line 361 of \lib\outputrenderers.php: call to block_manager->ensure_content_created() line 45 of \theme\clean\layout\columns3.php: call to core_renderer->standard_head_html() line 877 of \lib\outputrenderers.php: call to include() line 807 of \lib\outputrenderers.php: call to core_renderer->render_page_layout() line 250 of \course\view.php: call to core_renderer->header() This appeared in the course, in the Topic 1 section... Debug info: Error code: codingerror   Stack trace: line 71 of \availability\condition\date\classes\condition.php: coding_exception thrown line 234 of \availability\classes\tree.php: call to availability_date\condition->__construct() line 141 of \availability\classes\info.php: call to core_availability\tree->__construct() line 111 of \availability\classes\info.php: call to core_availability\info->decode_availability() line 233 of \availability\condition\completion\classes\condition.php: call to core_availability\info->get_availability_tree() line 622 of \availability\classes\info.php: call to availability_completion\condition::completion_value_used() line 668 of \course\renderer.php: call to core_availability\info::completion_value_used() line 1021 of \course\renderer.php: call to core_course_renderer->course_section_cm_completion() line 919 of \course\renderer.php: call to core_course_renderer->course_section_cm() line 1088 of \course\renderer.php: call to core_course_renderer->course_section_cm_list_item() line 767 of \course\format\renderer.php: call to core_course_renderer->course_section_cm_list() line 56 of \course\format\topics\format.php: call to format_section_renderer_base->print_multiple_section_page() line 286 of \course\view.php: call to require() The error appeared on subsequent visits to the course. As the activities weren't presented, I wasn't able to check that the availability and conditional access setting were upgraded.
            Hide
            Michael de Raadt added a comment -

            I've attached a screenshow showing where the error in the course section appeared.

            I thought it was also worth noting that I did view the course, with its variety of conditional activities, before running the upgrade. It appeared normally then.

            Show
            Michael de Raadt added a comment - I've attached a screenshow showing where the error in the course section appeared. I thought it was also worth noting that I did view the course, with its variety of conditional activities, before running the upgrade. It appeared normally then.
            Hide
            Ankit Agarwal added a comment - - edited

            I ran into following error when trying to upgrade my integration install from last weeks to this weeks setup

            Database transaction aborted automatically in /var/www/int/master/moodle/admin/cli/upgrade.php
            Default exception handler: Error reading from database Debug: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'DISTINCT cma.id) AS availcount,
                                       COUNT (DISTINCT cmf.i' at line 2
             
                                SELECT cm.id, cm.availablefrom, cm.availableuntil, cm.showavailability,
                                       COUNT (DISTINCT cma.id) AS availcount,
                                       COUNT (DISTINCT cmf.id) AS fieldcount
                                  FROM mdl_course_modules cm
                                       LEFT JOIN mdl_course_modules_availability cma ON cma.coursemoduleid = cm.id
                                       LEFT JOIN mdl_course_modules_avail_fields cmf ON cmf.coursemoduleid = cm.id
                                 WHERE cm.availablefrom != 0 OR
                                       cm.availableuntil != 0 OR
                                       cma.id IS NOT NULL OR
                                       cmf.id IS NOT NULL
                              GROUP BY cm.id, cm.availablefrom, cm.availableuntil, cm.showavailability
            [array (
            )]
            Error code: dmlreadexception
            * line 443 of /lib/dml/moodle_database.php: dml_read_exception thrown
            * line 953 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end()
            * line 3433 of /lib/db/upgrade.php: call to mysqli_native_moodle_database->get_recordset_sql()
            * line 1560 of /lib/upgradelib.php: call to xmldb_main_upgrade()
            * line 165 of /admin/cli/upgrade.php: call to upgrade_core()
             
            !!! Error reading from database !!!
            !! You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'DISTINCT cma.id) AS availcount,
                                       COUNT (DISTINCT cmf.i' at line 2
             
                                SELECT cm.id, cm.availablefrom, cm.availableuntil, cm.showavailability,
                                       COUNT (DISTINCT cma.id) AS availcount,
                                       COUNT (DISTINCT cmf.id) AS fieldcount
                                  FROM mdl_course_modules cm
                                       LEFT JOIN mdl_course_modules_availability cma ON cma.coursemoduleid = cm.id
                                       LEFT JOIN mdl_course_modules_avail_fields cmf ON cmf.coursemoduleid = cm.id
                                 WHERE cm.availablefrom != 0 OR
                                       cm.availableuntil != 0 OR
                                       cma.id IS NOT NULL OR
                                       cmf.id IS NOT NULL
                              GROUP BY cm.id, cm.availablefrom, cm.availableuntil, cm.showavailability
            [array (
            )]
            Error code: dmlreadexception !!
            !! Stack trace: * line 443 of /lib/dml/moodle_database.php: dml_read_exception thrown
            * line 953 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database-&gt;query_end()
            * line 3433 of /lib/db/upgrade.php: call to mysqli_native_moodle_database-&gt;get_recordset_sql()
            * line 1560 of /lib/upgradelib.php: call to xmldb_main_upgrade()
            * line 165 of /admin/cli/upgrade.php: call to upgrade_core()
             !!
            

            My mysql version is:-
            Server version: 5.5.31-0ubuntu0.12.10.1 (Ubuntu)

            Show
            Ankit Agarwal added a comment - - edited I ran into following error when trying to upgrade my integration install from last weeks to this weeks setup Database transaction aborted automatically in /var/www/int/master/moodle/admin/cli/upgrade.php Default exception handler: Error reading from database Debug: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'DISTINCT cma.id) AS availcount, COUNT (DISTINCT cmf.i' at line 2   SELECT cm.id, cm.availablefrom, cm.availableuntil, cm.showavailability, COUNT (DISTINCT cma.id) AS availcount, COUNT (DISTINCT cmf.id) AS fieldcount FROM mdl_course_modules cm LEFT JOIN mdl_course_modules_availability cma ON cma.coursemoduleid = cm.id LEFT JOIN mdl_course_modules_avail_fields cmf ON cmf.coursemoduleid = cm.id WHERE cm.availablefrom != 0 OR cm.availableuntil != 0 OR cma.id IS NOT NULL OR cmf.id IS NOT NULL GROUP BY cm.id, cm.availablefrom, cm.availableuntil, cm.showavailability [array ( )] Error code: dmlreadexception * line 443 of /lib/dml/moodle_database.php: dml_read_exception thrown * line 953 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end() * line 3433 of /lib/db/upgrade.php: call to mysqli_native_moodle_database->get_recordset_sql() * line 1560 of /lib/upgradelib.php: call to xmldb_main_upgrade() * line 165 of /admin/cli/upgrade.php: call to upgrade_core()   !!! Error reading from database !!! !! You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'DISTINCT cma.id) AS availcount, COUNT (DISTINCT cmf.i' at line 2   SELECT cm.id, cm.availablefrom, cm.availableuntil, cm.showavailability, COUNT (DISTINCT cma.id) AS availcount, COUNT (DISTINCT cmf.id) AS fieldcount FROM mdl_course_modules cm LEFT JOIN mdl_course_modules_availability cma ON cma.coursemoduleid = cm.id LEFT JOIN mdl_course_modules_avail_fields cmf ON cmf.coursemoduleid = cm.id WHERE cm.availablefrom != 0 OR cm.availableuntil != 0 OR cma.id IS NOT NULL OR cmf.id IS NOT NULL GROUP BY cm.id, cm.availablefrom, cm.availableuntil, cm.showavailability [array ( )] Error code: dmlreadexception !! !! Stack trace: * line 443 of /lib/dml/moodle_database.php: dml_read_exception thrown * line 953 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database-&gt;query_end() * line 3433 of /lib/db/upgrade.php: call to mysqli_native_moodle_database-&gt;get_recordset_sql() * line 1560 of /lib/upgradelib.php: call to xmldb_main_upgrade() * line 165 of /admin/cli/upgrade.php: call to upgrade_core() !! My mysql version is:- Server version: 5.5.31-0ubuntu0.12.10.1 (Ubuntu)
            Hide
            Rajesh Taneja added a comment -

            FYI: This is failing two behat features,

            1. completion/tests/behat/restrict_activity_by_grade.feature
            2. completion/tests/behat/restrict_section_availability.feature

            Restrictions are now added by clicking on button and not present by default. Will try to create a patch, after I finish my testing.

            Show
            Rajesh Taneja added a comment - FYI: This is failing two behat features, completion/tests/behat/restrict_activity_by_grade.feature completion/tests/behat/restrict_section_availability.feature Restrictions are now added by clicking on button and not present by default. Will try to create a patch, after I finish my testing.
            Hide
            Marina Glancy added a comment -

            I have the same failure as Ankit on mysql. Fix:

            git pull https://github.com/marinaglancy/moodle.git wip-MDL-44070-fix
            

            Show
            Marina Glancy added a comment - I have the same failure as Ankit on mysql. Fix: git pull https://github.com/marinaglancy/moodle.git wip-MDL-44070-fix
            Hide
            Damyon Wiese added a comment -

            Thanks Marina, I've pulled that fix in.

            Show
            Damyon Wiese added a comment - Thanks Marina, I've pulled that fix in.
            Hide
            Damyon Wiese added a comment -

            Note - leaving this as failed, because we still need a fix for behat.

            Show
            Damyon Wiese added a comment - Note - leaving this as failed, because we still need a fix for behat.
            Hide
            Michael de Raadt added a comment -

            I've unassigned myself as tester because I won't be in tomorrow to complete the test.

            It was very well set up, though. Thanks for your work there, Sam.

            Show
            Michael de Raadt added a comment - I've unassigned myself as tester because I won't be in tomorrow to complete the test. It was very well set up, though. Thanks for your work there, Sam.
            Show
            Rajesh Taneja added a comment - Attaching behat fix https://github.com/rajeshtaneja/moodle/commit/20687842297631bb39ed526796b9922631c8bcb3
            Hide
            Sam Marshall added a comment -

            Thank you very much for the fixes - I've reviewed both, look good.

            About the MySQL issue - wow, I didn't realise, will try to remember to watch out for whitespace there in future.

            About the Behat test - sorry about that, I forgot to check for existing tests that covered this area. It's probably worth keeping these existing tests for consistency but just for info, those situations are also covered by the new tests I wrote within this feature, specifically availability/conditions/grade/tests/behat and scenario 'Section availability display' in availability/tests/behat/display_availability.feature,

            Regarding Michael's error, doesn't look like we have an explanation for this yet? I will retry the upgrade test today. Obviously it's important that upgrade works! Some information would be useful if possible: which db version, which Moodle version prior to upgrade (2.6 or 2.7 prior to this commit?) If anybody else tries the upgrade test please let me know if it passes/fails.

            Show
            Sam Marshall added a comment - Thank you very much for the fixes - I've reviewed both, look good. About the MySQL issue - wow, I didn't realise, will try to remember to watch out for whitespace there in future. About the Behat test - sorry about that, I forgot to check for existing tests that covered this area. It's probably worth keeping these existing tests for consistency but just for info, those situations are also covered by the new tests I wrote within this feature, specifically availability/conditions/grade/tests/behat and scenario 'Section availability display' in availability/tests/behat/display_availability.feature, Regarding Michael's error, doesn't look like we have an explanation for this yet? I will retry the upgrade test today. Obviously it's important that upgrade works! Some information would be useful if possible: which db version, which Moodle version prior to upgrade (2.6 or 2.7 prior to this commit?) If anybody else tries the upgrade test please let me know if it passes/fails.
            Hide
            Sam Marshall added a comment -

            I repeated the upgrade test twice - from MOODLE_26_STABLE and from master, in both cases doing a fresh install, turning on the 3 required admin settings, restoring the provided course, then upgrading to integration/master. This was successful.

            I then worked out the reason for the error Michael was seeing. My guess is that Michael is running a Windows PHP setup. The problem is that my example course backup includes dates in 2044 which require the use of 64-bit integers to represent as they are past 2038. These didn't ever work (properly) on 32-bit systems, i.e. when PHP is running on Windows. (Even the 64-bit version of PHP for Windows apparently has the same integer range limitation...)

            I have now done tests to confirm this (I can reproduce the same error) and am about to update the test script. Might also submit a code change if this has identified another problem - but probably not if it only affects numbers greater than 2038 which don't work in Moodle 2.6 anyhow (if you look at the course page after restoring my test backup in 2.6, it shows the dates wrong already before doing any upgrade).

            Show
            Sam Marshall added a comment - I repeated the upgrade test twice - from MOODLE_26_STABLE and from master, in both cases doing a fresh install, turning on the 3 required admin settings, restoring the provided course, then upgrading to integration/master. This was successful. I then worked out the reason for the error Michael was seeing. My guess is that Michael is running a Windows PHP setup. The problem is that my example course backup includes dates in 2044 which require the use of 64-bit integers to represent as they are past 2038. These didn't ever work (properly) on 32-bit systems, i.e. when PHP is running on Windows. (Even the 64-bit version of PHP for Windows apparently has the same integer range limitation...) I have now done tests to confirm this (I can reproduce the same error) and am about to update the test script. Might also submit a code change if this has identified another problem - but probably not if it only affects numbers greater than 2038 which don't work in Moodle 2.6 anyhow (if you look at the course page after restoring my test backup in 2.6, it shows the dates wrong already before doing any upgrade).
            Hide
            Sam Marshall added a comment -

            The test script is now updated, along with a new version of the backup mbz, and should pass in 32-bit systems. (I tried it on my Windows install.)

            I have done code in MDL-45027 (one commit based on current integration master) that makes the availability system more robust so that if there's invalid data, such as the 32 bit PHP date issue, you just get a bunch of debugging messages but are still able to see the course page, edit the activity, change the settings if needed, and save it again to get rid of the warnings. This is probably a pretty important fix to have in 2.7 just in case there are any other such bugs.

            Show
            Sam Marshall added a comment - The test script is now updated, along with a new version of the backup mbz, and should pass in 32-bit systems. (I tried it on my Windows install.) I have done code in MDL-45027 (one commit based on current integration master) that makes the availability system more robust so that if there's invalid data, such as the 32 bit PHP date issue, you just get a bunch of debugging messages but are still able to see the course page, edit the activity, change the settings if needed, and save it again to get rid of the warnings. This is probably a pretty important fix to have in 2.7 just in case there are any other such bugs.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            I've applied Rajesh fix here, but still getting some failures:

            01. Checkbox matching locator "'id_availablefrom_enabled'" not found.
                In step `Given I click on "id_availablefrom_enabled" "checkbox"'.                           # behat_general::i_click_on()
                From scenario `Show activity greyed-out to students when available from date is in future'. # /Users/stronk7/git_moodle/integration/completion/tests/behat/restrict_activity_by_date.feature:31
                Of feature `Restrict activity availability through date conditions'.                        # /Users/stronk7/git_moodle/integration/completion/tests/behat/restrict_activity_by_date.feature
             
            02. Checkbox matching locator "'id_availableuntil_enabled'" not found.
                In step `Given I click on "id_availableuntil_enabled" "checkbox"'.                          # behat_general::i_click_on()
                From scenario `Show activity hidden to students when available until date is in past'.      # /Users/stronk7/git_moodle/integration/completion/tests/behat/restrict_activity_by_date.feature:51
                Of feature `Restrict activity availability through date conditions'.                        # /Users/stronk7/git_moodle/integration/completion/tests/behat/restrict_activity_by_date.feature
             
            11 escenarios (9 exitosos, 2 fallidos)
            484 pasos (468 exitosos, 14 omitidos, 2 fallidos)
            9m19.523
            

            Going to take a look to them... also.. the fill form there... does not seem to match the day/month/year names... "id_availableuntil_day|month|year" is in the test, but the fields are named "x[day|month|year]". With the "x" being a literal "x". Is that ok?

            Show
            Eloy Lafuente (stronk7) added a comment - I've applied Rajesh fix here, but still getting some failures: 01. Checkbox matching locator "'id_availablefrom_enabled'" not found. In step `Given I click on "id_availablefrom_enabled" "checkbox"'. # behat_general::i_click_on() From scenario `Show activity greyed-out to students when available from date is in future'. # /Users/stronk7/git_moodle/integration/completion/tests/behat/restrict_activity_by_date.feature:31 Of feature `Restrict activity availability through date conditions'. # /Users/stronk7/git_moodle/integration/completion/tests/behat/restrict_activity_by_date.feature   02. Checkbox matching locator "'id_availableuntil_enabled'" not found. In step `Given I click on "id_availableuntil_enabled" "checkbox"'. # behat_general::i_click_on() From scenario `Show activity hidden to students when available until date is in past'. # /Users/stronk7/git_moodle/integration/completion/tests/behat/restrict_activity_by_date.feature:51 Of feature `Restrict activity availability through date conditions'. # /Users/stronk7/git_moodle/integration/completion/tests/behat/restrict_activity_by_date.feature   11 escenarios (9 exitosos, 2 fallidos) 484 pasos (468 exitosos, 14 omitidos, 2 fallidos) 9m19.523 Going to take a look to them... also.. the fill form there... does not seem to match the day/month/year names... "id_availableuntil_day|month|year" is in the test, but the fields are named "x [day|month|year] ". With the "x" being a literal "x". Is that ok?
            Show
            Sam Marshall added a comment - Behat fix for date test: https://github.com/sammarshallou/moodle/commit/0113e46a013608e9bfe106b43df2905716cc2aa1
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Thanks, picking it and performing a complete run here...

            Show
            Eloy Lafuente (stronk7) added a comment - Thanks, picking it and performing a complete run here...
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Pulling both Rajesh & Sam commits, as far as they've lead to --tags='@core_availability,@core_completion' passing 100% here.

            11 escenarios (11 exitosos)
            493 pasos (493 exitosos)
            9m43.883s

            Going to perform a complete run with them applied.

            Show
            Eloy Lafuente (stronk7) added a comment - Pulling both Rajesh & Sam commits, as far as they've lead to --tags='@core_availability,@core_completion' passing 100% here. 11 escenarios (11 exitosos) 493 pasos (493 exitosos) 9m43.883s Going to perform a complete run with them applied.
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            (lol, not restricted to integrators)

            Ok, I'm experimenting here some problems (never seen before). Here it's a link to my complete execution, I tried it twice, the 2nd did not finished: http://pastebin.com/XJqW7vL1

            Note that's unrelated with this issue, just adding the info here because this was the issue that caused me to perform the runs. I'm going to look for newer firefox/selenium versions... just in case that helps.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - - edited (lol, not restricted to integrators) Ok, I'm experimenting here some problems (never seen before). Here it's a link to my complete execution, I tried it twice, the 2nd did not finished: http://pastebin.com/XJqW7vL1 Note that's unrelated with this issue, just adding the info here because this was the issue that caused me to perform the runs. I'm going to look for newer firefox/selenium versions... just in case that helps. Ciao
            Hide
            Damyon Wiese added a comment -

            I'm running behat on this now - I'll report back with the results, but I think this is safe to go back to testing in the meantime.

            Show
            Damyon Wiese added a comment - I'm running behat on this now - I'll report back with the results, but I think this is safe to go back to testing in the meantime.
            Hide
            Rajesh Taneja added a comment -

            Aha, I missed that behat feature file.

            Few minor issue which I observed:

            1. Got following error when any invalid restriction is added (Getting same error on master, so probably another issue)
              • Grade option value is not added etc.

                Invalid array parameter detected in required_param(): page
                line 637 of /lib/moodlelib.php: call to debugging()
                line 48 of /repository/wikimedia/lib.php: call to optional_param()
                line 2929 of /repository/lib.php: call to repository_wikimedia->__construct()
                line 81 of /cache/lib.php: call to repository::wake_from_cache()
                line 223 of /cache/lib.php: call to cache_cached_object->restore_object()
                line 81 of /cache/lib.php: call to cacheable_object_array::wake_from_cache()
                line 310 of /cache/classes/loaders.php: call to cache_cached_object->restore_object()
                line 1074 of /repository/lib.php: call to cache->get()
                line 3263 of /repository/lib.php: call to repository::get_instances()
                line 342 of /lib/form/editor.php: call to initialise_filepicker()
                line 183 of /lib/pear/HTML/QuickForm/Renderer/Tableless.php: call to MoodleQuickForm_editor->toHtml()
                line 2773 of /lib/formslib.php: call to HTML_QuickForm_Renderer_Tableless->renderElement()
                line 403 of /lib/pear/HTML/QuickForm/element.php: call to MoodleQuickForm_Renderer->renderElement()
                line 1632 of /lib/pear/HTML/QuickForm.php: call to HTML_QuickForm_element->accept()
                line 1694 of /lib/formslib.php: call to HTML_QuickForm->accept()
                line 1675 of /lib/pear/HTML/QuickForm.php: call to MoodleQuickForm->accept()
                line 435 of /lib/pear/HTML/Common.php: call to HTML_QuickForm->toHtml()
                line 204 of /lib/pear/HTML/QuickForm/DHTMLRulesTableless.php: call to HTML_Common->display()
                line 921 of /lib/formslib.php: call to HTML_QuickForm_DHTMLRulesTableless->display()
                line 314 of /course/modedit.php: call to moodleform->display()
                

            2. wrong tag (@wip) in availability/tests/behat/edit_availability.feature

            Rest all worked as expected.

            1. Behat passing
            2. Getting validation form error (Grade min <= max)
            3. Performance
              Performance param With availability Without availability
              Time 0.799 secs 0.8115 secs
              RAM 15.2MB 15.2MB
              RAM Peak 19.2MB 19.1MB
              Files Included 199 185
              get_strings_call 1319 1230
              DB reads/write 35/4 255/1
              DB queries time 0.03635 secs 0.10329 secs
            Show
            Rajesh Taneja added a comment - Aha, I missed that behat feature file. Few minor issue which I observed: Got following error when any invalid restriction is added (Getting same error on master, so probably another issue) Grade option value is not added etc. Invalid array parameter detected in required_param(): page line 637 of /lib/moodlelib.php: call to debugging() line 48 of /repository/wikimedia/lib.php: call to optional_param() line 2929 of /repository/lib.php: call to repository_wikimedia->__construct() line 81 of /cache/lib.php: call to repository::wake_from_cache() line 223 of /cache/lib.php: call to cache_cached_object->restore_object() line 81 of /cache/lib.php: call to cacheable_object_array::wake_from_cache() line 310 of /cache/classes/loaders.php: call to cache_cached_object->restore_object() line 1074 of /repository/lib.php: call to cache->get() line 3263 of /repository/lib.php: call to repository::get_instances() line 342 of /lib/form/editor.php: call to initialise_filepicker() line 183 of /lib/pear/HTML/QuickForm/Renderer/Tableless.php: call to MoodleQuickForm_editor->toHtml() line 2773 of /lib/formslib.php: call to HTML_QuickForm_Renderer_Tableless->renderElement() line 403 of /lib/pear/HTML/QuickForm/element.php: call to MoodleQuickForm_Renderer->renderElement() line 1632 of /lib/pear/HTML/QuickForm.php: call to HTML_QuickForm_element->accept() line 1694 of /lib/formslib.php: call to HTML_QuickForm->accept() line 1675 of /lib/pear/HTML/QuickForm.php: call to MoodleQuickForm->accept() line 435 of /lib/pear/HTML/Common.php: call to HTML_QuickForm->toHtml() line 204 of /lib/pear/HTML/QuickForm/DHTMLRulesTableless.php: call to HTML_Common->display() line 921 of /lib/formslib.php: call to HTML_QuickForm_DHTMLRulesTableless->display() line 314 of /course/modedit.php: call to moodleform->display() wrong tag (@wip) in availability/tests/behat/edit_availability.feature Rest all worked as expected. Behat passing Getting validation form error (Grade min <= max) Performance Performance param With availability Without availability Time 0.799 secs 0.8115 secs RAM 15.2MB 15.2MB RAM Peak 19.2MB 19.1MB Files Included 199 185 get_strings_call 1319 1230 DB reads/write 35/4 255/1 DB queries time 0.03635 secs 0.10329 secs
            Hide
            Rajesh Taneja added a comment -

            Removed unneeded tag and comment about failure (As this is selenium/browserkit issue and will be upstream soon)
            https://github.com/rajeshtaneja/moodle/commit/0adadbf00bcc9c9f7e2b90730ac766c936c4243a

            Show
            Rajesh Taneja added a comment - Removed unneeded tag and comment about failure (As this is selenium/browserkit issue and will be upstream soon) https://github.com/rajeshtaneja/moodle/commit/0adadbf00bcc9c9f7e2b90730ac766c936c4243a
            Hide
            Damyon Wiese added a comment -

            Thanks Raj - I've pulled that commit in now.

            Show
            Damyon Wiese added a comment - Thanks Raj - I've pulled that commit in now.
            Hide
            Rajesh Taneja added a comment -

            Thanks for the great improvement Sam,

            Works as mentioned.

            Passing...

            Show
            Rajesh Taneja added a comment - Thanks for the great improvement Sam, Works as mentioned. Passing...
            Hide
            Marina Glancy added a comment -

            Just noting here that this issue caused the performance regression MDL-45038

            Show
            Marina Glancy added a comment - Just noting here that this issue caused the performance regression MDL-45038
            Hide
            Dan Poltawski added a comment -

            Thanks for your efforts, this change is now part of Moodle!

            Show
            Dan Poltawski added a comment - Thanks for your efforts, this change is now part of Moodle!
            Hide
            Mary Cooch added a comment -

            Removing qa_test_required label as there are 3 tests in the master copy ready for 2.7 manual testing. If there are any errors or if any more should be created, please add a comment here.
            MDLQA-6688 and MDLQA-6691 and MDLQA-6690

            Show
            Mary Cooch added a comment - Removing qa_test_required label as there are 3 tests in the master copy ready for 2.7 manual testing. If there are any errors or if any more should be created, please add a comment here. MDLQA-6688 and MDLQA-6691 and MDLQA-6690
            Hide
            Mary Cooch added a comment -

            Removing docs_required label as the new settings have been documented here http://docs.moodle.org/27/en/Conditional_activities_settings (However, made my own note to self to give more specific examples in the other related pages shortly.)

            Show
            Mary Cooch added a comment - Removing docs_required label as the new settings have been documented here http://docs.moodle.org/27/en/Conditional_activities_settings (However, made my own note to self to give more specific examples in the other related pages shortly.)
            Hide
            Ankit Agarwal added a comment - - edited

            This seemed to have caused an unexpected regression MDL-45409

            Show
            Ankit Agarwal added a comment - - edited This seemed to have caused an unexpected regression MDL-45409
            Hide
            Ray Morris added a comment -

            Awesome work, Sam, thanks.

            Show
            Ray Morris added a comment - Awesome work, Sam, thanks.

              People

              • Votes:
                32 Vote for this issue
                Watchers:
                24 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: