Uploaded image for project: '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:

      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

        Gliffy Diagrams

          Issue Links

            Activity

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

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

            Show
            jerome Jérôme Mouneyrac added a comment - - edited Hi Daniel, (if possible) can you provide a git patch? Thanks. Jerome
            Hide
            danielneis 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
            danielneis 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
            jerome Jérôme Mouneyrac added a comment -

            Thanks Daniel.

            Show
            jerome Jérôme Mouneyrac added a comment - Thanks Daniel.
            Hide
            jerome 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
            jerome 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
            jerome Jérôme Mouneyrac added a comment -

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

            Show
            jerome Jérôme Mouneyrac added a comment - Thanks sending to integration (it was actually a peer-review, no need additional one)
            Hide
            jerome 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
            jerome 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
            jerome 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
            jerome 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
            poltawski 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
            poltawski 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
            poltawski 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
            poltawski 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
            poltawski Dan Poltawski added a comment -

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

            Show
            poltawski Dan Poltawski added a comment - Reopening, its missed this round and needs those testing instructions.
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Resubmitting with a slight change in the test instructions.

            Show
            jerome Jérôme Mouneyrac added a comment - Resubmitting with a slight change in the test instructions.
            Hide
            jerome 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
            jerome 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 Damyon Wiese added a comment -

            Thanks Daniel, Antonio and Jerome.

            This has been integrated to 23, 24 and master.

            Show
            damyon Damyon Wiese added a comment - Thanks Daniel, Antonio and Jerome. This has been integrated to 23, 24 and master.
            Hide
            dmonllao 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
            dmonllao 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
            poltawski Dan Poltawski added a comment -

            "Jerome: I tested it already"

            !

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

            So I owe 50 dollars to an association, pick one

            Show
            jerome Jérôme Mouneyrac added a comment - So I owe 50 dollars to an association, pick one
            Hide
            nebgor Aparup Banerjee added a comment -
            Show
            nebgor Aparup Banerjee added a comment - http://moodle.org/donations
            Hide
            jerome 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
            jerome 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
            dmonllao David Monllaó added a comment -

            Passing it according to Jerome comment

            Show
            dmonllao David Monllaó added a comment - Passing it according to Jerome comment
            Hide
            jerome 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
            jerome 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
            poltawski 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
            poltawski 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
            danielneis Daniel Neis added a comment -

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

            Kind regards,
            Daniel

            Show
            danielneis 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:
                  Fix Release Date:
                  13/May/13