Moodle
  1. Moodle
  2. MDL-35392

Feedback from module assign is not shown in the gradebook.

    Details

    • Testing Instructions:
      Hide

      Copying Michael Woods excellent reproduction steps here.

      Testing Steps:

      1. Create a brand new assignment and ensure 'Use Marking Workflow' = Yes.
      2. Go to the assignment grading screen and ensure 'Quick Grading' is ticked.
      3. Give a grade and some feedback to a single student, using the quick grading fields on the screen. Do not change the workflow state to released yet.
      4. Check in either the grader report or user report to ensure that neither the grade or feedback has flowed through yet. This is correct as we haven't released the grade yet.
      5. Back on the assignment grading screen, tick the student that you previously graded (the checkbox in the very first 'Select' column) and down the bottom, change 'With selected...' dropdown box to be 'Set Marking Workflow state' and Go.
      6. Change 'Marking Workflow State' dropdown selection to be 'Released' and click 'Save Changes'.
      7. Now check in either the grader report or user report.

      Expected Result: The grade and feedback comment would be present in the Grader Report and User Report, as the workflow status has been set to released.

      Note: I began to write behat tests but unfortunately, as there is a dialogue box that loads when you select and try to update work flow I cannot write working tests for this unless we alter that page.

      Show
      Copying Michael Woods excellent reproduction steps here. Testing Steps: Create a brand new assignment and ensure 'Use Marking Workflow' = Yes. Go to the assignment grading screen and ensure 'Quick Grading' is ticked. Give a grade and some feedback to a single student, using the quick grading fields on the screen. Do not change the workflow state to released yet. Check in either the grader report or user report to ensure that neither the grade or feedback has flowed through yet. This is correct as we haven't released the grade yet. Back on the assignment grading screen, tick the student that you previously graded (the checkbox in the very first 'Select' column) and down the bottom, change 'With selected...' dropdown box to be 'Set Marking Workflow state' and Go. Change 'Marking Workflow State' dropdown selection to be 'Released' and click 'Save Changes'. Now check in either the grader report or user report. Expected Result: The grade and feedback comment would be present in the Grader Report and User Report, as the workflow status has been set to released. Note: I began to write behat tests but unfortunately, as there is a dialogue box that loads when you select and try to update work flow I cannot write working tests for this unless we alter that page.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE, MOODLE_28_STABLE
    • Fixed Branches:
      MOODLE_27_STABLE, MOODLE_28_STABLE
    • Pull from Repository:
    • Pull 2.7 Branch:
    • Pull 2.8 Branch:
    • Pull Master Branch:
      MDL-35392-master
    • Sprint:
      Team Beards Sprint 3
    • Issue size:
      Small

      Description

      After grading an assignment and providing feedback, if the teacher then goes to the grade book and views the grades in the edit view, the feedback will not be present.
      If the teacher then enters feedback in the grade book, this feedback will not be present in the assignment area.

      The issue here is that in mod/assign/locallib.php there is get_user_grades_for_gradebook() which is doing a check for $assign->feedback instead of the value provided in $assign->feedbacktext. Rather than changing the API, we're changing the code which utilises that API so that we supply $assign->feedback.

      As this is used only by the bulk updating code for when we alter a students current workflow state, the impact and therefore the fix is rather small.

      Replication steps:

      1. Create an assignment with the new assign mod.
      2. Grade a students assignment and type in some feedback.
      3. Go to [Settings ► Grader report], turn on editing and select the graded mark.
      4. Notice that no feedback is shown.
      5. You can enter some feedback here and save it.
      6. Going back to the assign module and viewing the feedback there will show that the text entered from the grade book is not present.

        Gliffy Diagrams

        1. 1.jpg
          250 kB
        2. 2.jpg
          285 kB
        3. 3.jpg
          251 kB
        4. 4.jpg
          225 kB
        5. 5.jpg
          323 kB
        6. 6.jpg
          215 kB

          Issue Links

            Activity

            Hide
            Michael de Raadt added a comment -

            I was able to replicate half of this.

            I turned the quick feedback option to yes in the Grader report options, but I was able to edit the mark and the feedback without error. When I returned to the assignment, the edited feedback was not replicated there. That's not good.

            Was there some specific circumstances that led to your error? We might need more information to reproduce this.

            Show
            Michael de Raadt added a comment - I was able to replicate half of this. I turned the quick feedback option to yes in the Grader report options, but I was able to edit the mark and the feedback without error. When I returned to the assignment, the edited feedback was not replicated there. That's not good. Was there some specific circumstances that led to your error? We might need more information to reproduce this.
            Hide
            Adrian Greeve added a comment -

            After talking with Michael about this issue, I've divided it up into two separate issues. See MDL-35417 for the error message that I was getting.

            Show
            Adrian Greeve added a comment - After talking with Michael about this issue, I've divided it up into two separate issues. See MDL-35417 for the error message that I was getting.
            Hide
            anon added a comment - - edited

            I think this is related to the same issue as MDL-35398. The concept of "Quick Feedback" is not being displayed in User Reports or the Gradebook. It only will show there if a Teacher uses the TinyMCE editor to leave "Slow Feedback".

            Show
            anon added a comment - - edited I think this is related to the same issue as MDL-35398 . The concept of "Quick Feedback" is not being displayed in User Reports or the Gradebook. It only will show there if a Teacher uses the TinyMCE editor to leave "Slow Feedback".
            Hide
            Adrian Greeve added a comment -

            Thanks for the comments Jeremy,

            I didn't have quick feedback enabled when I discovered this issue. I was on the edit screen (grade/edit/tree/grade.php) So I was using TinyMCE to leave "Slow Feedback".

            Show
            Adrian Greeve added a comment - Thanks for the comments Jeremy, I didn't have quick feedback enabled when I discovered this issue. I was on the edit screen (grade/edit/tree/grade.php) So I was using TinyMCE to leave "Slow Feedback".
            Hide
            anon added a comment -

            Hi Adrian, yes I think it's important to note that feedback will be displayed in the grade book if a teacher uses the TinyMCE editor. If the teacher uses the so-called "Quick Feedback" text boxes to leave it, then that feedback will not be shown in the gradebook. It's "slow feedback" because to it would take 200 postbacks to grade students this way.

            What's interesting is that while quick feedback doesn't not show in User Reports/Gradebook, it does persist to the database. Students have to drill down into each assignment in their User Report to see the "quick feedback". As a high school teacher, this is problematic since we often want to print off a User Report for each student, and parents want to see something in the feedback column when a student fails an assignment.

            Show
            anon added a comment - Hi Adrian, yes I think it's important to note that feedback will be displayed in the grade book if a teacher uses the TinyMCE editor. If the teacher uses the so-called "Quick Feedback" text boxes to leave it, then that feedback will not be shown in the gradebook. It's "slow feedback" because to it would take 200 postbacks to grade students this way. What's interesting is that while quick feedback doesn't not show in User Reports/Gradebook, it does persist to the database. Students have to drill down into each assignment in their User Report to see the "quick feedback". As a high school teacher, this is problematic since we often want to print off a User Report for each student, and parents want to see something in the feedback column when a student fails an assignment.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            U P S T R E A M I Z E D !

            Many thanks, this is now available in all the repos (git & cvs).

            Closing, ciao

            Show
            Eloy Lafuente (stronk7) added a comment - U P S T R E A M I Z E D ! Many thanks, this is now available in all the repos (git & cvs). Closing, ciao
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Doh,

            somehow this issue was closed incorrectly when processing all the integrated issues this week. (sort of most voted and current in integration filters mix). Apologies for the confusion, reseting to previous status!

            Ciao, Eloy

            Show
            Eloy Lafuente (stronk7) added a comment - Doh, somehow this issue was closed incorrectly when processing all the integrated issues this week. (sort of most voted and current in integration filters mix). Apologies for the confusion, reseting to previous status! Ciao, Eloy
            Hide
            Jim Judges added a comment -

            Is this related to this issue?:
            https://tracker.moodle.org/browse/MDL-33168

            Show
            Jim Judges added a comment - Is this related to this issue?: https://tracker.moodle.org/browse/MDL-33168
            Hide
            Damyon Wiese added a comment -

            This just needs to be designed properly - currently the assignment only pushes feedback to the gradebook - it does not display the feedback that has been edited in the gradebook within the assignment.

            Show
            Damyon Wiese added a comment - This just needs to be designed properly - currently the assignment only pushes feedback to the gradebook - it does not display the feedback that has been edited in the gradebook within the assignment.
            Hide
            Stephen Overall added a comment -

            I see the roadmap shows that with 2.3.8 the 2.3 branch goes on "security fixes only" July 8, 2013. Any chance this bug gets some love before then?

            Show
            Stephen Overall added a comment - I see the roadmap shows that with 2.3.8 the 2.3 branch goes on "security fixes only" July 8, 2013. Any chance this bug gets some love before then?
            Hide
            Stephen Gormley added a comment -

            This issue is affecting 2.4.4 as well.

            Show
            Stephen Gormley added a comment - This issue is affecting 2.4.4 as well.
            Hide
            B. Friesen added a comment -

            We just upgraded from 2.2.4 to 2.5.2 and are experiencing this! I hope there's a solution somewhere.

            Show
            B. Friesen added a comment - We just upgraded from 2.2.4 to 2.5.2 and are experiencing this! I hope there's a solution somewhere.
            Hide
            Michael Woods added a comment - - edited

            This could still be a bug in 2.6.2+. I'm investigating an instance that is very similar to this now.

            Show
            Michael Woods added a comment - - edited This could still be a bug in 2.6.2+. I'm investigating an instance that is very similar to this now.
            Hide
            Michael Woods added a comment -

            I think what I found was an anomaly. Sorry for the false alarm.

            Show
            Michael Woods added a comment - I think what I found was an anomaly. Sorry for the false alarm.
            Hide
            Rob Monk added a comment -

            We are using Moodle 2.6.2+ (Build: 20140508) and this bug is alive and well.

            A teacher showed me detailed feedback she had given via the quick grading assignment screen.
            It does not show up in the feedback section of the user report.

            This is very annoying to staff who have put the work into giving the feedback. The only way students will see it is to click on the link to the assignment in the gradebook.

            Show
            Rob Monk added a comment - We are using Moodle 2.6.2+ (Build: 20140508) and this bug is alive and well. A teacher showed me detailed feedback she had given via the quick grading assignment screen. It does not show up in the feedback section of the user report. This is very annoying to staff who have put the work into giving the feedback. The only way students will see it is to click on the link to the assignment in the gradebook.
            Hide
            Cameron Banfield added a comment - - edited

            I can confirm this bug is still present in both Moodle 2.6.1 and 2.6.2. Feedback added in the assignment is not being displayed in the grader / user reports and exports.

            Show
            Cameron Banfield added a comment - - edited I can confirm this bug is still present in both Moodle 2.6.1 and 2.6.2. Feedback added in the assignment is not being displayed in the grader / user reports and exports.
            Hide
            Damyon Wiese added a comment -

            Anyone having this issue - can you report whether the admin setting for mod_assign "feedback_plugin_for_gradebook" is set to "Feedback comments". If it is not - then it should be and that is the cause of your issue.

            I tested this and it is working fine for me.

            Show
            Damyon Wiese added a comment - Anyone having this issue - can you report whether the admin setting for mod_assign "feedback_plugin_for_gradebook" is set to "Feedback comments". If it is not - then it should be and that is the cause of your issue. I tested this and it is working fine for me.
            Hide
            Marius Jugariu added a comment - - edited

            Hi Damyon

            I did the test again and it doesn't work:

            Screenshot 1: the settings are as you suggested
            Screenshot 2: in the assignment inbox I am looking at the student with yellow background in his picture. He has been marked, given feedback and released.
            Screenshot 3: when clicking the grade button I can see the grade and the feedback comments
            Screenshot 4: looking at gradebook with editing off - yellow student has only the mark
            Screenshot 5: looking at gradebook with editing turned on - yellow student still has only mark, no feedback
            Screenshot 6: if I try to edit the grade from gradebook there are no comments in that box

            If I were to do an export it only picks up the grades, no comments included.

            I'm using Moodle 2.6.1+ (Build: 20140117)

            Show
            Marius Jugariu added a comment - - edited Hi Damyon I did the test again and it doesn't work: Screenshot 1: the settings are as you suggested Screenshot 2: in the assignment inbox I am looking at the student with yellow background in his picture. He has been marked, given feedback and released. Screenshot 3: when clicking the grade button I can see the grade and the feedback comments Screenshot 4: looking at gradebook with editing off - yellow student has only the mark Screenshot 5: looking at gradebook with editing turned on - yellow student still has only mark, no feedback Screenshot 6: if I try to edit the grade from gradebook there are no comments in that box If I were to do an export it only picks up the grades, no comments included. I'm using Moodle 2.6.1+ (Build: 20140117)
            Hide
            Antton Rodriguez added a comment -

            We are having the same problem on 2.6.5+ at least if we use the workflow option.

            Show
            Antton Rodriguez added a comment - We are having the same problem on 2.6.5+ at least if we use the workflow option.
            Hide
            Michael Woods added a comment -

            I know exactly how to replicate this issue, in 2.6, 2.7 and now 2.8. Also, the admin setting on our site for mod_assign "feedback_plugin_for_gradebook" is set to "Feedback comments".

            Steps to reproduce error:
            1. Create a brand new assignment and ensure 'Use Marking Workflow' = Yes.
            2. Go to the assignment grading screen and ensure 'Quick Grading' is ticked.
            3. Give a grade and some feedback to a single student, using the quick grading fields on the screen. Do not change the workflow state to released yet.
            4. Check in either the grader report or user report to ensure that neither the grade or feedback has flowed through yet. This is correct as we haven't released the grade yet.
            5. Back on the assignment grading screen, tick the student that you previously graded (the checkbox in the very first 'Select' column) and down the bottom, change 'With selected...' dropdown box to be 'Set Marking Workflow state' and Go.
            6. Change 'Marking Workflow State' dropdown selection to be 'Released' and click 'Save Changes'.
            7. Now check in either the grader report or user report.

            Expected result: The grade and feedback comment would be present in the Grader Report and User Report, as the workflow status has been set to released.
            Actual result: Only the grade is present - not the feedback comment.

            Can this be looked at urgently? It's a big deal for our teachers with hundreds of students in their course, who have finally mastered batch releasing their grades, only to discover that their comments can't be read by students.

            The workaround to fix it (as others have mentioned) is to go into 'slow' grading and simply click 'Save Changes' or 'Save and show next' for every single submission. This writes the feedback to the gradebook.

            The other workaround is to individually set the workflow status via the quick grading dropdown selections at the same time as you enter your quick grading and quick feedback data.

            So, the only time it seems to really fail is when attempting to do the 'batch' set workflow status method, even for a single student.

            Show
            Michael Woods added a comment - I know exactly how to replicate this issue, in 2.6, 2.7 and now 2.8. Also, the admin setting on our site for mod_assign "feedback_plugin_for_gradebook" is set to "Feedback comments". Steps to reproduce error: 1. Create a brand new assignment and ensure 'Use Marking Workflow' = Yes. 2. Go to the assignment grading screen and ensure 'Quick Grading' is ticked. 3. Give a grade and some feedback to a single student, using the quick grading fields on the screen. Do not change the workflow state to released yet. 4. Check in either the grader report or user report to ensure that neither the grade or feedback has flowed through yet. This is correct as we haven't released the grade yet. 5. Back on the assignment grading screen, tick the student that you previously graded (the checkbox in the very first 'Select' column) and down the bottom, change 'With selected...' dropdown box to be 'Set Marking Workflow state' and Go. 6. Change 'Marking Workflow State' dropdown selection to be 'Released' and click 'Save Changes'. 7. Now check in either the grader report or user report. Expected result: The grade and feedback comment would be present in the Grader Report and User Report, as the workflow status has been set to released. Actual result: Only the grade is present - not the feedback comment. Can this be looked at urgently? It's a big deal for our teachers with hundreds of students in their course, who have finally mastered batch releasing their grades, only to discover that their comments can't be read by students. The workaround to fix it (as others have mentioned) is to go into 'slow' grading and simply click 'Save Changes' or 'Save and show next' for every single submission. This writes the feedback to the gradebook. The other workaround is to individually set the workflow status via the quick grading dropdown selections at the same time as you enter your quick grading and quick feedback data. So, the only time it seems to really fail is when attempting to do the 'batch' set workflow status method, even for a single student.
            Hide
            CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-35392 using repository: git://github.com/zbdd/moodle.git

            More information about this report

            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-35392 using repository: git://github.com/zbdd/moodle.git master (3 errors / 0 warnings) [branch: MDL-35392-master | CI Job ] phplint (0/0) , php (0/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (3/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            Frédéric Massart added a comment -

            Hi Zac,

            thanks for working on this issue, fixing it will make many people happy .

            I have not tried your patch, I have tried to understand your changes going through the code and experimenting myself. In the future it would be helpful if you could describe the solution in a comment in the issue, that helps when someone reviews the patch.

            While your patch might work (as I said I have not tried it), it seemed a bit strange that mod_assign has been using the wrong property name for that long without anyone noticing so I started my investigations.

            As the testing instructions demonstrate, this issue only happen when you bulk change the workflow. Changing the workflow using quick grading works fine. So I noticed that there was a method called gradebook_item_update() which calls convert_grade_for_gradebook() before sending the grade to the gradebook. The latter is converting feedbacktext to feedback, that seems to indicate that feedbacktext is OK to use.

            Now, if we look at how the bulk marking workflow works, we end up in assign_update_grades() which ends up calling mod_assign::get_user_grades_for_gradebook(). There we were wrongly using feedbacktext as the data is supposed to be in the right format, you fixed that and that is right I think.

            Looking at assign_update_grades() it looks like it would not behave properly when the grade was set to -1, I think that the gradebook expects null. Also, it does not seem that it would remove a feedback from the gradebook if the grade was empty. Those are different issues but maybe they are worth being investigated a bit. Those methods are called from lib/gradelib.php as callbacks.

            Back to your patch:

            1. Could you justify the changes of feedbacktext everywhere? I'd be inclined to only change the code causing issues but if you have a good reason then it's perfectly acceptable to change them everywhere.
              • If you keep your changes, you will have to update the testing instructions to cover each of those changes. And probably fix some Unit Tests too.
            2. Now would be a good time to expand a bit the associated Unit Tests to confirm that the gradebook formated grades come in the right format.
            3. Please note CIBOT's errors

            Thanks!
            Fred

            Show
            Frédéric Massart added a comment - Hi Zac, thanks for working on this issue, fixing it will make many people happy . I have not tried your patch, I have tried to understand your changes going through the code and experimenting myself. In the future it would be helpful if you could describe the solution in a comment in the issue, that helps when someone reviews the patch. While your patch might work (as I said I have not tried it), it seemed a bit strange that mod_assign has been using the wrong property name for that long without anyone noticing so I started my investigations. As the testing instructions demonstrate, this issue only happen when you bulk change the workflow. Changing the workflow using quick grading works fine. So I noticed that there was a method called gradebook_item_update() which calls convert_grade_for_gradebook() before sending the grade to the gradebook. The latter is converting feedbacktext to feedback , that seems to indicate that feedbacktext is OK to use. Now, if we look at how the bulk marking workflow works, we end up in assign_update_grades() which ends up calling mod_assign::get_user_grades_for_gradebook(). There we were wrongly using feedbacktext as the data is supposed to be in the right format, you fixed that and that is right I think. Looking at assign_update_grades() it looks like it would not behave properly when the grade was set to -1, I think that the gradebook expects null . Also, it does not seem that it would remove a feedback from the gradebook if the grade was empty. Those are different issues but maybe they are worth being investigated a bit. Those methods are called from lib/gradelib.php as callbacks. Back to your patch: Could you justify the changes of feedbacktext everywhere? I'd be inclined to only change the code causing issues but if you have a good reason then it's perfectly acceptable to change them everywhere. If you keep your changes, you will have to update the testing instructions to cover each of those changes. And probably fix some Unit Tests too. Now would be a good time to expand a bit the associated Unit Tests to confirm that the gradebook formated grades come in the right format. Please note CIBOT's errors Thanks! Fred
            Hide
            Zachary Durber added a comment -

            Thanks for the feedback Fred.

            1. I was incorrectly assuming that feedbacktext was just a local way of referring to the feedback in mod_assign and it's actually pretty standard across the core. I've altered my code so that I've only changed it where it needs to be. (get_user_grades_for_gradebook)
            2. Will look into some unit tests.
            3. CiBoT errors were fixed previously, I just forgot to cime again.
            4. Re the other items, a grade doesn't have to be submitted for feedback to be valid and left for a user, or would a null grade remove feedback from the gradebook. I don't think that behaviour is unwarranted.
            5. In this issue, we are not setting a grade when changing the workflow to released. I did go back and use the quick-saving feature with -1 as a grade and it worked correctly still
              1. Test 1. Quick save with grade as -1, feedback set and workflowstate to released. Worked!
              2. Test 2. Quick save with grade as -1, feedback set and workflowstate to unmarked. Then using the workflowstate dropdown to change to released. Worked!
                (by worked I mean feedback appeared in the gradebook when released only and not before)
            Show
            Zachary Durber added a comment - Thanks for the feedback Fred. I was incorrectly assuming that feedbacktext was just a local way of referring to the feedback in mod_assign and it's actually pretty standard across the core. I've altered my code so that I've only changed it where it needs to be. (get_user_grades_for_gradebook) Will look into some unit tests. CiBoT errors were fixed previously, I just forgot to cime again. Re the other items, a grade doesn't have to be submitted for feedback to be valid and left for a user, or would a null grade remove feedback from the gradebook. I don't think that behaviour is unwarranted. In this issue, we are not setting a grade when changing the workflow to released. I did go back and use the quick-saving feature with -1 as a grade and it worked correctly still Test 1. Quick save with grade as -1, feedback set and workflowstate to released. Worked! Test 2. Quick save with grade as -1, feedback set and workflowstate to unmarked. Then using the workflowstate dropdown to change to released. Worked! (by worked I mean feedback appeared in the gradebook when released only and not before)
            Hide
            Zachary Durber added a comment -

            Going to push ahead with this as is now. Spent some time looking into writing unit tests but I'd argue we should fix the inability to run behats with this kind of problem instead.
            Writing unit tests will take sometime and it will still on cover a small subset of functionality to check this 1 line change, something behat would do better.

            Show
            Zachary Durber added a comment - Going to push ahead with this as is now. Spent some time looking into writing unit tests but I'd argue we should fix the inability to run behats with this kind of problem instead. Writing unit tests will take sometime and it will still on cover a small subset of functionality to check this 1 line change, something behat would do better.
            Hide
            Zachary Durber added a comment -

            MDL-48607 raised regarding my inability to behat test this.

            Show
            Zachary Durber added a comment - MDL-48607 raised regarding my inability to behat test this.
            Hide
            Dan Poltawski 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
            Dan Poltawski 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
            Zachary Durber added a comment -

            Rebased.

            Show
            Zachary Durber added a comment - Rebased.
            Hide
            CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            CiBoT added a comment -

            Code verified against automated checks.

            Checked MDL-35392 using repository: git://github.com/zbdd/moodle.git

            More information about this report

            Show
            CiBoT added a comment - Code verified against automated checks. Checked MDL-35392 using repository: git://github.com/zbdd/moodle.git MOODLE_27_STABLE (0 errors / 0 warnings) [branch: MDL-35392-27 | CI Job ] MOODLE_28_STABLE (0 errors / 0 warnings) [branch: MDL-35392-28 | CI Job ] master (0 errors / 0 warnings) [branch: MDL-35392-master | CI Job ] More information about this report
            Hide
            Andrew Nicols added a comment -

            Hi Zac,

            Thanks for working on this issue. From reading the history of this issue, we're still lacking a detailed description of the change in the comments here - any chance you could add one.

            As Fred states:

            Now, if we look at how the bulk marking workflow works, we end up in assign_update_grades() which ends up calling mod_assign::get_user_grades_for_gradebook(). There we were wrongly using feedbacktext as the data is supposed to be in the right format, you fixed that and that is right I think.

            From what I can see, there are several other times in which we use feedbacktext within the assignment module. These include:

            1. locallib.php process_save_quick_grades
            2. locallib.php reveal_identities
            3. locallib.php apply_grade_to_user
            4. feedback/offline/locallib.php process_import_grades

            In each of these cases, we are passing the result of text_for_gradebook to feedbacktext, and setting the format, for example:

                                        $grade->feedbacktext = $plugin->text_for_gradebook($grade);
                                        $grade->feedbackformat = $plugin->format_for_gradebook($grade);
            

            Do these need to be addressed too, and can you confirm that each is working as expected?

            Cheers,

            Andrew

            Show
            Andrew Nicols added a comment - Hi Zac, Thanks for working on this issue. From reading the history of this issue, we're still lacking a detailed description of the change in the comments here - any chance you could add one. As Fred states: Now, if we look at how the bulk marking workflow works, we end up in assign_update_grades() which ends up calling mod_assign::get_user_grades_for_gradebook(). There we were wrongly using feedbacktext as the data is supposed to be in the right format, you fixed that and that is right I think. From what I can see, there are several other times in which we use feedbacktext within the assignment module. These include: locallib.php process_save_quick_grades locallib.php reveal_identities locallib.php apply_grade_to_user feedback/offline/locallib.php process_import_grades In each of these cases, we are passing the result of text_for_gradebook to feedbacktext, and setting the format, for example: $grade->feedbacktext = $plugin->text_for_gradebook($grade); $grade->feedbackformat = $plugin->format_for_gradebook($grade); Do these need to be addressed too, and can you confirm that each is working as expected? Cheers, Andrew
            Hide
            Andrew Nicols added a comment -

            Failing this from integration. I should have done this before Christmas but missed it in the Christmas Vacation rush. Sorry!

            Show
            Andrew Nicols added a comment - Failing this from integration. I should have done this before Christmas but missed it in the Christmas Vacation rush. Sorry!
            Hide
            CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            Zachary Durber added a comment -

            This is now blocked by MDL-48884. Cannot test properly as quickgrading feedback for a assignment submission releases the feedback to the gradebook straight away.

            Show
            Zachary Durber added a comment - This is now blocked by MDL-48884 . Cannot test properly as quickgrading feedback for a assignment submission releases the feedback to the gradebook straight away.
            Hide
            Jim Judges added a comment -

            We experienced a similar problem with our current 2.6.2 but I noticed something that could help others. If you turn quick grading on (whether you have used it or not) and check for any unexpected HTML characters e.g. in the Feedback comments I saw:

            <p>Well done........etc </p>

            When I deleted the HTML tags for just one comment and then clicked "Save all quick grading changes" this feedback did then display in the gradebook, for all assignments.

            This may be unconnected to the main issue but it may be worth checking if this helps anyone else as a workaround.

            Show
            Jim Judges added a comment - We experienced a similar problem with our current 2.6.2 but I noticed something that could help others. If you turn quick grading on (whether you have used it or not) and check for any unexpected HTML characters e.g. in the Feedback comments I saw: <p>Well done........etc </p> When I deleted the HTML tags for just one comment and then clicked "Save all quick grading changes" this feedback did then display in the gradebook, for all assignments. This may be unconnected to the main issue but it may be worth checking if this helps anyone else as a workaround.
            Hide
            Zachary Durber added a comment -

            Hi Andrew,

            To clarify the problem, it's because the batch processing code utilises the function assign_update_grades() which is not where the rest of the quickgrading steps through. As Fred mentioned the batch processing code steps through Line 7046 of mod/assign/locallib.php (get_user_grades_for_gradebook()).

            Here it's doing a check for $assign->feedback (line 7094) which doesn't exist as it's $assign->feedbacktext.
            Rather than changing this API which could effect plugins I've decided just to correct the code utilising it. (Hence the small change in the patch)

            The other processes here are not affected, although I'll add additional steps to ensure this during testing.

            Show
            Zachary Durber added a comment - Hi Andrew, To clarify the problem, it's because the batch processing code utilises the function assign_update_grades() which is not where the rest of the quickgrading steps through. As Fred mentioned the batch processing code steps through Line 7046 of mod/assign/locallib.php (get_user_grades_for_gradebook()). Here it's doing a check for $assign->feedback (line 7094) which doesn't exist as it's $assign->feedbacktext. Rather than changing this API which could effect plugins I've decided just to correct the code utilising it. (Hence the small change in the patch) The other processes here are not affected, although I'll add additional steps to ensure this during testing.
            Hide
            Zachary Durber added a comment -

            I tested this again based off MDL-48884 and it works as expected. Pushing back to Integration Review.
            I did offline-grading testing too, that is not affected as it doesn't listen to marking work flow changes anyway.

            Noting again that this is blocked by MDL-48884.

            Show
            Zachary Durber added a comment - I tested this again based off MDL-48884 and it works as expected. Pushing back to Integration Review. I did offline-grading testing too, that is not affected as it doesn't listen to marking work flow changes anyway. Noting again that this is blocked by MDL-48884 .
            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
            CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            CiBoT added a comment -

            Code verified against automated checks.

            Checked MDL-35392 using repository: git://github.com/zbdd/moodle.git

            More information about this report

            Show
            CiBoT added a comment - Code verified against automated checks. Checked MDL-35392 using repository: git://github.com/zbdd/moodle.git MOODLE_27_STABLE (0 errors / 0 warnings) [branch: MDL-35392-27 | CI Job ] MOODLE_28_STABLE (0 errors / 0 warnings) [branch: MDL-35392-28 | CI Job ] master (0 errors / 0 warnings) [branch: MDL-35392-master | CI Job ] More information about this report
            Hide
            Andrew Nicols added a comment -

            Hi Zac,

            As commented before, the actual changes and rationale behind them needs to be mentioned in this issue. As it stands, someone trying to understand this change is going to really struggle to see why this change fixes the problem.

            From what I can tell, and based on your comment on the 16th February, the issue here is that get_user_grades_for_gradebook() is doing a check for $assign->feedback instead of the value provided in $assign->feedbacktext. Rather than changing the API (ho ho ho), we're changing the code which utilises that API so that we supply $assign->feedback.

            Can you confirm that is correct? If so, please add it to the description of the issue.
            Additionally, can you please update the commit message to explain the change rather than the symptom. IMO, the feedback not being pushed to the gradebook is a symptom of the issue (i.e. it's the bug), and in this case the two are very difficult to tie together.

            Holding this in integration until the above are addressed.

            Andrew

            Show
            Andrew Nicols added a comment - Hi Zac, As commented before, the actual changes and rationale behind them needs to be mentioned in this issue. As it stands, someone trying to understand this change is going to really struggle to see why this change fixes the problem. From what I can tell, and based on your comment on the 16th February, the issue here is that get_user_grades_for_gradebook() is doing a check for $assign->feedback instead of the value provided in $assign->feedbacktext. Rather than changing the API (ho ho ho), we're changing the code which utilises that API so that we supply $assign->feedback. Can you confirm that is correct? If so, please add it to the description of the issue. Additionally, can you please update the commit message to explain the change rather than the symptom. IMO, the feedback not being pushed to the gradebook is a symptom of the issue (i.e. it's the bug), and in this case the two are very difficult to tie together. Holding this in integration until the above are addressed. Andrew
            Hide
            Andrew Nicols added a comment -

            Thanks for working on this Zac and for making these changes.

            I've integrated it now.

            Integrated to 27, 28, and master

            Show
            Andrew Nicols added a comment - Thanks for working on this Zac and for making these changes. I've integrated it now. Integrated to 27, 28, and master
            Hide
            Mark Nelson added a comment -

            Works as expected, thanks Zac.

            Glad to see this issue get resolved - the solution ended up being so easy. Oh well, better late than never.

            Show
            Mark Nelson added a comment - Works as expected, thanks Zac. Glad to see this issue get resolved - the solution ended up being so easy. Oh well, better late than never.
            Hide
            Dan Poltawski added a comment -

            Thanks for your contributions! This change is now available from the main moodle.git repository and will shortly be available on download.moodle.org.

            At some point software design becomes less about what and more about when.
            – Kent Beck

            Show
            Dan Poltawski added a comment - Thanks for your contributions! This change is now available from the main moodle.git repository and will shortly be available on download.moodle.org. At some point software design becomes less about what and more about when. – Kent Beck

              People

              • Votes:
                64 Vote for this issue
                Watchers:
                39 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Agile