Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-26882

user profile fields not shown in profile

    Details

    • Testing Instructions:
      Hide
      1. Login as a student (A), go and edit your profile and Add idnumber,address,phone,mobile phone,department and institution
      2. Login as teacher (Teacher at system level and doesnt have site:viewuseridentity capability) (B) and visit student A's profile (profile.php not course profile) and make sure you cannot see any of these fields.
      3. Now login as admin and give B the capability site:viewuseridentity
      4. Search for setting showuseridentity and check a few fields
      5. log back as user B and go to A's profile and make sure you can see the fields that you checked in step above.
      Show
      Login as a student (A), go and edit your profile and Add idnumber,address,phone,mobile phone,department and institution Login as teacher (Teacher at system level and doesnt have site:viewuseridentity capability) (B) and visit student A's profile (profile.php not course profile) and make sure you cannot see any of these fields. Now login as admin and give B the capability site:viewuseridentity Search for setting showuseridentity and check a few fields log back as user B and go to A's profile and make sure you can see the fields that you checked in step above.
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-26882-master

      Description

      If there is an entry in the user profile field, the entries is not shown in profile, not for the user, not for admins.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            danmarsden Dan Marsden added a comment -

            Thanks Ralf - the fields, Dept/Institution and Idnumber are all missing.

            IMO - all 3 should be listed as fields in hiddenuserfields (hidden by default)

            Idnumber is used by ldap/other systems and will usually need to be hidden by default but this should be at the admins discretion.

            Show
            danmarsden Dan Marsden added a comment - Thanks Ralf - the fields, Dept/Institution and Idnumber are all missing. IMO - all 3 should be listed as fields in hiddenuserfields (hidden by default) Idnumber is used by ldap/other systems and will usually need to be hidden by default but this should be at the admins discretion.
            Hide
            mayank_gupta2005 Mayank Gupta added a comment -

            Hello, I created a patch [1] to add the the missing fields in the profile page and the settings page.
            These fields (Idnumber, Department, Institution) are set to hidden by default.

            [1]- https://github.com/mayankgupta/moodle/commit/3e236e6ff6bfb84e9b9ea0e8f72426f3a4adc155

            I will be happy to modify it, if required.

            Thanks,
            Mayank.

            Show
            mayank_gupta2005 Mayank Gupta added a comment - Hello, I created a patch [1] to add the the missing fields in the profile page and the settings page. These fields (Idnumber, Department, Institution) are set to hidden by default. [1] - https://github.com/mayankgupta/moodle/commit/3e236e6ff6bfb84e9b9ea0e8f72426f3a4adc155 I will be happy to modify it, if required. Thanks, Mayank.
            Hide
            quen Sam Marshall added a comment -

            Hi, this patch looks good to me (I'm not an HQ dev) EXCEPT... if you just change the default for the field like that, I don't think it will apply to an upgrade that already has the field. So you need to add those 3 values to the existing field in a database update, otherwise fields will suddenly appear for everyone. Unless I am missing something.

            Show
            quen Sam Marshall added a comment - Hi, this patch looks good to me (I'm not an HQ dev) EXCEPT... if you just change the default for the field like that, I don't think it will apply to an upgrade that already has the field. So you need to add those 3 values to the existing field in a database update, otherwise fields will suddenly appear for everyone. Unless I am missing something.
            Hide
            mayank_gupta2005 Mayank Gupta added a comment -

            Hi Sam, thanks for reviewing it. This patch will work(the fields will be hidden by default) for a fresh install.
            While writing the patch I considered adding it to /lib/db/upgrade.php, but I was unsure in which version will this patch be pulled so I did not know with which version should I put a conditional check on (in upgrade.php).
            Should I go and add the conditional check depending on the version this patch was written on?

            Show
            mayank_gupta2005 Mayank Gupta added a comment - Hi Sam, thanks for reviewing it. This patch will work(the fields will be hidden by default) for a fresh install. While writing the patch I considered adding it to /lib/db/upgrade.php, but I was unsure in which version will this patch be pulled so I did not know with which version should I put a conditional check on (in upgrade.php). Should I go and add the conditional check depending on the version this patch was written on?
            Hide
            aborrow Anthony Borrow added a comment -

            Should we also include: phone, mobilephone, and address?

            For consistency, I would like to see that we follow the same order of the fields as they are listed in user/editadvanced.php, namely:
            city
            country
            description
            webpage
            icqnumber
            skypeid
            aimid
            yahooid
            msnid
            idnumber*
            institution*
            department*
            phone*
            mobilephone*
            address*

            where *indicates not currently in hiddenuserfields list

            Peace - Anthony

            Show
            aborrow Anthony Borrow added a comment - Should we also include: phone, mobilephone, and address? For consistency, I would like to see that we follow the same order of the fields as they are listed in user/editadvanced.php, namely: city country description webpage icqnumber skypeid aimid yahooid msnid idnumber* institution* department* phone* mobilephone* address* where *indicates not currently in hiddenuserfields list Peace - Anthony
            Hide
            ralfh Ralf Hilgenstock added a comment -

            I think it is best if admin can decide which fields are visible for other users, only for teacher or invisible.

            Show
            ralfh Ralf Hilgenstock added a comment - I think it is best if admin can decide which fields are visible for other users, only for teacher or invisible.
            Hide
            aborrow Anthony Borrow added a comment -

            Mayank - Just to clarify, I believe the behavior we are looking for is that default, for those upgrading, no new information will be revealed so the default values for the new fields is to include them in the list of hiddenuserfields; however, for new sites, the idea is that all of the fields would be visible and the admin can select during the initial install whatever needs to be hidden. As for the version number, just use the current version number and that can be changed at the last moment by the person integrating it - which is to say that it is pretty trivial. Thanks for your help in contributing a patch. Peace - Anthony

            Show
            aborrow Anthony Borrow added a comment - Mayank - Just to clarify, I believe the behavior we are looking for is that default, for those upgrading, no new information will be revealed so the default values for the new fields is to include them in the list of hiddenuserfields; however, for new sites, the idea is that all of the fields would be visible and the admin can select during the initial install whatever needs to be hidden. As for the version number, just use the current version number and that can be changed at the last moment by the person integrating it - which is to say that it is pretty trivial. Thanks for your help in contributing a patch. Peace - Anthony
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Hi Sam,
            Do you think the best way to implement this is using the new cap user:showuseridentity introduced in MDL-26647 ? Was there any reason that capability was not ported to user profiles?

            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Hi Sam, Do you think the best way to implement this is using the new cap user:showuseridentity introduced in MDL-26647 ? Was there any reason that capability was not ported to user profiles? Thanks
            Hide
            quen Sam Marshall added a comment -

            Hi Ankit,

            When I implemented that capability, it was alongside the new feature to display the identity options in lists/tables and I didn't think of user profile. You are right, there is no reason it shouldn't show those same identity options in the user profile page.

            (But, people might actually want to display additional options in the user profile page as well, because typically you'd only show like 'idnumber' or maybe 'idnumber+department' or something in the useridentity fields list, since they appear in tables where there isn't that much room.)

            I can see that showuseridentity could potentially also give access to extra fields from a different list on the user profile page, not sure. Obviously somebody needs to think about it to ensure it isn't a privacy violation (sounds ok to me).

            Show
            quen Sam Marshall added a comment - Hi Ankit, When I implemented that capability, it was alongside the new feature to display the identity options in lists/tables and I didn't think of user profile. You are right, there is no reason it shouldn't show those same identity options in the user profile page. (But, people might actually want to display additional options in the user profile page as well, because typically you'd only show like 'idnumber' or maybe 'idnumber+department' or something in the useridentity fields list, since they appear in tables where there isn't that much room.) I can see that showuseridentity could potentially also give access to extra fields from a different list on the user profile page, not sure. Obviously somebody needs to think about it to ensure it isn't a privacy violation (sounds ok to me).
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Thanks Sam, for the feedback. I guess introducing a new list might make user profile more complicated. We already have like 4-5 different caps controlling different parts of a user profile. IMHO it would be ideal to implement the showuseridentiy cap with existing list to keep things simple for admins.

            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Thanks Sam, for the feedback. I guess introducing a new list might make user profile more complicated. We already have like 4-5 different caps controlling different parts of a user profile. IMHO it would be ideal to implement the showuseridentiy cap with existing list to keep things simple for admins. Thanks
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            I am not sure if the cap check should be applied to email ids or not. Hence made a separate commit for the same.

            Show
            ankit_frenz Ankit Agarwal added a comment - I am not sure if the cap check should be applied to email ids or not. Hence made a separate commit for the same.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            This looks good Ankit.

            Show
            rwijaya Rossiani Wijaya added a comment - This looks good Ankit.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Thanks for the review Rosie.
            Sending for integration.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Thanks for the review Rosie. Sending for integration. Thanks
            Hide
            stronk7 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
            stronk7 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
            samhemelryk Sam Hemelryk added a comment -

            Thanks Ankit, this has been integrated now.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Ankit, this has been integrated now.
            Hide
            abgreeve Adrian Greeve added a comment -

            Tested on 2.2, 2.3 and master.
            User profile fields are now available for viewing if configured that way.
            Test passed.

            Show
            abgreeve Adrian Greeve added a comment - Tested on 2.2, 2.3 and master. User profile fields are now available for viewing if configured that way. Test passed.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Many thanks for the hard work.

            These changes have been spread upstream and are already available in the git and cvs repositories.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Many thanks for the hard work. These changes have been spread upstream and are already available in the git and cvs repositories. Ciao
            Hide
            marycooch Mary Cooch added a comment -

            (Housekeeping) Removing docs_required as I believe this was covered in http://docs.moodle.org/en/Roles_settings

            Show
            marycooch Mary Cooch added a comment - (Housekeeping) Removing docs_required as I believe this was covered in http://docs.moodle.org/en/Roles_settings

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  10/Sep/12