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

• core_course_groups_create_groupings

Creates new groupings

Parameters:

array of struct

Returns:

List of full grouping objects

• core_course_groups_update_groupings

Updates existing groupings

Parameters:

array of struct

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

Go for it Juan

Requesting a peer review, all the functions are implemented

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

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.

Hi Jerome,

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

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");

I fixed the test instructions too.

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

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

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...

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

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

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

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

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

Integrated, thanks!

This is working fine.

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

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

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

Ciao