Moodle
  1. Moodle
  2. MDL-33017

Reduce size of Navigation bar when in a course

    Details

    • Testing Instructions:
      Hide

      Good luck.....

      1. Log in as an admin
      2. Several up a dozen or so categories, make sure you have nested categories at least 3 levels deep.
      3. Create courses in most of the categories. Ensure there is at least one category with three courses in it.
      4. Enrol some students into courses.
      5. As an admin make sure the following settings are default: navshowcategories, navshowmycoursecategories, navshowallcourses.
      6. Set navcourselimit to 2.
      7. Log in as another user with enrolments and make sure the navigation is as you would expect.
      8. Notice that when in a course, or activity you have a current course branch.
      9. Make sure you play with AJAX loading and check things work.
      10. Log in as the admin and systematically change each of navshowcategories, navshowmycoursecategories, navshowallcourses. For each change test again and make sure the navigation behaves as you expect.
      Show
      Good luck..... Log in as an admin Several up a dozen or so categories, make sure you have nested categories at least 3 levels deep. Create courses in most of the categories. Ensure there is at least one category with three courses in it. Enrol some students into courses. As an admin make sure the following settings are default: navshowcategories, navshowmycoursecategories, navshowallcourses. Set navcourselimit to 2. Log in as another user with enrolments and make sure the navigation is as you would expect. Notice that when in a course, or activity you have a current course branch. Make sure you play with AJAX loading and check things work. Log in as the admin and systematically change each of navshowcategories, navshowmycoursecategories, navshowallcourses. For each change test again and make sure the navigation behaves as you expect.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      wip-MDL-33017-m24-r2
    • Rank:
      40193

      Description

      A number of our users have commented that if they are in many courses, then their 'My courses' list gets a bit ridiculous when they're trying to browse and edit the course.

      It would be great if both the 'Courses' and 'My courses' navigation nodes could be simplified so that if you are in a course, course module, or block context, the current course is shown in a 'Current course' navigation node. They should still appear in 'Courses' and 'My courses' but should not be expanded.

      I've had a quick go to prove the concept but I'm not very familiar with the navigation menu.

      1. currentcourse.png
        12 kB
      2. exampleissue.png
        80 kB
      3. Screenshot-2.png
        120 kB
      4. viewallcourses.png
        13 kB

        Issue Links

          Activity

          Hide
          Andrew Nicols added a comment -

          Added a screenshot demoing the look

          Show
          Andrew Nicols added a comment - Added a screenshot demoing the look
          Hide
          Andrew Nicols added a comment -

          Here's a screenshot displaying the issues that this can cause.

          In this, I'm viewing a course I'm not a member of. We have quite a few courses and to get to the settings block requires lots of scrolling.
          Moving the current course to it's own node would greatly help both in this scenario and when users are enrolled in many courses.

          Show
          Andrew Nicols added a comment - Here's a screenshot displaying the issues that this can cause. In this, I'm viewing a course I'm not a member of. We have quite a few courses and to get to the settings block requires lots of scrolling. Moving the current course to it's own node would greatly help both in this scenario and when users are enrolled in many courses.
          Hide
          Andrew Nicols added a comment -

          Sam - I'm hoping that we'll be able to find some time to work on this soon - do you have any pointers for how best to run this one?

          I know you said in MDL-32001 that you were planning to overhaul some of the code relating to this so I've been waiting for this to land. Any idea where this is on your schedule?

          Cheers,

          Andrew

          Show
          Andrew Nicols added a comment - Sam - I'm hoping that we'll be able to find some time to work on this soon - do you have any pointers for how best to run this one? I know you said in MDL-32001 that you were planning to overhaul some of the code relating to this so I've been waiting for this to land. Any idea where this is on your schedule? Cheers, Andrew
          Hide
          Andrew Nicols added a comment -

          Sam,

          This is the code I have so far. The main issue I'm seeing so far and haven't been able to find a solution for is that the Course is still shown in the Courses/My courses tree, and as a result it's still expanded. I guess the ideal would be to remove the current course from both Courses/My courses but I haven't been able to work out how to achieve this.

          Show
          Andrew Nicols added a comment - Sam, This is the code I have so far. The main issue I'm seeing so far and haven't been able to find a solution for is that the Course is still shown in the Courses/My courses tree, and as a result it's still expanded. I guess the ideal would be to remove the current course from both Courses/My courses but I haven't been able to work out how to achieve this.
          Hide
          Andrew Nicols added a comment -

          Rewritten with smaller diff

          Show
          Andrew Nicols added a comment - Rewritten with smaller diff
          Hide
          Sam Hemelryk added a comment -

          Sorry about the delay Andrew, but as promised first on my list for today

          Show
          Sam Hemelryk added a comment - Sorry about the delay Andrew, but as promised first on my list for today
          Hide
          Sam Hemelryk added a comment -

          Hi Andrew,

          Okay, so I've looked it over, had a quick play, and have had a good thing about.

          First up. I really like the idea of separating out the current course.
          Certainly there is always talk of how to simplify or shrink the navigation block and I think many people will welcome such a change.

          The code looks good, theres a couple of things I want to discuss (just below this) however I'm sure if we decide to run with this idea that the code you've got there will require few changes in order to be integrated.

          Noting there is one quirk I observed with this code, if you enable the navshowmycoursecategories things get a little crazy.
          This sort of leads onto what I would like to discuss.

          This change leads to the following:

          • Current course being separated out in the navigation. Great it simplifies navigation.
          • Current course being removed from the My courses / Courses branch. Not great this leads to inconsistent navigation.
          • Navigation bar changes to Home > Current course > Course name. Not great, its not reflective of the path to the course.

          I think simplification in the navigation is perfect, however the current path has the side effect of leading to inconsistent navigation and nav bar that doesn't make as much sense.
          I really like the concept of the current course branch, but I think we need to find a way to make it work in such a way that the course is available in a consistent way (always the same branch) in the navigation, and find some way to preserve the navbar to what it already is.

          I had a bit of a think about this and a play, one alternative would be to load the current course branch as part of the navigation block rather than create it within the navigation structure (really add it to output rather than to structure).
          Have a look at https://github.com/samhemelryk/moodle/compare/master...wip-MDL-33017-m24
          Its a little long, but that is because I added several useful things to the navigation API and fixed a bug in the navigation_node_collection class at the same time.

          Thats just one idea, I have particular reason to move it to output rather than structure, but perhaps one advantage is that this way you could make it a navigation block setting and only generate the current course branch if the site has enabled it. That way those who were happy with the navigation as it is would not have to change anything.

          Anyway, back to you Andrew, what do you think?

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, Okay, so I've looked it over, had a quick play, and have had a good thing about. First up. I really like the idea of separating out the current course. Certainly there is always talk of how to simplify or shrink the navigation block and I think many people will welcome such a change. The code looks good, theres a couple of things I want to discuss (just below this) however I'm sure if we decide to run with this idea that the code you've got there will require few changes in order to be integrated. Noting there is one quirk I observed with this code, if you enable the navshowmycoursecategories things get a little crazy. This sort of leads onto what I would like to discuss. This change leads to the following: Current course being separated out in the navigation. Great it simplifies navigation. Current course being removed from the My courses / Courses branch. Not great this leads to inconsistent navigation. Navigation bar changes to Home > Current course > Course name . Not great, its not reflective of the path to the course. I think simplification in the navigation is perfect, however the current path has the side effect of leading to inconsistent navigation and nav bar that doesn't make as much sense. I really like the concept of the current course branch, but I think we need to find a way to make it work in such a way that the course is available in a consistent way (always the same branch) in the navigation, and find some way to preserve the navbar to what it already is. I had a bit of a think about this and a play, one alternative would be to load the current course branch as part of the navigation block rather than create it within the navigation structure (really add it to output rather than to structure). Have a look at https://github.com/samhemelryk/moodle/compare/master...wip-MDL-33017-m24 Its a little long, but that is because I added several useful things to the navigation API and fixed a bug in the navigation_node_collection class at the same time. Thats just one idea, I have particular reason to move it to output rather than structure, but perhaps one advantage is that this way you could make it a navigation block setting and only generate the current course branch if the site has enabled it. That way those who were happy with the navigation as it is would not have to change anything. Anyway, back to you Andrew, what do you think? Cheers Sam
          Hide
          Andrew Nicols added a comment -

          Hi Sam,

          Thank you for the detailed review - very much appreciated.

          It would be really good to reduce the size of the navigation block and I hope that this will provide a way of doing so. I also like the potential ability to make this an administrator preference as I'm sure that there's bound to be someone who doesn't like the idea out there so your way seems the better way of doing so.

          I agree - the course should be available in a consistent fashion and should still appear in either the My courses, or the Courses branch - this is something I really struggled with as I'm still struggling to get to grips with the navigation code.

          In addition though, I think that the breadcrumbs should still point to the My courses/Courses branch rather than Current course. I've not been able to work out how to do this so far as the find_active_node() function returns the first node, which will always be the Current course branch. I don't think that this is a show stopper though.

          What's the best way to proceed with this? Do you want to take this on, or should I? I guess the bugfixes and new API calls should go into separate bugs really...

          Thanks again,

          Andrew

          Show
          Andrew Nicols added a comment - Hi Sam, Thank you for the detailed review - very much appreciated. It would be really good to reduce the size of the navigation block and I hope that this will provide a way of doing so. I also like the potential ability to make this an administrator preference as I'm sure that there's bound to be someone who doesn't like the idea out there so your way seems the better way of doing so. I agree - the course should be available in a consistent fashion and should still appear in either the My courses, or the Courses branch - this is something I really struggled with as I'm still struggling to get to grips with the navigation code. In addition though, I think that the breadcrumbs should still point to the My courses/Courses branch rather than Current course. I've not been able to work out how to do this so far as the find_active_node() function returns the first node, which will always be the Current course branch. I don't think that this is a show stopper though. What's the best way to proceed with this? Do you want to take this on, or should I? I guess the bugfixes and new API calls should go into separate bugs really... Thanks again, Andrew
          Hide
          Andrew Nicols added a comment - - edited

          As discussed, I've rebased on latest integration/master and adjusted comments as per code standards.

          Still need to:

          • remove the course shortname from beneath 'Current course'
          • ensure that the breadcrumbs appear consistently
          • only generate the content of 'My courses' when expanding the course list
          Show
          Andrew Nicols added a comment - - edited As discussed, I've rebased on latest integration/master and adjusted comments as per code standards. Still need to: remove the course shortname from beneath 'Current course' ensure that the breadcrumbs appear consistently only generate the content of 'My courses' when expanding the course list
          Hide
          Petr Škoda added a comment -

          Sam: here is the patch for quick lookup if user sees at least on course: https://github.com/skodak/moodle/compare/master...w43_MDL-33017_m24_hascourses

          Feel free to change the function name, I could not find anything better...

          Show
          Petr Škoda added a comment - Sam: here is the patch for quick lookup if user sees at least on course: https://github.com/skodak/moodle/compare/master...w43_MDL-33017_m24_hascourses Feel free to change the function name, I could not find anything better...
          Hide
          Petr Škoda added a comment -

          Hi, what is the progress here? I think we should consider this for 2.4.

          Show
          Petr Škoda added a comment - Hi, what is the progress here? I think we should consider this for 2.4.
          Hide
          Sam Hemelryk added a comment -

          Ok I think I've got this is a usable state now. After much stripping, debugging and testing!

          It now uses the current course and AJAX loads the courses and my courses branches.
          This allows us to cut out quite a bit, it should hopefully be a good performance win.
          I'll run performance tests as soon as I can and try to get +1's from where ever I can for 2.4. It'd definitely be worth landing to 2.4 if we can verify it doesn't break anything.

          Also worth noting is that there are 3 contributors here.
          Andrew - The Current Course change.
          Petr - The required enhancement of the enrolment API.
          Myself - The navigation stripping and clean up.

          Many thanks everyone
          Sam

          p.s. Petr you were down as peer-reviewer, I removed you just in case you arn't around or don't have time.

          Show
          Sam Hemelryk added a comment - Ok I think I've got this is a usable state now. After much stripping, debugging and testing! It now uses the current course and AJAX loads the courses and my courses branches. This allows us to cut out quite a bit, it should hopefully be a good performance win. I'll run performance tests as soon as I can and try to get +1's from where ever I can for 2.4. It'd definitely be worth landing to 2.4 if we can verify it doesn't break anything. Also worth noting is that there are 3 contributors here. Andrew - The Current Course change. Petr - The required enhancement of the enrolment API. Myself - The navigation stripping and clean up. Many thanks everyone Sam p.s. Petr you were down as peer-reviewer, I removed you just in case you arn't around or don't have time.
          Hide
          Sam Hemelryk added a comment -

          Also noting I will add test instructions when this is put up for peer-review. They will be rather large and in depth.

          Show
          Sam Hemelryk added a comment - Also noting I will add test instructions when this is put up for peer-review. They will be rather large and in depth.
          Hide
          Sam Hemelryk added a comment -

          Martin, I've added you as a watcher here.
          These are the first of the navigation changes requested during the hackfest.
          Implementing the current course change and the navigation simplification. It's a good performance win.
          Would love for you to have a quick play with this and just check you're happy with it.

          Show
          Sam Hemelryk added a comment - Martin, I've added you as a watcher here. These are the first of the navigation changes requested during the hackfest. Implementing the current course change and the navigation simplification. It's a good performance win. Would love for you to have a quick play with this and just check you're happy with it.
          Hide
          Petr Škoda added a comment -

          If this gets accepted we should IMO do the same Ajax loading trick for the admin settings - it would help admins and managers only though.

          Show
          Petr Škoda added a comment - If this gets accepted we should IMO do the same Ajax loading trick for the admin settings - it would help admins and managers only though.
          Hide
          Sam Hemelryk added a comment - - edited

          Performance comparison of master vs wip-MDL-33017-m24-r2: http://moodev.com/?w=300&h=150&before=MDL-33017-master-01&after=MDL-33017-02#statsarray
          Run:

          • 15 pages per run
          • 50 users per loop
          • Repeated 5 times
          • Throughput of 220 page requests per second
          • Ramp up period of 2 minutes.
          • Dedicated server running PHP 5.4.5 + PgSQL 9.0.7
          Show
          Sam Hemelryk added a comment - - edited Performance comparison of master vs wip- MDL-33017 -m24-r2: http://moodev.com/?w=300&h=150&before=MDL-33017-master-01&after=MDL-33017-02#statsarray Run: 15 pages per run 50 users per loop Repeated 5 times Throughput of 220 page requests per second Ramp up period of 2 minutes. Dedicated server running PHP 5.4.5 + PgSQL 9.0.7
          Hide
          Martin Dougiamas added a comment -

          One thing really concerns me from those stats:

          Logout dbwrites: +2800%

          Any idea what that is?

          Show
          Martin Dougiamas added a comment - One thing really concerns me from those stats: Logout dbwrites: +2800% Any idea what that is?
          Hide
          Martin Dougiamas added a comment -

          I've tried your branch and explored as much as I could. I think it's a good change, though we'll need to advertise about it widely as it will possibly impact some people (who do Moodle training) a little. My +5 for it.

          The only problem I found on your branch was that there were no blocks at all on the user course profile page (eg /user/view.php?id=2&course=2). No idea if that's connected but it's not a bug on master, so maybe.

          Show
          Martin Dougiamas added a comment - I've tried your branch and explored as much as I could. I think it's a good change, though we'll need to advertise about it widely as it will possibly impact some people (who do Moodle training) a little. My +5 for it. The only problem I found on your branch was that there were no blocks at all on the user course profile page (eg /user/view.php?id=2&course=2). No idea if that's connected but it's not a bug on master, so maybe.
          Hide
          Sam Hemelryk added a comment -

          Hehe those percentages are horribly unrepresentative sometimes sorry.
          In the case of the logout page typically there are 0 writes on logout, however 1 in 100 requests triggers something that is causing a several writes to the database in a single request, perhaps gc or something. In the before run this happened 1 in the 250 request (0.01 average writes per request), in the patched run it happened three times (0.29 average writes per request)
          The reality of that +2800% is just my shitty tool misrepresenting chance.

          The missing blocks, I'll have a look at that, I can't think what in this patch would effect that. Normally that is a page layout thing.

          Show
          Sam Hemelryk added a comment - Hehe those percentages are horribly unrepresentative sometimes sorry. In the case of the logout page typically there are 0 writes on logout, however 1 in 100 requests triggers something that is causing a several writes to the database in a single request, perhaps gc or something. In the before run this happened 1 in the 250 request (0.01 average writes per request), in the patched run it happened three times (0.29 average writes per request) The reality of that +2800% is just my shitty tool misrepresenting chance. The missing blocks, I'll have a look at that, I can't think what in this patch would effect that. Normally that is a page layout thing.
          Hide
          Martin Dougiamas added a comment -

          We worked out the user page thing is unrelated.

          Andrew if you have a few minutes to at least do a sanity check on Sam's code for him then I reckon we should commence the integrator bribing.

          Show
          Martin Dougiamas added a comment - We worked out the user page thing is unrelated. Andrew if you have a few minutes to at least do a sanity check on Sam's code for him then I reckon we should commence the integrator bribing.
          Hide
          Andrew Nicols added a comment -

          I've had a look at the changes and, sanity-wise, everything looks fine and should simplify things massively - it's a sanity improvement and makes the navigation code much easier to read.

          A few other trivial comments:

          blocks/navigation/yui/navigation/navigation.js

          • comparisons in JS should really use type eqaulity comparisons (===)
          • can we stop using hard-coded integers for the types and switch to variables - it would make the code much easier to read and understand
          if ((this.get('type') === TYPE.CATEGORY || this.get('type') === TYPE.ROOTNODE) && coursecount >= M.block_navigation.courselimit) {
          }
          

          lib/ajax/getnavbranch.php

          • A comment explaining why branchid accepts ALPHANUM would be great - it makes sense but you reading the patch to understand could cause issues in the future

          This change definitely gets my +1. I know that Dan likes beer and curry if that helps with the bribing...

          Show
          Andrew Nicols added a comment - I've had a look at the changes and, sanity-wise, everything looks fine and should simplify things massively - it's a sanity improvement and makes the navigation code much easier to read. A few other trivial comments: blocks/navigation/yui/navigation/navigation.js comparisons in JS should really use type eqaulity comparisons (===) can we stop using hard-coded integers for the types and switch to variables - it would make the code much easier to read and understand if (( this .get('type') === TYPE.CATEGORY || this .get('type') === TYPE.ROOTNODE) && coursecount >= M.block_navigation.courselimit) { } lib/ajax/getnavbranch.php A comment explaining why branchid accepts ALPHANUM would be great - it makes sense but you reading the patch to understand could cause issues in the future This change definitely gets my +1. I know that Dan likes beer and curry if that helps with the bribing...
          Hide
          Martin Dougiamas added a comment -

          Sam can you polish and start bribing?

          Show
          Martin Dougiamas added a comment - Sam can you polish and start bribing?
          Hide
          Sam Hemelryk added a comment -

          This has been polished now (thanks for looking over it Andrew) and is ready for integration review.

          I've added the integrators as watchers now and will ping in the integration chat as well.

          Many thanks
          sAm

          Show
          Sam Hemelryk added a comment - This has been polished now (thanks for looking over it Andrew) and is ready for integration review. I've added the integrators as watchers now and will ping in the integration chat as well. Many thanks sAm
          Hide
          Eloy Lafuente (stronk7) added a comment -

          TesTinG InStRuCtIoNs MiSinG, PlEaSe...

          Show
          Eloy Lafuente (stronk7) added a comment - TesTinG InStRuCtIoNs MiSinG, PlEaSe...
          Hide
          Sam Hemelryk added a comment -

          Testing instructions up

          Show
          Sam Hemelryk added a comment - Testing instructions up
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Super! +1

          Show
          Eloy Lafuente (stronk7) added a comment - Super! +1
          Hide
          Dan Poltawski added a comment -

          Hi Sam,

          I've just two minor comments:

          Would lib/ajax/getnavbranch.php:

           if ($branchtype != 0) {
          

          be clearer as:

            if ($branchtype != navigation_node::COURSE_OTHER) {
          

          I'm not a huge fan of the function name current_user_is_parent_role(), because its sort of like a hardcoded 'role concept' and when we're working in tree structures, talk of parents could be confusing. But, I don't really have a better, simple to understand alternative to offer, so.. ignore that comment

          Show
          Dan Poltawski added a comment - Hi Sam, I've just two minor comments: Would lib/ajax/getnavbranch.php: if ($branchtype != 0) { be clearer as: if ($branchtype != navigation_node::COURSE_OTHER) { I'm not a huge fan of the function name current_user_is_parent_role() , because its sort of like a hardcoded 'role concept' and when we're working in tree structures, talk of parents could be confusing. But, I don't really have a better, simple to understand alternative to offer, so.. ignore that comment
          Hide
          Sam Hemelryk added a comment -

          Hi Dan,

          The first point I don't think would probably work. It'd make it more readable in that we don't have an int just sitting there, but essentially that check is checking that we don't have a valid id, and has nothing to do with the intention that COURSE_OTHER was created with.
          Re: the second point. Absolutely agree. The whole parent thing is a nasty as hack. We had a big discussion about it when it was implemented but just couldn't come up with a better way of handling it. Hehe so while the outcome is perhaps ignored I 110% agree that it is just plain not great.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Dan, The first point I don't think would probably work. It'd make it more readable in that we don't have an int just sitting there, but essentially that check is checking that we don't have a valid id, and has nothing to do with the intention that COURSE_OTHER was created with. Re: the second point. Absolutely agree. The whole parent thing is a nasty as hack. We had a big discussion about it when it was implemented but just couldn't come up with a better way of handling it. Hehe so while the outcome is perhaps ignored I 110% agree that it is just plain not great. Many thanks Sam
          Hide
          Martin Dougiamas added a comment -

          The neutral way we talked about "parents" in code originally was to use mentor/mentee.

          Show
          Martin Dougiamas added a comment - The neutral way we talked about "parents" in code originally was to use mentor/mentee.
          Hide
          Dan Poltawski added a comment -

          Thanks Sam. I've integrated this now!

          Show
          Dan Poltawski added a comment - Thanks Sam. I've integrated this now!
          Hide
          Andrew Davis added a comment -

          I've spotted one oddity. Im not sure whether it should hold up this issue or if I should just open a new one.

          If you expand my courses and click on "view all courses" you're taken to /my. On the page you're taken to "my home" is highlighted in the navigation and the "my courses" navigation branch is collapsed. It makes it feel like your place has been lost.

          Also, is it possible to disable My Moodle anymore? I can't find the option anymore but you certainly used to be able to turn it off.

          Show
          Andrew Davis added a comment - I've spotted one oddity. Im not sure whether it should hold up this issue or if I should just open a new one. If you expand my courses and click on "view all courses" you're taken to /my. On the page you're taken to "my home" is highlighted in the navigation and the "my courses" navigation branch is collapsed. It makes it feel like your place has been lost. Also, is it possible to disable My Moodle anymore? I can't find the option anymore but you certainly used to be able to turn it off.
          Hide
          Andrew Davis added a comment - - edited

          Attaching a screenshot. See https://tracker.moodle.org/secure/attachment/29982/Screenshot-2.png

          Two things to note:

          1) the current course appears twice in the navigation. Very minor.

          2) the sections don't appear under "current course" initially at least. Note that I expanded "my courses" and went into a section. When the page reloaded I suddenly had sections under both "current course" and "my courses".

          Show
          Andrew Davis added a comment - - edited Attaching a screenshot. See https://tracker.moodle.org/secure/attachment/29982/Screenshot-2.png Two things to note: 1) the current course appears twice in the navigation. Very minor. 2) the sections don't appear under "current course" initially at least. Note that I expanded "my courses" and went into a section. When the page reloaded I suddenly had sections under both "current course" and "my courses".
          Hide
          Andrew Davis added a comment - - edited

          Unless I'm missing something "navshowcategories" has no effect. There are never categories visible to either a student or admin in the navigation block. If you go into a course the navigation bar says Home > (course short name) regardless of the setting.

          Update: It appears that the admin needs to turn on "navshowallcourses" for "navshowcategories" to have an effect. Assuming this is correct the description of "navshowcategories" should make mention of this.

          Show
          Andrew Davis added a comment - - edited Unless I'm missing something "navshowcategories" has no effect. There are never categories visible to either a student or admin in the navigation block. If you go into a course the navigation bar says Home > (course short name) regardless of the setting. Update: It appears that the admin needs to turn on "navshowallcourses" for "navshowcategories" to have an effect. Assuming this is correct the description of "navshowcategories" should make mention of this.
          Hide
          Andrew Davis added a comment - - edited

          With "navshowmycoursecategories" enabled the "View all courses" link is put in the wrong spot. See https://tracker.moodle.org/secure/attachment/29987/viewallcourses.png

          I have two top level categories. "top category" and "Miscellaneous".

          Show
          Andrew Davis added a comment - - edited With "navshowmycoursecategories" enabled the "View all courses" link is put in the wrong spot. See https://tracker.moodle.org/secure/attachment/29987/viewallcourses.png I have two top level categories. "top category" and "Miscellaneous".
          Hide
          Andrew Davis added a comment -

          I'm failing this as it seems to need more work.

          Show
          Andrew Davis added a comment - I'm failing this as it seems to need more work.
          Hide
          Andrew Davis added a comment -

          Some of these issues may actually be due to MDL-35279.

          Show
          Andrew Davis added a comment - Some of these issues may actually be due to MDL-35279 .
          Hide
          Sam Hemelryk added a comment -

          Thanks for reporting back on things Andrew, I've been looking through them now.

          To summarise the points you've raised:
          1/ Clicking on Navigation > My courses > View all courses takes you to yoursite.com/my and the navigation highlights both My Home and My Courses but My courses hasn't been opened.
          2/ Current course appears twice in the navigation. First under current course and second under its original location.
          3/ Sections don't appear under the current course initially. Note that I expanded "my courses" and went into a section. When the page reloaded I suddenly had sections under both "current course" and "my courses".
          4/ Unless I'm missing something "navshowcategories" has no effect. There are never categories visible to either a student or admin in the navigation block. If you go into a course the navigation bar says Home > (course short name) regardless of the setting. Update: It appears that the admin needs to turn on "navshowallcourses" for "navshowcategories" to have an effect. Assuming this is correct the description of "navshowcategories" should make mention of this.
          5/ With "navshowmycoursecategories" enabled the "View all courses" link is put in the wrong spot. See https://tracker.moodle.org/secure/attachment/29987/viewallcourses.png

          In reply:
          1/ Definitely something that could be done to improve usability.
          2/ This one is intentional and something that we've done in other places. The logic behind it is that the course is both the current course and one of the users "My courses". In order to keep the navigation consistent it must always exist under "My courses" so that they can always find it under there (even if they happen to be in it already).
          3/ For the life of me I've been unable to replicate this. I've tried to reproduce what I can see in the screenshot but with no luck. Can you see if you can still reproduce this and if so detail the steps your went through.
          4/ Again I couldn't replicate this sorry Andrew. With navshowcategories on and navshowallcourses off I still get categories under the Courses branch when not loggen in or loggen in as a user with no enrolments. Users with enrolments dont' see the courses branch unless navshowallcourses is on.
          Any chance you can see if you can still reproduce this, and if so provide the steps etc.
          5/ This is [presently] intentional. The `View all courses` link is for the category, and the URL should be taking you to view the category. This is something that hasn't changed in this patch. Perhaps in thinking about it it would make better sense to show a single 'View all courses' link at the bottom of the My courses branch at all times. Is that what you were expecting here?

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Thanks for reporting back on things Andrew, I've been looking through them now. To summarise the points you've raised: 1/ Clicking on Navigation > My courses > View all courses takes you to yoursite.com/my and the navigation highlights both My Home and My Courses but My courses hasn't been opened. 2/ Current course appears twice in the navigation. First under current course and second under its original location. 3/ Sections don't appear under the current course initially. Note that I expanded "my courses" and went into a section. When the page reloaded I suddenly had sections under both "current course" and "my courses". 4/ Unless I'm missing something "navshowcategories" has no effect. There are never categories visible to either a student or admin in the navigation block. If you go into a course the navigation bar says Home > (course short name) regardless of the setting. Update: It appears that the admin needs to turn on "navshowallcourses" for "navshowcategories" to have an effect. Assuming this is correct the description of "navshowcategories" should make mention of this. 5/ With "navshowmycoursecategories" enabled the "View all courses" link is put in the wrong spot. See https://tracker.moodle.org/secure/attachment/29987/viewallcourses.png In reply: 1/ Definitely something that could be done to improve usability. 2/ This one is intentional and something that we've done in other places. The logic behind it is that the course is both the current course and one of the users "My courses". In order to keep the navigation consistent it must always exist under "My courses" so that they can always find it under there (even if they happen to be in it already). 3/ For the life of me I've been unable to replicate this. I've tried to reproduce what I can see in the screenshot but with no luck. Can you see if you can still reproduce this and if so detail the steps your went through. 4/ Again I couldn't replicate this sorry Andrew. With navshowcategories on and navshowallcourses off I still get categories under the Courses branch when not loggen in or loggen in as a user with no enrolments. Users with enrolments dont' see the courses branch unless navshowallcourses is on. Any chance you can see if you can still reproduce this, and if so provide the steps etc. 5/ This is [presently] intentional. The `View all courses` link is for the category, and the URL should be taking you to view the category. This is something that hasn't changed in this patch. Perhaps in thinking about it it would make better sense to show a single 'View all courses' link at the bottom of the My courses branch at all times. Is that what you were expecting here? Many thanks Sam
          Hide
          Dan Poltawski added a comment -

          With the absence of Andrew, we're gonna need to make a decision on this. Seemingly most of the issues are as is expected, so lets leave this to QA to pick up

          (Andrew: not ignoring the isuses you've raised, particularly we could do with being able to reproduce that error).

          Show
          Dan Poltawski added a comment - With the absence of Andrew, we're gonna need to make a decision on this. Seemingly most of the issues are as is expected, so lets leave this to QA to pick up (Andrew: not ignoring the isuses you've raised, particularly we could do with being able to reproduce that error).
          Hide
          Dan Poltawski added a comment -

          Passing based on what we haave to morve this forward.

          Show
          Dan Poltawski added a comment - Passing based on what we haave to morve this forward.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Amazed. Inspired. Grateful. That’s how your generosity makes me feel.

          (not really)

          Closing, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Amazed. Inspired. Grateful. That’s how your generosity makes me feel. (not really) Closing, thanks!
          Hide
          Mary Cooch added a comment -

          Removing docs_required label as this is now documented here http://docs.moodle.org/24/en/Navigation_block

          Show
          Mary Cooch added a comment - Removing docs_required label as this is now documented here http://docs.moodle.org/24/en/Navigation_block

            People

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

              Dates

              • Created:
                Updated:
                Resolved: