Moodle
  1. Moodle
  2. MDL-30288

De-selecting an overridden check box requires saving twice.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0.5, 2.1.2, 2.2
    • Fix Version/s: 2.0.6, 2.1.3
    • Component/s: Gradebook
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide

      Create an assessment (assignment, quiz etc.) An online text assignment is probably the quickest.
      Log in as a student.
      Complete the assessment
      Log back in as a teacher and then grade the assessment. Do not provide feedback otherwise you may encounter another bug MDL-18095

      Next go to [Course administration->Grades]
      Turn editing on. Change (ie override) a grade and click update.
      Then click on the small pencil (edit grade) icon. De-select the overridden check box and click save. Do NOT enter feedback.

      The grade should have reverted to the value it had before you overrode it.

      Show
      Create an assessment (assignment, quiz etc.) An online text assignment is probably the quickest. Log in as a student. Complete the assessment Log back in as a teacher and then grade the assessment. Do not provide feedback otherwise you may encounter another bug MDL-18095 Next go to [Course administration->Grades] Turn editing on. Change (ie override) a grade and click update. Then click on the small pencil (edit grade) icon. De-select the overridden check box and click save. Do NOT enter feedback. The grade should have reverted to the value it had before you overrode it.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull Master Branch:
      MDL-30288_override_checkbox
    • Rank:
      32629

      Description

      From testing http://tracker.moodle.org/browse/MDLQA-1333

      Please note that you can't just overwrite the grade to reproduce this problem.

      To reproduce this problem you will need to set up a course. Include an assessment (assignment, quiz etc.) log in as a student. Complete the assessment, log back in as a teacher and then grade the assessment.

      Next go to [Course administration->Grades]
      Turn editing on. Change a grade and then click on the small pencil (edit grade) icon. De-select the overridden check box and click save. You should notice that the grade is still overridden. If you go back in and repeat the process it will finally revert the grade back to it's original score.

        Issue Links

          Activity

          Hide
          Andrew Davis added a comment -

          I can reproduce it. Quite a bizarre bug.

          Show
          Andrew Davis added a comment - I can reproduce it. Quite a bizarre bug.
          Hide
          Andrew Davis added a comment - - edited

          It appears that the overridden flag cannot be toggled unless the submitted final grade and feedback match that which is already in the database. Not sure why that restriction is in place for feedback as its just text.

          Anyhow, feedback isn't matching on the first page submission so the change in the overridden flag is being ignored. The first string is in the one in $data. The second is $old_grade_grade. Note the appearance of the text_to_html div.

          string(74) "<div class="text_to_html">
          <p>could be a little more thorough</p>
          </div>
          
          string(38) "<p>could be a little more thorough</p>"
          
          ... and $data->feedback === $old_grade_grade->feedback) {
                      // change overridden flag only if grade or feedback not changed
          

          When I hit F5 the string changes as follows indicating that the changed feedback was saved the first time we tried.

          string(74) "<div class="text_to_html">
          <p>could be a little more thorough</p>
          </div>"
          
          string(74) "<div class="text_to_html">
          <p>could be a little more thorough</p>
          </div>"
          

          However, something at some point, possibly during the regrading (Im guessing here), is cleaning the feedback string and stripping off the div around the p tags ie the changed feedback string gets saved, you repeat the action, this time the changed overridden flag is observed and regrading occurs and the feedback string gets cleaned. Something like that is going on.

          Show
          Andrew Davis added a comment - - edited It appears that the overridden flag cannot be toggled unless the submitted final grade and feedback match that which is already in the database. Not sure why that restriction is in place for feedback as its just text. Anyhow, feedback isn't matching on the first page submission so the change in the overridden flag is being ignored. The first string is in the one in $data. The second is $old_grade_grade. Note the appearance of the text_to_html div. string(74) "<div class=" text_to_html"> <p>could be a little more thorough</p> </div> string(38) "<p>could be a little more thorough</p>" ... and $data->feedback === $old_grade_grade->feedback) { // change overridden flag only if grade or feedback not changed When I hit F5 the string changes as follows indicating that the changed feedback was saved the first time we tried. string(74) "<div class=" text_to_html"> <p>could be a little more thorough</p> </div>" string(74) "<div class=" text_to_html"> <p>could be a little more thorough</p> </div>" However, something at some point, possibly during the regrading (Im guessing here), is cleaning the feedback string and stripping off the div around the p tags ie the changed feedback string gets saved, you repeat the action, this time the changed overridden flag is observed and regrading occurs and the feedback string gets cleaned. Something like that is going on.
          Hide
          Andrew Davis added a comment -

          That extra div is being added by text_to_html($text, $smiley_ignored=null, $para=true, $newlines=true). $para needs to be set to false.

          Show
          Andrew Davis added a comment - That extra div is being added by text_to_html($text, $smiley_ignored=null, $para=true, $newlines=true). $para needs to be set to false.
          Hide
          Andrew Davis added a comment - - edited

          I think I've figured it out. It sent me on a merry goose chase but the solution was actually pretty simple. Before the grade item was being passed into an mform format_text() is being called on the feedback text. The 'para' parameter wasn't being supplied and it defaults to true so the feedback was being incorrectly wrapped in a div making it look like the feedback had changed which prevents the override flag from being changed.

          I've added a branch for master and testing instructions. Ill create the rest of the branches after peer review.

          Show
          Andrew Davis added a comment - - edited I think I've figured it out. It sent me on a merry goose chase but the solution was actually pretty simple. Before the grade item was being passed into an mform format_text() is being called on the feedback text. The 'para' parameter wasn't being supplied and it defaults to true so the feedback was being incorrectly wrapped in a div making it look like the feedback had changed which prevents the override flag from being changed. I've added a branch for master and testing instructions. Ill create the rest of the branches after peer review.
          Hide
          Andrew Davis added a comment -

          Adrian, you seem like a suitable peer reviewer as you're already familiar with the issue. Hope you don't mind me volunteering you

          Show
          Andrew Davis added a comment - Adrian, you seem like a suitable peer reviewer as you're already familiar with the issue. Hope you don't mind me volunteering you
          Hide
          Adrian Greeve added a comment -

          I had a look and that fixes the problem nicely.

          Show
          Adrian Greeve added a comment - I had a look and that fixes the problem nicely.
          Hide
          Andrew Davis added a comment -

          Submitting for integration. Aparup has kindly offered to get it into 2.0 and 2.1 for me.

          Show
          Andrew Davis added a comment - Submitting for integration. Aparup has kindly offered to get it into 2.0 and 2.1 for me.
          Hide
          Aparup Banerjee added a comment - - edited

          Andrew,
          I've confirmed that this works for new or no grade item columns. This doesn't work for me for (even trying multiple times) for existing grade items.

          I'm getting difference of 'n' , perhaps we need to turn off newlines as well?

          differences of text seen in my test:

          <p>Good work, though a bit simple. It would be better something like this:</p>n<p>:-P</p>
          <p>Good work, though a bit simple. It would be better something like this:</p><p>:-P</p>

          Show
          Aparup Banerjee added a comment - - edited Andrew, I've confirmed that this works for new or no grade item columns. This doesn't work for me for (even trying multiple times) for existing grade items. I'm getting difference of 'n' , perhaps we need to turn off newlines as well? differences of text seen in my test: <p>Good work, though a bit simple. It would be better something like this:</p>n<p>:-P</p> <p>Good work, though a bit simple. It would be better something like this:</p><p>:-P</p>
          Hide
          Aparup Banerjee added a comment -

          I think it may be better here to approach this by detecting and flagging changes to feedback ($data->feedback === $old_grade_grade->feedback) before we start mangling it with clean_text() .

          Show
          Aparup Banerjee added a comment - I think it may be better here to approach this by detecting and flagging changes to feedback ($data->feedback === $old_grade_grade->feedback) before we start mangling it with clean_text() .
          Hide
          Andrew Davis added a comment -

          yeah, you may be right. Bounce this back to me and Ill fix it up.

          Show
          Andrew Davis added a comment - yeah, you may be right. Bounce this back to me and Ill fix it up.
          Hide
          Aparup Banerjee added a comment -

          back to you MAn!

          Show
          Aparup Banerjee added a comment - back to you MAn!
          Hide
          Andrew Davis added a comment - - edited

          Actually I dont think that doing the comparison before the call to format_text() will work. The post format_text() version is the one that is saved so its the output of format_text() we're comparing against.

          Still trying to figure out how you managed to get an 'n' in there

          I also don't think we can turn off new lines as such. The newlines parameter accepted by format_text() does this apparently.

          If true then lines newline breaks will be converted to HTML newline breaks. Default true.
          
          Show
          Andrew Davis added a comment - - edited Actually I dont think that doing the comparison before the call to format_text() will work. The post format_text() version is the one that is saved so its the output of format_text() we're comparing against. Still trying to figure out how you managed to get an 'n' in there I also don't think we can turn off new lines as such. The newlines parameter accepted by format_text() does this apparently. If true then lines newline breaks will be converted to HTML newline breaks. Default true .
          Hide
          Andrew Davis added a comment -

          Note also that the value of the overriden flag is explicitly thrown away if you have modified the feedback. you can do one or the other but not both at the same time. The code involved dates back to 2008 so its probable this whole are needs revisiting. I've linked a related issue.

          Show
          Andrew Davis added a comment - Note also that the value of the overriden flag is explicitly thrown away if you have modified the feedback. you can do one or the other but not both at the same time. The code involved dates back to 2008 so its probable this whole are needs revisiting. I've linked a related issue.
          Hide
          Aparup Banerjee added a comment -

          Just attaching the backup that i passed to Andrew.

          Show
          Aparup Banerjee added a comment - Just attaching the backup that i passed to Andrew.
          Hide
          Andrew Davis added a comment - - edited

          I'm unable to reproduce the problem Aparup reported. I'm going to try and get the peer reviewer (sorry Adrian) to test this and see what happens. Even with the potential edge case Aparup encountered this seems like a clear improvement. I've linked a related issue that involves coming back and giving this area a general clean up. In particular the interlocking of feedback and overriding appears to be unnecessary and a source of odd behavior.

          Show
          Andrew Davis added a comment - - edited I'm unable to reproduce the problem Aparup reported. I'm going to try and get the peer reviewer (sorry Adrian) to test this and see what happens. Even with the potential edge case Aparup encountered this seems like a clear improvement. I've linked a related issue that involves coming back and giving this area a general clean up. In particular the interlocking of feedback and overriding appears to be unnecessary and a source of odd behavior.
          Hide
          Aparup Banerjee added a comment - - edited

          hm, we could signal users that overridden isn't going to be effective if feedback is changed , which could be very confusing. - thats reason for some additional js there i'd think.
          Clearly this area needs to be improved much more, lets see what we can do for now and work on the rest in a follow up issue.

          Show
          Aparup Banerjee added a comment - - edited hm, we could signal users that overridden isn't going to be effective if feedback is changed , which could be very confusing. - thats reason for some additional js there i'd think. Clearly this area needs to be improved much more, lets see what we can do for now and work on the rest in a follow up issue.
          Hide
          Adrian Greeve added a comment - - edited

          Tested with the file provided by Aparup and I still need to edit the grade twice.

          Show
          Adrian Greeve added a comment - - edited Tested with the file provided by Aparup and I still need to edit the grade twice.
          Hide
          Andrew Davis added a comment - - edited

          Aparup, Adrian and I conducted a collaborative testing session and it seems that the bug still occurs if the activity module supplies multiline html feedback. Looking for a quick fix now although I may split that out into a separate bug.

          update: the first string is coming from the grade edit form. The second is from the database. New lines have been removed from the second one so they don't match even they that are functionally equivalent.

          string(55) "<p>this is html feedback</p>
          <p>and its multiline.</p>"
          string(53) "<p>this is html feedback</p><p>and its multiline.</p>"
          
          Show
          Andrew Davis added a comment - - edited Aparup, Adrian and I conducted a collaborative testing session and it seems that the bug still occurs if the activity module supplies multiline html feedback. Looking for a quick fix now although I may split that out into a separate bug. update: the first string is coming from the grade edit form. The second is from the database. New lines have been removed from the second one so they don't match even they that are functionally equivalent. string(55) "<p> this is html feedback</p> <p>and its multiline.</p>" string(53) "<p> this is html feedback</p><p>and its multiline.</p>"
          Hide
          Andrew Davis added a comment -

          There doesnt look like theres a quick fix for the multiline feedback problem. I suggest we leave that until MDL-18095 and push this fix through as it at least solves one of the potential causes of this problem.

          Show
          Andrew Davis added a comment - There doesnt look like theres a quick fix for the multiline feedback problem. I suggest we leave that until MDL-18095 and push this fix through as it at least solves one of the potential causes of this problem.
          Hide
          Aparup Banerjee added a comment -

          bringing this in (mdlqa)

          Show
          Aparup Banerjee added a comment - bringing this in (mdlqa)
          Hide
          Aparup Banerjee added a comment - - edited

          Thanks, this has been integrated into 2.x stables and master.

          Tested and the fix works for me. (except the multi line issue which will be dealt with in linked issue)
          please test.

          Show
          Aparup Banerjee added a comment - - edited Thanks, this has been integrated into 2.x stables and master. Tested and the fix works for me. (except the multi line issue which will be dealt with in linked issue) please test.
          Hide
          Rajesh Taneja added a comment -

          Works Great Andrew
          Thanks for fixing this.

          FYI:
          Test is done without adding feedback (As mentioned in Test instructions)

          Show
          Rajesh Taneja added a comment - Works Great Andrew Thanks for fixing this. FYI: Test is done without adding feedback (As mentioned in Test instructions)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this has landed upstream, just on time for the upcoming new releases week. Thanks for it!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this has landed upstream, just on time for the upcoming new releases week. Thanks for it! Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: