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

          Attachments

            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