Moodle
  1. Moodle
  2. MDL-42618

Importing Grades via CSV with blank or whitespace as useridnumber

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major 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

          Activity

          Hide
          Matthew G. Switlik added a comment -

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

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

          Andrew,

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

          Show
          Matthew G. Switlik added a comment - Andrew, Eric Merrill instructed me to add you to this ticket since it's a gradebook issue.
          Hide
          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
          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 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 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 Glancy added a comment -

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

          Show
          Marina Glancy added a comment - looks like a good change, with or without parenthesis, submitting for integration
          Hide
          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
          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 added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          CiBoT added a comment -

          Results for MDL-42618

          • Remote repository: git://github.com/SWiT/moodle
          Show
          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
          Dan Poltawski added a comment -

          Integrated to master, 26 and 25 - thanks Matt!

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

          Thanks Matthew,

          All working as described - passing.

          Show
          Andrew Nicols added a comment - Thanks Matthew, All working as described - passing.
          Hide
          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
          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: