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

          Attachments

            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:
                  8 Start watching this issue

                  Dates

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