Moodle
  1. Moodle
  2. MDL-38136

2.4 - user_get_user_details doesn't return idnumber

    Details

    • Testing Instructions:
      Hide

      Search for showuseridentity in the admin and enable all fields
      Create a student (in the script it's id = 3, you need to change for the correct id) - fill up all the showuseridentity fields.
      Create a manager
      Connect as manager

      Script to run:

        1 <?php
        2 require_once("./config.php");
        3 require_once("./user/lib.php");
        4 $user = $DB->get_record('user', array('id' => 3));
        5 $userdetails = user_get_user_details($user);
        7 print_r($userdetails);
      

      Run this script without the patch, you should not see idnumber, institution, department. Run the script with the patch, you should see them. Check that showuseridentityfields configuration is respected (following if you disable or enable some of them).

      Show
      Search for showuseridentity in the admin and enable all fields Create a student (in the script it's id = 3, you need to change for the correct id) - fill up all the showuseridentity fields. Create a manager Connect as manager Script to run: 1 <?php 2 require_once( "./config.php" ); 3 require_once( "./user/lib.php" ); 4 $user = $DB->get_record('user', array('id' => 3)); 5 $userdetails = user_get_user_details($user); 7 print_r($userdetails); Run this script without the patch, you should not see idnumber, institution, department. Run the script with the patch, you should see them. Check that showuseridentityfields configuration is respected (following if you disable or enable some of them).
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
    • Rank:
      47959

      Description

      It seems that its not working anymore on 2.4.
      At user/lib.php , line 240 we have

      $showuseridentityfields = get_extra_user_fields($context);

      That is an array of fields, but at line 240 the condition tests if the field is a key

      if ($isadmin or $currentuser or isset($showuseridentityfields['idnumber']))

      when it should test if the value exist in array

      if ($isadmin or $currentuser or in_array('idnumber', $showuseridentityfields))

      Same thing happens for institution, department, phone1 and phone2

        Issue Links

          Activity

          Hide
          Jérôme Mouneyrac added a comment - - edited

          Hi Daniel,
          (if possible) can you provide a git patch? Thanks.
          Jerome

          Show
          Jérôme Mouneyrac added a comment - - edited Hi Daniel, (if possible) can you provide a git patch? Thanks. Jerome
          Hide
          Daniel Neis added a comment -

          Hi,

          here is a patch to fix this in MOODLE_24_STABLE : https://github.com/danielneis/moodle/tree/MDL-38136

          Show
          Daniel Neis added a comment - Hi, here is a patch to fix this in MOODLE_24_STABLE : https://github.com/danielneis/moodle/tree/MDL-38136
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Daniel.

          Show
          Jérôme Mouneyrac added a comment - Thanks Daniel.
          Hide
          Jérôme Mouneyrac added a comment -

          The code seems good to me, I confirm the bug and I tested it does fix it.

          Show
          Jérôme Mouneyrac added a comment - The code seems good to me, I confirm the bug and I tested it does fix it.
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks sending to integration (it was actually a peer-review, no need additional one)

          Show
          Jérôme Mouneyrac added a comment - Thanks sending to integration (it was actually a peer-review, no need additional one)
          Hide
          Jérôme Mouneyrac added a comment - - edited

          It needs to be ported in 2.5, 2.3, 2.2 too. Cherry picked on latest STABLE_24, no conflicts.

          Show
          Jérôme Mouneyrac added a comment - - edited It needs to be ported in 2.5, 2.3, 2.2 too. Cherry picked on latest STABLE_24, no conflicts.
          Hide
          Jérôme Mouneyrac added a comment -

          I tested on 2.2, you can cherry pick without issues and it also still fix the issue. All good.

          Show
          Jérôme Mouneyrac added a comment - I tested on 2.2, you can cherry pick without issues and it also still fix the issue. All good.
          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
          Dan Poltawski added a comment -

          Hi,

          Please can you add some testing instructions testing that the showuseridentityfields configuration is respected, not just that it is shown?

          Show
          Dan Poltawski added a comment - Hi, Please can you add some testing instructions testing that the showuseridentityfields configuration is respected, not just that it is shown?
          Hide
          Dan Poltawski added a comment -

          Reopening, its missed this round and needs those testing instructions.

          Show
          Dan Poltawski added a comment - Reopening, its missed this round and needs those testing instructions.
          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
          Jérôme Mouneyrac added a comment -

          Resubmitting with a slight change in the test instructions.

          Show
          Jérôme Mouneyrac added a comment - Resubmitting with a slight change in the test instructions.
          Hide
          Jérôme Mouneyrac added a comment -

          I think my test instructions are good enough. I did spend some time testing I'm pretty confident. In case the code causes any bugs I take the entire blame and will send $50 to a caritative association.

          Show
          Jérôme Mouneyrac added a comment - I think my test instructions are good enough. I did spend some time testing I'm pretty confident. In case the code causes any bugs I take the entire blame and will send $50 to a caritative association.
          Hide
          Damyon Wiese added a comment -

          Thanks Daniel, Antonio and Jerome.

          This has been integrated to 23, 24 and master.

          Show
          Damyon Wiese added a comment - Thanks Daniel, Antonio and Jerome. This has been integrated to 23, 24 and master.
          Hide
          David Monllaó added a comment -

          The phone1 and phone2 attributes are always shown, the other fields works as expected.

          IMO it should fail, waiting for reply just in case (caritative associations are waiting)

          Show
          David Monllaó added a comment - The phone1 and phone2 attributes are always shown, the other fields works as expected. IMO it should fail, waiting for reply just in case (caritative associations are waiting)
          Hide
          Dan Poltawski added a comment -

          "Jerome: I tested it already"

          !

          Show
          Dan Poltawski added a comment - "Jerome: I tested it already" !
          Hide
          Jérôme Mouneyrac added a comment -

          So I owe 50 dollars to an association, pick one

          Show
          Jérôme Mouneyrac added a comment - So I owe 50 dollars to an association, pick one
          Hide
          Aparup Banerjee added a comment -
          Show
          Aparup Banerjee added a comment - http://moodle.org/donations
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Sorry guys, the test instruction are confusing. In the last test instruction sentence:

          Check that showuseridentityfields configuration is respected (following if you disable or enable some of them)

          I missed to indicate what the student should be able to see and what the manager should be able to see. I miss it because I know it but I should have at least reference showuseridentity (admin/search.php?query=showuser):

          When selecting or searching for users, and when displaying lists of users, these fields may be shown in addition to their full name. The fields are only shown to users who have the moodle/site:viewuseridentity capability; by default, teachers and managers. (This option makes most sense if you choose one or two fields that are mandatory at your institution.)

          It still works.

          Show
          Jérôme Mouneyrac added a comment - - edited Sorry guys, the test instruction are confusing. In the last test instruction sentence: Check that showuseridentityfields configuration is respected (following if you disable or enable some of them) I missed to indicate what the student should be able to see and what the manager should be able to see. I miss it because I know it but I should have at least reference showuseridentity (admin/search.php?query=showuser): When selecting or searching for users, and when displaying lists of users, these fields may be shown in addition to their full name. The fields are only shown to users who have the moodle/site:viewuseridentity capability; by default, teachers and managers. (This option makes most sense if you choose one or two fields that are mandatory at your institution.) It still works.
          Hide
          David Monllaó added a comment -

          Passing it according to Jerome comment

          Show
          David Monllaó added a comment - Passing it according to Jerome comment
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks David. Daniel's fix is still good, fixing what the issue is about and the fix does not introduce any regressions.

          However good catch about phone1/phone2, I'll open a different issue after more investigation about phone1/phone2/address behavior (apparently from reading a bit more code, these fields have different behavior from the others).

          Show
          Jérôme Mouneyrac added a comment - Thanks David. Daniel's fix is still good, fixing what the issue is about and the fix does not introduce any regressions. However good catch about phone1/phone2, I'll open a different issue after more investigation about phone1/phone2/address behavior (apparently from reading a bit more code, these fields have different behavior from the others).
          Hide
          Dan Poltawski added a comment -

          Blooming Marvelous! It's time for a knees up - your changes are upstream!

          Thanks for making Moodle better!

          Toodle pip

          Show
          Dan Poltawski added a comment - Blooming Marvelous! It's time for a knees up - your changes are upstream! Thanks for making Moodle better! Toodle pip
          Hide
          Daniel Neis added a comment -

          Thanks for everyone! I am very happy to have this patch accepted.

          Kind regards,
          Daniel

          Show
          Daniel Neis added a comment - Thanks for everyone! I am very happy to have this patch accepted. Kind regards, Daniel

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: