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

      Gliffy Diagrams

      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: