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 Bug
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      38210

      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.

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Rajesh Taneja added a comment -

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

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

          Hello Rossie,

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

          Thanks.

          Show
          Rajesh Taneja added a comment - Hello Rossie, I have done some changes, can you please peer-review this again for me. Thanks.
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Rajesh Taneja added a comment -

          Thanks Rossie,

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

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

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

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

          Hello Rossie,

          I think it's good for review now.

          Show
          Rajesh Taneja added a comment - Hello Rossie, I think it's good for review now.
          Hide
          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
          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
          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
          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
          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
          Rajesh Taneja added a comment -

          Branches re-based.

          Show
          Rajesh Taneja added a comment - Branches re-based.
          Hide
          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
          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
          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
          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
          Rajesh Taneja added a comment -

          Branches re-based

          Show
          Rajesh Taneja added a comment - Branches re-based
          Hide
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          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
          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
          Rajesh Taneja added a comment -

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

          Show
          Rajesh Taneja added a comment - Thanks for pointing that Petr, I have removed extra error message and it's now using require_capability.
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Rajesh Taneja added a comment -

          Thanks Sam,

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

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

          Thanks Raj, this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Raj, this has been integrated now
          Hide
          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 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
          Sam Hemelryk added a comment -

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

          Cheers
          Sam

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

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

          Show
          Ankit Agarwal added a comment - Thanks Sam, Tested and working great. Created MDL-32150 to fix the select all issue. Thanks
          Hide
          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
          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: