Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-27880

moodle_user_get_course_participants_by_id

    Details

    • Type: New Feature
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.1
    • Component/s: Web Services
    • Labels:
      None

      Description

      it should behave as same as moodle_user_get_users_by_id, but use course context instead

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              dongsheng Dongsheng Cai added a comment - - edited
              capability fields
              moodle/user:viewdetails userid, fullname, insterestss, all customfields, roles
              moodle/course:viewhiddenuserfields description, descriptionformat, country, city, address, phone1, phone2, email, url, icq, skype, yahoo, aim, msn, firstaccess, lastaccess
              moodle/site:viewfullnames firstname, lastname
              moodle/user:update auth, confirmed, idnumber, lang,theme, timezone, mailformat
              moodle/site:accessallgroups groups
              admin or current user institution, department
              Show
              dongsheng Dongsheng Cai added a comment - - edited capability fields moodle/user:viewdetails userid, fullname, insterestss, all customfields , roles moodle/course:viewhiddenuserfields description, descriptionformat, country, city, address, phone1, phone2, email, url, icq, skype, yahoo, aim, msn, firstaccess, lastaccess moodle/site:viewfullnames firstname, lastname moodle/user:update auth, confirmed, idnumber, lang,theme, timezone, mailformat moodle/site:accessallgroups groups admin or current user institution, department
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Hi Dongsheng,

              I've just been looking at this now, in general it looks good
              Most of the points below relate to performance, many of them were/are in fact present in the get_users_by_id method as well I think when I pointed it out to Jerome he choose to create issues so you could if this is urgent as well just link to those issues and comment in them that this new method needs to be cleaned up as well.

              General notes:

              1. Please fix the indenting of the get_course_participants_by_id_returns, could you please also fix the indenting of get_users_by_id_returns at the same time (I asked Jerome too but obviously forgot to check again before integrating).
              2. The if (empty($user->deleted)) should check check !empty and continue, it should also be at the top of the function before any other calls. Alternativily if you switch to fetching uses manually as suggested below in the perf notes you could just check it as a database condition.

              Things that could be done to improve performance:

              1. Rather than fetching all user contexts and then all users through user_get_users_by_id you should just use a recordset to retrieve users AND their contexts at the same time (using context_instance_preload* functions).
              2. Rather than fetching the course and context for each user within foreach you should really be fetching all courses (unique($courseids)) and could be doing the same as above and preloading the contexts at the same time.
              3. When fetching the custom fields the code is fetching all the categories and foreach category fetching all fields, certainly this could be one query rather than queries within a foreach, just fetch all fields sorting by category.sortorder, then field sortorder.

              So 2 minor points + 3 performance options. All looks good to me otherwise

              Cheers
              Sam

              Show
              samhemelryk Sam Hemelryk added a comment - Hi Dongsheng, I've just been looking at this now, in general it looks good Most of the points below relate to performance, many of them were/are in fact present in the get_users_by_id method as well I think when I pointed it out to Jerome he choose to create issues so you could if this is urgent as well just link to those issues and comment in them that this new method needs to be cleaned up as well. General notes: Please fix the indenting of the get_course_participants_by_id_returns, could you please also fix the indenting of get_users_by_id_returns at the same time (I asked Jerome too but obviously forgot to check again before integrating). The if (empty($user->deleted)) should check check !empty and continue, it should also be at the top of the function before any other calls. Alternativily if you switch to fetching uses manually as suggested below in the perf notes you could just check it as a database condition. Things that could be done to improve performance: Rather than fetching all user contexts and then all users through user_get_users_by_id you should just use a recordset to retrieve users AND their contexts at the same time (using context_instance_preload* functions). Rather than fetching the course and context for each user within foreach you should really be fetching all courses (unique($courseids)) and could be doing the same as above and preloading the contexts at the same time. When fetching the custom fields the code is fetching all the categories and foreach category fetching all fields, certainly this could be one query rather than queries within a foreach, just fetch all fields sorting by category.sortorder, then field sortorder. So 2 minor points + 3 performance options. All looks good to me otherwise Cheers Sam
              Hide
              dongsheng Dongsheng Cai added a comment -

              A)

              1. Fixed indentation
              2. Added !empty() and continue check to both get_users_by_id and get_course_participants_by_id

              B)

              1. Fixed
              2. Fixed
              3. Fixed
              Show
              dongsheng Dongsheng Cai added a comment - A) Fixed indentation Added !empty() and continue check to both get_users_by_id and get_course_participants_by_id B) Fixed Fixed Fixed
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Hi DS,

              Looks great to me!

              Thanks for going to the effort of fixing up those performance issues as well (plus fixing them in the get_users_by_id), fantastic to get them out of the way while this is all fresh.

              Cheers
              Sam

              Show
              samhemelryk Sam Hemelryk added a comment - Hi DS, Looks great to me! Thanks for going to the effort of fixing up those performance issues as well (plus fixing them in the get_users_by_id), fantastic to get them out of the way while this is all fresh. Cheers Sam
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Thanks DS this has been integrated now.

              Show
              samhemelryk Sam Hemelryk added a comment - Thanks DS this has been integrated now.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              passing, nobody tested this.

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - passing, nobody tested this.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              U.P.S.T.R.E.A.M

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - U.P.S.T.R.E.A.M

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    1/Jul/11