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

Lesson timer throws errors about undeclared javascript vars

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.6, 2.5, 2.6
    • Fix Version/s: 2.4.7, 2.5.3
    • Component/s: Lesson
    • Labels:
      None
    • Testing Instructions:
      Hide
      1. Go to a course and add a lesson with "Time limit (minutes)" set to 5
      2. Add a content page on it
      3. Login as student
      4. Open the inspector JS console and access the lesson
      5. You SHOULD NOT see any error about Uncaught ReferenceError: servertime|starttime is not defined
      6. Wait a while (not more than 5 minutes)
      7. The next steps are to ensure that the timer (in a block) is still working properly
        1. Wait a while and refresh the page
        2. The time remaining value SHOULD be less than before
        3. You SHOULD NOT see any error about Uncaught ReferenceError: servertime|starttime is not defined
        4. Wait more than 5 minutes and refresh the page
        5. You SHOULD see You did not answer any questions. You have received a 0 for this lesson.
      Show
      Go to a course and add a lesson with "Time limit (minutes)" set to 5 Add a content page on it Login as student Open the inspector JS console and access the lesson You SHOULD NOT see any error about Uncaught ReferenceError: servertime|starttime is not defined Wait a while (not more than 5 minutes) The next steps are to ensure that the timer (in a block) is still working properly Wait a while and refresh the page The time remaining value SHOULD be less than before You SHOULD NOT see any error about Uncaught ReferenceError: servertime|starttime is not defined Wait more than 5 minutes and refresh the page You SHOULD see You did not answer any questions. You have received a 0 for this lesson.
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-42663_master

      Description

      I've tried it in Chrome + Linux and using:

      • cachejs -> disabled + yuicomboloading -> disabled
      • cachejs -> enabled + yuicomboloading -> enabled

      I discovered it while working on MDL-42625 as the undeclared vars breaks the JS and M.util.pending_js is always full.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              rwijaya Rossiani Wijaya added a comment -

              Hi David,

              Just a minor fix for spacing on this line https://github.com/dmonllao/moodle/commit/bb7603f4a324257e24066b2bda691daacc6edd17#diff-ef19ca6b08229a8ac4ab12bd050361e1R52

              [y] Syntax
              [y] Whitespace, above message
              [y] Output
              [-] Language
              [-] Databases
              [y] Testing (instructions and automated tests)
              [-] Security
              [-] Documentation
              [y] Git
              []- Third party code
              [y] Sanity check

              Other than the above message. The patch look great. Thanks for fixing.

              Show
              rwijaya Rossiani Wijaya added a comment - Hi David, Just a minor fix for spacing on this line https://github.com/dmonllao/moodle/commit/bb7603f4a324257e24066b2bda691daacc6edd17#diff-ef19ca6b08229a8ac4ab12bd050361e1R52 [y] Syntax [y] Whitespace, above message [y] Output [-] Language [-] Databases [y] Testing (instructions and automated tests) [-] Security [-] Documentation [y] Git []- Third party code [y] Sanity check Other than the above message. The patch look great. Thanks for fixing.
              Hide
              dmonllao David Monllaó added a comment -

              Thanks Rossie, sending to integration

              Show
              dmonllao David Monllaó added a comment - Thanks Rossie, sending to integration
              Hide
              damyon Damyon Wiese added a comment -

              Thanks David - phantom for the win!

              Integrated to 24, 25 and master.

              (I tested on master and the fix worked for me).

              Show
              damyon Damyon Wiese added a comment - Thanks David - phantom for the win! Integrated to 24, 25 and master. (I tested on master and the fix worked for me).
              Hide
              dobedobedoh Andrew Nicols added a comment -

              This doesn't pass our linter either...

              parseInt() needs a radix in the second param, and we should do strict equality comparisons.

              Show
              dobedobedoh Andrew Nicols added a comment - This doesn't pass our linter either... parseInt() needs a radix in the second param, and we should do strict equality comparisons.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              and typeof is not a function, it's an operator:

              if (typeof clocksettings !== undefined) {
                  if (clocksettings.starttime) {
                      starttime = parseInt(clocksettings.starttime, 10);
                  }
              ...
               
              }

              Show
              dobedobedoh Andrew Nicols added a comment - and typeof is not a function, it's an operator: if (typeof clocksettings !== undefined) { if (clocksettings.starttime) { starttime = parseInt(clocksettings.starttime, 10); } ...   }
              Hide
              dmonllao David Monllaó added a comment -

              is just a copy & paste of the current code, is the current code passing our linter?

              Show
              dmonllao David Monllaó added a comment - is just a copy & paste of the current code, is the current code passing our linter?
              Hide
              damyon Damyon Wiese added a comment -

              in this case the code was just moved and we do not want to add regressions.

              Show
              damyon Damyon Wiese added a comment - in this case the code was just moved and we do not want to add regressions.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Hmm okay. We should really pick up those radix changes whenever possible as they do have potential to cause whacky things in certain situations (e.g. if passed a number with a leading zero, they change behaviour to base 8).

              Show
              dobedobedoh Andrew Nicols added a comment - Hmm okay. We should really pick up those radix changes whenever possible as they do have potential to cause whacky things in certain situations (e.g. if passed a number with a leading zero, they change behaviour to base 8).
              Hide
              fred Frédéric Massart added a comment -

              Works as expected. Passing.

              Show
              fred Frédéric Massart added a comment - Works as expected. Passing.
              Hide
              damyon Damyon Wiese added a comment -

              Here lies 52 bugs.
              All fixed or swept under a rug.
              If they come back one day,
              To our dismay,
              We all will feel quite un-smug.

              Thanks for the reporting/fixing/testing on this issue. It has been sent upstream.

              Show
              damyon Damyon Wiese added a comment - Here lies 52 bugs. All fixed or swept under a rug. If they come back one day, To our dismay, We all will feel quite un-smug. Thanks for the reporting/fixing/testing on this issue. It has been sent upstream.

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    11/Nov/13