Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.1
    • Fix Version/s: 2.5
    • Component/s: JavaScript
    • Labels:
    • Testing Instructions:
      Hide

      Moodle notification dialogues are used in various places, though currently mostly for exception displays. The new activity chooser also uses them and this is the easiest place to test them.

      • Open a course page
      • Open your JS console
      • Turn editing on
      • Open the Activity chooser
      • Confirm that it works as expected
      • Edit /enrol/ajax.php
      • Add something which has no chance of being parsed by a json parser (e.g. echo "{];"; die
      • Navigate to Course administration -> Users -> Enrolled users
        • Confirm that an error was shown in a dialogue
        • Confirm that no errors were thrown in the console
      Show
      Moodle notification dialogues are used in various places, though currently mostly for exception displays. The new activity chooser also uses them and this is the easiest place to test them. Open a course page Open your JS console Turn editing on Open the Activity chooser Confirm that it works as expected Edit /enrol/ajax.php Add something which has no chance of being parsed by a json parser (e.g. echo "{];"; die Navigate to Course administration -> Users -> Enrolled users Confirm that an error was shown in a dialogue Confirm that no errors were thrown in the console
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:

      Description

      Fix JSHint issues with moodle-core-notification

        Gliffy Diagrams

          Activity

          Hide
          damyon Damyon Wiese added a comment -

          Hi Andrew,

          Thanks for the patch - I just have one comment about this section:

              CONFIRM,
              AJAXEXCEPTION,
           
              DIALOGUE = function(config) {
              COUNT++;
              var id = 'moodle-dialogue-'+COUNT;
          
          

          I find the indenting here a bit jarring - to generalise I would say we should try not to assign a function block within a variable declaration list. I think this is much easier to read:

              CONFIRM,
              AJAXEXCEPTION,
              DIALOGUE;
           
          DIALOGUE = function(config) {
              COUNT++;
              var id = 'moodle-dialogue-'+COUNT;
          
          

          The rest of the patch looks good - I'll do some more testing on it and let you know if I find anything.

          Show
          damyon Damyon Wiese added a comment - Hi Andrew, Thanks for the patch - I just have one comment about this section: CONFIRM, AJAXEXCEPTION,   DIALOGUE = function(config) { COUNT++; var id = 'moodle-dialogue-'+COUNT; I find the indenting here a bit jarring - to generalise I would say we should try not to assign a function block within a variable declaration list. I think this is much easier to read: CONFIRM, AJAXEXCEPTION, DIALOGUE;   DIALOGUE = function(config) { COUNT++; var id = 'moodle-dialogue-'+COUNT; The rest of the patch looks good - I'll do some more testing on it and let you know if I find anything.
          Hide
          damyon Damyon Wiese added a comment -

          Just changed the testing instructions as I didn't need to click any buttons on the enrolled users page to see the error dialogue.

          Show
          damyon Damyon Wiese added a comment - Just changed the testing instructions as I didn't need to click any buttons on the enrolled users page to see the error dialogue.
          Hide
          dobedobedoh Andrew Nicols added a comment -

          Thanks Damyon,

          I agree - that does make it easier to read. I was in two minds when I wrote it that way in the first place.

          Submitting for integration. Thanks for the review.

          Andrew

          Show
          dobedobedoh Andrew Nicols added a comment - Thanks Damyon, I agree - that does make it easier to read. I was in two minds when I wrote it that way in the first place. Submitting for integration. Thanks for the review. Andrew
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Thanks Andrew,

          Works as described. Could see Syntax error dialogue, and no error was thrown in console.

          Show
          rajeshtaneja Rajesh Taneja added a comment - Thanks Andrew, Works as described. Could see Syntax error dialogue, and no error was thrown in console.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed.

          (and won't be revisiting it unless some regression is found)

          Thanks and ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed. (and won't be revisiting it unless some regression is found) Thanks and ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                14/May/13