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 to enable both course meta links and the new AJAX interface:

      1. In Site Administration > Enrolments > Manage enrol plugins, enable Course Meta Link.
      2. In Site Administration > Course meta link, set "Use manage course meta link UI" to yes.

      Test adding multiple courses:

      1. In a course, access Course Administration > Users > Enrollment methods. Choose Course Meta Link.
      2. Verify that you have lists of both linked and 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 the page reloads with the expected changes in the two search boxes.
      5. Click "Cancel" to return to the main enrolment instance page. Verify that the courses you selected are now listed with the "Course meta link" enrolment method.

      Test removing multiple courses:

      1. Choose "Course Meta Link" from the drop-down again.
      2. Select two or more linked courses and click "Unlink selected". Verify that the page reloads with the expected changes in the two search boxes.
      3. Click "Cancel" to return to the main enrolment instance page. Verify that the courses you selected are no longer listed with the "Course meta link" enrolment method.
      Show
      To test this you will need multiple courses with enroled users. You will then to enable both course meta links and the new AJAX interface: In Site Administration > Enrolments > Manage enrol plugins, enable Course Meta Link. In Site Administration > Course meta link, set "Use manage course meta link UI" to yes. Test adding multiple courses: In a course, access Course Administration > Users > Enrollment methods. Choose Course Meta Link. Verify that you have lists of both linked and 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 the page reloads with the expected changes in the two search boxes. Click "Cancel" to return to the main enrolment instance page. Verify that the courses you selected are now listed with the "Course meta link" enrolment method. Test removing multiple courses: Choose "Course Meta Link" from the drop-down again. Select two or more linked courses and click "Unlink selected". Verify that the page reloads with the expected changes in the two search boxes. Click "Cancel" to return to the main enrolment instance page. Verify that the courses you selected are no longer listed with the "Course meta link" enrolment method.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_23_STABLE, MOODLE_27_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-27628-master
    • Rank:
      45

      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.

      1. addinstance.php
        9 kB
        Robert 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 Škoda 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 Škoda 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
          Robert 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
          Robert 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
          Robert 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
          Robert 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
          Robert 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
          Robert 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
          Robert Puffer added a comment -

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

          Show
          Robert 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
          Robert 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
          Robert 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
          Robert 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
          Robert 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
          Robert 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
          Robert 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 Škoda 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 Škoda 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 Škoda 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 Škoda 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 Škoda 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 Škoda 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 Škoda 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 Škoda 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
          Robert 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
          Robert 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 Škoda 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 Škoda 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

            Dates

            • Created:
              Updated: