Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      39605

      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.

        Issue Links

          Activity

          Hide
          Jérôme Mouneyrac added a comment -

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

          Show
          Jérôme Mouneyrac added a comment - Hi Juan, can you write the specs here for all these functions.
          Hide
          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
          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
          Jérôme Mouneyrac added a comment -

          Go for it Juan

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

          Requesting a peer review, all the functions are implemented

          Show
          Juan Leyva added a comment - Requesting a peer review, all the functions are implemented
          Hide
          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
          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
          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
          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
          Juan Leyva added a comment -

          Hi Jerome,

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

          Show
          Juan Leyva added a comment - Hi Jerome, I just fixed all the issues you reported, commited the changes and updated the testing instructions
          Hide
          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
          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
          Jérôme Mouneyrac added a comment -

          I fixed the test instructions too.

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

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

          Show
          Jérôme Mouneyrac added a comment - Once you add the missing require_once I'll submit it. Thanks.
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          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
          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
          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
          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: