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

Grade Import Creates Millions of Invalid Temporary Rows; Provides no Error Feedback

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.5, 2.1.2, 2.2, 2.6.2, 2.7
    • Fix Version/s: 2.5.6, 2.6.3
    • Component/s: Gradebook
    • Labels:
    • Database:
      MySQL
    • Testing Instructions:
      Hide

      Setup on fresh Moodle:

      1. Login to Moodle as admin
      2. Create a course
      3. Create 10 users, with ID number set to user001@example.com through user010@example.com (see provided CSV if this enumeration is unclear)
      4. Add these users as Students on the course
      5. Create 4 manual grade items named Quiz 1 through Quiz 4 and worth 2, 1, 10 and 10 points respectively.

      Testing:

      1. Click on the Grades link in the sidebar settings panel
      2. On the Grades page, select 'Import from CSV' from the drop-down select at the top of the page
      3. Upload the attached CSV with a million+ "blank" rows in the useridnumber column (This is a common problem when Excel creates the CSV).
      4. Map from User ID Number to useridnumber
      5. Map the grade columns to the grades items created in the setup.
      6. Ensure that your request completes without timing out.
      7. Inspect the grade_import_values table to make sure that it is empty after the process is complete.
      Show
      Setup on fresh Moodle: Login to Moodle as admin Create a course Create 10 users, with ID number set to user001@example.com through user010@example.com (see provided CSV if this enumeration is unclear) Add these users as Students on the course Create 4 manual grade items named Quiz 1 through Quiz 4 and worth 2, 1, 10 and 10 points respectively. Testing: Click on the Grades link in the sidebar settings panel On the Grades page, select 'Import from CSV' from the drop-down select at the top of the page Upload the attached CSV with a million+ "blank" rows in the useridnumber column (This is a common problem when Excel creates the CSV). Map from User ID Number to useridnumber Map the grade columns to the grades items created in the setup. Ensure that your request completes without timing out. Inspect the grade_import_values table to make sure that it is empty after the process is complete.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-29572_master

      Description

      Unfortunately, Excel CSV exports have a disagreeable tendency to contain a million or more "blank" rows consisting solely of commas. When real errors do occur, the import exits without any sort of feedback. In some situations, the millions of extra rows encourage the system to exit before cleanup causing some setups to be left with several gigabytes of grade import rows which exist in a state of limbo and have the bad habit of being pulled in by future imports for a column.

        Gliffy Diagrams

        1. grade_import_fixes_phase1_mdl2.patch
          7 kB
          Jonathan Champ
        2. grade_import_fixes_phase1_mdl2.patch
          7 kB
          Jonathan Champ
        3. grade_import_fixes_phase1.patch
          6 kB
          Jonathan Champ
        4. grade_import_fixes_phase1.patch
          6 kB
          Jonathan Champ
        5. grade_import_large_sample.csv
          5.00 MB
          Glenn Ansley

          Issue Links

            Activity

            Hide
            jrchamp Jonathan Champ added a comment -

            Turn out that switch is treated as a loop by continue, so this version of the patch correctly sets continue to 3 instead of 2. No other changes.

            Show
            jrchamp Jonathan Champ added a comment - Turn out that switch is treated as a loop by continue, so this version of the patch correctly sets continue to 3 instead of 2. No other changes.
            Hide
            blepoxp Glenn Ansley added a comment -

            Applied patches to 1.9 Stable, 2.0 Stable, 2.1 Stable and master for Jonathan

            Show
            blepoxp Glenn Ansley added a comment - Applied patches to 1.9 Stable, 2.0 Stable, 2.1 Stable and master for Jonathan
            Hide
            blepoxp Glenn Ansley added a comment -

            Here is the file mentioned above in testing instructions

            Show
            blepoxp Glenn Ansley added a comment - Here is the file mentioned above in testing instructions
            Hide
            blepoxp Glenn Ansley added a comment -

            Just rebased all the patches to the updated heads on various branches.

            Show
            blepoxp Glenn Ansley added a comment - Just rebased all the patches to the updated heads on various branches.
            Hide
            phalacee Jason Fowler added a comment -

            Seems to be in order, the code is fine

            Show
            phalacee Jason Fowler added a comment - Seems to be in order, the code is fine
            Hide
            blepoxp Glenn Ansley added a comment -

            Hey, thanks for looking at this! I just rebased all the branches again for the integration review.

            Show
            blepoxp Glenn Ansley added a comment - Hey, thanks for looking at this! I just rebased all the branches again for the integration review.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi Glenn,

            the checks seems perfect, so no problem with them, good work! But I cannot agree with the "mechanisms" used to output information:

            1) Although there are more wrong uses, I would not add more "harcoded" strings there. They should be proper lang strings, unless there is any reason for that. This applies to:

            • The $user_fields[xxx]['label'].
            • The $OUTPUT->notification use.
            • The error() use
            • The echos within grade_import_commit()

            2) error() is deprecated, surely the final status should be notified with some $OUTPUT->notification() too, allowing the script to end, closing files, printing footers...

            3) The way you are capturing problems in grade_import_commit() is not nice. Surely, one way to achieve the same in a BC way is to accumulate all the errors as they happen into one $executionerrors[] array and, before returning with false, check if $verbose is set and printout all them if so (remember that they should be proper lang strings in any case).

            4) Backporting this to 1.9 is, initially, not allowed (support ended). If there is strong interest on that, ask Martin, he has the key to unlock that.

            Many thanks for the effort, the issue is really awful, and your solution seems really good, just a matter of improve a bit the output.

            I'm reopening this now, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi Glenn, the checks seems perfect, so no problem with them, good work! But I cannot agree with the "mechanisms" used to output information: 1) Although there are more wrong uses, I would not add more "harcoded" strings there. They should be proper lang strings, unless there is any reason for that. This applies to: The $user_fields [xxx] ['label'] . The $OUTPUT->notification use. The error() use The echos within grade_import_commit() 2) error() is deprecated, surely the final status should be notified with some $OUTPUT->notification() too, allowing the script to end, closing files, printing footers... 3) The way you are capturing problems in grade_import_commit() is not nice. Surely, one way to achieve the same in a BC way is to accumulate all the errors as they happen into one $executionerrors[] array and, before returning with false, check if $verbose is set and printout all them if so (remember that they should be proper lang strings in any case). 4) Backporting this to 1.9 is, initially, not allowed (support ended). If there is strong interest on that, ask Martin, he has the key to unlock that. Many thanks for the effort, the issue is really awful, and your solution seems really good, just a matter of improve a bit the output. I'm reopening this now, ciao
            Hide
            blepoxp Glenn Ansley added a comment -

            Hi Eloy,
            Thanks for your feedback. I imagine that most if the output mechanisms were overlooked by our team because we created the the fix for our 1.9 version of Moodle and then reapplied it to the 2.0+ versions at the last minute when submitting the patch. We weren't happy with output buffering either but noticed it was used elsewhere in core... we'll make suggested improvements and resubmit though. Thanks again!

            Show
            blepoxp Glenn Ansley added a comment - Hi Eloy, Thanks for your feedback. I imagine that most if the output mechanisms were overlooked by our team because we created the the fix for our 1.9 version of Moodle and then reapplied it to the 2.0+ versions at the last minute when submitting the patch. We weren't happy with output buffering either but noticed it was used elsewhere in core... we'll make suggested improvements and resubmit though. Thanks again!
            Hide
            blepoxp Glenn Ansley added a comment -

            Updated my patch per suggestions. Removed reference to 1.9 branch and added 22_STABLE branch. Rebased 20, 21, and master.

            I anticipate needed to clean up this work some more based on Moodle standards so don't be shy, just tell me what needs to change. I was uncertain about the objects I created for the strings.

            Suggestions?

            Show
            blepoxp Glenn Ansley added a comment - Updated my patch per suggestions. Removed reference to 1.9 branch and added 22_STABLE branch. Rebased 20, 21, and master. I anticipate needed to clean up this work some more based on Moodle standards so don't be shy, just tell me what needs to change. I was uncertain about the objects I created for the strings. Suggestions?
            Hide
            phalacee Jason Fowler added a comment -

            Looks better Glenn, good to go

            Show
            phalacee Jason Fowler added a comment - Looks better Glenn, good to go
            Hide
            blepoxp Glenn Ansley added a comment -

            Just rebased again. I don't have ability to submit for integration review though.

            Show
            blepoxp Glenn Ansley added a comment - Just rebased again. I don't have ability to submit for integration review though.
            Hide
            andyjdavis Andrew Davis added a comment -

            This seems to have fallen through the cracks.

            "continue 3;" <-- I had honestly never seen that before.

            Hi Glenn, could you please reword the testing instructions? Whoever tests this will be running your code before they ever see your testing instructions so there's no value in mentioning the behaviour before your fix. Just give some instructions on how someone can verify that your fix was correctly integrated and that it appears to be working fine.

            I'll message Jason to get him to come and have another look and to get this into integration.

            Show
            andyjdavis Andrew Davis added a comment - This seems to have fallen through the cracks. "continue 3;" <-- I had honestly never seen that before. Hi Glenn, could you please reword the testing instructions? Whoever tests this will be running your code before they ever see your testing instructions so there's no value in mentioning the behaviour before your fix. Just give some instructions on how someone can verify that your fix was correctly integrated and that it appears to be working fine. I'll message Jason to get him to come and have another look and to get this into integration.
            Hide
            blepoxp Glenn Ansley added a comment -

            Testing instructions updated.

            Show
            blepoxp Glenn Ansley added a comment - Testing instructions updated.
            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for reporting this issue.

            We have detected that this issue has been inactive for over a year. It was reported as affecting versions that are no longer supported.

            If you believe that this issue is still relevant to current versions (2.5 and beyond), please comment on the issue. Issues left inactive for a further month will be closed.

            Michael d.

            TW9vZGxlDQo=

            Show
            salvetore Michael de Raadt added a comment - Thanks for reporting this issue. We have detected that this issue has been inactive for over a year. It was reported as affecting versions that are no longer supported. If you believe that this issue is still relevant to current versions (2.5 and beyond), please comment on the issue. Issues left inactive for a further month will be closed. Michael d. TW9vZGxlDQo=
            Hide
            jrchamp Jonathan Champ added a comment -

            Visual comparison of the Pull Master Diff URL and current master indicates that few if any conflicts exist for the supplied patch.

            Given that there were issues with blank user mappings in the past and that the patch appears to apply cleanly, this is still relevant for current versions. Additionally, it avoids possible conflicts with other grade import processes and benefits from possible SQL query plan optimizations.

            Show
            jrchamp Jonathan Champ added a comment - Visual comparison of the Pull Master Diff URL and current master indicates that few if any conflicts exist for the supplied patch. Given that there were issues with blank user mappings in the past and that the patch appears to apply cleanly, this is still relevant for current versions. Additionally, it avoids possible conflicts with other grade import processes and benefits from possible SQL query plan optimizations.
            Hide
            jrchamp Jonathan Champ added a comment -

            Requesting for peer review is the only useful button I can click to try and get this back in the queue. Andrew Davis asked for testing instructions and Glenn provided them.

            Show
            jrchamp Jonathan Champ added a comment - Requesting for peer review is the only useful button I can click to try and get this back in the queue. Andrew Davis asked for testing instructions and Glenn provided them.
            Hide
            cibot CiBoT added a comment -

            Results for MDL-29572

            Show
            cibot CiBoT added a comment - Results for MDL-29572 Remote repository: https://github.com/ncsu-delta/moodle Remote branch MDL-29572 _Fix-csv-import-for-grades_Master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2199 Error: The MDL-29572 _Fix-csv-import-for-grades_Master branch at https://github.com/ncsu-delta/moodle is very old (>60 days ago). Please rebase against current master.
            Hide
            jrchamp Jonathan Champ added a comment -

            Rebased to current.

            Show
            jrchamp Jonathan Champ added a comment - Rebased to current.
            Show
            cibot CiBoT added a comment - Results for MDL-29572 Remote repository: https://github.com/jrchamp/moodle Remote branch MDL-29572 _moodle26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2400 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2400/artifact/work/smurf.html Remote branch MDL-29572 _master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2401 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2401/artifact/work/smurf.html
            Hide
            jrchamp Jonathan Champ added a comment -

            Fixed issues mentioned by CiBoT.

            Show
            jrchamp Jonathan Champ added a comment - Fixed issues mentioned by CiBoT.
            Show
            cibot CiBoT added a comment - Results for MDL-29572 Remote repository: https://github.com/jrchamp/moodle Remote branch MDL-29572 _moodle26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2406 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2406/artifact/work/smurf.html Remote branch MDL-29572 _master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2407 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2407/artifact/work/smurf.html
            Hide
            poltawski Dan Poltawski added a comment -

            Sending all 'waiting for peer review' issues for integration review. The integration team are doing this to ensure any 'integratable issues' will not got missed for freeze.

            Note: We will prioritise peer reviewed issues and may not spend as much time examining non-integratable, non peer-reviewed issues.

            This is a present from the iTeam - it means that peer review is not working well enough! We really do not want to do this again! Lets improve our peer review process!

            Show
            poltawski Dan Poltawski added a comment - Sending all 'waiting for peer review' issues for integration review. The integration team are doing this to ensure any 'integratable issues' will not got missed for freeze. Note: We will prioritise peer reviewed issues and may not spend as much time examining non-integratable, non peer-reviewed issues. This is a present from the iTeam - it means that peer review is not working well enough! We really do not want to do this again! Lets improve our peer review process!
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks for this work everyone! I've integrated the fix to master, 26 and 25

            Show
            poltawski Dan Poltawski added a comment - Thanks for this work everyone! I've integrated the fix to master, 26 and 25
            Hide
            markn Mark Nelson added a comment -

            Hi Jonathon, thanks for your work on this. However, can you please update the testing instructions. When writing the testing instructions assume the user is on a fresh Moodle site, so tell them what is required in the course/site etc. For example, mention that you will need 10 users with the following email addresses in order to successfully map the file. With the current steps I can not submit the CSV file as it is without receiving an error about an invalid grade.

            Show
            markn Mark Nelson added a comment - Hi Jonathon, thanks for your work on this. However, can you please update the testing instructions. When writing the testing instructions assume the user is on a fresh Moodle site, so tell them what is required in the course/site etc. For example, mention that you will need 10 users with the following email addresses in order to successfully map the file. With the current steps I can not submit the CSV file as it is without receiving an error about an invalid grade.
            Hide
            markn Mark Nelson added a comment -

            Thanks Jonathan, you're a champ! Hehe, sorry - bad joke and one I am sure you have heard many times before. This works as expected. Thanks for contributing to Moodle!

            Show
            markn Mark Nelson added a comment - Thanks Jonathan, you're a champ! Hehe, sorry - bad joke and one I am sure you have heard many times before. This works as expected. Thanks for contributing to Moodle!
            Hide
            jrchamp Jonathan Champ added a comment -

            Huzzah! I'm delighted to see this move in the right direction. Also, as puns go, I enjoy that one. Thank you for your time and effort getting this tested.

            Show
            jrchamp Jonathan Champ added a comment - Huzzah! I'm delighted to see this move in the right direction. Also, as puns go, I enjoy that one. Thank you for your time and effort getting this tested.
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks for your efforts, this change is now part of Moodle!

            Show
            poltawski Dan Poltawski added a comment - Thanks for your efforts, this change is now part of Moodle!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  12/May/14