Moodle
  1. Moodle
  2. MDL-43250

get_graders() matches potential graders from all groups

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.7, 2.5.3, 2.6
    • Fix Version/s: 2.4.8, 2.5.4, 2.6.1
    • Component/s: Assignment
    • Labels:
    • Testing Instructions:
      Hide

      25+

      1. Run unit tests.

      On 24 only (no tests there)

      1. create 2 groups in a course
      2. Add a teacher and a student to the first group
      3. Add a different teacher and the same student to the second group
      4. Create a grouping
      5. Add the first group to the grouping
      6. Create an assignment with groupmode = separate groups, and grouping set to this new grouping
      7. Add an assignment submission as the student
      8. Verify that the first teacher was notified - but the second one was not.
      Show
      25+ Run unit tests. On 24 only (no tests there) create 2 groups in a course Add a teacher and a student to the first group Add a different teacher and the same student to the second group Create a grouping Add the first group to the grouping Create an assignment with groupmode = separate groups, and grouping set to this new grouping Add an assignment submission as the student Verify that the first teacher was notified - but the second one was not.
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull 2.6 Branch:
    • Pull Master Branch:
      MDL-43250-master
    • Story Points (Obsolete):
      3
    • Sprint:
      FRONTEND Sprint 7

      Description

      We have come to belief that the function that determines who should recieve a notification to grade a submission (/mod/assign/locallib.php:get_graders())is too broad.

      When using an activity in SEPARATE groups mode the function currently identifies potential graders as users who hold the grader capability and are in ANY group with the submitter in the course.

      We believe that this may be an issue.

      We have spent a while talking about this and feel that the groups_get_all_groups() function should also pass the activity's groupingid value so that, if a grouping has been selected, the potential graders are restricted to users within the activity's grouping.

      This should be a case of simply changing the line to

      if ($groups = groups_get_allgroups($this->get_course()->id, $userid, $this->get_course_module()->groupingid) {
      

      Like I said we have spent a while talking about this and we can't see the reason why this works the way it does, so we would also welcome someone explaining why it is the way it is!

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Damyon Wiese added a comment -

            Seems like a bug and the solution sounds correct.

            Thanks for reporting.

            Show
            Damyon Wiese added a comment - Seems like a bug and the solution sounds correct. Thanks for reporting.
            Hide
            Damyon Wiese added a comment -

            Note - the unit test is slightly different for 25 verses 26 and master because the generators are better now.

            Show
            Damyon Wiese added a comment - Note - the unit test is slightly different for 25 verses 26 and master because the generators are better now.
            Hide
            Andrew Davis added a comment -

            Perhaps slightly reformat the unit test for readability.

            What it is currently

            $this->create_extra_users();
            $this->setUser($this->editingteachers[0]);
            $assign = $this->create_instance();

            $this->assertCount(self::DEFAULT_TEACHER_COUNT +
            self::DEFAULT_EDITING_TEACHER_COUNT +
            self::EXTRA_TEACHER_COUNT +
            self::EXTRA_EDITING_TEACHER_COUNT,
            $assign->testable_get_graders($this->students[0]->id));

            What is possibly should be.

            $this->create_extra_users();
            $this->setUser($this->editingteachers[0]);

            // Create an assignment with no groups.
            $assign = $this->create_instance();
            $this->assertCount(self::DEFAULT_TEACHER_COUNT +
            self::DEFAULT_EDITING_TEACHER_COUNT +
            self::EXTRA_TEACHER_COUNT +
            self::EXTRA_EDITING_TEACHER_COUNT,
            $assign->testable_get_graders($this->students[0]->id));

            $assign = $this->create_instance(array('groupingid'=>$groupingid, 'groupmode'=>SEPARATEGROUPS));
            You will want spaces either side of those =>'s.

            Otherwise it looks good. Submit for integration when you are ready.

            Show
            Andrew Davis added a comment - Perhaps slightly reformat the unit test for readability. What it is currently $this->create_extra_users(); $this->setUser($this->editingteachers [0] ); $assign = $this->create_instance(); $this->assertCount(self::DEFAULT_TEACHER_COUNT + self::DEFAULT_EDITING_TEACHER_COUNT + self::EXTRA_TEACHER_COUNT + self::EXTRA_EDITING_TEACHER_COUNT, $assign->testable_get_graders($this->students [0] ->id)); What is possibly should be. $this->create_extra_users(); $this->setUser($this->editingteachers [0] ); // Create an assignment with no groups. $assign = $this->create_instance(); $this->assertCount(self::DEFAULT_TEACHER_COUNT + self::DEFAULT_EDITING_TEACHER_COUNT + self::EXTRA_TEACHER_COUNT + self::EXTRA_EDITING_TEACHER_COUNT, $assign->testable_get_graders($this->students [0] ->id)); $assign = $this->create_instance(array('groupingid'=>$groupingid, 'groupmode'=>SEPARATEGROUPS)); You will want spaces either side of those =>'s. Otherwise it looks good. Submit for integration when you are ready.
            Hide
            Damyon Wiese added a comment -

            Thanks Andrew, those changes make sense - I have amended the commit with updated unit test for 25, 26 and master branches. Pushing for integration.

            Show
            Damyon Wiese added a comment - Thanks Andrew, those changes make sense - I have amended the commit with updated unit test for 25, 26 and master branches. Pushing for integration.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Hi guys,

            this is just a message to share with you that I'm going to perform a test of the automated pre-checker against all the current issues awaiting integration (16 issues).

            So, soon, you'll get some extra comments in this issue with some information from the pre-checker. Note it's not final, but just an experiment and there are lots of things to improve, from the message itself to various false positives in the checkers. So take any report with caution, it's not 100% accurate yet.

            Please, feel free to comment any idea/objection @ MDLSITE-2662. I'll be collecting everything there.

            TIA and ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Hi guys, this is just a message to share with you that I'm going to perform a test of the automated pre-checker against all the current issues awaiting integration (16 issues). So, soon, you'll get some extra comments in this issue with some information from the pre-checker. Note it's not final, but just an experiment and there are lots of things to improve, from the message itself to various false positives in the checkers. So take any report with caution, it's not 100% accurate yet. Please, feel free to comment any idea/objection @ MDLSITE-2662 . I'll be collecting everything there. TIA and ciao
            Hide
            CiBoT added a comment -

            Results for MDL-43250

            Show
            CiBoT added a comment - Results for MDL-43250 Branch MDL-43250 -24 to be integrated into upstream MOODLE_24_STABLE Executed job http://ci.stronk7.com/job/Precheck%20remote%20branch/693 Execution status: 2 Error: The MDL-43250 -24 branch at git://github.com/damyon/moodle.git is very old. Please rebase against current MOODLE_24_STABLE. Branch MDL-43250 -25 to be integrated into upstream MOODLE_25_STABLE Executed job http://ci.stronk7.com/job/Precheck%20remote%20branch/694 Execution status: 0 Warning: The MDL-43250 -25 branch at git://github.com/damyon/moodle.git has not been rebased recently. Details: http://ci.stronk7.com/job/Precheck%20remote%20branch/694/artifact/work/smurf.html Branch MDL-43250 -26 to be integrated into upstream MOODLE_26_STABLE Executed job http://ci.stronk7.com/job/Precheck%20remote%20branch/695 Execution status: 0 Details: http://ci.stronk7.com/job/Precheck%20remote%20branch/695/artifact/work/smurf.html Branch MDL-43250 -master to be integrated into upstream master Executed job http://ci.stronk7.com/job/Precheck%20remote%20branch/696 Execution status: 0 Details: http://ci.stronk7.com/job/Precheck%20remote%20branch/696/artifact/work/smurf.html
            Hide
            Sam Hemelryk added a comment -

            Thanks Damyon - this has been integrated now.

            Show
            Sam Hemelryk added a comment - Thanks Damyon - this has been integrated now.
            Hide
            Sam Hemelryk added a comment -

            Tested and passed during integration review thanks Damyon.

            Show
            Sam Hemelryk added a comment - Tested and passed during integration review thanks Damyon.
            Hide
            Sam Hemelryk added a comment -

            Thank you, your code has landed just in time for 2013.
            Merry Christmas and may your 2014 be even better than 2013.

            Kind regards with much holiday spirit
            Sam

            Show
            Sam Hemelryk added a comment - Thank you, your code has landed just in time for 2013. Merry Christmas and may your 2014 be even better than 2013. Kind regards with much holiday spirit Sam

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Agile