Details

    • 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:
    • Rank:
      47173

      Description

      Fix JSHint issues with moodle-core-notification

        Activity

        Hide
        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 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 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 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
        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
        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
        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
        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
        Eloy Lafuente (stronk7) added a comment -

        Integrated, thanks!

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

        Thanks Andrew,

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

        Show
        Rajesh Taneja added a comment - Thanks Andrew, Works as described. Could see Syntax error dialogue, and no error was thrown in console.
        Hide
        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
        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: