Moodle
  1. Moodle
  2. MDL-35678

Grade CSV Import does not comply with RFC 4180 Standards

    Details

    • Testing Instructions:
      Hide

      Pre-requisites

      • A course with a couple of assignment modules added.
      • Enrolled students.

      Download the gradebook

      1. Select a course.
      2. Go to [Settings ► Course administration ► Grades ► Export ► Plain text file]
      3. Tick "include feedback in export" and then submit.
      4. You will see a preview of what you are about to download. Click "Download".
      5. Save the file to your machine.

      Alter the downloaded csv file.

      1. Open the csv file in the editor of your choice.
      2. Alter the grades to some or all of the assignments.
      3. Provide feedback to some of the students. Enter in commas and new lines.
      4. Save the file (Make sure to keep it in csv format if you are using excel or Open office).

      Upload the csv file.

      1. Go to [Settings ► Course administration ► Grades ► Import ► CSV file]
      2. Upload your altered csv file.
      3. Make sure that "Separator" is selected and click "Upload grades"
        • Mapping of the csv file
        1. In the "Identify user by" section choose "Email address" for "Map from" and "useremail" for "Map to"
        2. In the "Grade item mappings" section map the assignment grades with the assignment in 'Grade items'.
        3. In the "Grade item mappings" section map the assignment feedback with the assignment in 'Comments'.
      4. Click "Upload grades".
      • You should see a message that says "Grade import success"
      • Click "Continue and observe that the grades and feeback are present
      Show
      Pre-requisites A course with a couple of assignment modules added. Enrolled students. Download the gradebook Select a course. Go to [Settings ► Course administration ► Grades ► Export ► Plain text file] Tick "include feedback in export" and then submit. You will see a preview of what you are about to download. Click "Download". Save the file to your machine. Alter the downloaded csv file. Open the csv file in the editor of your choice. Alter the grades to some or all of the assignments. Provide feedback to some of the students. Enter in commas and new lines. Save the file (Make sure to keep it in csv format if you are using excel or Open office). Upload the csv file. Go to [Settings ► Course administration ► Grades ► Import ► CSV file] Upload your altered csv file. Make sure that "Separator" is selected and click "Upload grades" Mapping of the csv file In the "Identify user by" section choose "Email address" for "Map from" and "useremail" for "Map to" In the "Grade item mappings" section map the assignment grades with the assignment in 'Grade items'. In the "Grade item mappings" section map the assignment feedback with the assignment in 'Comments'. Click "Upload grades". You should see a message that says "Grade import success" Click "Continue and observe that the grades and feeback are present
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-35678-master
    • Rank:
      44415

      Description

      The grade CSV import functionality still does NOT comply with the RFC 4180 standards, even though there is an almost identical issue that has been closed (MDL-25938).

      MDL-25938 was set to closed(duplicate) based on MDL-34075 which doesn't actually resolve the problem as the gradebook makes no use whatsoever of the csvlib, which is what's been fixed.

      Indications it's still broken (taken from git.moodle.org on 27-09-2012):

      grade/import/csv/index.php
      96     $headers = fgets($fp, GRADE_CSV_LINE_LENGTH);
      97     $header = explode($csv_delimiter, $headers);
      
      132     // --- get header (field names) ---
      133     $header = explode($csv_delimiter, fgets($fp, GRADE_CSV_LINE_LENGTH));
      
      147         $lines = explode($csv_delimiter, fgets($fp, GRADE_CSV_LINE_LENGTH));
      
      175         // --- get header (field names) ---
      176         $header = explode($csv_delimiter, clean_param(fgets($fp,GRADE_CSV_LINE_LENGTH), PARAM_RAW));
      
      223         // read the first line makes sure this doesn't get read again
      224         $header = explode($csv_delimiter, fgets($fp, GRADE_CSV_LINE_LENGTH));
      
      231             $line = explode($csv_delimiter, fgets($fp, GRADE_CSV_LINE_LENGTH));
      

      None of those lines will properly decode a CSV record that
      a) contains a comma as part of it's data
      b) is a multi-line record

      Either use the built in CSV library or replace all of those calls to fgetcsv.

        Issue Links

          Activity

          Hide
          Andrew Davis added a comment -

          Reassigning to Adrian as he dealt with MDL-25938

          Show
          Andrew Davis added a comment - Reassigning to Adrian as he dealt with MDL-25938
          Hide
          Frédéric Massart added a comment -

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

          Hi Adrian, I only have a few comments on the patch, but that has nothing to do with the logic which seem fine and working.

          As I went through the file, I noticed that this code is getting really confusing and is very poorly documented. I think some lines could be made more readable by changing their emplacement and add a comment on what you are doing. Of course your patch does not make it worse, but that could be a good time to add a few things. (IE: what are you doing at line 96?)

          Regarding the commit message, I think the correct component name for the gradebook would be core_grades.

          I quite dislike the fact that we are creating the $mform2 twice. It seems like the only reason is to set the $header. Would there be another way to keep the $mform2 object but adding this parameter?

          As you are adding a new require_once() which uses parenthesis, maybe you could fix the other dependencies to use parenthesis and absolute paths.

          The default value of the optional_param() could be an false/null instead of an empty string. I personaly like it better when it comes to check if a variable has been defined (line 96). Btw, why the new line in between $iid and the other params?

          Why are we declaring $header (on line 75) as an empty string, perhaps null would be safer? (It could actually be closer to where it is used, like at line 95)

          Line 162, the indentation is incorrect. And is this foreach really required now that we are using the class?

          Line 220, as far as I understood the logic here, $csv_encode does not seem to be used at all as we are parsing the value from the class. (It is declared around line 60)

          Why did you remove the parameter $verbosescales from $mform2? How does it get to the confirmation page?

          There might be a few whitespaces/periods here and there that could be added/removed.

          Thanks!

          Show
          Frédéric Massart added a comment - [Y] Syntax [-] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing [Y] Security [N] Documentation [N] Git [N] Sanity check Hi Adrian, I only have a few comments on the patch, but that has nothing to do with the logic which seem fine and working. As I went through the file, I noticed that this code is getting really confusing and is very poorly documented. I think some lines could be made more readable by changing their emplacement and add a comment on what you are doing. Of course your patch does not make it worse, but that could be a good time to add a few things. (IE: what are you doing at line 96?) Regarding the commit message, I think the correct component name for the gradebook would be core_grades. I quite dislike the fact that we are creating the $mform2 twice. It seems like the only reason is to set the $header. Would there be another way to keep the $mform2 object but adding this parameter? As you are adding a new require_once() which uses parenthesis, maybe you could fix the other dependencies to use parenthesis and absolute paths. The default value of the optional_param() could be an false/null instead of an empty string. I personaly like it better when it comes to check if a variable has been defined (line 96). Btw, why the new line in between $iid and the other params? Why are we declaring $header (on line 75) as an empty string, perhaps null would be safer? (It could actually be closer to where it is used, like at line 95) Line 162, the indentation is incorrect. And is this foreach really required now that we are using the class? Line 220, as far as I understood the logic here, $csv_encode does not seem to be used at all as we are parsing the value from the class. (It is declared around line 60) Why did you remove the parameter $verbosescales from $mform2? How does it get to the confirmation page? There might be a few whitespaces/periods here and there that could be added/removed. Thanks!
          Hide
          Adrian Greeve added a comment -

          Hi Fred,

          Thanks for the comments. I've gone through and made some changes.

          1. More comments have been included in the file.
          2. I think that the git commit message with the component of gradebook is fine.
          3. I've removed the second call for $mform2.
          4. All of the required variable are now in parenthesis.
          5. I changed the default value in this optional parameter to null.
          6. I removed the $header declaration.
          7. Indentation re-aligned.
          8. I replaced the $verbosescales parameter.
          9. I saw no whitespace problems with the commit.

          I've re-arranged the code a bit. I think that it makes more sense. Re-submitting for peer review.

          Show
          Adrian Greeve added a comment - Hi Fred, Thanks for the comments. I've gone through and made some changes. More comments have been included in the file. I think that the git commit message with the component of gradebook is fine. I've removed the second call for $mform2. All of the required variable are now in parenthesis. I changed the default value in this optional parameter to null. I removed the $header declaration. Indentation re-aligned. I replaced the $verbosescales parameter. I saw no whitespace problems with the commit. I've re-arranged the code a bit. I think that it makes more sense. Re-submitting for peer review.
          Hide
          Frédéric Massart added a comment -

          Thanks Adrian, this looks really good!

          Just a few minor comments, none of them are blockers. I'll let you decide whether this needs code update, or just some explanation (in code or on the tracker).

          • The constant GRADE_CSV_LINE_LENGTH does not seem to be used any more, should we keep it anyway?
          • When instantiating $mform you are passing true to $verbosescales, shouldn't that be the optional_param value?
          • I find 'includeseparator'=>!isset($CFG->CSV_DELIMITER) a bit confusing and hard to understand. Maybe just a small comment would help understanding.
          • The <div class=clearer> could be simplified in one call to html_writer::tag('div'), this is definitely trivial.
          • $csvimport and $header are defined twice but appear to be identical, is that required?
          • I see we clean $header after form submission, would that make sense to do it before, so that we would not end up having eventual wrong mappings? I didn't look at the logic.

          Cheers, feel free to push for integration whenever you like!

          Show
          Frédéric Massart added a comment - Thanks Adrian, this looks really good! Just a few minor comments, none of them are blockers. I'll let you decide whether this needs code update, or just some explanation (in code or on the tracker). The constant GRADE_CSV_LINE_LENGTH does not seem to be used any more, should we keep it anyway? When instantiating $mform you are passing true to $verbosescales, shouldn't that be the optional_param value? I find 'includeseparator'=>!isset($CFG->CSV_DELIMITER) a bit confusing and hard to understand. Maybe just a small comment would help understanding. The <div class=clearer> could be simplified in one call to html_writer::tag('div'), this is definitely trivial. $csvimport and $header are defined twice but appear to be identical, is that required? I see we clean $header after form submission, would that make sense to do it before, so that we would not end up having eventual wrong mappings? I didn't look at the logic. Cheers, feel free to push for integration whenever you like!
          Hide
          Adrian Greeve added a comment -

          Thanks Fred for the second look.

          Your points:

          1. I removed this line. You're right. This isn't used any more.
          2. When instantiaing $mform we want to show the option for verbose scales in the first form, so it's set as true, then we resubmit and this is where we check for the option of verbose scales with $mform2.
          3. I have to admit I'm not entirely sure as to why this is phrased as it it. I've changed it to true. I don't know why you wouldn't want to display the option for selecting the delimiter.
          4. I did try writing this as one call to html_writer, but it had issues displaying properly, that is why I've gone for two lines instead.
          5. After much fooling around I managed to find a way to remove a set of these call with out breaking the code.
          6. $header cleaning has been improved.

          Submitting for integration review.

          Show
          Adrian Greeve added a comment - Thanks Fred for the second look. Your points: I removed this line. You're right. This isn't used any more. When instantiaing $mform we want to show the option for verbose scales in the first form, so it's set as true, then we resubmit and this is where we check for the option of verbose scales with $mform2. I have to admit I'm not entirely sure as to why this is phrased as it it. I've changed it to true. I don't know why you wouldn't want to display the option for selecting the delimiter. I did try writing this as one call to html_writer, but it had issues displaying properly, that is why I've gone for two lines instead. After much fooling around I managed to find a way to remove a set of these call with out breaking the code. $header cleaning has been improved. Submitting for integration review.
          Hide
          Dan Poltawski added a comment -

          Thanks Adrian, i've integrated this now.

          I have to say I decided to be a friendly integrator for today, as I do not like the variable name '$iid' at all. Its not descriptive or meaningful to me:
          http://docs.moodle.org/dev/Coding_style#Variables

          But heres hoping the new functionality makes it worth it.

          Show
          Dan Poltawski added a comment - Thanks Adrian, i've integrated this now. I have to say I decided to be a friendly integrator for today, as I do not like the variable name '$iid' at all. Its not descriptive or meaningful to me: http://docs.moodle.org/dev/Coding_style#Variables But heres hoping the new functionality makes it worth it.
          Hide
          Dan Poltawski added a comment -

          I've added the qa_test_required flag. As I don't believe we have any QA tests for gradebook import.

          Show
          Dan Poltawski added a comment - I've added the qa_test_required flag. As I don't believe we have any QA tests for gradebook import.
          Hide
          David Monllaó added a comment -

          When I import the modified .csv file I receive this error:

          Error: mdb->get_record() found more than one record!
          line 1394 of /lib/dml/moodle_database.php: call to debugging()
          line 1354 of /lib/dml/moodle_database.php: call to moodle_database->get_record_sql()
          line 1333 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select()
          line 247 of /grade/import/csv/index.php: call to moodle_database->get_record()
          Error: mdb->get_record() found more than one record!
          line 1394 of /lib/dml/moodle_database.php: call to debugging()
          line 1354 of /lib/dml/moodle_database.php: call to moodle_database->get_record_sql()
          line 1333 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select()
          line 247 of /grade/import/csv/index.php: call to moodle_database->get_record()
          Error: mdb->get_record() found more than one record!
          line 1394 of /lib/dml/moodle_database.php: call to debugging()
          line 420 of /grade/import/csv/index.php: call to moodle_database->get_record_sql()
          
          Show
          David Monllaó added a comment - When I import the modified .csv file I receive this error: Error: mdb->get_record() found more than one record! line 1394 of /lib/dml/moodle_database.php: call to debugging() line 1354 of /lib/dml/moodle_database.php: call to moodle_database->get_record_sql() line 1333 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select() line 247 of /grade/ import /csv/index.php: call to moodle_database->get_record() Error: mdb->get_record() found more than one record! line 1394 of /lib/dml/moodle_database.php: call to debugging() line 1354 of /lib/dml/moodle_database.php: call to moodle_database->get_record_sql() line 1333 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select() line 247 of /grade/ import /csv/index.php: call to moodle_database->get_record() Error: mdb->get_record() found more than one record! line 1394 of /lib/dml/moodle_database.php: call to debugging() line 420 of /grade/ import /csv/index.php: call to moodle_database->get_record_sql()
          Hide
          David Monllaó added a comment -

          Thanks for reopening for testing.

          The data is saved correctly; the debugging messages appeared because in my test environment I have a MNET connection with 2 test sites with enrolments across sites, both sites uses the same preset of moodle users so there is more than one user with the same email address, the field that is mapped to import the data.

          Show
          David Monllaó added a comment - Thanks for reopening for testing. The data is saved correctly; the debugging messages appeared because in my test environment I have a MNET connection with 2 test sites with enrolments across sites, both sites uses the same preset of moodle users so there is more than one user with the same email address, the field that is mapped to import the data.
          Hide
          Dan Poltawski added a comment -

          Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week!

          ciao
          Dan

          Show
          Dan Poltawski added a comment - Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week! ciao Dan

            People

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

              Dates

              • Created:
                Updated:
                Resolved: