Moodle
  1. Moodle
  2. MDL-36061

grade_item->refresh_grades doesn't propagate $userid

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.2.5, 2.3.2
    • Fix Version/s: 2.2.7, 2.3.4
    • Component/s: Gradebook
    • Labels:
    • Testing Instructions:
      Hide

      Check the unit tests run successfully. Run "phpunit grade_item_testcase lib/grade/tests/grade_item_test.php" if you want to run them yourself.

      To test this you'll need an assignment that a student has a grade for. Create an assignment, have the student submit something and then mark them if you dont already have one. Make a note of the grade.

      In the gradebook on the grader report override the student grade.

      Click the edit icon next to the overriden grade. Clear the "Overridden" checkbox and Save.

      Check you dont receive any errors. The student's grade should have gone back to what it was originally.

      Show
      Check the unit tests run successfully. Run "phpunit grade_item_testcase lib/grade/tests/grade_item_test.php" if you want to run them yourself. To test this you'll need an assignment that a student has a grade for. Create an assignment, have the student submit something and then mark them if you dont already have one. Make a note of the grade. In the gradebook on the grader report override the student grade. Click the edit icon next to the overriden grade. Clear the "Overridden" checkbox and Save. Check you dont receive any errors. The student's grade should have gone back to what it was originally.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-36061_userid
    • Rank:
      44824

      Description

      The refresh_grades function takes as an optional userid parameter, which it then proceeds to do absolutely nothing with. Looking at the code, there is a call to grade_update_mod_grades, which also can take an optional userid parameter, only we aren't passing it in. By not passing in this value, we are causing unnecessary work to be done in certain cases (like when a single grade has it's override flag removed) by forcing ALL grades to be refreshed instead of just the one we care about.

      The following (from http://git.moodle.org/gw?p=moodle.git;a=blob;f=lib/grade/grade_item.php;h=12afe84e3930b769c3cd19829cc3e79c41604493;hb=HEAD)

      lib/grade/grade_item.php
      1448: grade_update_mod_grades($activity)
      

      should be replaced with this

      lib/grade/grade_item.php
      1448: grade_update_mod_grades($activity,$userid)
      

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for spotting that.

          Show
          Michael de Raadt added a comment - Thanks for spotting that.
          Hide
          Matthew G. Switlik added a comment -

          I want to deploy a patch for this as soon as possible. Is there someplace I can cherry-pick from? If not that's fine, I was planning on making branches for the fix on my github when I found this ticket. However I don't seem to have permission to add the github links to this ticket.

          Show
          Matthew G. Switlik added a comment - I want to deploy a patch for this as soon as possible. Is there someplace I can cherry-pick from? If not that's fine, I was planning on making branches for the fix on my github when I found this ticket. However I don't seem to have permission to add the github links to this ticket.
          Show
          Matthew G. Switlik added a comment - https://github.com/SWiT/moodle/ branches: MDL-36061 _master https://github.com/SWiT/moodle/commit/16990402dcb97833a7de2390639ce3e049e24414 MDL-36061 _23 https://github.com/SWiT/moodle/commit/5720724165cd42d92ace162abafdabab41e75b56 MDL-36061 _22 https://github.com/SWiT/moodle/commit/386c46124bf0d9f1c34134c539df56b3ac1cccf8
          Hide
          Andrew Davis added a comment -

          Ive added a second commit. https://github.com/andyjdavis/moodle/compare/master...MDL-36061_userid

          It's just some unit tests and a few changes to refresh_grades() to make it return something. I'll do some more testing and then put this up for peer review.

          Show
          Andrew Davis added a comment - Ive added a second commit. https://github.com/andyjdavis/moodle/compare/master...MDL-36061_userid It's just some unit tests and a few changes to refresh_grades() to make it return something. I'll do some more testing and then put this up for peer review.
          Hide
          Andrew Davis added a comment -

          I've put this up for peer review as well as raising 2 issues I encountered while working through this.

          Show
          Andrew Davis added a comment - I've put this up for peer review as well as raising 2 issues I encountered while working through this.
          Hide
          Rajesh Taneja added a comment -

          Thanks Andrew,

          Patch looks good. It will be nice to backport this on 22 and 23.

          [y] Syntax
          [y] Output
          [y] Whitespace
          [-] Language
          [-] Databases
          [y] Testing
          [-] Security
          [y] Documentation
          [y] Git
          [y] Sanity check

          Few minor things you might want to consider:

          1. Comment should start with capital letter and end with period (.).
          2. Also, not sure if we should extend testcase lib/tests/gradelib_test.php to include grade_update_mod_grades for a specific user (pass second param userid) and check if grade is updated for the specific user.
          Show
          Rajesh Taneja added a comment - Thanks Andrew, Patch looks good. It will be nice to backport this on 22 and 23. [y] Syntax [y] Output [y] Whitespace [-] Language [-] Databases [y] Testing [-] Security [y] Documentation [y] Git [y] Sanity check Few minor things you might want to consider: Comment should start with capital letter and end with period (.). https://github.com/andyjdavis/moodle/compare/master...MDL-36061_userid#L2R1193 https://github.com/andyjdavis/moodle/compare/master...MDL-36061_userid#L1R488 https://github.com/andyjdavis/moodle/compare/master...MDL-36061_userid#L1R493 https://github.com/andyjdavis/moodle/compare/master...MDL-36061_userid#L3R37 and more... Also, not sure if we should extend testcase lib/tests/gradelib_test.php to include grade_update_mod_grades for a specific user (pass second param userid) and check if grade is updated for the specific user.
          Hide
          Andrew Davis added a comment -

          Ive fixed all of that with the exception of enhancing the unit tests. I've split that out into MDL-36617.

          Show
          Andrew Davis added a comment - Ive fixed all of that with the exception of enhancing the unit tests. I've split that out into MDL-36617 .
          Hide
          Andrew Davis added a comment -

          Adding versions for 2.3 and 2.2. 2.2 doesn't contain the unit tests due to changes made to the unit test framework between 2.2 and 2.3.

          Show
          Andrew Davis added a comment - Adding versions for 2.3 and 2.2. 2.2 doesn't contain the unit tests due to changes made to the unit test framework between 2.2 and 2.3.
          Hide
          Dan Poltawski added a comment -

          Integrated, thanks Andrew (master, 23, 22)

          Show
          Dan Poltawski added a comment - Integrated, thanks Andrew (master, 23, 22)
          Hide
          Dan Poltawski added a comment -

          HP Fatal error: Call to undefined method gradelib_testcase::assertDebuggingCalled() in /Users/Shared/Jenkins/Home/git_repositories/MOODLE_23_STABLE/lib/tests/gradelib_test.php on line 43
          PHP Stack trace:
          PHP 1.

          {main}() /Users/stronk7/Sites/pear/bin/phpunit:0
          PHP 2. PHPUnit_TextUI_Command::main() /Users/stronk7/Sites/pear/bin/phpunit:46
          PHP 3. PHPUnit_TextUI_Command->run() /Users/stronk7/Sites/pear/PEAR/PHPUnit/TextUI/Command.php:130
          PHP 4. PHPUnit_TextUI_TestRunner->doRun() /Users/stronk7/Sites/pear/PEAR/PHPUnit/TextUI/Command.php:192
          PHP 5. PHPUnit_Framework_TestSuite->run() /Users/stronk7/Sites/pear/PEAR/PHPUnit/TextUI/TestRunner.php:325
          PHP 6. PHPUnit_Framework_TestSuite->run() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestSuite.php:705
          PHP 7. PHPUnit_Framework_TestSuite->run() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestSuite.php:705
          PHP 8. PHPUnit_Framework_TestSuite->runTest() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestSuite.php:745
          PHP 9. PHPUnit_Framework_TestCase->run() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestSuite.php:772
          PHP 10. PHPUnit_Framework_TestResult->run() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestCase.php:751
          PHP 11. advanced_testcase->runBare() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestResult.php:649
          PHP 12. PHPUnit_Framework_TestCase->runBare() /Users/Shared/Jenkins/Home/git_repositories/MOODLE_23_STABLE/lib/phpunit/classes/advanced_testcase.php:76
          PHP 13. PHPUnit_Framework_TestCase->runTest() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestCase.php:804
          PHP 14. ReflectionMethod->invokeArgs() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestCase.php:942
          PHP 15. gradelib_testcase->test_grade_update_mod_grades() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestCase.php:942
          Debugging: missing lib.php file in module doesntexist
          * line 1179 of /lib/gradelib.php: call to debugging()
          * line 41 of /lib/tests/gradelib_test.php: call to grade_update_mod_grades()
          * line ? of unknownfile: call to gradelib_testcase->test_grade_update_mod_grades()
          * line 942 of /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestCase.php: call to ReflectionMethod->invokeArgs()
          * line 804 of /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestCase.php: call to PHPUnit_Framework_TestCase->runTest()
          * line 76 of /lib/phpunit/classes/advanced_testcase.php: call to PHPUnit_Framework_TestCase->runBare()
          * line 649 of /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestResult.php: call to advanced_testcase->runBare()
          * line 751 of /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestCase.php: call to PHPUnit_Framework_TestResult->run()
          * line 772 of /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestCase->run()
          * line 745 of /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->runTest()
          * line 705 of /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->run()
          * line 705 of /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->run()
          * line 325 of /Users/stronk7/Sites/pear/PEAR/PHPUnit/TextUI/TestRunner.php: call to PHPUnit_Framework_TestSuite->run()
          * line 192 of /Users/stronk7/Sites/pear/PEAR/PHPUnit/TextUI/Command.php: call to PHPUnit_TextUI_TestRunner->doRun()
          * line 130 of /Users/stronk7/Sites/pear/PEAR/PHPUnit/TextUI/Command.php: call to PHPUnit_TextUI_Command->run()
          * line 46 of /Users/stronk7/Sites/pear/bin/phpunit: call to PHPUnit_TextUI_Command::main()

          Fatal error: Call to undefined method gradelib_testcase::assertDebuggingCalled() in /Users/Shared/Jenkins/Home/git_repositories/MOODLE_23_STABLE/lib/tests/gradelib_test.php on line 43

          Call Stack:
          0.0002 644232 1. {main}

          () /Users/stronk7/Sites/pear/bin/phpunit:0
          0.0035 1220080 2. PHPUnit_TextUI_Command::main() /Users/stronk7/Sites/pear/bin/phpunit:46
          0.0035 1220824 3. PHPUnit_TextUI_Command->run() /Users/stronk7/Sites/pear/PEAR/PHPUnit/TextUI/Command.php:130
          2.5526 200376152 4. PHPUnit_TextUI_TestRunner->doRun() /Users/stronk7/Sites/pear/PEAR/PHPUnit/TextUI/Command.php:192
          2.5566 200780368 5. PHPUnit_Framework_TestSuite->run() /Users/stronk7/Sites/pear/PEAR/PHPUnit/TextUI/TestRunner.php:325
          16.0713 209787776 6. PHPUnit_Framework_TestSuite->run() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestSuite.php:705
          64.3722 217037560 7. PHPUnit_Framework_TestSuite->run() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestSuite.php:705
          64.3724 217037960 8. PHPUnit_Framework_TestSuite->runTest() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestSuite.php:745
          64.3724 217037960 9. PHPUnit_Framework_TestCase->run() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestSuite.php:772
          64.3725 217037960 10. PHPUnit_Framework_TestResult->run() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestCase.php:751
          64.3727 217039232 11. advanced_testcase->runBare() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestResult.php:649
          64.3731 217061280 12. PHPUnit_Framework_TestCase->runBare() /Users/Shared/Jenkins/Home/git_repositories/MOODLE_23_STABLE/lib/phpunit/classes/advanced_testcase.php:76
          64.3732 217102400 13. PHPUnit_Framework_TestCase->runTest() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestCase.php:804
          64.3732 217103848 14. ReflectionMethod->invokeArgs() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestCase.php:942
          64.3732 217103880 15. gradelib_testcase->test_grade_update_mod_grades() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestCase.php:942

          Show
          Dan Poltawski added a comment - HP Fatal error: Call to undefined method gradelib_testcase::assertDebuggingCalled() in /Users/Shared/Jenkins/Home/git_repositories/MOODLE_23_STABLE/lib/tests/gradelib_test.php on line 43 PHP Stack trace: PHP 1. {main}() /Users/stronk7/Sites/pear/bin/phpunit:0 PHP 2. PHPUnit_TextUI_Command::main() /Users/stronk7/Sites/pear/bin/phpunit:46 PHP 3. PHPUnit_TextUI_Command->run() /Users/stronk7/Sites/pear/PEAR/PHPUnit/TextUI/Command.php:130 PHP 4. PHPUnit_TextUI_TestRunner->doRun() /Users/stronk7/Sites/pear/PEAR/PHPUnit/TextUI/Command.php:192 PHP 5. PHPUnit_Framework_TestSuite->run() /Users/stronk7/Sites/pear/PEAR/PHPUnit/TextUI/TestRunner.php:325 PHP 6. PHPUnit_Framework_TestSuite->run() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestSuite.php:705 PHP 7. PHPUnit_Framework_TestSuite->run() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestSuite.php:705 PHP 8. PHPUnit_Framework_TestSuite->runTest() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestSuite.php:745 PHP 9. PHPUnit_Framework_TestCase->run() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestSuite.php:772 PHP 10. PHPUnit_Framework_TestResult->run() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestCase.php:751 PHP 11. advanced_testcase->runBare() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestResult.php:649 PHP 12. PHPUnit_Framework_TestCase->runBare() /Users/Shared/Jenkins/Home/git_repositories/MOODLE_23_STABLE/lib/phpunit/classes/advanced_testcase.php:76 PHP 13. PHPUnit_Framework_TestCase->runTest() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestCase.php:804 PHP 14. ReflectionMethod->invokeArgs() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestCase.php:942 PHP 15. gradelib_testcase->test_grade_update_mod_grades() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestCase.php:942 Debugging: missing lib.php file in module doesntexist * line 1179 of /lib/gradelib.php: call to debugging() * line 41 of /lib/tests/gradelib_test.php: call to grade_update_mod_grades() * line ? of unknownfile: call to gradelib_testcase->test_grade_update_mod_grades() * line 942 of /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestCase.php: call to ReflectionMethod->invokeArgs() * line 804 of /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestCase.php: call to PHPUnit_Framework_TestCase->runTest() * line 76 of /lib/phpunit/classes/advanced_testcase.php: call to PHPUnit_Framework_TestCase->runBare() * line 649 of /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestResult.php: call to advanced_testcase->runBare() * line 751 of /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestCase.php: call to PHPUnit_Framework_TestResult->run() * line 772 of /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestCase->run() * line 745 of /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->runTest() * line 705 of /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->run() * line 705 of /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->run() * line 325 of /Users/stronk7/Sites/pear/PEAR/PHPUnit/TextUI/TestRunner.php: call to PHPUnit_Framework_TestSuite->run() * line 192 of /Users/stronk7/Sites/pear/PEAR/PHPUnit/TextUI/Command.php: call to PHPUnit_TextUI_TestRunner->doRun() * line 130 of /Users/stronk7/Sites/pear/PEAR/PHPUnit/TextUI/Command.php: call to PHPUnit_TextUI_Command->run() * line 46 of /Users/stronk7/Sites/pear/bin/phpunit: call to PHPUnit_TextUI_Command::main() Fatal error: Call to undefined method gradelib_testcase::assertDebuggingCalled() in /Users/Shared/Jenkins/Home/git_repositories/MOODLE_23_STABLE/lib/tests/gradelib_test.php on line 43 Call Stack: 0.0002 644232 1. {main} () /Users/stronk7/Sites/pear/bin/phpunit:0 0.0035 1220080 2. PHPUnit_TextUI_Command::main() /Users/stronk7/Sites/pear/bin/phpunit:46 0.0035 1220824 3. PHPUnit_TextUI_Command->run() /Users/stronk7/Sites/pear/PEAR/PHPUnit/TextUI/Command.php:130 2.5526 200376152 4. PHPUnit_TextUI_TestRunner->doRun() /Users/stronk7/Sites/pear/PEAR/PHPUnit/TextUI/Command.php:192 2.5566 200780368 5. PHPUnit_Framework_TestSuite->run() /Users/stronk7/Sites/pear/PEAR/PHPUnit/TextUI/TestRunner.php:325 16.0713 209787776 6. PHPUnit_Framework_TestSuite->run() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestSuite.php:705 64.3722 217037560 7. PHPUnit_Framework_TestSuite->run() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestSuite.php:705 64.3724 217037960 8. PHPUnit_Framework_TestSuite->runTest() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestSuite.php:745 64.3724 217037960 9. PHPUnit_Framework_TestCase->run() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestSuite.php:772 64.3725 217037960 10. PHPUnit_Framework_TestResult->run() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestCase.php:751 64.3727 217039232 11. advanced_testcase->runBare() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestResult.php:649 64.3731 217061280 12. PHPUnit_Framework_TestCase->runBare() /Users/Shared/Jenkins/Home/git_repositories/MOODLE_23_STABLE/lib/phpunit/classes/advanced_testcase.php:76 64.3732 217102400 13. PHPUnit_Framework_TestCase->runTest() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestCase.php:804 64.3732 217103848 14. ReflectionMethod->invokeArgs() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestCase.php:942 64.3732 217103880 15. gradelib_testcase->test_grade_update_mod_grades() /Users/stronk7/Sites/pear/PEAR/PHPUnit/Framework/TestCase.php:942
          Hide
          Dan Poltawski added a comment -

          I've pushed a fix to these broken 2.3 tests. (removing the ones causing debugging, and mod_assign -> mod_assignment)

          Show
          Dan Poltawski added a comment - I've pushed a fix to these broken 2.3 tests. (removing the ones causing debugging, and mod_assign -> mod_assignment)
          Hide
          Frédéric Massart added a comment - - edited

          Working as described on 2.2, 2.3 and master. Thanks!
          Note: I did not run the Unit Tests

          Show
          Frédéric Massart added a comment - - edited Working as described on 2.2, 2.3 and master. Thanks! Note: I did not run the Unit Tests
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many, many thanks for your effort!

          Millions of people will enjoy the results of your work, yay!

          Closing as fixed. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many, many thanks for your effort! Millions of people will enjoy the results of your work, yay! Closing as fixed. Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: