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

      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.

        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: