Moodle
  1. Moodle
  2. MDL-37339

Unique user profile fields do not allow duplicate empty values

    Details

    • Testing Instructions:
      Hide

      Pre-requisite:

      1. Custom profile field (Text input) with "Should the data be unique" set to YES and "Is this field required" set to NO
      2. Custom profile field (Text input) with "Should the data be unique" set to YES and "Is this field required" set to YES

      Steps to create custom profile field.

      1. Log in as admin
      2. Click on user profile field (Site administration ► Users ► Accounts ► User profile fields)
      3. Select "Text input"

      Test 1

      1. Log in as student, click edit profile (Home ► My profile settings ► Edit profile)
      2. Click "Update profile" and you should see "Required" error on Unique-required field.
      3. Input "TEST" in Unique-required field and leave Unique custom field blank.
      4. Click update profile and it should update student profile.
      5. Log in as another student and edit profile.
      6. Input "TEST" in Unique-required field and leave Unique custom field blank.
      7. You should see error (This value has already been used) for Unique-required field.
      8. Change "Unique-required" field value to "TEST1"
      9. Update profile and you should not see any problem.

      Test 2:

      1. Log in as student 1 and go to edit profile.
      2. Input "TEST" in Unique profile field and click update profile
      3. Log in as another student and go to edit profile
      4. Input "TEST" in Unique profile field and click update profile
      5. You should see error.
      6. Change it to "TEST1" and click update profile
      7. Profile should be updated without error.
      Show
      Pre-requisite: Custom profile field (Text input) with "Should the data be unique" set to YES and "Is this field required" set to NO Custom profile field (Text input) with "Should the data be unique" set to YES and "Is this field required" set to YES Steps to create custom profile field. Log in as admin Click on user profile field (Site administration ► Users ► Accounts ► User profile fields) Select "Text input" Test 1 Log in as student, click edit profile (Home ► My profile settings ► Edit profile) Click "Update profile" and you should see "Required" error on Unique-required field. Input "TEST" in Unique-required field and leave Unique custom field blank. Click update profile and it should update student profile. Log in as another student and edit profile. Input "TEST" in Unique-required field and leave Unique custom field blank. You should see error (This value has already been used) for Unique-required field. Change "Unique-required" field value to "TEST1" Update profile and you should not see any problem. Test 2: Log in as student 1 and go to edit profile. Input "TEST" in Unique profile field and click update profile Log in as another student and go to edit profile Input "TEST" in Unique profile field and click update profile You should see error. Change it to "TEST1" and click update profile Profile should be updated without error.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
      wip-mdl-37339-m24
    • Pull Master Branch:
      wip-mdl-37339
    • Rank:
      46956

      Description

      The user profile fields allow 2 settings: Required and Unique.

      It appears from edit_validate_field() function in /user/profile/lib.php that required-ness is only checked by the form's UI and unique-ness by the edit_validate_field() function.

      It appears that empty values are subjected to a unique-ness check. We store user matriculation numbers and library numbers in profile fields, which are unique but not required (as only students have matric. numbers and only library users have library numbers).

      In this case with Required == 0 and Unique == 1, adding a new staff (matric =="" and librarycard = "") results in the value already used message. We've fudge it by making the field's non-unique...but they should be if they have a value!

      I would suggest that either there is an option that allows or prevents empty values thus excluding "" from the unique-ness check, but controllable on a per-field basis (as there may be valid cases when it should never be emtpy)
      -or -
      to ensure that the "required" setting is used for this purpose in the edit_validate_field() function.

        Activity

        Hide
        Rajesh Taneja added a comment -

        Thanks for reporting this and providing patch.

        I've put that on the backlog and will try to get to this soon.

        Show
        Rajesh Taneja added a comment - Thanks for reporting this and providing patch. I've put that on the backlog and will try to get to this soon.
        Hide
        Rajesh Taneja added a comment -

        Sorry Michael, I have not picked your commit because:

        1. $this->is_empty() is not checking for input value. It is checking for existing field value.
        2. Check should be !empty || required. As in case of required value is not supposed to be empty.

        FYI:
        As per documentation: "unique - the data entered must be unique;", so if field in not required data is not required to be entered and hence duplicate empty should be allowed.

        Show
        Rajesh Taneja added a comment - Sorry Michael, I have not picked your commit because: $this->is_empty() is not checking for input value. It is checking for existing field value. Check should be !empty || required. As in case of required value is not supposed to be empty. FYI: As per documentation: " unique - the data entered must be unique;", so if field in not required data is not required to be entered and hence duplicate empty should be allowed.
        Hide
        Adrian Greeve added a comment -

        [Y] Syntax
        [Y] Output
        [Y] Whitespace
        [-] Language
        [-] Databases
        [Y] Testing
        [-] Security
        [-] Documentation
        [Y] Git
        [Y] Sanity check

        Hi Rajesh,

        The code looks good. I'm a big fan of the reformatting of the $value if statement. This is much easier to read.
        The patch works, now behaving as it should.
        My +1 for integration.

        Show
        Adrian Greeve added a comment - [Y] Syntax [Y] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check Hi Rajesh, The code looks good. I'm a big fan of the reformatting of the $value if statement. This is much easier to read. The patch works, now behaving as it should. My +1 for integration.
        Hide
        Rajesh Taneja added a comment -

        Thanks Adrian.

        Show
        Rajesh Taneja added a comment - Thanks Adrian.
        Hide
        Michael Hughes added a comment -

        No problem, it was a pretty quick and dirty patch. Glad to see it's being looked at so quickly.

        Show
        Michael Hughes added a comment - No problem, it was a pretty quick and dirty patch. Glad to see it's being looked at so quickly.
        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
        Eloy Lafuente (stronk7) added a comment -

        Hi:

        • Trivial, it seems that $value does not need to be init, because if/else always set it. Feel free to keep/delete it.
        • Surely not important at all: Seems that $usernew->{$this->inputname} is never verified to be set, do we know 100% that it exists ever? I was thinking about checkboxes and others that aren't sent in the request... note I've not looked to the profile fields in context and perhaps that's handled before the code is called. Just surprised me.
        • The real point: The logic seems perfect, but I don't think the $isempty calculation is ok. I only can imagine the empty string as a candidate, isn't it? If so, why not simply do $value === '' and done? Else there will be "other values" already evaluating as empty that perhaps we don't want to consider empty (say 0, say false, say null...). Again, note I don't know how booleans or integers arrive to that function, but IMO it sounds safer to look exactly (===) for the values we consider empty.

        So I'm reopening this to reconsider the points above, specially the 3rd one.

        Thanks and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Hi: Trivial, it seems that $value does not need to be init, because if/else always set it. Feel free to keep/delete it. Surely not important at all: Seems that $usernew->{$this->inputname} is never verified to be set, do we know 100% that it exists ever? I was thinking about checkboxes and others that aren't sent in the request... note I've not looked to the profile fields in context and perhaps that's handled before the code is called. Just surprised me. The real point: The logic seems perfect, but I don't think the $isempty calculation is ok. I only can imagine the empty string as a candidate, isn't it? If so, why not simply do $value === '' and done? Else there will be "other values" already evaluating as empty that perhaps we don't want to consider empty (say 0, say false, say null...). Again, note I don't know how booleans or integers arrive to that function, but IMO it sounds safer to look exactly (===) for the values we consider empty. So I'm reopening this to reconsider the points above, specially the 3rd one. Thanks and ciao
        Hide
        CiBoT added a comment -

        Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

        Show
        CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
        Hide
        Rajesh Taneja added a comment -

        Thanks for pointing this Eloy, I have done all the changes as suggested by you. Pushing it back for your review.

        FYI:

        1. Personally I prefer to init any variable. Anyway's it was not important so removed it.
        2. $usernew->($this->unputname} is always valid, but still added check to be on safer side.
        3. I agree in case of checkbox it might be an issue, so changed it.
        Show
        Rajesh Taneja added a comment - Thanks for pointing this Eloy, I have done all the changes as suggested by you. Pushing it back for your review. FYI: Personally I prefer to init any variable. Anyway's it was not important so removed it. $usernew->($this->unputname} is always valid, but still added check to be on safer side. I agree in case of checkbox it might be an issue, so changed it.
        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
        Aparup Banerjee added a comment - - edited

        thanks for the changes, this looks good and has been integrated into 23, 24 and master.

        ps: after integrating (sigh), i wondered about unit tests for user/profile/lib.php and even user/* but saw nothing except the external lib tests..

        Show
        Aparup Banerjee added a comment - - edited thanks for the changes, this looks good and has been integrated into 23, 24 and master. ps: after integrating (sigh), i wondered about unit tests for user/profile/lib.php and even user/* but saw nothing except the external lib tests..
        Hide
        David Monllaó added a comment -

        Tested in 23 and master. It passes

        Show
        David Monllaó added a comment - Tested in 23 and master. It passes
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks!

        Closing as fixed, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks! Closing as fixed, ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: