Moodle
  1. Moodle
  2. MDL-33874

Modify the formchangedchecker javascript function to ignore form elements with the class "ignoredirty".

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3.2
    • Component/s: Forms Library
    • Labels:
    • Testing Instructions:
      Hide

      Initially check for regressions

      • Open a course page
      • Turn editing on
      • Add a new activity - I chose Page
      • Refresh the page
        • Confirm that the page refreshes and doesn't show any warnings
      • Set a title
      • Refresh the page
        • Confirm that you get a warning message
        • Reload the page anyway
      • Set a title but do not leave focus from the form element
      • Refresh the page
        • Confirm that you get a warning message
        • Reload the page anyway

      Testing a broken form

      • Add a new Assignment to the course
      • View the submitted assignments using the 'View/grade all submissions' link
      • Tick the 'Quick grading' checkbox at the bottom of the page
        • Confirm that the page warns you even though you haven't made any other changes
      • Edit mod/assign/gradingoptionsform.php and change the quickgrading checkbox to add the ignoredirty class:

        $mform->addElement('checkbox', 'quickgrading', get_string('quickgrading', 'assign'), '', array('class' => 'ignoredirty'));

      • Refresh the submitted assignments page
      • Tick the 'Quick grading' checkbox at the bottom of the page
        • Confirm that the page reloads without warning
      • Enter a grade for a student
      • Tick the 'Quick grading' checkbox at the bottom of the page
        • Confirm that you get a warning message this time
      • Edit mod/page/mod_form.php and change the name textbox to add the ignoredirty class:

        $mform->addElement('text', 'name', get_string('name'), array('size'=>'48', 'class' => 'ignoredirty'));

      • Add a new Page on a course
      • Set a title but do not leave focus from the form element
      • Refresh the page
        • Confirm that you do not get a warning message
      Show
      Initially check for regressions Open a course page Turn editing on Add a new activity - I chose Page Refresh the page Confirm that the page refreshes and doesn't show any warnings Set a title Refresh the page Confirm that you get a warning message Reload the page anyway Set a title but do not leave focus from the form element Refresh the page Confirm that you get a warning message Reload the page anyway Testing a broken form Add a new Assignment to the course View the submitted assignments using the 'View/grade all submissions' link Tick the 'Quick grading' checkbox at the bottom of the page Confirm that the page warns you even though you haven't made any other changes Edit mod/assign/gradingoptionsform.php and change the quickgrading checkbox to add the ignoredirty class: $mform->addElement('checkbox', 'quickgrading', get_string('quickgrading', 'assign'), '', array('class' => 'ignoredirty')); Refresh the submitted assignments page Tick the 'Quick grading' checkbox at the bottom of the page Confirm that the page reloads without warning Enter a grade for a student Tick the 'Quick grading' checkbox at the bottom of the page Confirm that you get a warning message this time Edit mod/page/mod_form.php and change the name textbox to add the ignoredirty class: $mform->addElement('text', 'name', get_string('name'), array('size'=>'48', 'class' => 'ignoredirty')); Add a new Page on a course Set a title but do not leave focus from the form element Refresh the page Confirm that you do not get a warning message
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-33874-master-2

      Description

      In the assignment module (and probably elsewhere in Moodle) there are certain form elements that do not require dirty/change tracking. The dirty/change tracking is what prompts the "You have unsaved changes" dialog when you leave the page without submitting the form. In the assignment grading page it would be nice to disable this functionality for the selection checkboxes and the batch grading actions form.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Damyon Wiese added a comment -

            Hi Andrew - this issue came up on the grading page for the assignment.

            Checkboxes boxes are used for selecting rows - it would be nicer if we didn't show the warning if the only thing that has changed is which rows in the grading table are selected.

            I don't mind if this is done another way but it would be useful to be able to mark a form element as not affecting the dirty state of the form.

            • Thanks !
            Show
            Damyon Wiese added a comment - Hi Andrew - this issue came up on the grading page for the assignment. Checkboxes boxes are used for selecting rows - it would be nicer if we didn't show the warning if the only thing that has changed is which rows in the grading table are selected. I don't mind if this is done another way but it would be useful to be able to mark a form element as not affecting the dirty state of the form. Thanks !
            Hide
            Andrew Nicols added a comment -

            I've made the changes I suggested in MDL-33843.

            Damyon,

            Can you provide some details of any specific form fields that this needs to be applied to for testing?

            Cheers,

            Andrew

            Show
            Andrew Nicols added a comment - I've made the changes I suggested in MDL-33843 . Damyon, Can you provide some details of any specific form fields that this needs to be applied to for testing? Cheers, Andrew
            Hide
            Damyon Wiese added a comment -

            This would be a good example:

            diff --git a/mod/assign/gradingbatchoperationsform.php b/mod/assign/gradingbatchoperationsform.php
            index 2ae00a2..48f8210 100644
            --- a/mod/assign/gradingbatchoperationsform.php
            +++ b/mod/assign/gradingbatchoperationsform.php
            @@ -58,7 +58,7 @@ class mod_assign_grading_batch_operations_form extends moodleform {
                     $mform->addElement('hidden', 'returnaction', 'grading');
             
                     $objs = array();
            -        $objs[] =& $mform->createElement('select', 'operation', '', $options);
            +        $objs[] =& $mform->createElement('select', 'operation', '', $options, array('class'=>'ignoredirty'));
                     $objs[] =& $mform->createElement('submit', 'submit', get_string('go'));
                     $mform->addElement('group', 'actionsgrp', get_string('batchoperationsdescription', 'assign'), $objs, ' ', false);
            

            Show
            Damyon Wiese added a comment - This would be a good example: diff --git a/mod/assign/gradingbatchoperationsform.php b/mod/assign/gradingbatchoperationsform.php index 2ae00a2..48f8210 100644 --- a/mod/assign/gradingbatchoperationsform.php +++ b/mod/assign/gradingbatchoperationsform.php @@ -58,7 +58,7 @@ class mod_assign_grading_batch_operations_form extends moodleform { $mform->addElement('hidden', 'returnaction', 'grading'); $objs = array(); - $objs[] =& $mform->createElement('select', 'operation', '', $options); + $objs[] =& $mform->createElement('select', 'operation', '', $options, array('class'=>'ignoredirty')); $objs[] =& $mform->createElement('submit', 'submit', get_string('go')); $mform->addElement('group', 'actionsgrp', get_string('batchoperationsdescription', 'assign'), $objs, ' ', false);
            Hide
            Dan Poltawski added a comment -

            Seems sensible. It would be good to convert the form elements which are manually dealing with this to use this mechanism.

            Show
            Dan Poltawski added a comment - Seems sensible. It would be good to convert the form elements which are manually dealing with this to use this mechanism.
            Hide
            Andrew Nicols added a comment -

            Note to integrators:

            This should cherry-pick cleanly to 2.3 and I feel it's probably worth adding it even though technically it's a new feature as it will solve various form-related issues which will no-doubt crop up.

            Show
            Andrew Nicols added a comment - Note to integrators: This should cherry-pick cleanly to 2.3 and I feel it's probably worth adding it even though technically it's a new feature as it will solve various form-related issues which will no-doubt crop up.
            Hide
            Andrew Nicols added a comment -

            Added an issue to group together all issues relating to this.

            Show
            Andrew Nicols added a comment - Added an issue to group together all issues relating to this.
            Hide
            Sam Hemelryk added a comment -

            Thanks guys.
            This is certainly a worthwhile improvement and I've backported it to 2.3 as I agree that is well worth doing.

            All clear to look to upgrade custom solutions now.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Thanks guys. This is certainly a worthwhile improvement and I've backported it to 2.3 as I agree that is well worth doing. All clear to look to upgrade custom solutions now. Cheers Sam
            Hide
            Damyon Wiese added a comment -

            In testing this more the submitted patch does not cover all cases - if the focused element has the ignoredirty element it should be ignored as well:

                         // If a field has been focused and changed, but still has focus then the browser won't fire t
                         // onChange event. We check for this eventuality here
                         if (state.focused_element) {
            -                if (state.focused_element.element.get('value') != state.focused_element.initial_value) {
            -                    return 1;
            +                if (!state.focused_element.hasClass('ignoredirty')) {
            +                    if (state.focused_element.element.get('value') != state.focused_element.initial_value
            +                        return 1;
            +                    }
                             }
                         }
            

            Show
            Damyon Wiese added a comment - In testing this more the submitted patch does not cover all cases - if the focused element has the ignoredirty element it should be ignored as well: // If a field has been focused and changed, but still has focus then the browser won't fire t // onChange event. We check for this eventuality here if (state.focused_element) { - if (state.focused_element.element.get('value') != state.focused_element.initial_value) { - return 1; + if (!state.focused_element.hasClass('ignoredirty')) { + if (state.focused_element.element.get('value') != state.focused_element.initial_value + return 1; + } } }
            Hide
            Dan Poltawski added a comment -

            Sending this to the failed testing state. Thanks Damyon.

            Show
            Dan Poltawski added a comment - Sending this to the failed testing state. Thanks Damyon.
            Hide
            Sam Hemelryk added a comment -

            Thanks for spotting that Damyon.
            Andrew are you able to produce a patch for that?

            cheers
            Sam

            Show
            Sam Hemelryk added a comment - Thanks for spotting that Damyon. Andrew are you able to produce a patch for that? cheers Sam
            Hide
            Andrew Nicols added a comment -

            Your wish is my command. I've written this slightly differently to Damyon's suggestion - it checks as soon as the function is entered instead.

            Show
            Andrew Nicols added a comment - Your wish is my command. I've written this slightly differently to Damyon's suggestion - it checks as soon as the function is entered instead.
            Hide
            Sam Hemelryk added a comment -

            Hi Andrew, looks like that new commit may have gone to a different location.
            The branch doesn't appear to exist presently.
            Could you please check it out and see if you can find it.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Andrew, looks like that new commit may have gone to a different location. The branch doesn't appear to exist presently. Could you please check it out and see if you can find it. Cheers Sam
            Hide
            Andrew Nicols added a comment -

            Ahem - yes... apologies about that. The problem of having both a private, and a public repository. I really must get into the habit of checking when I update the tracker issue!

            Cheers,
            Andrew

            Show
            Andrew Nicols added a comment - Ahem - yes... apologies about that. The problem of having both a private, and a public repository. I really must get into the habit of checking when I update the tracker issue! Cheers, Andrew
            Hide
            Sam Hemelryk added a comment -

            Hehe no probs, thanks for fixing it up Andrew.
            This has been integrated again is once more ready for testing

            Show
            Sam Hemelryk added a comment - Hehe no probs, thanks for fixing it up Andrew. This has been integrated again is once more ready for testing
            Hide
            Frédéric Massart added a comment -

            Test passes on master! Thanks!

            Show
            Frédéric Massart added a comment - Test passes on master! Thanks!
            Hide
            Dan Poltawski added a comment -

            *Notice*: Undefined variable: friendlyintegrator in /Users/danp/git/tokenintegrationthanks.php on line 26

            Congratulations

            {tracker.user.name}

            !

            You've made into Moodle

            {tracker.fixversion-1}

            +

            I would like to personally thank you for this contribution on behalf of all Moodle users throughout the world.

            cheers!

            {tracker.friendlyintegrator}
            Show
            Dan Poltawski added a comment - * Notice *: Undefined variable: friendlyintegrator in /Users/danp/git/tokenintegrationthanks.php on line 26 Congratulations {tracker.user.name} ! You've made into Moodle {tracker.fixversion-1} + I would like to personally thank you for this contribution on behalf of all Moodle users throughout the world. cheers! {tracker.friendlyintegrator}
            Hide
            Marina Glancy added a comment -

            Andrew, lib/form/filemanager.js calls

            M.core_formchangechecker.set_form_changed()
            

            without arguments, i.e. e is undefined

            After your changes I get an error:

            Uncaught TypeError: Cannot read property 'target' of undefined
            M.core_formchangechecker.set_form_changed yui_combo.php:96
            Y.extend.filepicker_callback filemanager.js:190
            Y.extend.setup_select_file.getfile.on.request.callback filepicker.js:1181
            Y.extend.request.cfg.on.complete filepicker.js:605
            Y.Subscriber._notify event-custom-base.js:1154
            Y.Subscriber.notify event-custom-base.js:1183
            Y.CustomEvent._notify event-custom-base.js:879
            Y.CustomEvent._procSubs event-custom-base.js:984
            Y.CustomEvent.fire Simpleevent-custom-base.js:953
            Y.CustomEvent.fire event-custom-base.js:934
            ET.fire event-custom-base.js:1962
            IO._evt io-base.js:230
            IO.complete io-base.js:266
            IO._rS io-base.js:512
            

            Show
            Marina Glancy added a comment - Andrew, lib/form/filemanager.js calls M.core_formchangechecker.set_form_changed() without arguments, i.e. e is undefined After your changes I get an error: Uncaught TypeError: Cannot read property 'target' of undefined M.core_formchangechecker.set_form_changed yui_combo.php:96 Y.extend.filepicker_callback filemanager.js:190 Y.extend.setup_select_file.getfile.on.request.callback filepicker.js:1181 Y.extend.request.cfg.on.complete filepicker.js:605 Y.Subscriber._notify event-custom-base.js:1154 Y.Subscriber.notify event-custom-base.js:1183 Y.CustomEvent._notify event-custom-base.js:879 Y.CustomEvent._procSubs event-custom-base.js:984 Y.CustomEvent.fire Simpleevent-custom-base.js:953 Y.CustomEvent.fire event-custom-base.js:934 ET.fire event-custom-base.js:1962 IO._evt io-base.js:230 IO.complete io-base.js:266 IO._rS io-base.js:512
            Hide
            Dan Poltawski added a comment -

            Marina, please can you creae a new issue for this?

            Show
            Dan Poltawski added a comment - Marina, please can you creae a new issue for this?
            Hide
            Marina Glancy added a comment -

            created MDL-34433

            Show
            Marina Glancy added a comment - created MDL-34433

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: