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

          Crafton Williams created issue -
          Petr Škoda made changes -
          Field Original Value New Value
          Assignee Petr Škoda (skodak) [ skodak ] Jerome Mouneyrac [ jerome ]
          Michael de Raadt made changes -
          Fix Version/s STABLE backlog [ 10463 ]
          Labels triaged
          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.
          Jérôme Mouneyrac made changes -
          Status Open [ 1 ] Development in progress [ 3 ]
          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
          Jérôme Mouneyrac made changes -
          Status Development in progress [ 3 ] Open [ 1 ]
          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.
          Crafton Williams made changes -
          Labels triaged patch triaged
          Jérôme Mouneyrac made changes -
          Status Open [ 1 ] Development in progress [ 3 ]
          Jérôme Mouneyrac made changes -
          Testing Instructions 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)
          Jérôme Mouneyrac made changes -
          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.
          Jérôme Mouneyrac made changes -
          Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
          Peer reviewer rwijaya
          Hide
          Crafton Williams added a comment -

          no problem, glad i could assist.

          Show
          Crafton Williams added a comment - no problem, glad i could assist.
          Rossiani Wijaya made changes -
          Original Estimate 0 minutes [ 0 ]
          Remaining Estimate 0 minutes [ 0 ]
          Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
          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.
          Rossiani Wijaya made changes -
          Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
          Michael de Raadt made changes -
          Link This issue will help resolve MDL-34384 [ MDL-34384 ]
          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.
          Jérôme Mouneyrac made changes -
          Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
          Jérôme Mouneyrac made changes -
          Jérôme Mouneyrac made changes -
          Testing Instructions 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)
          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.
          Jérôme Mouneyrac made changes -
          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
          Dan Poltawski made changes -
          Currently in integration Yes [ 10041 ]
          Dan Poltawski made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          Integrator poltawski
          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 )
          Dan Poltawski made changes -
          Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
          Fix Version/s 2.2.5 [ 12352 ]
          Fix Version/s 2.3.2 [ 12353 ]
          Fix Version/s STABLE backlog [ 10463 ]
          Michael de Raadt made changes -
          Tester davmon
          Michael de Raadt made changes -
          Tester davmon ankit_frenz
          Ankit Agarwal made changes -
          Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
          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
          Ankit Agarwal made changes -
          Status Testing in progress [ 10011 ] Problem during testing [ 10007 ]
          Dan Poltawski made changes -
          Status Problem during testing [ 10007 ] Integration review in progress [ 10004 ]
          Dan Poltawski made changes -
          Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
          Dan Poltawski made changes -
          Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
          Hide
          Dan Poltawski added a comment -

          Done.

          Show
          Dan Poltawski added a comment - Done.
          Dan Poltawski made changes -
          Status Testing in progress [ 10011 ] Tested [ 10006 ]
          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!
          Dan Poltawski made changes -
          Status Tested [ 10006 ] Closed [ 6 ]
          Resolution Fixed [ 1 ]
          Integration date 02/Aug/12
          Dan Poltawski made changes -
          Currently in integration Yes [ 10041 ]
          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
          Daniel Neis made changes -
          Link This issue is a clone of MDL-38136 [ MDL-38136 ]
          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: