Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-38887

AJAX loaded navigation tree does not show hidden branches as hidden

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.3, 2.5
    • Fix Version/s: 2.4.4
    • Component/s: Navigation
    • Labels:
    • Testing Instructions:
      Hide
      1. Login as admin (to make sure you have permission to see all courses and hidden sections)
      2. Create a course in topics format with at least two sections, all sections displayed on the course page
      3. Make one section hidden (i.e. "Topic 2")
      4. Go to home page
      5. Expand navigation menu by clicking on arrow to load it via AJAX requests: click 'Courses' and then course name
      6. Make sure that Topic 1 has black color and Topic 2 has gray.
      Show
      Login as admin (to make sure you have permission to see all courses and hidden sections) Create a course in topics format with at least two sections, all sections displayed on the course page Make one section hidden (i.e. "Topic 2") Go to home page Expand navigation menu by clicking on arrow to load it via AJAX requests: click 'Courses' and then course name Make sure that Topic 1 has black color and Topic 2 has gray.
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      wip-MDL-38887-m25

      Description

      1. Login as admin (to make sure you have permission to see all courses and hidden sections)
      2. Create a course in topics format with at least two sections, all sections displayed on the course page
      3. Make one section hidden (i.e. "Topic 2")
      4. Note that in navigation menu Topic 1 has black color and Topic 2 has gray.
      5. Go to home page
      6. Expand navigation menu by clicking on arrow to load it via AJAX requests: click 'Courses' and then course name
      7. Note that both Topic 1 and Topic 2 are displayed black

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            marina Marina Glancy added a comment - - edited

            this fix does not make the whole code better, HTML is formed completely different when expanded in AJAX or when displayed on page

            Show
            marina Marina Glancy added a comment - - edited this fix does not make the whole code better, HTML is formed completely different when expanded in AJAX or when displayed on page
            Hide
            marina Marina Glancy added a comment -

            Andrew, I ask you to peer review. This is crazy - there are 4 JS YUI files in master - what am I supposed to do with them?

            Show
            marina Marina Glancy added a comment - Andrew, I ask you to peer review. This is crazy - there are 4 JS YUI files in master - what am I supposed to do with them?
            Hide
            marina Marina Glancy added a comment -

            I run shifter. This is crazy, I really think the build files should be ignored by git and rebuilt during integration. The diff is unreadable.

            Show
            marina Marina Glancy added a comment - I run shifter. This is crazy, I really think the build files should be ignored by git and rebuilt during integration. The diff is unreadable.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            [?] Syntax
            [Y] Output
            [Y] Whitespace
            [-] Language
            [-] Databases
            [Y] Testing
            [-] Security
            [-] Documentation
            [Y] Git
            [Y] Sanity check

            Syntax:
            You're using branchspan.append(this.get('name')) to append the name of the navigation node? The docs for Node.append() have a note about using Y.Escape.html() to escape the HTML content before appending. http://yuilibrary.com/yui/docs/api/classes/Node.html#method_append

            In this case I don't think that this is needed here, but if you could just check.

            Show
            dobedobedoh Andrew Nicols added a comment - [?] Syntax [Y] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check Syntax: You're using branchspan.append(this.get('name')) to append the name of the navigation node? The docs for Node.append() have a note about using Y.Escape.html() to escape the HTML content before appending. http://yuilibrary.com/yui/docs/api/classes/Node.html#method_append In this case I don't think that this is needed here, but if you could just check.
            Hide
            marina Marina Glancy added a comment -

            Andrew, I did not change this comparing how it was before and how it is in case of a link. If I was to write this code properly, I would return html in AJAX response to make sure it is the same as renderer wants it to be. Because at the moment it is not the same even for core theme. Besides AJAX script should be in navigation block and not in core.
            I would say here: it worked so far as it is - let's leave it. Even though it's not my motto generally.

            Show
            marina Marina Glancy added a comment - Andrew, I did not change this comparing how it was before and how it is in case of a link. If I was to write this code properly, I would return html in AJAX response to make sure it is the same as renderer wants it to be. Because at the moment it is not the same even for core theme. Besides AJAX script should be in navigation block and not in core. I would say here: it worked so far as it is - let's leave it. Even though it's not my motto generally.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Agreed - perhaps an issue to look at for 2.6

            Show
            dobedobedoh Andrew Nicols added a comment - Agreed - perhaps an issue to look at for 2.6
            Hide
            nebgor Aparup Banerjee added a comment - - edited

            thanks, thats integrated into master and 24.
            (tested on master)

            Show
            nebgor Aparup Banerjee added a comment - - edited thanks, thats integrated into master and 24. (tested on master)
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited

            Note that this was incorrectly shifter-ed:

            http://integration.moodle.org/job/09.%20Run%20shifter%20for%20all%20modules%20(master)/74/

            So has been done manually and pushed to integration.git by following the instructions @ http://docs.moodle.org/dev/Javascript/Shifter

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited Note that this was incorrectly shifter-ed: http://integration.moodle.org/job/09.%20Run%20shifter%20for%20all%20modules%20(master)/74/ So has been done manually and pushed to integration.git by following the instructions @ http://docs.moodle.org/dev/Javascript/Shifter Ciao
            Hide
            marina Marina Glancy added a comment -

            I woke up in the middle of the night thinking "I forgot to run shifter!", but you woke up before me so it seems Thanks Eloy

            Show
            marina Marina Glancy added a comment - I woke up in the middle of the night thinking "I forgot to run shifter!", but you woke up before me so it seems Thanks Eloy
            Hide
            phalacee Jason Fowler added a comment -

            All good Marina

            Show
            phalacee Jason Fowler added a comment - All good Marina
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Your awesome contributions are now part of Moodle, your fav LMS out there.

            Closing this as fixed.

            Many thanks for all the hard work, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Your awesome contributions are now part of Moodle, your fav LMS out there. Closing this as fixed. Many thanks for all the hard work, ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  13/May/13