Moodle
  1. Moodle
  2. MDL-31750

Capability check needs to be improved in course/edit.php and Course/category.php

    Details

    • Testing Instructions:
      Hide

      Things to do

      • Create a course creator
      • Add a couple of categories
      • Add a couple of courses under one of the categories

      Testing steps

      1. Go to [Settings ► Site administration ► Users ► Permissions ► Define roles]
      2. Click on Course creator.
      3. Click edit and show advanced and then do a search for site:manageblocks and set it to 'Allow'.
      4. Click 'allow' for manage categories (moodle/category:manage).
      5. login as the course creator.
      6. Go to [Settings ► Site administration ► Courses ► Add/edit courses]
      7. Click on misc.
        [Test] Try selecting and then moving courses to a different category. This will take you to an error page saying "You can not move this course to the category specified".
      • Repeat steps 1 - 3 but set moodle/course:delete to 'Allow'.

      [Test] Expected outcome: You should now be able to move courses to different categories.

      Show
      Things to do Create a course creator Add a couple of categories Add a couple of courses under one of the categories Testing steps Go to [Settings ► Site administration ► Users ► Permissions ► Define roles] Click on Course creator. Click edit and show advanced and then do a search for site:manageblocks and set it to 'Allow'. Click 'allow' for manage categories (moodle/category:manage). login as the course creator. Go to [Settings ► Site administration ► Courses ► Add/edit courses] Click on misc. [Test] Try selecting and then moving courses to a different category. This will take you to an error page saying "You can not move this course to the category specified". Repeat steps 1 - 3 but set moodle/course:delete to 'Allow'. [Test] Expected outcome: You should now be able to move courses to different categories.
    • Affected Branches:
      MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-31750-master
    • Rank:
      38347

      Description

      For moving a course from one category to another user should have both course:create and category:manage capability.
      Unfortunately, we are just checking for course:create capability in course/edit_form.php

      Also, Course/category.php should follow the same checks.

        Issue Links

          Activity

          Hide
          Adrian Greeve added a comment -

          I've added a check for moodle/course:create and moodle/category:manage where necessary. I also tidied up some of the context calls to use the more uptodate functions.

          Show
          Adrian Greeve added a comment - I've added a check for moodle/course:create and moodle/category:manage where necessary. I also tidied up some of the context calls to use the more uptodate functions.
          Hide
          Rajesh Taneja added a comment -

          Thanks for fixing this Adrian,

          Few things you might want to consider:
          course/category.php

          1. user should have course:create and category:manage permissions on both categories. You are just checking that on source category not on destination category.
          2. get_context_instance is depricated, so might want to remove it.
          3. You might want to fix inconsistent use of moveto variable, some places it is $moveto and some places $data->moveto
          4. No checks done while creating a combo box on the bottom.
          5. User should not be able to select course, if user doesn't have capabilty on a course.
            course/edit_form.php
          6. update_course doesn't do any check, so prone to hacking
          7. You should not change https://github.com/abgreeve/moodle/compare/wip-MDL-31750-master#L2L48, as this is new course you just need to check for course:create permission.
          8. make_categories_list should check both permissions, while editing course (not while adding new)
          Show
          Rajesh Taneja added a comment - Thanks for fixing this Adrian, Few things you might want to consider: course/category.php user should have course:create and category:manage permissions on both categories. You are just checking that on source category not on destination category. get_context_instance is depricated, so might want to remove it. You might want to fix inconsistent use of moveto variable, some places it is $moveto and some places $data->moveto No checks done while creating a combo box on the bottom. User should not be able to select course, if user doesn't have capabilty on a course. course/edit_form.php update_course doesn't do any check, so prone to hacking You should not change https://github.com/abgreeve/moodle/compare/wip-MDL-31750-master#L2L48 , as this is new course you just need to check for course:create permission. make_categories_list should check both permissions, while editing course (not while adding new)
          Hide
          Adrian Greeve added a comment -

          Hi Rajesh,

          Thanks for the additional help.

          1. Have now added a check for the destination category
          2. get_context_instance has been changed.
          3. $data->moveto has been changed to $moveto.
          4. At the moment make_categories_list does a check for both moodle/category:manage and moodle/course:create. Do you want a check to see if it reaches that piece of code?
          5. A check is now being done to see if the user has permission to manage categories before displaying these checkboxes.
          6. I had a close look at this area to see if a check is being done. When navigating to the page there are two variable that the user may arrive with. Depending on which one the user has will determine if they are editing or creating a new course. A check is done then to see if the user has permission to edit or create respectively.
          7. When creating a course, only moodle/course:create is now needed.
          8. This is also taken care of.

          Awaiting your next set of enquiries

          Show
          Adrian Greeve added a comment - Hi Rajesh, Thanks for the additional help. Have now added a check for the destination category get_context_instance has been changed. $data->moveto has been changed to $moveto. At the moment make_categories_list does a check for both moodle/category:manage and moodle/course:create. Do you want a check to see if it reaches that piece of code? A check is now being done to see if the user has permission to manage categories before displaying these checkboxes. I had a close look at this area to see if a check is being done. When navigating to the page there are two variable that the user may arrive with. Depending on which one the user has will determine if they are editing or creating a new course. A check is done then to see if the user has permission to edit or create respectively. When creating a course, only moodle/course:create is now needed. This is also taken care of. Awaiting your next set of enquiries
          Hide
          Rajesh Taneja added a comment -

          Hello Adrian,

          Changes look nice.

          FYI:
          In course/index.php, no proper checks are done. I think it will be nice to fix them as well

          Show
          Rajesh Taneja added a comment - Hello Adrian, Changes look nice. FYI: In course/index.php, no proper checks are done. I think it will be nice to fix them as well
          Hide
          Adrian Greeve added a comment -

          Thanks again Raj for the comments.

          I've left the function call get_context_instance() as is. MDL-33061 will update all of these call to the new API class call. This will also help for back porting to earlier versions.

          Show
          Adrian Greeve added a comment - Thanks again Raj for the comments. I've left the function call get_context_instance() as is. MDL-33061 will update all of these call to the new API class call. This will also help for back porting to earlier versions.
          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
          Adrian Greeve added a comment -

          Rebased and ready to go.

          Show
          Adrian Greeve added a comment - Rebased and ready to go.
          Hide
          Aparup Banerjee added a comment -

          Hi Adrian , a view at ur patch:

          • re get_context_instance() and waiting for MDL-33061 : sorry, patches using deprecated functions won't be accepted into master. infact, patches using deprecated functions won't be accepted full stop. might as well write it correctly now if you're changing that line in your patch. Please use context_coursecat::instance(). We want to move to a point where removing a deprecated function is safe (not risky or massive testing required).
          • the checks to allow the course move (moodle/course:create cap) would seem more appropriate within move_courses()
          • i wondered about needing moodle/course:create in the original context during a move as opposed to checking moodle/course:delete for the original context; but since we're actually updating the record within move_courses() and not deleting, it seems fine. oh course/search.php goes by that too (as a minimum). It fine but i still don't like it , 'moodle/course:delete' is the conceptual deletion of the course in the category imo. not the technical deletion from db - the techincal bits are defined as read/write within a permission. (perhaps moodle/course:update should be considered?)

          cheers!

          Show
          Aparup Banerjee added a comment - Hi Adrian , a view at ur patch: re get_context_instance() and waiting for MDL-33061 : sorry, patches using deprecated functions won't be accepted into master. infact, patches using deprecated functions won't be accepted full stop. might as well write it correctly now if you're changing that line in your patch. Please use context_coursecat::instance(). We want to move to a point where removing a deprecated function is safe (not risky or massive testing required). the checks to allow the course move (moodle/course:create cap) would seem more appropriate within move_courses() i wondered about needing moodle/course:create in the original context during a move as opposed to checking moodle/course:delete for the original context; but since we're actually updating the record within move_courses() and not deleting, it seems fine. oh course/search.php goes by that too (as a minimum). It fine but i still don't like it , 'moodle/course:delete' is the conceptual deletion of the course in the category imo. not the technical deletion from db - the techincal bits are defined as read/write within a permission. (perhaps moodle/course:update should be considered?) cheers!
          Hide
          Michael de Raadt added a comment -

          For those watching this issue... Adrian will be away for a couple of weeks. He will continue with this when he returns.

          Show
          Michael de Raadt added a comment - For those watching this issue... Adrian will be away for a couple of weeks. He will continue with this when he returns.
          Hide
          Adrian Greeve added a comment -

          Hi Apu,

          I've gone through your suggestions and made fixes where appropriate. Re-submitting for integration review.

          Show
          Adrian Greeve added a comment - Hi Apu, I've gone through your suggestions and made fixes where appropriate. Re-submitting for integration review.
          Hide
          Aparup Banerjee added a comment -

          Hi Adrian,

          i've got some comments from others and its a really crazy time right now to get this in.

          also noting that the test has to be updated along with any changes to code in the patch.

          just recording some feedback here for when we come back to this..

          (14:29:49) samhemelryk: ummm, can_edit_in_category
          (14:30:50) samhemelryk: category:manage => required to move subcategories and that category
          course:update => required to move course right?
          course:create => required to create course in category?
          Should that function be checking all three?

          Show
          Aparup Banerjee added a comment - Hi Adrian, i've got some comments from others and its a really crazy time right now to get this in. also noting that the test has to be updated along with any changes to code in the patch. just recording some feedback here for when we come back to this.. (14:29:49) samhemelryk: ummm, can_edit_in_category (14:30:50) samhemelryk: category:manage => required to move subcategories and that category course:update => required to move course right? course:create => required to create course in category? Should that function be checking all three?
          Hide
          Aparup Banerjee added a comment -

          taking this out of current integration for review later. (too near to release for this)

          Show
          Aparup Banerjee added a comment - taking this out of current integration for review later. (too near to release for this)
          Hide
          Michael de Raadt added a comment -

          Carrying over to new sprint.

          Show
          Michael de Raadt added a comment - Carrying over to new sprint.
          Hide
          Tim Hunt added a comment -

          One comment from me on this patch: https://github.com/abgreeve/moodle/compare/wip-MDL-31750-master#L0L96 I don't like the fact that you have taken out this line. An important principle with category editing is to not let someone do something they cannot undo. Therefore the permission checks have to be symmetrical.

          Show
          Tim Hunt added a comment - One comment from me on this patch: https://github.com/abgreeve/moodle/compare/wip-MDL-31750-master#L0L96 I don't like the fact that you have taken out this line. An important principle with category editing is to not let someone do something they cannot undo. Therefore the permission checks have to be symmetrical.
          Hide
          Adrian Greeve added a comment -

          Thanks for the comment Tim. This line hasn't been deleted, just moved into the move_courses() function so that we don't have to repeat the check in different areas.

          Show
          Adrian Greeve added a comment - Thanks for the comment Tim. This line hasn't been deleted, just moved into the move_courses() function so that we don't have to repeat the check in different areas.
          Hide
          Tim Hunt added a comment -

          Ah OK. Sorry, I should have read the patch more carefully.

          Show
          Tim Hunt added a comment - Ah OK. Sorry, I should have read the patch more carefully.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          For the time being (6h before packaging weeklies and minor releases), I don't think this can be integrated properly, so I'm sending it out from current integration.

          Just as a side-comment (I've not looked to code), if this is a security issue (in any degree), we should provide fixes for all supported branches (aka, from 21 to 24 nowadays).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - For the time being (6h before packaging weeklies and minor releases), I don't think this can be integrated properly, so I'm sending it out from current integration. Just as a side-comment (I've not looked to code), if this is a security issue (in any degree), we should provide fixes for all supported branches (aka, from 21 to 24 nowadays). Ciao
          Hide
          Sam Hemelryk added a comment -

          Hi Adrian,

          I'm going to send this back this week sorry, it still needs more work.
          First up a couple of notes I made looking at code.

          • This is a security issue and a blocker, for sure it needs to be backported to 21, 22, and 23 at least. This will also factor into points later on.
          • Moving the capability checks into move_courses is perhaps a bad move. move_courses until now has been without capability checks, purely an API function that does what it suggests. After this change it is checking half of the capabilities required in order to move courses, it may save on a couple of lines initially but as only half the caps are being checked calling code still needs to make its own checks anyway and now instead of having all of those checks in close proximity they are spread between action code and api.
            On top of that it breaks areas using this code in areas not requiring those capability checks (internal API calls) for instance category_delete_move calls it and may not require thoe checks (I havn't mapped its cap calls, you can do that, but I would suspect either not required or not corect presently). In the same way its not a backwards compatible and therefor would need to be strongly considered before backporting.
          • can_edit_in_category. You've removed moodle/course:create and added moodle/course:update. Without investigating it too deeply I would think any cap that is required to edit should be considered there. moodle/category:update is there anyway and in order to move a course you need that plus moodle/course:update so I don't think moodle/course:update is needed by itself, also moodle/course:create is standalone, you don't need any other caps to create a course do you? if correct then that should definitely be in there. Anyway use cases should be looked at and tested to support any changes there.

          Next the capabilites themselves. The category capability checks are very messy at the moment hence the issue.
          The first thing I did when starting to review this was to map out the locations where courses are moved between categories and document the capabilities.
          The following are my results.

          category.php

          • Action: Moving a course between categories
            • moodle/course:update on the origin category
            • moodle/category:update on the origin category
            • moodle/category:update on the destination category
            • moodle/category:manage on destination category (through move courses)
            • moodle/course:update on destination category (through move courses)
          • Output: Moving a course
            • moodle/category:manage on the origin category
            • moodle/course:update on the course
          • Output: List of categories to move to
            • moodle/category:manage on the destination category
            • moodle/course:update on the destination category

          edit_form.php

          • Output: Move course between categories
            • moodle/course:update on the origin category
            • moodle/category:manage on the origin category
            • moodle/course:update on the destination category
            • moodle/category:manage on the destination category

          lib.php

          • can_edit_in_category
            • moodle/category:manage in category
            • moodle/course:update in category
          • move_courses
            • moodle/category:manage in destination category
            • moodle/course:update in destination category

          search.php

          • Action: Move course between categories
            • moodle/course:create on course in origin category
            • moodle/category:manage on course in origin category
            • moodle/category:manage on destination category (through move courses)
            • moodle/course:update on destination category (through move courses)

          Its certainly an improvement on what was there but there is still inconsistency those checks that I think need to be recitifed.

          Moving on again the other thing that struck me is that we have moodle/course:changecategory in the docs described as "This allows a user to change the course category".
          How does this capability get factored in? I couldn't find reference to it in this issue but I do hope it has been discussed.
          Deprecating it from master is perhaps fine if we are sure it is best, however it is a risky change to be backporting perhaps.

          Anyway a bit to think about there perhaps, or at least to clarify.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Adrian, I'm going to send this back this week sorry, it still needs more work. First up a couple of notes I made looking at code. This is a security issue and a blocker, for sure it needs to be backported to 21, 22, and 23 at least. This will also factor into points later on. Moving the capability checks into move_courses is perhaps a bad move. move_courses until now has been without capability checks, purely an API function that does what it suggests. After this change it is checking half of the capabilities required in order to move courses, it may save on a couple of lines initially but as only half the caps are being checked calling code still needs to make its own checks anyway and now instead of having all of those checks in close proximity they are spread between action code and api. On top of that it breaks areas using this code in areas not requiring those capability checks (internal API calls) for instance category_delete_move calls it and may not require thoe checks (I havn't mapped its cap calls, you can do that, but I would suspect either not required or not corect presently). In the same way its not a backwards compatible and therefor would need to be strongly considered before backporting. can_edit_in_category. You've removed moodle/course:create and added moodle/course:update. Without investigating it too deeply I would think any cap that is required to edit should be considered there. moodle/category:update is there anyway and in order to move a course you need that plus moodle/course:update so I don't think moodle/course:update is needed by itself, also moodle/course:create is standalone, you don't need any other caps to create a course do you? if correct then that should definitely be in there. Anyway use cases should be looked at and tested to support any changes there. Next the capabilites themselves. The category capability checks are very messy at the moment hence the issue. The first thing I did when starting to review this was to map out the locations where courses are moved between categories and document the capabilities. The following are my results. category.php Action: Moving a course between categories moodle/course:update on the origin category moodle/category:update on the origin category moodle/category:update on the destination category moodle/category:manage on destination category (through move courses) moodle/course:update on destination category (through move courses) Output: Moving a course moodle/category:manage on the origin category moodle/course:update on the course Output: List of categories to move to moodle/category:manage on the destination category moodle/course:update on the destination category edit_form.php Output: Move course between categories moodle/course:update on the origin category moodle/category:manage on the origin category moodle/course:update on the destination category moodle/category:manage on the destination category lib.php can_edit_in_category moodle/category:manage in category moodle/course:update in category move_courses moodle/category:manage in destination category moodle/course:update in destination category search.php Action: Move course between categories moodle/course:create on course in origin category moodle/category:manage on course in origin category moodle/category:manage on destination category (through move courses) moodle/course:update on destination category (through move courses) Its certainly an improvement on what was there but there is still inconsistency those checks that I think need to be recitifed. Moving on again the other thing that struck me is that we have moodle/course:changecategory in the docs described as "This allows a user to change the course category". How does this capability get factored in? I couldn't find reference to it in this issue but I do hope it has been discussed. Deprecating it from master is perhaps fine if we are sure it is best, however it is a risky change to be backporting perhaps. Anyway a bit to think about there perhaps, or at least to clarify. Cheers Sam
          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
          Adrian Greeve added a comment -

          Issue size: XL

          Show
          Adrian Greeve added a comment - Issue size: XL
          Hide
          Adrian Greeve added a comment -

          We had a brief discussion about this issue during the scrum today. Fred mentioned that in issue MDL-8307 that moodle/course:changecategory is being used. I seem to remember having a conversation about using that capability, but I neglected to record that in this issue. I'll reconsider using that capability in this issue. As for inconsistency of the checks. I struggling to get my head around the large amount of different checks required to administer courses and categories.

          Show
          Adrian Greeve added a comment - We had a brief discussion about this issue during the scrum today. Fred mentioned that in issue MDL-8307 that moodle/course:changecategory is being used. I seem to remember having a conversation about using that capability, but I neglected to record that in this issue. I'll reconsider using that capability in this issue. As for inconsistency of the checks. I struggling to get my head around the large amount of different checks required to administer courses and categories.
          Hide
          Rajesh Taneja added a comment -

          Hello Sam and Adrian,

          I was revisiting this issue and it seems we need to decide on moodle/course:changecategory (http://tracker.moodle.org/browse/MDL-31640?focusedCommentId=144629&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-144629). This has been discussed in past in MDL-31640 and Tim's information on capability checks (http://tracker.moodle.org/browse/MDL-31640?focusedCommentId=144492&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-144492) might help us to group capabilities for different course/category actions.

          Thanks Sam, for grouping all capabilities for different actions and IMO they make sense. Also, we should consider deprecating moodle/course:changecategory capability as this doesn't help much.

          As per MDL-8307, MD wanted that to be simple for users, so whole lot capability check was ignored.

          Show
          Rajesh Taneja added a comment - Hello Sam and Adrian, I was revisiting this issue and it seems we need to decide on moodle/course:changecategory ( http://tracker.moodle.org/browse/MDL-31640?focusedCommentId=144629&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-144629 ). This has been discussed in past in MDL-31640 and Tim's information on capability checks ( http://tracker.moodle.org/browse/MDL-31640?focusedCommentId=144492&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-144492 ) might help us to group capabilities for different course/category actions. Thanks Sam, for grouping all capabilities for different actions and IMO they make sense. Also, we should consider deprecating moodle/course:changecategory capability as this doesn't help much. As per MDL-8307 , MD wanted that to be simple for users, so whole lot capability check was ignored.
          Hide
          Adrian Greeve added a comment - - edited

          Hello everyone,

          I was revisiting this, and it seems we need to first decide what permissions are valid for performing category/course actions.

          Me and Rajesh have consolidated the list of permissions for different actions on category/course. It will be helpful if you can provide your feedback on this matter, so that we can move forward with an appropriate solution.

          The listing below is an example of how we thing the permissions should work with multiple scenarios for moving a course to a different category.

          Category managemenet on course/index.php

          Move category to

          Source category Destination category
          moodle/category:manage moodle/category:manage

          Edit category

          Source category
          moodle/category:manage

          Show/hide categories (needs to be updated in docs)

          Source category
          moodle/category:manage

          re-order categories (needs to be updated in docs)

          Source category
          moodle/category:manage

          Delete category

          Source category
          moodle/category:manage
          moodle/course:delete (If courses are present)

          Create new category:

          Source category
          moodle/category:manage

          Create a new course

          category drop-down should check for

          System level default category Destination category
          moodle/course:changecategory moodle/category:manage moodle/category:manage
            moodle/course:create moodle/course:create

          Course managemenet course/category.php

          Edit course

          moodle/course:update

          Enrol users

          moodle/course:enrolreview (Needs to be updated in 23 docs)

          Delete course

          moodle/course:delete

          hide/show course

          moodle/course:visibility
          moodle/course:viewhiddencourses

          Backup

          moodle/backup:backupcourse

          Restore

          moodle/restore:restorecourse

          re-order course

          moodle/category:manage

          Edit category

          moodle/category:manage

          Create sub-category

          moodle/category:manage

          Move a course between categories

          Spec 1:

          Source category Destination category
          moodle/category:manage moodle/category:manage
          Course:delete course:create

          But as per Tim's suggestion, while moving a course between categories, a user should be able to undo his/her changes. So
          specs 2:

          Source category Destination category
          moodle/category:manage moodle/category:manage
          Course:create course:create
          Course:delete course:delete

          We can also think of using existing course:changecategory (IMO this is appropiate and should be used.)
          Specs 3

          Course Current course Category Destination Category
          moodle/course:changecategory moodle/category:manage moodle/category:manage
            moodle/course:delete moodle/course:create

          Course editing course/edit.php

          Change category

          Course Current course Category Destination Category
          moodle/course:changecategory moodle/category:manage moodle/category:manage
            moodle/course:delete moodle/course:create

          FYI:
          course:update is only for course settings updates.

          Show
          Adrian Greeve added a comment - - edited Hello everyone, I was revisiting this, and it seems we need to first decide what permissions are valid for performing category/course actions. Me and Rajesh have consolidated the list of permissions for different actions on category/course. It will be helpful if you can provide your feedback on this matter, so that we can move forward with an appropriate solution. The listing below is an example of how we thing the permissions should work with multiple scenarios for moving a course to a different category. Category managemenet on course/index.php Move category to Source category Destination category moodle/category:manage moodle/category:manage Edit category Source category moodle/category:manage Show/hide categories (needs to be updated in docs) Source category moodle/category:manage re-order categories (needs to be updated in docs) Source category moodle/category:manage Delete category Source category moodle/category:manage moodle/course:delete (If courses are present) Create new category: Source category moodle/category:manage Create a new course category drop-down should check for System level default category Destination category moodle/course:changecategory moodle/category:manage moodle/category:manage   moodle/course:create moodle/course:create Course managemenet course/category.php Edit course moodle/course:update Enrol users moodle/course:enrolreview (Needs to be updated in 23 docs) Delete course moodle/course:delete hide/show course moodle/course:visibility moodle/course:viewhiddencourses Backup moodle/backup:backupcourse Restore moodle/restore:restorecourse re-order course moodle/category:manage Edit category moodle/category:manage Create sub-category moodle/category:manage Move a course between categories Spec 1: Source category Destination category moodle/category:manage moodle/category:manage Course:delete course:create But as per Tim's suggestion, while moving a course between categories, a user should be able to undo his/her changes. So specs 2: Source category Destination category moodle/category:manage moodle/category:manage Course:create course:create Course:delete course:delete We can also think of using existing course:changecategory (IMO this is appropiate and should be used.) Specs 3 Course Current course Category Destination Category moodle/course:changecategory moodle/category:manage moodle/category:manage   moodle/course:delete moodle/course:create Course editing course/edit.php Change category Course Current course Category Destination Category moodle/course:changecategory moodle/category:manage moodle/category:manage   moodle/course:delete moodle/course:create FYI: course:update is only for course settings updates.
          Hide
          Adrian Greeve added a comment -

          Current way things are working so that you can make an easy comparison:

          course/index.php

          Create a new course

          system context
          moodle/course:create

          Delete a category

          Course context Parent context
          moodle/category:manage moodle/category:manage

          Move a category

          parent category destination category
          moodle/category:manage moodle/category:manage

          Hide a category

          parent category
          moodle/category:manage

          Show a category

          parent category
          moodle/category:manage

          Re-order a category

          Current course
          moodle/category:manage

          Create a new category

          system context
          moodle/category:manage

          Course/category.php

          Resort the category

          page context
          moodle/category:manage

          Move a course to a new category

          page context destination category
          moodle/category:manage moodle/category:manage

          Hide or show a course

          course context
          moodle/course:visibility

          Re-order a course

          page context
          moodle/category:manage

          Output: edit a course

          Course context
          moodle/course:update

          Output: enrol users

          Course context
          moodle/course:enrolreview

          Output: Delete course

          course context
          moodle/course:delete
          • If created within a day: moodle/course:create

          Output: Show / hide a course

          Course context
          moodle/course:visibility
          moodle/course:viewhiddencourses

          Output: backup a course

          Course context
          moodle/backup:backupcourse

          Output: restore a course

          Course context
          moodle/restore:restorecourse

          Output: re-order a course

          Page context
          moodle/category:manage

          Output: move category list

          Page context
          moodle/category:manage

          Output: re-sort courses

          Page context
          moodle/category:manage

          Output: create a new course

          Page context
          moodle/course:create

          Course/edit.php

          Editing a course

          Course context
          moodle/course:update

          Create a new course

          Category context
          moodle/course:create
          Show
          Adrian Greeve added a comment - Current way things are working so that you can make an easy comparison: course/index.php Create a new course system context moodle/course:create Delete a category Course context Parent context moodle/category:manage moodle/category:manage Move a category parent category destination category moodle/category:manage moodle/category:manage Hide a category parent category moodle/category:manage Show a category parent category moodle/category:manage Re-order a category Current course moodle/category:manage Create a new category system context moodle/category:manage Course/category.php Resort the category page context moodle/category:manage Move a course to a new category page context destination category moodle/category:manage moodle/category:manage Hide or show a course course context moodle/course:visibility Re-order a course page context moodle/category:manage Output: edit a course Course context moodle/course:update Output: enrol users Course context moodle/course:enrolreview Output: Delete course course context moodle/course:delete If created within a day: moodle/course:create Output: Show / hide a course Course context moodle/course:visibility moodle/course:viewhiddencourses Output: backup a course Course context moodle/backup:backupcourse Output: restore a course Course context moodle/restore:restorecourse Output: re-order a course Page context moodle/category:manage Output: move category list Page context moodle/category:manage Output: re-sort courses Page context moodle/category:manage Output: create a new course Page context moodle/course:create Course/edit.php Editing a course Course context moodle/course:update Create a new course Category context moodle/course:create
          Hide
          Frédéric Massart added a comment - - edited

          My +1 for the use of moodle/course:changecategory to move a course to another one.

          If such a capability exists, although it is not widely used, it means what it means and giving the right to a user to change the category of a course imply that they can move it somewhere else! We could probably complexify things more and check whether the destination category accepts creation from this user, but we'd confuse the users and ourselves too much!

          Regarding "the user has the ability to revert what he just did", I partially agree. Again it adds complexity for a case scenario which is not really likely to happen and can be fixed by the admin. Also, using moodle/course:changecategory would still allow the user to revert his changes, assuming he has the permission in the destination category, which IMO, we should not check for.

          Show
          Frédéric Massart added a comment - - edited My +1 for the use of moodle/course:changecategory to move a course to another one. If such a capability exists, although it is not widely used, it means what it means and giving the right to a user to change the category of a course imply that they can move it somewhere else! We could probably complexify things more and check whether the destination category accepts creation from this user, but we'd confuse the users and ourselves too much! Regarding "the user has the ability to revert what he just did", I partially agree. Again it adds complexity for a case scenario which is not really likely to happen and can be fixed by the admin. Also, using moodle/course:changecategory would still allow the user to revert his changes, assuming he has the permission in the destination category, which IMO, we should not check for.
          Hide
          Ankit Agarwal added a comment -

          I personally don't like the idea of Overlapping capabilities, they might be a easy solution out, from coding point of view, but they create lots of confusion down the line. consider profile viewing for example, we have so many different capabilities overlapping and conflicting with each other. Hence my +1 goes to step 2.

          Thanks

          Show
          Ankit Agarwal added a comment - I personally don't like the idea of Overlapping capabilities, they might be a easy solution out, from coding point of view, but they create lots of confusion down the line. consider profile viewing for example, we have so many different capabilities overlapping and conflicting with each other. Hence my +1 goes to step 2. Thanks
          Hide
          David Monllaó added a comment -

          For me your proposal has sense, I've only have doubts about:

          • About course/index.php -> delete category, I don't know the capability checkings workflow of that page, but 'moodle/course:delete' should only be required when "What to do" is "Delete all"
          • 'moodle/course:changecategory' seems IMO to be too much specific to be a capability, I'd rather prefer the category:manage + course:create + course:delete in both origin and destination
          Show
          David Monllaó added a comment - For me your proposal has sense, I've only have doubts about: About course/index.php -> delete category, I don't know the capability checkings workflow of that page, but 'moodle/course:delete' should only be required when "What to do" is "Delete all" 'moodle/course:changecategory' seems IMO to be too much specific to be a capability, I'd rather prefer the category:manage + course:create + course:delete in both origin and destination
          Hide
          Sam Hemelryk added a comment -

          Ok having stared at this for a little while I have to admin I am still a little confused about things here.
          I suppose it is because no matter how long I stare at your analysis Adrian I keep having scroll up and down between what is purposed and what is there, and as I have a truly poor short term memory I keep forgetting what I have already paired together in my head.
          Definitely myself to fault there, not you, but it makes it rather hard to try to summarise what you are purposing.
          Ideally, as an integrator what I would like to see come through is an organised moved towards an API, not a complete course+category API, but the core actions organised into separate functions and then used consistently throughout Moodle.
          Perhaps two sets of functions would be best, a set of can_perform_some_action and then perform_some_action.
          I'd bet some of those functions exist already and perhaps really we just lack the can_perform_some_action functions.
          The use of the API is about consistency after that we need to a worry about what each is checking.
          Also because at some point the course+category management pages/process is up for review this would provide the most useful and flexible approach. Backporting will need to be taken into consideration of course, however if we get it solved first in master we can address the stable branches later. This is a security issue but really there are plenty of capability checks going, its just that they are all over the place currently.
          If that approach is going to be the chosen one then I'd love to see two things a: What functions we're going to need, and b: for each function what elements are important and what capability checks would/should be made on each.
          Again just me struggling to piece it together at the moment, really if it is an organised approach that promotes 100% consistent actions across all areas and is bi-directional (do + undo) then I think run with it.

          In reply to others comments:

          Fred:
          Just in regards to the user having the ability to undo what they just did, this is mondo important from a usability perspective. Admin's arn't always available, and certainly users don't want to have to track down someone with more power to undo something they perhaps accidentally did.
          We don't have to provide that bi-directional flow, but we choose to. Also I consider it part of a smart design.

          Ankit:
          Overlapping capabilities is not an ideal situation I agree, but in some situations it cannot be avoided. Two situations spring to mind when thinking of where it would occur, the first in order to provide bi-directional flow (the ability to do and then undo) and second in cases where there isn't sufficient complexity or demand to warrant a more granular capability. Of course it can happen that we have poorly organised/thought-out capabilities, or two capabilities to do the same thing in which case it certainly can be seen as an issue (although not one to fix as part of this issue as it must be backported).
          I suppose what I am really saying is that there is nothing inherently wrong with having stacked capabilities, as long as there is reason for it.

          David:

          • Not to sure what you are seeing here, I havn't looked into it, but anything that results in the course record being removed needs to check course:delete. Any other situations (perhaps reset) should likely have their own capabilities.
          • moodle/course:changecategory is specific, that is true. But it serves an important function, and builds into a feature. The use case for this capability if that you want a teacher to be able to make changes to a course but not move the course around as they desire, essentially you have two people one who creates the structure of the site and another who creates the content. Its a basic but very true use case.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Ok having stared at this for a little while I have to admin I am still a little confused about things here. I suppose it is because no matter how long I stare at your analysis Adrian I keep having scroll up and down between what is purposed and what is there, and as I have a truly poor short term memory I keep forgetting what I have already paired together in my head. Definitely myself to fault there, not you, but it makes it rather hard to try to summarise what you are purposing. Ideally, as an integrator what I would like to see come through is an organised moved towards an API, not a complete course+category API, but the core actions organised into separate functions and then used consistently throughout Moodle. Perhaps two sets of functions would be best, a set of can_perform_some_action and then perform_some_action. I'd bet some of those functions exist already and perhaps really we just lack the can_perform_some_action functions. The use of the API is about consistency after that we need to a worry about what each is checking. Also because at some point the course+category management pages/process is up for review this would provide the most useful and flexible approach. Backporting will need to be taken into consideration of course, however if we get it solved first in master we can address the stable branches later. This is a security issue but really there are plenty of capability checks going, its just that they are all over the place currently. If that approach is going to be the chosen one then I'd love to see two things a: What functions we're going to need, and b: for each function what elements are important and what capability checks would/should be made on each. Again just me struggling to piece it together at the moment, really if it is an organised approach that promotes 100% consistent actions across all areas and is bi-directional (do + undo) then I think run with it. In reply to others comments: Fred: Just in regards to the user having the ability to undo what they just did, this is mondo important from a usability perspective. Admin's arn't always available, and certainly users don't want to have to track down someone with more power to undo something they perhaps accidentally did. We don't have to provide that bi-directional flow, but we choose to. Also I consider it part of a smart design. Ankit: Overlapping capabilities is not an ideal situation I agree, but in some situations it cannot be avoided. Two situations spring to mind when thinking of where it would occur, the first in order to provide bi-directional flow (the ability to do and then undo) and second in cases where there isn't sufficient complexity or demand to warrant a more granular capability. Of course it can happen that we have poorly organised/thought-out capabilities, or two capabilities to do the same thing in which case it certainly can be seen as an issue (although not one to fix as part of this issue as it must be backported). I suppose what I am really saying is that there is nothing inherently wrong with having stacked capabilities, as long as there is reason for it. David: Not to sure what you are seeing here, I havn't looked into it, but anything that results in the course record being removed needs to check course:delete. Any other situations (perhaps reset) should likely have their own capabilities. moodle/course:changecategory is specific, that is true. But it serves an important function, and builds into a feature. The use case for this capability if that you want a teacher to be able to make changes to a course but not move the course around as they desire, essentially you have two people one who creates the structure of the site and another who creates the content. Its a basic but very true use case. Cheers Sam
          Hide
          Adrian Greeve added a comment -

          Thanks Sam for your comments,

          I really like the idea of creating a function to check capabilities and in whole make the whole code easier to read.

          I'm moving this into the next sprint to continue development.

          Show
          Adrian Greeve added a comment - Thanks Sam for your comments, I really like the idea of creating a function to check capabilities and in whole make the whole code easier to read. I'm moving this into the next sprint to continue development.
          Hide
          Adrian Greeve added a comment -

          I've created a function to check the capabilities for moving a course to another category. It should be easy enough to change this function if we decided that we want the checks done differently. I had a talk with Sam and we agreed that refining the scope of this fix to just checking the capabilities for moving courses around capabilities was a good idea and other checks could be raised as separate issues.

          Thanks to Rajesh for creating the unit tests that accompany this patch.

          Show
          Adrian Greeve added a comment - I've created a function to check the capabilities for moving a course to another category. It should be easy enough to change this function if we decided that we want the checks done differently. I had a talk with Sam and we agreed that refining the scope of this fix to just checking the capabilities for moving courses around capabilities was a good idea and other checks could be raised as separate issues. Thanks to Rajesh for creating the unit tests that accompany this patch.
          Hide
          Rajesh Taneja added a comment -

          Patch looks good Adrian,

          You might want to fix few things before you push it for integration:

          1. Phpdoc https://github.com/abgreeve/moodle/compare/wip-MDL-31750-master#L3R4533 it should be int|array. AFAIK mixed is not allowed.
          2. https://github.com/abgreeve/moodle/compare/wip-MDL-31750-master#L4R330 array key should be plural (courses)
          3. Might be nice to rename key from move to something which defines action and course (within it) to movecourse or something similar https://github.com/abgreeve/moodle/compare/wip-MDL-31750-master#L4R335
          4. Please add trailing period (.) after every comment https://github.com/abgreeve/moodle/compare/wip-MDL-31750-master#L4R358
          5. It might be nice to add some comment at https://github.com/abgreeve/moodle/compare/wip-MDL-31750-master#L4R392.
          Show
          Rajesh Taneja added a comment - Patch looks good Adrian, You might want to fix few things before you push it for integration: Phpdoc https://github.com/abgreeve/moodle/compare/wip-MDL-31750-master#L3R4533 it should be int|array. AFAIK mixed is not allowed. https://github.com/abgreeve/moodle/compare/wip-MDL-31750-master#L4R330 array key should be plural (courses) Might be nice to rename key from move to something which defines action and course (within it) to movecourse or something similar https://github.com/abgreeve/moodle/compare/wip-MDL-31750-master#L4R335 Please add trailing period (.) after every comment https://github.com/abgreeve/moodle/compare/wip-MDL-31750-master#L4R358 It might be nice to add some comment at https://github.com/abgreeve/moodle/compare/wip-MDL-31750-master#L4R392 .
          Hide
          Adrian Greeve added a comment - - edited

          Thanks for having another look at this Raj,

          I've made all the suggested changes.
          Submitting for integration review.

          I had a talk with Rajesh and he mentioned that the permissions required for moving courses to different categories has changed. Documentation will be required to explain the new changes.
          Master only please.

          Show
          Adrian Greeve added a comment - - edited Thanks for having another look at this Raj, I've made all the suggested changes. Submitting for integration review. I had a talk with Rajesh and he mentioned that the permissions required for moving courses to different categories has changed. Documentation will be required to explain the new changes. Master only please.
          Hide
          Sam Hemelryk added a comment -

          Hi Adrian, I hope its OK but I think at this point this issue is best left until after release.
          Sorry about the delay in looking to this.
          Sam

          Show
          Sam Hemelryk added a comment - Hi Adrian, I hope its OK but I think at this point this issue is best left until after release. Sorry about the delay in looking to this. Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (master only), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (master only), thanks!
          Hide
          David Monllaó added a comment -

          It fails, logged as a teacher I can not edit the course settings.

          Show
          David Monllaó added a comment - It fails, logged as a teacher I can not edit the course settings.
          Hide
          Dan Poltawski added a comment -

          Reverted (painfully) and reopened as requested by adrian.

          Show
          Dan Poltawski added a comment - Reverted (painfully) and reopened as requested by adrian.
          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
          Martin Dougiamas added a comment -

          Sam and Marina, can you please:

          • Read this
          • Absorb it
          • Post what you think (and how it relates to the course list/management you are doing)

          My thinking is that if we do have to make slight changes to the permissions for course creators (and similar roles) in order to make everything consistent and nice from the very top of the category tree right down to editing a single course, then we should do it and do it now in 2.5 when all the course management GUI is changing anyway.

          Show
          Martin Dougiamas added a comment - Sam and Marina, can you please: Read this Absorb it Post what you think (and how it relates to the course list/management you are doing) My thinking is that if we do have to make slight changes to the permissions for course creators (and similar roles) in order to make everything consistent and nice from the very top of the category tree right down to editing a single course, then we should do it and do it now in 2.5 when all the course management GUI is changing anyway.
          Hide
          Sam Hemelryk added a comment -

          I spoke with Adrian about this just the other day.

          At this point looking over the change and thinking about what is required I personally feel that we would be best to add a new capabilities to handle moving courses within categories.
          First up the details:

          • New capabilities - category:movecourse
          • Capability checks as follows:
            • Create a course - course:create in category context
            • Delete a course - course:delete in course context
            • Edit a course - course:edit in course context
            • Create a category - category:manage in parent
            • Edit a category - category:manage in category context
            • Delete a category - category:manage in category context
            • Move a course - category:movecourse in current category context and in destination category context.
          • When editing a course and changing its category think about it as two actions, 1. edit the course details, 2. move the course between categories
          • Grant movecourse to manage and coursecreator by default

          The end result of this is that both managers and course creators will be able to move courses between categories and we won't need to modify the course creators current capabilities.
          Admins will still be able to restrict where creators can move courses from and too as well.

          What I don't like about modifying the course creator role is that we would need to give the creator the capability to delete courses in order to move courses. I think those two actions move and delete should be clearly separated.
          Our current approach appears to have been that in moving a course you are deleting it from one category and creating it in another.
          Worth noting is I don't know the history behind the course creator role. Perhaps giving them course:delete is a good thing anyway, I don't know.

          Its also a busy week on integration and I've not given this too much thought.
          I'm keen to see what you think Marina.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - I spoke with Adrian about this just the other day. At this point looking over the change and thinking about what is required I personally feel that we would be best to add a new capabilities to handle moving courses within categories. First up the details: New capabilities - category:movecourse Capability checks as follows: Create a course - course:create in category context Delete a course - course:delete in course context Edit a course - course:edit in course context Create a category - category:manage in parent Edit a category - category:manage in category context Delete a category - category:manage in category context Move a course - category:movecourse in current category context and in destination category context. When editing a course and changing its category think about it as two actions, 1. edit the course details, 2. move the course between categories Grant movecourse to manage and coursecreator by default The end result of this is that both managers and course creators will be able to move courses between categories and we won't need to modify the course creators current capabilities. Admins will still be able to restrict where creators can move courses from and too as well. What I don't like about modifying the course creator role is that we would need to give the creator the capability to delete courses in order to move courses. I think those two actions move and delete should be clearly separated. Our current approach appears to have been that in moving a course you are deleting it from one category and creating it in another. Worth noting is I don't know the history behind the course creator role. Perhaps giving them course:delete is a good thing anyway, I don't know. Its also a busy week on integration and I've not given this too much thought. I'm keen to see what you think Marina. Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          I should add I very much like what you've done here Adrian in splitting these actions out into a category API and that you've added unit tests for it.

          The splitting out into an API I think is hugely beneficial:

          • It allows us to detect problems in design like this and address them in a way that ensures consistency.
          • My work on the course/category management pages is going to involve splitting out other similar actions. This is probably one of the messiest so getting it sorted now is beneficial to my future work.
          • Moving existing code like this to API is going to be opening up more areas to caching. Without a standardised API writing cache conversions is much harder. Once we have the category code written as an API it will be much easier to look at caching category/course structure outlines.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - I should add I very much like what you've done here Adrian in splitting these actions out into a category API and that you've added unit tests for it. The splitting out into an API I think is hugely beneficial: It allows us to detect problems in design like this and address them in a way that ensures consistency. My work on the course/category management pages is going to involve splitting out other similar actions. This is probably one of the messiest so getting it sorted now is beneficial to my future work. Moving existing code like this to API is going to be opening up more areas to caching. Without a standardised API writing cache conversions is much harder. Once we have the category code written as an API it will be much easier to look at caching category/course structure outlines. Many thanks Sam
          Hide
          Martin Dougiamas added a comment -

          I also think this should not be treated like a course deletion/creation. Capabilities should be tied to what is really happening, and in the case of a course move you are actually editing the categories - the course itself is untouched.

          Show
          Martin Dougiamas added a comment - I also think this should not be treated like a course deletion/creation. Capabilities should be tied to what is really happening, and in the case of a course move you are actually editing the categories - the course itself is untouched.
          Hide
          Marina Glancy added a comment -

          I tried to read and absorb...
          Definitely agree that to move the course user does not NEED to have "delete" permission (although it helps if he does, see below)

          IMHO user must have "course:create" capability in the target category.

          Let's put it this way: if user is NOT allowed to create course in the category A, he can not move courses from any other category to category A. Otherwise the capability to create course may as well be system-level only.

          Also I think this should be the only capability for the target. I don't understand why user needs category:manage capability in the target category if he is allowed to create a course there from scratch.

          Regarding the source category I would say:

          • either (capability to edit AND to delete the course)
          • or (capability to edit the course AND category:movecourse in source category)
          • or (capability to backup the course AND category:movecourse in source category)

          (which all basically again have nothing to do with category:manage)
          Basically category:movecourse becomes a soft version of course:delete

          Sorry it seems to be yet another opinion.

          Show
          Marina Glancy added a comment - I tried to read and absorb... Definitely agree that to move the course user does not NEED to have "delete" permission (although it helps if he does, see below) IMHO user must have "course:create" capability in the target category . Let's put it this way: if user is NOT allowed to create course in the category A, he can not move courses from any other category to category A. Otherwise the capability to create course may as well be system-level only. Also I think this should be the only capability for the target. I don't understand why user needs category:manage capability in the target category if he is allowed to create a course there from scratch. Regarding the source category I would say: either (capability to edit AND to delete the course) or (capability to edit the course AND category:movecourse in source category) or (capability to backup the course AND category:movecourse in source category) (which all basically again have nothing to do with category:manage) Basically category:movecourse becomes a soft version of course:delete Sorry it seems to be yet another opinion.
          Hide
          Dan Poltawski added a comment -
          Show
          Dan Poltawski added a comment - Just to mention Tim's logic from a few years ago which he mentioned in the linked issue: https://tracker.moodle.org/browse/MDL-31640?focusedCommentId=144492&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-144492
          Hide
          Sam Hemelryk added a comment -

          Sorry Marina, I disagree there.

          Dan's pointed you to Tim's comment which is very pertinent.
          The user must be able to undo any action they have made. If we are checking create on the category that the course is being moved to we must also check it on the category it is being moved from. Such is the case with any other capability checks being made as well.

          As for using create I don't think the user needs it at all (of course by default they will).
          There is a very clear distinction between moving and creating. When creating the user is introducing new content to the site. When moving the user is simply moving. Nothing new and nothing removed.
          There is one use case I can think for which a user may have capabilities to move but not to create.
          An institution with an ldap server is using it for enrolments on their Moodle site. It is set to automatically create courses where they don't exist and all courses are created in a single category. It is then the responsibility of the user to move the courses into more appropriate categories.
          As for its context should move be added it shouldn't change, currently its coursecat which makes sense as courses can only be created in categories. Not sure why you see that change as necessary sorry.

          As for delete....
          At some point in the past it was decided not to give the course creator role the delete capability. I imagine because to the risk of data loss.
          http://docs.moodle.org/24/en/Course_creator_role says its possible for a course creator to delete a course.
          If you have a pull up can_delete_course in course/lib.php you'll find the hack to allow it.
          If the user has course:create and has created the course in the past 24 hours they are allowed to delete it.
          I don't know the full history of that role as I mentioned earlier but I would bet that the decision to add a hack to workaround capabilities wasn't added made lightly.
          Purely in regards to data loss I think giving the course creator role the capability to delete is a bad idea... of course sites that feel otherwise can override those capabilities.

          Anyway, I still stick by the idea of introducing a capability and clearly separating the create, delete, and move.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Sorry Marina, I disagree there. Dan's pointed you to Tim's comment which is very pertinent. The user must be able to undo any action they have made. If we are checking create on the category that the course is being moved to we must also check it on the category it is being moved from. Such is the case with any other capability checks being made as well. As for using create I don't think the user needs it at all (of course by default they will). There is a very clear distinction between moving and creating. When creating the user is introducing new content to the site. When moving the user is simply moving. Nothing new and nothing removed. There is one use case I can think for which a user may have capabilities to move but not to create. An institution with an ldap server is using it for enrolments on their Moodle site. It is set to automatically create courses where they don't exist and all courses are created in a single category. It is then the responsibility of the user to move the courses into more appropriate categories. As for its context should move be added it shouldn't change, currently its coursecat which makes sense as courses can only be created in categories. Not sure why you see that change as necessary sorry. As for delete.... At some point in the past it was decided not to give the course creator role the delete capability. I imagine because to the risk of data loss. http://docs.moodle.org/24/en/Course_creator_role says its possible for a course creator to delete a course. If you have a pull up can_delete_course in course/lib.php you'll find the hack to allow it. If the user has course:create and has created the course in the past 24 hours they are allowed to delete it. I don't know the full history of that role as I mentioned earlier but I would bet that the decision to add a hack to workaround capabilities wasn't added made lightly. Purely in regards to data loss I think giving the course creator role the capability to delete is a bad idea... of course sites that feel otherwise can override those capabilities. Anyway, I still stick by the idea of introducing a capability and clearly separating the create, delete, and move. Many thanks Sam
          Hide
          moodle.com added a comment -

          For those who might be following this issue... Adrian has gone on a short holiday and will return to this issue in a couple of weeks.

          Show
          moodle.com added a comment - For those who might be following this issue... Adrian has gone on a short holiday and will return to this issue in a couple of weeks.
          Hide
          Marina Glancy added a comment -

          Sam,
          new capability can solve it. But still we have to keep in mind that at the moment the control to move the course is available either from category edit page (available to those who have 'category/manage' cap or from course edit page (available to those who have 'course/edit' cap). If we introduce the new capability we have to make sure that our UI makes this action accessible in this rare (and maybe stupid) case when somebody has move capability but does not have any other.

          Show
          Marina Glancy added a comment - Sam, new capability can solve it. But still we have to keep in mind that at the moment the control to move the course is available either from category edit page (available to those who have 'category/manage' cap or from course edit page (available to those who have 'course/edit' cap). If we introduce the new capability we have to make sure that our UI makes this action accessible in this rare (and maybe stupid) case when somebody has move capability but does not have any other.
          Hide
          Sam Hemelryk added a comment -

          In regards to the two areas through which a user can move a course between categories:

          1. Management pages - we would need to ensure that if the user can move a category they can access the management pages and can move a course where permitted. If thats the only cap they hold thats fine.
          2. Course editing page - if they hold the cap allow them to move the category, if they don't then freeze it. If they only hold the move cap then they won't be able to get to that page anyway, I think that is an OK outcome, certainly keeps things simplier. They would still be able to move through the management pages.
          Show
          Sam Hemelryk added a comment - In regards to the two areas through which a user can move a course between categories: Management pages - we would need to ensure that if the user can move a category they can access the management pages and can move a course where permitted. If thats the only cap they hold thats fine. Course editing page - if they hold the cap allow them to move the category, if they don't then freeze it. If they only hold the move cap then they won't be able to get to that page anyway, I think that is an OK outcome, certainly keeps things simplier. They would still be able to move through the management pages.
          Hide
          moodle.com added a comment -

          In order to resolve this, we need some consensus. Sam and Marina, you seem to have differing opinions. It would be good if you could come to some common ground. Michael d.

          Show
          moodle.com added a comment - In order to resolve this, we need some consensus. Sam and Marina, you seem to have differing opinions. It would be good if you could come to some common ground. Michael d.
          Hide
          Michael de Raadt added a comment -

          Shifting to the next sprint.

          Show
          Michael de Raadt added a comment - Shifting to the next sprint.
          Hide
          Marina Glancy added a comment -

          I agree with a new capability approach

          Show
          Marina Glancy added a comment - I agree with a new capability approach
          Hide
          Adrian Greeve added a comment -

          I had a talk to Sam about this issue and he suggested that this be reviewed after the changes that Marina is making in MDL-38147.

          Show
          Adrian Greeve added a comment - I had a talk to Sam about this issue and he suggested that this be reviewed after the changes that Marina is making in MDL-38147 .

            People

            • Votes:
              2 Vote for this issue
              Watchers:
              12 Start watching this issue

              Dates

              • Created:
                Updated: