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

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

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: 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

          Attachments

            Issue Links

              Activity

              Hide
              damyon 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 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
              dobedobedoh 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
              dobedobedoh 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 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 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
              poltawski 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
              poltawski 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
              dobedobedoh 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
              dobedobedoh 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
              dobedobedoh Andrew Nicols added a comment -

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

              Show
              dobedobedoh Andrew Nicols added a comment - Added an issue to group together all issues relating to this.
              Hide
              samhemelryk 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
              samhemelryk 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 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 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
              poltawski Dan Poltawski added a comment -

              Sending this to the failed testing state. Thanks Damyon.

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

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

              cheers
              Sam

              Show
              samhemelryk Sam Hemelryk added a comment - Thanks for spotting that Damyon. Andrew are you able to produce a patch for that? cheers Sam
              Hide
              dobedobedoh 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
              dobedobedoh 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
              samhemelryk 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
              samhemelryk 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
              dobedobedoh 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
              dobedobedoh 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
              samhemelryk 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
              samhemelryk 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
              fred Frédéric Massart added a comment -

              Test passes on master! Thanks!

              Show
              fred Frédéric Massart added a comment - Test passes on master! Thanks!
              Hide
              poltawski 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
              poltawski 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 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 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
              poltawski Dan Poltawski added a comment -

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

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

              created MDL-34433

              Show
              marina 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:
                    Fix Release Date:
                    10/Sep/12