Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-31640

In course/search.php selected courses can't be moved to categories, no capability check while moving courses and turn editing on/off not working

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.1.6, 2.2.3
    • Component/s: Administration
    • Labels:
    • Environment:
      Removed blocklist param from the link.
    • Testing Instructions:
      Hide
      1. Log in as admin
      2. Turn editing on
      3. On Site Admin panel go to Plugins > Blocks > Manage Blocks
      4. Click on link in "Instances" column > on blocks (Copy this link somewhere, we need it to trick manager to see this page)
      5. select some courses and move to different category.
      6. Remove course:create and category:manage role for manager in Misc category(Select category and click on settings->permission)
      7. Log in as manager
      8. Copy and paste above link and change sesskey (get it from page view)
      9. Make sure checkbox for courses in Misc category is disabled and you have no way to move the course to another category.
      10. use firebug and enable checkbox and try moving them to another category.
      11. Make sure you encounter error.
      12. Log in as admin
      13. assign manager course:create and category:manage on Misc and one more categories
      14. Log in as manager
      15. Copy and paste above link and change sesskey (get it from page view)
      16. You should be able to move course between the two categories only.
      Show
      Log in as admin Turn editing on On Site Admin panel go to Plugins > Blocks > Manage Blocks Click on link in "Instances" column > on blocks (Copy this link somewhere, we need it to trick manager to see this page) select some courses and move to different category. Remove course:create and category:manage role for manager in Misc category(Select category and click on settings->permission) Log in as manager Copy and paste above link and change sesskey (get it from page view) Make sure checkbox for courses in Misc category is disabled and you have no way to move the course to another category. use firebug and enable checkbox and try moving them to another category. Make sure you encounter error. Log in as admin assign manager course:create and category:manage on Misc and one more categories Log in as manager Copy and paste above link and change sesskey (get it from page view) You should be able to move course between the two categories only.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      wip-mdl-31640-new

      Description

      Discover this issue while testing MDL-30388.

      There are few issues on course/search.php

      1. When trying to move courses to different category, the page jump to course/search.php without moving the course.
      2. No capability check done while moving the course
      3. Capability check should be based on category level and not system level (If user have system level create and manage capability, but have no capability on any category, then he should not be able to move the course)
      4. Turn editing on/off redirects user to search page.

      This occurs when there is blocklist param on the link.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Raj,

            Thanks for the quick fix. However, there are some changes needed before submitting this for integration.

            1. The capability check for should be moodle/course:changecategory instead of moodle/category:manage. I'm not sure why it is checking for course:create capability.
            2. the addition of hidden fields for search, perpage, and page are duplicate fields' name.

            Returning this back for development.

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Raj, Thanks for the quick fix. However, there are some changes needed before submitting this for integration. 1. The capability check for should be moodle/course:changecategory instead of moodle/category:manage. I'm not sure why it is checking for course:create capability. 2. the addition of hidden fields for search, perpage, and page are duplicate fields' name. Returning this back for development.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for the feedback Rossie,
            have removed duplicate fields, but not sure of changecategory. In course/category.php we use category:manage.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for the feedback Rossie, have removed duplicate fields, but not sure of changecategory. In course/category.php we use category:manage.
            Hide
            timhunt Tim Hunt added a comment -

            This brings back horrid memories. I did a lot of work on this while I was at HQ, so second half of 2008 or first half of 2009. All I can remember now is that it was very complex.

            Ah, here we go: https://github.com/moodle/moodle/commit/8ed5dd63eeba4e9fc2e290bd3b9584aed440c13c

            I think the logic is supposed to be:

            To move a course from category A to category B you need

            • moodle/category:manage in A; and
            • moodle/course:create in B (since you are adding a course to B)

            Oh, no. I am wrong. It has to be symmetrical, to make sure we don't let users to something that they cannot undo if they realise that they have made a mistake. Therefore, we check moodle/course:create in both categories. (I cannot remember if we also require category:manage.)

            So, important principles:

            1. Wherever possible, actions should be undoable. So, you should only have permission to move from A to B if you have permission to move from B to A too.
            2. It should not be possible to find work-arounds. For example, there is no point preventing a user from creating a course in B, if they can create a course in A, then move it to B.
            3. There should be as few capabilities as possible, to make the whole thing as easy as possible to understand. (So, for instance, I am not sure what the point of the moodle/course:changecategory capability is, other than to make things more complex and more buggy.)

            I hope that helps. That is what I remember, at any rate.

            Show
            timhunt Tim Hunt added a comment - This brings back horrid memories. I did a lot of work on this while I was at HQ, so second half of 2008 or first half of 2009. All I can remember now is that it was very complex. Ah, here we go: https://github.com/moodle/moodle/commit/8ed5dd63eeba4e9fc2e290bd3b9584aed440c13c I think the logic is supposed to be: To move a course from category A to category B you need moodle/category:manage in A; and moodle/course:create in B (since you are adding a course to B) Oh, no. I am wrong. It has to be symmetrical, to make sure we don't let users to something that they cannot undo if they realise that they have made a mistake. Therefore, we check moodle/course:create in both categories. (I cannot remember if we also require category:manage.) So, important principles: Wherever possible, actions should be undoable. So, you should only have permission to move from A to B if you have permission to move from B to A too. It should not be possible to find work-arounds. For example, there is no point preventing a user from creating a course in B, if they can create a course in A, then move it to B. There should be as few capabilities as possible, to make the whole thing as easy as possible to understand. (So, for instance, I am not sure what the point of the moodle/course:changecategory capability is, other than to make things more complex and more buggy.) I hope that helps. That is what I remember, at any rate.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Tim,

            Principles you mentioned, will certainly help. I think, we should check multiple capability, before changing course category.

            1. category:manage on both categories
            2. course:create on both
            3. course:changecategory on the course being moved.
            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Tim, Principles you mentioned, will certainly help. I think, we should check multiple capability, before changing course category. category:manage on both categories course:create on both course:changecategory on the course being moved.
            Hide
            poltawski Dan Poltawski added a comment -

            Raj & I were just discussing this - it is very complex and difficult to get our heads around..

            One thing which Tim mentioned which I think I agree with is that we probably should remove the moodle/course:changecategory permission.

            From the description my thought is that we need category:manage and course:create permissions on both the source and destination category in order to conduct a move.

            Show
            poltawski Dan Poltawski added a comment - Raj & I were just discussing this - it is very complex and difficult to get our heads around.. One thing which Tim mentioned which I think I agree with is that we probably should remove the moodle/course:changecategory permission. From the description my thought is that we need category:manage and course:create permissions on both the source and destination category in order to conduct a move.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for the inputs, Dan.
            I agree, while moving course we should just check for category:manage and course:create capability.
            But for showing "category select box", shouldn't we check for course:changecategory (in SYSTEM_CONTEXT), as it is used in edit_form.php (While editing course.)

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for the inputs, Dan. I agree, while moving course we should just check for category:manage and course:create capability. But for showing "category select box", shouldn't we check for course:changecategory (in SYSTEM_CONTEXT), as it is used in edit_form.php (While editing course.)
            Hide
            timhunt Tim Hunt added a comment -

            It certainly is complex. I wish you luck.

            However, I am sure that the permissions check for moving a course should not depend on which bit of UI the user is looking at. If the required permissions are

            1. category:manage on both categories
            2. course:create on both

            then the correct hehaviour on the edit course form is:

            • Show the change category dropdown if the user has category:manage and course:create on the current category.
            • the drop-down should only allow the user to select those categories where they also have category:manage and course:create
            Show
            timhunt Tim Hunt added a comment - It certainly is complex. I wish you luck. However, I am sure that the permissions check for moving a course should not depend on which bit of UI the user is looking at. If the required permissions are category:manage on both categories course:create on both then the correct hehaviour on the edit course form is: Show the change category dropdown if the user has category:manage and course:create on the current category. the drop-down should only allow the user to select those categories where they also have category:manage and course:create
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for all the help Tim,
            I will update the patch with capability check (as suggested by you.)

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for all the help Tim, I will update the patch with capability check (as suggested by you.)
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Hello Rossie,

            I have done some changes, can you please peer-review this again for me.

            Thanks.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Hello Rossie, I have done some changes, can you please peer-review this again for me. Thanks.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Raj,

            Removing manager's course:create and category:manage capabilities produce the following:
            Notice: Undefined index: 1 in /moodle/course/search.php on line 251

            Also, without those capabilities, the 'turn editing on/off' button is not available.

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Raj, Removing manager's course:create and category:manage capabilities produce the following: Notice: Undefined index: 1 in /moodle/course/search.php on line 251 Also, without those capabilities, the 'turn editing on/off' button is not available.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for pointing that Rossie,

            I have fixed this issue. For viewing, editing button, manager should have category:manage and course:create capability in system context. https://github.com/rajeshtaneja/moodle/blob/f1a83516be23401309f69cb20e410f05781f83a3/course/search.php#L196

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for pointing that Rossie, I have fixed this issue. For viewing, editing button, manager should have category:manage and course:create capability in system context. https://github.com/rajeshtaneja/moodle/blob/f1a83516be23401309f69cb20e410f05781f83a3/course/search.php#L196
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            In addition to mentioned problem of moving and capability check. There are few more things added to this patch:

            1. "Move to" capability should not rely on system_context. i.e. if user has course:create and category:manage capability on system level, but doesn't have these permission on any category, then "move to" should not be visible. Hence updated check to see if user has capability on any category. If yes, then only "move to should be visible"
            2. In addition to this, "Turn edition On/Off" was also doing checking for system level capability, and was not showing search page. This has also been fixed.

            NOTE: Two extra commits were added to this. If you want this can be put in two separate bugs.

            Show
            rajeshtaneja Rajesh Taneja added a comment - In addition to mentioned problem of moving and capability check. There are few more things added to this patch: "Move to" capability should not rely on system_context. i.e. if user has course:create and category:manage capability on system level, but doesn't have these permission on any category, then "move to" should not be visible. Hence updated check to see if user has capability on any category. If yes, then only "move to should be visible" In addition to this, "Turn edition On/Off" was also doing checking for system level capability, and was not showing search page. This has also been fixed. NOTE: Two extra commits were added to this. If you want this can be put in two separate bugs.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Raj,

            Found the following error:

            • Address url: /moodle/course/search.php (without any param)
              Notice: Undefined variable: searchterms in /moodle/course/search.php on line 195
              Warning: Invalid argument supplied for foreach() in /moodle/lib/datalib.php on line 749
            • As admin, and block instance is added to front page (page editing must be turn off to view the error)
              Notice: Undefined index: 0 in /moodle/course/search.php on line 254

            Also, you might want to update the test instruction to remove course:create and category:manage from system level. Other than that, it working as it supposed to.

            Almost there Raj

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Raj, Found the following error: Address url: /moodle/course/search.php (without any param) Notice: Undefined variable: searchterms in /moodle/course/search.php on line 195 Warning: Invalid argument supplied for foreach() in /moodle/lib/datalib.php on line 749 As admin, and block instance is added to front page (page editing must be turn off to view the error) Notice: Undefined index: 0 in /moodle/course/search.php on line 254 Also, you might want to update the test instruction to remove course:create and category:manage from system level. Other than that, it working as it supposed to. Almost there Raj
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Also,

            On language string, it's missing end bracket.

            You are not allowed to move course (id: 15) in category (id: 4

            Maybe it is better to display the course name and category name instead of their id or make it to more general to something like:

            You don't have capabilities to move the course into different category.

            Show
            rwijaya Rossiani Wijaya added a comment - Also, On language string, it's missing end bracket. You are not allowed to move course (id: 15) in category (id: 4 Maybe it is better to display the course name and category name instead of their id or make it to more general to something like: You don't have capabilities to move the course into different category.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Rossie,

            All notices fixed and string changed.
            Once more for peer-review

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Rossie, All notices fixed and string changed. Once more for peer-review
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Sorry Rossie,
            I am taking this back for now. Want to see, if fix can be better optimized.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Sorry Rossie, I am taking this back for now. Want to see, if fix can be better optimized.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Hello Rossie,

            I think it's good for review now.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Hello Rossie, I think it's good for review now.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Raj,

            This looks good now.

            However just noting on $searchform variable on line 202. Since the if/else sets the $searchform, it is not necessary to set $searchform = ''; It is good practice to initialize variable, so its your call to keep or remove it.

            Other than that, feel free to submit it for integration review.

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Raj, This looks good now. However just noting on $searchform variable on line 202. Since the if/else sets the $searchform, it is not necessary to set $searchform = ''; It is good practice to initialize variable, so its your call to keep or remove it. Other than that, feel free to submit it for integration review.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Hello Rossie,

            Thanks for pointing that. I know in moodle we don't usually initialise variables, but I personally prefer to intialise my variables before using them. Hope that won't be a problem.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Hello Rossie, Thanks for pointing that. I know in moodle we don't usually initialise variables, but I personally prefer to intialise my variables before using them. Hope that won't be a problem.
            Hide
            stronk7 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
            stronk7 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
            rajeshtaneja Rajesh Taneja added a comment -

            Branches re-based.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Branches re-based.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday, with point releases happening next week based on current status.

            This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

            This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

            Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday, with point releases happening next week based on current status. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Some hours ago...

            the main moodle.git repository has 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
            stronk7 Eloy Lafuente (stronk7) added a comment - Some hours ago... the main moodle.git repository has 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
            rajeshtaneja Rajesh Taneja added a comment -

            Branches re-based

            Show
            rajeshtaneja Rajesh Taneja added a comment - Branches re-based
            Hide
            skodak Petr Skoda added a comment -

            to integrators: is it really necessary to introduce yet another error message that nobody sees unless somebody concurrently modifies category permissions? To me it seems that a few good old require_capability() calls instead of has_all_capabilities() does the trick.

            Show
            skodak Petr Skoda added a comment - to integrators: is it really necessary to introduce yet another error message that nobody sees unless somebody concurrently modifies category permissions? To me it seems that a few good old require_capability() calls instead of has_all_capabilities() does the trick.
            Hide
            timhunt Tim Hunt added a comment -

            Or, we could make a require_all_capabilities function, or we could throw the same exception that require_capability throws from this calling code.

            Show
            timhunt Tim Hunt added a comment - Or, we could make a require_all_capabilities function, or we could throw the same exception that require_capability throws from this calling code.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            IMO we should either come-up with require_all_capabilities, as Tim suggested (which I think is not back-portable, so master only.) or use has_all_capabilities as it's much more readable and manageable.

            We have being using multiple require_capability calls, but it defeat agile methodology.

            Show
            rajeshtaneja Rajesh Taneja added a comment - IMO we should either come-up with require_all_capabilities, as Tim suggested (which I think is not back-portable, so master only.) or use has_all_capabilities as it's much more readable and manageable. We have being using multiple require_capability calls, but it defeat agile methodology.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for pointing that Petr, I have removed extra error message and it's now using require_capability.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for pointing that Petr, I have removed extra error message and it's now using require_capability.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

            This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

            This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

            Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
            Hide
            stronk7 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
            stronk7 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
            samhemelryk Sam Hemelryk added a comment -

            Hi Raj,

            I'm sending this back as there are a few things to tidy up.

            1. PHPDoc's should not be added to inline code. You should always use // for this. There's not point to using phpdocs there and single line comments are far more compact which we especially want for inline comments. (This is in the coding style as well)
            2. line 49: The foreach didn't include perpage because that is being set immediately below. To be truthful I think that whole foreach is evil (I know its not your code) and I'd consider the checking of the default and adding it as param if different to be both a better solution and more consistent with the rest of Moodle.
            3. ln 259: As this is a form the $modulelink data should be added as a hidden input rather than as a get param to the form. (Noting sesskey is already been added you just need modulelist||blocklist)

            Suggested things to fix as well:

            1. line 92 has_any_capability call could be changed to call can_edit_in_category, or better yet seeing as you are calling that already store the result and use it there.
            2. Seeing as you are reviewing this file anyway it would be nice if you could convert any context calls to the new context API, either way I would expect any new capability calls to be using the new API

            This has been an interesting issue, I've read through the code before I read through the issue and was about to ask where on earth the idea of checking moodle/course:create was coming from.
            My first point of call was course/category.php because that interface allows the user to move several courses like the search page and it is an area that I have been looking at recently.
            Course/category.php is checking that the user has moodle/category:manage in all of the original categories as well as the destination category. There is no checking or mentioning of course:create.
            Just because I was curious I checked http://docs.moodle.org/22/en/Capabilities/moodle/course:create but no mention of moving courses there (only creating).
            So finally I looked at course/edit.php (and of course course/edit_form.php) which was interesting because this is doing something completely different.
            It's not concerned about category:manage, however it is checking course capabilities albeit poorly.
            After reading through all of this I've come around to the idea of checking both moodle/course:create, and moodle/category:manage.
            I think certainly we need to look over all of Moodle for areas where courses are being moved and ensure capabilities are consistent. I see MDL-31750 has already been created so perhaps we repurpose that to look at all areas or we create a new issue and make that a subtask (I also think this should be a priority and we should aim to get all areas consistent for the next release so that when the security notes go out we can say fixed consistently rather than fixed in this one place).
            I also think that this needs to be documented somewhere, either on the capabilities pages in the docs (course:create and category:manage) or just somewhere separately about moving courses, whats required and the super uber important points Tim raised on the 15th about why things are being done like this.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Raj, I'm sending this back as there are a few things to tidy up. PHPDoc's should not be added to inline code. You should always use // for this. There's not point to using phpdocs there and single line comments are far more compact which we especially want for inline comments. (This is in the coding style as well) line 49: The foreach didn't include perpage because that is being set immediately below. To be truthful I think that whole foreach is evil (I know its not your code) and I'd consider the checking of the default and adding it as param if different to be both a better solution and more consistent with the rest of Moodle. ln 259: As this is a form the $modulelink data should be added as a hidden input rather than as a get param to the form. (Noting sesskey is already been added you just need modulelist||blocklist) Suggested things to fix as well: line 92 has_any_capability call could be changed to call can_edit_in_category, or better yet seeing as you are calling that already store the result and use it there. Seeing as you are reviewing this file anyway it would be nice if you could convert any context calls to the new context API, either way I would expect any new capability calls to be using the new API This has been an interesting issue, I've read through the code before I read through the issue and was about to ask where on earth the idea of checking moodle/course:create was coming from. My first point of call was course/category.php because that interface allows the user to move several courses like the search page and it is an area that I have been looking at recently. Course/category.php is checking that the user has moodle/category:manage in all of the original categories as well as the destination category. There is no checking or mentioning of course:create. Just because I was curious I checked http://docs.moodle.org/22/en/Capabilities/moodle/course:create but no mention of moving courses there (only creating). So finally I looked at course/edit.php (and of course course/edit_form.php) which was interesting because this is doing something completely different. It's not concerned about category:manage, however it is checking course capabilities albeit poorly. After reading through all of this I've come around to the idea of checking both moodle/course:create, and moodle/category:manage. I think certainly we need to look over all of Moodle for areas where courses are being moved and ensure capabilities are consistent. I see MDL-31750 has already been created so perhaps we repurpose that to look at all areas or we create a new issue and make that a subtask (I also think this should be a priority and we should aim to get all areas consistent for the next release so that when the security notes go out we can say fixed consistently rather than fixed in this one place). I also think that this needs to be documented somewhere, either on the capabilities pages in the docs (course:create and category:manage) or just somewhere separately about moving courses, whats required and the super uber important points Tim raised on the 15th about why things are being done like this. Cheers Sam
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Sam,

            I have updated the branch with your suggestions.
            On the other thought, I was wondering if this should go in stable?

            Also, not sure what is expected from (suggested things) 2nd point (convert any context calls to the new context API).

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Sam, I have updated the branch with your suggestions. On the other thought, I was wondering if this should go in stable? Also, not sure what is expected from (suggested things) 2nd point (convert any context calls to the new context API).
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Raj,

            Given every script that allows a course to be moved does its own thing, and that we are going to have several issues to fix up each area I think we either backport them all, or don't backport any of them.
            And given that its a bug I do think we should backport them unless there is a good reason not too.

            As for the context API I am referring to things like:

            // Old API
            $context = get_context_instance(CONTEXT_COURSECAT, $category->id);
            // New API
            $context = context_coursecat::instance($category->id);

            It's not essential that its done, but at least this way you know for the future.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Raj, Given every script that allows a course to be moved does its own thing, and that we are going to have several issues to fix up each area I think we either backport them all, or don't backport any of them. And given that its a bug I do think we should backport them unless there is a good reason not too. As for the context API I am referring to things like: // Old API $context = get_context_instance(CONTEXT_COURSECAT, $category->id); // New API $context = context_coursecat::instance($category->id); It's not essential that its done, but at least this way you know for the future. Cheers Sam
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Sam,

            Changed context calls in master and 22.
            Up for your review again.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Sam, Changed context calls in master and 22. Up for your review again.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Raj, this has been integrated now

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Raj, this has been integrated now
            Hide
            ankit_frenz Ankit Agarwal added a comment - - edited

            Putting testing of this on hold until MDL-31528 is fixed/reverted

            Tested this on master and 22 , it works fine. Altough I encountered an issue with "Select all" buttons we have, ideally it should only check the boxes which are not disabled, but it doesn't follow that rule and checks everything.
            Will create a separate issue for that.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - - edited Putting testing of this on hold until MDL-31528 is fixed/reverted Tested this on master and 22 , it works fine. Altough I encountered an issue with "Select all" buttons we have, ideally it should only check the boxes which are not disabled, but it doesn't follow that rule and checks everything. Will create a separate issue for that. Thanks
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Ankit,
            Looking at MDL-31528 it looks like this can be tested now.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Ankit, Looking at MDL-31528 it looks like this can be tested now. Cheers Sam
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Thanks Sam,
            Tested and working great.
            Created MDL-32150 to fix the select all issue.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Thanks Sam, Tested and working great. Created MDL-32150 to fix the select all issue. Thanks
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Congratulations are in order, you've made it, or at least your code has!
            It's now part of Moodle and both the git and cvs repositories have been updated.

            This issue is being marked as fixed and closed.

            Thank you.

            Show
            samhemelryk Sam Hemelryk added a comment - Congratulations are in order, you've made it, or at least your code has! It's now part of Moodle and both the git and cvs repositories have been updated. This issue is being marked as fixed and closed. Thank you.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/May/12