Moodle
  1. Moodle
  2. MDL-40142

JS fatal error in navigation block

    Details

    • Testing Instructions:
      Hide
      1. Log in as an admin.
      2. Use the navigation and expand the "Courses" branch to check AJAX expansion works.
      3. Turn on editing.
      4. Edit the navigation block and set "Generate navigation for the following" to "Categories, courses, and course structures"
      5. Save and browse back to the front page.
      6. Expand the courses branch in the navigation to ensure things load via AJAX.
      7. Browse to a course.
      8. Ensure you can expand any branch in the navigation still.
      9. Check that there have been no JS errors.
      Show
      Log in as an admin. Use the navigation and expand the "Courses" branch to check AJAX expansion works. Turn on editing. Edit the navigation block and set "Generate navigation for the following" to "Categories, courses, and course structures" Save and browse back to the front page. Expand the courses branch in the navigation to ensure things load via AJAX. Browse to a course. Ensure you can expand any branch in the navigation still. Check that there have been no JS errors.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull 2.4 Branch:
      wip-MDL-40142-m24
    • Pull 2.5 Branch:
      wip-MDL-40142-m25
    • Pull Master Branch:
      wip-MDL-40142-m26
    • Rank:
      50885

      Description

      As reported by John Porten in
      https://tracker.moodle.org/browse/MDL-39949?focusedCommentId=227916&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-227916

      We were able to reproduce the bug reported by Doug Moody.

      To recap: After upgrading to 2.5, the Activity Chooser link (+Add an activity or resource) was not working.

      What we found: A JS error occurs when the Navigation Block configuration option "Generate navigation for the following" is set to "Categories, courses, and course structures". The other options do not cause the error and the Activity Chooser link functions normally.

        Issue Links

          Activity

          Hide
          Marina Glancy added a comment -

          I was able to reproduce it on course view page

          Error in JS console:

          TypeError: branch.get is not a function
          [Break On This Error]
          this.branches[branch.get('id')] = branch;

          Show
          Marina Glancy added a comment - I was able to reproduce it on course view page Error in JS console: TypeError: branch.get is not a function [Break On This Error] this.branches [branch.get('id')] = branch;
          Hide
          Sam Hemelryk added a comment -

          Alrighty - I'm putting up a patch to fix the direct JavaScript error.

          Essentially there is a bigger problem at play here, why the navigation block is telling JavaScript that there are expandable section branches when in fact there are not.
          After looking at the problem for a wee while I've come to the conclusion that fixing that may result in UI changes, or at the very least community discussion. Both of which will likely take more time to fix and draw out this problem.
          In the interests of getting a fix for this ASAP I'm going to put this up as it is (fixing the problem in JS only) and then open another issue to investigate why the navigation code is informing us about section nodes that aren't being renderered.

          The JS issue here is that the wire method is supposed to be chainable. If the node is not found then it returns false, breaking the chain-ability of the method and causing this error.
          The bug has existed in the JS code since I first wrote the navigation JS this is just the first time the issue has arisen. Likely the first time the navigation HTML and JS fallen out of sync.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Alrighty - I'm putting up a patch to fix the direct JavaScript error. Essentially there is a bigger problem at play here, why the navigation block is telling JavaScript that there are expandable section branches when in fact there are not. After looking at the problem for a wee while I've come to the conclusion that fixing that may result in UI changes, or at the very least community discussion. Both of which will likely take more time to fix and draw out this problem. In the interests of getting a fix for this ASAP I'm going to put this up as it is (fixing the problem in JS only) and then open another issue to investigate why the navigation code is informing us about section nodes that aren't being renderered. The JS issue here is that the wire method is supposed to be chainable. If the node is not found then it returns false, breaking the chain-ability of the method and causing this error. The bug has existed in the JS code since I first wrote the navigation JS this is just the first time the issue has arisen. Likely the first time the navigation HTML and JS fallen out of sync. Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          Linking to MDL-40143 to look at what is going wrong between the HTML and JS.

          Show
          Sam Hemelryk added a comment - Linking to MDL-40143 to look at what is going wrong between the HTML and JS.
          Hide
          Andrew Nicols added a comment -

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

          The code looks good to me.

          Testing

          Needs testing instructions.

          Documentation

          Since you're adding the relevant yuidoc there, it's probably worth adding the method name:

          @method wire
          

          Misc

          I assume that something elsewhere does actually set expandable and haschildren to true - I'm guessing that the data- attributes inform it.

          I've also just noticed that BRANCH is a global variable - another one for the backlog.

          Show
          Andrew Nicols added a comment - [Y] Syntax [Y] Output [Y] Whitespace [-] Language [-] Databases [N] Testing [Y] Security [N] Documentation [Y] Git [Y] Sanity check The code looks good to me. Testing Needs testing instructions. Documentation Since you're adding the relevant yuidoc there, it's probably worth adding the method name: @method wire Misc I assume that something elsewhere does actually set expandable and haschildren to true - I'm guessing that the data- attributes inform it. I've also just noticed that BRANCH is a global variable - another one for the backlog.
          Hide
          Sam Hemelryk added a comment -

          Thanks Andrew - I'm putting this up for integration now.
          I've added the @method tag and am adding test instructions now.
          In regards to set expandable and has children, that is done when the branch is first constructed.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Andrew - I'm putting this up for integration now. I've added the @method tag and am adding test instructions now. In regards to set expandable and has children, that is done when the branch is first constructed. Many thanks Sam
          Hide
          Marina Glancy added a comment -

          Thanks Sam, this has been integrated in 2.3, 2.4, 2.5 and master

          TO TESTERS: It is likely that the original bug can not be reproduced under specified conditions in 2.3 and 2.4 but worth fixing anyway

          Show
          Marina Glancy added a comment - Thanks Sam, this has been integrated in 2.3, 2.4, 2.5 and master TO TESTERS: It is likely that the original bug can not be reproduced under specified conditions in 2.3 and 2.4 but worth fixing anyway
          Hide
          Rajesh Taneja added a comment -

          Thanks for fixing this Sam,

          Navigation works fine, was able to expand different nodes without any error.

          Show
          Rajesh Taneja added a comment - Thanks for fixing this Sam, Navigation works fine, was able to expand different nodes without any error.
          Hide
          Dan Poltawski added a comment -

          Thanks for your contributions!

          _main:
          @ BB#0:
                  push    {r7, lr}
                  mov     r7, sp
                  sub     sp, #4
                  movw    r0, :lower16:(L_.str-(LPC0_0+4))
                  movt    r0, :upper16:(L_.str-(LPC0_0+4))
          LPC0_0:
                  add     r0, pc
                  bl      _printf
                  movs    r1, #0
                  movt    r1, #0
                  str     r0, [sp]                @ 4-byte Spill
                  mov     r0, r1
                  add     sp, #4
                  pop     {r7, pc}
          
                  .section        __TEXT,__cstring,cstring_literals
          L_.str:                                 @ @.str
                  .asciz   "This code is now upstream!"
          
          Show
          Dan Poltawski added a comment - Thanks for your contributions! _main: @ BB#0: push {r7, lr} mov r7, sp sub sp, #4 movw r0, :lower16:(L_.str-(LPC0_0+4)) movt r0, :upper16:(L_.str-(LPC0_0+4)) LPC0_0: add r0, pc bl _printf movs r1, #0 movt r1, #0 str r0, [sp] @ 4- byte Spill mov r0, r1 add sp, #4 pop {r7, pc} .section __TEXT,__cstring,cstring_literals L_.str: @ @.str .asciz "This code is now upstream!"

            People

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

              Dates

              • Created:
                Updated:
                Resolved: