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

Input data should be trimmed during bulk user upload

    Details

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

      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

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            phalacee 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
            phalacee 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
            abgreeve 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
            abgreeve 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
            nebgor 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
            nebgor 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
            nebgor Aparup Banerjee added a comment -

            Thanks, integrated into master only.

            Show
            nebgor Aparup Banerjee added a comment - Thanks, integrated into master only.
            Hide
            andyjdavis 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
            andyjdavis 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
            andyjdavis 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
            andyjdavis 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
            nebgor 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
            nebgor 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
            markn 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
            markn 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
            abgreeve Adrian Greeve added a comment -

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

            Show
            abgreeve Adrian Greeve added a comment - No this trims the start and the end of the whole document, not individual values.
            Hide
            markn 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
            markn 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
            markn 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
            markn 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
            abgreeve 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
            abgreeve 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
            markn 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
            markn 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:
                  Fix Release Date:
                  3/Dec/12