Moodle
  1. Moodle
  2. MDL-8307

Add Course category to new course request

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.7.2, 1.8, 2.4
    • Fix Version/s: 2.4
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide

      Test pre-requisites

      • Testing preferably across the DB types as there is added field.
      • Do not upgrade your Moodle, you'll have to test the upgrade script during the first test
      • A manager on a site level
      • Remove the capability moodle/course:changecategory from the manager on a site level
      • Create a few categories
      • Enable course requests enablecourserequests
      • Set the default category (defaultrequestcategory) to something else than Miscellaneous
      • Create a course request called 'I was here before'

      Test 1

      1. Upgrade Moodle to the latest
      2. As an admin, check the page course/pending.php
      3. Make sure that the course request 'I was here before' has the default category
      4. As a user, create 3 other course requests (course/request.php) with different categories than the default one
      5. As an admin, back in course/pending.php, make sure you can see the categories of the requests
      6. Approve 'I was here before' and make sure the category set is the default one
      7. Approve another request make sure the category set is the right one
      8. Login as your manager and go to course/pending.php
      9. Make sure the category displayed is now the default one, not the one set when the course request was created
      10. Approve one of them, and make sure it is created under the default category
      11. As an admin, give the permission defaultrequestcategory to your manager
      12. Back with your manager, make sure you can now approve the course request with the category it was requested with

      Test 2

      1. Install a new instance of the latest version of Moodle
      2. Make sure the install works
      3. Create a category
      4. Enable the course requests
      5. Make sure you can create a new course request in the new category
      6. Make sure the admin can see the new category in the pending request page
      Show
      Test pre-requisites Testing preferably across the DB types as there is added field. Do not upgrade your Moodle, you'll have to test the upgrade script during the first test A manager on a site level Remove the capability moodle/course:changecategory from the manager on a site level Create a few categories Enable course requests enablecourserequests Set the default category ( defaultrequestcategory ) to something else than Miscellaneous Create a course request called 'I was here before' Test 1 Upgrade Moodle to the latest As an admin, check the page course/pending.php Make sure that the course request 'I was here before' has the default category As a user, create 3 other course requests ( course/request.php ) with different categories than the default one As an admin, back in course/pending.php , make sure you can see the categories of the requests Approve 'I was here before' and make sure the category set is the default one Approve another request make sure the category set is the right one Login as your manager and go to course/pending.php Make sure the category displayed is now the default one, not the one set when the course request was created Approve one of them, and make sure it is created under the default category As an admin, give the permission defaultrequestcategory to your manager Back with your manager, make sure you can now approve the course request with the category it was requested with Test 2 Install a new instance of the latest version of Moodle Make sure the install works Create a category Enable the course requests Make sure you can create a new course request in the new category Make sure the admin can see the new category in the pending request page
    • Affected Branches:
      MOODLE_17_STABLE, MOODLE_18_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-8307-master
    • Rank:
      3451

      Description

      In course request form, the ability to also suggest a existing course category (eg. via dropdown menu) would be advantageous. The default category for requested courses should be proposed as the default choice, but i can think of many instances where choosing the correct category would be good.

        Issue Links

          Activity

          Hide
          Patrick Pollet added a comment -

          Please find enclosed a patch to Moodle 1.72 course files to allow :

          -added a drop down of current categories with the config default category selected

          -added a field for course idnumber

          this required adding to mdl_course_request table :

          category int(11) Non 0 Pp categorie ou inserer le cours
          idnumber varchar(64) Oui NULL PP code ECTS

          as per http://tracker.moodle.org/browse/MDL-6160
          the following code should also be added to admin/cron.php

          mtrace("Checking course creation requests");
          if ($pending = get_records("course_request"))

          { // $admin = get_admin(); $admin=get_record('user','username','your_course_manager'); $subject = "Moodle Course requests pending"; $messagetext = "Course requests have been submitted for approval. Please check $CFG->wwwroot/course/pending.php."; email_to_user($admin,$admin,$subject,$messagetext); }

          mtrace("Course requests checked");

          Cheers.

          Show
          Patrick Pollet added a comment - Please find enclosed a patch to Moodle 1.72 course files to allow : -added a drop down of current categories with the config default category selected -added a field for course idnumber this required adding to mdl_course_request table : category int(11) Non 0 Pp categorie ou inserer le cours idnumber varchar(64) Oui NULL PP code ECTS as per http://tracker.moodle.org/browse/MDL-6160 the following code should also be added to admin/cron.php mtrace("Checking course creation requests"); if ($pending = get_records("course_request")) { // $admin = get_admin(); $admin=get_record('user','username','your_course_manager'); $subject = "Moodle Course requests pending"; $messagetext = "Course requests have been submitted for approval. Please check $CFG->wwwroot/course/pending.php."; email_to_user($admin,$admin,$subject,$messagetext); } mtrace("Course requests checked"); Cheers.
          Hide
          Patrick Pollet added a comment -

          Content :

          rw-rr- root/root 9320 2007-06-04 12:05:59 course_request_with_categories.diff
          rw-rr- root/root 4907 2007-06-04 11:44:58 request.php
          rw-rr- root/root 3541 2007-06-04 10:26:03 request.html
          rw-rr- root/root 6967 2007-06-04 10:33:40 pending.php

          Show
          Patrick Pollet added a comment - Content : rw-r r - root/root 9320 2007-06-04 12:05:59 course_request_with_categories.diff rw-r r - root/root 4907 2007-06-04 11:44:58 request.php rw-r r - root/root 3541 2007-06-04 10:26:03 request.html rw-r r - root/root 6967 2007-06-04 10:33:40 pending.php
          Hide
          Heiko Schach added a comment - - edited

          Adding course category to new course request form would be a useful improvement for Moodle 2.x
          With category information provided by the person requesting the course, the admin wouldn't have to guess and manually select a category when approving the course.

          Show
          Heiko Schach added a comment - - edited Adding course category to new course request form would be a useful improvement for Moodle 2.x With category information provided by the person requesting the course, the admin wouldn't have to guess and manually select a category when approving the course.
          Hide
          Frédéric Massart added a comment -

          Sending for peer review.

          Show
          Frédéric Massart added a comment - Sending for peer review.
          Hide
          Rajesh Taneja added a comment -

          Patch looks good Fred, although there are few things you might want to consider.

          1. Category field should also contain UNSIGNED="true". Same way as it gets created in course
            <FIELD NAME="category" TYPE="int" LENGTH="10" NOTNULL="true" UNSIGNED="true" DEFAULT="0" SEQUENCE="false" PREVIOUS="id" NEXT="sortorder"/>
            
          2. While populating list of categories (https://github.com/FMCorz/moodle/compare/MDL-8307-master#L2R73), should we exclude hidden categories or check for some capability?
          3. Not sure about https://github.com/FMCorz/moodle/compare/MDL-8307-master#L1R118. It will show different category for users with different category. Shouldn't this be actual value submitted by user, or some warning that user can't accept this request ?
          4. At https://github.com/FMCorz/moodle/compare/MDL-8307-master#L0R4304 you should also check for moodle:course/create capability, else if user is prohibited to create a course in certain category will be able to create course in that category.
          5. Lastly, should we also add another config to force category (original behaviour), rather then showing options to requesting user ?
          Show
          Rajesh Taneja added a comment - Patch looks good Fred, although there are few things you might want to consider. Category field should also contain UNSIGNED="true" . Same way as it gets created in course <FIELD NAME="category" TYPE="int" LENGTH="10" NOTNULL="true" UNSIGNED="true" DEFAULT="0" SEQUENCE="false" PREVIOUS="id" NEXT="sortorder"/> While populating list of categories ( https://github.com/FMCorz/moodle/compare/MDL-8307-master#L2R73 ), should we exclude hidden categories or check for some capability? Not sure about https://github.com/FMCorz/moodle/compare/MDL-8307-master#L1R118 . It will show different category for users with different category. Shouldn't this be actual value submitted by user, or some warning that user can't accept this request ? At https://github.com/FMCorz/moodle/compare/MDL-8307-master#L0R4304 you should also check for moodle:course/create capability, else if user is prohibited to create a course in certain category will be able to create course in that category. Lastly, should we also add another config to force category (original behaviour), rather then showing options to requesting user ?
          Hide
          Rajesh Taneja added a comment -

          Sorry Fred, ignore my first comment. MDL-27982

          Show
          Rajesh Taneja added a comment - Sorry Fred, ignore my first comment. MDL-27982
          Hide
          Martin Dougiamas added a comment -

          Regarding also checking for moodle:course/create ... I do agree with you that it's probably logical to be able to create courses if you can approve them, but it's probably not worth complexifying all this as its not critical to most Moodle sites. My impression is that it's only for non-serious applications like test courses or student clubs, where they want a little human check in the workflow.

          It's already pretty complex with all the category checking

          Show
          Martin Dougiamas added a comment - Regarding also checking for moodle:course/create ... I do agree with you that it's probably logical to be able to create courses if you can approve them, but it's probably not worth complexifying all this as its not critical to most Moodle sites. My impression is that it's only for non-serious applications like test courses or student clubs, where they want a little human check in the workflow. It's already pretty complex with all the category checking
          Hide
          Frédéric Massart added a comment -

          Thank you for your review Raj.

          2. The user already need course:request, plus hidden categories are hidden from that, and AFAIK there is no other capability to prevent a category from being seen.

          3. I don't know what's best here. I actually like the idea of the user being able to approve the category even if he does not have that capability... and if he does, then he should see where the course will be created. I'd like more feedback on that.

          4. As Martin mentioned, it makes more sense but maybe add too much complexity to this option.

          5. I've added a setting for this and default it to No.

          I am pushing for integration to gather more feedback on these points.

          Show
          Frédéric Massart added a comment - Thank you for your review Raj. 2. The user already need course:request, plus hidden categories are hidden from that, and AFAIK there is no other capability to prevent a category from being seen. 3. I don't know what's best here. I actually like the idea of the user being able to approve the category even if he does not have that capability... and if he does, then he should see where the course will be created. I'd like more feedback on that. 4. As Martin mentioned, it makes more sense but maybe add too much complexity to this option. 5. I've added a setting for this and default it to No. I am pushing for integration to gather more feedback on these points.
          Hide
          Rajesh Taneja added a comment -

          Thanks Martin and Fred,

          I agree we should keep things simple, but shouldn't we respect capability check (which we are doing in course/edit and other places)?
          Also, if we want to keep it simple then https://github.com/FMCorz/moodle/compare/MDL-8307-master#L2R118 check should not be done. This will show different results to users with different capabilities.

          Show
          Rajesh Taneja added a comment - Thanks Martin and Fred, I agree we should keep things simple, but shouldn't we respect capability check (which we are doing in course/edit and other places)? Also, if we want to keep it simple then https://github.com/FMCorz/moodle/compare/MDL-8307-master#L2R118 check should not be done. This will show different results to users with different capabilities.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Aparup Banerjee added a comment -

          Hi guys,
          this code has obviously been thru some nice iterations here. all looks fine to me. re discussion above:
          I've always understood the hidden courses are for hiding from general student type roles.

          The questions here seems to be whether to create the strict dependence about the capability relationship within code or to have it 'understood' by admins so that it remain 'flexible' for some kind of edge case setup. IMO as long as we clarify this within docs , Admins will be able to pick this up for any of their edge cases. imo, thats better than requiring the multi caps which could cause more difficulty (along with being tightly coupled).

          Perhaps if there is demand for this and is reported from out there, it could be handled more generally as there are many scenarios elsewhere that have such relationships between caps. even a general security level setting is possible where we require 'tightlycoupledcaps' setting

          things needing more work here:

          • unit tests for approve() and create() need to be looked at and added to in 'courselib_testcase'
          • please rebase new patch, there are minor conflicts atm.
          Show
          Aparup Banerjee added a comment - Hi guys, this code has obviously been thru some nice iterations here. all looks fine to me. re discussion above: I've always understood the hidden courses are for hiding from general student type roles. The questions here seems to be whether to create the strict dependence about the capability relationship within code or to have it 'understood' by admins so that it remain 'flexible' for some kind of edge case setup. IMO as long as we clarify this within docs , Admins will be able to pick this up for any of their edge cases. imo, thats better than requiring the multi caps which could cause more difficulty (along with being tightly coupled). Perhaps if there is demand for this and is reported from out there, it could be handled more generally as there are many scenarios elsewhere that have such relationships between caps. even a general security level setting is possible where we require 'tightlycoupledcaps' setting things needing more work here: unit tests for approve() and create() need to be looked at and added to in 'courselib_testcase' please rebase new patch, there are minor conflicts atm.
          Hide
          Aparup Banerjee added a comment -

          reopening for Fred to add unit tests here. (as spoken)

          Show
          Aparup Banerjee added a comment - reopening for Fred to add unit tests here. (as spoken)
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Frédéric Massart added a comment -

          Pushing for integration again. Unit Tests are MDL-35301.

          Show
          Frédéric Massart added a comment - Pushing for integration again. Unit Tests are MDL-35301 .
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (master only), thanks!

          Side note: admin setting descriptions should be: xxxx_desc instead of configxxxx, but I've ignored that to keep it looking the same than the rest.

          http://docs.moodle.org/dev/Admin_settings

          But, perhaps it would be a good idea to create one task about to move (master only) all the settings to _desc once and forever. Else we are not going to normalize that ever.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (master only), thanks! Side note: admin setting descriptions should be: xxxx_desc instead of configxxxx, but I've ignored that to keep it looking the same than the rest. http://docs.moodle.org/dev/Admin_settings But, perhaps it would be a good idea to create one task about to move (master only) all the settings to _desc once and forever. Else we are not going to normalize that ever. Ciao
          Hide
          Tim Barker added a comment -

          We only just found out about this today. It is a very big test across all of the dbs so we do not have time to do it. MdR and I both agree that it should wait until next week.

          Show
          Tim Barker added a comment - We only just found out about this today. It is a very big test across all of the dbs so we do not have time to do it. MdR and I both agree that it should wait until next week.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oki, going to leave it here in this status... but will be rolling weeklies without reverting. Next week will decide if such an action is needed.

          Show
          Eloy Lafuente (stronk7) added a comment - Oki, going to leave it here in this status... but will be rolling weeklies without reverting. Next week will decide if such an action is needed.
          Hide
          Michael de Raadt added a comment -

          This worked as expected apart from one minor aspect.

          When upgrading, the default course was not used in the new category field in the request table. This was initialised to zero for requests that existed before the upgrade. The miscellaneous category is category 1. When the requests were shown in the pending screen, they appeared in the miscellaneous category and ended up there after approving the request.

          Show
          Michael de Raadt added a comment - This worked as expected apart from one minor aspect. When upgrading, the default course was not used in the new category field in the request table. This was initialised to zero for requests that existed before the upgrade. The miscellaneous category is category 1. When the requests were shown in the pending screen, they appeared in the miscellaneous category and ended up there after approving the request.
          Hide
          Frédéric Massart added a comment -

          I think this is trivial and should not cause the test fail. I will investigate to see if it is really an issue, or my testing instructions that are dodgy...

          Show
          Frédéric Massart added a comment - I think this is trivial and should not cause the test fail. I will investigate to see if it is really an issue, or my testing instructions that are dodgy...
          Hide
          Frédéric Massart added a comment -

          I have added a commit to my branch to fix this issue. What happened was that after upgrade, the category field is set as 0, but when calling get_course_category() with 0 as argument, the method returns the default category in Moodle which is Miscellaneous. So now I am checking also checking if the category is set or not. The best approach would have been to have an upgrade script to update the missing categories, but as this is already in stable I guess this should be enough. I could create a new issue to have add the upgrade script and this last patch. Let me know.

          Show
          Frédéric Massart added a comment - I have added a commit to my branch to fix this issue. What happened was that after upgrade, the category field is set as 0, but when calling get_course_category() with 0 as argument, the method returns the default category in Moodle which is Miscellaneous. So now I am checking also checking if the category is set or not. The best approach would have been to have an upgrade script to update the missing categories, but as this is already in stable I guess this should be enough. I could create a new issue to have add the upgrade script and this last patch. Let me know.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          New commit integrated, thanks!

          (I don't think we need one upgrade step at all, with the last commit any 0 is already transformed to $CFG->defaultrequestcategory automatically, providing the same, correct, result)

          Show
          Eloy Lafuente (stronk7) added a comment - New commit integrated, thanks! (I don't think we need one upgrade step at all, with the last commit any 0 is already transformed to $CFG->defaultrequestcategory automatically, providing the same, correct, result)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sending back to testing...

          Show
          Eloy Lafuente (stronk7) added a comment - Sending back to testing...
          Hide
          Dan Poltawski added a comment -

          What are we doing with this issue?

          Show
          Dan Poltawski added a comment - What are we doing with this issue?
          Hide
          Frédéric Massart added a comment -

          Michael had tested the whole issue. I guess the only thing to test now is having a course_request with a category set to 0, and make sure the default category (preferably different than Miscellaneous) appears on the pending request page, and on approval the course is created in that category. (Test 1 up to step 6)

          Show
          Frédéric Massart added a comment - Michael had tested the whole issue. I guess the only thing to test now is having a course_request with a category set to 0, and make sure the default category (preferably different than Miscellaneous) appears on the pending request page, and on approval the course is created in that category. (Test 1 up to step 6)
          Hide
          Michael de Raadt added a comment -

          Test result: Success.

          Tested with Oracle, MSSQL, MySQL and PostgreSQL.

          Show
          Michael de Raadt added a comment - Test result: Success. Tested with Oracle, MSSQL, MySQL and PostgreSQL.
          Hide
          Dan Poltawski added a comment -

          Congratulations, you've done it!

          Thanks, this change is now in the latest weekly release!

          Join the crowds of people tomorrow from 8am and download this Moodle release from your local apple store!

          Show
          Dan Poltawski added a comment - Congratulations, you've done it! Thanks, this change is now in the latest weekly release! Join the crowds of people tomorrow from 8am and download this Moodle release from your local apple store!
          Hide
          Heikki Wilenius added a comment -

          Any chance of a backport to Moodle 2.3? Sorry if this is a breach of tracker etiquette, am unsure whether I should just open a new issue for requesting a backport.

          Show
          Heikki Wilenius added a comment - Any chance of a backport to Moodle 2.3? Sorry if this is a breach of tracker etiquette, am unsure whether I should just open a new issue for requesting a backport.
          Hide
          Frédéric Massart added a comment -

          Hi Heikki, unfortunately as this issue is an improvement and required database changes it won't be available in version < 2.4.

          Show
          Frédéric Massart added a comment - Hi Heikki, unfortunately as this issue is an improvement and required database changes it won't be available in version < 2.4.
          Hide
          Heikki Wilenius added a comment -

          Ok, thanks for the quick reply!

          Show
          Heikki Wilenius added a comment - Ok, thanks for the quick reply!
          Hide
          Helen Foster added a comment -

          Fred, thanks for implementing this improvement. I've just made a note of it in the docs http://docs.moodle.org/24/en/Adding_a_new_course#Course_requests

          Show
          Helen Foster added a comment - Fred, thanks for implementing this improvement. I've just made a note of it in the docs http://docs.moodle.org/24/en/Adding_a_new_course#Course_requests

            People

            • Votes:
              5 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: