Moodle
  1. Moodle
  2. MDL-17344

Bulk upload user fails if shortname contains a capital letter

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.8.7, 1.9.3, 2.0
    • Fix Version/s: 2.0.3
    • Component/s: Administration
    • Labels:
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE, MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      4815

      Description

      When attempting to bulk upload users via a csv file it fails if the shortname contains a capital letter. We should probably ensure that the shortname does not contain any capital letters since it is functioning like a fieldname. I'm working on a patch that will add this check when the form validates. Peace - Anthony

        Issue Links

          Activity

          Hide
          Anthony Borrow added a comment -

          This patch adds the check and a language string. Peace - Anthony

          Show
          Anthony Borrow added a comment - This patch adds the check and a language string. Peace - Anthony
          Hide
          Anthony Borrow added a comment -

          We may also want to port this back to Moodle 1.8.

          Show
          Anthony Borrow added a comment - We may also want to port this back to Moodle 1.8.
          Hide
          Anthony Borrow added a comment -

          adding 1.8 as affected version

          Show
          Anthony Borrow added a comment - adding 1.8 as affected version
          Hide
          Anthony Borrow added a comment -

          Just adding a link to a forum post about a user affected by this issue (I think in 1.8) - http://moodle.org/mod/forum/discuss.php?d=107355#p474086 Peace - Anthony

          Show
          Anthony Borrow added a comment - Just adding a link to a forum post about a user affected by this issue (I think in 1.8) - http://moodle.org/mod/forum/discuss.php?d=107355#p474086 Peace - Anthony
          Hide
          Anthony Borrow added a comment -

          I have run into this issue again in another setting. Looking at the patch, I think we should also make it part of an upgrade and go through and change any existing user profile fields that contain a capital letter to all lowercase. We might also need to verify that the new lowercase version is in fact unique. I do not recall if it was possible to create case-insensitive duplicates (i.e. dob and DOB). If the lowercase version exists already then we would need to append a number to the shortname field name. Peace - Anthony

          Show
          Anthony Borrow added a comment - I have run into this issue again in another setting. Looking at the patch, I think we should also make it part of an upgrade and go through and change any existing user profile fields that contain a capital letter to all lowercase. We might also need to verify that the new lowercase version is in fact unique. I do not recall if it was possible to create case-insensitive duplicates (i.e. dob and DOB). If the lowercase version exists already then we would need to append a number to the shortname field name. Peace - Anthony
          Hide
          Anthony Borrow added a comment -

          I have added Kathreja as a watcher of this issue as she needs to know not to create custom user profile fields with capital letters - they should all be lower case. Peace - Anthony

          Show
          Anthony Borrow added a comment - I have added Kathreja as a watcher of this issue as she needs to know not to create custom user profile fields with capital letters - they should all be lower case. Peace - Anthony
          Hide
          Anthony Borrow added a comment -

          We need to either prevent admins from creating custom user profile field shortnames with caps (which is what the patch in MDL-17344 accomplishes) or do a better job of making the fieldname case insensitive. For simplicity, since the shortname is never really seen I would recommend just preventing folks from putting capital letters in it. Peace - Anthony

          Show
          Anthony Borrow added a comment - We need to either prevent admins from creating custom user profile field shortnames with caps (which is what the patch in MDL-17344 accomplishes) or do a better job of making the fieldname case insensitive. For simplicity, since the shortname is never really seen I would recommend just preventing folks from putting capital letters in it. Peace - Anthony
          Hide
          Helen Foster added a comment -

          Anthony, thanks for your report and patch. Apologies for the delay in responding. If you have chance, please could you confirm that this issue also affects 2.0.1.

          Show
          Helen Foster added a comment - Anthony, thanks for your report and patch. Apologies for the delay in responding. If you have chance, please could you confirm that this issue also affects 2.0.1.
          Hide
          Jack Challenger added a comment -

          I can confirm that this does affect 2.0.1 as well. Creating a user profile field with shortname starting with a capital is allowed, and the resultant .csv upload does not work. Thanks!

          Show
          Jack Challenger added a comment - I can confirm that this does affect 2.0.1 as well. Creating a user profile field with shortname starting with a capital is allowed, and the resultant .csv upload does not work. Thanks!
          Hide
          Anthony Borrow added a comment -

          Jacques - Thanks for confirming that the csv user import fails with a captial letter in 2.0.1. Peace - Anthony

          Show
          Anthony Borrow added a comment - Jacques - Thanks for confirming that the csv user import fails with a captial letter in 2.0.1. Peace - Anthony
          Hide
          Aparup Banerjee added a comment -

          wow, in mysql (case-insensitive) the shortname query seemed to maintain uniqueness, not for other DBs!

          i'm wondering about the upgrade-update of conflicting shortnames suggested by Anthony:
          'If the lowercase version exists already then we would need to append a number to the shortname field name'

          Maybe during the upgrade we should somehow be glaringly informative about this change.

          Show
          Aparup Banerjee added a comment - wow, in mysql (case-insensitive) the shortname query seemed to maintain uniqueness, not for other DBs! i'm wondering about the upgrade-update of conflicting shortnames suggested by Anthony: 'If the lowercase version exists already then we would need to append a number to the shortname field name' Maybe during the upgrade we should somehow be glaringly informative about this change.
          Hide
          Aparup Banerjee added a comment -

          Petr, is there any specific reason why uu_validate_user_upload_columns() has to convert the fieldnames to lowercase?

          they field names are after all stored as entered in user_info_field.shortname .

          Show
          Aparup Banerjee added a comment - Petr, is there any specific reason why uu_validate_user_upload_columns() has to convert the fieldnames to lowercase? they field names are after all stored as entered in user_info_field.shortname .
          Hide
          Aparup Banerjee added a comment -

          please see https://github.com/nebgor/moodle/compare/mistress...MDL-17344 for patch that allows uppercase in csv fieldnames , hmm i should probably just allow this only for profilefields...

          Show
          Aparup Banerjee added a comment - please see https://github.com/nebgor/moodle/compare/mistress...MDL-17344 for patch that allows uppercase in csv fieldnames , hmm i should probably just allow this only for profilefields...
          Hide
          Aparup Banerjee added a comment -

          ok, nope, had a talk with the guys and we do want lowercase. so using that patch from Anthony, also upgrading will just show a message in page when changing conflicting shortnames (appending 2)

          Show
          Aparup Banerjee added a comment - ok, nope, had a talk with the guys and we do want lowercase. so using that patch from Anthony, also upgrading will just show a message in page when changing conflicting shortnames (appending 2)
          Hide
          Aparup Banerjee added a comment -
          Show
          Aparup Banerjee added a comment - added rosie for review : https://github.com/nebgor/moodle/compare/mistress...MDL-17344 (updated)
          Show
          Aparup Banerjee added a comment - also created https://github.com/nebgor/moodle/compare/MOODLE_20_STABLE...MDL-17344_m20
          Hide
          Rossiani Wijaya added a comment -

          Apu the patch is great.

          Some suggestion for the patch:
          1. add field id to notification message
          2. instead of manually add '02' to new field name, create count loop, embedded to field name and check if the new field name is already exist. if it is exist, try the next number from the loop.

          Note: i'm not sure with the incremental number for the version file. Probably should ask for second opinion from Sam or Andrew

          Show
          Rossiani Wijaya added a comment - Apu the patch is great. Some suggestion for the patch: 1. add field id to notification message 2. instead of manually add '02' to new field name, create count loop, embedded to field name and check if the new field name is already exist. if it is exist, try the next number from the loop. Note: i'm not sure with the incremental number for the version file. Probably should ask for second opinion from Sam or Andrew
          Hide
          Aparup Banerjee added a comment -

          Thanks Rossi!
          both
          https://github.com/nebgor/moodle/compare/mistress...MDL-17344
          https://github.com/nebgor/moodle/compare/MOODLE_20_STABLE...MDL-17344_m20
          have been updated.

          version.php, i tried what i think is best, but it will hopefully be handled by the integrators as it is so confusing and theres wasn't any specific guidance on this yet

          Show
          Aparup Banerjee added a comment - Thanks Rossi! both https://github.com/nebgor/moodle/compare/mistress...MDL-17344 https://github.com/nebgor/moodle/compare/MOODLE_20_STABLE...MDL-17344_m20 have been updated. version.php, i tried what i think is best, but it will hopefully be handled by the integrators as it is so confusing and theres wasn't any specific guidance on this yet
          Hide
          Rossiani Wijaya added a comment -

          Looks good Apu!
          +1 for pull request.

          Show
          Rossiani Wijaya added a comment - Looks good Apu! +1 for pull request.
          Hide
          Aparup Banerjee added a comment -

          thanks Rosie: PULL-582 and PULL-583 created!

          Show
          Aparup Banerjee added a comment - thanks Rosie: PULL-582 and PULL-583 created!
          Hide
          Aparup Banerjee added a comment -

          PULL-601 and PULL-602 created for coming integration.

          Show
          Aparup Banerjee added a comment - PULL-601 and PULL-602 created for coming integration.
          Hide
          Petr Škoda added a comment -

          Lowering priority and reopening, the change of existing fields may affect auth/ldap or any other link to external system. Instead we could patch the user uploaded script which would not have any other side effects imo.

          Thanks anyway!

          Show
          Petr Škoda added a comment - Lowering priority and reopening, the change of existing fields may affect auth/ldap or any other link to external system. Instead we could patch the user uploaded script which would not have any other side effects imo. Thanks anyway!
          Show
          Aparup Banerjee added a comment - ok, so we're going full circle back to creating the patch that was created in http://tracker.moodle.org/browse/MDL-17344?focusedCommentId=108006&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-108006
          Hide
          Martin Dougiamas added a comment -

          Petr, please stop rejecting issues at integration for reasons separate to the actual code, it's too late by then.

          We already decided to fix the shortnames to make them consistent, surely it's very sensible to not allow "Middlename" and "middlename" to exist as separate fields.

          If LDAP has a problem with being case-sensitive then I think that's a separate LDAP bug, and there should be a new issue created (linked to this one) that makes sure that auth/ldap lowercases everything properly too, before talking to the the Moodle db.

          Show
          Martin Dougiamas added a comment - Petr, please stop rejecting issues at integration for reasons separate to the actual code, it's too late by then. We already decided to fix the shortnames to make them consistent, surely it's very sensible to not allow "Middlename" and "middlename" to exist as separate fields. If LDAP has a problem with being case-sensitive then I think that's a separate LDAP bug, and there should be a new issue created (linked to this one) that makes sure that auth/ldap lowercases everything properly too, before talking to the the Moodle db.
          Hide
          Petr Škoda added a comment -

          Changing shortname format in the middle of stable branch is not ok, sorry it took me so long to realise this. We can not break stuff in one commit in STABLE and wait for the fix for a few weeks, that could work in DEV but I believe it we should do this any more in stable especially for non-critical bugs.

          Show
          Petr Škoda added a comment - Changing shortname format in the middle of stable branch is not ok, sorry it took me so long to realise this. We can not break stuff in one commit in STABLE and wait for the fix for a few weeks, that could work in DEV but I believe it we should do this any more in stable especially for non-critical bugs.
          Hide
          Petr Škoda added a comment -

          Hmm, I have studied my code in upload user script and indeed the bug is there, I even found some more problems there. I will post a link here once I finish the patch.

          Show
          Petr Škoda added a comment - Hmm, I have studied my code in upload user script and indeed the bug is there, I even found some more problems there. I will post a link here once I finish the patch.
          Hide
          Petr Škoda added a comment - - edited

          here is a proposed fix: PULL-667

          Show
          Petr Škoda added a comment - - edited here is a proposed fix: PULL-667
          Hide
          Petr Škoda added a comment -

          The lowercase could be required in 2.1dev, but I think we should imho do the same for all short names - roles, groups, groupings, profile fields, etc.

          Show
          Petr Škoda added a comment - The lowercase could be required in 2.1dev, but I think we should imho do the same for all short names - roles, groups, groupings, profile fields, etc.
          Hide
          Petr Škoda added a comment -

          Sorry for taking over this issue, this bug was created by me and the previous PULL requests seemed to be creating new potential problems.

          Please file new issue for forced lowercasing of all short names in Moodle if necessary.

          Thanks for the report and all patch proposals!

          Show
          Petr Škoda added a comment - Sorry for taking over this issue, this bug was created by me and the previous PULL requests seemed to be creating new potential problems. Please file new issue for forced lowercasing of all short names in Moodle if necessary. Thanks for the report and all patch proposals!
          Hide
          Petr Škoda added a comment -

          To Martin: I believe it is correct to reject issue if I find potential problem in STABLE branch (especially when the change can not be undone automatically in later upgrade step which was exactly this case), otherwise these reviews have no sense for me and I would prefer to work on something different instead.

          Show
          Petr Škoda added a comment - To Martin: I believe it is correct to reject issue if I find potential problem in STABLE branch (especially when the change can not be undone automatically in later upgrade step which was exactly this case), otherwise these reviews have no sense for me and I would prefer to work on something different instead.

            People

            • Votes:
              2 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: