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

          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