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

Importing Grades via CSV with blank or whitespace as useridnumber

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.5.3, 2.6.1
    • Fix Version/s: 2.5.5, 2.6.2
    • Component/s: Gradebook
    • Labels:
    • Testing Instructions:
      Hide

      Export your courses grades to a CSV file. Modify the CSV file to include a row with an idnumber of " " like the example John Doe in the included switlik-test1_Grades.csv file.

      Try to import the modified file based on ID number.

      You should receive an error saying 'user mapping error, could not find user with idnumber ""'.

      Show
      Export your courses grades to a CSV file. Modify the CSV file to include a row with an idnumber of " " like the example John Doe in the included switlik-test1_Grades.csv file. Try to import the modified file based on ID number. You should receive an error saying 'user mapping error, could not find user with idnumber ""'.
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-42618_master

      Description

      If a CSV file contains a row with a "ID number" set to a whitespace or blank. If you have deleted users in the system. It may cause a crash that leaves abandoned records in the grade_import_values and grade_import_newitem tables.

      Deleted users have a idnumber of "" in Moodle. At least they do in Moodle running on PostgreSQL as the DB.

      The import csv code checks if the user exists by doing a get_record() based solely on idnumber. It should handle a value of "" correctly and abort the import.

      The system will declare "Grade import success" with the following:
      This import included the following grades for users not currently enrolled in this course:
      User [DELETED USER] () on item

      If the system has a lot of deleted users the system may crash do to memory exhaustion. The example switlik-test1_Grades.csv file crashed my development server with a 3.5GB max memory setting but not on our production system with 4GB max memory set.

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            swit Matthew G. Switlik added a comment -

            I will make a MOODLE_26_STABLE patch branch once v2.6 releases.

            Show
            swit Matthew G. Switlik added a comment - I will make a MOODLE_26_STABLE patch branch once v2.6 releases.
            Hide
            swit Matthew G. Switlik added a comment -

            Andrew,

            Eric Merrill instructed me to add you to this ticket since it's a gradebook issue.

            Show
            swit Matthew G. Switlik added a comment - Andrew, Eric Merrill instructed me to add you to this ticket since it's a gradebook issue.
            Hide
            swit Matthew G. Switlik added a comment -

            This is the first issue I've addressed as a flagged "Developer". I ran through the http://docs.moodle.org/dev/Peer_reviewing_checklist but if there is anything I forgot or should change please let me know.

            Show
            swit Matthew G. Switlik added a comment - This is the first issue I've addressed as a flagged "Developer". I ran through the http://docs.moodle.org/dev/Peer_reviewing_checklist but if there is anything I forgot or should change please let me know.
            Hide
            marina Marina Glancy added a comment -

            Hello Matthew,
            thanks a lot for spotting and fixing this. The patch looks very good.
            One thing I'd like to ask you to change is to add parenthesis:

             if (empty($value) || !($user = $DB->get_record('user', array('idnumber' => $value)))) {

            It's a good practice when you use assignment and conditions in one statement even if it's ok in this particular case

            Show
            marina Marina Glancy added a comment - Hello Matthew, thanks a lot for spotting and fixing this. The patch looks very good. One thing I'd like to ask you to change is to add parenthesis: if (empty($value) || !($user = $DB->get_record('user', array('idnumber' => $value)))) { It's a good practice when you use assignment and conditions in one statement even if it's ok in this particular case
            Hide
            marina Marina Glancy added a comment -

            looks like a good change, with or without parenthesis, submitting for integration

            Show
            marina Marina Glancy added a comment - looks like a good change, with or without parenthesis, submitting for integration
            Hide
            stronk7 Eloy Lafuente (stronk7) 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
            stronk7 Eloy Lafuente (stronk7) 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
            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
            cibot CiBoT added a comment -

            Results for MDL-42618

            • Remote repository: git://github.com/SWiT/moodle
            Show
            cibot CiBoT added a comment - Results for MDL-42618 Remote repository: git://github.com/SWiT/moodle Remote branch https://github.com/SWiT/moodle/tree/MDL-42618_25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1530 Error: Unable to fetch information from https://github.com/SWiT/moodle/tree/MDL-42618_25 branch at git://github.com/SWiT/moodle. Remote branch https://github.com/SWiT/moodle/tree/MDL-42618_master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1531 Error: Unable to fetch information from https://github.com/SWiT/moodle/tree/MDL-42618_master branch at git://github.com/SWiT/moodle.
            Hide
            poltawski Dan Poltawski added a comment -

            Integrated to master, 26 and 25 - thanks Matt!

            Show
            poltawski Dan Poltawski added a comment - Integrated to master, 26 and 25 - thanks Matt!
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Thanks Matthew,

            All working as described - passing.

            Show
            dobedobedoh Andrew Nicols added a comment - Thanks Matthew, All working as described - passing.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            I claim to be a simple individual
            liable to err like any other fellow mortal.
            I own, however, that I have humility enough
            to confess my errors and to retrace my steps.

            Mahatma Gandhi

            Your awesome code has met upstream, closing, thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - I claim to be a simple individual liable to err like any other fellow mortal. I own, however, that I have humility enough to confess my errors and to retrace my steps. Mahatma Gandhi Your awesome code has met upstream, closing, thanks!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  10/Mar/14