Moodle
  1. Moodle
  2. MDL-36112

Importing entries into a database activity generates php warnings

    Details

    • Testing Instructions:
      Hide

      Pre-requisites

      • Debugging should be set to "Developer" level.

      Testing: Import of csv file.

      1. Create a database activity.
      2. Create two fields: A URL and a text area.
      3. Create a csv file with contents similar to what is mentioned in the description.
      4. Import the file [Settings ► Database activity administration ► Import entries].
        Check that no php warnings are generated when importing the file.

      Testing: URL form aesthetics.

      1. Click the 'Add entry' tab.
        Observe that the 'Choose a link...' button is next to the URL field.
      2. Click the 'Fields' tab and edit the url field type.
      3. Check 'Autolink the URL'.
      4. Click the 'Add entry' tab.
        Observe that the 'Choose a link...' button is next to the URL field.
      Show
      Pre-requisites Debugging should be set to "Developer" level. Testing: Import of csv file. Create a database activity. Create two fields: A URL and a text area. Create a csv file with contents similar to what is mentioned in the description. Import the file [Settings ► Database activity administration ► Import entries] . Check that no php warnings are generated when importing the file. Testing: URL form aesthetics. Click the 'Add entry' tab. Observe that the 'Choose a link...' button is next to the URL field. Click the 'Fields' tab and edit the url field type. Check 'Autolink the URL'. Click the 'Add entry' tab. Observe that the 'Choose a link...' button is next to the URL field.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-36112-master
    • Rank:
      44878

      Description

      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)

        Issue Links

          Activity

          Hide
          Adrian Greeve added a comment -

          I found that mod/data/import.php line 159 was checking for both latlong and URL. URL actually only has one value and this is why the php warnings were being generated.

          Show
          Adrian Greeve added a comment - I found that mod/data/import.php line 159 was checking for both latlong and URL. URL actually only has one value and this is why the php warnings were being generated.
          Hide
          Frédéric Massart added a comment -

          Hi Adrian,

          this URL field is a bit confusing as depending on the settings, some parameters might be needed or not. I think we should keep the option for the second parameter as it can represent the "text" of the link, but is not required in all cases.

          When setting the URL field without a 'forced name' and with auto linking on, then there is an option to set the name of the link. On import we probably want to read that value as well, except that we should not throw a warning if it wasn't set (or we should depending on the settings of the field, your call.).

          I noticed another trivial issue (unrelated to your patch), which could be raised or fixed within this one. When adding a new URL field with auto linking and no forced name, the URL field in "Add entry" does not have the "Choose link" button next to it. That's a shame it is present when the field settings are different.

          Hope I made sense here.

          Thank you!
          Fred

          Show
          Frédéric Massart added a comment - Hi Adrian, this URL field is a bit confusing as depending on the settings, some parameters might be needed or not. I think we should keep the option for the second parameter as it can represent the "text" of the link, but is not required in all cases. When setting the URL field without a 'forced name' and with auto linking on, then there is an option to set the name of the link. On import we probably want to read that value as well, except that we should not throw a warning if it wasn't set (or we should depending on the settings of the field, your call.). I noticed another trivial issue (unrelated to your patch), which could be raised or fixed within this one. When adding a new URL field with auto linking and no forced name, the URL field in "Add entry" does not have the "Choose link" button next to it. That's a shame it is present when the field settings are different. Hope I made sense here. Thank you! Fred
          Hide
          Adrian Greeve added a comment -

          Thanks for the review Fred,

          Nice catch with the text field. I didn't spot that as I was trying different iterations of settings for the URL.
          The button display is definitely outside of the scope of this issue, but I've added a fix for that as well. Testing instructions have been updated and linked issues also updated.

          Show
          Adrian Greeve added a comment - Thanks for the review Fred, Nice catch with the text field. I didn't spot that as I was trying different iterations of settings for the URL. The button display is definitely outside of the scope of this issue, but I've added a fix for that as well. Testing instructions have been updated and linked issues also updated.
          Hide
          Adrian Greeve added a comment -

          Please note that I haven't changed the hard coded html. This is intentional to keep the appearance the same.

          Show
          Adrian Greeve added a comment - Please note that I haven't changed the hard coded html. This is intentional to keep the appearance the same.
          Hide
          Frédéric Massart added a comment -

          Thanks Adrian, things look good so I'm pushing for integration. I noticed the possibility of having a latlong field with only one value, but as you mentioned this will be rewritten in MDL-36274 it's all good. Cheers!

          Show
          Frédéric Massart added a comment - Thanks Adrian, things look good so I'm pushing for integration. I noticed the possibility of having a latlong field with only one value, but as you mentioned this will be rewritten in MDL-36274 it's all good. Cheers!
          Hide
          Dan Poltawski 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
          Dan Poltawski 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
          Adrian Greeve added a comment -

          Rebased and ready to go.

          Show
          Adrian Greeve added a comment - Rebased and ready to go.
          Hide
          Dan Poltawski added a comment -

          Integrated to master only. Thanks

          Show
          Dan Poltawski added a comment - Integrated to master only. Thanks
          Hide
          Rajesh Taneja added a comment -

          Thanks Adrian, for clear test instructions.
          Works Great.

          Show
          Rajesh Taneja added a comment - Thanks Adrian, for clear test instructions. Works Great.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Amazed. Inspired. Grateful. That’s how your generosity makes me feel.

          (not really)

          Closing, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Amazed. Inspired. Grateful. That’s how your generosity makes me feel. (not really) Closing, thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: