Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.4
    • Component/s: Web Services
    • Labels:
    • Rank:
      38502

      Description

      Create web service core_users_get_users_with_capability

      Parameters: Array of course ids (required), Array of capabilities (required)
      Returns: Array of users that have the capabilities for each course specified

        Issue Links

          Activity

          Hide
          Jérôme Mouneyrac added a comment -

          Hi Paul,
          thanks for writting all these issues. For any new issues, can you link all your issues to the roadmap issue as indicated in the description: MDL-29934
          Also add the maintainer for each of them as watcher. Maintainers have to valid the specs.
          You can also assign the issues to yourself if you are going to implement them. I don't know if you have these kind of permissions in Jira (link/assign), let me know if you have any issue for it.
          Thanks.

          Show
          Jérôme Mouneyrac added a comment - Hi Paul, thanks for writting all these issues. For any new issues, can you link all your issues to the roadmap issue as indicated in the description: MDL-29934 Also add the maintainer for each of them as watcher. Maintainers have to valid the specs. You can also assign the issues to yourself if you are going to implement them. I don't know if you have these kind of permissions in Jira (link/assign), let me know if you have any issue for it. Thanks.
          Hide
          Jérôme Mouneyrac added a comment -

          About this function:
          why do you need to retrieve users by capabilities?

          Looking at it without more informatin it seems to be quite a specific function. Could it be integrated with Fabio's conribution (not reviewed yet) in MDL-29938 ?

          Show
          Jérôme Mouneyrac added a comment - About this function: why do you need to retrieve users by capabilities? Looking at it without more informatin it seems to be quite a specific function. Could it be integrated with Fabio's conribution (not reviewed yet) in MDL-29938 ?
          Hide
          Paul Charsley added a comment -

          Hi Jerome,
          Lightwork will use this to get a list of markers, marking managers and students in a course. This is part of the marking management functionality that will be included in the new Moodle 2.3 assignment module. However, it seems likely that other web service clients might want to use this to identify users with specific capabilities for other contexts such as assignments. It seems very different to MDL-29938 which plans to use key pairs to search with user attributes.

          I will remember to follow your instructions for any new issues.

          Thanks, Paul

          Show
          Paul Charsley added a comment - Hi Jerome, Lightwork will use this to get a list of markers, marking managers and students in a course. This is part of the marking management functionality that will be included in the new Moodle 2.3 assignment module. However, it seems likely that other web service clients might want to use this to identify users with specific capabilities for other contexts such as assignments. It seems very different to MDL-29938 which plans to use key pairs to search with user attributes. I will remember to follow your instructions for any new issues. Thanks, Paul
          Hide
          Jérôme Mouneyrac added a comment -

          Ah you are right. I guess we can return less information about the user, just the minimal.

          core_users_get_users_with_capability

          Parameters:
          Array of

          { - context id - Array of capabilities }

          Returns:
          Array of

          { - context id - capability - array of user }
          Show
          Jérôme Mouneyrac added a comment - Ah you are right. I guess we can return less information about the user, just the minimal. core_users_get_users_with_capability Parameters: Array of { - context id - Array of capabilities } Returns: Array of { - context id - capability - array of user }
          Hide
          Jérôme Mouneyrac added a comment -

          Moving the issue to the roadmap.

          Show
          Jérôme Mouneyrac added a comment - Moving the issue to the roadmap.
          Hide
          Paul Charsley added a comment -

          Hi Jerome,

          Now that we are moving away from the use of context ids in parameters I suggest renaming this function to core_users_get_course_participants_with_capability and replace the contextids parameter with courseids.

          Thanks, Paul

          Show
          Paul Charsley added a comment - Hi Jerome, Now that we are moving away from the use of context ids in parameters I suggest renaming this function to core_users_get_course_participants_with_capability and replace the contextids parameter with courseids. Thanks, Paul
          Hide
          Yang Yang added a comment -

          Hello Jerome

          I am currently working on this issue with Paul but I have trouble of getting this it assigned it to me. Is there anything I need to setup to be a participant of this issue? Paul was trying to assign this issue to me but could not find my name on the list.

          Thanks for your help.
          Yang

          Show
          Yang Yang added a comment - Hello Jerome I am currently working on this issue with Paul but I have trouble of getting this it assigned it to me. Is there anything I need to setup to be a participant of this issue? Paul was trying to assign this issue to me but could not find my name on the list. Thanks for your help. Yang
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Yang, I don't have the permission to assign people to the list. You need to see with Michael.

          Show
          Jérôme Mouneyrac added a comment - Hi Yang, I don't have the permission to assign people to the list. You need to see with Michael .
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Hi Yang, here's my review:

          • sorting user by capabilities doesn't exist in Moodle (at least I don't know it). You need to find out exactly which capability should be checked. It seems to me like it's going to be some role cap. 'moodle/user:viewdetails' doesn't seem correct to me, it is used to see user profile not his/her capabilities. From looking at the permission list: moodle/role:review OR moodle/role:manage + moodle/role:assign. But I'm not 100% sure as it's a new feature. You'll be told during integration review. Then note that this function will probably most of time be only used by admins/managers, as I don't see students having permission to see/sort capability of other users.
          • functions should be ordered this way: xxx_params(), xxx() then xxx_returns()
          • capability are PARAM_CAPABILITY
          • capability should be called capabilities as it's an array of capability
          • 'Capability names, such as mod/forum:viewdiscussion' is describing one capability, it should be singular.
          • PARAM_NUMBER is deprecated for PARAM_FLOAT. However for ids you want to use PARAM_INT
          • look at get_course_user_profiles_returns() to be consistent with the PARAM_XXX.

          I think it's about it.
          Thanks Yang.

          About warning messages:
          let's define this on http://docs.moodle.org/dev/Errors_handling_in_web_services

          Show
          Jérôme Mouneyrac added a comment - - edited Hi Yang, here's my review: sorting user by capabilities doesn't exist in Moodle (at least I don't know it). You need to find out exactly which capability should be checked. It seems to me like it's going to be some role cap. 'moodle/user:viewdetails' doesn't seem correct to me, it is used to see user profile not his/her capabilities. From looking at the permission list: moodle/role:review OR moodle/role:manage + moodle/role:assign. But I'm not 100% sure as it's a new feature. You'll be told during integration review. Then note that this function will probably most of time be only used by admins/managers, as I don't see students having permission to see/sort capability of other users. functions should be ordered this way: xxx_params(), xxx() then xxx_returns() capability are PARAM_CAPABILITY capability should be called capabilities as it's an array of capability 'Capability names, such as mod/forum:viewdiscussion' is describing one capability, it should be singular. PARAM_NUMBER is deprecated for PARAM_FLOAT. However for ids you want to use PARAM_INT look at get_course_user_profiles_returns() to be consistent with the PARAM_XXX. I think it's about it. Thanks Yang. About warning messages: let's define this on http://docs.moodle.org/dev/Errors_handling_in_web_services
          Hide
          Yang Yang added a comment -

          Hi Jerome

          Thanks for your reply. I have made changes according to the review. get_users_by_capability() has been replaced with get_enrolled_users(). Also the function is likely to be used by markers and markingmanagers in the Lightwork, so the required capability is changed to 'mod/assignment:grade'.

          Regards,
          Yang

          Show
          Yang Yang added a comment - Hi Jerome Thanks for your reply. I have made changes according to the review. get_users_by_capability() has been replaced with get_enrolled_users(). Also the function is likely to be used by markers and markingmanagers in the Lightwork, so the required capability is changed to 'mod/assignment:grade'. Regards, Yang
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Hi Yang,
          I'd just like you to share a thought about getting users by web service:

          • currently client devs can retrieve users with core_user_get_users_by_id (system profile) and core_user_get_course_user_profiles (perticipant/course profile).
          • I suppose core_users_get_users_with_capability and core_user_get_course_user_profiles should have a similar code as they return participant profiles. I didn't check why core_user_get_course_user_profiles doesn't use get_enrolled_users but it would seem accurate to use it in this function too.
          • a new core_user_get_users function is been implemented: MDL-29938. It will be a mixed of system/participant profiles. I think that core_users_get_users_with_capability is called in very specific cases so I don't mind if we keep it and core_user_get_users doesn't do it. In the worst situation if core_users_get_users_with_capability code is really similar to core_user_get_users code, we will deprecate core_users_get_users_with_capability later and we will add it to core_user_get_users.

          Now to come back to the function review:

          • remove the reference to lightwork (you will add it later if lightwork is integrated to core):
                'Lightwork' => array(
             	  'functions' => array (
             	  'core_users_get_users_with_capability'),
             	  'restrictedusers' => 0,
             	  'enabled' => 1,
             	  'shortname' => 'Lightwork'
             	)
            
          • for the warning description, I came up with that: http://docs.moodle.org/dev/Errors_handling_in_web_services#Warning_messages. It's just a suggestion but as you are at lightwork team using most of these kind of warnings, it would be good we agree on something generic quickly. I was thinking that returning multiple elements could be useful sometimes but maybe it's better to change the warning structure depending what the function really need to return.
          • I'm unsure about what capability should be used. Letting people search by capability because they can grade 'mod/assignment:grade' seems a bit strange. You can keep it though, we'll see what the integrator thinks.
          • in your fields parameter 'name, 1' what is 1 ?
          • timemodified should be PARAM_INT
          Show
          Jérôme Mouneyrac added a comment - - edited Hi Yang, I'd just like you to share a thought about getting users by web service: currently client devs can retrieve users with core_user_get_users_by_id (system profile) and core_user_get_course_user_profiles (perticipant/course profile). I suppose core_users_get_users_with_capability and core_user_get_course_user_profiles should have a similar code as they return participant profiles. I didn't check why core_user_get_course_user_profiles doesn't use get_enrolled_users but it would seem accurate to use it in this function too. a new core_user_get_users function is been implemented: MDL-29938 . It will be a mixed of system/participant profiles. I think that core_users_get_users_with_capability is called in very specific cases so I don't mind if we keep it and core_user_get_users doesn't do it. In the worst situation if core_users_get_users_with_capability code is really similar to core_user_get_users code, we will deprecate core_users_get_users_with_capability later and we will add it to core_user_get_users. Now to come back to the function review: remove the reference to lightwork (you will add it later if lightwork is integrated to core): 'Lightwork' => array( 'functions' => array ( 'core_users_get_users_with_capability'), 'restrictedusers' => 0, 'enabled' => 1, 'shortname' => 'Lightwork' ) for the warning description, I came up with that: http://docs.moodle.org/dev/Errors_handling_in_web_services#Warning_messages . It's just a suggestion but as you are at lightwork team using most of these kind of warnings, it would be good we agree on something generic quickly. I was thinking that returning multiple elements could be useful sometimes but maybe it's better to change the warning structure depending what the function really need to return. I'm unsure about what capability should be used. Letting people search by capability because they can grade 'mod/assignment:grade' seems a bit strange. You can keep it though, we'll see what the integrator thinks. in your fields parameter 'name, 1' what is 1 ? timemodified should be PARAM_INT
          Hide
          Jérôme Mouneyrac added a comment -

          Except the above points, I tested and all works well.

          Show
          Jérôme Mouneyrac added a comment - Except the above points, I tested and all works well.
          Hide
          Jérôme Mouneyrac added a comment -
          • running the test instruction I found out we don't need the part about testing if the capability exist. When we valid against PARAM_CAPABILITY, the lower code check if this capability exists. I think you can remove all the code checking if the capability exists.
          Show
          Jérôme Mouneyrac added a comment - running the test instruction I found out we don't need the part about testing if the capability exist. When we valid against PARAM_CAPABILITY, the lower code check if this capability exists. I think you can remove all the code checking if the capability exists.
          Hide
          Yang Yang added a comment -

          Hi Jerome

          I have changed the code according to your review. Warning description has not been modified and Paul will discuss this with you at some stage.

          Regarding capability, this function is likely to be used by the marking managers and markers in Lightwork. So I am looking for a capability that both teacher and non-editing teacher have. "mod/assignment:grade" seems to be one of them. Another one would be "report/courseoverview:view". I assume in other cases, this function would be mostly used by admin. But in Lightwork, teacher and non-editing are allowed to use this function. Should I find a capability that would suit admin, teacher and non-editing teacher? What's your thought on this?

          Regards,
          Yang

          Show
          Yang Yang added a comment - Hi Jerome I have changed the code according to your review. Warning description has not been modified and Paul will discuss this with you at some stage. Regarding capability, this function is likely to be used by the marking managers and markers in Lightwork. So I am looking for a capability that both teacher and non-editing teacher have. "mod/assignment:grade" seems to be one of them. Another one would be "report/courseoverview:view". I assume in other cases, this function would be mostly used by admin. But in Lightwork, teacher and non-editing are allowed to use this function. Should I find a capability that would suit admin, teacher and non-editing teacher? What's your thought on this? Regards, Yang
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Yang,
          thanks for your changes.

          About capabilities: I don't think "mod/assignment:grade"/"report/courseoverview:view" suit 'see user capabilities'. I suppose you should stick to the capabilities that let you see the role capability list + the user role. At worst you could add a new capability for this web services like moodle/user:viewcapabilities but I'm unsure of the repercussions.

          Message to the integrator: you can review all the issue. The main point to review is about the capability checking:
          a) should we create a new cap?
          b) should we use existing caps and in this cases which ones/combinations ?

          Show
          Jérôme Mouneyrac added a comment - Hi Yang, thanks for your changes. About capabilities: I don't think "mod/assignment:grade"/"report/courseoverview:view" suit 'see user capabilities'. I suppose you should stick to the capabilities that let you see the role capability list + the user role. At worst you could add a new capability for this web services like moodle/user:viewcapabilities but I'm unsure of the repercussions. Message to the integrator: you can review all the issue. The main point to review is about the capability checking: a) should we create a new cap? b) should we use existing caps and in this cases which ones/combinations ?
          Hide
          Jérôme Mouneyrac added a comment -

          Hum... I had a look to the existing core_user_get_users_by_courseid() and inside this function it seems possible to retrieve user by capabilities. It seems core_users_get_users_with_capability is a duplicate?

          By the way the capability checked is 'moodle/role:review' in this function. It seems better suit.

          PS: I'll create a new tracker issue about the client dev description of get_users_by_courseid being not explicit. Client dev should not have to read the code to guess what the function is doing. And Yang would not have had to spend time on this current issue...

          Show
          Jérôme Mouneyrac added a comment - Hum... I had a look to the existing core_user_get_users_by_courseid() and inside this function it seems possible to retrieve user by capabilities. It seems core_users_get_users_with_capability is a duplicate? By the way the capability checked is 'moodle/role:review' in this function. It seems better suit. PS: I'll create a new tracker issue about the client dev description of get_users_by_courseid being not explicit. Client dev should not have to read the code to guess what the function is doing. And Yang would not have had to spend time on this current issue...
          Hide
          Jérôme Mouneyrac added a comment -

          Ah in fact I was looking at the wrong branch. It's now called core_enrol_get_enrolled_users and the description is correctly written. I think it does exactly what you want Yang, can we close this issue?

          Show
          Jérôme Mouneyrac added a comment - Ah in fact I was looking at the wrong branch. It's now called core_enrol_get_enrolled_users and the description is correctly written. I think it does exactly what you want Yang, can we close this issue?
          Hide
          Yang Yang added a comment -

          Hi Jerome

          I think core_enrol_get_enrolled_users works slightly different from what we would like to achieve. It only allows us to define one course and one capability at a time. However, the new function core_users_get_users_with_capability allows multiple capabilities and courseids to be specified. It would be more efficient to work with Lightwork. For example, if a client calls the webservice to get all non-editing teachers and teachers for multiple courses, core_enrol_get_enrolled_users wouldn't be efficient to handle this. What do you think, Jerome?

          Regards,
          Yang

          Show
          Yang Yang added a comment - Hi Jerome I think core_enrol_get_enrolled_users works slightly different from what we would like to achieve. It only allows us to define one course and one capability at a time. However, the new function core_users_get_users_with_capability allows multiple capabilities and courseids to be specified. It would be more efficient to work with Lightwork. For example, if a client calls the webservice to get all non-editing teachers and teachers for multiple courses, core_enrol_get_enrolled_users wouldn't be efficient to handle this. What do you think, Jerome? Regards, Yang
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Hi Yang,
          ok, change the capability check for 'moodle/role:review', it seems a better capability check to me as it matches the check in core_enrol_get_enrolled_users.
          I would also allow people that have both 'moodle/role:manage' + 'moodle/role:assign' as they can deduce what capability a user has.

          Then I'll resubmit it for integration if it gets rejected before you have time to discretely change it

          Show
          Jérôme Mouneyrac added a comment - - edited Hi Yang, ok, change the capability check for 'moodle/role:review', it seems a better capability check to me as it matches the check in core_enrol_get_enrolled_users. I would also allow people that have both 'moodle/role:manage' + 'moodle/role:assign' as they can deduce what capability a user has. Then I'll resubmit it for integration if it gets rejected before you have time to discretely change it
          Hide
          Yang Yang added a comment -

          Done. The capability has been changed to "moodle/role:review" according to the review.

          Show
          Yang Yang added a comment - Done. The capability has been changed to "moodle/role:review" according to the review.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Hi Yang,
          sorry to insist but let me know the reason you didn't implemented this check so I know why. I'm not saying that I'm right but it's good for my knowledge to understand the reason when I'm wrong:

          I would also allow people that have both 'moodle/role:manage' + 'moodle/role:assign' as they can deduce what capability a user has.

          Show
          Jérôme Mouneyrac added a comment - - edited Hi Yang, sorry to insist but let me know the reason you didn't implemented this check so I know why. I'm not saying that I'm right but it's good for my knowledge to understand the reason when I'm wrong: I would also allow people that have both 'moodle/role:manage' + 'moodle/role:assign' as they can deduce what capability a user has.
          Hide
          Yang Yang added a comment - - edited

          Hi Jerome

          My initial thought was that for users who have 'moodle/role:manage' + 'moodle/role:assign' capabilities would normally have "moodle/role:review" capability already, e.g. manager. Also this new function calls get_enrolled_users(withcapability) which requires 'moodle/role:review'. So if someone has only 'moodle/role:manage' and 'moodle/role:assign' capabilities, it would not be able to pass the capability check in get_enrolled_users. I am not entirely sure about this but it seems to be true based on the code

          ....in get_enrolled_users( .......

          if ($withcapability)

          { require_capability('moodle/role:review', $coursecontext); }


          .....

          Please let me know what you think.
          Cheers
          Yang

          Show
          Yang Yang added a comment - - edited Hi Jerome My initial thought was that for users who have 'moodle/role:manage' + 'moodle/role:assign' capabilities would normally have "moodle/role:review" capability already, e.g. manager. Also this new function calls get_enrolled_users(withcapability) which requires 'moodle/role:review'. So if someone has only 'moodle/role:manage' and 'moodle/role:assign' capabilities, it would not be able to pass the capability check in get_enrolled_users. I am not entirely sure about this but it seems to be true based on the code ....in get_enrolled_users( ....... if ($withcapability) { require_capability('moodle/role:review', $coursecontext); } ..... Please let me know what you think. Cheers Yang
          Hide
          Jérôme Mouneyrac added a comment -

          yes it's a good reason, I forgot about get_enrolled_users doing a capability check.

          I'm waiting for some senior dev to review http://docs.moodle.org/dev/Errors_handling_in_web_services before submitting to integration.

          Show
          Jérôme Mouneyrac added a comment - yes it's a good reason, I forgot about get_enrolled_users doing a capability check. I'm waiting for some senior dev to review http://docs.moodle.org/dev/Errors_handling_in_web_services before submitting to integration.
          Hide
          Aparup Banerjee added a comment -

          just quickly attaching smurf.xml code checking result.
          I glanced and saw some issues with tab, whitespace and braces.

          Show
          Aparup Banerjee added a comment - just quickly attaching smurf.xml code checking result. I glanced and saw some issues with tab, whitespace and braces.
          Hide
          Aparup Banerjee added a comment -

          reopening:

          • code needs a whitespaces and Tab usages checked.
          • "Errors_handling_in_web_services" - warning formats etc still needs agreement Jerome mentioned.

          seems fine otherwise.

          Show
          Aparup Banerjee added a comment - reopening: code needs a whitespaces and Tab usages checked. "Errors_handling_in_web_services" - warning formats etc still needs agreement Jerome mentioned. seems fine otherwise.
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Yang,
          can you have a look to http://docs.moodle.org/dev/Errors_handling_in_web_services for dealing with exception and warnings. Eloy and I agreed on it. You can adapt your code following it.

          You can also put the warnings() function into lib/externallib.php (the one from http://docs.moodle.org/dev/Errors_handling_in_web_services)? It will avoid to create it in all other external class.

          Thanks.

          Show
          Jérôme Mouneyrac added a comment - Hi Yang, can you have a look to http://docs.moodle.org/dev/Errors_handling_in_web_services for dealing with exception and warnings. Eloy and I agreed on it. You can adapt your code following it. You can also put the warnings() function into lib/externallib.php (the one from http://docs.moodle.org/dev/Errors_handling_in_web_services)? It will avoid to create it in all other external class. Thanks.
          Hide
          Yang Yang added a comment -

          Hi Aparup and Jerome

          I have made changes according to your suggestions. Also whitespaces and tab usages were checked.

          Thanks
          Yang

          Show
          Yang Yang added a comment - Hi Aparup and Jerome I have made changes according to your suggestions. Also whitespaces and tab usages were checked. Thanks Yang
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Yang, I decided to create a separate issue for the external warnings. I cherry-picked your commit and recommit marking you as the author.
          The reason to create a different issue is that we already talked about it with Eloy so it will be integrated faster than this entire function.

          Concerning this web service function, I'll add a commit with the required change before submitting. Note that from http://docs.moodle.org/dev/Errors_handling_in_web_services when something is skipped we throw an exception. I'll do these changes too.
          Cheers,
          Jerome

          Show
          Jérôme Mouneyrac added a comment - Thanks Yang, I decided to create a separate issue for the external warnings. I cherry-picked your commit and recommit marking you as the author. The reason to create a different issue is that we already talked about it with Eloy so it will be integrated faster than this entire function. Concerning this web service function, I'll add a commit with the required change before submitting. Note that from http://docs.moodle.org/dev/Errors_handling_in_web_services when something is skipped we throw an exception. I'll do these changes too. Cheers, Jerome
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Yang,
          I started to do some changes but I noticed some missing capability checks. So I pushed my changes on my Github rep. You can retrieve it and keep going: https://github.com/mouneyrac/moodle/tree/MDL-31859
          Note that I included your commit without the warning code as it has been integrated in a different issue: MDL-32998

          To do:

          • run the moodlecheck/codechecker on your modified file to check if you code match Moodle standard.
          • I still recommend to add 'moodle/role:manage' + 'moodle/role:assign' . It would be one chance less to go through the exception (and it does respect the fact that someone if these capabilities can deduce the result search from the web interface).
          • I switched the implemented warnings for exceptions: see this web service errors doc.
          • except if $USER is admin or the returned user, no one can see firstname/lastname. You need to implement some checks.
          • as not much people can see lastname/firstname I added fullname, I thought it could be useful.
          • other checks are missing for email, username,... Have a look at MDL-29938 on how to checks these fields.
          • missing version.php bump
          • missing web service function declaration in services.php
          • I added correct VALUE_OPTIONAL to the return info as we will need them once we have the emai/username/firstname... checks.
          • $users['users'] = $user; should be in the if condition, just after the foreach so you don't need $user = array ();
          • currently the function retrieves the enrolled user by capability. It implies that we also need to check that the user have the capability to search enrolled users. I would add:
          if ($courseid == SITEID) {
              require_capability('moodle/site:viewparticipants', $context);
          } else {
              require_capability('moodle/course:viewparticipants', $context);
          }
          
          • regarding what this function currently does, it seems more to be an enrol function. I would call it core_enrol_get_enrolled_users_with_capability() and I'll move it in /enrol/externallib.php.
          • looking at get_enrolled_users I also notice you may want to add a limitfrom/limitnumber to avoid timeout on massive site like Moodle.org.
          • I think that letting the client dev decide of the returned fields could be nice. Calling user_get_user_details() will definitively remove the nightmare of capability checking for email/username/... as this function should be doing that.

          OR

          • At the end if you want high chance of success to be integrated I would copy the get_enrolled_users and adapt it to support multiple capabilities/courseid. PS: it could be also an occasion to fix some issue you could discover on get_enrolled_users()

          Let me know if any of these points are not clear. Thank you.

          Show
          Jérôme Mouneyrac added a comment - Hi Yang, I started to do some changes but I noticed some missing capability checks. So I pushed my changes on my Github rep. You can retrieve it and keep going: https://github.com/mouneyrac/moodle/tree/MDL-31859 Note that I included your commit without the warning code as it has been integrated in a different issue: MDL-32998 To do: run the moodlecheck / codechecker on your modified file to check if you code match Moodle standard. I still recommend to add 'moodle/role:manage' + 'moodle/role:assign' . It would be one chance less to go through the exception (and it does respect the fact that someone if these capabilities can deduce the result search from the web interface). I switched the implemented warnings for exceptions: see this web service errors doc . except if $USER is admin or the returned user, no one can see firstname/lastname. You need to implement some checks. as not much people can see lastname/firstname I added fullname, I thought it could be useful. other checks are missing for email, username,... Have a look at MDL-29938 on how to checks these fields. missing version.php bump missing web service function declaration in services.php I added correct VALUE_OPTIONAL to the return info as we will need them once we have the emai/username/firstname... checks. $users ['users'] = $user; should be in the if condition, just after the foreach so you don't need $user = array (); currently the function retrieves the enrolled user by capability. It implies that we also need to check that the user have the capability to search enrolled users. I would add: if ($courseid == SITEID) { require_capability('moodle/site:viewparticipants', $context); } else { require_capability('moodle/course:viewparticipants', $context); } regarding what this function currently does, it seems more to be an enrol function. I would call it core_enrol_get_enrolled_users_with_capability() and I'll move it in /enrol/externallib.php. looking at get_enrolled_users I also notice you may want to add a limitfrom/limitnumber to avoid timeout on massive site like Moodle.org. I think that letting the client dev decide of the returned fields could be nice. Calling user_get_user_details() will definitively remove the nightmare of capability checking for email/username/... as this function should be doing that. OR At the end if you want high chance of success to be integrated I would copy the get_enrolled_users and adapt it to support multiple capabilities/courseid. PS: it could be also an occasion to fix some issue you could discover on get_enrolled_users() Let me know if any of these points are not clear. Thank you.
          Hide
          Yang Yang added a comment - - edited

          Hi Jerome

          For the Lightwork, We would need teacher and non-editing to be able to call the function to retrive a list of enrolled students, teachers and non-editing teachers. In Lightwork, this function will be called by teacher and non-editing rather than manager or admin.

          There are two issues with capability check right now.

          First of all, by default, teacher and non-editing teacher can only be assigned to course and activity module, so checking capability of ('moodle/role:review', $context) in system level would disallow them calling the function. Should that be checked in course level? E.g., for each $course

          {require_capability('moodle/role:review', $coursecontext);....}

          ?

          Secondly, we need teacher and non-editing teacher to be able to see users' firstname and lastname. So it would not work for us if only admin and returned user can see firstname/lastname.

          Thanks and please let me know what you think.
          Yang

          Show
          Yang Yang added a comment - - edited Hi Jerome For the Lightwork, We would need teacher and non-editing to be able to call the function to retrive a list of enrolled students, teachers and non-editing teachers. In Lightwork, this function will be called by teacher and non-editing rather than manager or admin. There are two issues with capability check right now. First of all, by default, teacher and non-editing teacher can only be assigned to course and activity module, so checking capability of ('moodle/role:review', $context) in system level would disallow them calling the function. Should that be checked in course level? E.g., for each $course {require_capability('moodle/role:review', $coursecontext);....} ? Secondly, we need teacher and non-editing teacher to be able to see users' firstname and lastname. So it would not work for us if only admin and returned user can see firstname/lastname. Thanks and please let me know what you think. Yang
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Yang,
          Moodle core web service functions need to match 100% the capabilities. In Moodle, all the checks are implemented against capability/context. There is no code logic checks implemented against role. The reason being that a role is just a set of capabilities and a role can be edited/deleted. For this reason you must not think "role" but "capabilities".

          If you want to create functions that do not respect the implemented capability checks in Moodle, you'll need to write them in your own plugin. Let me know if it's not clear, or if it is clear let me know what you decide to do next with this issue.

          Show
          Jérôme Mouneyrac added a comment - Hi Yang, Moodle core web service functions need to match 100% the capabilities. In Moodle, all the checks are implemented against capability/context. There is no code logic checks implemented against role. The reason being that a role is just a set of capabilities and a role can be edited/deleted. For this reason you must not think "role" but "capabilities". If you want to create functions that do not respect the implemented capability checks in Moodle, you'll need to write them in your own plugin. Let me know if it's not clear, or if it is clear let me know what you decide to do next with this issue.
          Hide
          Yang Yang added a comment - - edited

          Hi Jerome

          I am sorry for the confusion. Yes, we are dealing with capabilities and I do agree to respect the capability checks in Moodle. But I am not very clear about having capability check of 'moodle/role:review' in system level? Our lecturers would be using this function to get students and teachers in the courses they teach, we would not want to assign "moodle/role:review" capability at the system level to all the lecturers.

          Thanks
          Yang

          Show
          Yang Yang added a comment - - edited Hi Jerome I am sorry for the confusion. Yes, we are dealing with capabilities and I do agree to respect the capability checks in Moodle. But I am not very clear about having capability check of 'moodle/role:review' in system level? Our lecturers would be using this function to get students and teachers in the courses they teach, we would not want to assign "moodle/role:review" capability at the system level to all the lecturers. Thanks Yang
          Hide
          Martin Dougiamas added a comment -

          Sadly it's looking like this won't make 2.3 unless all these things are resolved very quickly ...

          Show
          Martin Dougiamas added a comment - Sadly it's looking like this won't make 2.3 unless all these things are resolved very quickly ...
          Hide
          Paul Charsley added a comment -

          Hi Martin, Jerome,

          I've read through the previous comments.

          Our main concern is that we need this web service to be used by teachers using Lightwork so that the details of the students, Markers and Marking Managers on their courses can be displayed for them in Lightwork.

          Markers and Marking Managers are a new concept and will be identified by 2 new capabilities which we wish to introduce with the marking management functionality now planned for Moodle 2.4.

          Jerome, from your comments it looks like you wish to only allow admin users to be able to view user details such as firstname and lastname. Is this correct? For example, please can you explain what you mean by:

          except if $USER is admin or the returned user, no one can see firstname/lastname. You need to implement some checks.

          Thanks, Paul

          Show
          Paul Charsley added a comment - Hi Martin, Jerome, I've read through the previous comments. Our main concern is that we need this web service to be used by teachers using Lightwork so that the details of the students, Markers and Marking Managers on their courses can be displayed for them in Lightwork. Markers and Marking Managers are a new concept and will be identified by 2 new capabilities which we wish to introduce with the marking management functionality now planned for Moodle 2.4. Jerome, from your comments it looks like you wish to only allow admin users to be able to view user details such as firstname and lastname. Is this correct? For example, please can you explain what you mean by: except if $USER is admin or the returned user, no one can see firstname/lastname. You need to implement some checks. Thanks, Paul
          Hide
          Yang Yang added a comment - - edited

          Hi Jerome

          I have created a new function get_enrolled_users_with_capability that supports multiple capabilities/courseid. It has all the capability checks that are implemented in the get_enrolled_users.

          Thanks
          Yang

          Show
          Yang Yang added a comment - - edited Hi Jerome I have created a new function get_enrolled_users_with_capability that supports multiple capabilities/courseid. It has all the capability checks that are implemented in the get_enrolled_users. Thanks Yang
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Paul,
          from what I've been told firstname/lastname are not visible to anyone except admin in Moodle (e.g. firstname/lastname are not searchable/displayed, only fullname should be searchable/displayed). So until I discover that a feature allow to search/display firstname/lastname, and that the feature is not a design error, I'll be mentioning to not search on firstname/lastname in peer-reviews.

          Show
          Jérôme Mouneyrac added a comment - Hi Paul, from what I've been told firstname/lastname are not visible to anyone except admin in Moodle (e.g. firstname/lastname are not searchable/displayed, only fullname should be searchable/displayed). So until I discover that a feature allow to search/display firstname/lastname, and that the feature is not a design error, I'll be mentioning to not search on firstname/lastname in peer-reviews.
          Hide
          Paul Charsley added a comment -

          Hi Jerome,

          I have to admit to still being puzzled since in the assignment submissions page teachers and non-editing teachers are able to view and sort by firstname and lastname. Is there a Moodle page that lists these firstname/lastname visibility rules that you describe?

          Anyway, in order that this does not become a road block then we will add a check that only populates the lastname and firstname if the user is admin. We will modify our Lightwork client so that it only uses fullname.

          Will this be acceptable?

          Cheers, Paul

          Show
          Paul Charsley added a comment - Hi Jerome, I have to admit to still being puzzled since in the assignment submissions page teachers and non-editing teachers are able to view and sort by firstname and lastname. Is there a Moodle page that lists these firstname/lastname visibility rules that you describe? Anyway, in order that this does not become a road block then we will add a check that only populates the lastname and firstname if the user is admin. We will modify our Lightwork client so that it only uses fullname. Will this be acceptable? Cheers, Paul
          Hide
          Martin Dougiamas added a comment -

          Hi Paul.

          The confusion is that there is a site setting called "fullnamedisplay" and a function called fullname() which uses it. If this is changed then the new assignment does indeed only show what it should (the old assignment didn't and that was a bug).

          Some sites do not want to reveal firstname and lastname to teacher, students etc. Some sites hack this function to display only the idnumber or username, for example (I have seen this done in muslim countries for anonymity of women, or on sites used for counselling).

          There is however a capability called "moodle/site:viewfullnames" which could be used to decide whether to ADDITIONALLY return those two fields (making a total of three: fullname, firstname and lastname). it's not exact but I think it's close enough in intent.

          A site could then easily tweak roles to make it work as they want it to.

          Is this sufficient?

          Life's not easy eh?

          Show
          Martin Dougiamas added a comment - Hi Paul. The confusion is that there is a site setting called "fullnamedisplay" and a function called fullname() which uses it. If this is changed then the new assignment does indeed only show what it should (the old assignment didn't and that was a bug). Some sites do not want to reveal firstname and lastname to teacher, students etc. Some sites hack this function to display only the idnumber or username, for example (I have seen this done in muslim countries for anonymity of women, or on sites used for counselling). There is however a capability called "moodle/site:viewfullnames" which could be used to decide whether to ADDITIONALLY return those two fields (making a total of three: fullname, firstname and lastname). it's not exact but I think it's close enough in intent. A site could then easily tweak roles to make it work as they want it to. Is this sufficient? Life's not easy eh?
          Hide
          Jérôme Mouneyrac added a comment -

          So at the end we are good because the function call user_get_user_details() which does check against moodle/site:viewfullnames. Thanks for explaining Martin. Searching/sorting on firstname/lastname will need this capability too (not applicable to this function as it doesn't search/sort by firstname/lastname).

          Show
          Jérôme Mouneyrac added a comment - So at the end we are good because the function call user_get_user_details() which does check against moodle/site:viewfullnames. Thanks for explaining Martin. Searching/sorting on firstname/lastname will need this capability too (not applicable to this function as it doesn't search/sort by firstname/lastname).
          Hide
          Paul Charsley added a comment -

          Hi Martin, Jerome,
          Thanks very much for the explanation. Now I understand!
          Paul

          Show
          Paul Charsley added a comment - Hi Martin, Jerome, Thanks very much for the explanation. Now I understand! Paul
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Yang,
          my peer-review:

          • $USER is not used
          • new external_function_parameters() is returned by get_enrolled_users_with_capability_parameters not get_enrolled_users_with_capability_returns
          • reformat the indentation of the returns value to be a bit more readable (also reformat the indentation inside the main function, there are few lines indented)
          • from what I understand, you are writing this function to perform faster than multiple calls of the original function which retrieves enrolled users with one capability. I think you don't need the included foreach. Your request should do one query per course and not 'n' capabilities query per course. Looking at get_enrolled_sql, maybe it would be possible to change withcapability to withcapabilities and accept array and tweak the function to search on multiple capabilities.
          • From reading the code you are executing an OR operation. This should be mentioned in the description/phpdoc. In fact before reading the code I thought that this function returned the users who had all the capabilities. So it's very important to write it down for the devs. I have a question, could some devs be interested by the AND operation? It could be good to offer support for both operations if yes (it's a question, I don't know if useful).
          • assigning $withcapability = $capability; is not necessary, you can use $capability straight away

          Cheers,
          Jerome

          Show
          Jérôme Mouneyrac added a comment - Hi Yang, my peer-review: $USER is not used new external_function_parameters() is returned by get_enrolled_users_with_capability_parameters not get_enrolled_users_with_capability_returns reformat the indentation of the returns value to be a bit more readable (also reformat the indentation inside the main function, there are few lines indented) from what I understand, you are writing this function to perform faster than multiple calls of the original function which retrieves enrolled users with one capability. I think you don't need the included foreach. Your request should do one query per course and not 'n' capabilities query per course. Looking at get_enrolled_sql, maybe it would be possible to change withcapability to withcapabilities and accept array and tweak the function to search on multiple capabilities. From reading the code you are executing an OR operation. This should be mentioned in the description/phpdoc. In fact before reading the code I thought that this function returned the users who had all the capabilities. So it's very important to write it down for the devs. I have a question, could some devs be interested by the AND operation? It could be good to offer support for both operations if yes (it's a question, I don't know if useful). assigning $withcapability = $capability; is not necessary, you can use $capability straight away Cheers, Jerome
          Hide
          Jérôme Mouneyrac added a comment -

          Hi (again ) Yang, we are about to integrate the text format code changes in 2.3: http://docs.moodle.org/dev/How_to_contribute_a_web_service_function_to_core#Create_a_tracker_issue
          Can you apply them when it gets integrated: MDL-32581
          Thank you.

          Show
          Jérôme Mouneyrac added a comment - Hi (again ) Yang, we are about to integrate the text format code changes in 2.3: http://docs.moodle.org/dev/How_to_contribute_a_web_service_function_to_core#Create_a_tracker_issue Can you apply them when it gets integrated: MDL-32581 Thank you.
          Hide
          Jérôme Mouneyrac added a comment -

          I also forgot: don't forget to bump the version number in version.php. Otherwise it makes the tester wonder why it doesn't work. (web service functions are updated/installed/removed automatically every new version). Thanks.

          Show
          Jérôme Mouneyrac added a comment - I also forgot: don't forget to bump the version number in version.php. Otherwise it makes the tester wonder why it doesn't work. (web service functions are updated/installed/removed automatically every new version). Thanks.
          Hide
          Yang Yang added a comment -

          Hi Jerome

          Could you please clarify if you would like me to change the existing get_enrolled_sql() function to allow multiple capabilities or just borrowing code form the function and make modification.

          I will add description about the OR operation. I would suggest that maybe we could add the support of AND operation in the next version if other devs found it is useful to have. What do you think?

          Thanks.
          Yang

          Show
          Yang Yang added a comment - Hi Jerome Could you please clarify if you would like me to change the existing get_enrolled_sql() function to allow multiple capabilities or just borrowing code form the function and make modification. I will add description about the OR operation. I would suggest that maybe we could add the support of AND operation in the next version if other devs found it is useful to have. What do you think? Thanks. Yang
          Hide
          Yang Yang added a comment -

          Hi Jerome,

          With reference to your comment:

          I think you don't need the included foreach. Your request should do one query per course and not 'n' capabilities query per course. Looking at get_enrolled_sql, maybe it would be possible to change withcapability to withcapabilities and accept array and tweak the function to search on multiple capabilities

          I have investigated your idea. Unfortunately this is not practical since even after tweaking it as you suggest, we would then need to perform further queries to identify the capability/capabilities of each user. I think it would be better to continue using the existing function as it is.

          If during testing, performance is found to be an issue, we can investigate the creation of a new function.

          Regards,
          Yang

          Show
          Yang Yang added a comment - Hi Jerome, With reference to your comment: I think you don't need the included foreach. Your request should do one query per course and not 'n' capabilities query per course. Looking at get_enrolled_sql, maybe it would be possible to change withcapability to withcapabilities and accept array and tweak the function to search on multiple capabilities I have investigated your idea. Unfortunately this is not practical since even after tweaking it as you suggest, we would then need to perform further queries to identify the capability/capabilities of each user. I think it would be better to continue using the existing function as it is. If during testing, performance is found to be an issue, we can investigate the creation of a new function. Regards, Yang
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Yang,
          can you add the PHPunit test. PHPunit test is now requirement to send a web service function to integration. An example can be found there: https://github.com/moodle/moodle/blob/master/course/tests/externallib_test.php (you can look at get_categories as example).
          Cheers,
          Jerome

          Show
          Jérôme Mouneyrac added a comment - Hi Yang, can you add the PHPunit test. PHPunit test is now requirement to send a web service function to integration. An example can be found there: https://github.com/moodle/moodle/blob/master/course/tests/externallib_test.php (you can look at get_categories as example). Cheers, Jerome
          Hide
          Paul Charsley added a comment -

          Hi Jerome,

          PHPunit test added.

          Cheers, Paul

          Show
          Paul Charsley added a comment - Hi Jerome, PHPunit test added. Cheers, Paul
          Hide
          Paul Charsley added a comment -

          Hi Jerome,
          I added the PHP unit test over a month ago. Can this issue be assigned to a peer reviewer now?
          Cheers, Paul

          Show
          Paul Charsley added a comment - Hi Jerome, I added the PHP unit test over a month ago. Can this issue be assigned to a peer reviewer now? Cheers, Paul
          Hide
          Paul Charsley added a comment -

          Hi Jerome, this issue is still waiting to go to integration. I added the php unit test on 27th July. Please can it be progressed.
          Cheers, Paul

          Show
          Paul Charsley added a comment - Hi Jerome, this issue is still waiting to go to integration. I added the php unit test on 27th July. Please can it be progressed. Cheers, Paul
          Hide
          Jérôme Mouneyrac added a comment -

          Hello Paul,
          I read my last peer-review and the points don't seem to have been fixed (http://tracker.moodle.org/browse/MDL-31859?focusedCommentId=160977&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-160977). Can you fix them, thanks.
          Jerome

          Show
          Jérôme Mouneyrac added a comment - Hello Paul, I read my last peer-review and the points don't seem to have been fixed ( http://tracker.moodle.org/browse/MDL-31859?focusedCommentId=160977&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-160977 ). Can you fix them, thanks. Jerome
          Hide
          Paul Charsley added a comment -

          Hi Jerome,

          Sorry, I'm not sure how that got missed. I've now addressed your points and also removed more deprecated and unneeded code. I've updated the comments to make it clear that we are returning lists of users for each course/capability combination specified.

          Cheers, Paul

          Show
          Paul Charsley added a comment - Hi Jerome, Sorry, I'm not sure how that got missed. I've now addressed your points and also removed more deprecated and unneeded code. I've updated the comments to make it clear that we are returning lists of users for each course/capability combination specified. Cheers, Paul
          Hide
          Jérôme Mouneyrac added a comment -

          thanks Paul. Two points to fix and I send it to integration:

          • get_enrolled_users_with_capability_returns() still return external_function_parameters(). Use external_single_structure() instead of external_function_parameters(). external_function_parameters() has been created to show developers that xxx_parameters() functions should always start with a single structure. But in fact you should even remove the following:
            new external_function_parameters( array {
                         'userlist'   =>
            

            I think you can return the array straight away. The return value doesn't have to be a single structure like the parameters.

          • I would change the services.php description for the text you wrote in the phpdocs: "For each course and capability specified, return a list of the users that are enrolled in the course and have that capability."

          cheers,
          Jerome

          Show
          Jérôme Mouneyrac added a comment - thanks Paul. Two points to fix and I send it to integration: get_enrolled_users_with_capability_returns() still return external_function_parameters(). Use external_single_structure() instead of external_function_parameters(). external_function_parameters() has been created to show developers that xxx_parameters() functions should always start with a single structure. But in fact you should even remove the following: new external_function_parameters( array { 'userlist' => I think you can return the array straight away. The return value doesn't have to be a single structure like the parameters. I would change the services.php description for the text you wrote in the phpdocs: "For each course and capability specified, return a list of the users that are enrolled in the course and have that capability." cheers, Jerome
          Hide
          Paul Charsley added a comment -

          Hi Jerome,
          Well spotted! There's nothing worse than having extra unneeded wrappers. I've fixed those 2 points and removed some merge commits.

          Cheers, Paul

          Show
          Paul Charsley added a comment - Hi Jerome, Well spotted! There's nothing worse than having extra unneeded wrappers. I've fixed those 2 points and removed some merge commits. Cheers, Paul
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks, submitting.

          Show
          Jérôme Mouneyrac added a comment - Thanks, submitting.
          Hide
          Dan Poltawski 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
          Dan Poltawski 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
          Paul Charsley added a comment -

          rebase done

          Show
          Paul Charsley added a comment - rebase done
          Hide
          Dan Poltawski added a comment -

          I have some concerns that get_enrolled_users_with_capability_returns() will quickly get out of date, but i'm ignoring that problem for now.

          Show
          Dan Poltawski added a comment - I have some concerns that get_enrolled_users_with_capability_returns() will quickly get out of date, but i'm ignoring that problem for now.
          Hide
          Dan Poltawski added a comment -

          I'd also like to see more extensive unit tests, e.g. testing capabilties missing for private user data etc. But this can come later.

          Show
          Dan Poltawski added a comment - I'd also like to see more extensive unit tests, e.g. testing capabilties missing for private user data etc. But this can come later.
          Hide
          Dan Poltawski added a comment -

          I've integrated this now.

          Thanks guys.

          Show
          Dan Poltawski added a comment - I've integrated this now. Thanks guys.
          Hide
          Dan Poltawski added a comment -

          phpunit success

          Show
          Dan Poltawski added a comment - phpunit success
          Hide
          Dan Poltawski added a comment -

          Congratulations, you've done it!

          Nf n erjneq sbe fhpprfshy vagrtengvba vagb guvf jrrxf eryrnfr, V pna abj qvfpybfr gb lbh gur rkvfgnapr bs shapgvba fge_ebg13(), gb tb va lbhe gbbyxvg nybat jvgu uggc://cuc.arg/znahny/ra/shapgvba.tmtrgff.cuc

          Cyrnfr qb abg nyybj guvf vasbezngvba gb cnff shegure.

          Show
          Dan Poltawski added a comment - Congratulations, you've done it! Nf n erjneq sbe fhpprfshy vagrtengvba vagb guvf jrrxf eryrnfr, V pna abj qvfpybfr gb lbh gur rkvfgnapr bs shapgvba fge_ebg13(), gb tb va lbhe gbbyxvg nybat jvgu uggc://cuc.arg/znahny/ra/shapgvba.tmtrgff.cuc Cyrnfr qb abg nyybj guvf vasbezngvba gb cnff shegure.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: