Moodle
  1. Moodle
  2. MDL-31830

Replacement for the course and category management pages

    Details

    • Type: Epic Epic
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.3, 2.5.2
    • Fix Version/s: 2.6
    • Component/s: Course
    • Labels:
    • Story Points:
      100
    • Rank:
      52028

      Description

      This issue has been created to look at a possible replacement for the course and category management pages within Moodle.
      The management system we currently have is getting pretty antiquated and could really do with a make over and could be greatly streamlined to get all of the desired/common features onto fewer pages.
      It would also be great if the process of adding, moving and deleting categories could be simplified and likely JS enhancement could help this (think AJAX loading and drag + drop).

      1. 31830-s1.png
        143 kB
      2. 31830-s2.png
        180 kB
      3. alignment.png
        165 kB
      4. bonfire-screenshot-20131011-134543-108.png
        211 kB
      5. cleancourselist.png
        38 kB
      6. controls1.png
        16 kB
      7. controls2.png
        19 kB
      8. coursemanagement1.png
        180 kB
      9. coursemanagement2.png
        112 kB
      10. hidden.png
        27 kB
      11. hide.png
        28 kB
      12. hide show error.png
        11 kB
      13. howdoesthishappen.png
        37 kB
      14. long_categories.png
        58 kB
      15. moveCourse.png
        23 kB
      16. swapcategory.png
        12 kB
      17. ugly css.png
        27 kB
      18. what.png
        26 kB
      19. whatthe.png
        27 kB

        Issue Links

          Issues in Epic

            Activity

            Hide
            Tim Hunt added a comment -

            The unit tests added here assume that no course categorires are created on install by any add-ons. That is a false assumption. MDL-43926

            Show
            Tim Hunt added a comment - The unit tests added here assume that no course categorires are created on install by any add-ons. That is a false assumption. MDL-43926
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Yes, it's happening (somewhere in the French Polynesia, right now). And you did it, raising Moodle to new excellency levels.

            Or, if you prefer, yes, you fixed that boring issue.

            Thanks anyway! Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Yes, it's happening (somewhere in the French Polynesia, right now). And you did it, raising Moodle to new excellency levels. Or, if you prefer, yes, you fixed that boring issue. Thanks anyway! Ciao
            Hide
            Ankit Agarwal added a comment -

            The error is now gone. and since MDL-42245 is marked as must fix for 2.6 am passing this.

            Thanks

            Show
            Ankit Agarwal added a comment - The error is now gone. and since MDL-42245 is marked as must fix for 2.6 am passing this. Thanks
            Hide
            Damyon Wiese added a comment -

            One fix added and all follow up issues created - back to testing.

            Show
            Damyon Wiese added a comment - One fix added and all follow up issues created - back to testing.
            Hide
            Damyon Wiese added a comment -

            And Marinas: MDL-42304

            Show
            Damyon Wiese added a comment - And Marinas: MDL-42304
            Hide
            Damyon Wiese added a comment -

            From Ankits points above:

            1. is MDL-42251
            2. is MDL-42296
            3. is MDL-42297
            4. is MDL-42248
            5. is MDL-42298
            6. is MDL-42299
            7. is MDL-42249
            8. is MDL-42253
            9. is MDL-42300
            10. is MDL-42301
            11. is MDL-42302

            Show
            Damyon Wiese added a comment - From Ankits points above: 1. is MDL-42251 2. is MDL-42296 3. is MDL-42297 4. is MDL-42248 5. is MDL-42298 6. is MDL-42299 7. is MDL-42249 8. is MDL-42253 9. is MDL-42300 10. is MDL-42301 11. is MDL-42302
            Hide
            Damyon Wiese added a comment -

            Pushed a fix for the last error spotted by Ankit (moving 0 courses).

            It seems un-productive to revert this issue given there are already fixes based on it waiting for integration - so lets make sure we have followup issues created for the other bugs/improvements mentioned here (including the behat fail).

            Show
            Damyon Wiese added a comment - Pushed a fix for the last error spotted by Ankit (moving 0 courses). It seems un-productive to revert this issue given there are already fixes based on it waiting for integration - so lets make sure we have followup issues created for the other bugs/improvements mentioned here (including the behat fail).
            Hide
            Ankit Agarwal added a comment -

            Everything looks alright on IE (besides the things already commented on)

            Rosie found out that when you submit "move to category" drop down without selecting any categories following warning is generated
            Warning: Invalid argument supplied for foreach() in /var/www/int/master/moodle/course/management.php on line 293 Call Stack: 0.0007 357120 1.

            {main}

            () /var/www/int/master/moodle/course/management.php:0

            Am failing this as there seems to be a few things that can be fixed or improved.

            Show
            Ankit Agarwal added a comment - Everything looks alright on IE (besides the things already commented on) Rosie found out that when you submit "move to category" drop down without selecting any categories following warning is generated Warning: Invalid argument supplied for foreach() in /var/www/int/master/moodle/course/management.php on line 293 Call Stack: 0.0007 357120 1. {main} () /var/www/int/master/moodle/course/management.php:0 Am failing this as there seems to be a few things that can be fixed or improved.
            Hide
            Marina Glancy added a comment -

            just looked at this weird issue with right border alignment in Clean for courses that have idnumber and don't. From looking at CSS I've noticed that there is a lot of references to div idnumbers, for example #course-category-listings . This is not good (because it does not inherit styles). It really should be using the body path, such as .path-course-management

            Which is absent...

            ... maybe because

            navigation_node::override_active_url($url); 
            

            is still present in /course/management.php ...

            Show
            Marina Glancy added a comment - just looked at this weird issue with right border alignment in Clean for courses that have idnumber and don't. From looking at CSS I've noticed that there is a lot of references to div idnumbers, for example #course-category-listings . This is not good (because it does not inherit styles). It really should be using the body path, such as .path-course-management Which is absent... ... maybe because navigation_node::override_active_url($url); is still present in /course/management.php ...
            Hide
            David Monllaó added a comment -

            There is still a failure in the CI server, not appears always and is passing for me locally... any idea? Could be related with CAT4 not having enough time to get dimmed after it's parent get's dimmed.

            http://nightly01:8080/job/8.-%20Run%20all%20behat%20features%20(master)/166/console

            Show
            David Monllaó added a comment - There is still a failure in the CI server, not appears always and is passing for me locally... any idea? Could be related with CAT4 not having enough time to get dimmed after it's parent get's dimmed. http://nightly01:8080/job/8.-%20Run%20all%20behat%20features%20(master)/166/console
            Hide
            Ankit Agarwal added a comment - - edited

            I noticed the following issues, some of these might have already been noticed/not related. Just reporting things I noticed:-
            These are the results using firefox.

            1. Arrows are going out of screen for course listing div, refer screenshot. I was not able to see this on Adrian's system, so seems something specific to mine. Also as marina pointed out icons alignment is wrong when course has idnumber set.
            2. Cannot drag and move course by dragging the course name, need to drag arrow always.
            3. There is no support for moving categories up and down by dragging.
            4. We don't normally show up and down arrows when moving by drag and drop is an option, for example sections. But for course listing here we are showing both.
            5. Not sure if this is intentional. But when I try to edit a category , once I click save am redirected back to the category page for that particular category, irrespective of what page I was on before.
            6. When am clicking on Re-sort subcategories all expanded nodes are closed when the page reloads.
            7. May be it is just me, but the up and down arrows beside "sorting links" gives an impression that we are allowed to sort in both ascending and descending order. But that doesn't seems to be the case.
            8. I have around 20 course in a category and with 10 courses per page, I am given options as (1, 2, next, last) for pagination. There is no need for the "last" there imo.
            9. There are two ways to sort sub-categories on a category page. (Refer screenshot). A drop down and a link on top. Am I correct to understand the link is only for the sub categories of the category whose management page we are viewing, where as the drop down is for the ones selected by the checkbox? Looks a bit confusing tbh.
            10. Is it intentional that there is no way to delete a course from this page?
            11. Marking a category hidden, turns all sub-categories to hidden mode, makes the text dimmed, which is correct. However it doesn't do the same for all the courses in the same category.
            Show
            Ankit Agarwal added a comment - - edited I noticed the following issues, some of these might have already been noticed/not related. Just reporting things I noticed:- These are the results using firefox. Arrows are going out of screen for course listing div, refer screenshot. I was not able to see this on Adrian's system, so seems something specific to mine. Also as marina pointed out icons alignment is wrong when course has idnumber set. Cannot drag and move course by dragging the course name, need to drag arrow always. There is no support for moving categories up and down by dragging. We don't normally show up and down arrows when moving by drag and drop is an option, for example sections. But for course listing here we are showing both. Not sure if this is intentional. But when I try to edit a category , once I click save am redirected back to the category page for that particular category, irrespective of what page I was on before. When am clicking on Re-sort subcategories all expanded nodes are closed when the page reloads. May be it is just me, but the up and down arrows beside "sorting links" gives an impression that we are allowed to sort in both ascending and descending order. But that doesn't seems to be the case. I have around 20 course in a category and with 10 courses per page, I am given options as (1, 2, next, last) for pagination. There is no need for the "last" there imo. There are two ways to sort sub-categories on a category page. (Refer screenshot). A drop down and a link on top. Am I correct to understand the link is only for the sub categories of the category whose management page we are viewing, where as the drop down is for the ones selected by the checkbox? Looks a bit confusing tbh. Is it intentional that there is no way to delete a course from this page? Marking a category hidden, turns all sub-categories to hidden mode, makes the text dimmed, which is correct. However it doesn't do the same for all the courses in the same category.
            Hide
            David Monllaó added a comment -

            A MDL-42013 fix has been integrated and all tests including these ones are passing, waiting for CI

            Show
            David Monllaó added a comment - A MDL-42013 fix has been integrated and all tests including these ones are passing, waiting for CI
            Hide
            Andrew Nicols added a comment -

            FYI, I've started creating Usability + UI issues with this new interface in MDL-42245 now that this Epic has been integrated.

            Show
            Andrew Nicols added a comment - FYI, I've started creating Usability + UI issues with this new interface in MDL-42245 now that this Epic has been integrated.
            Hide
            David Monllaó added a comment -

            As discussed there are many cases and point of views to consider and is slow to check each solution as whole runs lasts for long time, so I would say that this is the best we can until we find a solution.

            Show
            David Monllaó added a comment - As discussed there are many cases and point of views to consider and is slow to check each solution as whole runs lasts for long time, so I would say that this is the best we can until we find a solution.
            Hide
            Damyon Wiese added a comment -

            Let me just confirm with David about how much work it is to fix the i_can_see - I would rather fix that than remove tests if possible.

            Show
            Damyon Wiese added a comment - Let me just confirm with David about how much work it is to fix the i_can_see - I would rather fix that than remove tests if possible.
            Hide
            Sam Hemelryk added a comment -

            Several behat tests I wrote for this issue have being failing.
            We've tracked the issue down to MDL-42013 which partially fixed "should not see" syntax but caused a regression for my tests as it doesn't take into account the visibility of node.
            An "in the mean time" solution has been requested, comment out the failing assertions.

            https://github.com/samhemelryk/moodle/commit/31830-26i
            git pull git://github.com/samhemelryk/moodle.git 31830-26i
            

            As part of fixing the visibility checks we should revert this commit:

            git revert 20730d9
            

            Damyon would you mind pulling that in for me.

            Show
            Sam Hemelryk added a comment - Several behat tests I wrote for this issue have being failing. We've tracked the issue down to MDL-42013 which partially fixed "should not see" syntax but caused a regression for my tests as it doesn't take into account the visibility of node. An "in the mean time" solution has been requested, comment out the failing assertions. https: //github.com/samhemelryk/moodle/commit/31830-26i git pull git: //github.com/samhemelryk/moodle.git 31830-26i As part of fixing the visibility checks we should revert this commit: git revert 20730d9 Damyon would you mind pulling that in for me.
            Hide
            Damyon Wiese added a comment -

            Thanks Sam,

            Looks like a winner to me.

            This has been integrated now - off to testing.

            I suggest 2 further css changes for your consideration:

            Make the cursor a pointer when hovering over the drag handles (standard and clean)

            Remove this style from base:

            #course-category-listings li[data-selected='1'] > div

            {background-image:url([[pix:t/collapsed]]);background-repeat: no-repeat; background-position: right center;}

            The arrow on the right side of the selected course/category looks odd IMO.

            Thanks again!

            Show
            Damyon Wiese added a comment - Thanks Sam, Looks like a winner to me. This has been integrated now - off to testing. I suggest 2 further css changes for your consideration: Make the cursor a pointer when hovering over the drag handles (standard and clean) Remove this style from base: #course-category-listings li [data-selected='1'] > div {background-image:url([[pix:t/collapsed]]);background-repeat: no-repeat; background-position: right center;} The arrow on the right side of the selected course/category looks odd IMO. Thanks again!
            Hide
            Sam Hemelryk added a comment -

            Alrighty I've fixed up the accessibility issues as well, I've merged them with the last commit so force pull sorry.

            Details are:

            1. The category tree is now presented using aria attributes as per http://www.w3.org/WAI/GL/wiki/Using_ARIA_trees
            2. Bulk action selectors now use aria-labelledby
            3. Category course count now given access hidden label.
            4. Pagination links now have better titles.
            5. The course action icons now present with role=button
            6. aria-selected is not valid in this context but it was a good idea.
            Show
            Sam Hemelryk added a comment - Alrighty I've fixed up the accessibility issues as well, I've merged them with the last commit so force pull sorry. Details are: The category tree is now presented using aria attributes as per http://www.w3.org/WAI/GL/wiki/Using_ARIA_trees Bulk action selectors now use aria-labelledby Category course count now given access hidden label. Pagination links now have better titles. The course action icons now present with role=button aria-selected is not valid in this context but it was a good idea.
            Hide
            Sam Hemelryk added a comment -

            Hey dude, thanks for looking at it.

            In reply to your first comment:

            Regarding the coding style preferences I feel completely the opposite way

            1. I am a fan of stdClass; from way back, at several times we've debated whether there is a preferred way however its always been split and I believe we settled that as long as its consistent its fine. I hope I am consistent, I am a lover of stdClass;
            2. Absolute namespaces for the win, anything else is guessing and open to intrusion. If you don't preceed with a starting slash then local is assumed and to me that suggests that the method relative to the current namespace. By always prefixing functions and classes I ensure that there is no guessing or potential mystery going on an instead I am, being absolute about the function or class I wish to use.

            Themey things:

            1. This made me chuckle a little (half because I expected it to come up). My reply in short is that I have styled to what is available, while its not bootstrap it works and is available without the need for additional styles. I think when bootstrap is our default theme that moving all uses of YUI3 style to it would be worthwhile, however for the time being as it envolves additional CSS in some themes I would rather not.
            2. Fixed the ' ' title for the create new button plus two other icons I found as well.
            3. Adding padding to items displayed in the action menu drop down. base + bootstrapbase.
            4. Padding adjusted to the same as on the course page. base + bootstrapbase. I'm not sure about this change truthfully, it bites into the available space before we have to split a list item over two lines. On a large screen it is fine, however on a medium screen while it is still two columns this is reasonably annoying. Lets see if anyone complains and I can change it again in the future.
            5. Removed hover icon from categories. base only.

            Others:

            1. Fixed categroy typo
            2. Removed min-width that was being applied to coursename making its clickable area larger than normal.
            3. That they would. I would rather have a separate issue to fix this under as I would like to this properly - creating a core component to allow this to be easily added to any list of checkboxes.
            4. Ahh yes this little beastie. I tried at some point to get M.core.dragdrop to apply instead of using Y.DD directly however it wasn't a smooth sail at all and as such I spent my time elsewhere. If its uber critical I can tackle it now, otherwise it will have to become a new issue.
            Show
            Sam Hemelryk added a comment - Hey dude, thanks for looking at it. In reply to your first comment: Regarding the coding style preferences I feel completely the opposite way I am a fan of stdClass; from way back, at several times we've debated whether there is a preferred way however its always been split and I believe we settled that as long as its consistent its fine. I hope I am consistent, I am a lover of stdClass; Absolute namespaces for the win, anything else is guessing and open to intrusion. If you don't preceed with a starting slash then local is assumed and to me that suggests that the method relative to the current namespace. By always prefixing functions and classes I ensure that there is no guessing or potential mystery going on an instead I am, being absolute about the function or class I wish to use. Themey things: This made me chuckle a little (half because I expected it to come up). My reply in short is that I have styled to what is available, while its not bootstrap it works and is available without the need for additional styles. I think when bootstrap is our default theme that moving all uses of YUI3 style to it would be worthwhile, however for the time being as it envolves additional CSS in some themes I would rather not. Fixed the ' ' title for the create new button plus two other icons I found as well. Adding padding to items displayed in the action menu drop down. base + bootstrapbase. Padding adjusted to the same as on the course page. base + bootstrapbase. I'm not sure about this change truthfully, it bites into the available space before we have to split a list item over two lines. On a large screen it is fine, however on a medium screen while it is still two columns this is reasonably annoying. Lets see if anyone complains and I can change it again in the future. Removed hover icon from categories. base only. Others: Fixed categroy typo Removed min-width that was being applied to coursename making its clickable area larger than normal. That they would. I would rather have a separate issue to fix this under as I would like to this properly - creating a core component to allow this to be easily added to any list of checkboxes. Ahh yes this little beastie. I tried at some point to get M.core.dragdrop to apply instead of using Y.DD directly however it wasn't a smooth sail at all and as such I spent my time elsewhere. If its uber critical I can tackle it now, otherwise it will have to become a new issue.
            Hide
            Damyon Wiese added a comment -

            OK - finished looking - thats everything for now - I'll wait till you've pinged me before looking again.

            Show
            Damyon Wiese added a comment - OK - finished looking - thats everything for now - I'll wait till you've pinged me before looking again.
            Hide
            Damyon Wiese added a comment -

            Accessibility things:

            1. It would be nice to use aria-expanded attributes on the categories.
            2. The select/go combinations should have aria-labelled by attributes
            3. The course count info could have some access-hidden text
            4. The pagination links all have title="Actions" which should be removed
            5. The action-icons for courses could have role="button"
            6. We could use aria-selected on the selected category/course

            (Note - I'm not expecting all these things addressed in integration - as long as we have issues created for them).

            Show
            Damyon Wiese added a comment - Accessibility things: 1. It would be nice to use aria-expanded attributes on the categories. 2. The select/go combinations should have aria-labelled by attributes 3. The course count info could have some access-hidden text 4. The pagination links all have title="Actions" which should be removed 5. The action-icons for courses could have role="button" 6. We could use aria-selected on the selected category/course (Note - I'm not expecting all these things addressed in integration - as long as we have issues created for them).
            Hide
            Damyon Wiese added a comment -

            Thanks Sam, this looks really great - a big improvement on the old interface.

            Just posting some review in progress comments - I'm still checking a few things so might post a few more in a bit.

            Coding style/preference things:

            1. I like "new stdClass()" not "new stdClass"
            2. functions do not require \ so all your \get_string calls can be just get_string

            Themey things:

            1. .yui3-button - the policy here is to use the bootstrap style HTML and add the supporting CSS for it to work in other themes - so this should be .button with .button added to the base theme. But IMO the pagination links should be links, not buttons (because they do not perform an action).
            2. Create new + button should not have a title attribute (currently a space). (it shows empty hover which looks weird).
            3. Menu entries in action menu need padding between icon and text (testing in standard)
            4. Menu entries in action menu have different padding to the action menu on the course page - they should be the same (testing in standard)
            5. Hover for the category adds an arrow on the right hand side - it seems wrong as the entire category row is not clickable and it doesnt seem to indicate anything. It seems to be used to indicate the selected category - but that choice of icon does not make sense to me for that purpose.

            Other:
            1. Typo: "You cannot make categroy 'Miscellaneous sub' a subcategory of one of its own sub categories"
            2. I can select a course by clicking anywhere in the row, but not a category.
            3. Select all/Select none would be nice
            4. Selecting "Show All", then moving a course to a different category changes the pagination to "Per page: 999".
            5. The drag drop does not work with the keyboard, because you have subclassed Y.DD.* directly and not used M.core.dragdrop.

            Also discussed in chat - I get JS errors when using the drag and drop.

            Show
            Damyon Wiese added a comment - Thanks Sam, this looks really great - a big improvement on the old interface. Just posting some review in progress comments - I'm still checking a few things so might post a few more in a bit. Coding style/preference things: 1. I like "new stdClass()" not "new stdClass" 2. functions do not require \ so all your \get_string calls can be just get_string Themey things: 1. .yui3-button - the policy here is to use the bootstrap style HTML and add the supporting CSS for it to work in other themes - so this should be .button with .button added to the base theme. But IMO the pagination links should be links, not buttons (because they do not perform an action). 2. Create new + button should not have a title attribute (currently a space). (it shows empty hover which looks weird). 3. Menu entries in action menu need padding between icon and text (testing in standard) 4. Menu entries in action menu have different padding to the action menu on the course page - they should be the same (testing in standard) 5. Hover for the category adds an arrow on the right hand side - it seems wrong as the entire category row is not clickable and it doesnt seem to indicate anything. It seems to be used to indicate the selected category - but that choice of icon does not make sense to me for that purpose. Other: 1. Typo: "You cannot make categroy 'Miscellaneous sub' a subcategory of one of its own sub categories" 2. I can select a course by clicking anywhere in the row, but not a category. 3. Select all/Select none would be nice 4. Selecting "Show All", then moving a course to a different category changes the pagination to "Per page: 999". 5. The drag drop does not work with the keyboard, because you have subclassed Y.DD.* directly and not used M.core.dragdrop. Also discussed in chat - I get JS errors when using the drag and drop.
            Hide
            David Monllaó added a comment -

            Hi Sam, I've only done a quick sight on the behat part after the changes and it look all good; the only thing is that, in the steps definitions code (php) we can use get_string() calls rather than hardcoded language strings (https://github.com/samhemelryk/moodle/compare/26...31830-26#diff-13db1839b0df927311869804b8c801bcR72) so we reduce maintenance costs in case there are changes in the lang strings.

            Show
            David Monllaó added a comment - Hi Sam, I've only done a quick sight on the behat part after the changes and it look all good; the only thing is that, in the steps definitions code (php) we can use get_string() calls rather than hardcoded language strings ( https://github.com/samhemelryk/moodle/compare/26...31830-26#diff-13db1839b0df927311869804b8c801bcR72 ) so we reduce maintenance costs in case there are changes in the lang strings.
            Hide
            Sam Hemelryk added a comment -

            Ok behat tests have all being fixed up and this is up for integation review once more

            Just noting that I finished fixing the tests this morning and am still in the process of running the full suite to test it.

            Show
            Sam Hemelryk added a comment - Ok behat tests have all being fixed up and this is up for integation review once more Just noting that I finished fixing the tests this morning and am still in the process of running the full suite to test it.
            Hide
            Michael de Raadt added a comment -

            Removing this particular issue from the sprint as its "sub-tasks" are complete and its epic-linked tasks can exist in the sprint independently.

            Show
            Michael de Raadt added a comment - Removing this particular issue from the sprint as its "sub-tasks" are complete and its epic-linked tasks can exist in the sprint independently.
            Hide
            Michael de Raadt added a comment -

            Carrying over to the next sprint, although it may be able to be removed.

            Show
            Michael de Raadt added a comment - Carrying over to the next sprint, although it may be able to be removed.
            Hide
            Sam Hemelryk added a comment - - edited

            Oh there was one thing from your notes I didn't do Marina, making the grey category links redirect to course/index.php probably shouldn't be done sorry, those links trigger the category to expand and I think making them link to another page would have a negative effect on usability for limited access users.

            Show
            Sam Hemelryk added a comment - - edited Oh there was one thing from your notes I didn't do Marina, making the grey category links redirect to course/index.php probably shouldn't be done sorry, those links trigger the category to expand and I think making them link to another page would have a negative effect on usability for limited access users.
            Hide
            Sam Hemelryk added a comment -

            OK new branch pushed up with the following fixes made after your latest review Marina

            • Tidied up course detail permissions so that user is not shown information they couldn't access elsewhere.
            • category link dimming now accounts for course creation as an action as well.
            • category single select when in courses view mode is now limited to courses user can action in.
            • There is now a check at the start of the management page to redirect to course/index.php if the user isn't able to manage in any category.
            • Tweaked navigation again, to give the limited users a navbar structure similar to the system cap'd user.
            • Cancelling a category delete now takes you back to the category you were viewing.
            • Fixed undefined notice
            • Improved placement of course request and approval links.
            • Fixed display of the up/down arrows for courses when paginated.
            • Fixed JS typo
            • Several styling tweaks/improvements to the base theme.
            • Several styling tweaks/improvements to the bootstrapbase theme.

            I'll tidy up the behat tests after lunch.

            Show
            Sam Hemelryk added a comment - OK new branch pushed up with the following fixes made after your latest review Marina Tidied up course detail permissions so that user is not shown information they couldn't access elsewhere. category link dimming now accounts for course creation as an action as well. category single select when in courses view mode is now limited to courses user can action in. There is now a check at the start of the management page to redirect to course/index.php if the user isn't able to manage in any category. Tweaked navigation again, to give the limited users a navbar structure similar to the system cap'd user. Cancelling a category delete now takes you back to the category you were viewing. Fixed undefined notice Improved placement of course request and approval links. Fixed display of the up/down arrows for courses when paginated. Fixed JS typo Several styling tweaks/improvements to the base theme. Several styling tweaks/improvements to the bootstrapbase theme. I'll tidy up the behat tests after lunch.
            Hide
            Sam Hemelryk added a comment -

            Awesome thanks for continuing the review Marina I'm working on your latest points now, I'm shooting off for the night but I hope to have them done before you are online tomorrow again.

            Andrew I've addressed nearly all of the points you noted in the JS. What I havn't addressed I'll list here with reasons why so that we can discuss them if you want:

            category.js
            26: The default is there because the call chain for a setter starts with the previous value. I set default only to be 100% explicit about my starting point.
            29: Getter definitely takes two args, the first the value the second the name being set (mainly for use with subattribute I believe). As per my design the use of getter here would be correct.
            35: I am pretty sure that is required (hehe horrible mentality I know but the YUI docs arn't overly descriptive and investigation seems to be leading me to the conclusion that to find out I'll have to test and I'm pushed for time presently sorry).
            68: You'll have to be more specific here sorry Andrew - I'm can't spot any potential pitfalls there but perhaps I am missing something?
            118: The type handling is because at that point it is being read from the DOM rather than the data store for the element. It will be a string and as far as values are concerned we are only concerned about '0'. When it gets set I use true, this will only ever be logged in the elements data store and '0' will not be available again through getData. There was a bug here in that to ensure CSS styling is reflective I'm now also calling setAttribute('data-expanded', '1') there.
            142: Yip - I need the attribute to be written to the DOM as it can potentially be styled upon. setData only stores the data it doesn't write back to the DOM (unless that has changed since I last looked)

            console.js

            382: I'd designed this function to work solely with complete such that any callback method is responsible for its own handling.
            388: data should be a key value string, optionally you can include the querystring-stringify-simple sub module and then provide a single level object, however as we have the functionality in javascript-static already I think the use of the additional module is a bit of a waste.

            Show
            Sam Hemelryk added a comment - Awesome thanks for continuing the review Marina I'm working on your latest points now, I'm shooting off for the night but I hope to have them done before you are online tomorrow again. Andrew I've addressed nearly all of the points you noted in the JS. What I havn't addressed I'll list here with reasons why so that we can discuss them if you want: category.js 26: The default is there because the call chain for a setter starts with the previous value. I set default only to be 100% explicit about my starting point. 29: Getter definitely takes two args, the first the value the second the name being set (mainly for use with subattribute I believe). As per my design the use of getter here would be correct. 35: I am pretty sure that is required (hehe horrible mentality I know but the YUI docs arn't overly descriptive and investigation seems to be leading me to the conclusion that to find out I'll have to test and I'm pushed for time presently sorry). 68: You'll have to be more specific here sorry Andrew - I'm can't spot any potential pitfalls there but perhaps I am missing something? 118: The type handling is because at that point it is being read from the DOM rather than the data store for the element. It will be a string and as far as values are concerned we are only concerned about '0'. When it gets set I use true, this will only ever be logged in the elements data store and '0' will not be available again through getData. There was a bug here in that to ensure CSS styling is reflective I'm now also calling setAttribute('data-expanded', '1') there. 142: Yip - I need the attribute to be written to the DOM as it can potentially be styled upon. setData only stores the data it doesn't write back to the DOM (unless that has changed since I last looked) console.js 382: I'd designed this function to work solely with complete such that any callback method is responsible for its own handling. 388: data should be a key value string, optionally you can include the querystring-stringify-simple sub module and then provide a single level object, however as we have the functionality in javascript-static already I think the use of the additional module is a bit of a waste.
            Hide
            Marina Glancy added a comment -

            Other minor comments.

            I tested as the following users:

            • admin
            • manager01: user with Manager role in one course category
            • manager02: user with 'Course creator' role on system level
            • manager03: user with 'Course creator' role in one course category

            1. It looks like you don't need anymore to call navigation_node::override_active_url() on /course/management.php and /course/editcategory.php?id=2 - now they only make breadcrumb worse.

            2. I can see checkboxes for courses when I can't do anything about them

            3. As manager I can see "Delete category" for my top category. If I click I get an exception (which is correct but I should not see the link too)

            4. If I delete category and click 'Cancel' I am returned to the top category instead of category I was editing

            5. When I move categories up and down some get highlighted with green and then it disappears on mouse over (happens often on standard and sometimes happens on clean)

            6. Maybe in case when user does not have a manage cap on system level we should not display dropdown under "Create new"? (creating new category)

            7. Notice: Undefined variable: id in /home/marina/repositories/int_master/integration/course/management.php on line 405

            8. see screenshot in Clean theme (note also that arrow down is missing for the last course on page even though it is not the last course in category. Same with up arrow for first course on 2nd page):

            Show
            Marina Glancy added a comment - Other minor comments. I tested as the following users: admin manager01: user with Manager role in one course category manager02: user with 'Course creator' role on system level manager03: user with 'Course creator' role in one course category 1. It looks like you don't need anymore to call navigation_node::override_active_url() on /course/management.php and /course/editcategory.php?id=2 - now they only make breadcrumb worse. 2. I can see checkboxes for courses when I can't do anything about them 3. As manager I can see "Delete category" for my top category. If I click I get an exception (which is correct but I should not see the link too) 4. If I delete category and click 'Cancel' I am returned to the top category instead of category I was editing 5. When I move categories up and down some get highlighted with green and then it disappears on mouse over (happens often on standard and sometimes happens on clean) 6. Maybe in case when user does not have a manage cap on system level we should not display dropdown under "Create new"? (creating new category) 7. Notice: Undefined variable: id in /home/marina/repositories/int_master/integration/course/management.php on line 405 8. see screenshot in Clean theme (note also that arrow down is missing for the last course on page even though it is not the last course in category. Same with up arrow for first course on 2nd page):
            Hide
            Marina Glancy added a comment - - edited

            awesome Sam!

            I was reviewing it as different users with different permissions on different levels. I think we need to make some policy about when to display the management page.
            For example, I can type /course/management.php as a student and navigate through categories and courses. This obviously does not display me more than if I typed /course/index.php but still I don't think we should allow it. At the moment the user gets redirected away from management page if he can not do manage it or create courses in it. Which I think is a good idea. If for some strange reason admin gives a user permission to assign/review roles or filters but does not give managecategory/createcourse capabilities, the links will be displayed in navigation for this category and no management page is needed.

            I liked how you made grey the links to the unmanageable categories but I would also recommend to point them to /course/index.php?categoryid=xx instead of management.php.
            Also you need to check cap to managecategory/createcourse (not only manage) to decide which link to display. BTW there is a function can_edit_in_category()

            Also we should display only manage-able categories in the dropdown in "Viewing courses" mode.

            What do you think?

            P.S. the permission check in course info is a separate topic

            Show
            Marina Glancy added a comment - - edited awesome Sam! I was reviewing it as different users with different permissions on different levels. I think we need to make some policy about when to display the management page. For example, I can type /course/management.php as a student and navigate through categories and courses. This obviously does not display me more than if I typed /course/index.php but still I don't think we should allow it. At the moment the user gets redirected away from management page if he can not do manage it or create courses in it. Which I think is a good idea. If for some strange reason admin gives a user permission to assign/review roles or filters but does not give managecategory/createcourse capabilities, the links will be displayed in navigation for this category and no management page is needed. I liked how you made grey the links to the unmanageable categories but I would also recommend to point them to /course/index.php?categoryid=xx instead of management.php. Also you need to check cap to managecategory/createcourse (not only manage) to decide which link to display. BTW there is a function can_edit_in_category() Also we should display only manage-able categories in the dropdown in "Viewing courses" mode. What do you think? P.S. the permission check in course info is a separate topic
            Hide
            Sam Hemelryk added a comment -

            Ok I've a bit of time to work on this again. I've pushed up a couple more commits to address Marina's testing comments:

            1. Fixed the highlighing of the default category when no categories have been selected.
            2. Fixed the bug preventing the action menu for AJAX loaded categories from functioning.
            3. Repositioned course idnumber infront of course actions.
            4. Fixed display of moveup on first item and movedown on last item (courses and categories)
            5. I couldn't reproduce this visibility bug - perhaps it has been fixed in mean time (I tested several times on both themes)
            6. Fixed up resort selected categories string.
            7. I couldn't reproduce this sub cat creation bug. It is now covered by a behat test so perhaps already fixed elsewhere.
            8. Fixes made for a limited user include:
              • Navigation/settings now show best available option.
              • Resorting categories works now for limited access user.
              • Display of up and down arrows corrected.
              • Checkboxes no longer displayed if an action can't be performed.
            Show
            Sam Hemelryk added a comment - Ok I've a bit of time to work on this again. I've pushed up a couple more commits to address Marina's testing comments: Fixed the highlighing of the default category when no categories have been selected. Fixed the bug preventing the action menu for AJAX loaded categories from functioning. Repositioned course idnumber infront of course actions. Fixed display of moveup on first item and movedown on last item (courses and categories) I couldn't reproduce this visibility bug - perhaps it has been fixed in mean time (I tested several times on both themes) Fixed up resort selected categories string. I couldn't reproduce this sub cat creation bug. It is now covered by a behat test so perhaps already fixed elsewhere. Fixes made for a limited user include: Navigation/settings now show best available option. Resorting categories works now for limited access user. Display of up and down arrows corrected. Checkboxes no longer displayed if an action can't be performed.
            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
            Marina Glancy added a comment -

            Reopening upon Sam's request.

            Almost there, will be integrated next week!

            Show
            Marina Glancy added a comment - Reopening upon Sam's request. Almost there, will be integrated next week!
            Hide
            Andrew Nicols added a comment -

            Also just noticed that the @namespace is also incorrect - this is M.course.console, not M.core_course.management.Console.
            Both @namespace, and @class are therefore wrong in all js files.

            Show
            Andrew Nicols added a comment - Also just noticed that the @namespace is also incorrect - this is M.course.console, not M.core_course.management.Console. Both @namespace, and @class are therefore wrong in all js files.
            Hide
            Andrew Nicols added a comment -

            Some peer review on the JS for you as discussed this morning.

            There are a couple of items which are found on several files - I won't
            repeat these through because I'm too lazy

            category.js:

            • 18 - categoryid: The correct @type is Number - JS doesn't have floats and ints, only numbers. Other locations affected too.
            • 18 - categoryid: Can you specify a @default too?
            • 20 - categoryid: Why is the default an object?
            • 20 - categoryid: Should be readOnly
            • 26/39 - selected: This default doesn't make sense when the setter() forces any null value to a false
            • 29 - selected: Should this not be the setter rather than the getter? AIR, a getter only takes one arg and to me this looks more like a setter than a getter.
            • 35 - selected: As I recall, you shouldn't need to use this.set() to override the value in the setter
            • 43: typo and missing trailing punctuation
            • 46: Default should be Array
            • 39-56: Very confused by this setter... surely you should be either
              convert the value to an array if it's not one (preferable), or rejecting
              the value if not (less preferable). it seems wrong that when you call set
              it appends to an existin array.
            • 67: You could do this in the getter instead? The first time that it's
            • called it can get the data. This was you can set lazyAdd: true on the
            • attribute. Alternatively it should set writeOnce: true (with docs).
            • 68: itemname: Again, this should probably be set once
            • 77: getName: How does this handle multilang strings?
            • 85: register_course: Naming pattern differs (underscore vs camelCase)
            • 97-125: e.halt() - should this not be e.preventDefault to allow other
              handlers to hook in? This will prevent third-party events listening too
            • 118/9: In the first step, you're strictly comparing expanded to a string '0', and
              then setting it to a boolean. Is there a reason you switch types?
            • 129: Can you make the Y.log source the module name instead to make life
              easier when MDL-41985 lands (other instance of Y.log too)
            • 142: You're calling setAttribute() for a data- value?
            • 143: You can chain this off 142
            • Same for the next function
            • 186: You're only returning if the outcome was false - wat if it wasn't?
            • 174: Should be @return (not @returns) - other instances likely affected
            • too
            • 320: Should return this and be marked as @chainable in docblock
            • 346: Should return this and be marked as @chainable in docblock

            console.js:

            • 30: typeof isn't a function, it's an operator:
              if (typeof node === 'string')
              
            • 42,49, etc - these should probably default to null (and specify a
              @default). If you test this.get('categorylisting'), it will return a
              truthy value - try this in your JS console:
              if ({}) { console.log("Truthful"); } else { console.log("falsey"); }
              
            • attributes: Can these be camelCase please?
            • 175: Should we set the default here rather than on line 171?
            • 203: A loglevel of 'note' isn't valid. Although you can use custom
              logLevels with YUI, I wouldn't recommend it as we then can't filter progressively
              on them.
            • 331: Does it make sense for get_category_by_id to return the id it was
              passed? Shoudl we not return null?
            • 348: Shoudl return null instead of false (and docblock is wrong either way)
            • 324/341: Can we break out the assignations onto separate lines to make blame easier.
              Shifter will minify them anyway.
            • 325: missing
              if (!courses.hadOwnPropery(i)) {
                  continue;
              }
              

              (though it may not be required depending on the nature of courses)

            • 359: length is used without being defined
            • 382: Needs error handling for on: failure to open a new M.core.exception
            • 388: IIRC, data shoudl take an object not a built queryString
            • 396-401: Please don't docblock the parent namespace - it makes the doc generation a
              little fruity.
            • 408-410: I'd love to move this to Y.Moodle.course if that's appropriate? It's possibly
              too much work given remaining time though.

            course.js

            • 245: Should really be chainable and return this;

            There may be more, but that's probably enough to start you off

            Show
            Andrew Nicols added a comment - Some peer review on the JS for you as discussed this morning. There are a couple of items which are found on several files - I won't repeat these through because I'm too lazy category.js: 18 - categoryid: The correct @type is Number - JS doesn't have floats and ints, only numbers. Other locations affected too. 18 - categoryid: Can you specify a @default too? 20 - categoryid: Why is the default an object? 20 - categoryid: Should be readOnly 26/39 - selected: This default doesn't make sense when the setter() forces any null value to a false 29 - selected: Should this not be the setter rather than the getter? AIR, a getter only takes one arg and to me this looks more like a setter than a getter. 35 - selected: As I recall, you shouldn't need to use this.set() to override the value in the setter 43: typo and missing trailing punctuation 46: Default should be Array 39-56: Very confused by this setter... surely you should be either convert the value to an array if it's not one (preferable), or rejecting the value if not (less preferable). it seems wrong that when you call set it appends to an existin array. 67: You could do this in the getter instead? The first time that it's called it can get the data. This was you can set lazyAdd: true on the attribute. Alternatively it should set writeOnce: true (with docs). 68: itemname: Again, this should probably be set once 77: getName: How does this handle multilang strings? 85: register_course: Naming pattern differs (underscore vs camelCase) 97-125: e.halt() - should this not be e.preventDefault to allow other handlers to hook in? This will prevent third-party events listening too 118/9: In the first step, you're strictly comparing expanded to a string '0', and then setting it to a boolean. Is there a reason you switch types? 129: Can you make the Y.log source the module name instead to make life easier when MDL-41985 lands (other instance of Y.log too) 142: You're calling setAttribute() for a data- value? 143: You can chain this off 142 Same for the next function 186: You're only returning if the outcome was false - wat if it wasn't? 174: Should be @return (not @returns) - other instances likely affected too 320: Should return this and be marked as @chainable in docblock 346: Should return this and be marked as @chainable in docblock console.js: 30: typeof isn't a function, it's an operator: if (typeof node === 'string') 42,49, etc - these should probably default to null (and specify a @default). If you test this.get('categorylisting'), it will return a truthy value - try this in your JS console: if ({}) { console.log( "Truthful" ); } else { console.log( "falsey" ); } attributes: Can these be camelCase please? 175: Should we set the default here rather than on line 171? 203: A loglevel of 'note' isn't valid. Although you can use custom logLevels with YUI, I wouldn't recommend it as we then can't filter progressively on them. 331: Does it make sense for get_category_by_id to return the id it was passed? Shoudl we not return null? 348: Shoudl return null instead of false (and docblock is wrong either way) 324/341: Can we break out the assignations onto separate lines to make blame easier. Shifter will minify them anyway. 325: missing if (!courses.hadOwnPropery(i)) { continue ; } (though it may not be required depending on the nature of courses) 359: length is used without being defined 382: Needs error handling for on: failure to open a new M.core.exception 388: IIRC, data shoudl take an object not a built queryString 396-401: Please don't docblock the parent namespace - it makes the doc generation a little fruity. 408-410: I'd love to move this to Y.Moodle.course if that's appropriate? It's possibly too much work given remaining time though. course.js 245: Should really be chainable and return this; There may be more, but that's probably enough to start you off
            Hide
            Sam Hemelryk added a comment -

            Awesome thanks for the comments so far guys.

            Marina I've made two commits to address the code review comments and naming consistency issues.
            Details:
            Changes made from code review comments

            1. I can rebase this for you/the integrator before it goes in for sure no probs
            2. Fixed double phpdoc block of course_change_visibility
            3. Moved permission checks out of course_move_after_course and into helper function.
            4. Reviewed setType calls for editcategory_form.php. Noting I left idnumber as that is what it was previously and matches with that of course.
            5. Reviewed all uses of can_resort and added more specific methods. Fixed method mentioned in exception.
            6. Converted calls to fetch courses to call get_course. Noting I left the expectation of course_in_list.
            7. Exceptions now thrown when trying to move courses and problems arise.
            8. Fixed unnecessary namespace hinting in core_course_management_renderer.
            9. Abstracted common logic of can_resort_any and can_change_parent_any. Removed check for system level capability from has_manage_capability_on_any.
            10. Reviewed debugging calls I've introduced.

            Renamed several functions:

            1. course_move_by_one => course_change_sortorder_by_one
            2. course_move_after_course => course_move_after_course
            3. helper::action_course_move_after_course => helper::action_course_change_sortorder_after_course
            4. helper::action_course_moveup => helper::action_course_change_sortorder_up_one
            5. helper::action_course_movedown => helper::action_course_change_sortorder_down_one
            6. helper::action_course_moveup_by_record => helper::action_course_change_sortorder_up_one_by_record
            7. helper::action_course_movedown_by_record => helper::action_course_change_sortorder_down_one_by_record
            8. helper::action_category_movedown => helper::action_category_change_sortorder_down_one
            9. helper::action_category_movedown_by_id => helper::action_category_change_sortorder_down_one_by_id
            10. helper::action_category_moveup => helper::action_category_change_sortorder_up_one
            11. helper::action_category_moveup_by_id => helper::action_category_change_sortorder_up_one_by_id
            12. coursecat::move_by_one => coursecat::change_sortorder_by_one

            I'm going to work on the integration queue for the next while and my next task will be to address Marina's testing comments and Davids behat suggestions.

            Many thanks
            Sam

            Show
            Sam Hemelryk added a comment - Awesome thanks for the comments so far guys. Marina I've made two commits to address the code review comments and naming consistency issues. Details: Changes made from code review comments I can rebase this for you/the integrator before it goes in for sure no probs Fixed double phpdoc block of course_change_visibility Moved permission checks out of course_move_after_course and into helper function. Reviewed setType calls for editcategory_form.php. Noting I left idnumber as that is what it was previously and matches with that of course. Reviewed all uses of can_resort and added more specific methods. Fixed method mentioned in exception. Converted calls to fetch courses to call get_course. Noting I left the expectation of course_in_list. Exceptions now thrown when trying to move courses and problems arise. Fixed unnecessary namespace hinting in core_course_management_renderer. Abstracted common logic of can_resort_any and can_change_parent_any. Removed check for system level capability from has_manage_capability_on_any. Reviewed debugging calls I've introduced. Renamed several functions: course_move_by_one => course_change_sortorder_by_one course_move_after_course => course_move_after_course helper::action_course_move_after_course => helper::action_course_change_sortorder_after_course helper::action_course_moveup => helper::action_course_change_sortorder_up_one helper::action_course_movedown => helper::action_course_change_sortorder_down_one helper::action_course_moveup_by_record => helper::action_course_change_sortorder_up_one_by_record helper::action_course_movedown_by_record => helper::action_course_change_sortorder_down_one_by_record helper::action_category_movedown => helper::action_category_change_sortorder_down_one helper::action_category_movedown_by_id => helper::action_category_change_sortorder_down_one_by_id helper::action_category_moveup => helper::action_category_change_sortorder_up_one helper::action_category_moveup_by_id => helper::action_category_change_sortorder_up_one_by_id coursecat::move_by_one => coursecat::change_sortorder_by_one I'm going to work on the integration queue for the next while and my next task will be to address Marina's testing comments and Davids behat suggestions. Many thanks Sam
            Hide
            David Monllaó added a comment -

            Hi Sam,

            Good to see the amount of good functional tests for this adding a few comments about them:

            1. All new step definitions must include a description, they are listed in admin/tool/behat/index.php using Moodle's web interface to provide info for people writing tests; related with the next point
            2. The arguments step definitions expects are supposed to be restricted to a few types as described in http://docs.moodle.org/dev/Acceptance_testing#Check_list, this means that sometimes we will need to add two step definitions for one functionality (https://github.com/samhemelryk/moodle/compare/26...31830-26#diff-13db1839b0df927311869804b8c801bcR999) but on the other hand we provide clear documentation for test writers; the reason is that we parse the regex to transform it into a more human-friendly view, replacing them for IDNUMBER_STRING for example, so test writers (following the guidelines in the MDocs page) knows what to substitute with what A (?P<idnumber>[^"]) would be ok, but maybe *(?P<listing>category|course) would not match the regex regex (https://github.com/moodlehq/moodle-behat-extension/blob/master/src/Moodle/BehatExtension/HelpPrinter/MoodleDefinitionsPrinter.php#L73) and the output in admin/tool/behat/index.php would not provide any info for a non-dev user about what to write in there or even if it is part of the sentence or an expected argument, another option would be to parse also the (option1|option2) and display a select box with the options
            3. https://github.com/samhemelryk/moodle/compare/26...31830-26#diff-13db1839b0df927311869804b8c801bcR1096 when is possible (don't know if you can do it here) is always better to specify the tag name, is faster than to match nodes against a *
            4. Also about xpaths, to find out if there is a node that contains the specified class I usually use contains(concat(' ', normalize-space(@class), ' '), ' CLASSNAME '), respecting the spaces before and after CLASSNAME
            5. In behat_course::i_create_a_course_with() is mixing performing actions with checking the outcomes, steps definitions scope should be restricted to one or the another as steps definitions are just sentences to be used by .feature files, the feature is what is supposed to check for outcomes. In this case would be better to remove the should I... and if there are outcomes to check, check them at the Then section of the feature description, also probably the feature will fail if I should see the "Course categories" management page or I should see the "Course categories and courses" management page would be failing
            6. Could be good to add custom exception messages to methods like behat_course::get_category_id(), if the test writer provides a wrong idnumber in i_click_to_toggle_subcategories_expansion() the exception message will be something db related which depending on the case will not be enough explicative for someone that is not necessarily a developer.
            Show
            David Monllaó added a comment - Hi Sam, Good to see the amount of good functional tests for this adding a few comments about them: All new step definitions must include a description, they are listed in admin/tool/behat/index.php using Moodle's web interface to provide info for people writing tests; related with the next point The arguments step definitions expects are supposed to be restricted to a few types as described in http://docs.moodle.org/dev/Acceptance_testing#Check_list , this means that sometimes we will need to add two step definitions for one functionality ( https://github.com/samhemelryk/moodle/compare/26...31830-26#diff-13db1839b0df927311869804b8c801bcR999 ) but on the other hand we provide clear documentation for test writers; the reason is that we parse the regex to transform it into a more human-friendly view, replacing them for IDNUMBER_STRING for example, so test writers (following the guidelines in the MDocs page) knows what to substitute with what A (?P<idnumber> [^"] ) would be ok, but maybe *(?P<listing>category|course) would not match the regex regex ( https://github.com/moodlehq/moodle-behat-extension/blob/master/src/Moodle/BehatExtension/HelpPrinter/MoodleDefinitionsPrinter.php#L73 ) and the output in admin/tool/behat/index.php would not provide any info for a non-dev user about what to write in there or even if it is part of the sentence or an expected argument, another option would be to parse also the (option1|option2) and display a select box with the options https://github.com/samhemelryk/moodle/compare/26...31830-26#diff-13db1839b0df927311869804b8c801bcR1096 when is possible (don't know if you can do it here) is always better to specify the tag name, is faster than to match nodes against a * Also about xpaths, to find out if there is a node that contains the specified class I usually use contains(concat(' ', normalize-space(@class), ' '), ' CLASSNAME ') , respecting the spaces before and after CLASSNAME In behat_course::i_create_a_course_with() is mixing performing actions with checking the outcomes, steps definitions scope should be restricted to one or the another as steps definitions are just sentences to be used by .feature files, the feature is what is supposed to check for outcomes. In this case would be better to remove the should I... and if there are outcomes to check, check them at the Then section of the feature description, also probably the feature will fail if I should see the "Course categories" management page or I should see the "Course categories and courses" management page would be failing Could be good to add custom exception messages to methods like behat_course::get_category_id() , if the test writer provides a wrong idnumber in i_click_to_toggle_subcategories_expansion() the exception message will be something db related which depending on the case will not be enough explicative for someone that is not necessarily a developer.
            Hide
            Marina Glancy added a comment -

            This is the list of all new functions dealing with resorting of courses and categories. I would really like to have the word 'sortorder' in the names rather than 'move'. Especially since there are already functions that are named 'move' and they refer to changing the parent category (either for courses or for categories)

            /course/lib.php
            course_move_by_one() -> course_change_sortorder_by_one()
            course_move_after_course() -> course_change_sortorder_after()

            /lib/coursecatlib.php (methods of class coursecat)
            can_resort()
            can_change_sortorder()
            resort_subcategories()
            resort_courses()
            move_by_one() -> change_sortorder_by_one()

            /course/classes/management/helper.php (methods of class helper) - naming is up to you
            action_category_moveup()
            action_category_movedown()
            action_category_moveup_by_id()
            action_category_movedown_by_id()
            action_category_resort_subcategories()
            action_category_resort_courses()
            action_course_moveup()
            action_course_movedown()
            action_course_moveup_by_record()
            action_course_movedown_by_record()

            Show
            Marina Glancy added a comment - This is the list of all new functions dealing with resorting of courses and categories. I would really like to have the word 'sortorder' in the names rather than 'move'. Especially since there are already functions that are named 'move' and they refer to changing the parent category (either for courses or for categories) /course/lib.php course_move_by_one() -> course_change_sortorder_by_one() course_move_after_course() -> course_change_sortorder_after() /lib/coursecatlib.php (methods of class coursecat) can_resort() can_change_sortorder() resort_subcategories() resort_courses() move_by_one() -> change_sortorder_by_one() /course/classes/management/helper.php (methods of class helper) - naming is up to you action_category_moveup() action_category_movedown() action_category_moveup_by_id() action_category_movedown_by_id() action_category_resort_subcategories() action_category_resort_courses() action_course_moveup() action_course_movedown() action_course_moveup_by_record() action_course_movedown_by_record()
            Hide
            Marina Glancy added a comment -

            This screenshot by user who has management capability only in cat1:

            Show
            Marina Glancy added a comment - This screenshot by user who has management capability only in cat1:
            Hide
            Marina Glancy added a comment - - edited

            Hi Sam. Wow, this is landing!

            It works like magic and looks awesome! But it's a lot of code so there are some things that I've spotted. Some are big some are not.

            Code review comments:

            1. there is an easy merging conflict with integration
            2. function course_change_visibility() has extra phpdocs block
            3. In course_move_after_course() you check capability to resort, in course_move_by_one() you don't. And usually there are no capabilities check in API functions in /course/lib.php. But in any case if you do it should be an exception and not the developer debugging
            4. course/classes/editcategory_form.php pls review setType() methods. PARAM_RAW for idnumber is not good and setType for 'editor' field is not needed.
            5. It looks like there is a little confusion in functions coursecat::can_resort() and coursecat::can_change_sortorder():
              • in helper::get_category_listitem_actions() you check can_resort() and should be can_change_sortorder()
              • helper::action_category_moveup and action_category_movedown there are wrong exceptions
            6. What's the point of converting to course_in_list in functions action_course_moveup_by_record and action_course_movedown_by_record ? The only reason would be for context preloading but you don't retrieve context fields in DB query (and it's not needed anyway). Also they could be using get_course() function for easier readability. At the same time action_course_show_by_record and action_course_hide_by_record should be retrieving contexts
            7. helper::move_courses_into_category() imho should be either no messages or exceptions but not debugging.
            8. class core_course_management_renderer indicating \ namespace is not necessary
            9. functions coursecat::can_resort_any() and cat_change_parent_any() are [almost] identical. The almost-ness is especially confusing because they both set the same value in cache for both. Is there a good reason for that? Also, what happens if user is granted manage capability on system level but is revoked it on one of categories levels?
            10. Overall too many debugging() statements - most of them should be either removed or converted to exceptions

            Testing comments:

            1. on /course/management.php page in standard theme there is a right arrow for the first category when category is not yet selected (no arrow in clean theme)
            2. The editing icons are unavailable for categories loaded with AJAX (see attachment 'coursemanagement1.png')
            3. Course idnumber appears in strange place (see attachment 'coursemanagement1.png')
            4. There are displayed an up arrow for first subcategory and a down arrow for the last one
            5. Bug in changing category visibility:
              • Open a category with existing subcategory
              • Make current category invisible
              • Refresh the page
              • Make current category visible
              • Subcategory appears as invisible but if you refresh it is visible (as it should be)
            6. string "Re-sort sub categories of selected categories by" should not have "by" in the end
            7. After I create subcategory it does not appear in the list until I purge the caches
            8. I gave a user Manager role in one category
              • There is no active item in 'Settings' block
              • I see the link to resort subcategories but get an error when I try to do so
              • I can see up/down arrows for my category, when I click category moves but when I refresh it jumps back (probably this is the confused function name that I've spotted in code review)
              • I can see the checkboxes next to unmanageable categories and dropdowns to resort them (get an error when I try to do it)

            Show
            Marina Glancy added a comment - - edited Hi Sam. Wow, this is landing! It works like magic and looks awesome! But it's a lot of code so there are some things that I've spotted. Some are big some are not. Code review comments: there is an easy merging conflict with integration function course_change_visibility() has extra phpdocs block In course_move_after_course() you check capability to resort, in course_move_by_one() you don't. And usually there are no capabilities check in API functions in /course/lib.php. But in any case if you do it should be an exception and not the developer debugging course/classes/editcategory_form.php pls review setType() methods. PARAM_RAW for idnumber is not good and setType for 'editor' field is not needed. It looks like there is a little confusion in functions coursecat::can_resort() and coursecat::can_change_sortorder(): in helper::get_category_listitem_actions() you check can_resort() and should be can_change_sortorder() helper::action_category_moveup and action_category_movedown there are wrong exceptions What's the point of converting to course_in_list in functions action_course_moveup_by_record and action_course_movedown_by_record ? The only reason would be for context preloading but you don't retrieve context fields in DB query (and it's not needed anyway). Also they could be using get_course() function for easier readability. At the same time action_course_show_by_record and action_course_hide_by_record should be retrieving contexts helper::move_courses_into_category() imho should be either no messages or exceptions but not debugging. class core_course_management_renderer indicating \ namespace is not necessary functions coursecat::can_resort_any() and cat_change_parent_any() are [almost] identical. The almost-ness is especially confusing because they both set the same value in cache for both. Is there a good reason for that? Also, what happens if user is granted manage capability on system level but is revoked it on one of categories levels? Overall too many debugging() statements - most of them should be either removed or converted to exceptions Testing comments: on /course/management.php page in standard theme there is a right arrow for the first category when category is not yet selected (no arrow in clean theme) The editing icons are unavailable for categories loaded with AJAX (see attachment 'coursemanagement1.png') Course idnumber appears in strange place (see attachment 'coursemanagement1.png') There are displayed an up arrow for first subcategory and a down arrow for the last one Bug in changing category visibility: Open a category with existing subcategory Make current category invisible Refresh the page Make current category visible Subcategory appears as invisible but if you refresh it is visible (as it should be) string "Re-sort sub categories of selected categories by" should not have "by" in the end After I create subcategory it does not appear in the list until I purge the caches I gave a user Manager role in one category There is no active item in 'Settings' block I see the link to resort subcategories but get an error when I try to do so I can see up/down arrows for my category, when I click category moves but when I refresh it jumps back (probably this is the confused function name that I've spotted in code review) I can see the checkboxes next to unmanageable categories and dropdowns to resort them (get an error when I try to do it)
            Hide
            Sam Hemelryk added a comment -

            Ok, I'm going to try my luck here and I'm putting this up for integration review.

            I've tidied up all points I can see noted by Andrew and Marina, I've also added a pile more unit tests and behat tests to cover a good chunk of the interface I've created here.
            Over the weekend I tested on a site with 7500 total categories, of which 500 were base categories. While slow (round 20s) the interface didn't break and we didn't need to raise memory or time for any operation.

            Integrator, this is obviously a pretty big change please feel free to ping me about any matters here.

            Show
            Sam Hemelryk added a comment - Ok, I'm going to try my luck here and I'm putting this up for integration review. I've tidied up all points I can see noted by Andrew and Marina, I've also added a pile more unit tests and behat tests to cover a good chunk of the interface I've created here. Over the weekend I tested on a site with 7500 total categories, of which 500 were base categories. While slow (round 20s) the interface didn't break and we didn't need to raise memory or time for any operation. Integrator, this is obviously a pretty big change please feel free to ping me about any matters here.
            Hide
            Andrew Davis added a comment -

            Taking this out of peer review mode but naturally anyone/everyone can chime in with anything untoward that they find.

            Show
            Andrew Davis added a comment - Taking this out of peer review mode but naturally anyone/everyone can chime in with anything untoward that they find.
            Hide
            Andrew Davis added a comment - - edited

            I still think you should get rid of "Bulk actions" in the UI but that's not a deal breaker.

            One oddity. I select to view courses then pick a course. That gets me a listing of the courses in the current category on the left and the course is displayed on the right. If I choose to hide the course when the page reloads I'm shifted to the 3 block page (all categories, courses in the current category, current course).

            I can't help but notice that thunderbirds_are_go() is still present.

            Is it worth considering this sort of trickery http://stackoverflow.com/questions/6543351/how-to-update-a-column-for-all-rows-in-a-table-with-different-values-for-each-ro in resort_courses(). In particular for this

            +            foreach ($courses as $course) {
            +                $DB->set_field('course', 'sortorder', $this->sortorder + $i, array('id' => $course->id));
            +                $i++;
            +            }
            

            I'm not sure if we can do anything in a DB independent way although case statements in an update may actually work. It would just be nice to avoid hitting the DB in a loop.

            Marina, do you have anything else to add?

            Show
            Andrew Davis added a comment - - edited I still think you should get rid of "Bulk actions" in the UI but that's not a deal breaker. One oddity. I select to view courses then pick a course. That gets me a listing of the courses in the current category on the left and the course is displayed on the right. If I choose to hide the course when the page reloads I'm shifted to the 3 block page (all categories, courses in the current category, current course). I can't help but notice that thunderbirds_are_go() is still present. Is it worth considering this sort of trickery http://stackoverflow.com/questions/6543351/how-to-update-a-column-for-all-rows-in-a-table-with-different-values-for-each-ro in resort_courses(). In particular for this + foreach ($courses as $course) { + $DB->set_field('course', 'sortorder', $ this ->sortorder + $i, array('id' => $course->id)); + $i++; + } I'm not sure if we can do anything in a DB independent way although case statements in an update may actually work. It would just be nice to avoid hitting the DB in a loop. Marina, do you have anything else to add?
            Hide
            Sam Hemelryk added a comment -

            Updated with a couple more fixes now.

            Show
            Sam Hemelryk added a comment - Updated with a couple more fixes now.
            Hide
            Sam Hemelryk added a comment -

            Fixed now thanks Andrew.

            Show
            Sam Hemelryk added a comment - Fixed now thanks Andrew.
            Hide
            Andrew Davis added a comment -

            Still getting the above error

            Show
            Andrew Davis added a comment - Still getting the above error
            Hide
            Andrew Davis added a comment -

            Getting the following when re-sorting categories.

            Can not find data record in database table course_categories.

            More information about this error
            Debug info: SELECT id,parent FROM

            Unknown macro: {course_categories}

            WHERE id = ?
            [array (
            0 => 0,
            )]
            Error code: invalidrecord
            Stack trace:

            line 1385 of /lib/dml/moodle_database.php: dml_missing_record_exception thrown
            line 1361 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select()
            line 6533 of /lib/accesslib.php: call to moodle_database->get_record()
            line 2123 of /lib/coursecatlib.php: call to context_coursecat::instance()
            line 2132 of /lib/coursecatlib.php: call to coursecat->get_context()
            line 2158 of /lib/coursecatlib.php: call to coursecat->has_manage_capability()
            line 543 of /course/classes/management/helper.php: call to coursecat->can_resort()
            line 131 of /course/management.php: call to core_course\management\helper::action_category_resort_subcategories()

            Show
            Andrew Davis added a comment - Getting the following when re-sorting categories. Can not find data record in database table course_categories. More information about this error Debug info: SELECT id,parent FROM Unknown macro: {course_categories} WHERE id = ? [array ( 0 => 0, )] Error code: invalidrecord Stack trace: line 1385 of /lib/dml/moodle_database.php: dml_missing_record_exception thrown line 1361 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select() line 6533 of /lib/accesslib.php: call to moodle_database->get_record() line 2123 of /lib/coursecatlib.php: call to context_coursecat::instance() line 2132 of /lib/coursecatlib.php: call to coursecat->get_context() line 2158 of /lib/coursecatlib.php: call to coursecat->has_manage_capability() line 543 of /course/classes/management/helper.php: call to coursecat->can_resort() line 131 of /course/management.php: call to core_course\management\helper::action_category_resort_subcategories()
            Hide
            Andrew Davis added a comment -

            Tried to move a course between categories and got this. I'm not sure how as I haven't been able to get the error to occur again.

            A required parameter (categoryid) was missing

            More information about this error
            Debug info:
            Error code: missingparam
            Stack trace:

            line 476 of /lib/setuplib.php: moodle_exception thrown
            line 539 of /lib/moodlelib.php: call to print_error()
            line 229 of /course/management.php: call to required_param()

            We can probably remove "Bulk actions" from the UI. The "Move selected courses to..." type labels are fairly explanatory.

            The count of courses in each category is a little odd if you have subcategories. A subcategory can have many courses in it but when the parent category is collapsed it says there are 0 courses in there. How hard would it be to have the parent category show the total number of courses within it when it is collapsed then only the number of courses directly within it when its expanded?

            Show
            Andrew Davis added a comment - Tried to move a course between categories and got this. I'm not sure how as I haven't been able to get the error to occur again. A required parameter (categoryid) was missing More information about this error Debug info: Error code: missingparam Stack trace: line 476 of /lib/setuplib.php: moodle_exception thrown line 539 of /lib/moodlelib.php: call to print_error() line 229 of /course/management.php: call to required_param() We can probably remove "Bulk actions" from the UI. The "Move selected courses to..." type labels are fairly explanatory. The count of courses in each category is a little odd if you have subcategories. A subcategory can have many courses in it but when the parent category is collapsed it says there are 0 courses in there. How hard would it be to have the parent category show the total number of courses within it when it is collapsed then only the number of courses directly within it when its expanded?
            Hide
            Andrew Davis added a comment -

            Not sure if its really an issue or not but in Standard there is no gap between the category list box and the category detail box.

            If you select a course, click delete then click cancel it forgets what course you had selected.

            Show
            Andrew Davis added a comment - Not sure if its really an issue or not but in Standard there is no gap between the category list box and the category detail box. If you select a course, click delete then click cancel it forgets what course you had selected.
            Hide
            Sam Hemelryk added a comment -

            (but yes I was waiting for someone to pick on my naming of that function)

            Show
            Sam Hemelryk added a comment - (but yes I was waiting for someone to pick on my naming of that function)
            Hide
            Sam Hemelryk added a comment -

            Mr T pities the developer who isn't all knowing of puppet pop culture

            Show
            Sam Hemelryk added a comment - Mr T pities the developer who isn't all knowing of puppet pop culture
            Hide
            Andrew Davis added a comment -

            While I find it amusing I'm not sure its a great idea to call the renderer initialization function thunderbirds_are_go(). Im not saying that it has to change just that maybe it should.

            Show
            Andrew Davis added a comment - While I find it amusing I'm not sure its a great idea to call the renderer initialization function thunderbirds_are_go(). Im not saying that it has to change just that maybe it should.
            Hide
            Sam Hemelryk added a comment -

            Hi Marina,

            In response to the points you've raised so far:

            Fixes made:

            • Fixed regression in coursecat::get_tree and added unit test to cover it.
            • Fixed purging when resorting categories.
            • Fixed coursecat::resort_courses to make DB query with preload context. Also has unit test now.
            • Removed additional test in coursecatlib_test.php search_courses.
            • ajax/management.php now calls header() footer()
            • fixed up use of course list name where appropriate. Sometimes (course detail for instance) I still needed fullname and shortname separate. However list display is now corrected.
            • Fixed new course link.
            • fixed inclusion of /course/moodleform_mod.php
            • idnumbertake => categoryidnumbertaken
            • Moved logic from management helper to appropriate places: perform_* are no more, now on coursecat or course/lib.php. I've also converted to use update_course where applicable.
            • core_course_management_renderer now extends plugin_renderer_base instead of core_course_renderer
            • The inverse delete course hash check has been fixed.

            Regarding points you raised that I have an opinion on (who'd have guessed):

            • coursecat::can_create_top_level_category I've left in there for the time being. After thinking about it I like the distinction it creates for top level categories and should be ever create additional checks or limits for creating to level categories such a method will make it a little easier to apply. A sort of future proofing. That being said I'm not in love with it so should it be felt that its best removed I'd happily do so.
            • Regarding preloading of contexts in course_in_list it appears it always preloads during construction. Is it necessary to have it double check contexts have been preloaded?
            • The files with 2002 copyrights comes from the file creation date is all. Rather than using the 1999 standard I bumped it to that.
            • The changes in outputcomponents.php we made as a separate issue and were integrated this week. I've now rebased and this patch no longer makes any changes to that file.
            • classes/editcategory_form.php must include moodleform_mod.php
            • autoloading renderer. Haha I thought that may raise a few questions. It's not being done before but to me that solves a vital issue, where to put secondary renderers for a component/plugin. I've converted it to extend plugin_renderer_base now as it doesn't depend upon core_course_renderer at all.

            Left to do (still on my list and a bit bigger no doubt):

            • coursecat_sortable_records & coursecat::sort_records
            • explain why I don't check managecap before displaying bulk actions.
            • make course/classes/management/tree.php course/classes/management/tree_category.php use existing coursecat.

            Keep it up!

            Many thanks
            Sam

            Show
            Sam Hemelryk added a comment - Hi Marina, In response to the points you've raised so far: Fixes made: Fixed regression in coursecat::get_tree and added unit test to cover it. Fixed purging when resorting categories. Fixed coursecat::resort_courses to make DB query with preload context. Also has unit test now. Removed additional test in coursecatlib_test.php search_courses. ajax/management.php now calls header() footer() fixed up use of course list name where appropriate. Sometimes (course detail for instance) I still needed fullname and shortname separate. However list display is now corrected. Fixed new course link. fixed inclusion of /course/moodleform_mod.php idnumbertake => categoryidnumbertaken Moved logic from management helper to appropriate places: perform_* are no more, now on coursecat or course/lib.php. I've also converted to use update_course where applicable. core_course_management_renderer now extends plugin_renderer_base instead of core_course_renderer The inverse delete course hash check has been fixed. Regarding points you raised that I have an opinion on (who'd have guessed): coursecat::can_create_top_level_category I've left in there for the time being. After thinking about it I like the distinction it creates for top level categories and should be ever create additional checks or limits for creating to level categories such a method will make it a little easier to apply. A sort of future proofing. That being said I'm not in love with it so should it be felt that its best removed I'd happily do so. Regarding preloading of contexts in course_in_list it appears it always preloads during construction. Is it necessary to have it double check contexts have been preloaded? The files with 2002 copyrights comes from the file creation date is all. Rather than using the 1999 standard I bumped it to that. The changes in outputcomponents.php we made as a separate issue and were integrated this week. I've now rebased and this patch no longer makes any changes to that file. classes/editcategory_form.php must include moodleform_mod.php autoloading renderer. Haha I thought that may raise a few questions. It's not being done before but to me that solves a vital issue, where to put secondary renderers for a component/plugin. I've converted it to extend plugin_renderer_base now as it doesn't depend upon core_course_renderer at all. Left to do (still on my list and a bit bigger no doubt): coursecat_sortable_records & coursecat::sort_records explain why I don't check managecap before displaying bulk actions. make course/classes/management/tree.php course/classes/management/tree_category.php use existing coursecat. Keep it up! Many thanks Sam
            Hide
            Vadim Dvorovenko added a comment -

            As far as we a speaking about deep rewriting of course category management pages, as lang-pack maintainer i want to ask you to store all strings in separate file, even if they where prevously taken from core. It will simplify translation (I often have situation when one string from moodle.php langfile is used in many places in such a different contexts, that it couldn't be correctly translated to fit everywhere) and will help making moodle langpack a bit more modular.

            Show
            Vadim Dvorovenko added a comment - As far as we a speaking about deep rewriting of course category management pages, as lang-pack maintainer i want to ask you to store all strings in separate file, even if they where prevously taken from core. It will simplify translation (I often have situation when one string from moodle.php langfile is used in many places in such a different contexts, that it couldn't be correctly translated to fit everywhere) and will help making moodle langpack a bit more modular.
            Hide
            Marina Glancy added a comment -

            course/classes/editcategory_form.php

            1. Why require_once($CFG->dirroot.'/course/moodleform_mod.php'); ? (I can see though that it was copy-pasted from original file)
            2. It should check the manage cap in the parent category before displaying a dropdown to change parent
            3. get_string('idnumbertaken') - Please note that this string has been recently changed, you might need a new one - MDL-41256

            course/classes/management/helper.php

            1. Please pass the full $course object to get_fast_modinfo() when you have it - it will save a DB query when validating the caches
            2. There should be no API functions and throwing evens or writing to logs there, they should be new functions either in course/lib.php or in lib/coursecatlib.php:
              1. perform_course_move_by_one(), also don't add a snapshot there because it is not accurate and does not reflect the new sortorder after calling fix_course_sortorder(), see changes in MDL-41557
              2. perform_course_change_visibility() should be a function in /course/lib.php maybe even a bulk one, like move_courses()
              3. perform_category_move_by_one() should be some function in class coursecat, and it should not call add_to_log but trigger an event
              4. move_course_after() again, /course/lib.php and you are missing event there
                If you want, you can delegate to backend team all these three functions (they will include unittests, etc.)

            course/classes/management/tree.php, course/classes/management/tree_category.php
            I'm really surprised... if you need some tree functionality that coursecat does not have - let's better fix coursecat...
            Let's talk tomorrow about it

            course/classes/management_renderer.php
            Do we have a policy that we can put renderers in autoloader? imho it's confusing.
            Besides, extending another core renderer is sooo bad, we have so many problems with it in course formats already. Developers don't realise that overwriting format_section_renderer_base does not affect any of the formats (such as topics or week or contributed) that extend this class. Instead of extending it's much better to call get_renderer('core', 'course') when you want to access functions from core_course_renderer.
            BTW speaking of core_course_renderer, we are about to submit for integration MDL-38661

            course/delete.php
            This looks crazy:

            // Check if we've got confirmation.
            if ($delete !== md5($course->timemodified)) {
                // We do - time to delete the course. 
            

            Are you sure it should be !== ?

            PS I did not look at JS and CSS at all

            Show
            Marina Glancy added a comment - course/classes/editcategory_form.php Why require_once($CFG->dirroot.'/course/moodleform_mod.php'); ? (I can see though that it was copy-pasted from original file) It should check the manage cap in the parent category before displaying a dropdown to change parent get_string('idnumbertaken') - Please note that this string has been recently changed, you might need a new one - MDL-41256 course/classes/management/helper.php Please pass the full $course object to get_fast_modinfo() when you have it - it will save a DB query when validating the caches There should be no API functions and throwing evens or writing to logs there, they should be new functions either in course/lib.php or in lib/coursecatlib.php: perform_course_move_by_one(), also don't add a snapshot there because it is not accurate and does not reflect the new sortorder after calling fix_course_sortorder(), see changes in MDL-41557 perform_course_change_visibility() should be a function in /course/lib.php maybe even a bulk one, like move_courses() perform_category_move_by_one() should be some function in class coursecat, and it should not call add_to_log but trigger an event move_course_after() again, /course/lib.php and you are missing event there If you want, you can delegate to backend team all these three functions (they will include unittests, etc.) course/classes/management/tree.php, course/classes/management/tree_category.php I'm really surprised... if you need some tree functionality that coursecat does not have - let's better fix coursecat... Let's talk tomorrow about it course/classes/management_renderer.php Do we have a policy that we can put renderers in autoloader? imho it's confusing. Besides, extending another core renderer is sooo bad, we have so many problems with it in course formats already. Developers don't realise that overwriting format_section_renderer_base does not affect any of the formats (such as topics or week or contributed) that extend this class. Instead of extending it's much better to call get_renderer('core', 'course') when you want to access functions from core_course_renderer. BTW speaking of core_course_renderer, we are about to submit for integration MDL-38661 course/delete.php This looks crazy: // Check if we've got confirmation. if ($delete !== md5($course->timemodified)) { // We do - time to delete the course. Are you sure it should be !== ? PS I did not look at JS and CSS at all
            Hide
            Andrew Davis added a comment -

            My question is just, what's up with all the back slashes?

            Show
            Andrew Davis added a comment - My question is just, what's up with all the back slashes?
            Hide
            Sam Hemelryk added a comment -

            Thanks for continuing the effort guys

            Pushed up a couple of modified branches to address the following:

            • Allowed view modes to be viewed without selected components (now displays a message to select a category where required).
            • Fixed missing param when resorting courses. Regression from earlier patch.
            • Fixed display of 999 now uses 999 and displays "all".
            • Fixed course show/hide button.
            • Fixed regressions in course/delete.php
            • Fixed phpdocs @package on modified files.

            In regards to Andrews points:

            • I decided against hiding the move icon. After playing around with it it just looked visually wrong to have it appear when mouse over.
            • I'm not too sure what you are getting at with the action_course_moveup_by_record and movedown questions sorry Andrew, can you please clarify?

            Marina thanks for all the great points, I'll start looking at your points first thing tomorrow (end of my day here).

            Many thanks
            Sam

            Show
            Sam Hemelryk added a comment - Thanks for continuing the effort guys Pushed up a couple of modified branches to address the following: Allowed view modes to be viewed without selected components (now displays a message to select a category where required). Fixed missing param when resorting courses. Regression from earlier patch. Fixed display of 999 now uses 999 and displays "all". Fixed course show/hide button. Fixed regressions in course/delete.php Fixed phpdocs @package on modified files. In regards to Andrews points: I decided against hiding the move icon. After playing around with it it just looked visually wrong to have it appear when mouse over. I'm not too sure what you are getting at with the action_course_moveup_by_record and movedown questions sorry Andrew, can you please clarify? Marina thanks for all the great points, I'll start looking at your points first thing tomorrow (end of my day here). Many thanks Sam
            Hide
            Marina Glancy added a comment -

            Link "New course" leads to page editcategory.php

            Show
            Marina Glancy added a comment - Link "New course" leads to page editcategory.php
            Hide
            Marina Glancy added a comment -

            Sam, you have so many comments on this issue, I don't know how you are going to go through all of them...

            Displaying course names and re-sorting courses

            There is a setting $CFG->courselistshortnames and there is a function get_course_display_name_for_list(). Course name in the list may be changed.
            There is also a function coursecat_helper::get_course_formatted_name() that displays course name as it is specified in the setting (which forgets to preload course context but that's another story - it's my bug and I'll fix it).

            Also for sorting of the list of courses YOU helped me to create a class coursecat_sortable_records and there is a function coursecat::sort_records() which you don't use when sorting courses.

            This is just FYI because you seem to implement the similar functions in course_in_list.

            Show
            Marina Glancy added a comment - Sam, you have so many comments on this issue, I don't know how you are going to go through all of them... Displaying course names and re-sorting courses There is a setting $CFG->courselistshortnames and there is a function get_course_display_name_for_list(). Course name in the list may be changed. There is also a function coursecat_helper::get_course_formatted_name() that displays course name as it is specified in the setting (which forgets to preload course context but that's another story - it's my bug and I'll fix it). Also for sorting of the list of courses YOU helped me to create a class coursecat_sortable_records and there is a function coursecat::sort_records() which you don't use when sorting courses. This is just FYI because you seem to implement the similar functions in course_in_list.
            Hide
            Marina Glancy added a comment -

            General:
            I noticed some Martin's copyrights dated 2002 in the new files - is it intentional?

            lib/outputcomponents.php

            1. look like it should be a separate issue with testing of impact on other places where it is used
            2. action_menu_link::__construct() and also primary, secondary - how can you set default value of an argument in the middle of the list?

            lib/tests/coursecatlib_test.php

            1. Please don't add the test in the end of test_get_search_courses(), it is unreachable on mssql because non-latin search fails it

            course/ajax/management.php

            1. It should call echo $OUTPUT->header();
            Show
            Marina Glancy added a comment - General: I noticed some Martin's copyrights dated 2002 in the new files - is it intentional? lib/outputcomponents.php look like it should be a separate issue with testing of impact on other places where it is used action_menu_link::__construct() and also primary, secondary - how can you set default value of an argument in the middle of the list? lib/tests/coursecatlib_test.php Please don't add the test in the end of test_get_search_courses(), it is unreachable on mssql because non-latin search fails it course/ajax/management.php It should call echo $OUTPUT->header();
            Hide
            Andrew Davis added a comment -

            + * @package core
            + * @copyright 2002 onwards Martin Dougiamas (http://dougiamas.com)
            + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
            + */

            Should that be core_course?

            Similarly, see http://docs.moodle.org/dev/Coding_style#.40package Im pretty sure

            + * @package core
            + * @subpackage course

            should be

            + * @package core_course

            What is going on here?

            $outcome->outcome = \core_course\management\helper::action_course_moveup_by_record($courseid);

            and here?

            +            $actions['movedown'] = array(
            +                'url' => new \moodle_url($baseurl, array('action' => 'movecategorydown')),
            +                'icon' => new \pix_icon('t/down', new \lang_string('down')),
            +                'string' => new \lang_string('down')
            +            );
            
            Show
            Andrew Davis added a comment - + * @package core + * @copyright 2002 onwards Martin Dougiamas ( http://dougiamas.com ) + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ Should that be core_course? Similarly, see http://docs.moodle.org/dev/Coding_style#.40package Im pretty sure + * @package core + * @subpackage course should be + * @package core_course What is going on here? $outcome->outcome = \core_course\management\helper::action_course_moveup_by_record($courseid); and here? + $actions['movedown'] = array( + 'url' => new \moodle_url($baseurl, array('action' => 'movecategorydown')), + 'icon' => new \pix_icon('t/down', new \lang_string('down')), + 'string' => new \lang_string('down') + );
            Hide
            Marina Glancy added a comment - - edited

            Hi Sam, thanks for working on it.
            I'm doing a code review first, will post several comments.

            lib/coursecatlib.php

            1. Your changes to coursecat::get_tree() may cause a regression when somebody requests coursecat::get_tree('countall') as the first request after cache was cleared.
            2. I'm not sure I like static coursecat::can_create_top_level_category() - I'd prefer to have all login in can_create_subcategory() and use: coursecat::get(0)->can_create_subcategory(). But it's up to you
            3. coursecat::resort_categories_cleanup() should purge by event 'changesincoursecat'
            4. Why do you use global function get_courses() in coursecat::resort_courses() ? It performs a lot of unnecessary actions like retrieving contexts and also checks capability to view hidden courses. Except that it is extra work/queries, if for some crazy reason user does not have such capability it could lead to mess in the new sortorder. imho it's better to make one DB query instead. PS Looking at that I noticed that fix_course_sortorder() does not retrieve and preload contexts though it should but that's another issue.
            5. course_in_list::get_context() must call first context_helper::preload_from_record(...) - it is preloaded only for hidden courses, see coursecat::get_course_records(). Similarly functions can_delete_course() and can_access() should make sure that they preloaded context

            edited:

            1. course_in_list::get_formatted_fullname() and course_in_list::get_formatted_shortname(). Hm, I've just noticed that I also format course and categories names to their context which is not performance-wise. I do it in coursecat_helper::get_course_formatted_name() and apparently not optimal (see my previous comment about preloading contexts). Which made me thinking that something needs to be changed in 2.5 as well... Let's discuss it later separately

            ... to be continued

            Show
            Marina Glancy added a comment - - edited Hi Sam, thanks for working on it. I'm doing a code review first, will post several comments. lib/coursecatlib.php Your changes to coursecat::get_tree() may cause a regression when somebody requests coursecat::get_tree('countall') as the first request after cache was cleared. I'm not sure I like static coursecat::can_create_top_level_category() - I'd prefer to have all login in can_create_subcategory() and use: coursecat::get(0)->can_create_subcategory(). But it's up to you coursecat::resort_categories_cleanup() should purge by event 'changesincoursecat' Why do you use global function get_courses() in coursecat::resort_courses() ? It performs a lot of unnecessary actions like retrieving contexts and also checks capability to view hidden courses. Except that it is extra work/queries, if for some crazy reason user does not have such capability it could lead to mess in the new sortorder. imho it's better to make one DB query instead. PS Looking at that I noticed that fix_course_sortorder() does not retrieve and preload contexts though it should but that's another issue. course_in_list::get_context() must call first context_helper::preload_from_record(...) - it is preloaded only for hidden courses, see coursecat::get_course_records(). Similarly functions can_delete_course() and can_access() should make sure that they preloaded context edited: course_in_list::get_formatted_fullname() and course_in_list::get_formatted_shortname(). Hm, I've just noticed that I also format course and categories names to their context which is not performance-wise. I do it in coursecat_helper::get_course_formatted_name() and apparently not optimal (see my previous comment about preloading contexts). Which made me thinking that something needs to be changed in 2.5 as well... Let's discuss it later separately ... to be continued
            Hide
            Andrew Davis added a comment -

            When I select a category then a course and click the "Show" button in the course box it appears bring me back to the same screen. What exactly is going on there?

            When I click the delete button I get this.

            A required parameter (sesskey) was missing

            More information about this error
            Debug info:
            Error code: missingparam
            Stack trace:

            line 476 of /lib/setuplib.php: moodle_exception thrown
            line 539 of /lib/moodlelib.php: call to print_error()
            line 1033 of /lib/sessionlib.php: call to required_param()
            line 1044 of /lib/sessionlib.php: call to confirm_sesskey()
            line 51 of /course/delete.php: call to require_sesskey()

            Output buffer: <br /> <font size='1'><table class='xdebug-error xe-notice' dir='ltr' border='1' cellspacing='0' cellpadding='1'> <tr><th align='left' bgcolor='#f57900' colspan="5"><span style='background-color: #cc0000; color: #fce94f; font-size: x-large;'>( ! )</span> Notice: Undefined variable: course in /home/andrew/Desktop/code/moodle/dev/master/course/delete.php on line <i>35</i></th></tr> <tr><th align='left' bgcolor='#e9b96e' colspan='5'>Call Stack</th></tr> <tr><th align='center' bgcolor='#eeeeec'>#</th><th align='left' bgcolor='#eeeeec'>Time</th><th align='left' bgcolor='#eeeeec'>Memory</th><th align='left' bgcolor='#eeeeec'>Function</th><th align='left' bgcolor='#eeeeec'>Location</th></tr> <tr><td bgcolor='#eeeeec' align='center'>1</td><td bgcolor='#eeeeec' align='center'>0.0226</td><td bgcolor='#eeeeec' align='right'>254096</td><td bgcolor='#eeeeec'>

            Unknown macro: {main}

            ( )</td><td title='/home/andrew/Desktop/code/moodle/dev/master/course/delete.php' bgcolor='#eeeeec'>../delete.php<b>:</b>0</td></tr> </table></font> <br /> <font size='1'><table class='xdebug-error xe-notice' dir='ltr' border='1' cellspacing='0' cellpadding='1'> <tr><th align='left' bgcolor='#f57900' colspan="5"><span style='background-color: #cc0000; color: #fce94f; font-size: x-large;'>( ! )</span> Notice: Trying to get property of non-object in /home/andrew/Desktop/code/moodle/dev/master/course/delete.php on line <i>35</i></th></tr> <tr><th align='left' bgcolor='#e9b96e' colspan='5'>Call Stack</th></tr> <tr><th align='center' bgcolor='#eeeeec'>#</th><th align='left' bgcolor='#eeeeec'>Time</th><th align='left' bgcolor='#eeeeec'>Memory</th><th align='left' bgcolor='#eeeeec'>Function</th><th align='left' bgcolor='#eeeeec'>Location</th></tr> <tr><td bgcolor='#eeeeec' align='center'>1</td><td bgcolor='#eeeeec' align='center'>0.0226</td><td bgcolor='#eeeeec' align='right'>254096</td><td bgcolor='#eeeeec'>

            ( )</td><td title='/home/andrew/Desktop/code/moodle/dev/master/course/delete.php' bgcolor='#eeeeec'>../delete.php<b>:</b>0</td></tr> </table></font>

            Show
            Andrew Davis added a comment - When I select a category then a course and click the "Show" button in the course box it appears bring me back to the same screen. What exactly is going on there? When I click the delete button I get this. A required parameter (sesskey) was missing More information about this error Debug info: Error code: missingparam Stack trace: line 476 of /lib/setuplib.php: moodle_exception thrown line 539 of /lib/moodlelib.php: call to print_error() line 1033 of /lib/sessionlib.php: call to required_param() line 1044 of /lib/sessionlib.php: call to confirm_sesskey() line 51 of /course/delete.php: call to require_sesskey() Output buffer: <br /> <font size='1'><table class='xdebug-error xe-notice' dir='ltr' border='1' cellspacing='0' cellpadding='1'> <tr><th align='left' bgcolor='#f57900' colspan="5"><span style='background-color: #cc0000; color: #fce94f; font-size: x-large;'>( ! )</span> Notice: Undefined variable: course in /home/andrew/Desktop/code/moodle/dev/master/course/delete.php on line <i>35</i></th></tr> <tr><th align='left' bgcolor='#e9b96e' colspan='5'>Call Stack</th></tr> <tr><th align='center' bgcolor='#eeeeec'>#</th><th align='left' bgcolor='#eeeeec'>Time</th><th align='left' bgcolor='#eeeeec'>Memory</th><th align='left' bgcolor='#eeeeec'>Function</th><th align='left' bgcolor='#eeeeec'>Location</th></tr> <tr><td bgcolor='#eeeeec' align='center'>1</td><td bgcolor='#eeeeec' align='center'>0.0226</td><td bgcolor='#eeeeec' align='right'>254096</td><td bgcolor='#eeeeec'> Unknown macro: {main} ( )</td><td title='/home/andrew/Desktop/code/moodle/dev/master/course/delete.php' bgcolor='#eeeeec'>../delete.php<b>:</b>0</td></tr> </table></font> <br /> <font size='1'><table class='xdebug-error xe-notice' dir='ltr' border='1' cellspacing='0' cellpadding='1'> <tr><th align='left' bgcolor='#f57900' colspan="5"><span style='background-color: #cc0000; color: #fce94f; font-size: x-large;'>( ! )</span> Notice: Trying to get property of non-object in /home/andrew/Desktop/code/moodle/dev/master/course/delete.php on line <i>35</i></th></tr> <tr><th align='left' bgcolor='#e9b96e' colspan='5'>Call Stack</th></tr> <tr><th align='center' bgcolor='#eeeeec'>#</th><th align='left' bgcolor='#eeeeec'>Time</th><th align='left' bgcolor='#eeeeec'>Memory</th><th align='left' bgcolor='#eeeeec'>Function</th><th align='left' bgcolor='#eeeeec'>Location</th></tr> <tr><td bgcolor='#eeeeec' align='center'>1</td><td bgcolor='#eeeeec' align='center'>0.0226</td><td bgcolor='#eeeeec' align='right'>254096</td><td bgcolor='#eeeeec'> ( )</td><td title='/home/andrew/Desktop/code/moodle/dev/master/course/delete.php' bgcolor='#eeeeec'>../delete.php<b>:</b>0</td></tr> </table></font>
            Hide
            Andrew Davis added a comment -

            When I select "per page ALL" its still displaying "Per page: 999". Using the value 999 is fine but I dont think we should be displaying it in the UI.

            Show
            Andrew Davis added a comment - When I select "per page ALL" its still displaying "Per page: 999". Using the value 999 is fine but I dont think we should be displaying it in the UI.
            Hide
            Andrew Davis added a comment -

            Very minor (and maybe the current behaviour is correct) but the arrow drag course icons are always visible while the rest of the icons only appear when the mouse is over the course.

            I pick a category with 2 courses in it and try to re-sort the courses I get the following. It doesn't matter what I search by.

            A required parameter (categoryid) was missing

            More information about this error
            Debug info:
            Error code: missingparam
            Stack trace:

            line 476 of /lib/setuplib.php: moodle_exception thrown
            line 539 of /lib/moodlelib.php: call to print_error()
            line 133 of /course/management.php: call to required_param()

            Show
            Andrew Davis added a comment - Very minor (and maybe the current behaviour is correct) but the arrow drag course icons are always visible while the rest of the icons only appear when the mouse is over the course. I pick a category with 2 courses in it and try to re-sort the courses I get the following. It doesn't matter what I search by. A required parameter (categoryid) was missing More information about this error Debug info: Error code: missingparam Stack trace: line 476 of /lib/setuplib.php: moodle_exception thrown line 539 of /lib/moodlelib.php: call to print_error() line 133 of /course/management.php: call to required_param()
            Hide
            Andrew Davis added a comment - - edited

            There's some clarification of the logic needed with the navigation link.

            When I first arrive no category is selected. If I select "Courses" or "Course categories and courses" I seem to be bounced back to "viewing course categories". Once I've selected any category I can then select these items and happily switch categories within those pages.

            What about doing one of these things (or something else entirely):

            1) disable those items in the nav link thing until a category is selected.

            2) display those pages but with some "empty" boxes. For example the "Course categories and courses" page, the category box on the right could be a box that just says "please select a category" until a category is selected.

            Show
            Andrew Davis added a comment - - edited There's some clarification of the logic needed with the navigation link. When I first arrive no category is selected. If I select "Courses" or "Course categories and courses" I seem to be bounced back to "viewing course categories". Once I've selected any category I can then select these items and happily switch categories within those pages. What about doing one of these things (or something else entirely): 1) disable those items in the nav link thing until a category is selected. 2) display those pages but with some "empty" boxes. For example the "Course categories and courses" page, the category box on the right could be a box that just says "please select a category" until a category is selected.
            Hide
            Sam Hemelryk added a comment -

            Vadim if you are still around if you are able to take another look at this I'd really appreciate your thoughts on it now that I've tidied things up.
            Obviously Andrew is still finding issues but we are now working to smooth things out.

            Show
            Sam Hemelryk added a comment - Vadim if you are still around if you are able to take another look at this I'd really appreciate your thoughts on it now that I've tidied things up. Obviously Andrew is still finding issues but we are now working to smooth things out.
            Hide
            Sam Hemelryk added a comment -

            LOL Icon madness?!

            I spotted one bug in there but while looking into it I thought up a much better means of handling and as such have re-written how we "toggle" icons. Now both icons are rendered always as we only show the relevant option.
            This removes the requirement for JS to handle toggling, and in doing so reduces the the requirement for the HTML to be so rigid.

            The following fixes have now been made:

            • Fixed show/hide icon toggling.
            • Moving a course now triggers a redirect ensure existing objects are not stale.
            • Fixed wording when a category contains no courses.
            • The default category is no longer selected by default when the user arrives at the management pages without params (this was noted by Vadim above as well).

            All yours again thanks Andrew

            Show
            Sam Hemelryk added a comment - LOL Icon madness?! I spotted one bug in there but while looking into it I thought up a much better means of handling and as such have re-written how we "toggle" icons. Now both icons are rendered always as we only show the relevant option. This removes the requirement for JS to handle toggling, and in doing so reduces the the requirement for the HTML to be so rigid. The following fixes have now been made: Fixed show/hide icon toggling. Moving a course now triggers a redirect ensure existing objects are not stale. Fixed wording when a category contains no courses. The default category is no longer selected by default when the user arrives at the management pages without params (this was noted by Vadim above as well). All yours again thanks Andrew
            Hide
            Andrew Davis added a comment -

            If I have a category with one course in it and i move that course to another category I'm still told "Showing all 1 courses". Clicking on the category name to get the page to reload causes the message to correct. Are we determining the number of courses before processing the move?

            Show
            Andrew Davis added a comment - If I have a category with one course in it and i move that course to another category I'm still told "Showing all 1 courses". Clicking on the category name to get the page to reload causes the message to correct. Are we determining the number of courses before processing the move?
            Hide
            Andrew Davis added a comment -

            If I select a category with 0 courses in it the category detail block says "Showing courses -19 to 0 of 0 courses"

            Show
            Andrew Davis added a comment - If I select a category with 0 courses in it the category detail block says "Showing courses -19 to 0 of 0 courses"
            Hide
            Andrew Davis added a comment -

            Hiding/unhiding still isnt quite right. I'm not sure what I did to get where I am but I'm having trouble getting back. I tried to sort it out but only made it worse.

            A visible category and a hidden course with the same eye icon.
            https://tracker.moodle.org/secure/attachment/34415/hidden.png

            A visible category with the hidden icon.
            https://tracker.moodle.org/secure/attachment/34416/howdoesthishappen.png

            Now a visible category and course with the hidden icon.
            https://tracker.moodle.org/secure/attachment/34417/what.png

            Two visible categories yet they have different icons.
            https://tracker.moodle.org/secure/attachment/34418/whatthe.png

            Show
            Andrew Davis added a comment - Hiding/unhiding still isnt quite right. I'm not sure what I did to get where I am but I'm having trouble getting back. I tried to sort it out but only made it worse. A visible category and a hidden course with the same eye icon. https://tracker.moodle.org/secure/attachment/34415/hidden.png A visible category with the hidden icon. https://tracker.moodle.org/secure/attachment/34416/howdoesthishappen.png Now a visible category and course with the hidden icon. https://tracker.moodle.org/secure/attachment/34417/what.png Two visible categories yet they have different icons. https://tracker.moodle.org/secure/attachment/34418/whatthe.png
            Hide
            Andrew Davis added a comment -

            Maybe I should change all to "max" do you think?

            Not sure thats worth bothering about. Ideally this hard limit would be documented somewhere that you can link to if this comes up in the forums (along with a link to a document explaining why having upwards of a thousand courses in a single category is a bad idea).

            Just make sure that its not displaying 999 in the UI. If I select all (or max) it should say all (or max).

            Show
            Andrew Davis added a comment - Maybe I should change all to "max" do you think? Not sure thats worth bothering about. Ideally this hard limit would be documented somewhere that you can link to if this comes up in the forums (along with a link to a document explaining why having upwards of a thousand courses in a single category is a bad idea). Just make sure that its not displaying 999 in the UI. If I select all (or max) it should say all (or max).
            Hide
            Andrew Davis added a comment -

            In regards to the action icons weirdness, I've set it up so that the action icons are always visible for the category/course that is currently selected.

            Ah, I see. I think there's something a little inconsistent here that we just need to iron out.

            We I first go to /course/management.php it appears to auto-select the first category in the list. That category is highlighted in category list block and displayed in the category detail box. No course box is displayed. When I click a course in the category detail box the course box is displayed.

            I'm not sure that we should be automatically selecting the first category. Perhaps we should initially either not display the category detail box (similar to how we only show the course box once a course is selected) or display it "empty".

            The category highlight makes more sense if it happens as a result of me selecting a category.

            Show
            Andrew Davis added a comment - In regards to the action icons weirdness, I've set it up so that the action icons are always visible for the category/course that is currently selected. Ah, I see. I think there's something a little inconsistent here that we just need to iron out. We I first go to /course/management.php it appears to auto-select the first category in the list. That category is highlighted in category list block and displayed in the category detail box. No course box is displayed. When I click a course in the category detail box the course box is displayed. I'm not sure that we should be automatically selecting the first category. Perhaps we should initially either not display the category detail box (similar to how we only show the course box once a course is selected) or display it "empty". The category highlight makes more sense if it happens as a result of me selecting a category.
            Hide
            Sam Hemelryk added a comment -

            Thanks Andrew,

            I've been looking at things this morning and think I've now addressed all of the points you've noted here.
            Changes made are as follows:

            • Fixed responsive CSS in the bootstrapbase theme.
            • Fixed navbar when category/course selected.
            • Fixed category visibility bugs.
            • Fixed redirect to manage.php when you cancel deleting category.
            • Improved error reporting when moving categories.
            • Clarified the wording when creating categories/subcategories.

            In regards to points:
            Haha yeah - originally I had it === -1 but in reality more than 1000 course on a page is browser death through shear HTML. Pagination to a certain level is required.
            Maybe I should change all to "max" do you think?

            In regards to the action icons weirdness, I've set it up so that the action icons are always visible for the category/course that is currently selected.
            Action icons are there for all courses and categories but for the unselected ones you must mouse over for them to become visible.
            Perhaps it is confusing to have it like this do you think. It can be easily changed to be visible all the time if we decide that is the way to go.

            All yours once more

            Many thanks and keep it up!
            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Thanks Andrew, I've been looking at things this morning and think I've now addressed all of the points you've noted here. Changes made are as follows: Fixed responsive CSS in the bootstrapbase theme. Fixed navbar when category/course selected. Fixed category visibility bugs. Fixed redirect to manage.php when you cancel deleting category. Improved error reporting when moving categories. Clarified the wording when creating categories/subcategories. In regards to points: Haha yeah - originally I had it === -1 but in reality more than 1000 course on a page is browser death through shear HTML. Pagination to a certain level is required. Maybe I should change all to "max" do you think? In regards to the action icons weirdness, I've set it up so that the action icons are always visible for the category/course that is currently selected. Action icons are there for all courses and categories but for the unselected ones you must mouse over for them to become visible. Perhaps it is confusing to have it like this do you think. It can be easily changed to be visible all the time if we decide that is the way to go. All yours once more Many thanks and keep it up! Cheers Sam
            Hide
            Andrew Davis added a comment -

            When you click "Create new" in the category box you get the option of category or subcategory. Its a slightly bogus choice. All it appears to be doing is setting the parent drop down on the following page. I can select sub category, change parent to Top and get a top level category. Is it even worth giving the user this category Vs subcategory choice?

            When adding and sorting categories I noticed some weirdness with the showing and hiding of action icons in the list of categories. There always seems to be one category whose icons are always visible. The other categories show and hide their icons depending on the mouse position but one special category always shows its icons.
            https://tracker.moodle.org/secure/attachment/34399/controls1.png
            then refresh the page and
            https://tracker.moodle.org/secure/attachment/34400/controls2.png
            No amount of clicking or mouse moving will make that row of icons disappear.

            Show
            Andrew Davis added a comment - When you click "Create new" in the category box you get the option of category or subcategory. Its a slightly bogus choice. All it appears to be doing is setting the parent drop down on the following page. I can select sub category, change parent to Top and get a top level category. Is it even worth giving the user this category Vs subcategory choice? When adding and sorting categories I noticed some weirdness with the showing and hiding of action icons in the list of categories. There always seems to be one category whose icons are always visible. The other categories show and hide their icons depending on the mouse position but one special category always shows its icons. https://tracker.moodle.org/secure/attachment/34399/controls1.png then refresh the page and https://tracker.moodle.org/secure/attachment/34400/controls2.png No amount of clicking or mouse moving will make that row of icons disappear.
            Hide
            Andrew Davis added a comment - - edited

            The UI allows me to try and move a category into itself. When I do I'm taken to a page that says the following.

            Cannot move category

            More information about this error
            Debug info:
            Error code: cannotmovecategory
            Stack trace:

            line 476 of /lib/setuplib.php: moodle_exception thrown
            line 220 of /course/management.php: call to print_error()

            Theres no breadcrumb or selected node in the nav block.

            Clicking the continue button takes me to /admin/index.php?cache=1 If nothing else this shouldn't be moving the user to another area of Moodle.

            Show
            Andrew Davis added a comment - - edited The UI allows me to try and move a category into itself. When I do I'm taken to a page that says the following. Cannot move category More information about this error Debug info: Error code: cannotmovecategory Stack trace: line 476 of /lib/setuplib.php: moodle_exception thrown line 220 of /course/management.php: call to print_error() Theres no breadcrumb or selected node in the nav block. Clicking the continue button takes me to /admin/index.php?cache=1 If nothing else this shouldn't be moving the user to another area of Moodle.
            Hide
            Andrew Davis added a comment - - edited

            If I click "per page: 10" (or whatever) in the current category and select "All" the link changes to "Per page: 999" Infinity == 999. who knew

            Show
            Andrew Davis added a comment - - edited If I click "per page: 10" (or whatever) in the current category and select "All" the link changes to "Per page: 999" Infinity == 999. who knew
            Hide
            Andrew Davis added a comment -

            If I go to delete a category but click cancel I'm taken to a page that says...

            This script has been deprecated and will be removed in the future. Please update any bookmarks you have.

            line 42 of /course/manage.php: call to debugging()

            Show
            Andrew Davis added a comment - If I go to delete a category but click cancel I'm taken to a page that says... This script has been deprecated and will be removed in the future. Please update any bookmarks you have. line 42 of /course/manage.php: call to debugging()
            Hide
            Andrew Davis added a comment - - edited

            Hiding categories doesn't appear to work. There are no JS errors.

            In the Course categories box if I click the eye icon the category name goes grey, the eye icon doesnt change. When I refresh the page the category does not appear to have been hidden.

            If click the eye icon twice, first click and the category name goes grey, second click the eye icon changes to hidden. Refresh the page and it appears that the category still hasnt been hidden.

            Firefox 23.0 in Ubuntu 13.04.

            UPDATE: Something may have been hidden. https://tracker.moodle.org/secure/attachment/34398/hide.png Note the different icon on the course Vs the category. The tooltip on both icons is "Hide" which isnt particularly helpful.

            Show
            Andrew Davis added a comment - - edited Hiding categories doesn't appear to work. There are no JS errors. In the Course categories box if I click the eye icon the category name goes grey, the eye icon doesnt change. When I refresh the page the category does not appear to have been hidden. If click the eye icon twice, first click and the category name goes grey, second click the eye icon changes to hidden. Refresh the page and it appears that the category still hasnt been hidden. Firefox 23.0 in Ubuntu 13.04. UPDATE: Something may have been hidden. https://tracker.moodle.org/secure/attachment/34398/hide.png Note the different icon on the course Vs the category. The tooltip on both icons is "Hide" which isnt particularly helpful.
            Hide
            Andrew Davis added a comment - - edited

            When i first go to site admin > courses > manage courses and categories there is a breadcrumb displayed and the node in the nav block is selected. As soon as I used the top right nav link thing there is no breadcrumb and no selected node in the nav block. Actually doing pretty much anything causes you to wind up on a page with no breadcrumb and no selected nav block node. Hopefully they are all resolved by a single fix.

            Show
            Andrew Davis added a comment - - edited When i first go to site admin > courses > manage courses and categories there is a breadcrumb displayed and the node in the nav block is selected. As soon as I used the top right nav link thing there is no breadcrumb and no selected node in the nav block. Actually doing pretty much anything causes you to wind up on a page with no breadcrumb and no selected nav block node. Hopefully they are all resolved by a single fix.
            Hide
            Sam Hemelryk added a comment -

            Thanks Andrew,

            I've pushed up new branches now to address the operational issues here.
            The following fixes have been made:

            • Changed lang strings "resort" to "re-sort"
            • Fixed typo in string for bulk move courses
            • Fixed optional_param argument that needed to be array based.
            • Fixed missing properties when re-sorting courses.
            • Fixed draganddrop JS issues when changing viewmode (viewing course category and view courses).

            As for the alignment.png issue definitely something going wrong here. I had to convert the media queries when I upgraded this from pre to post 2.5 to fit with bootstrap and rewrite the grid layout to work with either bootstrap or YUI grids depending upon availability. I bet I've missed something there which is causing the issues you are noting with boxes not sitting together nicely.

            Show
            Sam Hemelryk added a comment - Thanks Andrew, I've pushed up new branches now to address the operational issues here. The following fixes have been made: Changed lang strings "resort" to "re-sort" Fixed typo in string for bulk move courses Fixed optional_param argument that needed to be array based. Fixed missing properties when re-sorting courses. Fixed draganddrop JS issues when changing viewmode (viewing course category and view courses). As for the alignment.png issue definitely something going wrong here. I had to convert the media queries when I upgraded this from pre to post 2.5 to fit with bootstrap and rewrite the grid layout to work with either bootstrap or YUI grids depending upon availability. I bet I've missed something there which is causing the issues you are noting with boxes not sitting together nicely.
            Hide
            Andrew Davis added a comment -

            This is at site admin > courses > manage courses and categories

            In the UI should "resort" be "re-sort"? Its a very small thing but I keep reading it as resort as in palm trees, deck chairs and drinks by the pool.

            I have a category called Miscellaneous with two courses in it. https://tracker.moodle.org/secure/attachment/34396/moveCourse.png
            Pretty sure "Move selected categories to" should be "Move selected courses to".

            Im able to move a course to the category it is already in. Pretty minor.

            When I do that I get the following

            Invalid array parameter detected in required_param(): bc
            
                line 631 of /lib/moodlelib.php: call to debugging()
                line 192 of /course/management.php: call to optional_param()
            

            If I sort my two courses by full name or short name I get the following

            ( ! ) Notice: Undefined property: stdClass::$id in /home/andrew/Desktop/code/moodle/dev/master/lib/datalib.php on line 647
            Call Stack

            1. Time Memory Function Location
              1 0.0012 320816
              Unknown macro: {main}

              ( ) ../management.php:0
              2 0.3208 19296032 coursecat->resort_courses( ) ../management.php:117
              3 0.3208 19296288 get_courses( ) ../coursecatlib.php:2271

              ( ! ) Notice: Undefined property: stdClass::$id in /home/andrew/Desktop/code/moodle/dev/master/lib/datalib.php on line 647
              Call Stack

              1. Time Memory Function Location
                1 0.0012 320816

              ( ) ../management.php:0
              2 0.3208 19296032 coursecat->resort_courses( ) ../management.php:117
              3 0.3208 19296288 get_courses( ) ../coursecatlib.php:2271

            ( ! ) Notice: Undefined property: stdClass::$id in /home/andrew/Desktop/code/moodle/dev/master/lib/coursecatlib.php on line 2276
            Call Stack

            1. Time Memory Function Location
              1 0.0012 320816
              Unknown macro: {main}

              ( ) ../management.php:0
              2 0.3208 19296032 coursecat->resort_courses( ) ../management.php:117

            If I sort by id number I get something slightly different. It turns out neither of these courses have an ID number which presumably explains at least the first part of this.

            Did you remember to make the first column something unique in your call to get_records? Duplicate value '' found in column 'idnumber'.

            line 771 of /lib/dml/pgsql_native_moodle_database.php: call to debugging()
            line 636 of /lib/datalib.php: call to pgsql_native_moodle_database->get_records_sql()
            line 2271 of /lib/coursecatlib.php: call to get_courses()
            line 117 of /course/management.php: call to coursecat->resort_courses()

            ( ! ) Notice: Undefined property: stdClass::$id in /home/andrew/Desktop/code/moodle/dev/master/lib/datalib.php on line 647
            Call Stack

            1. Time Memory Function Location
              1 0.0012 320928
              Unknown macro: {main}

              ( ) ../management.php:0
              2 0.2915 19295880 coursecat->resort_courses( ) ../management.php:117
              3 0.2915 19296136 get_courses( ) ../coursecatlib.php:2271

              ( ! ) Notice: Undefined property: stdClass::$id in /home/andrew/Desktop/code/moodle/dev/master/lib/coursecatlib.php on line 2276
              Call Stack

              1. Time Memory Function Location
                1 0.0012 320928

              ( ) ../management.php:0
              2 0.2915 19295880 coursecat->resort_courses( ) ../management.php:117

            When I right clicked a course name an opened a new tab to check the ID numbers I got this. https://tracker.moodle.org/secure/attachment/34397/alignment.png The three boxes aren't left aligned properly for some reason. The shift to the right causes the course box to extend off the right hand edge of the screen even though there isn't anything in that side of the box. I'm guessing its size is being set to fill the screen then something is shifting it to the right.

            Why does the category screen have two boxes side by side but when I click through to a course I get them one under the other? This may be the same issue as above.

            Also, if I click on a course name should the course be on top? It does make sense to go category, courses in category, course. Its just a bit odd to click on a course and then to have to scroll to the bottom to see the course I selected.

            At the top right of the page is a nav link type thing (I presume thats what it is). When it says "Viewing Course categories and courses" you can click on it and get a drop down. However if you go to "Viewing Course categories" or "Viewing Courses" it stays disabled and does nothing when you click on it. No JS errors, it just doesnt do anything.

            Show
            Andrew Davis added a comment - This is at site admin > courses > manage courses and categories In the UI should "resort" be "re-sort"? Its a very small thing but I keep reading it as resort as in palm trees, deck chairs and drinks by the pool. I have a category called Miscellaneous with two courses in it. https://tracker.moodle.org/secure/attachment/34396/moveCourse.png Pretty sure "Move selected categories to" should be "Move selected courses to". Im able to move a course to the category it is already in. Pretty minor. When I do that I get the following Invalid array parameter detected in required_param(): bc line 631 of /lib/moodlelib.php: call to debugging() line 192 of /course/management.php: call to optional_param() If I sort my two courses by full name or short name I get the following ( ! ) Notice: Undefined property: stdClass::$id in /home/andrew/Desktop/code/moodle/dev/master/lib/datalib.php on line 647 Call Stack Time Memory Function Location 1 0.0012 320816 Unknown macro: {main} ( ) ../management.php:0 2 0.3208 19296032 coursecat->resort_courses( ) ../management.php:117 3 0.3208 19296288 get_courses( ) ../coursecatlib.php:2271 ( ! ) Notice: Undefined property: stdClass::$id in /home/andrew/Desktop/code/moodle/dev/master/lib/datalib.php on line 647 Call Stack Time Memory Function Location 1 0.0012 320816 ( ) ../management.php:0 2 0.3208 19296032 coursecat->resort_courses( ) ../management.php:117 3 0.3208 19296288 get_courses( ) ../coursecatlib.php:2271 ( ! ) Notice: Undefined property: stdClass::$id in /home/andrew/Desktop/code/moodle/dev/master/lib/coursecatlib.php on line 2276 Call Stack Time Memory Function Location 1 0.0012 320816 Unknown macro: {main} ( ) ../management.php:0 2 0.3208 19296032 coursecat->resort_courses( ) ../management.php:117 If I sort by id number I get something slightly different. It turns out neither of these courses have an ID number which presumably explains at least the first part of this. Did you remember to make the first column something unique in your call to get_records? Duplicate value '' found in column 'idnumber'. line 771 of /lib/dml/pgsql_native_moodle_database.php: call to debugging() line 636 of /lib/datalib.php: call to pgsql_native_moodle_database->get_records_sql() line 2271 of /lib/coursecatlib.php: call to get_courses() line 117 of /course/management.php: call to coursecat->resort_courses() ( ! ) Notice: Undefined property: stdClass::$id in /home/andrew/Desktop/code/moodle/dev/master/lib/datalib.php on line 647 Call Stack Time Memory Function Location 1 0.0012 320928 Unknown macro: {main} ( ) ../management.php:0 2 0.2915 19295880 coursecat->resort_courses( ) ../management.php:117 3 0.2915 19296136 get_courses( ) ../coursecatlib.php:2271 ( ! ) Notice: Undefined property: stdClass::$id in /home/andrew/Desktop/code/moodle/dev/master/lib/coursecatlib.php on line 2276 Call Stack Time Memory Function Location 1 0.0012 320928 ( ) ../management.php:0 2 0.2915 19295880 coursecat->resort_courses( ) ../management.php:117 When I right clicked a course name an opened a new tab to check the ID numbers I got this. https://tracker.moodle.org/secure/attachment/34397/alignment.png The three boxes aren't left aligned properly for some reason. The shift to the right causes the course box to extend off the right hand edge of the screen even though there isn't anything in that side of the box. I'm guessing its size is being set to fill the screen then something is shifting it to the right. Why does the category screen have two boxes side by side but when I click through to a course I get them one under the other? This may be the same issue as above. Also, if I click on a course name should the course be on top? It does make sense to go category, courses in category, course. Its just a bit odd to click on a course and then to have to scroll to the bottom to see the course I selected. At the top right of the page is a nav link type thing (I presume thats what it is). When it says "Viewing Course categories and courses" you can click on it and get a drop down. However if you go to "Viewing Course categories" or "Viewing Courses" it stays disabled and does nothing when you click on it. No JS errors, it just doesnt do anything.
            Hide
            Sam Hemelryk added a comment -

            Updating the branch details to the dev branch.
            Noting there is also 31830-26-parts with individual commits on it (when pushed for integration I'll push a single commit I think)

            Show
            Sam Hemelryk added a comment - Updating the branch details to the dev branch. Noting there is also 31830-26-parts with individual commits on it (when pushed for integration I'll push a single commit I think)
            Hide
            Sam Hemelryk added a comment -

            Hi Vadim,

            Thanks for having a look at this.
            This project hasn't been touched in the past couple of months and needs to be pulled back into line.
            I've picked up this project again yesterday and will be working on it for the next while.
            I'll have a good look through your list and be sure the address the issues you've noted.

            Many thanks
            Sam

            Show
            Sam Hemelryk added a comment - Hi Vadim, Thanks for having a look at this. This project hasn't been touched in the past couple of months and needs to be pulled back into line. I've picked up this project again yesterday and will be working on it for the next while. I'll have a good look through your list and be sure the address the issues you've noted. Many thanks Sam
            Hide
            Vadim Dvorovenko added a comment -

            Hello, everyone. As far as i understand to test you should use wip-MDL-31830-m26 and then call moodle/course/managemant.php.
            Currently I’m very disappointed with the testing results - the current state of project does not allow including it to mainstream. Here's some of errors
            1. When you open managemant.php without any params, you see all top-level categories and subcategories of first top-level category. This is confusing - if I want to manage top level categories, I do not want to see what's inside. When I want to manage second level category, I
            click top-level category, this correspond to calling managemant.php?categoryid=id and then manage its subcategories.
            2. The second thing that is confusing - ugly css, that does not allow truly see the real structure. Take a look at ‘ugly css.png’ - all checked categories are on the same level, but it's hardly to see.
            3. I've checked the ability to manage categories if you have not global rights. There's problem too. Currently you can move category up and down if you have rights on this category. To move category up or down, you have to check rights on parent category. Here's the difference in behavior of show/hide and up/down.
            4. When trying to reorder categories, error appears – see swapcategory.png
            5. When I do not have global rights and have only manager rights in some category, the button for subcategories creation does not appear. The same with bulk actions.
            6. Trying edit subcategory leads to categoryedit.php instead of editcategory.php, trying to show/hide category leads to error too.
            7. The bulk actions part looks ugly in case of long categories structure. Take a look at ‘long_categories.png’. It also takes additional resourses to build full categories list, which can have more than 1000 categories. I think that bulk actions settings should appear in dialog window only when you press some button. Maybe this button should be invisible until you check some categories or courses.
            8. The most disappointing thing that I found out, that it is not really ajax interface. When I move course up/down – it reloads full page, when I show/hide category – it reloads full page, when I try to edit or delete course – it redirects to another page. I see just some interface that does not make admin’s life better – it does not work faster than old interface. I think category name editing, category moving and bulk actions should be showed in dialog box, not on separate page.
            As for course and categories ordering, I’d prefer if they are dragable.
            9. After all I’m think that representing categories structure with tree is not very good idea, as for me. If we say about managing, not just viewing, it’s better to show only one level of categories with ‘up‘ or ‘..’ button, like in old-school file managers.

            Show
            Vadim Dvorovenko added a comment - Hello, everyone. As far as i understand to test you should use wip- MDL-31830 -m26 and then call moodle/course/managemant.php. Currently I’m very disappointed with the testing results - the current state of project does not allow including it to mainstream. Here's some of errors 1. When you open managemant.php without any params, you see all top-level categories and subcategories of first top-level category. This is confusing - if I want to manage top level categories, I do not want to see what's inside. When I want to manage second level category, I click top-level category, this correspond to calling managemant.php?categoryid=id and then manage its subcategories. 2. The second thing that is confusing - ugly css, that does not allow truly see the real structure. Take a look at ‘ugly css.png’ - all checked categories are on the same level, but it's hardly to see. 3. I've checked the ability to manage categories if you have not global rights. There's problem too. Currently you can move category up and down if you have rights on this category. To move category up or down, you have to check rights on parent category. Here's the difference in behavior of show/hide and up/down. 4. When trying to reorder categories, error appears – see swapcategory.png 5. When I do not have global rights and have only manager rights in some category, the button for subcategories creation does not appear. The same with bulk actions. 6. Trying edit subcategory leads to categoryedit.php instead of editcategory.php, trying to show/hide category leads to error too. 7. The bulk actions part looks ugly in case of long categories structure. Take a look at ‘long_categories.png’. It also takes additional resourses to build full categories list, which can have more than 1000 categories. I think that bulk actions settings should appear in dialog window only when you press some button. Maybe this button should be invisible until you check some categories or courses. 8. The most disappointing thing that I found out, that it is not really ajax interface. When I move course up/down – it reloads full page, when I show/hide category – it reloads full page, when I try to edit or delete course – it redirects to another page. I see just some interface that does not make admin’s life better – it does not work faster than old interface. I think category name editing, category moving and bulk actions should be showed in dialog box, not on separate page. As for course and categories ordering, I’d prefer if they are dragable. 9. After all I’m think that representing categories structure with tree is not very good idea, as for me. If we say about managing, not just viewing, it’s better to show only one level of categories with ‘up‘ or ‘..’ button, like in old-school file managers.
            Hide
            Vadim Dvorovenko added a comment -

            Error, when you're trying to hide or show category.

            Show
            Vadim Dvorovenko added a comment - Error, when you're trying to hide or show category.
            Hide
            Vadim Dvorovenko added a comment -

            Categories list can be very long and very wide - it should not be showed when it's not needed

            Show
            Vadim Dvorovenko added a comment - Categories list can be very long and very wide - it should not be showed when it's not needed
            Hide
            Vadim Dvorovenko added a comment -

            Error when reordering categories

            Show
            Vadim Dvorovenko added a comment - Error when reordering categories
            Hide
            Vadim Dvorovenko added a comment -

            All checked Categories are on the same level, but it's hard to understand

            Show
            Vadim Dvorovenko added a comment - All checked Categories are on the same level, but it's hard to understand
            Hide
            Simon Coggins added a comment -

            At Totara we hire an independent usability consultant (http://op101.com/) to help with product development and do usability reviews.

            We're willing to fund some of his time to review this work and produce a report if that would be helpful. The only issue is that he's not available for the next 3-4 weeks.

            Martin Dougiamas How quickly would you need the feedback for it to still be useful?

            Simon

            Show
            Simon Coggins added a comment - At Totara we hire an independent usability consultant ( http://op101.com/ ) to help with product development and do usability reviews. We're willing to fund some of his time to review this work and produce a report if that would be helpful. The only issue is that he's not available for the next 3-4 weeks. Martin Dougiamas How quickly would you need the feedback for it to still be useful? Simon
            Hide
            Brian King added a comment -

            Which branch should we check?

            Show
            Brian King added a comment - Which branch should we check? https://github.com/samhemelryk/moodle-tool_coursemanagement (last commit a year ago) https://github.com/samhemelryk/moodle/tree/wip-MDL-31830-m26 (last commit 4 months ago)
            Hide
            Martin Dougiamas added a comment -

            Since we are pushed for time on this, we would really appreciate anyone who can try the branch as it is for usability/accessibility and give us feedback on any issues you discover.

            Show
            Martin Dougiamas added a comment - Since we are pushed for time on this, we would really appreciate anyone who can try the branch as it is for usability/accessibility and give us feedback on any issues you discover.
            Hide
            Marina Glancy added a comment -

            Hi Sam, linking to MDL-20996 about the absence of permission to remove categories (for non-administrators), hopefully your issue will resolve it

            Show
            Marina Glancy added a comment - Hi Sam, linking to MDL-20996 about the absence of permission to remove categories (for non-administrators), hopefully your issue will resolve it
            Hide
            Martin Dougiamas added a comment -

            Main things to look at are:

            • accessibility and keyboard control
            • responsiveness on all screens
            • testing all browsers
            • wiring in properly from all entry points in Moodle
            • language strings and translation asap
            Show
            Martin Dougiamas added a comment - Main things to look at are: accessibility and keyboard control responsiveness on all screens testing all browsers wiring in properly from all entry points in Moodle language strings and translation asap
            Hide
            Martin Dougiamas added a comment -

            OK, I'm putting this on the queue for the next sprint to look at finally getting it into 2.6.

            Show
            Martin Dougiamas added a comment - OK, I'm putting this on the queue for the next sprint to look at finally getting it into 2.6.
            Hide
            Brian King added a comment -

            Well, I tried https://github.com/samhemelryk/moodle/tree/wip-MDL-31830-m26 and think it's a great start!

            I suggest having an option to have the target category (e.g. for moving selected categories to) be selectable by opening up a tree of categories, instead of having a huge drop down list. This makes it easier to deal with once the number of categories is, say, > 300.

            With lots of categories and courses, course/management.php is able to load in 10 - 20 seconds, whereas course/manage.php hits a php timeout limit of 120 seconds.

            Show
            Brian King added a comment - Well, I tried https://github.com/samhemelryk/moodle/tree/wip-MDL-31830-m26 and think it's a great start! I suggest having an option to have the target category (e.g. for moving selected categories to) be selectable by opening up a tree of categories, instead of having a huge drop down list. This makes it easier to deal with once the number of categories is, say, > 300. With lots of categories and courses, course/management.php is able to load in 10 - 20 seconds, whereas course/manage.php hits a php timeout limit of 120 seconds.
            Hide
            Brian King added a comment - - edited

            Hi Sam,

            Have you or anyone else been working on this lately? I'd like to kick the wheels with one of the large Moodles I work with. The last commit on the linked github repo was a year ago.

            Is this where I should be looking: https://github.com/samhemelryk/moodle/tree/wip-MDL-31830-m26 ?

            Show
            Brian King added a comment - - edited Hi Sam, Have you or anyone else been working on this lately? I'd like to kick the wheels with one of the large Moodles I work with. The last commit on the linked github repo was a year ago. Is this where I should be looking: https://github.com/samhemelryk/moodle/tree/wip-MDL-31830-m26 ?
            Hide
            Ray Lawrence added a comment -

            Great stuff. Thanks.

            Show
            Ray Lawrence added a comment - Great stuff. Thanks.
            Hide
            Martin Dougiamas added a comment -

            Ray they actually do wrap nicely and responsively on smaller screens. And yes, working with many courses was part of the spec.

            Show
            Martin Dougiamas added a comment - Ray they actually do wrap nicely and responsively on smaller screens. And yes, working with many courses was part of the spec.
            Hide
            Ray Lawrence added a comment -

            The screenshots are very wide. Wider than many admin's monitors.

            Ajax may be pretty but will it be possible to bulk move categories and courses? Dragging courses one at a time will be a PITN. If we consider way that drag and drop of blocks work on typical monitors i.e. very slow and fiddly when you want to drag to a location that's not currently visible there are some lessons for how this shouldn't behave with long lists.

            +1 for the new interface being replicated at individual category level if possible (I anticipate complexity there on permissions e.g. a user assigned to course creator wouldn't normally have permission to enrol users (unless they also had a role on a course with that permission).

            Show
            Ray Lawrence added a comment - The screenshots are very wide. Wider than many admin's monitors. Ajax may be pretty but will it be possible to bulk move categories and courses? Dragging courses one at a time will be a PITN. If we consider way that drag and drop of blocks work on typical monitors i.e. very slow and fiddly when you want to drag to a location that's not currently visible there are some lessons for how this shouldn't behave with long lists. +1 for the new interface being replicated at individual category level if possible (I anticipate complexity there on permissions e.g. a user assigned to course creator wouldn't normally have permission to enrol users (unless they also had a role on a course with that permission).
            Hide
            Howard Miller added a comment -

            I'm just throwing this in as it occurs to me. Whatever we end up getting, can I be assured that it will be tested on a site with hundreds of courses and categories? This is were it all goes horribly wrong on the current system - a category 5 levels down with (say) 30 sub-categories is uneditable.

            However, as has been mentioned, surely the only sensible answer is to have each subcategory fully editable in its own page.

            Show
            Howard Miller added a comment - I'm just throwing this in as it occurs to me. Whatever we end up getting, can I be assured that it will be tested on a site with hundreds of courses and categories? This is were it all goes horribly wrong on the current system - a category 5 levels down with (say) 30 sub-categories is uneditable. However, as has been mentioned, surely the only sensible answer is to have each subcategory fully editable in its own page.
            Hide
            Vadim Dvorovenko added a comment -

            Hi, Martin. If you're not planning to make this tool in this year, why not to take look at MDL-20996? It just add's couple of very needed button's. I'm speaking about this problems since 1.9, but nothing has still changed.

            Show
            Vadim Dvorovenko added a comment - Hi, Martin. If you're not planning to make this tool in this year, why not to take look at MDL-20996 ? It just add's couple of very needed button's. I'm speaking about this problems since 1.9, but nothing has still changed.
            Hide
            Martin Dougiamas added a comment -

            Due to circumstances beyond our control, this is obviously not going to make it for 2.5.

            But I hope to see it land soon afterwards so it can get a lot of playtesting for 2.6.

            Show
            Martin Dougiamas added a comment - Due to circumstances beyond our control, this is obviously not going to make it for 2.5. But I hope to see it land soon afterwards so it can get a lot of playtesting for 2.6.
            Hide
            Vadim Dvorovenko added a comment -

            Hi, Sam.
            Every time when we i look at this tool, i'm thinking about users with manager role in category, who currently cannot delete subcategories and reorder them.
            Can you assing some user manager role in "categoty 3" and in "subcategory "1 of 5", and make couple screenshots to see what such user can see and what can do.
            The other question is how such user can access this tool if it's made as admintool plugin.
            As for working on small screens. We have 4 levels of categories. All categories having about 50 chars in name. Course shortnames are about 30 chars, fullnames are about 200 chars. Please use in your examples longer categories and courses names to see how text wrapping will look.

            Show
            Vadim Dvorovenko added a comment - Hi, Sam. Every time when we i look at this tool, i'm thinking about users with manager role in category, who currently cannot delete subcategories and reorder them. Can you assing some user manager role in "categoty 3" and in "subcategory "1 of 5", and make couple screenshots to see what such user can see and what can do. The other question is how such user can access this tool if it's made as admintool plugin. As for working on small screens. We have 4 levels of categories. All categories having about 50 chars in name. Course shortnames are about 30 chars, fullnames are about 200 chars. Please use in your examples longer categories and courses names to see how text wrapping will look.
            Hide
            Daniel Kaelin added a comment -

            I was brought to this page from another tracker.

            I'm hoping a way to alphabetically sort categories within a parent category can be added.

            Show
            Daniel Kaelin added a comment - I was brought to this page from another tracker. I'm hoping a way to alphabetically sort categories within a parent category can be added.
            Hide
            Martin Dougiamas added a comment -

            Screenshots are looking pretty awesome, Sam, keen to try it soon.

            I'm just worrying a little about what they will do on a small screen. Will the parts reflow to be vertical?

            Show
            Martin Dougiamas added a comment - Screenshots are looking pretty awesome, Sam, keen to try it soon. I'm just worrying a little about what they will do on a small screen. Will the parts reflow to be vertical?
            Hide
            Sam Hemelryk added a comment -

            A couple of updated screenshots

            Show
            Sam Hemelryk added a comment - A couple of updated screenshots
            Hide
            Howard Miller added a comment -

            Can I ask what the status is with this development. I'm noting that the attached git repository hasn't been touched in a year. This is all quite important to me. In fact I'd be very willing to apply some effort if you like.

            Show
            Howard Miller added a comment - Can I ask what the status is with this development. I'm noting that the attached git repository hasn't been touched in a year. This is all quite important to me. In fact I'd be very willing to apply some effort if you like.
            Hide
            Marina Glancy added a comment -

            Sam, I'm linking this issue as being blocked by MDL-37572 where I separate viewing and editing mode for course categories.

            Show
            Marina Glancy added a comment - Sam, I'm linking this issue as being blocked by MDL-37572 where I separate viewing and editing mode for course categories.
            Hide
            Lesli Smith added a comment -

            Hi. I just happened on this conversation after, coincidentally, posting in this thread (https://moodle.org/mod/forum/discuss.php?d=219645#p957500) this morning about ways I also have tried to cope with the category manager issue. I thought it was just that I missed a memo somewhere and that somebody was once again going to say to me, "Lesli, you keep using these words, 'roles' and 'permissions' and 'category contexts' and I don't think they mean what you think they mean." Inconceivable! (princess bride humor)

            I'm so on board with the idea of improvements in this area, but I do have two concerns, somewhat echoing what Howard says:

            1) I've seen categories set up to house departments and/or whole schools making it easier for reports to be run that aggregate data for that specific category in a meaningful way for that organization or section of the organization. (Departments can show how they are meeting certain goals for accreditation reviews; schools can be accountable to their supervisory boards, etc.)

            Sooo...whatever you do t "fix" this issue, please keep in mind that is how it is currently being used in case it breaks something in the way we can currently run reports on categories.

            2) I want to echo Tim's concern about an all-or-nothing approach to alphabetical sorting. But it looks like you are considering that already. Thanks for making these improvements!

            Show
            Lesli Smith added a comment - Hi. I just happened on this conversation after, coincidentally, posting in this thread ( https://moodle.org/mod/forum/discuss.php?d=219645#p957500 ) this morning about ways I also have tried to cope with the category manager issue. I thought it was just that I missed a memo somewhere and that somebody was once again going to say to me, "Lesli, you keep using these words, 'roles' and 'permissions' and 'category contexts' and I don't think they mean what you think they mean." Inconceivable! (princess bride humor) I'm so on board with the idea of improvements in this area, but I do have two concerns, somewhat echoing what Howard says: 1) I've seen categories set up to house departments and/or whole schools making it easier for reports to be run that aggregate data for that specific category in a meaningful way for that organization or section of the organization. (Departments can show how they are meeting certain goals for accreditation reviews; schools can be accountable to their supervisory boards, etc.) Sooo...whatever you do t "fix" this issue, please keep in mind that is how it is currently being used in case it breaks something in the way we can currently run reports on categories. 2) I want to echo Tim's concern about an all-or-nothing approach to alphabetical sorting. But it looks like you are considering that already. Thanks for making these improvements!
            Hide
            Martin Dougiamas added a comment -

            This is definitely happening for 2.5!

            Show
            Martin Dougiamas added a comment - This is definitely happening for 2.5!
            Hide
            Ralf Hilgenstock added a comment -

            Please see also MDL-35500

            Show
            Ralf Hilgenstock added a comment - Please see also MDL-35500
            Hide
            Vadim Dvorovenko added a comment -

            I've updated MDL-20996 pathces, and think this issue should be reopenned.
            Howard, the code is rebased on latest versions so you can easily take part in testing of that code.

            Show
            Vadim Dvorovenko added a comment - I've updated MDL-20996 pathces, and think this issue should be reopenned. Howard, the code is rebased on latest versions so you can easily take part in testing of that code.
            Hide
            Howard Miller added a comment -

            It's because nobody sees the value of assingning Managers to Course Categories. Course Categories are - in the real world - a valuable way to represent the actual organisation (example - a course category per college department). Each part of the organisation (== Course Category) can then have its own restricted Manager.

            The prevelant view is that every Moodle site has one big administrator who does everything. This simply doesn't scale at all for large organisations.

            I've been banging my head off a wall with this one ever since, basically, the whole Course Category thing was broken in Moodle 2.0

            Show
            Howard Miller added a comment - It's because nobody sees the value of assingning Managers to Course Categories. Course Categories are - in the real world - a valuable way to represent the actual organisation (example - a course category per college department). Each part of the organisation (== Course Category) can then have its own restricted Manager. The prevelant view is that every Moodle site has one big administrator who does everything. This simply doesn't scale at all for large organisations. I've been banging my head off a wall with this one ever since, basically, the whole Course Category thing was broken in Moodle 2.0
            Hide
            Vadim Dvorovenko added a comment -

            You're right, Howard. There are only course category page improvements, intended for Managers at course category level, and, of course, with security in mind. Shortly, it just adds two buttons - delete category (only if it is not empty) and resort subcategories, both with proper capabilities check. Very strange that i cannot convince anyone to include this simple changes into core since 1.9

            Show
            Vadim Dvorovenko added a comment - You're right, Howard. There are only course category page improvements, intended for Managers at course category level, and, of course, with security in mind. Shortly, it just adds two buttons - delete category (only if it is not empty) and resort subcategories, both with proper capabilities check. Very strange that i cannot convince anyone to include this simple changes into core since 1.9
            Hide
            Howard Miller added a comment -

            To be honest, I haven't had a chance to look at your new code. However, just to note that this must work properly for users limited to Management rights at the Course Category level. That is, if I assign a user the Manager role for a particular Course Category they can manipulate the categories and courses only within that category. The current big problem is that the category screen is a site-wide entity rather than working properly within each course category. This would also stop it being so unmanageably long on busy sites.

            Show
            Howard Miller added a comment - To be honest, I haven't had a chance to look at your new code. However, just to note that this must work properly for users limited to Management rights at the Course Category level. That is, if I assign a user the Manager role for a particular Course Category they can manipulate the categories and courses only within that category. The current big problem is that the category screen is a site-wide entity rather than working properly within each course category. This would also stop it being so unmanageably long on busy sites.
            Hide
            Vadim Dvorovenko added a comment -

            Hi, Sam! You course management tool looks pretty, but currently it does not solve some very needed actions: alphabetic sorting; any management of categories at all (adding, removing, renaming).
            It does not provide any interface for local managers, wich does not have global admin's rights.

            How about to integrate changes from MDL-20996 into core? It alredy solves problems whith sorting and categories management. If it's necessary I'll rebase patches on latest releases and fix Dan's remarks.

            Show
            Vadim Dvorovenko added a comment - Hi, Sam! You course management tool looks pretty, but currently it does not solve some very needed actions: alphabetic sorting; any management of categories at all (adding, removing, renaming). It does not provide any interface for local managers, wich does not have global admin's rights. How about to integrate changes from MDL-20996 into core? It alredy solves problems whith sorting and categories management. If it's necessary I'll rebase patches on latest releases and fix Dan's remarks.
            Hide
            Tim Hunt added a comment -

            Oh, I thought about the blocks thing right back when we were first working on the new navigation / page / block changes for 2.0. It just never happened, except for the My Moodle page, where Hubert from remote learner stepped up to make the changes.

            Anyway, I won't do anything until after your changes have gone in. Hopefully, after that, the code will be beautifully clean, and easy to slice into blocks.

            Show
            Tim Hunt added a comment - Oh, I thought about the blocks thing right back when we were first working on the new navigation / page / block changes for 2.0. It just never happened, except for the My Moodle page, where Hubert from remote learner stepped up to make the changes. Anyway, I won't do anything until after your changes have gone in. Hopefully, after that, the code will be beautifully clean, and easy to slice into blocks.
            Hide
            Sam Hemelryk added a comment -

            Hi Tim,

            A big part of this change is separating the view and editing of course + categories. course/index.php and course/category.php will view only for sure.
            I do like the idea of having the sort option on the category, and I'm sure we could look to work that in.
            The idea of using blocks for the display and converting the page to block regions is a really interesting idea. Not one that I had considered.
            I don't see any reason that couldn't be done, and perhaps even a good chance to look at the course/category blocks we have.

            Anyway, when we pick this up again I'm sure those two points will be closely considered.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Tim, A big part of this change is separating the view and editing of course + categories. course/index.php and course/category.php will view only for sure. I do like the idea of having the sort option on the category, and I'm sure we could look to work that in. The idea of using blocks for the display and converting the page to block regions is a really interesting idea. Not one that I had considered. I don't see any reason that couldn't be done, and perhaps even a good chance to look at the course/category blocks we have. Anyway, when we pick this up again I'm sure those two points will be closely considered. Cheers Sam
            Hide
            Tim Hunt added a comment -

            Sam, I assume your new UI is intended to replace course/index.php and course/category.php when editing is on, leaving those two pages to just display the information to users when editing is off. If so, +1 from me.

            ------

            I am grappling with a feature request here to always display courses in alphabetical order, rather than having a custom order, which normally ends up being wrong. I actually wonder if the way to do this is by adding a new setting to course_categories:

            Sort questions in this category: According to the custom sort-order / Alphabetically byname / Alphabetically by short-name.

            For example, you might have an "Introduction" course category with just three courses that you want in a specific order that is not related to the name, but you want your "Maths courses" category always in alphabetical order.

            ------

            Anyway, that requirement for custom course sort-order makes me remember that we have an out-standing proposed change for index.php, course/index.php and course/category.php. Theses pages should be like my/index.php, in that they contain only blocks, and the admin can configure what blocks those are. By default they would have blocks that replicate the current display.

            Is that a change you could do while working on disentangling the editing and display parts of the code?

            Show
            Tim Hunt added a comment - Sam, I assume your new UI is intended to replace course/index.php and course/category.php when editing is on, leaving those two pages to just display the information to users when editing is off. If so, +1 from me. ------ I am grappling with a feature request here to always display courses in alphabetical order, rather than having a custom order, which normally ends up being wrong. I actually wonder if the way to do this is by adding a new setting to course_categories: Sort questions in this category: According to the custom sort-order / Alphabetically byname / Alphabetically by short-name. For example, you might have an "Introduction" course category with just three courses that you want in a specific order that is not related to the name, but you want your "Maths courses" category always in alphabetical order. ------ Anyway, that requirement for custom course sort-order makes me remember that we have an out-standing proposed change for index.php, course/index.php and course/category.php. Theses pages should be like my/index.php, in that they contain only blocks, and the admin can configure what blocks those are. By default they would have blocks that replicate the current display. Is that a change you could do while working on disentangling the editing and display parts of the code?
            Hide
            Susan Mangan added a comment -

            Same issue. We have always struggled with the manual sorting of course categories being a timely and laborious task. As a workaround I manually go into the database and change the category sort order.

            Show
            Susan Mangan added a comment - Same issue. We have always struggled with the manual sorting of course categories being a timely and laborious task. As a workaround I manually go into the database and change the category sort order.
            Hide
            Donna Hrynkiw added a comment -

            At the beginning of each semester I spend a considerable amount of time sorting newly-created course categories into (generally but not always) alphabetical order. It's quite cumbersome to arrange them using the up/down arrows.

            It would be awfully nice if I could drag-n-drop the categories into order just as teachers are able to drag-n-drop course resources (using the "AJAX and Javascript advanced web features").

            Alternately, a button for sorting categories similar to the "Re-sort courses by name" would be very welcome.

            Show
            Donna Hrynkiw added a comment - At the beginning of each semester I spend a considerable amount of time sorting newly-created course categories into (generally but not always) alphabetical order. It's quite cumbersome to arrange them using the up/down arrows. It would be awfully nice if I could drag-n-drop the categories into order just as teachers are able to drag-n-drop course resources (using the "AJAX and Javascript advanced web features"). Alternately, a button for sorting categories similar to the "Re-sort courses by name" would be very welcome.
            Hide
            Michael de Raadt added a comment -

            It looks like there is some interest in this from administrators.

            Show
            Michael de Raadt added a comment - It looks like there is some interest in this from administrators.
            Hide
            Adrian Greeve added a comment -

            Dan made a comment in MDL-20996 that this is on the road map for inclusion in Moodle 2.4.
            This is a reminder to include an ability for non-global administrators (managers) to manage a possible sub-sets of categories and courses if they have the appropriate permissions.

            Show
            Adrian Greeve added a comment - Dan made a comment in MDL-20996 that this is on the road map for inclusion in Moodle 2.4. This is a reminder to include an ability for non-global administrators (managers) to manage a possible sub-sets of categories and courses if they have the appropriate permissions.
            Hide
            Paul Nicholls added a comment -

            One thing that would be great to add would be the ability to enrol users into courses.
            Possible workflow:

            • select a bunch of courses (or just one)
            • choose to enrol users into selected course(s)
            • choose a role (this could be a dropdown/set of radio buttons/etc on the next step)
            • search for users to add
            Show
            Paul Nicholls added a comment - One thing that would be great to add would be the ability to enrol users into courses. Possible workflow: select a bunch of courses (or just one) choose to enrol users into selected course(s) choose a role (this could be a dropdown/set of radio buttons/etc on the next step) search for users to add
            Hide
            Sam Hemelryk added a comment -

            Thanks for spotting that Dan, tracked down the issue, root categories that are hidden were attempting to make a capability check on their parent (of course being root they don't have a parent).
            I've patched that now and pushed it up.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Thanks for spotting that Dan, tracked down the issue, root categories that are hidden were attempting to make a capability check on their parent (of course being root they don't have a parent). I've patched that now and pushed it up. Cheers Sam
            Hide
            Dan Poltawski added a comment -

            [Sorry I know this is alpha, but just throughout i'd report results of installing on existing site without any debugging by myself]

            Can not find data record in database table course_categories.
            
            More information about this error
            
            Debug info: SELECT id,parent FROM {course_categories} WHERE id = ?
            [array (
            0 => '0',
            )]
            Stack trace:
            line 1272 of /lib/dml/moodle_database.php: dml_missing_record_exception thrown
            line 1249 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select()
            line 5939 of /lib/accesslib.php: call to moodle_database->get_record()
            line 297 of /admin/tool/coursemanagement/locallib.php: call to context_coursecat::instance()
            line 109 of /admin/tool/coursemanagement/renderer.php: call to coursemanagement_category->can_view()
            line 61 of /admin/tool/coursemanagement/renderer.php: call to tool_coursemanagement_renderer->category_tree_listing()
            
            Show
            Dan Poltawski added a comment - [Sorry I know this is alpha, but just throughout i'd report results of installing on existing site without any debugging by myself] Can not find data record in database table course_categories. More information about this error Debug info: SELECT id,parent FROM {course_categories} WHERE id = ? [array ( 0 => '0', )] Stack trace: line 1272 of /lib/dml/moodle_database.php: dml_missing_record_exception thrown line 1249 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select() line 5939 of /lib/accesslib.php: call to moodle_database->get_record() line 297 of /admin/tool/coursemanagement/locallib.php: call to context_coursecat::instance() line 109 of /admin/tool/coursemanagement/renderer.php: call to coursemanagement_category->can_view() line 61 of /admin/tool/coursemanagement/renderer.php: call to tool_coursemanagement_renderer->category_tree_listing()
            Hide
            Sam Hemelryk added a comment -

            Hi guys,

            I've created a prototype for a tool to replace the current course and category management pages.
            https://github.com/samhemelryk/moodle-tool_coursemanagement

            This is in fact the second prototype, Martin provided excellent feedback and direction on the first prototype which has led to this hopefully more on target prototype.
            This prototype aims to reduce the course and category management down to a single separate tool that makes use of JS an AJAX enhancement to get the job done more efficiently.
            I'd hope that this tool is lining up to be considered for integration into core before the release of 2.3, however it is an admin tool and as such anyone can install and use this tool.

            What can this tool do:

            When viewing a category:

            • Expand the category tree to see subcategories. This happens via AJAX with no reloads or redirections.
            • Create a new basic course through a JS widget that gets automatically added to the stack with no reloads or redirections.

            When viewing courses in a category:

            • Use drag and drop to reorder the courses within a category. AJAX is used so that this happens immediately behind the scenes no reloads or redirections.
            • Drag a course over another category to move the course from its current category to the targeted category.
            • Click on a course to view details. Details are loaded by AJAX with no reloads or redirections.

            When viewing a courses details:

            • Perform normal actions such as editing, deleting through the existing methods.

            What stage is this tool in?
            This tool is still very much a prototype. The current code works for the features mentioned above however it is still missing security checks in several areas and needs a lot more polishing.
            You'll notice that there are no confirmations on any of the actions being performed and several areas still have only basic styling.
            Another big thing to note is that not all actions have been implemented yet. I hope to add several more possible actions when viewing courses (such as quicklinks to enrolments and roles).

            What is still to come:
            The biggest thing missing at the moment is category management. The goal is to allow the user to create, reorder, and delete categories through this interface in much the same way they currently can with courses.

            What now?
            At this point I would like to hold off on further development to give people time to have a look at this thing and see whether there is any feedback or interesting ideas about how this tool may develop.
            I'll be posting in the forums shortly about this issue in an attempt to drum up a bit of thought.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi guys, I've created a prototype for a tool to replace the current course and category management pages. https://github.com/samhemelryk/moodle-tool_coursemanagement This is in fact the second prototype, Martin provided excellent feedback and direction on the first prototype which has led to this hopefully more on target prototype. This prototype aims to reduce the course and category management down to a single separate tool that makes use of JS an AJAX enhancement to get the job done more efficiently. I'd hope that this tool is lining up to be considered for integration into core before the release of 2.3, however it is an admin tool and as such anyone can install and use this tool. What can this tool do: When viewing a category: Expand the category tree to see subcategories. This happens via AJAX with no reloads or redirections. Create a new basic course through a JS widget that gets automatically added to the stack with no reloads or redirections. When viewing courses in a category: Use drag and drop to reorder the courses within a category. AJAX is used so that this happens immediately behind the scenes no reloads or redirections. Drag a course over another category to move the course from its current category to the targeted category. Click on a course to view details. Details are loaded by AJAX with no reloads or redirections. When viewing a courses details: Perform normal actions such as editing, deleting through the existing methods. What stage is this tool in? This tool is still very much a prototype. The current code works for the features mentioned above however it is still missing security checks in several areas and needs a lot more polishing. You'll notice that there are no confirmations on any of the actions being performed and several areas still have only basic styling. Another big thing to note is that not all actions have been implemented yet. I hope to add several more possible actions when viewing courses (such as quicklinks to enrolments and roles). What is still to come: The biggest thing missing at the moment is category management. The goal is to allow the user to create, reorder, and delete categories through this interface in much the same way they currently can with courses. What now? At this point I would like to hold off on further development to give people time to have a look at this thing and see whether there is any feedback or interesting ideas about how this tool may develop. I'll be posting in the forums shortly about this issue in an attempt to drum up a bit of thought. Cheers Sam

              People

              • Votes:
                36 Vote for this issue
                Watchers:
                54 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: