Moodle
  1. Moodle
  2. MDL-38887

AJAX loaded navigation tree does not show hidden branches as hidden

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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 2.4 Branch:
      wip-MDL-38887-m24
    • Pull Master Branch:
      wip-MDL-38887-m25
    • Rank:
      48983

      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

        Issue Links

          Activity

          Hide
          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 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 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 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 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 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
          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
          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 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 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
          Andrew Nicols added a comment -

          Agreed - perhaps an issue to look at for 2.6

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

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

          Show
          Aparup Banerjee added a comment - - edited thanks, thats integrated into master and 24. (tested on master)
          Hide
          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
          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 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 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
          Jason Fowler added a comment -

          All good Marina

          Show
          Jason Fowler added a comment - All good Marina
          Hide
          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
          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: