Moodle
  1. Moodle
  2. MDL-31654

Upload of Users with Custom Profile Menu fields may fail

    Details

    • Database:
      PostgreSQL
    • Testing Instructions:
      Hide
      1. Log in as admin
      2. Create custom profile field "menu" of type menu. Options "one two three four five" (Site administration -> Users -> Accounts -> User profile fields)
      3. Try uploading attached file (Site administration -> Users -> Accounts -> Upload users)
      4. You should not see any error and two new users should be uploaded, check there profile and make sure menu has same option as csv file
      5. Now edit upload file and change menu entries to "4" and "2"
      6. Try upload and you should not any way to upload file. Also, error should be visible in status.
      Show
      Log in as admin Create custom profile field "menu" of type menu. Options "one two three four five" (Site administration -> Users -> Accounts -> User profile fields) Try uploading attached file (Site administration -> Users -> Accounts -> Upload users) You should not see any error and two new users should be uploaded, check there profile and make sure menu has same option as csv file Now edit upload file and change menu entries to "4" and "2" Try upload and you should not any way to upload file. Also, error should be visible in status.
    • Workaround:
      Hide

      Create Custom Profile yearlevel as text field

      Show
      Create Custom Profile yearlevel as text field
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      wip-mdl-31654
    • Rank:
      38227

      Description

      Uploading users that have a column for a Custom Profile field which is a menu may fail because the data from the CSV is directly treated as a key in the Menu Class.

      This means if you have user_profile_yearlevel = 9, it will return the value for key 9 or blank in custom profile field yearlevel which may or may not exist. If it did exist it could return the wrong value to be assigned to the yearlevel.

      Reproduction steps:

      1. Create custom profile of type menu.
      2. Assign various values to the menu.
      3. Create a csv to upload a value to the menu.
      4. Update existing users and Existing user details from the file will work.

      When the csv is processed you will see this error :-

      Error writing to database
      
      More information about this error
      
      Debug info: ERROR: null value in column "data" violates not-null constraint
      UPDATE mdl_user_info_data SET userid = $1,fieldid = $2,data = $3 WHERE id=$4
      [array (
      'userid' => '80',
      'fieldid' => '4',
      'data' => NULL,
      0 => '384',
      )]
      Stack trace:
      line 397 of /lib/dml/moodle_database.php: dml_write_exception thrown
      line 232 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end()
      line 936 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end()
      line 976 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->update_record_raw()
      line 118 of /user/profile/lib.php: call to pgsql_native_moodle_database->update_record()
      line 427 of /user/profile/lib.php: call to profile_field_base->edit_save_data()
      line 572 of /admin/uploaduser.php: call to profile_save_data()
      
      1. user.csv
        0.2 kB
        Michael de Raadt
      2. user.csv
        0.2 kB
        Rajesh Taneja

        Issue Links

          Activity

          Hide
          Tim Lock added a comment -

          Applies cleanly to 2.0, 2.1 & 2.2

          Show
          Tim Lock added a comment - Applies cleanly to 2.0, 2.1 & 2.2
          Hide
          Rajesh Taneja added a comment -

          Thanks for providing the patch Tim.
          You might want to check for field.class.php file before including it, as there can be custom profile fields and this might break.

          Show
          Rajesh Taneja added a comment - Thanks for providing the patch Tim. You might want to check for field.class.php file before including it, as there can be custom profile fields and this might break.
          Hide
          Michael de Raadt added a comment -

          Thanks for reporting this.

          Raj is keen to work on that very soon.

          Show
          Michael de Raadt added a comment - Thanks for reporting this. Raj is keen to work on that very soon.
          Hide
          Tim Lock added a comment -
          Show
          Tim Lock added a comment - I have added another patch. https://github.com/tlock/moodle/commit/97f4e51d4f4e7e4ba697880ecdc33480a3d2e691 Regards, Tim
          Hide
          Rajesh Taneja added a comment -

          Thanks for the great patch Tim,

          I have created branch with your patch, and did following modifications:

          1. In convert_csv_data function, for loop is replaced with array_search
          2. docblock added for pre_process_profile_data
          3. In 22 and master upload has been moved under tools.

          I still have one doubt about convert_csv_data returning '0' if value not found in options. Should user get error if value is not found or default value is acceptable? Will push this for peer-review to get more thoughts on this.

          Show
          Rajesh Taneja added a comment - Thanks for the great patch Tim, I have created branch with your patch, and did following modifications: In convert_csv_data function, for loop is replaced with array_search docblock added for pre_process_profile_data In 22 and master upload has been moved under tools. I still have one doubt about convert_csv_data returning '0' if value not found in options. Should user get error if value is not found or default value is acceptable? Will push this for peer-review to get more thoughts on this.
          Hide
          Petr Škoda added a comment -

          sorry, this is horrible:

          if ($field->datatype == 'menu') {
          

          please do not hardcode any plugin names in core! Things like this have to be implemented in plugins, not core.

          Also this is going to break backwards compatibility really badly for please that know how it works right now - files that work now would break after this change, right? Maybe numeric values could be treated as keys and non-numeric as values. In any case this must be documented in at least release notes.

          Show
          Petr Škoda added a comment - sorry, this is horrible: if ($field->datatype == 'menu') { please do not hardcode any plugin names in core! Things like this have to be implemented in plugins, not core. Also this is going to break backwards compatibility really badly for please that know how it works right now - files that work now would break after this change, right? Maybe numeric values could be treated as keys and non-numeric as values. In any case this must be documented in at least release notes.
          Hide
          Rajesh Taneja added a comment - - edited

          Thanks for the feedback Petr,

          I don't see why this will break backward compatibility. Also, we can't be sure that values are non-numeric, as they can be numeric as well.

          Removed Hardcoding from core, and will update docs, if this gets integrated.

          Show
          Rajesh Taneja added a comment - - edited Thanks for the feedback Petr, I don't see why this will break backward compatibility. Also, we can't be sure that values are non-numeric, as they can be numeric as well. Removed Hardcoding from core, and will update docs, if this gets integrated.
          Hide
          Petr Škoda added a comment -

          Backwards compatibility:
          1/ create file that uses numerical indexes - works at the moment 100%
          2/ apply your patch
          3/ test it - it should work, if not you have major BC problem

          Show
          Petr Škoda added a comment - Backwards compatibility: 1/ create file that uses numerical indexes - works at the moment 100% 2/ apply your patch 3/ test it - it should work, if not you have major BC problem
          Hide
          Rajesh Taneja added a comment -

          Thanks Petr,
          So do you recommend to have this documented, then to have this patch?
          or shall we have this in master and not back-port it?

          Show
          Rajesh Taneja added a comment - Thanks Petr, So do you recommend to have this documented, then to have this patch? or shall we have this in master and not back-port it?
          Hide
          Rajesh Taneja added a comment -

          As discussed in devchat, this patch can go in master and release doc for 2.3 should contain this information. Hence adding master branch and doc_required tag

          Show
          Rajesh Taneja added a comment - As discussed in devchat, this patch can go in master and release doc for 2.3 should contain this information. Hence adding master branch and doc_required tag
          Hide
          Rossiani Wijaya added a comment -

          Hi Raj,

          When entering in valid value for the custom profile menu, I still get the following error:

          Error writing to database
          
          More information about this error
          Debug info: Column 'data' cannot be null
          INSERT INTO mdl_user_info_data (userid,fieldid,data) VALUES(?,?,?)
          [array (
          0 => 378,
          1 => '127',
          2 => NULL,
          )]
          Stack trace:
          
              line 413 of /lib/dml/moodle_database.php: dml_write_exception thrown
              line 902 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end()
              line 944 of /lib/dml/mysqli_native_moodle_database.php: call to mysqli_native_moodle_database->insert_record_raw()
              line 120 of /user/profile/lib.php: call to mysqli_native_moodle_database->insert_record()
              line 427 of /user/profile/lib.php: call to profile_field_base->edit_save_data()
              line 747 of /admin/tool/uploaduser/index.php: call to profile_save_data()
          

          Isn't the patch should prevent displaying the above error?

          Show
          Rossiani Wijaya added a comment - Hi Raj, When entering in valid value for the custom profile menu, I still get the following error: Error writing to database More information about this error Debug info: Column 'data' cannot be null INSERT INTO mdl_user_info_data (userid,fieldid,data) VALUES(?,?,?) [array ( 0 => 378, 1 => '127', 2 => NULL, )] Stack trace: line 413 of /lib/dml/moodle_database.php: dml_write_exception thrown line 902 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end() line 944 of /lib/dml/mysqli_native_moodle_database.php: call to mysqli_native_moodle_database->insert_record_raw() line 120 of /user/profile/lib.php: call to mysqli_native_moodle_database->insert_record() line 427 of /user/profile/lib.php: call to profile_field_base->edit_save_data() line 747 of /admin/tool/uploaduser/index.php: call to profile_save_data() Isn't the patch should prevent displaying the above error?
          Hide
          Rajesh Taneja added a comment -

          I have added another commit to handle wrong data for custom menu profile field. I am not sure if this should be part of this bug as we never did any checks on custom profile fields and probably needs to be improved for all the required custom_profile_fields.

          Show
          Rajesh Taneja added a comment - I have added another commit to handle wrong data for custom menu profile field. I am not sure if this should be part of this bug as we never did any checks on custom profile fields and probably needs to be improved for all the required custom_profile_fields.
          Hide
          Rossiani Wijaya added a comment -

          Hi Raj,

          Some comments regarding the patch.

          1. On user/profile/field/menu/field.class.php line 97, you might want to change -1 to null (online comment).

          2. It would probably better to move function pre_process_profile_data() to user/profile/lib.php.

          3. On admin/tool/uploaduser/index.php, the content on line 1039 (to 1058) and pre_process_profile_data() look similar. The function could probably be modified to handle both cases.

          Show
          Rossiani Wijaya added a comment - Hi Raj, Some comments regarding the patch. 1. On user/profile/field/menu/field.class.php line 97, you might want to change -1 to null (online comment). 2. It would probably better to move function pre_process_profile_data() to user/profile/lib.php. 3. On admin/tool/uploaduser/index.php, the content on line 1039 (to 1058) and pre_process_profile_data() look similar. The function could probably be modified to handle both cases.
          Hide
          Rajesh Taneja added a comment - - edited

          Thanks Rossie,

          pre_process_profile_data can't be moved to uer/profile/lib.php, but can be moved to admin/tools/uploaduser/localib.php. As it is only related to user upload feature only. As per code-replication, in first case it's stdClass and updates the value. In second case it's an array and sets error message.

          Not sure if we should combine the two and make it complicated and update data-type.

          Show
          Rajesh Taneja added a comment - - edited Thanks Rossie, pre_process_profile_data can't be moved to uer/profile/lib.php, but can be moved to admin/tools/uploaduser/localib.php. As it is only related to user upload feature only. As per code-replication, in first case it's stdClass and updates the value. In second case it's an array and sets error message. Not sure if we should combine the two and make it complicated and update data-type.
          Hide
          Rajesh Taneja added a comment -

          Hello Rossie,

          As suggested, I have combined two loops.
          Can you please review it again.

          Show
          Rajesh Taneja added a comment - Hello Rossie, As suggested, I have combined two loops. Can you please review it again.
          Hide
          Rossiani Wijaya added a comment -

          It looks good Raj.

          Thanks for fixing.

          Show
          Rossiani Wijaya added a comment - It looks good Raj. Thanks for fixing.
          Hide
          Rajesh Taneja added a comment -

          Thanks Rossie,

          pushing it for integration review.

          Show
          Rajesh Taneja added a comment - Thanks Rossie, pushing it for integration review.
          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
          Rajesh Taneja added a comment -

          Branches re-based.

          Show
          Rajesh Taneja added a comment - Branches re-based.
          Hide
          Petr Škoda added a comment -

          to integrators: I still believe this might be viewed as regression if original CSV file with numeric menu indexes does not work, this would create hard to detect problems if menu uses numeric values. Change like this would need to be announced and documented.

          Show
          Petr Škoda added a comment - to integrators: I still believe this might be viewed as regression if original CSV file with numeric menu indexes does not work, this would create hard to detect problems if menu uses numeric values. Change like this would need to be announced and documented.
          Hide
          Aparup Banerjee added a comment -

          Hi Raj,

          I'm sending this back for further discussion.
          I feel the decision to affect systems out there to the extent that they might have to change other systems just to be able to upgrade to 2.3 should deserve a little more discussion.

          I've noted this in dev chat : http://moodle.org/local/chatlogs/index.php?conversationid=9615

          I think we should separate the solution (and its effects) from a design decision here too. Do we want to support numerical indexes? yes/no ? (we have for so long - what changed the design? )

          if yes - how?
          if no - you're solution seems fine but we need to plan this change so that future uptakes of the version can be planned for by others.

          also note :
          admin/tool/uploaduser/locallib.php:414: trailing whitespace.
          if (method_exists($formfield, 'convert_external_data') &&

          ps: Also noting that in a stable (fixes) sprint - we may not have given this enough design/supportability thought too.

          so simply - please discuss this change in support with more views/people.

          Show
          Aparup Banerjee added a comment - Hi Raj, I'm sending this back for further discussion. I feel the decision to affect systems out there to the extent that they might have to change other systems just to be able to upgrade to 2.3 should deserve a little more discussion. I've noted this in dev chat : http://moodle.org/local/chatlogs/index.php?conversationid=9615 I think we should separate the solution (and its effects) from a design decision here too. Do we want to support numerical indexes? yes/no ? (we have for so long - what changed the design? ) if yes - how? if no - you're solution seems fine but we need to plan this change so that future uptakes of the version can be planned for by others. also note : admin/tool/uploaduser/locallib.php:414: trailing whitespace. if (method_exists($formfield, 'convert_external_data') && ps: Also noting that in a stable (fixes) sprint - we may not have given this enough design/supportability thought too. so simply - please discuss this change in support with more views/people.
          Hide
          Rajesh Taneja added a comment -

          Thanks for the feedback Aparup,

          This was discussed in Devchat. Tim, Eloy agreed about having this in 2.3
          Not sure, about more discussion. As per rest of the fields, they expect value but with menu it expects index.
          You can see there is one duplicate for this issue, so seems it is a problem.

          As per stable, as this is change in functionality, I haven't back-ported this.

          @Petr: If this gets integrated, I will update the docs.

          Show
          Rajesh Taneja added a comment - Thanks for the feedback Aparup, This was discussed in Devchat. Tim, Eloy agreed about having this in 2.3 Not sure, about more discussion. As per rest of the fields, they expect value but with menu it expects index. You can see there is one duplicate for this issue, so seems it is a problem. As per stable, as this is change in functionality, I haven't back-ported this. @Petr: If this gets integrated, I will update the docs.
          Hide
          Petr Škoda added a comment - - edited

          Thanks everybody! I somehow missed the discussion in dev chat, sorry, sometimes it is better to copy paste any decisions from there into tracker.

          Show
          Petr Škoda added a comment - - edited Thanks everybody! I somehow missed the discussion in dev chat, sorry, sometimes it is better to copy paste any decisions from there into tracker.
          Hide
          Rajesh Taneja added a comment -

          Thanks Petr,

          My Bad, I should have copied the discussion. Will remember to paste it next time

          As per the whole conversation, it is a bug. Unfortunately, we have been living with it. So, 2.3 is a good option to correct it.

          Show
          Rajesh Taneja added a comment - Thanks Petr, My Bad, I should have copied the discussion. Will remember to paste it next time As per the whole conversation, it is a bug. Unfortunately, we have been living with it. So, 2.3 is a good option to correct it.
          Hide
          Rajesh Taneja added a comment -

          I have added Martin, Michael, Sam and Eloy as watchers to have there opinion on this.
          @Petr: If you think patch makes sense then please give +1 for it.

          Show
          Rajesh Taneja added a comment - I have added Martin, Michael, Sam and Eloy as watchers to have there opinion on this. @Petr: If you think patch makes sense then please give +1 for it.
          Hide
          Rajesh Taneja added a comment - - edited

          As mentioned by Rossie, about user upload showing exception. This is just a status message and will let user proceed. Updating user profile, doesn't mention about wrong fields if uploaded will not let you proceed (which is included in this patch). So doc update is required.

          Show
          Rajesh Taneja added a comment - - edited As mentioned by Rossie, about user upload showing exception. This is just a status message and will let user proceed. Updating user profile , doesn't mention about wrong fields if uploaded will not let you proceed (which is included in this patch). So doc update is required.
          Hide
          Michael de Raadt added a comment -

          I don't see a problem with this if we note this as a behavioural change in the release notes and update the user docs accordingly. The current labels are set correctly to prompt this.

          Show
          Michael de Raadt added a comment - I don't see a problem with this if we note this as a behavioural change in the release notes and update the user docs accordingly. The current labels are set correctly to prompt this.
          Hide
          Rajesh Taneja added a comment -

          Branches re-based.
          Hoping will get more +1's, by the time integrator reviews it

          Show
          Rajesh Taneja added a comment - Branches re-based. Hoping will get more +1's, by the time integrator reviews it
          Hide
          Aparup Banerjee added a comment -

          ok i suppose enough time has passed and hopefully more people see this coming.
          Thanks for the work here, its been integrated now and is ready for testing in master.

          This change should be highlighted release.

          ps: Rajesh, as spoken this was only master, futher to that i take it that there is no fix possible for stable branches.

          Show
          Aparup Banerjee added a comment - ok i suppose enough time has passed and hopefully more people see this coming. Thanks for the work here, its been integrated now and is ready for testing in master. This change should be highlighted release. ps: Rajesh, as spoken this was only master, futher to that i take it that there is no fix possible for stable branches.
          Hide
          Michael de Raadt added a comment -

          I've added a corrected CSV without quotes.

          Show
          Michael de Raadt added a comment - I've added a corrected CSV without quotes.
          Hide
          Michael de Raadt added a comment -

          Test result: Success.

          Tested in master with MySQL, PostgreSQL and MS SQL.

          Thanks to all involved.

          Show
          Michael de Raadt added a comment - Test result: Success. Tested in master with MySQL, PostgreSQL and MS SQL. Thanks to all involved.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this has landed upstream, finally! Yay!

          תודה רבה && شكرا جزيلا



          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this has landed upstream, finally! Yay! תודה רבה && شكرا جزيلا Closing, ciao
          Hide
          Helen Foster added a comment -

          Please could anyone advise what needs to be added to the user docs for this issue, or should the label be dev_docs? (I notice it's mentioned in http://docs.moodle.org/dev/Moodle_2.3_release_notes )

          Show
          Helen Foster added a comment - Please could anyone advise what needs to be added to the user docs for this issue, or should the label be dev_docs? (I notice it's mentioned in http://docs.moodle.org/dev/Moodle_2.3_release_notes )
          Hide
          Rajesh Taneja added a comment -

          Hello Helen,

          For custom profile fields that are a menu, it was mentioned to use corresponding menu number (index), and now it's the value itself.
          http://docs.moodle.org/22/en/Upload_users#Fields_that_can_be_included - Custom profile field names (first example, second line)

          Show
          Rajesh Taneja added a comment - Hello Helen, For custom profile fields that are a menu, it was mentioned to use corresponding menu number (index), and now it's the value itself. http://docs.moodle.org/22/en/Upload_users#Fields_that_can_be_included - Custom profile field names (first example, second line)
          Hide
          Helen Foster added a comment -

          Thanks Raj, http://docs.moodle.org/23/en/Upload_users#Fields_that_can_be_included now states:

          For custom profile fields that are a menu, use the corresponding values (new in Moodle 2.3 onwards).

          Example: if your custom field is department and there are three choices - HR, Marketing,Training- then use HR, Marketing and Training.

          Hope I've understood it correctly - please shout if not!

          Show
          Helen Foster added a comment - Thanks Raj, http://docs.moodle.org/23/en/Upload_users#Fields_that_can_be_included now states: For custom profile fields that are a menu, use the corresponding values (new in Moodle 2.3 onwards). Example: if your custom field is department and there are three choices - HR, Marketing,Training- then use HR, Marketing and Training. Hope I've understood it correctly - please shout if not!
          Hide
          Rajesh Taneja added a comment -

          Thanks Helen,

          This is perfect.

          Show
          Rajesh Taneja added a comment - Thanks Helen, This is perfect.
          Hide
          Martin Dougiamas added a comment - - edited

          "HR, Marketing and Training"

          The code will parse that "and"?

          Shouldn't it be something like:

          Example: A custom field 'Department' with one of three values 'HR', 'Marketing' or 'Training'. Just insert one of those three words (eg 'Training') as the value for that field.

          Show
          Martin Dougiamas added a comment - - edited "HR, Marketing and Training" The code will parse that "and"? Shouldn't it be something like: Example: A custom field 'Department' with one of three values 'HR', 'Marketing' or 'Training'. Just insert one of those three words (eg 'Training') as the value for that field.
          Hide
          Helen Foster added a comment -

          Thanks Martin, I have amended the example as suggested.

          Show
          Helen Foster added a comment - Thanks Martin, I have amended the example as suggested.

            People

            • Votes:
              5 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: