Moodle
  1. Moodle
  2. MDL-28998

Custom user profile field entry allows non-unique value when it should not

    Details

    • Database:
      MySQL
    • Testing Instructions:
      Hide

      1. Create a custom user profile field type and implement field validation with the edit_validate_field() function.
      2. Add a custom user profile field of custom field type created in step 1, set the field to display on user signup.
      3. Register a new user via email-based self-registration and enter an invalid value for the custom field.
      4. We expect the field to be validated and a warning message to show, but the field is not validated and the registration succeeds.

      Show
      1. Create a custom user profile field type and implement field validation with the edit_validate_field() function. 2. Add a custom user profile field of custom field type created in step 1, set the field to display on user signup. 3. Register a new user via email-based self-registration and enter an invalid value for the custom field. 4. We expect the field to be validated and a warning message to show, but the field is not validated and the registration succeeds.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-28998-master

      Description

      A new custom user profile field type (in the /user/profile/field directory) has been created. The edit_field_add() function has been used to display the field and the edit_validate_field() function has been used to validate the field input upon form submission. The problem is that the field is only validated when updating the field value on the user's profile, but not on user signup (email-based self-registration).

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Michael de Raadt added a comment -

            Thanks for raising this.

            I tested this and I was able to enter a value that was not unique when a unique value was expected (which was bad), but that was the only sort of validation that I could think to test. What kind of validation are you expecting?

            Show
            Michael de Raadt added a comment - Thanks for raising this. I tested this and I was able to enter a value that was not unique when a unique value was expected (which was bad), but that was the only sort of validation that I could think to test. What kind of validation are you expecting?
            Hide
            John Grobler added a comment -

            In our case we also validate for uniqueness, which we cannot get to work (non-unique entries are accepted and stored).

            Show
            John Grobler added a comment - In our case we also validate for uniqueness, which we cannot get to work (non-unique entries are accepted and stored).
            Hide
            Michael de Raadt added a comment -

            OK. I've updated the summary of this issue and bumped the priority.

            Obviously the code to validate uniqueness of the field exists, but it is not being applied correctly.

            In the meantime feel free to help us work on this issue.

            Show
            Michael de Raadt added a comment - OK. I've updated the summary of this issue and bumped the priority. Obviously the code to validate uniqueness of the field exists, but it is not being applied correctly. In the meantime feel free to help us work on this issue.
            Hide
            Ankit Agarwal added a comment -

            Hi,
            The signup form was missing the call to function that is responsible for validating customisable profile fields. Besides that the signup validation function handels submitted data as an array where is its handeled as an object at other places (profile edit), so I had to convert and use the submitted data. It might be a good idea to make this consistent in another improvment MDL.
            submitting for review.
            Thanks

            Show
            Ankit Agarwal added a comment - Hi, The signup form was missing the call to function that is responsible for validating customisable profile fields. Besides that the signup validation function handels submitted data as an array where is its handeled as an object at other places (profile edit), so I had to convert and use the submitted data. It might be a good idea to make this consistent in another improvment MDL. submitting for review. Thanks
            Hide
            Dan Poltawski added a comment -

            Sorry for the delay in peer review..

            Looks good to me. I would have just cast the object in the function but that is not important - please submit for integration!

            Show
            Dan Poltawski added a comment - Sorry for the delay in peer review.. Looks good to me. I would have just cast the object in the function but that is not important - please submit for integration!
            Hide
            Ankit Agarwal added a comment -

            Thanks Dan for the review.
            Submitting for integration
            Thanks

            Show
            Ankit Agarwal added a comment - Thanks Dan for the review. Submitting for integration Thanks
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Some hours ago...

            the main moodle.git repository has 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 - Some hours ago... the main moodle.git repository has 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
            Sam Hemelryk added a comment -

            Thanks Ankit - this has been integrated now

            Show
            Sam Hemelryk added a comment - Thanks Ankit - this has been integrated now
            Hide
            Petr Skoda added a comment -

            Did you test it with debug mode enabled? The user->id does not exist yet during signup validation, but the code tries to use it.

            Show
            Petr Skoda added a comment - Did you test it with debug mode enabled? The user->id does not exist yet during signup validation, but the code tries to use it.
            Hide
            Ankit Agarwal added a comment -

            Hi Petr,
            Thanks for pointing that out, not sure how it passed my testing. Anyways had a chat with Dan about this and I have set user->id to 0 since thats the default value used in profile_base_class as well.

            Submitting again.
            Thanks

            Show
            Ankit Agarwal added a comment - Hi Petr, Thanks for pointing that out, not sure how it passed my testing. Anyways had a chat with Dan about this and I have set user->id to 0 since thats the default value used in profile_base_class as well. Submitting again. Thanks
            Hide
            Sam Hemelryk added a comment -

            Thanks for spotting that Petr and thanks Ankit for getting a fix up for it.
            I was two minds about whether it should be using 0 or -1 however when looking through code I see that it won't actually make any difference. (Was looking at -1 as that is what other uses of profile_validation are using).

            Up for testing again

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Thanks for spotting that Petr and thanks Ankit for getting a fix up for it. I was two minds about whether it should be using 0 or -1 however when looking through code I see that it won't actually make any difference. (Was looking at -1 as that is what other uses of profile_validation are using). Up for testing again Cheers Sam
            Hide
            Rajesh Taneja added a comment -

            Thanks Ankit, Works great

            Few things noticed, while testing this:

            1. profile_field_base::edit_validate_field has one argument but when called from profile_validation it pass two arguments.
            2. profile_field_base::edit_validate_field for loop seems redundant, same logic can be used in sql to get unique values which current not entered by this user.

            Let me know if you want me to open issues for same.

            Show
            Rajesh Taneja added a comment - Thanks Ankit, Works great Few things noticed, while testing this: profile_field_base::edit_validate_field has one argument but when called from profile_validation it pass two arguments. profile_field_base::edit_validate_field for loop seems redundant, same logic can be used in sql to get unique values which current not entered by this user. Let me know if you want me to open issues for same.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            FCT (fixed, closing, thanks). Ciao

            "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!"
            ~ Benjamin Disraeli

            Show
            Eloy Lafuente (stronk7) added a comment - FCT (fixed, closing, thanks). Ciao "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!" ~ Benjamin Disraeli

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: