Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Groups, Web Services
    • Labels:
    • Testing Instructions:
      Hide

      Use this client:
      https://github.com/moodlehq/sample-ws-clients/tree/master/PHP-REST

      functionname
      core_group_create_groupings

      /// PARAMETERS
      $params = array('groupings' => array(
      array(
      'courseid' => $courseid,
      'name' => 'My grouping 1'.rand(),
      'description' => '<b>Description</b>'
      ),
      array(
      'courseid' => $courseid,
      'name' => 'My second grouping'.rand(),
      'description' => '<h2></h2><p><b>HTML description</b></p>'
      )))

      core_group_update_groupings

      /// PARAMETERS
      // Change the id values
      $params = array('groupings' => array(
      array(
      'id' => 1,
      'name' => 'My grouping 1'.rand(),
      'description' => '<b>Description</b>'
      ),
      array(
      'id' => 2,
      'name' => 'My second grouping'.rand(),
      'description' => '<h2></h2><p><b>HTML description</b></p>'
      )))

      core_group_get_groupings
      // Change groupingids
      $params = array('groupingids' => array(1,2))

      core_group_get_course_groupings
      // Change courseid value

      $params = array('courseid' => 2)

      core_group_delete_groupings
      // Change groupingids

      $params = array('groupingids' => array(1,2))

      core_group_assign_grouping
      // Change groupingid and groupid to your Moodle test installation values

      $params = array('assignments' => array(array(
      'groupingid' => 1,
      'groupid' => 1
      ),
      array(
      'groupingid' => 1,
      'groupid' => 2
      ))),

      core_group_unassign_grouping
      // Change groupingid and groupid to your Moodle test installation values

      $params = array('unassignments' => array(array(
      'groupingid' => 1,
      'groupid' => 1
      ),
      array(
      'groupingid' => 1,
      'groupid' => 2
      ))),

      Show
      Use this client: https://github.com/moodlehq/sample-ws-clients/tree/master/PHP-REST functionname core_group_create_groupings /// PARAMETERS $params = array('groupings' => array( array( 'courseid' => $courseid, 'name' => 'My grouping 1'.rand(), 'description' => '<b>Description</b>' ), array( 'courseid' => $courseid, 'name' => 'My second grouping'.rand(), 'description' => '<h2></h2><p><b>HTML description</b></p>' ))) core_group_update_groupings /// PARAMETERS // Change the id values $params = array('groupings' => array( array( 'id' => 1, 'name' => 'My grouping 1'.rand(), 'description' => '<b>Description</b>' ), array( 'id' => 2, 'name' => 'My second grouping'.rand(), 'description' => '<h2></h2><p><b>HTML description</b></p>' ))) core_group_get_groupings // Change groupingids $params = array('groupingids' => array(1,2)) core_group_get_course_groupings // Change courseid value $params = array('courseid' => 2) core_group_delete_groupings // Change groupingids $params = array('groupingids' => array(1,2)) core_group_assign_grouping // Change groupingid and groupid to your Moodle test installation values $params = array('assignments' => array(array( 'groupingid' => 1, 'groupid' => 1 ), array( 'groupingid' => 1, 'groupid' => 2 ))), core_group_unassign_grouping // Change groupingid and groupid to your Moodle test installation values $params = array('unassignments' => array(array( 'groupingid' => 1, 'groupid' => 1 ), array( 'groupingid' => 1, 'groupid' => 2 ))),
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-32662-core_course_groups_xxx_groupings

      Description

      • core_group_create_groupings
      • core_group_update_groupings
      • core_group_delete_groupings
      • core_group_get_groupings
      • core_group_get_course_groupings
      • core_group_assing_grouping
      • core_group_unassing_grouping

      Note for all web service contributors: please keep following the document http://docs.moodle.org/dev/How_to_contribute_a_web_service_function_to_core#Create_a_tracker_issue => one web service function == one tracker issue. This bulk issue is an exception. All the group functions are pretty similar and it is a first test to see if this implementation process saves some time or not compare to deal with one web service function per issue. Thank you.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            jerome Jérôme Mouneyrac added a comment -

            Hi Juan, can you write the specs here for all these functions.

            Show
            jerome Jérôme Mouneyrac added a comment - Hi Juan, can you write the specs here for all these functions.
            Hide
            jleyva Juan Leyva added a comment -

            • core_course_groups_create_groupings

            Creates new groupings

            Parameters:

            array of struct {
            name
            description
            courseid
            }

            Returns:

            List of full grouping objects

            • core_course_groups_update_groupings

            Updates existing groupings

            Parameters:

            array of struct {
            id
            name
            description
            }

            Returns:

            List of full grouping objects

            • core_course_groups_delete_groupings

            Deletes all specified groupings

            Parameters:

            array of groupids int

            Returns:

            null

            • core_course_groups_get_groupings

            Returns groupings details.

            Parameters:

            array of groupids int

            Returns:

            List of full grouping objects

            courseid
            name
            description

            • core_course_groups_get_course_groupings

            Returns all groupings in specified course.

            Parameters:

            courseid int

            Returns:

            List of full grouping objects

            courseid
            name
            description

            • core_course_groups_assing_grouping

            Assing groups to groupings
            Parameters:

            array of struct
            groupid
            groupingid

            Returns:

            null

            • core_course_groups_unassing_grouping

            Unassing groups from groupings
            Parameters:

            array of struct
            groupid
            groupingid

            Returns:

            null

            Show
            jleyva Juan Leyva added a comment - • core_course_groups_create_groupings Creates new groupings Parameters: array of struct { name description courseid } Returns: List of full grouping objects • core_course_groups_update_groupings Updates existing groupings Parameters: array of struct { id name description } Returns: List of full grouping objects • core_course_groups_delete_groupings Deletes all specified groupings Parameters: array of groupids int Returns: null • core_course_groups_get_groupings Returns groupings details. Parameters: array of groupids int Returns: List of full grouping objects courseid name description • core_course_groups_get_course_groupings Returns all groupings in specified course. Parameters: courseid int Returns: List of full grouping objects courseid name description • core_course_groups_assing_grouping Assing groups to groupings Parameters: array of struct groupid groupingid Returns: null • core_course_groups_unassing_grouping Unassing groups from groupings Parameters: array of struct groupid groupingid Returns: null
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Go for it Juan

            Show
            jerome Jérôme Mouneyrac added a comment - Go for it Juan
            Hide
            jleyva Juan Leyva added a comment -

            Requesting a peer review, all the functions are implemented

            Show
            jleyva Juan Leyva added a comment - Requesting a peer review, all the functions are implemented
            Hide
            jleyva Juan Leyva added a comment -

            I forgot mention, please review if the way I handle the description fields (forcing HTML format, disabling filtering on output etc.) it's correct

            The docs indicate that I must use PARAM_HTMLCLEAN in output, but I've read Eloy comments in other issues that creates me some confusion

            Show
            jleyva Juan Leyva added a comment - I forgot mention, please review if the way I handle the description fields (forcing HTML format, disabling filtering on output etc.) it's correct The docs indicate that I must use PARAM_HTMLCLEAN in output, but I've read Eloy comments in other issues that creates me some confusion
            Hide
            jerome Jérôme Mouneyrac added a comment - - edited

            Thanks Juan, my peer review:

            • phpdoc needs cleaning. Have a look to some examples: https://github.com/mouneyrac/moodle/compare/master...MDL-30986-attempt2. The essential missing part is the @since 2.3 for each function.
            • add testing instructions (link to the REST demo client + one or two "$params =..." per function will be enough). I'll be testing before submitting.
            • description fields should be PARAM_RAW in xxx_parameters()
            • Optional: you can do a $DB->count_records() instead of a $DB->get_record() because it's running a bit faster.
            • you don't need to do $groupingid = validate_param($groupingid, PARAM_INTEGER);. It's already done by validate_parameters().
            • PARAM_INTEGER are deprecated use PARAM_INT (but anyway this line is going away)
            • I don't think we should return description in create_groupings_returns/update_groupings_returns. The client dev knows what (s)he sent. It would be less trafic.

            It's only about these minor changes. That's very nice to review web service functions when they call a core lib function. I think you were right, we need to let contributor work on multiple function in one issue if the contributor think it will make his/her work faster (and the reviewer work too). I'll modify the Moodledocs.

            Show
            jerome Jérôme Mouneyrac added a comment - - edited Thanks Juan, my peer review: phpdoc needs cleaning. Have a look to some examples: https://github.com/mouneyrac/moodle/compare/master...MDL-30986-attempt2 . The essential missing part is the @since 2.3 for each function. add testing instructions (link to the REST demo client + one or two "$params =..." per function will be enough). I'll be testing before submitting. description fields should be PARAM_RAW in xxx_parameters() Optional: you can do a $DB->count_records() instead of a $DB->get_record() because it's running a bit faster. you don't need to do $groupingid = validate_param($groupingid, PARAM_INTEGER);. It's already done by validate_parameters(). PARAM_INTEGER are deprecated use PARAM_INT (but anyway this line is going away) I don't think we should return description in create_groupings_returns/update_groupings_returns. The client dev knows what (s)he sent. It would be less trafic. It's only about these minor changes. That's very nice to review web service functions when they call a core lib function. I think you were right, we need to let contributor work on multiple function in one issue if the contributor think it will make his/her work faster (and the reviewer work too). I'll modify the Moodledocs.
            Hide
            jleyva Juan Leyva added a comment -

            Hi Jerome,

            I just fixed all the issues you reported, commited the changes and updated the testing instructions

            Show
            jleyva Juan Leyva added a comment - Hi Jerome, I just fixed all the issues you reported, commited the changes and updated the testing instructions
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Just one error:
            when running get_groupings() / get_course_groupings()

            PHP Fatal error:  Call to undefined function file_rewrite_pluginfile_urls() in /Users/jerome/Sites/Moodle_for_review/group/externallib.php on line 729

            miss: require_once("$CFG->libdir/filelib.php");

            Show
            jerome Jérôme Mouneyrac added a comment - Just one error: when running get_groupings() / get_course_groupings() PHP Fatal error: Call to undefined function file_rewrite_pluginfile_urls() in /Users/jerome/Sites/Moodle_for_review/group/externallib.php on line 729 miss: require_once("$CFG->libdir/filelib.php");
            Hide
            jerome Jérôme Mouneyrac added a comment -

            I fixed the test instructions too.

            Show
            jerome Jérôme Mouneyrac added a comment - I fixed the test instructions too.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Once you add the missing require_once I'll submit it. Thanks.

            Show
            jerome Jérôme Mouneyrac added a comment - Once you add the missing require_once I'll submit it. Thanks.
            Hide
            jleyva Juan Leyva added a comment -

            I added the missing require, https://github.com/jleyva/moodle/commit/e94cfbacbdca22986a8f328e4843c5184d141276 (I don't understand how it works for me)

            Show
            jleyva Juan Leyva added a comment - I added the missing require, https://github.com/jleyva/moodle/commit/e94cfbacbdca22986a8f328e4843c5184d141276 (I don't understand how it works for me)
            Hide
            jerome Jérôme Mouneyrac added a comment -

            hum it's a bit weird I agree. Someother external function doesn't require it. But here, with the demo client, it crashes if it's not included...

            Show
            jerome Jérôme Mouneyrac added a comment - hum it's a bit weird I agree. Someother external function doesn't require it. But here, with the demo client, it crashes if it's not included...
            Hide
            jleyva Juan Leyva added a comment -

            Note for integrations, I've rebased and also squashed in one the 3 commits regarding peer review

            There is one commit for each webservice added, a commit for the serviles files and one commit for the peer review issues

            I don't care much about developer credits but I think this number of commits reflects well the work done

            Show
            jleyva Juan Leyva added a comment - Note for integrations, I've rebased and also squashed in one the 3 commits regarding peer review There is one commit for each webservice added, a commit for the serviles files and one commit for the peer review issues I don't care much about developer credits but I think this number of commits reflects well the work done
            Hide
            stronk7 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
            stronk7 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
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited

            Jerome, I've been talking with Juan about the 'core_group_get_groupings' function being apparently not necessary/out of scope and so on.

            And he told me that he just cloned the ones existing for groups, and yes we have there (introduced for 2.2):

            • core_group_get_groups: to get groups by ids.
            • core_group_get_course_groups: to get groups for a given course.

            So I bet that, for consistency between groups and groupings it's better to also accept both for groupings. Not the ideal naming in my opinion, but consistent with groups sister functions.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited Jerome, I've been talking with Juan about the 'core_group_get_groupings' function being apparently not necessary/out of scope and so on. And he told me that he just cloned the ones existing for groups, and yes we have there (introduced for 2.2): core_group_get_groups: to get groups by ids. core_group_get_course_groups: to get groups for a given course. So I bet that, for consistency between groups and groupings it's better to also accept both for groupings. Not the ideal naming in my opinion, but consistent with groups sister functions. Ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi Juan, I've reviewed this and the only point were I've found one potential problem is in core_group_update_groupings() so the new name to be used is not checked to be a duplicate in the course (with id different from the processed one).

            The rest looked perfect. So, if you fix that... I'll be happy to integrate this.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi Juan, I've reviewed this and the only point were I've found one potential problem is in core_group_update_groupings() so the new name to be used is not checked to be a duplicate in the course (with id different from the processed one). The rest looked perfect. So, if you fix that... I'll be happy to integrate this. Ciao
            Hide
            jleyva Juan Leyva added a comment -

            Fixed,

            here you have the commit diff

            https://github.com/jleyva/moodle/commit/2c8ad38effa7389655c4a99d28cfe466d09b51be#group/externallib.php

            PD: I also fixed a white space before new line

            Show
            jleyva Juan Leyva added a comment - Fixed, here you have the commit diff https://github.com/jleyva/moodle/commit/2c8ad38effa7389655c4a99d28cfe466d09b51be#group/externallib.php PD: I also fixed a white space before new line
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated, thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
            Hide
            rwijaya Rossiani Wijaya added a comment -

            This is working fine.

            Note: moodle version need to be increment in order to test perform the test properly.

            Show
            rwijaya Rossiani Wijaya added a comment - This is working fine. Note: moodle version need to be increment in order to test perform the test properly.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            U P S T R E A M I Z E D !

            Many thanks for the hard work, closing this as fixed.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - U P S T R E A M I Z E D ! Many thanks for the hard work, closing this as fixed. Ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  25/Jun/12