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.

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:

Create custom profile of type menu.

Assign various values to the menu.

Create a csv to upload a value to the menu.

Update existing users and Existing user details from the file will work.

Rajesh Taneja
added a comment - 16/Feb/12 11:28 AM 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.

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.

Rajesh Taneja
added a comment - 27/Feb/12 1:58 PM 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.

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.

Petr Skoda
added a comment - 27/Feb/12 4:34 PM 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.

Rajesh Taneja
added a comment - 27/Feb/12 4:45 PM - 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.

Rossiani Wijaya
added a comment - 06/Mar/12 1:05 PM 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?

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.

Rajesh Taneja
added a comment - 06/Mar/12 3:28 PM 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.

Rossiani Wijaya
added a comment - 12/Mar/12 12:39 PM 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.

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.

Rajesh Taneja
added a comment - 12/Mar/12 1:26 PM - 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.

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.

Eloy Lafuente (stronk7)
added a comment - 15/Mar/12 11:21 PM 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

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.

Petr Skoda
added a comment - 20/Mar/12 5:56 AM 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.

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.

Aparup Banerjee
added a comment - 20/Mar/12 4:01 PM 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.

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.

Rajesh Taneja
added a comment - 20/Mar/12 5:34 PM 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.

Rajesh Taneja
added a comment - 21/Mar/12 9:46 AM 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.

Rajesh Taneja
added a comment - 21/Mar/12 10:01 AM 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.

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.

Rajesh Taneja
added a comment - 21/Mar/12 10:57 AM - 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.

Michael de Raadt
added a comment - 23/Mar/12 8:35 AM 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.

Aparup Banerjee
added a comment - 27/Mar/12 1:50 PM 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.

Helen Foster
added a comment - 23/Jul/12 9:19 PM 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 )

Rajesh Taneja
added a comment - 24/Jul/12 9:24 AM 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)

Helen Foster
added a comment - 24/Jul/12 4:56 PM 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!

Martin Dougiamas
added a comment - 24/Jul/12 5:11 PM - 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.