Moodle
  1. Moodle
  2. MDL-35850

Input data should be trimmed during bulk user upload

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4
    • Component/s: Administration
    • Labels:
    • Testing Instructions:
      Hide

      Testing

      1. Create a database activity with two fields. Make the last field a textarea.
      2. Make sure that you save the templates to display entries in the database.
      3. Create a csv file with some entries. Make sure that there are several blank lines after your entries.
      4. Go to [Settings ► Database activity administration ► Import entries]. Choose your csv file and submit it.
        The entries should enter in the database with no errors
      Show
      Testing Create a database activity with two fields. Make the last field a textarea. Make sure that you save the templates to display entries in the database. Create a csv file with some entries. Make sure that there are several blank lines after your entries. Go to [Settings ► Database activity administration ► Import entries] . Choose your csv file and submit it. The entries should enter in the database with no errors
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      wip-MDL-35850-master
    • Rank:
      44613

      Description

      When you try to import a csv file which contain spaces after the delimiter, it doesnt work since the fieldname in the csv doesnt match the one in database because of extra space in the beginning or end.

      1. Goto admin>users>upload bulk users
      2. upload a valid csv file with spaces after "," (Example:- username, password, firstname, lastname, email)
      3. It generates an error when it shouldnt

        Issue Links

          Activity

          Hide
          Jason Fowler added a comment -

          Hi Adrian, looked over the code. Very simple, very straight forward, just wondering if it would be better to use trim() as opposed to rtrim() ?

          Show
          Jason Fowler added a comment - Hi Adrian, looked over the code. Very simple, very straight forward, just wondering if it would be better to use trim() as opposed to rtrim() ?
          Hide
          Adrian Greeve added a comment -

          Thanks Jason.

          You're right. I was thinking before that we were not likely to get many files with new lines and such at the start of the file, but this being Moodle you can't really trust anything that is coming in from the public.

          I've changed the rtrim to trim.

          Sending to integration.

          Show
          Adrian Greeve added a comment - Thanks Jason. You're right. I was thinking before that we were not likely to get many files with new lines and such at the start of the file, but this being Moodle you can't really trust anything that is coming in from the public. I've changed the rtrim to trim. Sending to integration.
          Hide
          Aparup Banerjee added a comment -

          Hi Adrian,
          just wondering if this is in lines with the RFC or are we straying and if that matters at all or not.

          also is this for backporting?

          Show
          Aparup Banerjee added a comment - Hi Adrian, just wondering if this is in lines with the RFC or are we straying and if that matters at all or not. also is this for backporting?
          Hide
          Aparup Banerjee added a comment -

          Thanks, integrated into master only.

          Show
          Aparup Banerjee added a comment - Thanks, integrated into master only.
          Hide
          Andrew Davis added a comment -

          Here is the contents of my csv file. It imports successfully but I get the following php warnings. My database activity contains a URL and a text area field.

          myblog, about me
          http://bob.com,I am bob. Fear me.
          http://joe.com, I am joe. I like Joe. Joe good.
          http://blamo.com,blamo i am.
          
          
          
          Notice: Undefined offset: 1 in /home/andrew/Desktop/code/moodle/int/master/mod/data/import.php on line 162 Added 1. Entry (ID 1)
          Notice: Undefined offset: 1 in /home/andrew/Desktop/code/moodle/int/master/mod/data/import.php on line 162 Added 2. Entry (ID 2)
          Notice: Undefined offset: 1 in /home/andrew/Desktop/code/moodle/int/master/mod/data/import.php on line 162 Added 3. Entry (ID 3)

          Is that bad enough to warrant failing this or should I open a new bug?

          If I put a newline in the middle of the file (which is admittedly not something that should happen) I get a fatal error.

          myblog, about me
          http://bob.com,I am bob. Fear me.
          http://joe.com, I am joe. I like Joe. Joe good.
          
          http://blamo.com,blamo i am.
          
          
          
          Unable to read the raw data from the CSV file
          
          More information about this error
          Debug info:
          Error code: csvfailed
          Stack trace:
          
              line 467 of /lib/setuplib.php: moodle_exception thrown
              line 107 of /mod/data/import.php: call to print_error()
          
          Show
          Andrew Davis added a comment - Here is the contents of my csv file. It imports successfully but I get the following php warnings. My database activity contains a URL and a text area field. myblog, about me http: //bob.com,I am bob. Fear me. http: //joe.com, I am joe. I like Joe. Joe good. http: //blamo.com,blamo i am. Notice: Undefined offset: 1 in /home/andrew/Desktop/code/moodle/ int /master/mod/data/ import .php on line 162 Added 1. Entry (ID 1) Notice: Undefined offset: 1 in /home/andrew/Desktop/code/moodle/ int /master/mod/data/ import .php on line 162 Added 2. Entry (ID 2) Notice: Undefined offset: 1 in /home/andrew/Desktop/code/moodle/ int /master/mod/data/ import .php on line 162 Added 3. Entry (ID 3) Is that bad enough to warrant failing this or should I open a new bug? If I put a newline in the middle of the file (which is admittedly not something that should happen) I get a fatal error. myblog, about me http: //bob.com,I am bob. Fear me. http: //joe.com, I am joe. I like Joe. Joe good. http: //blamo.com,blamo i am. Unable to read the raw data from the CSV file More information about this error Debug info: Error code: csvfailed Stack trace: line 467 of /lib/setuplib.php: moodle_exception thrown line 107 of /mod/data/ import .php: call to print_error()
          Hide
          Andrew Davis added a comment -

          After some discussion Im passing this. I have split out those issues into two new MDLs which are linked to this issue. One bug forward, two bugs back.

          Show
          Andrew Davis added a comment - After some discussion Im passing this. I have split out those issues into two new MDLs which are linked to this issue. One bug forward, two bugs back.
          Hide
          Aparup Banerjee added a comment -

          Your issue has dug up some gold.
          It works great i've been told.
          Go forth, be brave, be bold.

          yay! "All your thoughts are belong to everyone."

          Thanks and ciao!

          Show
          Aparup Banerjee added a comment - Your issue has dug up some gold. It works great i've been told. Go forth, be brave, be bold. yay! "All your thoughts are belong to everyone." Thanks and ciao!
          Hide
          Mark Nelson added a comment -

          Did this actually trim the individual values? It seems it just did a trim for the whole content of the file. See MDL-42547 where this does not actually trim the email at all.

          Show
          Mark Nelson added a comment - Did this actually trim the individual values? It seems it just did a trim for the whole content of the file. See MDL-42547 where this does not actually trim the email at all.
          Hide
          Adrian Greeve added a comment -

          No this trims the start and the end of the whole document, not individual values.

          Show
          Adrian Greeve added a comment - No this trims the start and the end of the whole document, not individual values.
          Hide
          Mark Nelson added a comment -

          "When you try to import a csv file which contain spaces after the delimiter, it doesnt work since the fieldname in the csv doesnt match the one in database because of extra space in the beginning or end." - the delimiter being the comma. This does not trim input data, it does the whole file.

          Show
          Mark Nelson added a comment - "When you try to import a csv file which contain spaces after the delimiter, it doesnt work since the fieldname in the csv doesnt match the one in database because of extra space in the beginning or end." - the delimiter being the comma. This does not trim input data, it does the whole file.
          Hide
          Mark Nelson added a comment -

          Lol, seems there was some misunderstanding during the progress of this issue. Oh well, trimming the whole file won't cause any harm.

          Show
          Mark Nelson added a comment - Lol, seems there was some misunderstanding during the progress of this issue. Oh well, trimming the whole file won't cause any harm.
          Hide
          Adrian Greeve added a comment -

          Actually not trimming the file results in errors when trying to convert the csv into an array. So this is a necessary fix. After talking with Fred, I feel that trimming each value would result in other problems down the line. It is not always guaranteed that the information you are trying to match or include shouldn't start with a space. Course short name for example will save in the database with a leading space. If we are using some of the functionality that Fred added with the bulk course functions, then it will break if we trim each csv field.

          I would suggest that data be sanitized elsewhere when we know for sure that a space should not be included at the start.

          Show
          Adrian Greeve added a comment - Actually not trimming the file results in errors when trying to convert the csv into an array. So this is a necessary fix. After talking with Fred, I feel that trimming each value would result in other problems down the line. It is not always guaranteed that the information you are trying to match or include shouldn't start with a space. Course short name for example will save in the database with a leading space. If we are using some of the functionality that Fred added with the bulk course functions, then it will break if we trim each csv field. I would suggest that data be sanitized elsewhere when we know for sure that a space should not be included at the start.
          Hide
          Mark Nelson added a comment -

          Sure, then the description or the issue should have been changed, or a new one created. As this one clearly states that the trimming happens with each value, not the whole file.

          Show
          Mark Nelson added a comment - Sure, then the description or the issue should have been changed, or a new one created. As this one clearly states that the trimming happens with each value, not the whole file.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: