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

Marker Allocation and Management

    Details

    • Type: New Feature
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.6
    • Component/s: Assignment
    • Labels:

      Description

      Add marker allocation and management to allow marks to be reviewed and approved before released to students.
      Full spec available here:
      http://docs.moodle.org/dev/Assignment/Draft_Features/Marker_Allocation_and_Management
      Discussion here:
      https://moodle.org/mod/forum/discuss.php?d=220723

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              danmarsden Dan Marsden added a comment -

              Hi Damyon - any chance of a peer review before the code freeze?

              Show
              danmarsden Dan Marsden added a comment - Hi Damyon - any chance of a peer review before the code freeze?
              Hide
              damyon Damyon Wiese added a comment -

              Thanks for the patch Dan, I'll take a quick look this week and get some idea of how much might need changing (hopefully not much!). At any rate if all looks good I'll have started the peer review proper before the code freeze so that will be OK.

              Show
              damyon Damyon Wiese added a comment - Thanks for the patch Dan, I'll take a quick look this week and get some idea of how much might need changing (hopefully not much!). At any rate if all looks good I'll have started the peer review proper before the code freeze so that will be OK.
              Hide
              danmarsden Dan Marsden added a comment -

              Hi Damyon,

              Have you had a chance to review the code yet? - thanks.

              btw - I pushed this commit today but I'm not 100% sure it was the best way:
              https://github.com/danmarsden/moodle/commit/e2c6998d873ed0b489386794ba1f151008761173

              Show
              danmarsden Dan Marsden added a comment - Hi Damyon, Have you had a chance to review the code yet? - thanks. btw - I pushed this commit today but I'm not 100% sure it was the best way: https://github.com/danmarsden/moodle/commit/e2c6998d873ed0b489386794ba1f151008761173
              Hide
              damyon Damyon Wiese added a comment -

              Sorry - not quite yet. It is almost at the top of my list.

              Show
              damyon Damyon Wiese added a comment - Sorry - not quite yet. It is almost at the top of my list.
              Hide
              damyon Damyon Wiese added a comment -

              Thanks Dan,

              I had a read through the patch and here are the things I spotted. I'll install it and give more feedback next week.

              Note: About to Conflict!
              A largish resubmission history patch is in integration review and about to land. The main change that will affect this code is that the workflowstate and allocatedmarker columns will need to be added to a new assign_user_flags table instead of the grade table (because there may be multiple grades per student).

              Small things:

              1. No need for OUTPUT and PAGE globals in view_grading_table
              2. view_batch_markingallocation missing phpdocs
              3. view_batch_set_workflow_state missing phpdocs
              4. in view_batch_markingallocation it would be better to get the extra user fields outside the foreach loop
              5. process_set_batch_marking_allocation is not checking the workflow state before allocating a marker
              6. You are assigning the copyright for the new code to NetSpot
              7. phpdoc for class mod_assign_batch_set_marking_workflow_state_form is copy/pasted
              8. No unit tests for the new functionality (but you haven't broken any either!)
              9. in assign::view() I recently implemented "redirect after post" so you should change:

              $this->process_set_batch_marking_allocation();                                                                          
              $action = 'grading';    

              to

              $action = 'redirect';                                                                                               
              $nextpageparams['action'] = 'grading';

              Overall this code is excellent - the patch is easy to read and it follows the style of the rest of the assign module very well. The most important thing I see to fix here is the lack of a unit test.

              Thanks for the patch!

              Show
              damyon Damyon Wiese added a comment - Thanks Dan, I had a read through the patch and here are the things I spotted. I'll install it and give more feedback next week. Note: About to Conflict! A largish resubmission history patch is in integration review and about to land. The main change that will affect this code is that the workflowstate and allocatedmarker columns will need to be added to a new assign_user_flags table instead of the grade table (because there may be multiple grades per student). Small things: No need for OUTPUT and PAGE globals in view_grading_table view_batch_markingallocation missing phpdocs view_batch_set_workflow_state missing phpdocs in view_batch_markingallocation it would be better to get the extra user fields outside the foreach loop process_set_batch_marking_allocation is not checking the workflow state before allocating a marker You are assigning the copyright for the new code to NetSpot phpdoc for class mod_assign_batch_set_marking_workflow_state_form is copy/pasted No unit tests for the new functionality (but you haven't broken any either!) in assign::view() I recently implemented "redirect after post" so you should change: $this->process_set_batch_marking_allocation(); $action = 'grading'; to $action = 'redirect'; $nextpageparams['action'] = 'grading'; Overall this code is excellent - the patch is easy to read and it follows the style of the rest of the assign module very well. The most important thing I see to fix here is the lack of a unit test. Thanks for the patch!
              Hide
              danmarsden Dan Marsden added a comment -

              Thanks Damyon - working on it now.

              first version based on MDL-36804 here:
              https://github.com/danmarsden/moodle/compare/MDL-36804-master...master_MDL-36804-38359
              (few more things to go)

              do I need to get unit tests in this patch before it's integrated - or can I commit to doing those early next week?

              Show
              danmarsden Dan Marsden added a comment - Thanks Damyon - working on it now. first version based on MDL-36804 here: https://github.com/danmarsden/moodle/compare/MDL-36804-master...master_MDL-36804-38359 (few more things to go) do I need to get unit tests in this patch before it's integrated - or can I commit to doing those early next week?
              Hide
              danmarsden Dan Marsden added a comment -

              updated based on Damyon's feedback rebased into a single commit on top of MDL-36804 code.

              Still to come: Unit tests

              Show
              danmarsden Dan Marsden added a comment - updated based on Damyon's feedback rebased into a single commit on top of MDL-36804 code. Still to come: Unit tests
              Hide
              danmarsden Dan Marsden added a comment -

              unit tests added/rebased again on top of MDL-36804 - I think this is now ready for integration - thanks Damyon - hopefully it will be accepted this week.

              NOTE TO INTEGRATOR - you may find it easier to cherry-pick this commit.

              Show
              danmarsden Dan Marsden added a comment - unit tests added/rebased again on top of MDL-36804 - I think this is now ready for integration - thanks Damyon - hopefully it will be accepted this week. NOTE TO INTEGRATOR - you may find it easier to cherry-pick this commit.
              Hide
              poltawski 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
              poltawski 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
              danmarsden Dan Marsden added a comment -

              rebased

              Show
              danmarsden Dan Marsden added a comment - rebased
              Hide
              damyon Damyon Wiese added a comment -

              Hi Dan,

              Thanks for your work on this issue - while I really wanted to get it in 2.5 and thought we could make it - I discussed it with Martin and we decided on Friday the extra load on testing this in combination with MDL-36804 was too risky. We only just got MDL-36804 through in time for the beta. It is unfortunate because the patch itself and the functionality is very good - it will definitely be integrated as soon as the on sync period for 2.6 is over. Now that the beta is released the freeze is officially in place and new features will not land in 2.5. This is important because it gives QA testers and documentation writers time to do their work.

              Thanks again for the work and sorry it didn't quite make it.

              Regards, Damyon

              Show
              damyon Damyon Wiese added a comment - Hi Dan, Thanks for your work on this issue - while I really wanted to get it in 2.5 and thought we could make it - I discussed it with Martin and we decided on Friday the extra load on testing this in combination with MDL-36804 was too risky. We only just got MDL-36804 through in time for the beta. It is unfortunate because the patch itself and the functionality is very good - it will definitely be integrated as soon as the on sync period for 2.6 is over. Now that the beta is released the freeze is officially in place and new features will not land in 2.5. This is important because it gives QA testers and documentation writers time to do their work. Thanks again for the work and sorry it didn't quite make it. Regards, Damyon
              Hide
              danmarsden Dan Marsden added a comment -

              rebased again today.

              Show
              danmarsden Dan Marsden added a comment - rebased again today.
              Hide
              damyon Damyon Wiese added a comment -

              Thanks Dan,

              MDL-37621 adds defaults for all assignment settings so will likely conflict with this. Once that issue is integrated I can quickly add a branch to fix the conflicts and add the defaults for the new settings.

              Show
              damyon Damyon Wiese added a comment - Thanks Dan, MDL-37621 adds defaults for all assignment settings so will likely conflict with this. Once that issue is integrated I can quickly add a branch to fix the conflicts and add the defaults for the new settings.
              Hide
              damyon Damyon Wiese added a comment -

              Rebased and settings updated to use admin defaults. I changed the version bump in Dans patch.

              Show
              damyon Damyon Wiese added a comment - Rebased and settings updated to use admin defaults. I changed the version bump in Dans patch.
              Hide
              damyon Damyon Wiese added a comment -

              Unit tests passing...

              Show
              damyon Damyon Wiese added a comment - Unit tests passing...
              Hide
              poltawski Dan Poltawski added a comment -

              Just commented to Damyon, the header without title will cause problems now I think:

              $mform->addElement('header', '', get_string('batchsetallocatedmarker', 'assign', count($params['users'])));

              Show
              poltawski Dan Poltawski added a comment - Just commented to Damyon, the header without title will cause problems now I think: $mform->addElement('header', '', get_string('batchsetallocatedmarker', 'assign', count($params['users'])));
              Hide
              damyon Damyon Wiese added a comment -

              Thanks Dan - I added a commit to the branch to set a name for those 2 headers.

              Show
              damyon Damyon Wiese added a comment - Thanks Dan - I added a commit to the branch to set a name for those 2 headers.
              Hide
              poltawski Dan Poltawski added a comment -

              Thanks Damyon and Dan, i've integrated this to master now.

              Looks nice and clean

              Show
              poltawski Dan Poltawski added a comment - Thanks Damyon and Dan, i've integrated this to master now. Looks nice and clean
              Hide
              damyon Damyon Wiese added a comment -

              This is failing the course unit tests because they have hardcoded test data that needs to be kept in sync with the module.

              Added a patch here:

              https://github.com/damyon/moodle/commit/7f198356efffba3d815aa00edaeea4ad18108237

              Show
              damyon Damyon Wiese added a comment - This is failing the course unit tests because they have hardcoded test data that needs to be kept in sync with the module. Added a patch here: https://github.com/damyon/moodle/commit/7f198356efffba3d815aa00edaeea4ad18108237
              Hide
              danmarsden Dan Marsden added a comment -

              great - thanks Damyon/Dan

              I'm not around on Fri and my time is limited on Thurs so if there are any issues during testing we may need to revert this until next week or if integrators are happy we could create new bugs to cover off any issues which I will resolve next week.

              Show
              danmarsden Dan Marsden added a comment - great - thanks Damyon/Dan I'm not around on Fri and my time is limited on Thurs so if there are any issues during testing we may need to revert this until next week or if integrators are happy we could create new bugs to cover off any issues which I will resolve next week.
              Hide
              damyon Damyon Wiese added a comment -

              Just setting a blocker link because these issues conflict with each other (this is in case we want to roll back any issues).

              Show
              damyon Damyon Wiese added a comment - Just setting a blocker link because these issues conflict with each other (this is in case we want to roll back any issues).
              Hide
              rwijaya Rossiani Wijaya added a comment -

              Hi Dan,

              There's a leftover echo for $where in mod/assign/gradingtable.php ~ line 215.

              I also noticed the following behaviour on:
              Grade visibility:

              • With quickgrading enable:
                Grade student submission without giving them any feedback and set the workflow to released. Results: The grade for the assignment is not displayed on student page.
                Add feedback for the above student and 'save all quick grading changes'. Results: the grade is now available for the student.
              • With quickgrading disable:
                Grade, provide some feedback for the submission and set the status to anything except 'released'. On view submissions page, select the user, select the 'set marking workflow state' option and set it to 'release'. Result: the grade is not available for student.

              After testing this a few times with different combinations, I think the bug occurs within the bulk action for 'set marking workflow state'.

              Marking workflow and marking allocation:

              • With quickgrading on, set the submissions workflow state back to 'marking completed' and save. The marker can't be changed.
              • With quickgrading off, enter the grading page for a student that has made a submission and change the allocated marker hit “save changes” (from testing instruction). Result: if the submission has a marker, the select option for the marker is disabled on the edit page.

              Non-editing teacher/allocated marker test - doing marking

              • This issue still exist. Testing instruction:

                Testing 02/04: As a marker I changed the status of a student from 'Not marked' to 'In marking' - all allocations where lost; the marker couldn't see the students anymore, in the teacher display all allocations to this marker were replaced with a none allocation (I tried this with quick grading enabled). Dan 03/04: could be a result of the incomplete code you got yesterday - could you please try again and let me know if it's still failing somewhere?

              Dan, could you confirm the above issues please?

              Thanks.

              Show
              rwijaya Rossiani Wijaya added a comment - Hi Dan, There's a leftover echo for $where in mod/assign/gradingtable.php ~ line 215. I also noticed the following behaviour on: Grade visibility: With quickgrading enable: Grade student submission without giving them any feedback and set the workflow to released. Results: The grade for the assignment is not displayed on student page. Add feedback for the above student and 'save all quick grading changes'. Results: the grade is now available for the student. With quickgrading disable: Grade, provide some feedback for the submission and set the status to anything except 'released'. On view submissions page, select the user, select the 'set marking workflow state' option and set it to 'release'. Result: the grade is not available for student. After testing this a few times with different combinations, I think the bug occurs within the bulk action for 'set marking workflow state'. Marking workflow and marking allocation: With quickgrading on, set the submissions workflow state back to 'marking completed' and save. The marker can't be changed. With quickgrading off, enter the grading page for a student that has made a submission and change the allocated marker hit “save changes” (from testing instruction). Result: if the submission has a marker, the select option for the marker is disabled on the edit page. Non-editing teacher/allocated marker test - doing marking This issue still exist. Testing instruction: Testing 02/04: As a marker I changed the status of a student from 'Not marked' to 'In marking' - all allocations where lost; the marker couldn't see the students anymore, in the teacher display all allocations to this marker were replaced with a none allocation (I tried this with quick grading enabled). Dan 03/04: could be a result of the incomplete code you got yesterday - could you please try again and let me know if it's still failing somewhere? Dan, could you confirm the above issues please? Thanks.
              Hide
              poltawski Dan Poltawski added a comment -

              Setting this to failed, because we at least need that $where to go frm master!

              Show
              poltawski Dan Poltawski added a comment - Setting this to failed, because we at least need that $where to go frm master!
              Hide
              danmarsden Dan Marsden added a comment -

              thanks Rosie - have updated the branch above to remove the echo statements - looking at the other items now - we may need to revert if I can't find the time tonight to resolve these.

              Show
              danmarsden Dan Marsden added a comment - thanks Rosie - have updated the branch above to remove the echo statements - looking at the other items now - we may need to revert if I can't find the time tonight to resolve these.
              Hide
              poltawski Dan Poltawski added a comment - - edited

              Thanks Dan - I think we can tolerate a bit of disruption on master (if Damyon agrees).

              Show
              poltawski Dan Poltawski added a comment - - edited Thanks Dan - I think we can tolerate a bit of disruption on master (if Damyon agrees).
              Hide
              danmarsden Dan Marsden added a comment -

              "Grade visibility"
              confirmed some bugs there - when set to released it's not always pushing the grade to the gradebook - should be an easy fix, working on it now.

              "Marking workflow and marking allocation:" sounds like expected behaviour there to me - the allocated marker can only be changed if the workflow state is set to "not marked" or "in Marking" - the manual grade edit page disables the allocated marker field automatically depending on the value set by the form - the quickgrade form doesn't do this automatically as it's not an mform - if you save the quickform changes it hides the select drop list and just displays the markers name
              I guess it makes sense to improve the UI we should probably add functionality that operates in a similar way to the manual grading form - can this be a new improvement request or does it need to be done before upstream?

              Non-editing teacher/allocated marker test - confirmed a bug there too, will try to fix it tonight.

              Show
              danmarsden Dan Marsden added a comment - "Grade visibility" confirmed some bugs there - when set to released it's not always pushing the grade to the gradebook - should be an easy fix, working on it now. "Marking workflow and marking allocation:" sounds like expected behaviour there to me - the allocated marker can only be changed if the workflow state is set to "not marked" or "in Marking" - the manual grade edit page disables the allocated marker field automatically depending on the value set by the form - the quickgrade form doesn't do this automatically as it's not an mform - if you save the quickform changes it hides the select drop list and just displays the markers name I guess it makes sense to improve the UI we should probably add functionality that operates in a similar way to the manual grading form - can this be a new improvement request or does it need to be done before upstream? Non-editing teacher/allocated marker test - confirmed a bug there too, will try to fix it tonight.
              Hide
              poltawski Dan Poltawski added a comment -

              (pulled in the debug code removal)

              Show
              poltawski Dan Poltawski added a comment - (pulled in the debug code removal)
              Hide
              danmarsden Dan Marsden added a comment -

              Thanks Dan -
              I've just pushed fixes for the other ones Rosie noticed - except for the "Marking workflow and marking allocation:" one which I'm happy to generate a new improvement request for but won't manage to fix tonight - will need to implement some Javascript to disable the selectors on the quickgrading page like it does on the manual grade editing page.

              Show
              danmarsden Dan Marsden added a comment - Thanks Dan - I've just pushed fixes for the other ones Rosie noticed - except for the "Marking workflow and marking allocation:" one which I'm happy to generate a new improvement request for but won't manage to fix tonight - will need to implement some Javascript to disable the selectors on the quickgrading page like it does on the manual grade editing page.
              Hide
              poltawski Dan Poltawski added a comment -

              Thanks for finding the time for it, Dan. I've integrated the fixes.

              Rosie, please can you re-test? (ps nice work finding these issues )

              Show
              poltawski Dan Poltawski added a comment - Thanks for finding the time for it, Dan. I've integrated the fixes. Rosie, please can you re-test? (ps nice work finding these issues )
              Hide
              rwijaya Rossiani Wijaya added a comment -

              Thank you Dan and Dan for fixing and integrate it.

              Re-test the issue and it works as expected now.

              Test passed.

              Show
              rwijaya Rossiani Wijaya added a comment - Thank you Dan and Dan for fixing and integrate it. Re-test the issue and it works as expected now. Test passed.
              Hide
              danmarsden Dan Marsden added a comment -

              Awesomeness - thanks everyone!

              Show
              danmarsden Dan Marsden added a comment - Awesomeness - thanks everyone!
              Hide
              marina Marina Glancy added a comment -

              Thanks for your awesome work! This has now become a part of Moodle.

              Closing as fixed!

              Show
              marina Marina Glancy added a comment - Thanks for your awesome work! This has now become a part of Moodle. Closing as fixed!
              Hide
              corleone Raymond Antonio added a comment -

              Thanks Dan and this is a great feature. I think I just found a (small) bug when you go to select 'Set allocated marker' on 'With selected...' drop-down menu in the grading table and if there is no allocated markers in the drop-down menu to choose and then hit 'save changes' or 'cancel' button, it gives an error 'A required parameter (allocatedmarker) was missing'. I think I'll create a separate tracker for this issue.

              Show
              corleone Raymond Antonio added a comment - Thanks Dan and this is a great feature. I think I just found a (small) bug when you go to select 'Set allocated marker' on 'With selected...' drop-down menu in the grading table and if there is no allocated markers in the drop-down menu to choose and then hit 'save changes' or 'cancel' button, it gives an error 'A required parameter (allocatedmarker) was missing'. I think I'll create a separate tracker for this issue.
              Hide
              danmarsden Dan Marsden added a comment -

              thanks Raymond - yes please create a new tracker issue and let me know the bug number so I can take a look

              Show
              danmarsden Dan Marsden added a comment - thanks Raymond - yes please create a new tracker issue and let me know the bug number so I can take a look
              Hide
              marycooch Mary Cooch added a comment -

              A couple of QA tests here ready for inclusion in 2.6 testing. Possibly I need to do another one so I will leave the qa_test_required label just for now. https://tracker.moodle.org/browse/MDLQA-5728 and https://tracker.moodle.org/browse/MDLQA-5727

              Show
              marycooch Mary Cooch added a comment - A couple of QA tests here ready for inclusion in 2.6 testing. Possibly I need to do another one so I will leave the qa_test_required label just for now. https://tracker.moodle.org/browse/MDLQA-5728 and https://tracker.moodle.org/browse/MDLQA-5727
              Hide
              danmarsden Dan Marsden added a comment -

              Thanks Mary - have you seen the testing instructions linked above? - I had hoped they could be translated into qa tests without too much effort

              Show
              danmarsden Dan Marsden added a comment - Thanks Mary - have you seen the testing instructions linked above? - I had hoped they could be translated into qa tests without too much effort
              Hide
              marycooch Mary Cooch added a comment -

              Ah there is a link I had looked for text on the page but not clicked the link;sorry; but that is excellent thanks as I can use it for one or two more; thanks.

              Show
              marycooch Mary Cooch added a comment - Ah there is a link I had looked for text on the page but not clicked the link;sorry; but that is excellent thanks as I can use it for one or two more; thanks.
              Hide
              marycooch Mary Cooch added a comment -

              Another QA test, thanks to Dan's useful link: MDLQA-5733

              Show
              marycooch Mary Cooch added a comment - Another QA test, thanks to Dan's useful link: MDLQA-5733
              Hide
              marycooch Mary Cooch added a comment -

              Removing qa_test_required label as there are now four QA tests for this: MDLQA-5734, MDLQA-5733, MDLQA-5728 and MDLQA-5727. Please comment if anything major has been left out.

              Show
              marycooch Mary Cooch added a comment - Removing qa_test_required label as there are now four QA tests for this: MDLQA-5734 , MDLQA-5733 , MDLQA-5728 and MDLQA-5727 . Please comment if anything major has been left out.
              Hide
              marycooch Mary Cooch added a comment -

              Removing docs_required label as this is now documented on http://docs.moodle.org/26/en/Assignment_settings (with thanks to Dan Marsden for the helpful dev docs on http://docs.moodle.org/dev/Assignment/Draft_Features/Marker_Allocation_and_Management)

              Show
              marycooch Mary Cooch added a comment - Removing docs_required label as this is now documented on http://docs.moodle.org/26/en/Assignment_settings (with thanks to Dan Marsden for the helpful dev docs on http://docs.moodle.org/dev/Assignment/Draft_Features/Marker_Allocation_and_Management )

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    18/Nov/13