Moodle
  1. Moodle
  2. MDL-40297

Empty Categories in My Courses are always shown.

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4, 2.5
    • Fix Version/s: 2.4.5, 2.5.1
    • Component/s: Navigation
    • Labels:
    • Testing Instructions:
      Hide

      Create these categories and courses:

      • cat1, with course c1
      • cat2, with course c2
      • cat3, without courses and with subcategories:
        • cat31, with course c31
        • cat32, with course c32
        • cat33, with courses c331 and c332

      Enrol a user (as student) in all the "odd" courses (those which name ends with "1", aka c1, c31 and c331). Log in as that user.

      In the navigation block, my courses:

      • With $CFG->navshowmycoursecategories disabled you get the raw list of courses in the block (c1, c31 and c331).
      • With $CFG->navshowmycoursecategories enabled, expand the whole tree, you get the categories tree and only the enrolled courses (c1, c31 and c331).
      • Both the category cat2 and and cat32 aren't shown in the tree as far as the user does not have enrolled courses there (before the patch c32 was shown in the tree).

      Optionally, if MDL-40392 is already integrated, run the behat tests specified there, they should pass (2 scenarios).

      Show
      Create these categories and courses: cat1, with course c1 cat2, with course c2 cat3, without courses and with subcategories: cat31, with course c31 cat32, with course c32 cat33, with courses c331 and c332 Enrol a user (as student) in all the "odd" courses (those which name ends with "1", aka c1, c31 and c331). Log in as that user. In the navigation block, my courses: With $CFG->navshowmycoursecategories disabled you get the raw list of courses in the block (c1, c31 and c331). With $CFG->navshowmycoursecategories enabled, expand the whole tree, you get the categories tree and only the enrolled courses (c1, c31 and c331). Both the category cat2 and and cat32 aren't shown in the tree as far as the user does not have enrolled courses there (before the patch c32 was shown in the tree). Optionally, if MDL-40392 is already integrated, run the behat tests specified there, they should pass (2 scenarios).
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
    • Pull 2.5 Branch:
    • Pull Master Branch:
    • Rank:
      51069

      Description

      This issue is closely related to MDL-38631 and assumes the changes from MDL-38631 were integrated.

      All categories in My Courses are shown, even if a user does not have any courses in them.

      Steps to reproduce:

      1. Create multiple categories with courses
      2. Enlist a user into a course in of single category
      3. In My Courses every available category is shown, but when you click on a category where you don't have any courses it is simply empty.

      In my oppinion, showing empty categories serves no purpose at all. I've implemented a sample fix for this issue, it can be found in repo git://129.187.81.134/moodle/moodle.git in branch MDL-38631-hide_empty . This fix is used by us (TU München) since March and we did not find any issues with it.

        Issue Links

          Activity

          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Hi Dmytro,

          I've assumed that these THREE commits from your repo were the ones needed to implement this:

          • 58c460a881224dd9316ee10252e90659d7a937a8
          • d622560ae3d165662d13f66575f68351a9680b51
          • 33e4ce42794b3cea55b097d939152a4dc4701d5f

          So I've squashed them into an unique commit (as far as they were iterations over the same code) and also have added an extra commit fixing some comments and illegal whitespace.

          The results are available @:

          https://github.com/stronk7/moodle/compare/master...MDL-40297

          So, I'm sending this to peer review. In the mean time... could you please add some testing instructions to this? Perhaps the case explained @ MDL-38631 is enough or perhaps it's interesting to add some more levels... for your consideration.

          Edited, I forgot it, MANY thanks for your collaboration in these issues!

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Hi Dmytro, I've assumed that these THREE commits from your repo were the ones needed to implement this: 58c460a881224dd9316ee10252e90659d7a937a8 d622560ae3d165662d13f66575f68351a9680b51 33e4ce42794b3cea55b097d939152a4dc4701d5f So I've squashed them into an unique commit (as far as they were iterations over the same code) and also have added an extra commit fixing some comments and illegal whitespace. The results are available @: https://github.com/stronk7/moodle/compare/master...MDL-40297 So, I'm sending this to peer review. In the mean time... could you please add some testing instructions to this? Perhaps the case explained @ MDL-38631 is enough or perhaps it's interesting to add some more levels... for your consideration. Edited, I forgot it, MANY thanks for your collaboration in these issues!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          To integrators: this cherry picks without conflicts both to 25 and 24 stables (where MDL-38631 was fixed).

          Show
          Eloy Lafuente (stronk7) added a comment - To integrators: this cherry picks without conflicts both to 25 and 24 stables (where MDL-38631 was fixed).
          Hide
          Dmytro Vorona added a comment -

          Hello Eloy,

          yes, these are the right commits. We'll think about some additional tests and post them here.

          Regards,

          Dimitri.

          Show
          Dmytro Vorona added a comment - Hello Eloy, yes, these are the right commits. We'll think about some additional tests and post them here. Regards, Dimitri.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          Changes look great thanks. Only thing I noted was the new variable names containing underscores which is against our coding style guidelines.
          Please fix them up and get this up for integration review.
          If you wish I can create cleaned up branches for you.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, Changes look great thanks. Only thing I noted was the new variable names containing underscores which is against our coding style guidelines. Please fix them up and get this up for integration review. If you wish I can create cleaned up branches for you. Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          Noting I found a bug while reviewing this MDL-40344

          Show
          Sam Hemelryk added a comment - Noting I found a bug while reviewing this MDL-40344
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Branch updated with variable names cleanup, thanks!

          Once we roll current weeklies I'll generate the rebased 24 & 25 branches. And, with a good testing case coming from Dimitri... this will be sent to integration.

          Show
          Eloy Lafuente (stronk7) added a comment - Branch updated with variable names cleanup, thanks! Once we roll current weeklies I'll generate the rebased 24 & 25 branches. And, with a good testing case coming from Dimitri... this will be sent to integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Added 24_STABLE and 25 STABLE branches.

          Show
          Eloy Lafuente (stronk7) added a comment - Added 24_STABLE and 25 STABLE branches.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Added testing instructions with the simple case I used @ MDL-38631. Feel free to complete if there is something more complex to verify. Sending to integration.

          Show
          Eloy Lafuente (stronk7) added a comment - Added testing instructions with the simple case I used @ MDL-38631 . Feel free to complete if there is something more complex to verify. Sending to integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I've also created MDL-40392 that covers this with behat tests.

          Show
          Eloy Lafuente (stronk7) added a comment - I've also created MDL-40392 that covers this with behat tests.
          Hide
          Dan Poltawski added a comment -

          Integrated to master, 25 and 24 - thanks!

          Show
          Dan Poltawski added a comment - Integrated to master, 25 and 24 - thanks!
          Hide
          Dmytro Vorona added a comment -

          Hello Eloy,

          do you still require additional test instructions?

          Dimitri.

          Show
          Dmytro Vorona added a comment - Hello Eloy, do you still require additional test instructions? Dimitri.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Dimitri,

          If you think the case used covers the patch completely then no more tests are needed. Only if there is some other combination interesting to verify... feel free to do so.

          Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Dimitri, If you think the case used covers the patch completely then no more tests are needed. Only if there is some other combination interesting to verify... feel free to do so. Thanks!
          Hide
          Ankit Agarwal added a comment -

          Skipping behat tests as MDL-40392 is not integrated yet

          Show
          Ankit Agarwal added a comment - Skipping behat tests as MDL-40392 is not integrated yet
          Hide
          Ankit Agarwal added a comment -

          tested on 24,25, master
          Works as described.
          Thanks

          Show
          Ankit Agarwal added a comment - tested on 24,25, master Works as described. Thanks
          Hide
          Damyon Wiese added a comment -

          This issue is fixed! Hurray! Hurray!
          Your issue is fixed, what a wonderful day!

          Cheers, Damyon

          Show
          Damyon Wiese added a comment - This issue is fixed! Hurray! Hurray! Your issue is fixed, what a wonderful day! Cheers, Damyon

            People

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

              Dates

              • Created:
                Updated:
                Resolved: