Moodle
  1. Moodle
  2. MDL-27628

Meta linked courses can only be added individually.

    Details

    • Database:
      MySQL
    • Testing Instructions:
      Hide

      To test this you will need multiple courses with enroled users. You will then need to enable the course meta link enrolment method.

      1. In Site Administration > Enrolments > Manage enrol plugins, enable Course Meta Link.

      Test adding multiple courses:

      1. In a course, access Course Administration > Users > Enrollment methods. Choose Course Meta Link.
      2. Verify that you have lists unlinked courses.
      3. Select a search term which will match the shortname, fullname, or idnumber of multiple courses on the test instance. Enter that term in the search box. Verify that you have search results.
      4. Select two or more search results and click "Link selected". Verify that you return to the enrolment instance list and that the courses you selected are now listed with the "Course meta link" enrolment method.
      Show
      To test this you will need multiple courses with enroled users. You will then need to enable the course meta link enrolment method. In Site Administration > Enrolments > Manage enrol plugins, enable Course Meta Link. Test adding multiple courses: In a course, access Course Administration > Users > Enrollment methods. Choose Course Meta Link. Verify that you have lists unlinked courses. Select a search term which will match the shortname, fullname, or idnumber of multiple courses on the test instance. Enter that term in the search box. Verify that you have search results. Select two or more search results and click "Link selected". Verify that you return to the enrolment instance list and that the courses you selected are now listed with the "Course meta link" enrolment method.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_23_STABLE, MOODLE_27_STABLE, MOODLE_28_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-27628-master
    • Sprint:
      Team Beards Sprint 4
    • Issue size:
      Large

      Description

      Pre-2.0 it was possible to add many "child" courses in one operation.

      The revised interface makes adding multiple courses via meta linking very time intensive.

      Use case: Support area (course) needs all courses on site to be linked to it.

        Gliffy Diagrams

        1. addinstance.php
          9 kB
          Bob Puffer
        2. MDL-27628-2.2-Patch.diff
          4 kB
          James Rudd
        1. Course meta link 2011-12-08 13-36-59.png
          19 kB
        2. metamanageui.png
          61 kB
        3. Screen Shot 2012-06-06 at 21.12.05.png
          107 kB

          Issue Links

            Activity

            Hide
            Ian McNaught added a comment -

            Agreed, meta courses were an awesome feature, but close to useless now if you want more that 2-3 courses combined.

            Show
            Ian McNaught added a comment - Agreed, meta courses were an awesome feature, but close to useless now if you want more that 2-3 courses combined.
            Hide
            Justin Litalien added a comment -

            Absolutely Ian. Our institution has thousands of classes for both UG and GR. Finding each course to combine is like searching for a needle in a haystack. Very time consuming. Disappointed that the older version was modified.

            Is there an update on this modification to Moodle 2?

            Show
            Justin Litalien added a comment - Absolutely Ian. Our institution has thousands of classes for both UG and GR. Finding each course to combine is like searching for a needle in a haystack. Very time consuming. Disappointed that the older version was modified. Is there an update on this modification to Moodle 2?
            Hide
            Troy Williams added a comment - - edited

            Hi,

            The following code allows for multiple courses to be linked:

            Code available at https://github.com/troywilliams/moodle/compare/MOODLE_20_STABLE...wip-MDL-27628-MOODLE_20_STABLE

            Features:
            Allows for searching and adding multiple courses at a time.
            AJAX search capabilities.

            TODO:
            Display count of returned courses from search
            Show all button, if rowlimit set.

            Config:
            Add to you config.php file

             
            $CFG->enrol_meta_addmultiple = true; // replaces addinstance link in dropdown redirection
            $CFG->enrol_meta_addmultiple_rowlimit = 250; // rows displayed
            

            Cheers

            Troy

            Show
            Troy Williams added a comment - - edited Hi, The following code allows for multiple courses to be linked: Code available at https://github.com/troywilliams/moodle/compare/MOODLE_20_STABLE...wip-MDL-27628-MOODLE_20_STABLE Features: Allows for searching and adding multiple courses at a time. AJAX search capabilities. TODO: Display count of returned courses from search Show all button, if rowlimit set. Config: Add to you config.php file $CFG->enrol_meta_addmultiple = true; // replaces addinstance link in dropdown redirection $CFG->enrol_meta_addmultiple_rowlimit = 250; // rows displayed Cheers Troy
            Hide
            Teresa Gibbison added a comment -

            This patch is just brilliant, it saves so much time! Thanks Troy for your hard work.

            Show
            Teresa Gibbison added a comment - This patch is just brilliant, it saves so much time! Thanks Troy for your hard work.
            Hide
            Ann Adamcik added a comment -

            This is a huge issue for large sites. With 5000+ courses, a drop-down selector with no search function is just unusable. This really ought to be a bug rather than an improvement, as it's a significant usability regression from 1.9.

            Thanks, Troy, for the patch! I'll try it out this week.

            Show
            Ann Adamcik added a comment - This is a huge issue for large sites. With 5000+ courses, a drop-down selector with no search function is just unusable. This really ought to be a bug rather than an improvement, as it's a significant usability regression from 1.9. Thanks, Troy, for the patch! I'll try it out this week.
            Hide
            James Rudd added a comment - - edited

            Just wanted to agree this is a big issue for any site which wanted to add several meta classes of several courses.
            Has anyone heard any update since last year? We are running 2.2 and this still has not been included.

            Also is there an updated patch/branch for 2.2 available?

            Show
            James Rudd added a comment - - edited Just wanted to agree this is a big issue for any site which wanted to add several meta classes of several courses. Has anyone heard any update since last year? We are running 2.2 and this still has not been included. Also is there an updated patch/branch for 2.2 available?
            Hide
            Troy Williams added a comment -

            I try get a branches up and running for 2.1, 2.2 it probably won't be till later next week though. Could give a go yourself if urgent.

            new files:

            enrol/meta/addmultiple.php
            enrol/meta/addmultiple_form.php
            enrol/meta/module.js
            enrol/meta/search.php

            minor edits to existing (check link above at github for diff):

            enrol/meta/lang/en/enrol_meta.php
            enrol/meta/lib.php

            Then add $CFG vars to your config.php

            Troy

            Show
            Troy Williams added a comment - I try get a branches up and running for 2.1, 2.2 it probably won't be till later next week though. Could give a go yourself if urgent. new files: enrol/meta/addmultiple.php enrol/meta/addmultiple_form.php enrol/meta/module.js enrol/meta/search.php minor edits to existing (check link above at github for diff): enrol/meta/lang/en/enrol_meta.php enrol/meta/lib.php Then add $CFG vars to your config.php Troy
            Hide
            James Rudd added a comment -

            Thanks Troy, I'll give it a go.

            Show
            James Rudd added a comment - Thanks Troy, I'll give it a go.
            Hide
            James Rudd added a comment -

            Hi Troy, I cherry picked the changes straight into 2.2 and it appears to work OK. Main thing I want to change is to remove the need for additions to config.php and just have the two options added to the Meta Links plugin settings page. I probably won't get a chance to do this until next week.

            Show
            James Rudd added a comment - Hi Troy, I cherry picked the changes straight into 2.2 and it appears to work OK. Main thing I want to change is to remove the need for additions to config.php and just have the two options added to the Meta Links plugin settings page. I probably won't get a chance to do this until next week.
            Hide
            Troy Williams added a comment -

            Hi James,

            Cool lol, I have just been setting up 2.1, 2.2 branches on github for this issue.
            2.1
            https://github.com/troywilliams/moodle/compare/MOODLE_21_STABLE...wip-MDL-27628-MOODLE_21_STABLE
            2.2
            https://github.com/troywilliams/moodle/compare/MOODLE_22_STABLE...wip-MDL-27628-MOODLE_22_STABLE

            yeah definitely a good idea to put in settings page, the rowlimit could probably be hard coded, I expect people will be using the search feature and the results are highly unlikely to exceed thousands of courses listed, makes the performance better.

            Show
            Troy Williams added a comment - Hi James, Cool lol, I have just been setting up 2.1, 2.2 branches on github for this issue. 2.1 https://github.com/troywilliams/moodle/compare/MOODLE_21_STABLE...wip-MDL-27628-MOODLE_21_STABLE 2.2 https://github.com/troywilliams/moodle/compare/MOODLE_22_STABLE...wip-MDL-27628-MOODLE_22_STABLE yeah definitely a good idea to put in settings page, the rowlimit could probably be hard coded, I expect people will be using the search feature and the results are highly unlikely to exceed thousands of courses listed, makes the performance better.
            Hide
            James Rudd added a comment -

            I still think this is the way to go, but I'll just mention an alternative someone emailed me, http://moodle.org/plugins/view.php?plugin=block_metalink
            It may be useful for bulk creation but this issue still needs to be addressed.

            Show
            James Rudd added a comment - I still think this is the way to go, but I'll just mention an alternative someone emailed me, http://moodle.org/plugins/view.php?plugin=block_metalink It may be useful for bulk creation but this issue still needs to be addressed.
            Hide
            James Rudd added a comment - - edited

            Attached a patch for Troy's modifications to move settings from config.php to meta settings page.
            I am unsure of default for Row limit, I have set it to 250, but 0 may be better. Also help strings probably need some work.

            I also noticed during testing that the search function only seems to search to shortname not the full name, I'm not sure if this is deliberate or a bug.

            Show
            James Rudd added a comment - - edited Attached a patch for Troy's modifications to move settings from config.php to meta settings page. I am unsure of default for Row limit, I have set it to 250, but 0 may be better. Also help strings probably need some work. I also noticed during testing that the search function only seems to search to shortname not the full name, I'm not sure if this is deliberate or a bug.
            Hide
            Nathan Kowald added a comment - - edited

            Thanks Troy for your great work! AJAX search capability is great. Thanks James for your patch.

            I agree with James, better search results can be achieved by using Moodle's existing course search function, 'get_courses_search()' --> here: https://github.com/moodle/moodle/blob/master/lib/datalib.php#L730.

            This will take a search term of "unit fitness", recognise this contains two keywords and search on both. It also searches course summary, fullname and idnumber. It checks course visibility and the 'moodle/course:viewhiddencourses' capability too.

            Edit:
            Yes! Glad this is being fixed in STABLE versions of Moodle.

            Until this time, I've taken Troy and James's work and modified 'search.php' and 'addmultiple.php' to use get_courses_search() - for better results. Also noticed AJAX results were being ordered by 'course id' instead of the given SQL order (shortname ASC). I fixed this here: https://github.com/conel/moodle/blob/da0f8dcac9b748e56cf2a04c33c2cede0ca0748e/enrol/meta/search.php#L60-61

            Here's a compare of 'MOODLE_22_STABLE' and the changes mentioned above: https://github.com/conel/moodle/compare/moodle:MOODLE_22_STABLE...conel:wip-MDL-27628-MOODLE_22_STABLE

            Show
            Nathan Kowald added a comment - - edited Thanks Troy for your great work! AJAX search capability is great. Thanks James for your patch. I agree with James, better search results can be achieved by using Moodle's existing course search function, 'get_courses_search()' --> here: https://github.com/moodle/moodle/blob/master/lib/datalib.php#L730 . This will take a search term of "unit fitness", recognise this contains two keywords and search on both. It also searches course summary, fullname and idnumber. It checks course visibility and the 'moodle/course:viewhiddencourses' capability too. Edit: Yes! Glad this is being fixed in STABLE versions of Moodle. Until this time, I've taken Troy and James's work and modified 'search.php' and 'addmultiple.php' to use get_courses_search() - for better results. Also noticed AJAX results were being ordered by 'course id' instead of the given SQL order (shortname ASC). I fixed this here: https://github.com/conel/moodle/blob/da0f8dcac9b748e56cf2a04c33c2cede0ca0748e/enrol/meta/search.php#L60-61 Here's a compare of 'MOODLE_22_STABLE' and the changes mentioned above: https://github.com/conel/moodle/compare/moodle:MOODLE_22_STABLE...conel:wip-MDL-27628-MOODLE_22_STABLE
            Hide
            Dan Poltawski added a comment -

            Converting to stable issue and bug since this sounds like a regression fron 1.9

            Show
            Dan Poltawski added a comment - Converting to stable issue and bug since this sounds like a regression fron 1.9
            Hide
            Ray Lawrence added a comment -

            Groovy, thanks.

            Show
            Ray Lawrence added a comment - Groovy, thanks.
            Hide
            Dan Poltawski added a comment -

            Hi Everyone who has voted for and helped on this issue. I'm currently looking into this issue.

            Apologies that it has taken us so long to look at it.

            Show
            Dan Poltawski added a comment - Hi Everyone who has voted for and helped on this issue. I'm currently looking into this issue. Apologies that it has taken us so long to look at it.
            Hide
            Dan Poltawski added a comment -

            Attaching screenshot of the 1.9 interface.

            Show
            Dan Poltawski added a comment - Attaching screenshot of the 1.9 interface.
            Hide
            Petr Skoda added a comment -

            hi, a few notes:
            1/ we do not use $CFG->enrol_meta_addmultiple any more, please use the new plugin config instead
            2/ all ajax scripts should use define('AJAX_SCRIPT', true) especially because it simplifies stuff like require_login() and require_sesskey()
            3/ I do not like the debug code in search.php at all, all devs should be able to read json code in debug consoles easily these days
            4/ $searchgroup[] = &$mform->createElement is not necessary in PHP5

            Show
            Petr Skoda added a comment - hi, a few notes: 1/ we do not use $CFG->enrol_meta_addmultiple any more, please use the new plugin config instead 2/ all ajax scripts should use define('AJAX_SCRIPT', true) especially because it simplifies stuff like require_login() and require_sesskey() 3/ I do not like the debug code in search.php at all, all devs should be able to read json code in debug consoles easily these days 4/ $searchgroup[] = &$mform->createElement is not necessary in PHP5
            Hide
            Dan Poltawski added a comment -

            One thing this patch doesn't do is bulk removal.. As you can see from the above 1.9 screenshot, the old interface worked both ways. However, removing instances quickly like this also is perhaps not something we want to encourage. With enrolments, the phrase 'you can't put the toothpaste back in the tube' comes to mind. If a student has participated in a course, its not that simple to just remove the metacourse link.

            On the specifics of the patch, i'm afraid the patch attached here in its current form would not be suitable for the stable branches (2.1-2.3) because its doing a little bit too much for what we would likely consider in the stable branches.

            However I wonder if there is something which can be achieved with much a much simpler improvement of that select box, without any fancy ajax search to do multiple selections. In 2.4 we could have a more improved interface, with a good universal search box etc.

            Show
            Dan Poltawski added a comment - One thing this patch doesn't do is bulk removal.. As you can see from the above 1.9 screenshot, the old interface worked both ways. However, removing instances quickly like this also is perhaps not something we want to encourage. With enrolments, the phrase 'you can't put the toothpaste back in the tube' comes to mind. If a student has participated in a course, its not that simple to just remove the metacourse link. On the specifics of the patch, i'm afraid the patch attached here in its current form would not be suitable for the stable branches (2.1-2.3) because its doing a little bit too much for what we would likely consider in the stable branches. However I wonder if there is something which can be achieved with much a much simpler improvement of that select box, without any fancy ajax search to do multiple selections. In 2.4 we could have a more improved interface, with a good universal search box etc.
            Hide
            Ray Lawrence added a comment -

            "However, removing instances quickly like this also is perhaps not something we want to encourage."

            Really? Why's that? Certainly there are considerations with unenrolments (in general) but that process to intiate that should be as simple as enrolments.

            Show
            Ray Lawrence added a comment - "However, removing instances quickly like this also is perhaps not something we want to encourage." Really? Why's that? Certainly there are considerations with unenrolments (in general) but that process to intiate that should be as simple as enrolments.
            Hide
            Troy Williams added a comment -

            Hi,

            Code was developed for Moodle 2.0.x. I put this together pretty quickly after my inbox started to fill with emails like ** PROBLEM Service Alert: moodle admin is CRITICAL **

            I had initially looked at using a searchable selector formlib element. I quickly found out that don't work to good when you have 14000+ Moodle courses.

            As you may of noticed the code is stolen/borrowed from the user selector which I don't think has changed much from 2.0.x to the current master(2.3dev)

            I guess the one thing I would like to point out, is that any solution has to handle a large number of courses.

            Show
            Troy Williams added a comment - Hi, Code was developed for Moodle 2.0.x. I put this together pretty quickly after my inbox started to fill with emails like ** PROBLEM Service Alert: moodle admin is CRITICAL ** I had initially looked at using a searchable selector formlib element. I quickly found out that don't work to good when you have 14000+ Moodle courses. As you may of noticed the code is stolen/borrowed from the user selector which I don't think has changed much from 2.0.x to the current master(2.3dev) I guess the one thing I would like to point out, is that any solution has to handle a large number of courses.
            Hide
            James Rudd added a comment -

            Hi Nathan,
            Just a comment on your latest Git diff.
            In enrol/meta/lib.php you do not need "global $CFG;" if you are using the "$plugin->get_config('addmultiple')" method.

            The new search method works well and correctly matches both the course short name and full name.

            Show
            James Rudd added a comment - Hi Nathan, Just a comment on your latest Git diff. In enrol/meta/lib.php you do not need "global $CFG;" if you are using the "$plugin->get_config('addmultiple')" method. The new search method works well and correctly matches both the course short name and full name.
            Hide
            Nathan Kowald added a comment -

            Thanks James. Corrected.

            Show
            Nathan Kowald added a comment - Thanks James. Corrected.
            Hide
            Keith Dingley added a comment -

            I have just added this upgrade (https://github.com/conel/moodle/compare/moodle:MOODLE_22_STABLE...conel:wip-MDL-27628-MOODLE_22_STABLE#diff-3 ) to a 2.2 site. It seems to work well. Is there any chance of it being put in as a "Updated Metacourse search/enrolment" plugin rather than have to go through the git hub? This would make it more readily available to other users.

            Show
            Keith Dingley added a comment - I have just added this upgrade ( https://github.com/conel/moodle/compare/moodle:MOODLE_22_STABLE...conel:wip-MDL-27628-MOODLE_22_STABLE#diff-3 ) to a 2.2 site. It seems to work well. Is there any chance of it being put in as a "Updated Metacourse search/enrolment" plugin rather than have to go through the git hub? This would make it more readily available to other users.
            Hide
            CLAIRE BROWNE added a comment -

            Thanks for the update, will this work for oracle databases as well?

            Show
            CLAIRE BROWNE added a comment - Thanks for the update, will this work for oracle databases as well?
            Hide
            Matthew Putz added a comment -

            It seems that this interface could be radically improved by the simple addition of a search function, not as a hack, but as core code. We're not so concerned about being able to add multiple courses (although that would be wonderful), but we are concerned about having to take ten minutes to find each course from a massive list. This module is unusable at the moment.

            Show
            Matthew Putz added a comment - It seems that this interface could be radically improved by the simple addition of a search function, not as a hack, but as core code. We're not so concerned about being able to add multiple courses (although that would be wonderful), but we are concerned about having to take ten minutes to find each course from a massive list. This module is unusable at the moment.
            Hide
            Eric Strom added a comment -

            I would agree the search function would be lovely. I do have a need to assign about 150 child courses to a particular course and sorely miss the functionality of 1.9 in this case. Is the current upgrade available supported in 2.3?

            Show
            Eric Strom added a comment - I would agree the search function would be lovely. I do have a need to assign about 150 child courses to a particular course and sorely miss the functionality of 1.9 in this case. Is the current upgrade available supported in 2.3?
            Hide
            James Rudd added a comment -

            I have it working in Moodle 2.3. Here is a patch from my GitHub for 2.3.
            https://github.com/ruddj/moodle/commit/c06da88cf82e38e603028e597f393fe4cafe711f.patch

            Show
            James Rudd added a comment - I have it working in Moodle 2.3. Here is a patch from my GitHub for 2.3. https://github.com/ruddj/moodle/commit/c06da88cf82e38e603028e597f393fe4cafe711f.patch
            Hide
            Eric Strom added a comment -

            James, thanks. We tried out the Upload Meta-course Links block (http://moodle.org/plugins/pluginversion.php?id=557) instead which seemed to work in 2.3 as well.

            Show
            Eric Strom added a comment - James, thanks. We tried out the Upload Meta-course Links block ( http://moodle.org/plugins/pluginversion.php?id=557 ) instead which seemed to work in 2.3 as well.
            Hide
            Dan Poltawski added a comment -

            Hi,

            Sorry i'm not going to have a chance to look at this issue in the near future and so i'm sending it back to moodle.com so someone can pick it up.

            Note that as mentioned when initially looking at this, one of the problems is that if we add this one way solution (add ability to add lots of a time) we need a way to remove them too.

            Show
            Dan Poltawski added a comment - Hi, Sorry i'm not going to have a chance to look at this issue in the near future and so i'm sending it back to moodle.com so someone can pick it up. Note that as mentioned when initially looking at this, one of the problems is that if we add this one way solution (add ability to add lots of a time) we need a way to remove them too.
            Hide
            James Rudd added a comment -

            Although not a real solution when I need to bulk delete I just Ctrl+Click the delete MetaLink for each course to open each in a new tab, and then progress through Confirm window & close for all links. Works OK for a few, but would not work well >10.

            Show
            James Rudd added a comment - Although not a real solution when I need to bulk delete I just Ctrl+Click the delete MetaLink for each course to open each in a new tab, and then progress through Confirm window & close for all links. Works OK for a few, but would not work well >10.
            Hide
            Eloy Lafuente (stronk7) added a comment -

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

            Many thanks, this is now available in all the repos (git & cvs).

            Closing, ciao

            Show
            Eloy Lafuente (stronk7) added a comment - U P S T R E A M I Z E D ! Many thanks, this is now available in all the repos (git & cvs). Closing, ciao
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Doh,

            somehow this issue was closed incorrectly when processing all the integrated issues this week. (sort of most voted and current in integration filters mix). Apologies for the confusion, reseting to previous status!

            Ciao, Eloy

            Show
            Eloy Lafuente (stronk7) added a comment - Doh, somehow this issue was closed incorrectly when processing all the integrated issues this week. (sort of most voted and current in integration filters mix). Apologies for the confusion, reseting to previous status! Ciao, Eloy
            Hide
            Iñigo Zendegi added a comment - - edited

            Hi,

            I've applied the patch for 2.2.x and worked great, thanks!

            It's a pity that this issue not been integrated just because it hasn't a way of easily remove meta linked courses (if it's this the reason), considering that the standard method neither has a deleting option and anyway, deleting can already be easily done from /enrol/instances.php

            Considering this, I would consider integrating this improvement as it is now, without waiting for a easy deleting improvement.

            Show
            Iñigo Zendegi added a comment - - edited Hi, I've applied the patch for 2.2.x and worked great, thanks! It's a pity that this issue not been integrated just because it hasn't a way of easily remove meta linked courses (if it's this the reason), considering that the standard method neither has a deleting option and anyway, deleting can already be easily done from /enrol/instances.php Considering this, I would consider integrating this improvement as it is now, without waiting for a easy deleting improvement.
            Hide
            Iñigo Zendegi added a comment -

            Hi,

            I've found something in this patch that works different to the expected: in the standard way to add meta-linked courses, when a course is meta-linked it isn't shown any more on the "avalaible-to-add" course list, and using this patch a course is shown in the list even if it has been already linked so it can be linked twice (and this doesn't make sense, does it?).

            So, excluding from the course list those courses that had been already meta-linked could be a good improvement to the patch.

            Show
            Iñigo Zendegi added a comment - Hi, I've found something in this patch that works different to the expected: in the standard way to add meta-linked courses, when a course is meta-linked it isn't shown any more on the "avalaible-to-add" course list, and using this patch a course is shown in the list even if it has been already linked so it can be linked twice (and this doesn't make sense, does it?). So, excluding from the course list those courses that had been already meta-linked could be a good improvement to the patch.
            Hide
            Iñigo Zendegi added a comment - - edited

            Hi there,

            It seems like this issue got frozen the last months

            I don't know if this issue is considered a regression from 1.9 and it will be included in the core or not, any info about it would be appreciated.

            Anyway, meanwhile I've been testing the proposed patch and it works fine on Moodle 2.3.x too.

            I haven't tested it on 2.4 yet, but it seems like it will work too (I've seen that there are some changes on these files but it seems there will be no conflict with this patch).

            Show
            Iñigo Zendegi added a comment - - edited Hi there, It seems like this issue got frozen the last months I don't know if this issue is considered a regression from 1.9 and it will be included in the core or not, any info about it would be appreciated. Anyway, meanwhile I've been testing the proposed patch and it works fine on Moodle 2.3.x too. I haven't tested it on 2.4 yet, but it seems like it will work too (I've seen that there are some changes on these files but it seems there will be no conflict with this patch).
            Hide
            Bob Puffer added a comment -

            I had to do a bunch of meta course enrollments and just got tired of the this silly interface. The patch above didn't quite cut it for me so I took what we had in 1.9 (not pretty but highly functional) and made it work for 2.x. Have attached addinstance.php to replace the existing addinstance.php in enrol/meta. Give it a try if you like, I didn't gussy up the HTML form by implementing mform but it does what its supposed to do.

            Show
            Bob Puffer added a comment - I had to do a bunch of meta course enrollments and just got tired of the this silly interface. The patch above didn't quite cut it for me so I took what we had in 1.9 (not pretty but highly functional) and made it work for 2.x. Have attached addinstance.php to replace the existing addinstance.php in enrol/meta. Give it a try if you like, I didn't gussy up the HTML form by implementing mform but it does what its supposed to do.
            Hide
            Bob Puffer added a comment -

            replace enrol/meta/addinstance.php with this file after backing up original and you will have the 1.9 meta course enrollment back in full

            Show
            Bob Puffer added a comment - replace enrol/meta/addinstance.php with this file after backing up original and you will have the 1.9 meta course enrollment back in full
            Hide
            Bob Puffer added a comment - - edited

            New addinstance.php rewritten in html_writer syntax (which most stuff doesn't appear to use yet). Works with 2.3.x - 2.5. Hope we can move this along.

            Show
            Bob Puffer added a comment - - edited New addinstance.php rewritten in html_writer syntax (which most stuff doesn't appear to use yet). Works with 2.3.x - 2.5. Hope we can move this along.
            Hide
            Troy Williams added a comment -

            I have rehashed my original code to include a method to remove multiple linked courses. It doesn't use the get_courses_search($searchterms, $sort, $page, $recordsperpage, &$totalcount) function and only searches against shortname and fullname. I think would have to do custom version of that function for it to work. The code probably also needs a clean up, remove redundant stuff etc. Anyway code @

            https://github.com/troywilliams/moodle/commit/5583c30a414b3dc08ecd9c4ef82d220da233033d

            If people would like to test/provide feedback, and I'll try my best to fix/change... within reason of course

            Show
            Troy Williams added a comment - I have rehashed my original code to include a method to remove multiple linked courses. It doesn't use the get_courses_search($searchterms, $sort, $page, $recordsperpage, &$totalcount) function and only searches against shortname and fullname. I think would have to do custom version of that function for it to work. The code probably also needs a clean up, remove redundant stuff etc. Anyway code @ https://github.com/troywilliams/moodle/commit/5583c30a414b3dc08ecd9c4ef82d220da233033d If people would like to test/provide feedback, and I'll try my best to fix/change... within reason of course
            Hide
            Bob Puffer added a comment -

            Hi Troy,
            Have you taken a look at my addinstance.php?

            Show
            Bob Puffer added a comment - Hi Troy, Have you taken a look at my addinstance.php?
            Hide
            Troy Williams added a comment -

            @Robert

            I did drop addinstance.php into our test system this morning, got few missing string errors. e.g

            Invalid get_string() identifier: 'assigncourses' or component 'enrol_meta'. Perhaps you are missing $string['assigncourses'] = ''; in /var/www/html/elearn/enrol/meta/lang/en/enrol_meta.php?
            

            Have you seen http://docs.moodle.org/dev/Git_for_developers? Makes life a bit easier for sharing code.

            It pretty slow on big sites, below key performance indicators on addinstance.php load:

            12.039845 secs
            DB reads/writes: 18024/2
            

            The DB reads are pretty heavy.

            Long course names cause display issues under "standard" theme and maybe others.

            Show
            Troy Williams added a comment - @Robert I did drop addinstance.php into our test system this morning, got few missing string errors. e.g Invalid get_string() identifier: 'assigncourses' or component 'enrol_meta'. Perhaps you are missing $string['assigncourses'] = ''; in /var/www/html/elearn/enrol/meta/lang/en/enrol_meta.php? Have you seen http://docs.moodle.org/dev/Git_for_developers ? Makes life a bit easier for sharing code. It pretty slow on big sites, below key performance indicators on addinstance.php load: 12.039845 secs DB reads/writes: 18024/2 The DB reads are pretty heavy. Long course names cause display issues under "standard" theme and maybe others.
            Hide
            Bob Puffer added a comment -

            I do use the git but didn't for this one-off. But actually the latest, refactored version of this did add some lang strings so I think I'll probably branch and make a formal pull request. Perhaps you'd code review?

            Show
            Bob Puffer added a comment - I do use the git but didn't for this one-off. But actually the latest, refactored version of this did add some lang strings so I think I'll probably branch and make a formal pull request. Perhaps you'd code review?
            Hide
            Bob Puffer added a comment -

            I have a fork off from master that addresses the problem with how adding child courses to meta courses is handled (we revert back to the old way) and also auto-creating groups for child courses in the meta course. Because the former requires a (mostly) rewrite of enrol/meta/addinstance.php and that also would be where auto-groups would be created for the children, I've had to gang these two issues (this one and MDL-17929) together – I'm not going to maintain separate branches for such trivial code as required by the auto-groups. The auto-groups feature auto-populates the groups initially but does nothing for maintaining them as child course enrollments are updated (a significant effort). You can pick this up at https://github.com/bobpuffer/moodle_fork/tree/MDL-17929_27628-revert_meta_enrol-auto-groups_for_children

            Show
            Bob Puffer added a comment - I have a fork off from master that addresses the problem with how adding child courses to meta courses is handled (we revert back to the old way) and also auto-creating groups for child courses in the meta course. Because the former requires a (mostly) rewrite of enrol/meta/addinstance.php and that also would be where auto-groups would be created for the children, I've had to gang these two issues (this one and MDL-17929 ) together – I'm not going to maintain separate branches for such trivial code as required by the auto-groups. The auto-groups feature auto-populates the groups initially but does nothing for maintaining them as child course enrollments are updated (a significant effort). You can pick this up at https://github.com/bobpuffer/moodle_fork/tree/MDL-17929_27628-revert_meta_enrol-auto-groups_for_children
            Hide
            Bob Puffer added a comment -

            I think its pretty incredible this has been lingering for 2-1/2 years with a verifiable and code-checked solution sitting in the repo mentioned in my post. Testing:
            1. backup enrol/meta/addinstance.php
            2. replace enrol/meta/addinstance.php same file from repository
            3. add meta child courses to your heart's content

            Show
            Bob Puffer added a comment - I think its pretty incredible this has been lingering for 2-1/2 years with a verifiable and code-checked solution sitting in the repo mentioned in my post. Testing: 1. backup enrol/meta/addinstance.php 2. replace enrol/meta/addinstance.php same file from repository 3. add meta child courses to your heart's content
            Hide
            Mary Evans added a comment - - edited

            Hi Bob,
            Just saw this as I was just about to log out. If you reckon what you have added in your commit you referred to then why not create a new branch based on the latest version of Moodle (master) add your commit to it and then add it here for peer review?
            Cheers
            Mary

            Show
            Mary Evans added a comment - - edited Hi Bob, Just saw this as I was just about to log out. If you reckon what you have added in your commit you referred to then why not create a new branch based on the latest version of Moodle (master) add your commit to it and then add it here for peer review? Cheers Mary
            Hide
            Nadav Kavalerchik added a comment - - edited

            Please see if you can move this forward. Especially the auto-group feature.

            Show
            Nadav Kavalerchik added a comment - - edited Please see if you can move this forward. Especially the auto-group feature .
            Hide
            Charles Fulton added a comment -

            I've updated Troy's excellent patch to work against Moodle 2.7. I've pushed up a second commit which makes the following changes:

            1. Code-checker issues.
            2. Removed the "Search anywhere" box, so that the full text is always searched.
            3. Moved the "Link selected" button beneath the search results box.
            4. Added idnumber to the list of search fields.

            We did performance testing with a single user on a 40,000-course system and performance seemed fine (the standard course meta link interface actually runs out of resources and crashes on the same system).

            Show
            Charles Fulton added a comment - I've updated Troy's excellent patch to work against Moodle 2.7. I've pushed up a second commit which makes the following changes: Code-checker issues. Removed the "Search anywhere" box, so that the full text is always searched. Moved the "Link selected" button beneath the search results box. Added idnumber to the list of search fields. We did performance testing with a single user on a 40,000-course system and performance seemed fine (the standard course meta link interface actually runs out of resources and crashes on the same system).
            Hide
            CiBoT added a comment -

            Results for MDL-27628

            Show
            CiBoT added a comment - Results for MDL-27628 Remote repository: https://github.com/mackensen/moodle Remote branch MDL-27628 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/341 Warning: The MDL-27628 -master branch at https://github.com/mackensen/moodle has not been rebased recently. Details: http://integration.moodle.org/job/Precheck%20remote%20branch/341/artifact/work/smurf.html
            Hide
            Eric Merrill added a comment -

            Code comments:
            manage_form.php:
            Needs function scopes
            Lines 43 and 78 are too long
            Two empty lines at 67
            PHPdocs

            manage.php
            Not a fan of combing arrays with + (line 53_

            settings.php
            Lines 38 and 39 are too long

            locallib.php:
            PHPdocs on enrol_meta_linked_courses
            Array combining on line 600

            lang file:
            Out of order strings
            Why does it say ‘branch MOODLE_20_STABLE’?

            Show
            Eric Merrill added a comment - Code comments: manage_form.php: Needs function scopes Lines 43 and 78 are too long Two empty lines at 67 PHPdocs manage.php Not a fan of combing arrays with + (line 53_ settings.php Lines 38 and 39 are too long locallib.php: PHPdocs on enrol_meta_linked_courses Array combining on line 600 lang file: Out of order strings Why does it say ‘branch MOODLE_20_STABLE’?
            Hide
            Petr Skoda added a comment -

            Hello, I do not think the integration with current enrol UI is appropriate, there does not seem to be any need for yet another admin setting. Instead there should be a new way for enrol plugins to inject some buttons into the admin methods page without having any existing method in the course - I guess this should be done in a separate issue. I did not study the patch much, but it seems to me that the adding logic with relevant access control is reversed in the patch, users expect the 1.9 logic when linking one course (enrol/meta:selectaslinked) to multiple other courses (moodle/course:enrolconfig + enrol/meta:config + login access). My -1 for integration, sorry.

            Show
            Petr Skoda added a comment - Hello, I do not think the integration with current enrol UI is appropriate, there does not seem to be any need for yet another admin setting. Instead there should be a new way for enrol plugins to inject some buttons into the admin methods page without having any existing method in the course - I guess this should be done in a separate issue. I did not study the patch much, but it seems to me that the adding logic with relevant access control is reversed in the patch, users expect the 1.9 logic when linking one course (enrol/meta:selectaslinked) to multiple other courses (moodle/course:enrolconfig + enrol/meta:config + login access). My -1 for integration, sorry.
            Hide
            Ray Lawrence added a comment -

            Thanks for reviewing the proposed patch.

            What are the next steps? The number of watchers and votes suggests there is considerable interest in an improved UI.

            Show
            Ray Lawrence added a comment - Thanks for reviewing the proposed patch. What are the next steps? The number of watchers and votes suggests there is considerable interest in an improved UI.
            Hide
            Willy Lee added a comment -

            Petr, I'm having a hard time understanding what your feedback means.

            Are you suggesting that someone develop a hook or something to allow plugins to extend an enrollment method?

            Is that a separate notion from your access control comment? What do you think we need to do to get that worked out?

            Show
            Willy Lee added a comment - Petr, I'm having a hard time understanding what your feedback means. Are you suggesting that someone develop a hook or something to allow plugins to extend an enrollment method? Is that a separate notion from your access control comment? What do you think we need to do to get that worked out?
            Hide
            Petr Skoda added a comment -

            The current enrolment UI asks the enrolment plugins to add some buttons/info to the methods page and navigation tree only if there are instances present in the course. The problem with this issue is that it deals with enrolments in other courses, not the current course you are viewing. I would vote for adding a new way for enrol plugins to add buttons to the enrol UI even if they do not have any instances yet, because hijacking the add-instances button is not solution.

            MDL-44078 is one of the options we could use for advanced customisations of enrol UI and navigation...

            Show
            Petr Skoda added a comment - The current enrolment UI asks the enrolment plugins to add some buttons/info to the methods page and navigation tree only if there are instances present in the course. The problem with this issue is that it deals with enrolments in other courses, not the current course you are viewing. I would vote for adding a new way for enrol plugins to add buttons to the enrol UI even if they do not have any instances yet, because hijacking the add-instances button is not solution. MDL-44078 is one of the options we could use for advanced customisations of enrol UI and navigation...
            Hide
            Ray Lawrence added a comment -

            I don't understand why it's so hard to revert the selection to a multi-pick list like we had before and generate the enrollment instances from that. It seems that improvements made considered everything except the end user experience. sorry to be a grump but this interface is close to rendering it unusable in an implementation of any scale.

            Show
            Ray Lawrence added a comment - I don't understand why it's so hard to revert the selection to a multi-pick list like we had before and generate the enrollment instances from that. It seems that improvements made considered everything except the end user experience. sorry to be a grump but this interface is close to rendering it unusable in an implementation of any scale.
            Hide
            Petr Skoda added a comment -

            Feel free to implement it in any way, but please do not remove the current "simple"/standard way to add meta course links to course.

            Show
            Petr Skoda added a comment - Feel free to implement it in any way, but please do not remove the current "simple"/standard way to add meta course links to course.
            Hide
            Mark Pearson added a comment -

            Feel free to implement it in any way, but please do not remove the current "simple"/standard way to add meta course links to course.

            So Petr, are you saying that there should be two buttons : [Add a single metacourse] and [add multiple metacourses] ?

            The issue is, to put it bluntly, that the current user interface to repeatedly scroll through an unsorted list of all courses in order to select courses one by one is not fit for purpose.

            I would vote for adding a new way for enrol plugins to add buttons to the enrol UI even if they do not have any instances yet, because hijacking the add-instances button is not solution.

            Why is this not a solution? One is obviously badly needed, and this seems to work. Are you saying that this should await the addition of hooks (MDL-44078) at some unspecific future point?

            Show
            Mark Pearson added a comment - Feel free to implement it in any way, but please do not remove the current "simple"/standard way to add meta course links to course. So Petr, are you saying that there should be two buttons : [Add a single metacourse] and [add multiple metacourses] ? The issue is, to put it bluntly, that the current user interface to repeatedly scroll through an unsorted list of all courses in order to select courses one by one is not fit for purpose. I would vote for adding a new way for enrol plugins to add buttons to the enrol UI even if they do not have any instances yet, because hijacking the add-instances button is not solution. Why is this not a solution? One is obviously badly needed, and this seems to work. Are you saying that this should await the addition of hooks ( MDL-44078 ) at some unspecific future point?
            Hide
            Petr Skoda added a comment -

            Please lets keep things consistent, all actions on the page with enrolment methods affect enrolments of the current course only. This new feature proposed here should be in my opinion separate UI with own button or link.

            Show
            Petr Skoda added a comment - Please lets keep things consistent, all actions on the page with enrolment methods affect enrolments of the current course only. This new feature proposed here should be in my opinion separate UI with own button or link.
            Hide
            Bob Puffer added a comment -

            Petr if you don't have something else to offer then this conversation is so ridiculous its sad. The current metacourse enrollment is entirely unusable for any school with more than ten courses and you're talking about keeping things consistent. Lets please apply some common sense here.

            Show
            Bob Puffer added a comment - Petr if you don't have something else to offer then this conversation is so ridiculous its sad. The current metacourse enrollment is entirely unusable for any school with more than ten courses and you're talking about keeping things consistent. Lets please apply some common sense here.
            Hide
            Willy Lee added a comment -

            Please lets keep things consistent, all actions on the page with enrolment methods affect enrolments of the current course only.

            As this patch stands, it doesn't affect the enrolment of the other course, it affects the course you are in. If in course M, I add a Course meta link to course A, nothing really changes in course A. Course M now has the enrolments of course A included. This is functionally equivalent to the Course meta link behavior as it stand right now in master, it just adds a much more usable interface to do so.

            Show
            Willy Lee added a comment - Please lets keep things consistent, all actions on the page with enrolment methods affect enrolments of the current course only. As this patch stands, it doesn't affect the enrolment of the other course, it affects the course you are in. If in course M, I add a Course meta link to course A, nothing really changes in course A. Course M now has the enrolments of course A included. This is functionally equivalent to the Course meta link behavior as it stand right now in master, it just adds a much more usable interface to do so.
            Hide
            Mark Pearson added a comment -

            users expect the 1.9 logic when linking one course (enrol/meta:selectaslinked) to multiple other courses (moodle/course:enrolconfig + enrol/meta:config + login access). My -1 for integration, sorry.

            May I also point out that this seems to be rather a misunderstanding of what is proposed here. As far as I can see the outcome of this patch is exactly the same as the current functionality – all that changes is that the user interface goes from clumsy, error prone, tedious and non-scalable to being, well, usable. I don't see how this is connected with users expecting 1.9 logic.

            Show
            Mark Pearson added a comment - users expect the 1.9 logic when linking one course (enrol/meta:selectaslinked) to multiple other courses (moodle/course:enrolconfig + enrol/meta:config + login access). My -1 for integration, sorry. May I also point out that this seems to be rather a misunderstanding of what is proposed here. As far as I can see the outcome of this patch is exactly the same as the current functionality – all that changes is that the user interface goes from clumsy, error prone, tedious and non-scalable to being, well, usable. I don't see how this is connected with users expecting 1.9 logic.
            Hide
            Petr Skoda added a comment -

            Oh, sorry Mark, I misread the patch, you are right the patch in https://github.com/mackensen/moodle/compare/MDL-27628-master adds students from other courses to current course. Anyway I do not want to participate in this discussion, I am not the maintainer of the enrol subsystem any more. The patch is using deprecated coding style for javascript, old PHP4 OOP with =&, language strings are not sorted, the $search is not treated as plaintext (potential security issue if sesskey was missing somewhere), sql query indentation is not standard, I am not sure about the page types (we did some changes in 2.6 during the introduction of dynamically loaded admin settings), enrol_meta_course_search() seems to leak data because it does not have access control on current course and raw course data, etc. My personal +1 to the general direction of the patch, but -1 for actual implementation. Thanks everybody and ciao.

            Show
            Petr Skoda added a comment - Oh, sorry Mark, I misread the patch, you are right the patch in https://github.com/mackensen/moodle/compare/MDL-27628-master adds students from other courses to current course. Anyway I do not want to participate in this discussion, I am not the maintainer of the enrol subsystem any more. The patch is using deprecated coding style for javascript, old PHP4 OOP with =&, language strings are not sorted, the $search is not treated as plaintext (potential security issue if sesskey was missing somewhere), sql query indentation is not standard, I am not sure about the page types (we did some changes in 2.6 during the introduction of dynamically loaded admin settings), enrol_meta_course_search() seems to leak data because it does not have access control on current course and raw course data, etc. My personal +1 to the general direction of the patch, but -1 for actual implementation. Thanks everybody and ciao.
            Hide
            Michael de Raadt added a comment -

            This issue is of interest two a large group of people.

            There are two problems I see here...

            1. The issue seems to have drifted from the original desire to have the behaviour from Moodle 1.9 to a slightly different behaviour. It would be good if we people could note the differences and state whether they are happy with the behaviour shared in the provided patch.
            2. Petr has offered some suggestions for improving the code in the provided patch in his last comment, which would need to be fixed before the change goes further through the development process.

            When there is agreement on the behaviour of the interface and code has been updated, this could be put up for peer review again.

            Show
            Michael de Raadt added a comment - This issue is of interest two a large group of people. There are two problems I see here... The issue seems to have drifted from the original desire to have the behaviour from Moodle 1.9 to a slightly different behaviour. It would be good if we people could note the differences and state whether they are happy with the behaviour shared in the provided patch. Petr has offered some suggestions for improving the code in the provided patch in his last comment, which would need to be fixed before the change goes further through the development process. When there is agreement on the behaviour of the interface and code has been updated, this could be put up for peer review again.
            Hide
            Ray Lawrence added a comment -

            Only one as I see it... the issue I reported originally.

            The interface to select courses that are enrolled in the course with meta course enrolments enabled needs to be capable of selecting more than one course at a time.

            This isn't a general request to revert to the way metacourses worked prior to the 2.x

            Show
            Ray Lawrence added a comment - Only one as I see it... the issue I reported originally. The interface to select courses that are enrolled in the course with meta course enrolments enabled needs to be capable of selecting more than one course at a time. This isn't a general request to revert to the way metacourses worked prior to the 2.x
            Hide
            Elizabeth Dalton added a comment -

            Please, could we at least get a way to search for courses to add as meta links! I wouldn't mind having to do them individually if we could get that. We have thousands of courses on our server at this point, and every term we have to create meta links for a couple of them. Our hosting provider won't patch core, so we need some kind of fix integrated in core itself. Please, let's not let "perfect" be the enemy of "good" on this.

            Show
            Elizabeth Dalton added a comment - Please, could we at least get a way to search for courses to add as meta links! I wouldn't mind having to do them individually if we could get that. We have thousands of courses on our server at this point, and every term we have to create meta links for a couple of them. Our hosting provider won't patch core, so we need some kind of fix integrated in core itself. Please, let's not let "perfect" be the enemy of "good" on this.
            Hide
            Troy Williams added a comment -

            If anyone interested I have put up initial code for a local plugin "bulkmeta" for Moodle 2.7.It contains search and multi link/unlink functionality. It is dependant on course meta enrolment plugin. Uses the navigation settings extends call to place a link under "Course administration" -> "Users"

            Free free to test and post up issues on relevant github page please. Repo located @

            https://github.com/troywilliams/moodle-local_bulkmeta

            Keep smiling

            Show
            Troy Williams added a comment - If anyone interested I have put up initial code for a local plugin "bulkmeta" for Moodle 2.7.It contains search and multi link/unlink functionality. It is dependant on course meta enrolment plugin. Uses the navigation settings extends call to place a link under "Course administration" -> "Users" Free free to test and post up issues on relevant github page please. Repo located @ https://github.com/troywilliams/moodle-local_bulkmeta Keep smiling
            Hide
            Chad Bergeron added a comment -

            Troy,
            Great work on making this functionality a local plugin. Testing it out now with good results, and I've tossed a few things onto the github issues page.

            Show
            Chad Bergeron added a comment - Troy, Great work on making this functionality a local plugin. Testing it out now with good results, and I've tossed a few things onto the github issues page.
            Hide
            CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-27628 using repository: https://github.com/mackensen/moodle

            More information about this report

            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-27628 using repository: https://github.com/mackensen/moodle master [branch: MDL-27628-master | CI Job ] Info: the branch is based off moodle.git Info: base commit 2d84748c263083c76c38202d696dac2086e3668e being used as initial commit. Error: The MDL-27628 -master branch at https://github.com/mackensen/moodle is very old (>60 days ago). Please rebase against current master. More information about this report
            Hide
            Frédéric Massart added a comment -

            Hi everyone,

            I'm sorry it took so long for someone to have a closer look at the patch offered here. I'm also sorry to bring the bad news that I don't think it is heading in the right direction. While I think that the local plugin offered by Troy is a nice workaround to the issue, I don't think the same can be applied to the meta enrolment plugin itself.

            Mainly, I have two major concerns.

            The first one is in regard with performance, we could certainly update the meta plugin to support more than one course per instance, and thus doing a lot more with one database query.

            The second concern is the UI. Surely it is a lot easier to manage instances now, but the manage action should not be associated with each instance. When you click manage you expect to manage that instance but you effectively manage all the instances. I was surprised ending up with 60 meta instances after managing one.

            What I would suggest is to:

            1. Support more than one course per instance
            2. Multiple courses can be linked upon creation
            3. Rely on an edit button to change the linked courses
            4. Using a similar selector to the one proposed in the patch, but maybe more abstracted so that it could be re-used throughout core (like the user_selector).

            I have removed the assignee and patch label from this issue to give a chance to anyone to become the assignee and carry on with the work, including one of us at HQ.

            Meantime, I am hoping that MDL-32161 will get more attention as it would greatly help if there was a way to search for courses.

            Thanks,
            Fred

            Show
            Frédéric Massart added a comment - Hi everyone, I'm sorry it took so long for someone to have a closer look at the patch offered here. I'm also sorry to bring the bad news that I don't think it is heading in the right direction. While I think that the local plugin offered by Troy is a nice workaround to the issue, I don't think the same can be applied to the meta enrolment plugin itself. Mainly, I have two major concerns. The first one is in regard with performance, we could certainly update the meta plugin to support more than one course per instance, and thus doing a lot more with one database query. The second concern is the UI. Surely it is a lot easier to manage instances now, but the manage action should not be associated with each instance. When you click manage you expect to manage that instance but you effectively manage all the instances. I was surprised ending up with 60 meta instances after managing one. What I would suggest is to: Support more than one course per instance Multiple courses can be linked upon creation Rely on an edit button to change the linked courses Using a similar selector to the one proposed in the patch, but maybe more abstracted so that it could be re-used throughout core (like the user_selector). I have removed the assignee and patch label from this issue to give a chance to anyone to become the assignee and carry on with the work, including one of us at HQ. Meantime, I am hoping that MDL-32161 will get more attention as it would greatly help if there was a way to search for courses. Thanks, Fred
            Hide
            Mark Pearson added a comment -

            So basically after 7 months we're back to square one? Is that what you're saying?

            Show
            Mark Pearson added a comment - So basically after 7 months we're back to square one? Is that what you're saying?
            Hide
            Mary Evans added a comment -

            Quote:
            "...Meantime, I am hoping that MDL-32161 will get more attention as it would greatly help if there was a way to search for courses..."
            

            I thought there was a way to search for courses?

            Show
            Mary Evans added a comment - Quote: "...Meantime, I am hoping that MDL-32161 will get more attention as it would greatly help if there was a way to search for courses..." I thought there was a way to search for courses?
            Hide
            Marina Glancy added a comment -

            Hello,
            this issue (and MDL-32161) have lots of votes and they are on the radar of Moodle HQ. Unfortunately this particular patch is not ready enough for integration as it is so the issue remains in the backlog at the moment. Rejecting the review of the code does not mean rejecting of the issue itself.

            Also I want to remind you of the possibility of developing your own enrolment plugin to replace the standard enrol_meta or complement it. Plugins database already has plenty of registered enrolment plugins

            Show
            Marina Glancy added a comment - Hello, this issue (and MDL-32161 ) have lots of votes and they are on the radar of Moodle HQ. Unfortunately this particular patch is not ready enough for integration as it is so the issue remains in the backlog at the moment. Rejecting the review of the code does not mean rejecting of the issue itself. Also I want to remind you of the possibility of developing your own enrolment plugin to replace the standard enrol_meta or complement it. Plugins database already has plenty of registered enrolment plugins
            Hide
            Charles Fulton added a comment -

            Hi Marina, I think we all appreciate the feedback but maintaining or installing a different enrollment plugin and taking on all that associated technical debt isn't an ideal solution. It's frustrating, particularly for small schools which have this use case for manual metalinks, to be left without a reasonable alternative. I would be happy to put the work in to improve the patch if I thought there was some prospect that it would be accepted for integration but Frederic's feedback leaves me dead in the water without any real incentive to develop the patch further. Over the years many people from core have left feedback to the effect "don't do it this way." If everyone's so certain about how it shouldn't be done would it be asking too much for someone at core to tell us how it should be done and provide a code solution? I think I'm done trying for a while.

            Show
            Charles Fulton added a comment - Hi Marina, I think we all appreciate the feedback but maintaining or installing a different enrollment plugin and taking on all that associated technical debt isn't an ideal solution. It's frustrating, particularly for small schools which have this use case for manual metalinks, to be left without a reasonable alternative. I would be happy to put the work in to improve the patch if I thought there was some prospect that it would be accepted for integration but Frederic's feedback leaves me dead in the water without any real incentive to develop the patch further. Over the years many people from core have left feedback to the effect "don't do it this way." If everyone's so certain about how it shouldn't be done would it be asking too much for someone at core to tell us how it should be done and provide a code solution? I think I'm done trying for a while.
            Hide
            Willy Lee added a comment -

            Most of the feedback here also applies to the core functionality as it is currently delivered.

            Let's say I have a Lab Safety Course and I want to meta-enroll every chemistry course to it so they can watch a video and do a quiz.

            Currently in core, I would go to the enrollment methods page and add a meta-enrollment by scrolling down a long list of shortnames. Once that was done, I would end up back on the enrollment methods page and need to go through that process again. If I was looking for all the chemistry classes, they could be sorted by shortname, sort order, courseid, or fullname, while I can only see the fullname. This becomes pretty difficult to use when we don't even show the field we are searching by in most cases.

            Currently in core, if you have linked twenty five courses, you end up with twenty five entries on the enrollment methods page. I'm not sure what database call Frédéric Massart is referring to, but I can't see how the patch is any different than core in number of queries.

            Show
            Willy Lee added a comment - Most of the feedback here also applies to the core functionality as it is currently delivered. Let's say I have a Lab Safety Course and I want to meta-enroll every chemistry course to it so they can watch a video and do a quiz. Currently in core, I would go to the enrollment methods page and add a meta-enrollment by scrolling down a long list of shortnames. Once that was done, I would end up back on the enrollment methods page and need to go through that process again. If I was looking for all the chemistry classes, they could be sorted by shortname, sort order, courseid, or fullname, while I can only see the fullname. This becomes pretty difficult to use when we don't even show the field we are searching by in most cases. Currently in core, if you have linked twenty five courses, you end up with twenty five entries on the enrollment methods page. I'm not sure what database call Frédéric Massart is referring to, but I can't see how the patch is any different than core in number of queries.
            Hide
            Willy Lee added a comment -

            Currently, several enrollment methods allow you to add multiple instances of them. Even ignoring the ones available as plugins, just in core, you have PayPal, self enrollment, and course meta link. As an architecture, the enrollment plugin suggests that this is the correct behavior.

            If we move to a model where managing one type of enrollment method in a course gave you an interface to manage all of these instances, this becomes even more complicated since methods like PayPal and Self Enrollment each have several settings per instance.

            I'm not opposed to doing so, but I want everyone in this discussion to realize some of the ramifications if we keep down that path.

            Show
            Willy Lee added a comment - Currently, several enrollment methods allow you to add multiple instances of them. Even ignoring the ones available as plugins, just in core, you have PayPal, self enrollment, and course meta link. As an architecture, the enrollment plugin suggests that this is the correct behavior. If we move to a model where managing one type of enrollment method in a course gave you an interface to manage all of these instances, this becomes even more complicated since methods like PayPal and Self Enrollment each have several settings per instance. I'm not opposed to doing so, but I want everyone in this discussion to realize some of the ramifications if we keep down that path.
            Hide
            Damyon Wiese added a comment -

            I think - first of all, the changes Fred has described are not so big - it's not throwing this patch away - it's just improving it further.

            There are some process things here that may have confused people - the reason we remove the patch label is because we have looked at the patch and it's not ready without further work. The reason we removed the assignee is because sadly this patch has sat waiting for review for a long time and we could not be sure that Charles would still be willing to work on it - and this would allow someone else to pick it up (or Charles to pick it up again if he is still interested).

            Meta courses are something specifically that it would be common to have many instances of - and showing each instance separately does seem to be a poor UI as well as preventing us from optimising the queries. Fred was not comparing this to patch to core and saying it is any better now - he is saying that we should fix this properly and create the best solution.

            Finally - this issue does seem very important and we are still looking at it.

            Show
            Damyon Wiese added a comment - I think - first of all, the changes Fred has described are not so big - it's not throwing this patch away - it's just improving it further. There are some process things here that may have confused people - the reason we remove the patch label is because we have looked at the patch and it's not ready without further work. The reason we removed the assignee is because sadly this patch has sat waiting for review for a long time and we could not be sure that Charles would still be willing to work on it - and this would allow someone else to pick it up (or Charles to pick it up again if he is still interested). Meta courses are something specifically that it would be common to have many instances of - and showing each instance separately does seem to be a poor UI as well as preventing us from optimising the queries. Fred was not comparing this to patch to core and saying it is any better now - he is saying that we should fix this properly and create the best solution. Finally - this issue does seem very important and we are still looking at it.
            Hide
            Mark Pearson added a comment -

            Fred was not comparing this to patch to core and saying it is any better now - he is saying that we should fix this properly and create the best solution.

            But the problem, Damyon, is that, as Willy Lee has pointed out, there is little consistency here and it just feels like this patch is being singled out. Furthermore, volunteer developers are understandably frustrated when the Core developers offer criticism but no concrete solution. There are over 100 votes for this feature which has been on the radar for years and long suffering administrators can't help feeling that in this case "the best is the enemy of the good".

            We look forward to a proper fix and the best solution from Moodle HQ.

            Show
            Mark Pearson added a comment - Fred was not comparing this to patch to core and saying it is any better now - he is saying that we should fix this properly and create the best solution. But the problem, Damyon, is that, as Willy Lee has pointed out, there is little consistency here and it just feels like this patch is being singled out. Furthermore, volunteer developers are understandably frustrated when the Core developers offer criticism but no concrete solution. There are over 100 votes for this feature which has been on the radar for years and long suffering administrators can't help feeling that in this case "the best is the enemy of the good". We look forward to a proper fix and the best solution from Moodle HQ.
            Hide
            Willy Lee added a comment -

            Help me understand what we're looking for here. I'm not against a change, but I'm having trouble thinking this all the way through.

            Currently, the enrollment methods instance page shows a HTML row for each database row in mdl_enrol. For a course with a number of meta course links, there would be one HTML row for each of those meta course links. This is also the case for self enrollment, paypal, and possibly other enrollment methods.

            Is the suggestion that we change this to have a single HTML row on this page per type of enrollment method? If that's the case, should we change the number of users HTML column? Should we add a HTML column to show how many of that method type there are (e.g. 5 meta course links)?

            There's currently a sort order to enrollment methods. If we group these together, we complicate that sorting. For example, this interface wouldn't handle an ordered list of meta1, ldap, meta2. Not that I have a great use case for this, just wanted to raise this in case someone else sees an issue here (in fact, I don't actually know how enrollment method sort order affects anything besides this display.)

            Should that be reflected in having a singular database row in the mdl_enrol table for each course/method combination? In this case, do we need to create mdl_enrol_PLUGINNAME tables for each of those that could have multiple configurations? I don't think it's ideal to try to jam these into the custom fields in mdl_enrol. Should those have a sort order?

            Show
            Willy Lee added a comment - Help me understand what we're looking for here. I'm not against a change, but I'm having trouble thinking this all the way through. Currently, the enrollment methods instance page shows a HTML row for each database row in mdl_enrol. For a course with a number of meta course links, there would be one HTML row for each of those meta course links. This is also the case for self enrollment, paypal, and possibly other enrollment methods. Is the suggestion that we change this to have a single HTML row on this page per type of enrollment method? If that's the case, should we change the number of users HTML column? Should we add a HTML column to show how many of that method type there are (e.g. 5 meta course links)? There's currently a sort order to enrollment methods. If we group these together, we complicate that sorting. For example, this interface wouldn't handle an ordered list of meta1, ldap, meta2. Not that I have a great use case for this, just wanted to raise this in case someone else sees an issue here (in fact, I don't actually know how enrollment method sort order affects anything besides this display.) Should that be reflected in having a singular database row in the mdl_enrol table for each course/method combination? In this case, do we need to create mdl_enrol_PLUGINNAME tables for each of those that could have multiple configurations? I don't think it's ideal to try to jam these into the custom fields in mdl_enrol. Should those have a sort order?
            Hide
            Damyon Wiese added a comment -

            Hi Willy,

            I don't think it should work like this for all enrolment methods, but I think it makes sense for the meta enrolment, because:

            a) it would be common to have lots of instances of this enrolment method for one course
            b) the only difference between each of the instances is the course link

            For comparison - I do not think it makes sense, e.g. for paypal - because exactly the opposite is true:

            a) you are not likely to have many different instances of this for the same course
            b) if you do - the config for each will be quite different

            In terms of the DB - yes there would probably be only one row in mdl_enrol and the list of courses could be stored either in one of the custom fields (probably not feasible), or a new table linked by the instanceid (mdl_enrol_meta). Again - we are only talking about meta enrolments, not all the other enrolment methods.

            Show
            Damyon Wiese added a comment - Hi Willy, I don't think it should work like this for all enrolment methods, but I think it makes sense for the meta enrolment, because: a) it would be common to have lots of instances of this enrolment method for one course b) the only difference between each of the instances is the course link For comparison - I do not think it makes sense, e.g. for paypal - because exactly the opposite is true: a) you are not likely to have many different instances of this for the same course b) if you do - the config for each will be quite different In terms of the DB - yes there would probably be only one row in mdl_enrol and the list of courses could be stored either in one of the custom fields (probably not feasible), or a new table linked by the instanceid (mdl_enrol_meta). Again - we are only talking about meta enrolments, not all the other enrolment methods.
            Hide
            Willy Lee added a comment -

            Currently, figuring out who is enrolled in a course joins mdl_user to mdl_user_enrolments to mdl_enrol to mdl_course. In this proposal, is there any reason why we'd need to join the new mdl_enrol_meta? Is that granularity important at any point and what performance cost are we paying in this join? As it is, I could see an opportunity for optimization by keeping a copy of courseid in mdl_user_enrolments.

            Show
            Willy Lee added a comment - Currently, figuring out who is enrolled in a course joins mdl_user to mdl_user_enrolments to mdl_enrol to mdl_course. In this proposal, is there any reason why we'd need to join the new mdl_enrol_meta? Is that granularity important at any point and what performance cost are we paying in this join? As it is, I could see an opportunity for optimization by keeping a copy of courseid in mdl_user_enrolments.
            Hide
            Willy Lee added a comment -

            Damyon Wiese, can you give us some more feedback on this, we'd like to come back to this issue in the next few days.

            Currently, you can set the order of enrolment methods to metalink to A, manual enrolment, and metalink to B. If we put all metalinks together, that's impossible. Is that a problem?

            If all metalinks end up collapsed an in a different setting page, what should the row look like. Currently we have name, users, up/down, edit (which could include, delete, hide, edit, and a few other things.) Should the users column total all the users in each metalink? Does the consolidated metalink row have a delete and hide? What would that do? Remove/hide all metalinks?

            I want to be clear that I'm not raising these to obstruct doing something. I'm honestly looking for guidance.

            If we decide that those are too complex, would core consider a changeset that simply allowed an AJAX search for a course in the enrol/meta/addinstance.php ? Would it be ok to allow a user to select several on that page? I'd be OK with addressing the editing of existing links at some other point.

            Show
            Willy Lee added a comment - Damyon Wiese , can you give us some more feedback on this, we'd like to come back to this issue in the next few days. Currently, you can set the order of enrolment methods to metalink to A, manual enrolment, and metalink to B. If we put all metalinks together, that's impossible. Is that a problem? If all metalinks end up collapsed an in a different setting page, what should the row look like. Currently we have name, users, up/down, edit (which could include, delete, hide, edit, and a few other things.) Should the users column total all the users in each metalink? Does the consolidated metalink row have a delete and hide? What would that do? Remove/hide all metalinks? I want to be clear that I'm not raising these to obstruct doing something. I'm honestly looking for guidance. If we decide that those are too complex, would core consider a changeset that simply allowed an AJAX search for a course in the enrol/meta/addinstance.php ? Would it be ok to allow a user to select several on that page? I'd be OK with addressing the editing of existing links at some other point.
            Hide
            Jon Witts added a comment -

            Willy,

            In my opinion the current UI which displays the order of enrolment methods is fine; as each metalink is a separate enrolment it makes sense to me to keep them displayed as they are currently. However what is a real bind to use when adding multiple metalink enrolments is the fact that you have to choose them one at a time.

            If the UI for adding a metalink enrolment could be changed so that you had the ability to chose multiple courses; then when you clicked "add" it added each in as a separate row in the enrolments page... that would work for me....

            I can see perhaps the need to have the ability to remove all metalink enrolments as well as edit / hide individual ones; but that should not be too hard to include in the UI... perhaps a hide / delete link for all metalink next to the first instance... something like this?

            Jon

            Show
            Jon Witts added a comment - Willy, In my opinion the current UI which displays the order of enrolment methods is fine; as each metalink is a separate enrolment it makes sense to me to keep them displayed as they are currently. However what is a real bind to use when adding multiple metalink enrolments is the fact that you have to choose them one at a time. If the UI for adding a metalink enrolment could be changed so that you had the ability to chose multiple courses; then when you clicked "add" it added each in as a separate row in the enrolments page... that would work for me.... I can see perhaps the need to have the ability to remove all metalink enrolments as well as edit / hide individual ones; but that should not be too hard to include in the UI... perhaps a hide / delete link for all metalink next to the first instance... something like this? Jon
            Hide
            Charles Fulton added a comment -

            I've refactored the patch per Willy's suggestion. This is overall a slimmer solution, which does the following:

            1. Allows adding multiple enrolments at once.
            2. Replaces the existing search with an AJAX search.

            Compared the original version which has been discussed here, it does not do the following:

            1. Allow unlinking multiple course meta link enrolments.
            2. Allow editing a course meta link enrolment from another enrolment.
            3. Add a secondary form interface (manage/manage_form). Everything is in addinstance/addinstance_form

            Hopefully this provides a way forward.

            Show
            Charles Fulton added a comment - I've refactored the patch per Willy's suggestion. This is overall a slimmer solution, which does the following: Allows adding multiple enrolments at once. Replaces the existing search with an AJAX search. Compared the original version which has been discussed here, it does not do the following: Allow unlinking multiple course meta link enrolments. Allow editing a course meta link enrolment from another enrolment. Add a secondary form interface (manage/manage_form). Everything is in addinstance/addinstance_form Hopefully this provides a way forward.
            Hide
            CiBoT added a comment -
            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-27628 using repository: https://github.com/mackensen/moodle master (8 errors / 6 warnings) [branch: wip-MDL-27628-master | CI Job ] phplint (0/0) , php (4/0) , js (0/5) , css (0/0) , phpdoc (3/1) , commit (1/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            Willy Lee added a comment -

            Thanks Charles Fulton, I think that does a very good job of improving the interface for adding metalinks but doesn't lead to any confusion when editing that Frédéric Massart brought up.

            Show
            Willy Lee added a comment - Thanks Charles Fulton , I think that does a very good job of improving the interface for adding metalinks but doesn't lead to any confusion when editing that Frédéric Massart brought up.
            Hide
            CiBoT added a comment -
            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-27628 using repository: https://github.com/mackensen/moodle master (1 errors / 0 warnings) [branch: wip-MDL-27628-master | CI Job ] phplint (0/0) , php (0/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (1/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            Damyon Wiese added a comment -

            Thanks again Charles for persisting with this. I had a look at this solution with Fred, as well as the alternative patch on MDL-40270 which achieves the same thing in a different way - and we think that each solution has different pros / cons.

            What we will do from here is mark this issue so it's added to the next sprint and we will combine the 2 solutions to produce the best result and add a bit of polish where it's needed. The areas needing polish are in the accessibility and styling of the ajax search. The alternative solution also makes this form input type a generic thing which could be reused which sounds like a good idea.

            I will also mark the other issue as a duplicate of this.

            With regards to editing multiple instances - looking at this patch I think we have arrived at an reasonable solution here. You can easily add multiple, but from that point they are managed individually. In a separate issue we could look at adding some things like being able to delete multiple instances at once - if people find this is something they do often.

            Show
            Damyon Wiese added a comment - Thanks again Charles for persisting with this. I had a look at this solution with Fred, as well as the alternative patch on MDL-40270 which achieves the same thing in a different way - and we think that each solution has different pros / cons. What we will do from here is mark this issue so it's added to the next sprint and we will combine the 2 solutions to produce the best result and add a bit of polish where it's needed. The areas needing polish are in the accessibility and styling of the ajax search. The alternative solution also makes this form input type a generic thing which could be reused which sounds like a good idea. I will also mark the other issue as a duplicate of this. With regards to editing multiple instances - looking at this patch I think we have arrived at an reasonable solution here. You can easily add multiple, but from that point they are managed individually. In a separate issue we could look at adding some things like being able to delete multiple instances at once - if people find this is something they do often.
            Hide
            Kevin Wiliarty added a comment -

            For future reference: Deleting and adding multiple metalinks is absolutely something I do often. Here's the scenario: One of my departments has a Moodle course with information for anyone currently taking any class in the department. We turn over forty or fifty course metalinks every semester. They can all be captured nicely and uniquely with an Ajax search. Deleting them individually is not nearly as painful as trying to add them from a dropdown menu, but still...

            Show
            Kevin Wiliarty added a comment - For future reference: Deleting and adding multiple metalinks is absolutely something I do often. Here's the scenario: One of my departments has a Moodle course with information for anyone currently taking any class in the department. We turn over forty or fifty course metalinks every semester. They can all be captured nicely and uniquely with an Ajax search. Deleting them individually is not nearly as painful as trying to add them from a dropdown menu, but still...
            Hide
            Damyon Wiese added a comment -

            This has been added to the current HQ sprint and will get picked up soon (as noted above).

            Show
            Damyon Wiese added a comment - This has been added to the current HQ sprint and will get picked up soon (as noted above).
            Hide
            Gilles-Philippe Leblanc added a comment -

            We also waiting for this great improvement. Damyon Wiese, Does this task is still on the current HQ sprint ?

            Show
            Gilles-Philippe Leblanc added a comment - We also waiting for this great improvement. Damyon Wiese , Does this task is still on the current HQ sprint ?

              Dates

              • Created:
                Updated:

                Agile