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
    • Rank:
      16507

      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.

        Issue Links

          Activity

          Hide
          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
          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 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 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
          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
          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 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 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
          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
          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
          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
          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
          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
          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 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 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
          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
          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 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 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 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 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
          Rossiani Wijaya added a comment -

          This looks good Ankit.

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

          Thanks for the review Rosie.
          Sending for integration.
          Thanks

          Show
          Ankit Agarwal added a comment - Thanks for the review Rosie. Sending for integration. Thanks
          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
          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
          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
          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
          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
          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
          Mary Cooch added a comment -

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

          Show
          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: