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

Modify core_group_get_groupings to optionally return its associated groups

    Details

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            jerome 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
            jerome 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
            pcharsle 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
            pcharsle 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
            pcharsle Paul Charsley added a comment -

            Rebased with latest development code.

            Jerome, can this go into integration review now?

            Show
            pcharsle Paul Charsley added a comment - Rebased with latest development code. Jerome, can this go into integration review now?
            Hide
            jerome 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
            jerome 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 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 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
            stronk7 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
            stronk7 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
            pcharsle 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
            pcharsle 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 CiBoT added a comment -

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

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            pcharsle 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
            pcharsle 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
            jerome 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
            jerome 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 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 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
            pcharsle Paul Charsley added a comment -

            Rebased with latest weekly release and successfully ran PHPUnit tests

            Show
            pcharsle Paul Charsley added a comment - Rebased with latest weekly release and successfully ran PHPUnit tests
            Hide
            stronk7 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
            stronk7 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
            dmonllao David Monllaó added a comment -

            Passing as tested by unit test

            Show
            dmonllao David Monllaó added a comment - Passing as tested by unit test
            Hide
            jerome 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
            jerome 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
            jerome 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
            jerome 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 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 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:
                  Fix Release Date:
                  14/May/13