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

Apply grades and feedback to entire group does not work with Annotate PDF feature

    Details

    • Testing Instructions:
      Hide
      1. As teacher, create group assignment with Annotate PDF enabled
      2. As student, submit PDF document on behalf of group
      3. As teacher, provide grade and annotations to PDF document
      4. Save changes with 'Apply grades and feedback to entire group' option set to 'Yes'
      5. As each group member, view assignment feedback.

      The annotated PDF is only visible to the group member whose submission was annotated

      Show
      As teacher, create group assignment with Annotate PDF enabled As student, submit PDF document on behalf of group As teacher, provide grade and annotations to PDF document Save changes with 'Apply grades and feedback to entire group' option set to 'Yes' As each group member, view assignment feedback. The annotated PDF is only visible to the group member whose submission was annotated
    • Workaround:
      Hide

      No easy workaround - possibly downloading annotated PDF, enabling file submissions and returning the annotated PDF to each group member as a file.

      Show
      No easy workaround - possibly downloading annotated PDF, enabling file submissions and returning the annotated PDF to each group member as a file.
    • Affected Branches:
      MOODLE_26_STABLE, MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE, MOODLE_27_STABLE
    • Pull 2.6 Branch:
      m26_MDL-45582
    • Pull 2.7 Branch:
      m27_MDL-45582
    • Pull Master Branch:
      master_MDL-45582

      Description

      When providing feedback on a group assignment using the editpdf (Annotate PDF) and selecting Yes to the 'Apply grades and feedback to entire group' option, the annotated PDF is only returned to the group member being marked. This prevents all group members from being able to see annotated PDF feedback documents.

      This option continues to return grades and other forms of feedback to all group members as expected, this is just related to annotated PDF documents.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            damyon Damyon Wiese added a comment -

            Thanks for reporting this.

            I've put that on the backlog.

            In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.

            Show
            damyon Damyon Wiese added a comment - Thanks for reporting this. I've put that on the backlog. In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.
            Hide
            gregor89 Greg Faller added a comment -

            Hi Damyon Wiese,

            I found that there is a check in the save function that is looking for comments and annotations for each user within the group. The proposed fix checks to see whether applytoall is enabled in the save function, and if so, duplicates the comments and annotations for each additional group member and generates the feedback file.

            Show
            gregor89 Greg Faller added a comment - Hi Damyon Wiese , I found that there is a check in the save function that is looking for comments and annotations for each user within the group. The proposed fix checks to see whether applytoall is enabled in the save function, and if so, duplicates the comments and annotations for each additional group member and generates the feedback file.
            Hide
            cibot CiBoT added a comment -

            Results for MDL-45582

            • Remote repository: git://github.com/greg-or/moodle-mod_assign.git
            Show
            cibot CiBoT added a comment - Results for MDL-45582 Remote repository: git://github.com/greg-or/moodle-mod_assign.git Remote branch m26_ MDL-45582 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3535 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3535/artifact/work/smurf.html Remote branch m27_ MDL-45582 to be integrated into upstream MOODLE_27_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3536 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3536/artifact/work/smurf.html
            Hide
            gregor89 Greg Faller added a comment -

            I've realised that this patch could possibly cause the annotated PDF to be created for each group member each time save() was called.

            Is it possible to identify the user whose PDF document was annotated in the save function when save_grade is looping through each group member when applytoall is enabled?

            Show
            gregor89 Greg Faller added a comment - I've realised that this patch could possibly cause the annotated PDF to be created for each group member each time save() was called. Is it possible to identify the user whose PDF document was annotated in the save function when save_grade is looping through each group member when applytoall is enabled?
            Hide
            gregor89 Greg Faller added a comment -

            I've added a hidden userid parameter to the form when grading a group assignment. This can be used to make sure that the annotated feedback file is only replicated for each group member when saving grade data for the user whose PDF file was annotated.

            Show
            gregor89 Greg Faller added a comment - I've added a hidden userid parameter to the form when grading a group assignment. This can be used to make sure that the annotated feedback file is only replicated for each group member when saving grade data for the user whose PDF file was annotated.
            Hide
            cibot CiBoT added a comment -

            Results for MDL-45582

            • Remote repository: git://github.com/greg-or/moodle-mod_assign.git
            Show
            cibot CiBoT added a comment - Results for MDL-45582 Remote repository: git://github.com/greg-or/moodle-mod_assign.git Remote branch m26_ MDL-45582 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3641 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3641/artifact/work/smurf.html Remote branch m27_ MDL-45582 to be integrated into upstream MOODLE_27_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3642 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3642/artifact/work/smurf.html
            Hide
            fred Frédéric Massart added a comment -

            Just noting that outcomes do not seem to be saved on other students either. Maybe this could be fixed here too.

            Show
            fred Frédéric Massart added a comment - Just noting that outcomes do not seem to be saved on other students either. Maybe this could be fixed here too.
            Hide
            gregor89 Greg Faller added a comment -

            Hi Frédéric Massart,

            Just noting that the outcome issue seems easily fixable by altering process_outcomes() to something like:

                        if ($this->get_instance()->teamsubmission && $formdata->applytoall) {
                            $currentuser = $formdata->userid;
                        } else {
                            $currentuser = $userid;
                        }
                        foreach ($gradinginfo->outcomes as $index => $oldoutcome) {
                            $name = 'outcome_'.$index;
                            if (isset($formdata->{$name}[$currentuser]) &&
                                    $oldoutcome->grades[$userid]->grade != $formdata->{$name}[$currentuser]) {
                                $data[$index] = $formdata->{$name}[$currentuser];
                            }
                        }
            

            I was going to update my branch but noticed that a peer review is in progress.

            Show
            gregor89 Greg Faller added a comment - Hi Frédéric Massart , Just noting that the outcome issue seems easily fixable by altering process_outcomes() to something like: if ($this->get_instance()->teamsubmission && $formdata->applytoall) { $currentuser = $formdata->userid; } else { $currentuser = $userid; } foreach ($gradinginfo->outcomes as $index => $oldoutcome) { $name = 'outcome_'.$index; if (isset($formdata->{$name}[$currentuser]) && $oldoutcome->grades[$userid]->grade != $formdata->{$name}[$currentuser]) { $data[$index] = $formdata->{$name}[$currentuser]; } } I was going to update my branch but noticed that a peer review is in progress.
            Hide
            fred Frédéric Massart added a comment - - edited

            Hi Greg,

            thanks a lot for working on this. I peer reviewed your code and have a few comments, you seem to have solved the issue but I would like to discuss some of its details.

            As per my understanding, the method save_grade is actually the only one that knows whether we are in a team submission or not, and simply calls apply_grade_to_user() for each of the members of the group. That means that the editpdf feedback method save will be called for each member of the group. But because the latter is looking for an edited PDF for the current member, nothing is saved. I do not think that feedback plugins should handle the team submission cases, I think they should just work.

            Do you think we could revise the editpdf::save() method to work regardless. The same way the comments and file feedback plugins work. For example, we could add a form key containing the user ID (or grade ID) of the edited PDF if there is any. What do you think about this?

            I had a look at the outcomes, and the problem is similar but the solution is different, I will raise another issue for that, thanks for having a look too.

            Many thanks!
            Fred

            Show
            fred Frédéric Massart added a comment - - edited Hi Greg, thanks a lot for working on this. I peer reviewed your code and have a few comments, you seem to have solved the issue but I would like to discuss some of its details. As per my understanding, the method save_grade is actually the only one that knows whether we are in a team submission or not, and simply calls apply_grade_to_user() for each of the members of the group. That means that the editpdf feedback method save will be called for each member of the group. But because the latter is looking for an edited PDF for the current member, nothing is saved. I do not think that feedback plugins should handle the team submission cases, I think they should just work. Do you think we could revise the editpdf::save() method to work regardless. The same way the comments and file feedback plugins work. For example, we could add a form key containing the user ID (or grade ID) of the edited PDF if there is any. What do you think about this? I had a look at the outcomes, and the problem is similar but the solution is different, I will raise another issue for that, thanks for having a look too. Many thanks! Fred
            Hide
            fred Frédéric Massart added a comment -

            (I raised MDL-45870 for the outcome issue)

            Show
            fred Frédéric Massart added a comment - (I raised MDL-45870 for the outcome issue)
            Hide
            gregor89 Greg Faller added a comment -

            Hi Frédéric Massart,

            I agree that it doesn't seem ideal for the plugins to be doing processing that has already been done by the save_grades() function, but not sure how to work around this. The main issue being that the user who had annotations applied is not necessary the first user processed through apply_grade_to_user, meaning that several users may be processed before the annotation file is generated (I presume get_submission_group_members sorts by id ASC). This is why I chose to process the annotation feedback file when annotations are discovered and then generate annotations in the database and a file for each subsequent user.

            I think either way we'd need a new function to replicate the annotations in the database for each group member to ensure that a marker can mark any student within a particular group and continue annotating the document regardless of whoever was initially graded. Without copying the annotations and comments for each student, I believe only one student within the group would have editable annotations visible to teachers.

            With this in mind, I'm not sure how much of the processing in the proposed patch could be cut down. What do you think?

            Thanks,
            Greg

            Show
            gregor89 Greg Faller added a comment - Hi Frédéric Massart , I agree that it doesn't seem ideal for the plugins to be doing processing that has already been done by the save_grades() function, but not sure how to work around this. The main issue being that the user who had annotations applied is not necessary the first user processed through apply_grade_to_user, meaning that several users may be processed before the annotation file is generated (I presume get_submission_group_members sorts by id ASC). This is why I chose to process the annotation feedback file when annotations are discovered and then generate annotations in the database and a file for each subsequent user. I think either way we'd need a new function to replicate the annotations in the database for each group member to ensure that a marker can mark any student within a particular group and continue annotating the document regardless of whoever was initially graded. Without copying the annotations and comments for each student, I believe only one student within the group would have editable annotations visible to teachers. With this in mind, I'm not sure how much of the processing in the proposed patch could be cut down. What do you think? Thanks, Greg
            Hide
            fred Frédéric Massart added a comment -

            Hi Greg,

            Yes, there needs to be a function to copy drafts from one user to another, that's true. In regard with the potential solution, perhaps a code example would be more explanatory than me . This is what I had in mind:

            diff --git a/mod/assign/feedback/editpdf/locallib.php b/mod/assign/feedback/editpdf/locallib.php
            index 719e29e..c8b44a8 100644
            --- a/mod/assign/feedback/editpdf/locallib.php
            +++ b/mod/assign/feedback/editpdf/locallib.php
            @@ -178,6 +178,7 @@ class assign_feedback_editpdf extends assign_feedback_plugin {
                         $html = $renderer->render($widget);
                         $mform->addElement('static', 'editpdf', get_string('editpdf', 'assignfeedback_editpdf'), $html);
                         $mform->addHelpButton('editpdf', 'editpdf', 'assignfeedback_editpdf');
            +            $mform->addElement('hidden', 'editpdf_source_userid', $userid);
                     }
                 }
             
            @@ -189,6 +190,10 @@ class assign_feedback_editpdf extends assign_feedback_plugin {
                  * @return bool
                  */
                 public function save(stdClass $grade, stdClass $data) {
            +        $sourceuserid = $data->editpdf_source_userid;
            +        if ($sourceuserid !== $grade->userid) {
            +            document_services::copy_drafts_from_to($sourceuserid, $grade->userid);
            +        }
                     if (page_editor::has_annotations_or_comments($grade->id, true)) {
                         document_services::generate_feedback_document($this->assignment, $grade->userid, $grade->attemptnumber);
                     }
            

            So, in the form itself you would store what user you are editing the PDF of, then if you are saving the grades of another user you would first copy the draft annotations to that user. It would not matter in what order the users are processed as the drafts are never deleted.

            Show
            fred Frédéric Massart added a comment - Hi Greg, Yes, there needs to be a function to copy drafts from one user to another, that's true. In regard with the potential solution, perhaps a code example would be more explanatory than me . This is what I had in mind: diff --git a/mod/assign/feedback/editpdf/locallib.php b/mod/assign/feedback/editpdf/locallib.php index 719e29e..c8b44a8 100644 --- a/mod/assign/feedback/editpdf/locallib.php +++ b/mod/assign/feedback/editpdf/locallib.php @@ -178,6 +178,7 @@ class assign_feedback_editpdf extends assign_feedback_plugin { $html = $renderer->render($widget); $mform->addElement('static', 'editpdf', get_string('editpdf', 'assignfeedback_editpdf'), $html); $mform->addHelpButton('editpdf', 'editpdf', 'assignfeedback_editpdf'); + $mform->addElement('hidden', 'editpdf_source_userid', $userid); } } @@ -189,6 +190,10 @@ class assign_feedback_editpdf extends assign_feedback_plugin { * @return bool */ public function save(stdClass $grade, stdClass $data) { + $sourceuserid = $data->editpdf_source_userid; + if ($sourceuserid !== $grade->userid) { + document_services::copy_drafts_from_to($sourceuserid, $grade->userid); + } if (page_editor::has_annotations_or_comments($grade->id, true)) { document_services::generate_feedback_document($this->assignment, $grade->userid, $grade->attemptnumber); } So, in the form itself you would store what user you are editing the PDF of, then if you are saving the grades of another user you would first copy the draft annotations to that user. It would not matter in what order the users are processed as the drafts are never deleted.
            Hide
            damyon Damyon Wiese added a comment -

            I talked with Fred about his comment above and agree that seems the best solution.

            Show
            damyon Damyon Wiese added a comment - I talked with Fred about his comment above and agree that seems the best solution.
            Hide
            gregor89 Greg Faller added a comment -

            Hi Frédéric Massart,

            Makes sense - thanks. I've altered my commit based on your feedback and moved the copy_drafts_from_to function into the page_editor class (seemed more fitting).

            Something I've noticed is that the feedback files seem to regenerate regardless of whether new annotations have been added or not. I assume fixing MDL-45582 would compound this issue when a teacher only wants to alter a grade (no changed to PDF), but the feedback files are regenerated for each group member. It seems as if has_annotations_or_comments within page_editor would benefit from checking whether there are any draft annotations only, rather than all annotations, and if so, then regenerate the feedback file?

            Thanks,
            Greg

            Show
            gregor89 Greg Faller added a comment - Hi Frédéric Massart , Makes sense - thanks. I've altered my commit based on your feedback and moved the copy_drafts_from_to function into the page_editor class (seemed more fitting). Something I've noticed is that the feedback files seem to regenerate regardless of whether new annotations have been added or not. I assume fixing MDL-45582 would compound this issue when a teacher only wants to alter a grade (no changed to PDF), but the feedback files are regenerated for each group member. It seems as if has_annotations_or_comments within page_editor would benefit from checking whether there are any draft annotations only, rather than all annotations, and if so, then regenerate the feedback file? Thanks, Greg
            Hide
            cibot CiBoT added a comment -

            Results for MDL-45582

            • Remote repository: git://github.com/greg-or/moodle-mod_assign.git
            Show
            cibot CiBoT added a comment - Results for MDL-45582 Remote repository: git://github.com/greg-or/moodle-mod_assign.git Remote branch m26_ MDL-45582 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3955 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3955/artifact/work/smurf.html Remote branch m27_ MDL-45582 to be integrated into upstream MOODLE_27_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3956 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3956/artifact/work/smurf.html
            Hide
            fred Frédéric Massart added a comment -

            Thanks Greg,

            this is looking pretty good! I just have 3 minor suggestions and I think it's ready to go for integration.

            1. The variable $sourceusergradeid is not a gradeid, but a grade, would it make more sense to name it $sourceusergrade?
            2. When getting the annotations/comments, you are also copying the non-draft ones. This is not really an issue but that leads to unnecessary queries, and maybe we should try to be as fast as possible (thinking of a group with many students).

            Many thanks!
            Fred

            Show
            fred Frédéric Massart added a comment - Thanks Greg, this is looking pretty good! I just have 3 minor suggestions and I think it's ready to go for integration. The variable $sourceusergradeid is not a gradeid, but a grade, would it make more sense to name it $sourceusergrade ? When getting the annotations/comments, you are also copying the non-draft ones. This is not really an issue but that leads to unnecessary queries, and maybe we should try to be as fast as possible (thinking of a group with many students). Many thanks! Fred
            Hide
            gregor89 Greg Faller added a comment -

            Hi Frédéric Massart,

            I've updated the branch based on your feedback.

            Regards,
            Greg

            Show
            gregor89 Greg Faller added a comment - Hi Frédéric Massart , I've updated the branch based on your feedback. Regards, Greg
            Hide
            fred Frédéric Massart added a comment -

            Hi Greg,

            I think I forgot to mention the lack of branch for master, could you please add them? I also now feel that an inline comment where we compare the sourceuserid with the currentid could be useful to understand what is happening there later on.

            Sorry for missing those points before!

            Cheers,

            Fred

            Show
            fred Frédéric Massart added a comment - Hi Greg, I think I forgot to mention the lack of branch for master, could you please add them? I also now feel that an inline comment where we compare the sourceuserid with the currentid could be useful to understand what is happening there later on. Sorry for missing those points before! Cheers, Fred
            Hide
            gregor89 Greg Faller added a comment -

            Hi Fred,

            Master branch and inline comment added.

            Thanks,
            Greg

            Show
            gregor89 Greg Faller added a comment - Hi Fred, Master branch and inline comment added. Thanks, Greg
            Hide
            fred Frédéric Massart added a comment -

            Awesome, thanks! Pushing for integration.

            Show
            fred Frédéric Massart added a comment - Awesome, thanks! Pushing for integration.
            Hide
            cibot CiBoT added a comment -

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

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

            I have been talking to Fred about one small issue here - we need to make sure the sourceuserid is not modified in the form, or you could copy grades and feedback from some random user by hacking the form.

            Show
            damyon Damyon Wiese added a comment - I have been talking to Fred about one small issue here - we need to make sure the sourceuserid is not modified in the form, or you could copy grades and feedback from some random user by hacking the form.
            Hide
            damyon Damyon Wiese added a comment -

            This just needs to call setConstant on the editpdf_source_userid field in the form to prevent any hacking of it (which would cause it to load any other users comments).

            Show
            damyon Damyon Wiese added a comment - This just needs to call setConstant on the editpdf_source_userid field in the form to prevent any hacking of it (which would cause it to load any other users comments).
            Hide
            gregor89 Greg Faller added a comment -

            Hi Damyon Wiese,

            I've added the setConstant call.

            Thanks,
            Greg

            Show
            gregor89 Greg Faller added a comment - Hi Damyon Wiese , I've added the setConstant call. Thanks, Greg
            Hide
            damyon Damyon Wiese added a comment -

            Thanks for the update Greg,

            Behat detected one more fail - this patch was throwing debugging when there is no pdf editor in the form (because there are no pdfs) - I added a fix for that and pushed to 26, 27 and master.

            Show
            damyon Damyon Wiese added a comment - Thanks for the update Greg, Behat detected one more fail - this patch was throwing debugging when there is no pdf editor in the form (because there are no pdfs) - I added a fix for that and pushed to 26, 27 and master.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for working on this Greg,

            Works as expected...

            Passing...

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for working on this Greg, Works as expected... Passing...
            Hide
            damyon Damyon Wiese added a comment -

            Look at bug
            CTRL-C
            www.stackoverflow.com
            CTRL-V
            <enter>
            CTRL-C
            CTRL-V
            Bug fixed

            (it's never that easy really)

            Thanks for working on this issue, it has been released as part of Moodle.

            Show
            damyon Damyon Wiese added a comment - Look at bug CTRL-C www.stackoverflow.com CTRL-V <enter> CTRL-C CTRL-V Bug fixed (it's never that easy really) Thanks for working on this issue, it has been released as part of Moodle.
            Hide
            gaudreaj Jean-Philippe Gaudreau added a comment -

            We found a major regression associated to this fix : MDL-46912
            This bug has been reproduced in 2.6.4 and 2.8dev

            Show
            gaudreaj Jean-Philippe Gaudreau added a comment - We found a major regression associated to this fix : MDL-46912 This bug has been reproduced in 2.6.4 and 2.8dev

              People

              • Votes:
                2 Vote for this issue
                Watchers:
                10 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/Jul/14