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:

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Crafton Williams created issue -
            Petr Skoda 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: