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

      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

        Gliffy Diagrams

          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 Skoda 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 Skoda 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 Skoda 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 Skoda 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 Skoda 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 Skoda 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 Skoda added a comment - - edited

            here is a proposed fix: PULL-667

            Show
            Petr Skoda added a comment - - edited here is a proposed fix: PULL-667
            Hide
            Petr Skoda 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 Skoda 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 Skoda 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 Skoda 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 Skoda 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 Skoda 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: