Moodle
  1. Moodle
  2. MDL-27976

moodle_user_get_users_by_courseid

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.1
    • Component/s: Web Services
    • Labels:
      None
    • Rank:
      17614
    1. xmlrpc.php
      0.9 kB
      Dongsheng Cai

      Issue Links

        Activity

        Hide
        Dongsheng Cai added a comment -

        Sam, can you please peer review this change? If you don't have time, I will ask others to review.

        Thanks.

        Regards,
        Dongsheng

        Show
        Dongsheng Cai added a comment - Sam, can you please peer review this change? If you don't have time, I will ask others to review. Thanks. Regards, Dongsheng
        Hide
        Dongsheng Cai added a comment -

        I wrote a simple xmlrpc client in PHP for testing

        Show
        Dongsheng Cai added a comment - I wrote a simple xmlrpc client in PHP for testing
        Hide
        Rossiani Wijaya added a comment -

        Tested with the attached xmlrpc testing file above and it works great.
        Return fields varies depending on user role, which it should be.

        This looks great to me.

        Show
        Rossiani Wijaya added a comment - Tested with the attached xmlrpc testing file above and it works great. Return fields varies depending on user role, which it should be. This looks great to me.
        Hide
        Rossiani Wijaya added a comment -

        submit for integration

        Show
        Rossiani Wijaya added a comment - submit for integration
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Aha, I see the change to central/reused user_get_user_details() like a nice thing... but...

        1 - why fake $course @ get_users_by_id() ? It should be course-independent user information IMO, so surely should accept empty $course.

        2 - same for $usercontext @ get_users_by_id(). It is ALWAYS the $user context, isn't it? Why do we need to pass the same info twice?

        3 - why "innocent" information like the theme or the timezone is dependent of $hasuserupdatecap ?

        4 - get_course_participants_by_id() sets the context to course context when calling user_get_user_details(), isn't that, again repeating information? IMO the $course optional param commented in 1) should be enough to calculate context.

        5 - also note I'm not 100% sure that all the checks performed within the user_get_user_details() can be done both when the function is called with a $course (course context) or without it (user context). IMO they are not interchangeable at all. Some only have sense at one context and not at the other at all.

        6 - get_users_by_courseid() is subject to exactly the same problem that the ones commented @ 4) and 5) above.

        7 - So, based in all the above IMO user_get_user_details():

        7a) should accept mandatory $user and optional $course.
        7b) based on the params in 7a and 7b, it should calculate the proper user and course contexts and then, use them as necessary for each capability check. But for sure a lot of them are not interchangeable at all

        8) Integrating this means that the 3 services involved (get_users_by_id, get_course_participants_by_id, get_users_by_courseid) and the new central function itself (user_get_user_details) must be:

        8a) retested
        8b) fully covered by unit tests

        I'm sorry but I'm not 100% confident all this stuff is ready for integration in current form. After all, those services are about disclosing personal info and must be triple checked / ensured to be doing everything in a correct way. An right now they seem to need some more love.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Aha, I see the change to central/reused user_get_user_details() like a nice thing... but... 1 - why fake $course @ get_users_by_id() ? It should be course-independent user information IMO, so surely should accept empty $course. 2 - same for $usercontext @ get_users_by_id(). It is ALWAYS the $user context, isn't it? Why do we need to pass the same info twice? 3 - why "innocent" information like the theme or the timezone is dependent of $hasuserupdatecap ? 4 - get_course_participants_by_id() sets the context to course context when calling user_get_user_details(), isn't that, again repeating information? IMO the $course optional param commented in 1) should be enough to calculate context. 5 - also note I'm not 100% sure that all the checks performed within the user_get_user_details() can be done both when the function is called with a $course (course context) or without it (user context). IMO they are not interchangeable at all. Some only have sense at one context and not at the other at all. 6 - get_users_by_courseid() is subject to exactly the same problem that the ones commented @ 4) and 5) above. 7 - So, based in all the above IMO user_get_user_details(): 7a) should accept mandatory $user and optional $course. 7b) based on the params in 7a and 7b, it should calculate the proper user and course contexts and then, use them as necessary for each capability check. But for sure a lot of them are not interchangeable at all 8) Integrating this means that the 3 services involved (get_users_by_id, get_course_participants_by_id, get_users_by_courseid) and the new central function itself (user_get_user_details) must be: 8a) retested 8b) fully covered by unit tests I'm sorry but I'm not 100% confident all this stuff is ready for integration in current form. After all, those services are about disclosing personal info and must be triple checked / ensured to be doing everything in a correct way. An right now they seem to need some more love. Ciao
        Hide
        Dongsheng Cai added a comment -

        Hi, Eloy

        I just fixed user_get_user_details to take $user and optional $course parameters.

        Can you explain further about the non-interchangeable fields, from what I see, groups and roles doesn't make sense in user context, but they are excluded by !empty($course) check.

        Regards,
        Dongsheng

        Show
        Dongsheng Cai added a comment - Hi, Eloy I just fixed user_get_user_details to take $user and optional $course parameters. Can you explain further about the non-interchangeable fields, from what I see, groups and roles doesn't make sense in user context, but they are excluded by !empty($course) check. Regards, Dongsheng
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Ho, from my reviewing the other day I concluded that the same capabilities were being checked, sometimes for a user context and others for a course context.

        And that's 99% of the times a sign of something wrong, as far as user-related capabilities are very special and should be only tested at user contexts. And also the opposite. And I think I saw some cap. checks behaving that way, that was the 5) above).

        Also this misses testing instructions so I was asking for them, so testers will be able to do the job, or alternatively point to some QAs for that or, alternatively, know if these (four) functions are covered by unit tests.

        So I think this can be integrated if:

        a - 5) above is not happening, so each cap is checked in correct context always.
        b - know which is the testing status for this (add instructions, point to existing MDLQAs, rely exclusively on unittests...

        Sure this would be improved later but a) and b) are mandatory, I think. Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Ho, from my reviewing the other day I concluded that the same capabilities were being checked, sometimes for a user context and others for a course context. And that's 99% of the times a sign of something wrong, as far as user-related capabilities are very special and should be only tested at user contexts. And also the opposite. And I think I saw some cap. checks behaving that way, that was the 5) above). Also this misses testing instructions so I was asking for them, so testers will be able to do the job, or alternatively point to some QAs for that or, alternatively, know if these (four) functions are covered by unit tests. So I think this can be integrated if: a - 5) above is not happening, so each cap is checked in correct context always. b - know which is the testing status for this (add instructions, point to existing MDLQAs, rely exclusively on unittests... Sure this would be improved later but a) and b) are mandatory, I think. Ciao
        Hide
        Dongsheng Cai added a comment -

        Hi Eloy

        In moodle, we have user/profile.php for full profile and user/view.php for course profile, I was thinking the get_users_by_courseid should behave as user/view.php, but unfortunately, after talking with martin, I knew that page is unfinished, that means we haven't well defined what capabilities required and what fields what be displayed in course context.

        In the webservice, there are 6 caps involved:

        moodle/user:viewdetails (used in course and user context)

        moodle/course:viewhiddenuserfields (if $course not null, test it in course context)
        moodle/user:viewhiddendetails (if $course is null, use it for user context)

        moodle/site:viewfullnames (used in course and user context)

        moodle/course:useremail (used in course context)

        moodle/site:accessallgroups (used in course context)

        moodle/user:viewdetails and moodle/site:viewfullnames checked by both contexts, in lib/db/access.php, it defined allowing context_course, I am not sure if this is the problem.

        Regarding unittest, I understand it's essential, but I don't think I could finish unit tests for all these webservices by tomorrow, I do have a php script to test the result.

        Regards,
        Dongsheng

        Show
        Dongsheng Cai added a comment - Hi Eloy In moodle, we have user/profile.php for full profile and user/view.php for course profile, I was thinking the get_users_by_courseid should behave as user/view.php, but unfortunately, after talking with martin, I knew that page is unfinished, that means we haven't well defined what capabilities required and what fields what be displayed in course context. In the webservice, there are 6 caps involved: moodle/user:viewdetails (used in course and user context) moodle/course:viewhiddenuserfields (if $course not null, test it in course context) moodle/user:viewhiddendetails (if $course is null, use it for user context) moodle/site:viewfullnames (used in course and user context) moodle/course:useremail (used in course context) moodle/site:accessallgroups (used in course context) moodle/user:viewdetails and moodle/site:viewfullnames checked by both contexts, in lib/db/access.php, it defined allowing context_course, I am not sure if this is the problem. Regarding unittest, I understand it's essential, but I don't think I could finish unit tests for all these webservices by tomorrow, I do have a php script to test the result. Regards, Dongsheng
        Hide
        Michael de Raadt added a comment -

        I believe Dongsheng is referring to the Course profile page being unfinished.

        My interpretation of this is that as much profile information as possible is being returned, as permitted by the capacities.

        I agree that we don't have time to create unit tests and QA tests for these changes prior to 2.1 release. Dongsheng has created an issue so he can deal with that post-2.1 (MDL-28089). A review of unit test and QA test coverage is on my priority list for post-2.1.

        Michael;

        Show
        Michael de Raadt added a comment - I believe Dongsheng is referring to the Course profile page being unfinished. My interpretation of this is that as much profile information as possible is being returned, as permitted by the capacities. I agree that we don't have time to create unit tests and QA tests for these changes prior to 2.1 release. Dongsheng has created an issue so he can deal with that post-2.1 ( MDL-28089 ). A review of unit test and QA test coverage is on my priority list for post-2.1. Michael;
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Oki, perfect, I just was asking for confirmation of:

        a) all the caps are always evaluated in the correct (course/user) contexts. Dong confirmed it.
        b) MDLQA and unittests are coming (after release) for this and all WS. Michael confirmed it.

        So, then, this is going to be integrated. Simple

        Show
        Eloy Lafuente (stronk7) added a comment - Oki, perfect, I just was asking for confirmation of: a) all the caps are always evaluated in the correct (course/user) contexts. Dong confirmed it. b) MDLQA and unittests are coming (after release) for this and all WS. Michael confirmed it. So, then, this is going to be integrated. Simple
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated, thanks.

        Note for testers: These 3 WS need to be tested:

        • get_users_by_id
        • get_course_participants_by_id
        • get_users_by_courseid
        Show
        Eloy Lafuente (stronk7) added a comment - Integrated, thanks. Note for testers: These 3 WS need to be tested: get_users_by_id get_course_participants_by_id get_users_by_courseid
        Hide
        Aparup Banerjee added a comment - - edited

        for : XML-RPC protocol: moodle_user_get_users_by_id

        URL: http://aparup.moodle.local/m20/mysql/moodle/webservice/xmlrpc/server.php?wstoken=3f3d0c0b10fb49ad897cb4d500bb0aae
        Warning: SimpleXMLElement::__construct(): Entity: line 2: parser error : Start tag expected, '<' not found in /home/aparup/mcode/m20/mysql/moodle/lib/zend/Zend/XmlRpc/Response.php on line 180 Call Stack: 0.0005 727808 1.

        {main}() /home/aparup/mcode/m20/mysql/moodle/admin/webservice/testclient.php:0 0.9973 87215896 2. webservice_xmlrpc_test_client->simpletest() /home/aparup/mcode/m20/mysql/moodle/admin/webservice/testclient.php:154 1.0045 88637608 3. Zend_XmlRpc_Client->call() /home/aparup/mcode/m20/mysql/moodle/webservice/xmlrpc/locallib.php:72 1.2948 89878856 4. Zend_XmlRpc_Client->doRequest() /home/aparup/mcode/m20/mysql/moodle/lib/zend/Zend/XmlRpc/Client.php:361 1.6612 89893424 5. Zend_XmlRpc_Response->loadXml() /home/aparup/mcode/m20/mysql/moodle/lib/zend/Zend/XmlRpc/Client.php:297 1.6612 89893968 6. SimpleXMLElement->_construct() /home/aparup/mcode/m20/mysql/moodle/lib/zend/Zend/XmlRpc/Response.php:180 Warning: SimpleXMLElement::_construct(): Notice: Undefined variable: canviewdetailscap in /home/aparup/mcode/m20/mysql/mo in /home/aparup/mcode/m20/mysql/moodle/lib/zend/Zend/XmlRpc/Response.php on line 180 Call Stack: 0.0005 727808 1. {main}

        () /home/aparup/mcode/m20/mysql/moodle/admin/webservice/testclient.php:0 0.9973 87215896 2. webservice_xmlrpc_test_client->simpletest() /home/aparup/mcode/m20/mysql/moodle/admin/webservice/testclient.php:154 1.0045 88637608 3. Zend_XmlRpc_Client->call() /home/aparup/mcode/m20/mysql/moodle/webservice/xmlrpc/locallib.php:72 1.2948 89878856 4. Zend_XmlRpc_Client->doRequest() /home/aparup/mcode/m20/mysql/moodle/lib/zend/Zend/XmlRpc/Client.php:361 1.6612 89893424 5. Zend_XmlRpc_Response->loadXml() /home/aparup/mcode/m20/mysql/moodle/lib/zend/Zend/XmlRpc/Client.php:297 1.6612 89893968 6. SimpleXMLElement->_construct() /home/aparup/mcode/m20/mysql/moodle/lib/zend/Zend/XmlRpc/Response.php:180 Warning: SimpleXMLElement::_construct(): ^ in /home/aparup/mcode/m20/mysql/moodle/lib/zend/Zend/XmlRpc/Response.php on line 180 Call Stack: 0.0005 727808 1.

        {main}() /home/aparup/mcode/m20/mysql/moodle/admin/webservice/testclient.php:0 0.9973 87215896 2. webservice_xmlrpc_test_client->simpletest() /home/aparup/mcode/m20/mysql/moodle/admin/webservice/testclient.php:154 1.0045 88637608 3. Zend_XmlRpc_Client->call() /home/aparup/mcode/m20/mysql/moodle/webservice/xmlrpc/locallib.php:72 1.2948 89878856 4. Zend_XmlRpc_Client->doRequest() /home/aparup/mcode/m20/mysql/moodle/lib/zend/Zend/XmlRpc/Client.php:361 1.6612 89893424 5. Zend_XmlRpc_Response->loadXml() /home/aparup/mcode/m20/mysql/moodle/lib/zend/Zend/XmlRpc/Client.php:297 1.6612 89893968 6. SimpleXMLElement->__construct() /home/aparup/mcode/m20/mysql/moodle/lib/zend/Zend/XmlRpc/Response.php:180 exception 'Zend_XmlRpc_Client_FaultException' with message 'Failed to parse response' in /home/aparup/mcode/m20/mysql/moodle/lib/zend/Zend/XmlRpc/Client.php:370
        Stack trace:
        #0 /home/aparup/mcode/m20/mysql/moodle/webservice/xmlrpc/locallib.php(72): Zend_XmlRpc_Client->call('moodle_user_get...', Array)
        #1 /home/aparup/mcode/m20/mysql/moodle/admin/webservice/testclient.php(154): webservice_xmlrpc_test_client->simpletest('http://aparup.m...', 'moodle_user_get...', Array)
        #2 {main}
        Show
        Aparup Banerjee added a comment - - edited for : XML-RPC protocol: moodle_user_get_users_by_id URL: http://aparup.moodle.local/m20/mysql/moodle/webservice/xmlrpc/server.php?wstoken=3f3d0c0b10fb49ad897cb4d500bb0aae Warning: SimpleXMLElement::__construct(): Entity: line 2: parser error : Start tag expected, '<' not found in /home/aparup/mcode/m20/mysql/moodle/lib/zend/Zend/XmlRpc/Response.php on line 180 Call Stack: 0.0005 727808 1. {main}() /home/aparup/mcode/m20/mysql/moodle/admin/webservice/testclient.php:0 0.9973 87215896 2. webservice_xmlrpc_test_client->simpletest() /home/aparup/mcode/m20/mysql/moodle/admin/webservice/testclient.php:154 1.0045 88637608 3. Zend_XmlRpc_Client->call() /home/aparup/mcode/m20/mysql/moodle/webservice/xmlrpc/locallib.php:72 1.2948 89878856 4. Zend_XmlRpc_Client->doRequest() /home/aparup/mcode/m20/mysql/moodle/lib/zend/Zend/XmlRpc/Client.php:361 1.6612 89893424 5. Zend_XmlRpc_Response->loadXml() /home/aparup/mcode/m20/mysql/moodle/lib/zend/Zend/XmlRpc/Client.php:297 1.6612 89893968 6. SimpleXMLElement->_ construct() /home/aparup/mcode/m20/mysql/moodle/lib/zend/Zend/XmlRpc/Response.php:180 Warning: SimpleXMLElement:: _construct(): Notice: Undefined variable: canviewdetailscap in /home/aparup/mcode/m20/mysql/mo in /home/aparup/mcode/m20/mysql/moodle/lib/zend/Zend/XmlRpc/Response.php on line 180 Call Stack: 0.0005 727808 1. {main} () /home/aparup/mcode/m20/mysql/moodle/admin/webservice/testclient.php:0 0.9973 87215896 2. webservice_xmlrpc_test_client->simpletest() /home/aparup/mcode/m20/mysql/moodle/admin/webservice/testclient.php:154 1.0045 88637608 3. Zend_XmlRpc_Client->call() /home/aparup/mcode/m20/mysql/moodle/webservice/xmlrpc/locallib.php:72 1.2948 89878856 4. Zend_XmlRpc_Client->doRequest() /home/aparup/mcode/m20/mysql/moodle/lib/zend/Zend/XmlRpc/Client.php:361 1.6612 89893424 5. Zend_XmlRpc_Response->loadXml() /home/aparup/mcode/m20/mysql/moodle/lib/zend/Zend/XmlRpc/Client.php:297 1.6612 89893968 6. SimpleXMLElement->_ construct() /home/aparup/mcode/m20/mysql/moodle/lib/zend/Zend/XmlRpc/Response.php:180 Warning: SimpleXMLElement:: _construct(): ^ in /home/aparup/mcode/m20/mysql/moodle/lib/zend/Zend/XmlRpc/Response.php on line 180 Call Stack: 0.0005 727808 1. {main}() /home/aparup/mcode/m20/mysql/moodle/admin/webservice/testclient.php:0 0.9973 87215896 2. webservice_xmlrpc_test_client->simpletest() /home/aparup/mcode/m20/mysql/moodle/admin/webservice/testclient.php:154 1.0045 88637608 3. Zend_XmlRpc_Client->call() /home/aparup/mcode/m20/mysql/moodle/webservice/xmlrpc/locallib.php:72 1.2948 89878856 4. Zend_XmlRpc_Client->doRequest() /home/aparup/mcode/m20/mysql/moodle/lib/zend/Zend/XmlRpc/Client.php:361 1.6612 89893424 5. Zend_XmlRpc_Response->loadXml() /home/aparup/mcode/m20/mysql/moodle/lib/zend/Zend/XmlRpc/Client.php:297 1.6612 89893968 6. SimpleXMLElement->__construct() /home/aparup/mcode/m20/mysql/moodle/lib/zend/Zend/XmlRpc/Response.php:180 exception 'Zend_XmlRpc_Client_FaultException' with message 'Failed to parse response' in /home/aparup/mcode/m20/mysql/moodle/lib/zend/Zend/XmlRpc/Client.php:370 Stack trace: #0 /home/aparup/mcode/m20/mysql/moodle/webservice/xmlrpc/locallib.php(72): Zend_XmlRpc_Client->call('moodle_user_get...', Array) #1 /home/aparup/mcode/m20/mysql/moodle/admin/webservice/testclient.php(154): webservice_xmlrpc_test_client->simpletest('http://aparup.m...', 'moodle_user_get...', Array) #2 {main}
        Hide
        Aparup Banerjee added a comment -

        dongsheng has fixed that just now (its a quick fix Notice: Undefined variable: canviewdetailscap)

        Show
        Aparup Banerjee added a comment - dongsheng has fixed that just now (its a quick fix Notice: Undefined variable: canviewdetailscap)
        Hide
        Dongsheng Cai added a comment - - edited

        Here is the fix: https://github.com/dongsheng/moodle/commit/1e539f647d32eccf1a2692c495f8dcf0c6539736

        commit hash: 1e539f647d32eccf1a2692c495f8dcf0c6539736

        This branch is against integration/master

        Show
        Dongsheng Cai added a comment - - edited Here is the fix: https://github.com/dongsheng/moodle/commit/1e539f647d32eccf1a2692c495f8dcf0c6539736 commit hash: 1e539f647d32eccf1a2692c495f8dcf0c6539736 This branch is against integration/master
        Hide
        Aparup Banerjee added a comment -

        ok,

        • get_users_by_id
        • get_users_by_courseid
          both work for me.
        Show
        Aparup Banerjee added a comment - ok, get_users_by_id get_users_by_courseid both work for me.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Yay, this is now part of the just released Moodle 2.1 ! Thanks for all the hard work!

        Show
        Eloy Lafuente (stronk7) added a comment - Yay, this is now part of the just released Moodle 2.1 ! Thanks for all the hard work!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: