Moodle
  1. Moodle
  2. MDL-36541

Modify core_group_get_groupings to optionally return its associated groups

    Details

    • Rank:
      46012

      Description

      The web services currently do not have a function that returns the groups associated with a grouping. This could be done by adding an optional parameter to the function core_group_get_groupings so that the groups associated with each requested grouping are also returned.

      For example, this modification would be useful for a web service client that wishes to determine the teams and their members in a team assignment in the new Moodle assignment module. The mdl_assign table has a "teamsubmissiongroupingid" that links to the grouping which is associated with the groups/teams.

        Issue Links

          Activity

          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Paul.
          Why does the group contains groupingid/courseid, can this value be different from the grouping info?
          Otherwise it looks good to me.

          Show
          Jérôme Mouneyrac added a comment - Thanks Paul. Why does the group contains groupingid/courseid, can this value be different from the grouping info? Otherwise it looks good to me.
          Hide
          Paul Charsley added a comment -

          Hi Jerome,

          The groupingid/courseid for the group should be the same as the grouping information. Although repeated, I thought it best to include this data since the overhead is negligible and I believe it will make the function easier and clearer to use.

          Paul

          Show
          Paul Charsley added a comment - Hi Jerome, The groupingid/courseid for the group should be the same as the grouping information. Although repeated, I thought it best to include this data since the overhead is negligible and I believe it will make the function easier and clearer to use. Paul
          Hide
          Paul Charsley added a comment -

          Rebased with latest development code.

          Jerome, can this go into integration review now?

          Show
          Paul Charsley added a comment - Rebased with latest development code. Jerome, can this go into integration review now?
          Hide
          Jérôme Mouneyrac added a comment -

          I don't have much experience using it so I believe you if you tell the overhead is negligible. Sending to integration.

          Show
          Jérôme Mouneyrac added a comment - I don't have much experience using it so I believe you if you tell the overhead is negligible. Sending to integration.
          Hide
          Damyon Wiese 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.

          Cheers!

          Show
          Damyon Wiese 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. Cheers!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sorry, I'm reopening this. While negligible / small ... I don't think it's correct to return any structure with redundant information. Otherwise, perfect.

          Show
          Eloy Lafuente (stronk7) added a comment - Sorry, I'm reopening this. While negligible / small ... I don't think it's correct to return any structure with redundant information. Otherwise, perfect.
          Hide
          Paul Charsley added a comment -

          Hi Eloy, yes I agree it's fine to remove the redundant groupingid.

          However, I am a little concerned about removing the courseid. Although its unlikely to happen, I think that the Moodle database schema for groups and groupings could allow incorrect courseids to be entered in the courseid field (possibly caused by a wayward database script).

          If that were to ever happen, clients using this web service would wrongly assume that the course ids matched with unforeseen results.

          Paul

          Show
          Paul Charsley added a comment - Hi Eloy, yes I agree it's fine to remove the redundant groupingid. However, I am a little concerned about removing the courseid. Although its unlikely to happen, I think that the Moodle database schema for groups and groupings could allow incorrect courseids to be entered in the courseid field (possibly caused by a wayward database script). If that were to ever happen, clients using this web service would wrongly assume that the course ids matched with unforeseen results. Paul
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Paul Charsley added a comment -

          Hi Jerome, Eloy,
          I've removed the redundant groupingid.

          After looking through the code and forums I think that it is best to leave the courseid for the group. Although the courseid in the groups is normally expected to be the same as in the grouping I can't see anything that checks/enforces this. If we omit the courseid in the groups we are implicitly telling the web client developer to assume that it is always the same as the grouping. I'm not sure that this is/will always be the case.

          Submitting for review

          Thanks, Paul

          Show
          Paul Charsley added a comment - Hi Jerome, Eloy, I've removed the redundant groupingid. After looking through the code and forums I think that it is best to leave the courseid for the group. Although the courseid in the groups is normally expected to be the same as in the grouping I can't see anything that checks/enforces this. If we omit the courseid in the groups we are implicitly telling the web client developer to assume that it is always the same as the grouping. I'm not sure that this is/will always be the case. Submitting for review Thanks, Paul
          Hide
          Jérôme Mouneyrac added a comment -

          I think no need to send it to peer-review each time it goes back from integration if the fix done will be easily checked by the integrator.

          Even thought we were suggesting one commit only a year ago per issue, I think the mentality changed. Personally I prefer one commit the first time, then one commit after each review. So it's easier to re-peer-review/re-integration-review.

          Sending to integration. Thanks Paul.

          Show
          Jérôme Mouneyrac added a comment - I think no need to send it to peer-review each time it goes back from integration if the fix done will be easily checked by the integrator. Even thought we were suggesting one commit only a year ago per issue, I think the mentality changed. Personally I prefer one commit the first time, then one commit after each review. So it's easier to re-peer-review/re-integration-review. Sending to integration. Thanks Paul.
          Hide
          Damyon Wiese 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.

          Thanks!

          Show
          Damyon Wiese 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. Thanks!
          Hide
          Paul Charsley added a comment -

          Rebased with latest weekly release and successfully ran PHPUnit tests

          Show
          Paul Charsley added a comment - Rebased with latest weekly release and successfully ran PHPUnit tests
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (master only), thanks!

          PS: Note that this is a change of an existing function (defaulting to old behavior), but I've added the api_changes and dev_docs_required labels because surely this change needs to be documented somewhere. I'm not 100% sure about which is the place (group/upgrade.txt?..) or if it's enough with function descriptors. Asking Jerome about the policy here.

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (master only), thanks! PS: Note that this is a change of an existing function (defaulting to old behavior), but I've added the api_changes and dev_docs_required labels because surely this change needs to be documented somewhere. I'm not 100% sure about which is the place (group/upgrade.txt?..) or if it's enough with function descriptors. Asking Jerome about the policy here.
          Hide
          David Monllaó added a comment -

          Passing as tested by unit test

          Show
          David Monllaó added a comment - Passing as tested by unit test
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Eloy, we don't have a policy about ws API change. I'll create a web service API changes doc where we can list them. Thanks for the notification.

          Show
          Jérôme Mouneyrac added a comment - Hi Eloy, we don't have a policy about ws API change. I'll create a web service API changes doc where we can list them. Thanks for the notification.
          Hide
          Jérôme Mouneyrac added a comment -

          I added http://docs.moodle.org/dev/Web_services_API_Changes
          From now every changes in the web service API should be written there by the dev.

          Show
          Jérôme Mouneyrac added a comment - I added http://docs.moodle.org/dev/Web_services_API_Changes From now every changes in the web service API should be written there by the dev.
          Hide
          Damyon Wiese added a comment -

          Thanks for your hard work - this issue has made it! Moodle is now a little bit better.

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Thanks for your hard work - this issue has made it! Moodle is now a little bit better. Regards, Damyon

            People

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

              Dates

              • Created:
                Updated:
                Resolved: