Moodle
  1. Moodle
  2. MDL-33869

user_get_user_details doesn't return idnumber

    Details

    • Testing Instructions:
      Hide

      Get my PHPunit branch wip: https://github.com/mouneyrac/moodle/tree/MDL-33529
      and run PHPunit tests:
      phpunit core_user_external_testcase user/tests/externallib_test.php
      phpunit core_enrol_external_testcase enrol/tests/enrolexternallib_test.php

      Check that neither get_course_user_profiles test, neither get_enrolled_users test return an error (it's ok if other test functions return errors, it's still a wip branch)

      For 2.2:
      call the function user_get_user_details() as admin and check that idnumber is returned.

      Show
      Get my PHPunit branch wip: https://github.com/mouneyrac/moodle/tree/MDL-33529 and run PHPunit tests: phpunit core_user_external_testcase user/tests/externallib_test.php phpunit core_enrol_external_testcase enrol/tests/enrolexternallib_test.php Check that neither get_course_user_profiles test, neither get_enrolled_users test return an error (it's ok if other test functions return errors, it's still a wip branch) For 2.2: call the function user_get_user_details() as admin and check that idnumber is returned.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull Master Branch:
    • Rank:
      41970

      Description

      The user_get_user_details function in /moodle/user/lib.php accurately lists 'idnumber' in the $defaultfields array but never returns it as a member of the $user object.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for reporting this.

          I've put that on the backlog.

          In the meantime feel free to help us work on this issue. If you are able to provide a patch, please add a patch label so we will spot it.

          Show
          Michael de Raadt added a comment - Thanks for reporting this. I've put that on the backlog. In the meantime feel free to help us work on this issue. If you are able to provide a patch, please add a patch label so we will spot it.
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Crafton. I had a look to the code following this report and I noticed that only get_users_by_id() external function return the idnumber. The following functions need to return it too:

          • get_enrolled_users
          • get_course_user_profiles
          Show
          Jérôme Mouneyrac added a comment - Thanks Crafton. I had a look to the code following this report and I noticed that only get_users_by_id() external function return the idnumber. The following functions need to return it too: get_enrolled_users get_course_user_profiles
          Hide
          Crafton Williams added a comment -

          Hi, sure i'll provide a patch.

          Show
          Crafton Williams added a comment - Hi, sure i'll provide a patch.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Cool, I was going to do it but as you nicely volunteer I let it to you Crafton. Please submit the patch on github. I am reverting this issue status to not in development

          Show
          Jérôme Mouneyrac added a comment - - edited Cool, I was going to do it but as you nicely volunteer I let it to you Crafton. Please submit the patch on github. I am reverting this issue status to not in development
          Hide
          Crafton Williams added a comment -

          Will do.

          Show
          Crafton Williams added a comment - Will do.
          Hide
          Crafton Williams added a comment -

          Here's the patch. https://github.com/crafton/moodle/commit/f9d425070643bda137281be71153b7c417fb6f14.

          Spent most of the time setting up git.

          I think though in user_get_user_details, instead of an admin & currentuser check for Department, Institution and Idnumber, I suggest we go a moodle/site:viewuseridentity capability check. I think this is a little cleaner and would allow the admin to manage this from the admin interface.

          Show
          Crafton Williams added a comment - Here's the patch. https://github.com/crafton/moodle/commit/f9d425070643bda137281be71153b7c417fb6f14 . Spent most of the time setting up git. I think though in user_get_user_details, instead of an admin & currentuser check for Department, Institution and Idnumber, I suggest we go a moodle/site:viewuseridentity capability check. I think this is a little cleaner and would allow the admin to manage this from the admin interface.
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Crafton for the 'idnumber' report, the fix and the suggestion about the missing capability. I added the missing capability checks.

          Show
          Jérôme Mouneyrac added a comment - Thanks Crafton for the 'idnumber' report, the fix and the suggestion about the missing capability. I added the missing capability checks.
          Hide
          Crafton Williams added a comment -

          no problem, glad i could assist.

          Show
          Crafton Williams added a comment - no problem, glad i could assist.
          Hide
          Rossiani Wijaya added a comment -

          Hi Jerome,

          Running the following commands produced these errors:

          phpunit core_user_external_testcase user/tests/externallib_test.php
          Cannot open file "user/tests/externallib_test.php".
          
          phpunit core_enrol_external_testcase enrol/tests/enrolexternallib_test.php
          Cannot open file "user/tests/externallib_test.php".
          
          Show
          Rossiani Wijaya added a comment - Hi Jerome, Running the following commands produced these errors: phpunit core_user_external_testcase user/tests/externallib_test.php Cannot open file "user/tests/externallib_test.php" . phpunit core_enrol_external_testcase enrol/tests/enrolexternallib_test.php Cannot open file "user/tests/externallib_test.php" .
          Hide
          Rossiani Wijaya added a comment -

          Please ignore the previous comment. It needs MDL-33529 to run this issue properly.

          The patch looks good.

          Show
          Rossiani Wijaya added a comment - Please ignore the previous comment. It needs MDL-33529 to run this issue properly. The patch looks good.
          Hide
          Nuku Maar added a comment -

          When will this be in a new weekly build release?
          Is get_enrolled_users also solved and returning idnumber?

          Show
          Nuku Maar added a comment - When will this be in a new weekly build release? Is get_enrolled_users also solved and returning idnumber?
          Hide
          Jérôme Mouneyrac added a comment -

          Sending to integration. Thanks for the reminder Nuku.

          Show
          Jérôme Mouneyrac added a comment - Sending to integration. Thanks for the reminder Nuku.
          Hide
          Aparup Banerjee 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
          Aparup Banerjee 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 -

          Thanks Crafton and Jerome, i've integrated this now.

          (It'd be great to have the phpunit branch integrated for testing this )

          Show
          Dan Poltawski added a comment - Thanks Crafton and Jerome, i've integrated this now. (It'd be great to have the phpunit branch integrated for testing this )
          Hide
          Ankit Agarwal added a comment - - edited

          Hi,
          This works as expected.
          @Dan,
          can you please pass this test?
          Thanks

          Show
          Ankit Agarwal added a comment - - edited Hi, This works as expected. @Dan, can you please pass this test? Thanks
          Hide
          Dan Poltawski added a comment -

          Done.

          Show
          Dan Poltawski added a comment - Done.
          Hide
          Dan Poltawski added a comment -

          asko, Дзякуй, ধন্যবাদ, Благодаря, Gràcies, 感谢, 謝謝, Hvala, Díky, Tak, Bedankt, Tänan, متشکریم, Salamat, Kiitokset, Merci, Grazas, Danke, Ευχαριστώ, આભાર, תודה, धन्यवाद, Köszönjük, Takk fyrir, Terima Kasih, Grazie, ありがとうございます, Рахмет, សូមអរគុណ, 감사합니다, gratiās, Pateicamies, Ačiū, Благодарам, Tēnā koa, Kia Ora Rawa Atu, आभारी आहोत, Талархал, Takk, Dziękuję, Obrigado, Mulţumesc, Engraziel, Спасибо, Fa'afetai, Хвала, Hvala, ස්තූතියි, Vďaka, Hvala, Mahadsanid, Thanks, Gracias, Tack, Salamat, நன்றி, నెనరులు, ขอบคุณค่ะ!

          Your work has made it into this weeks Moodle release! There are no gold medals available this week - but millions around the world will benefit. Thank you!

          Show
          Dan Poltawski added a comment - asko, Дзякуй, ধন্যবাদ, Благодаря, Gràcies, 感谢, 謝謝, Hvala, Díky, Tak, Bedankt, Tänan, متشکریم, Salamat, Kiitokset, Merci, Grazas, Danke, Ευχαριστώ, આભાર, תודה, धन्यवाद, Köszönjük, Takk fyrir, Terima Kasih, Grazie, ありがとうございます, Рахмет, សូមអរគុណ, 감사합니다, gratiās, Pateicamies, Ačiū, Благодарам, Tēnā koa, Kia Ora Rawa Atu, आभारी आहोत, Талархал, Takk, Dziękuję, Obrigado, Mulţumesc, Engraziel, Спасибо, Fa'afetai, Хвала, Hvala, ස්තූතියි, Vďaka, Hvala, Mahadsanid, Thanks, Gracias, Tack, Salamat, நன்றி, నెనరులు, ขอบคุณค่ะ! Your work has made it into this weeks Moodle release! There are no gold medals available this week - but millions around the world will benefit. Thank you!
          Hide
          Daniel Neis added a comment -

          Hello,

          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 later at line 413 the condition tests if the field is a key

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

          when it should be

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

          This is true for other fields like institution, department, phone1 and phone2.

          Show
          Daniel Neis added a comment - Hello, 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 later at line 413 the condition tests if the field is a key if ($isadmin or $currentuser or isset($showuseridentityfields['idnumber'])) when it should be if ($isadmin or $currentuser or in_array('idnumber',$showuseridentityfields)) This is true for other fields like institution, department, phone1 and phone2.
          Hide
          Daniel Neis added a comment -

          Hello,

          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

          Show
          Daniel Neis added a comment - Hello, 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
          Hide
          Dan Poltawski added a comment -

          How did this ever pass testing???

          Show
          Dan Poltawski added a comment - How did this ever pass testing???
          Hide
          Dan Poltawski added a comment -

          Thanks Daniel, please could you file a new bug for this? Seems like we should have some unit tests for this to.

          Show
          Dan Poltawski added a comment - Thanks Daniel, please could you file a new bug for this? Seems like we should have some unit tests for this to.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: