Moodle
  1. Moodle
  2. MDL-29476

moodle_enrol_get_users_courses should return number of participants, moodle_user_get_users_by_courseid should limit the fields to be returned

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.1.1
    • Fix Version/s: 2.2
    • Component/s: Web Services
    • Labels:
    • Testing Instructions:
      Hide

      You will have to download web service client for testing: https://github.com/moodlehq/sample-ws-clients

      1. Request moodle_enrol_get_users_courses webservice, there will be a field called enrolledusercount which indicates the number of enrolled users in this course
      2. Request moodle_user_get_users_by_courseid with userfields option name , the option value will be the list of userfields which separated by comma, the return value will be the fields specified in your reuqest plus userid and user's fullname

      Show
      You will have to download web service client for testing: https://github.com/moodlehq/sample-ws-clients 1. Request moodle_enrol_get_users_courses webservice, there will be a field called enrolledusercount which indicates the number of enrolled users in this course 2. Request moodle_user_get_users_by_courseid with userfields option name , the option value will be the list of userfields which separated by comma, the return value will be the fields specified in your reuqest plus userid and user's fullname
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:
      dev_MDL-29476_ws_participants_master
    • Rank:
      19054

      Description

      moodle_enrol_get_users_courses should return number of participants, moodle_user_get_users_by_courseid should limit the fields to be returned

      This is intended to solve the performance issue related to MDL-29347

        Issue Links

          Activity

          Hide
          Jérôme Mouneyrac added a comment -

          Good to me.

          Few trivial things to change before submitting:

          • remove debug() calls from user_get_user_details(...)
          • add little phpdoc comment: //removing not requested details
          • Put (id, fullname, profileimageurlsmall, profileimageurl) in userfields[] before asking for the details. Otherwise they will get removed by user_get_user_details(), and as they are not VALUE_OPTIONAL in the external description, the return validation process will throw an exception on them.
          Show
          Jérôme Mouneyrac added a comment - Good to me. Few trivial things to change before submitting: remove debug() calls from user_get_user_details(...) add little phpdoc comment: //removing not requested details Put (id, fullname, profileimageurlsmall, profileimageurl) in userfields[] before asking for the details. Otherwise they will get removed by user_get_user_details(), and as they are not VALUE_OPTIONAL in the external description, the return validation process will throw an exception on them.
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Sam Hemelryk added a comment -

          Hi guys,

          I've just been going through this now and have noted the following. Needs more work presently sorry.

          • user_get_user_details the default result has changed from returning all user details to returning just the selected fields.
            This is an interesting one - are you sure the default result from this function should be changed? This change is not at all backwards compatible, it is just a core change so perhaps it is permissible.
            Certainly the solution that you have at the moment can, and needs to be improved. Presently if no user fields are provided only id, and fullname are returned. However the function still goes through the process of filling the other fields many of which require DB interaction and lots of processing and then unsetting them if they are not wanted.
            I think the following things need to happen:
            1. Decide whether we change the default return from all details to just id, fullname.
            2. Change the function definition so that $userfields is "array $userfields = array()" this will fix the error you receive if you don't specify it (because it is assumed to be an array but defaults to null).
            3. At the start of the function set up $userfields so that it contains the fields we want to return. If we change the default then make sure it is a non-empty array OR make it an array containing id and full name. If we choose to keep the current default then make it an array containing all possible fields.
            4. Only fill $userdetails with fields within $userfields to avoid DB queries + processing to get information that we won't use.
          • I think get_enrolled_users should validate the $userfields passed in against a list of permitted fields that can be returned. Presently if someone requests fields that don't exist the user object will just be sent back without those fields. Personally I think that we should be throwing an exception if someone requests a field not in the permitted list. This will aid both security and debugging.
          • get_users_courses
            1. First up the fetching of the number of enrolled users should be moved after the check to see if the user is the current user OR has the view participants capability so thats its only executed if needed.
            2. count_records_sql expects an SQL statement like "SELECT COUNT …." the result of get_enrolled_sql is not suitable for use within count_records_sql without wrapping it in more SQL.
              Note: It still returns the correct result but it displays an error as well.. not sure how WS deals with that presently I assume that perhaps it doesn't recognise the error if you guys aren't seeing anything come back and should probably have a bug created for it so that we can ensure WS doesn't ignore DB errors... although I'm not even sure how that error is being displayed/thrown.
            3. I personally think that 'numberorparticipants' should be renamed to 'enrolledusercount' or something like that seeing as that is technically what it is returning.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've just been going through this now and have noted the following. Needs more work presently sorry. user_get_user_details the default result has changed from returning all user details to returning just the selected fields. This is an interesting one - are you sure the default result from this function should be changed? This change is not at all backwards compatible, it is just a core change so perhaps it is permissible. Certainly the solution that you have at the moment can, and needs to be improved. Presently if no user fields are provided only id, and fullname are returned. However the function still goes through the process of filling the other fields many of which require DB interaction and lots of processing and then unsetting them if they are not wanted. I think the following things need to happen: Decide whether we change the default return from all details to just id, fullname. Change the function definition so that $userfields is "array $userfields = array()" this will fix the error you receive if you don't specify it (because it is assumed to be an array but defaults to null). At the start of the function set up $userfields so that it contains the fields we want to return. If we change the default then make sure it is a non-empty array OR make it an array containing id and full name. If we choose to keep the current default then make it an array containing all possible fields. Only fill $userdetails with fields within $userfields to avoid DB queries + processing to get information that we won't use. I think get_enrolled_users should validate the $userfields passed in against a list of permitted fields that can be returned. Presently if someone requests fields that don't exist the user object will just be sent back without those fields. Personally I think that we should be throwing an exception if someone requests a field not in the permitted list. This will aid both security and debugging. get_users_courses First up the fetching of the number of enrolled users should be moved after the check to see if the user is the current user OR has the view participants capability so thats its only executed if needed. count_records_sql expects an SQL statement like "SELECT COUNT …." the result of get_enrolled_sql is not suitable for use within count_records_sql without wrapping it in more SQL. Note: It still returns the correct result but it displays an error as well.. not sure how WS deals with that presently I assume that perhaps it doesn't recognise the error if you guys aren't seeing anything come back and should probably have a bug created for it so that we can ensure WS doesn't ignore DB errors... although I'm not even sure how that error is being displayed/thrown. I personally think that 'numberorparticipants' should be renamed to 'enrolledusercount' or something like that seeing as that is technically what it is returning. Cheers Sam
          Hide
          Dongsheng Cai added a comment -

          Hello Sam

          Thanks for taking time to review this, I fixed following problem according to your comment:

          1. user_get_user_details accepts array $userfields only
          2. Define $defaultuserfields in user_get_user_details, if passed empty userfields, we use defaultuserfields, $id and $fullname are required all time
          3. Fill return values based on userfields to reduce db operations
          4. If fields don't exist, user_get_user_details will throw exception, I did this in user_get_user_details to avoid maintain another default user fields array
          5. Fixed `enrolledusercount` SQL, use subquery to include the enrolled sql.
          Show
          Dongsheng Cai added a comment - Hello Sam Thanks for taking time to review this, I fixed following problem according to your comment: user_get_user_details accepts array $userfields only Define $defaultuserfields in user_get_user_details, if passed empty userfields, we use defaultuserfields, $id and $fullname are required all time Fill return values based on userfields to reduce db operations If fields don't exist, user_get_user_details will throw exception, I did this in user_get_user_details to avoid maintain another default user fields array Fixed `enrolledusercount` SQL, use subquery to include the enrolled sql.
          Hide
          Dongsheng Cai added a comment -

          Sam

          If no userfields option is provided, this web service return the same results as before, it only makes differences if web service client asks for specific fields.

          Show
          Dongsheng Cai added a comment - Sam If no userfields option is provided, this web service return the same results as before, it only makes differences if web service client asks for specific fields.
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Dongsheng, it seems good to me.

          Show
          Jérôme Mouneyrac added a comment - Thanks Dongsheng, it seems good to me.
          Hide
          Eloy Lafuente (stronk7) 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

          PS: Note this is the last message of this type that you will receive along the whole November month, because we are running continuous integration this weeks while QA tests for release of Moodle 2.2 (Dec 1st) are being performed.

          Show
          Eloy Lafuente (stronk7) 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 PS: Note this is the last message of this type that you will receive along the whole November month, because we are running continuous integration this weeks while QA tests for release of Moodle 2.2 (Dec 1st) are being performed.
          Hide
          Jérôme Mouneyrac added a comment -

          Integrator: you should have a little conflict with MDL-30027 that Dongsheng nicely fixed on the fly in this issue too.

          Show
          Jérôme Mouneyrac added a comment - Integrator: you should have a little conflict with MDL-30027 that Dongsheng nicely fixed on the fly in this issue too.
          Hide
          Sam Hemelryk added a comment -

          Thanks for cleaning that up DS, this has been integrated now.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks for cleaning that up DS, this has been integrated now. Cheers Sam
          Hide
          Rajesh Taneja added a comment -

          Works Great
          Thanks for fixing this DS.

          Show
          Rajesh Taneja added a comment - Works Great Thanks for fixing this DS.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this has landed upstream, just on time for the upcoming new releases week. Thanks for it!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this has landed upstream, just on time for the upcoming new releases week. Thanks for it! Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: