Thanks Paul for writing this sub-task. Here is the comment I posted previously in the forums about this function specification:

that's kind of tricky. If the parameter is 'capability' then maybe the name is not correct. Or maybe 'capability' should be just one of the parameters. It seems like two functions in one. Which is not bad at the end as it could avoid two web service calls. But in this case a new parameter should be able to tell the function to return either the courses either the assignments. Note we already have a function that return courses, we should avoid duplicating feature as much as possible. Finally the web service function is more related to assignments than course, it should be named core_assignment_... I think we need to discuss all that in its issue.

Can you explain all the use cases for this functions (in which situations clients are going to call this function, is there a role in particular for which this function was suggested,...)? At this moment my idea on the issue is to specify a more generic function.

Hi Jerome,

When Lightwork is getting the data it requires for marking, this is the first web service that is called. The web service needs to return data about all the courses and assignments that the user is able to "manage and mark" or just "mark". Once Lightwork has this data it is able to:

• populate its local database
• Display the courses and assignments in its UI
• Call subsequent web services to get further course and assignment related information (eg course participants, rubrics, submissions etc)

In the current version of Lightwork we have created 2 capabilities for "manage and mark" and "mark" which we use to identify the courses and assignments to return. By default these capabilities are assigned to the roles "teacher" and "non-editing teacher" and so generally the courses and assignments that are returned are those for which the user has the role of "teacher" and "non-editing teacher".

In order to simplify and make the web services more generic I am thinking that it would be easiest for us to remove the capabilities and just use roles.

This would mean that we would call this web service by passing in a role parameter (teacher or non-editing teacher in our case) and the web service would return all courses and assignments for which the user has the role of XXXXX.

Yes I think it is perfectly sensible to rename it to core_assignment... (or should it be mod_assignment..)? We would prefer this web service to get both assignment and course information in one go since it avoids 2 calls and the amount of course data that we require to populate our UI is small.

Thanks, Paul

Hi,
assignment is just one of the module types that course may have. I would suggest to pass the types of modules that you are interested as an argument. Would be something like
core_course_get_contents($moduletype = null)
or
core_course_get_modules($moduletype = null)
that would return all modules in the course and if $moduletype is specified than only modules of this type will be returned.

Hi Jerome, Marina,

Looking at the web services roadmap I can see that there is already a core_course_get_contents.

How about core_course_get_assignment_contents? Then later on there could be other web services for the other modules such as core_course_get_forum_contents etc.

Clients needing data for all modules would use the existing core_course_get_contents and those (like Lightwork) that target specific modules would call web services such as core_course_get_assignment_contents

core_course_get_assignment_contents would take an optional role as a parameter to allow clients to, for example, just get the courses for which the user is a teacher.

Would this be acceptable?

I'm adding Damyon as he is in charge of the assignment rewritting. Paul and Damyon should have a meeting about it soon. Let us know what you come up with.

I suppose as assignment type could be anything, we'll end up to have functions call assignment_TYPE_get_contents(assignmentids, ...). Each of them having specific return value structures. Otherwise if all assignment type return the same value structure, mod_assignment_get_contents(component = 'assignment_type', courseid, ...) should be enough.

To update:

Hi Jerome, Marina, Damyon,

Damyon and I met to discuss this webservice. We agreed on the name core_course_get_contents but I forgot that that web service already exists so am returning to my previous suggestion of core_course_get_assignment_contents. We also agreed that an optional list of capabilities (not role ids) should be one of the optional parameters. Note that this web service will be used with the new Moodle 2.3 assignment and so there will be no need to specify an assignment type.

I suggest the following:

core_course_get_assignment_contents

parameters:

optional array of course ids
optional array of capabilities

If course ids are not specified and 1 or more capabilities are specified then only the courses and assignments for
which the user has the specified capabilities would be returned

return value:

Array of courses
Each course has an assignments array

Actually I think the name should be mod_assign_get_course_assignments and it should sit in the assignment module since it is assignment specific.

Yes, I agree, mod_assign_get_course_assignments is good.

I propose that the following data structure be returned:

An array of courses. This will contain 0 or more course arrays with the following data:

course (note: only courses with at least 1 assignment are returned)
{
id: 2
contextid: 4
fullname: 'My full course name'
shortname: 'short name'
timemodified: 1234
assignments:

}

assignment

An array of warnings. This will contain 0 or more warning arrays as follows:

warning

Returning assignment info is the main purpose of the function. My preference would go for mod_assign_get_assignments yet.

I think that if an external application knows the course id then this external application knows fullname, shortname, timemodified... All these info should have been returned by core_course_get_courses(). I don't think we need to return more than the id as course info here. I also think that contextid is not necessary as with the course id, the ws functions always knows the right course context.

I'm moving the issue in the roadmap as it seems pretty obvious that this function need to be implemented (regardless the course info issue).

Hi Jerome,
The course information is there because this web service can be called without specifying any course ids. In this case the courses and assignments returned depend on the capabilities of the current user (in Lightwork's case we want all the courses and assignments for which the user has the marking capability). Including the basic course information also avoids having to make an additional web service call.

a) I think the external app should know about the course info at the moment of this call. If not, it means that the external app doesn't store any information locally. I could become messy if we have to return every single info because some app doesn't have local storage.

Sending a course id parameter is not related to the return value structure (or I didn't understand the function purpose):

b) we don't have a very clear way to deal with errors in Moodle web service yet. It's a mix of exception (most of the time), string error message, and sometimes additional error code number. This need to be cleared out. I'll ask on the forum and go to write some specs.

Hi Jerome,

Lightwork does of course store course information locally. However, the courses that the user can mark can change at any time on Moodle (they may have been unenrolled from a course, enrolled in a new course or the course may even have been deleted). For this reason, every time data is synchronized we will need to call this web service WITHOUT specifying course ids even though we may already have them.

If we do not include the course information in the return value as you suggest then we would need to call another web service after this just to populate the few pieces of course data that we use for display purposes. I think this would be inefficient and I also suspect that the ability to display course and assignment data together is likely to be a common need for clients in the future.

What do you think?
Paul

Hi Paul,
I would like to avoid it. In the worst case where the app does not know about a course info, the app will do one more web service call. It will look not nice for the user (loading message), but it should be pretty rare. In my opinion your app should end up to deal with situation like that.

In another hand I agree that with my point of view, the app local storage gets complex - My Moodle iOS app core data is quite complex. It also require more DB requests in the app.

You are contributing and your are spending lot of time, so I will agree with you Let's go with some basics course info.

I removed the context ids from the specs following last comment in MDL-31681.

Thanks Paul. Here is my peer-review for this function (I didn't test it, I just read the code). There are several little fixes and suggestions, but don't worry it's normal for a first ws peer-review function. And a lot are even related to the same initial issue:

• for the phpdoc, follow the code style documentation. Specially don't forget the @since Moodle 2.3 for each function.
• if possible add a test in the webservice/simpletest.php (it's quite time consuming but you could catch some bugs and discover use cases. It's as you wish.)
• you forgot context validation and capabilities checking. For example:

  self::validate_context($context);

  require_capability('moodle/assignments:view', $context); //it could be some different capabilities, this is just an example. Read the script displaying the assignments in the Moodle UI script (I didn't look which one it is, I let you find) to know what are the expected capability checks.

• delete the error_log calls
• use $USER->id instead of $userid. $USER is set by web service servers.
  also delete:

  // get the current user
          if ($user === null) {
              $userid = $USER->id;
          } else {
              $userid = is_object($user) ? $user->id : $user;
          }
  

  also change this: has_capability($cap, $context, $userid) // remove the $userid param, has_capability automatically use $USER

• enrol_get_users_courses($userid, true, $fields, $sort);
  $sort => is never assigned before this call
  enrol_get_users_courses() return an array with for keys the course ids. It is going to make you save a lot of line of codes. The following can be replaces by less lines checking if the key(id) exists in $courses:

  $unavailablecourseids = array(); // used to test for ids that have been requested but can't be returned
          if (count($courseids)>0){
          	$unavailablecourseids = $courseids;
              foreach ($courses as $id=>$course) {        	
          		$found = false;
          	    foreach ($courseids as $courseid){
          		    if ($courseid == $id){
          		    	$found = true;
          		    	$unavailablecourseids = array_diff($unavailablecourseids, array($courseid));
          		    	break;
          		    }
          	    }
          	    if ($found == false){
          	        unset($courses[$id]);    	
          	    }        	
              }
          }
  

  php function: array_key_exists($key, $array) //it's not known as a super fast function but it's easier to read and maintain than multiple foreach and if conditions.

• try to use $params['courseids'] instead $courseids (same comment for all params). The idea is that this params have been cleaned. If I remember it doesn't make a big difference because the validate_parameters() should stop the execution if it detects an error, but for consistency I think it's better to use the validate $params. It also show new devs to not forget the validate_parameters().
• avoid using $k1 / $v1. If the $courses key was the id you could put $id => $course which is more readable. Actually you should even go for a $courses => $course and using $course->id
• in the error message you said "does not have required capability". I think you mean "does not have requested capability" ? Checking required capability are decided by the function not by the client. I would translate this string as they could be displayed by the client.
• some variable name can be straightforward like assignmentarray => assignments (but really I'm not the best person to talk about variable name ).
• when returning a courseid do not call it course but courseid, there is no possible questioning (is it an object, an id...) with courseid. Same for grade (gradetype).

If possible can you separate every web service function contribution in different git branch (named as the MDL-XXXX), with one commit per branch. It is easier for integration.

Thank you

Hi Jerome,

Thanks very much for your feedback. We've addressed the issues and have put the work on a branch named MDL-31683 as you requested.

Thanks, Paul

Hi Paul, I edited the issue to show you what the integrators are expecting into the Pull fields:

Pull Master Diff URL: it is the difference between your master branch (same code as the Moodle git master branch) and your MDL branch. It's important that your branch contains only the code related to the function, otherwise it makes the integration very difficult.

Pull Master Diff URL: name of the MDL branch

Pull from Repository: git address of your repository

------------------------------------------------------------------------------------------------------

If ever you don't know how to separate your current code, I can suggest the following in git workflow:
a) checkout your MDL-31683 branch and do "git reset --soft HEAD" to go back to HEAD status without losing your change. (PS: check if it's the right command, I personally do one "soft commit reset" at the time with "git reset --soft HEAD^")
b) keep only the code related to mod_assign_get_assignments (with all the new files)
c) do one unique commit -a -m "MDL-31683 mod_assign_get_assignments web service function"

== Beginning of Optional: when your master branch is outdated compared to Moodle git master branch / when your MDL branch is outdated compared to your master branch ==
d) checkout your master branch and pull the last change from Mooddle git master branch
e) checkout your MDL-31683 branch and do 'git rebase master'. If any conflicts, fix them and do 'git add name_of_file' and 'git rebase ---continue'.
== End of Optional ==

f) finally push the commit into your github MDL-31683 branch. The issue is good for integration review.

Hi Jerome,

Thanks for helping out with this. I ended up using the very useful "git rebase --interactive master" command which allowed me to combine all my commits into a single commit. Hopefully, this will make integration simpler.

Paul

Hi Paul,
my main concern in my last comment was to remove the skeleton code about 'mod_assign_get_submissions' in each different files. In the current state:
1- Integrators need to identify which lines of code do not concern this MDL and delete these lines.
2- Integrators have to play with git command to reattribute you as author.

Cheers,
Jerome

Hi Jerome,

I've just modified branch MDL-31683 to remove all references to mod_assign_get_submissions. Hope it's OK for you now!

Cheers, Paul

It's good, thanks Paul. Last little pre-integration fix: it is important to avoid to change all lines when in fact you added only few lines: https://github.com/Lightwork-Marking/moodle-mod_assign/compare/master...MDL-31683#diff-0
Otherwise you'll be "git blame" (or "git praise") for all the lines of this file.

Can you also either write a test description with a client demo (like in MDL-30084) or either create a unit test in /webservice/simpletest/testwebservice.php. A tester will test the function after the integration process, this person needs detailled test instructions.

Hi Jerome,

I'll work out how to fix the "all lines" change problem. Not sure what happened there. Git is a very powerful and flexible tool but it certainly has lots of traps for the unwary!

We create all our unit tests in Java but setting them up for your testers might be tricky so I'll have a look at the other options you suggest.

Cheers, Paul

Hi Jerome,

The pre integration fix has been completed and test instructions have been added.

Cheers, Paul

Thanks Paul, we're almost here. Your commits still change all lines of service.php. At the moment, the result of git blame is marking you as the modifier of the entire file: https://github.com/Lightwork-Marking/moodle-mod_assign/blame/23bc526e2fca999333cdee5af2c0ea0708216aac/lib/db/services.php

You just need to do a soft reset of your two commits, recommit again and push -f. Then we are back on track.

Thanks for the test instructions it's going to be useful for the tester and myself. I'll test before to go to holidays so it can gets integrated quickly and it's easier for you to work on other issues having the external file already integrated in git.

Cheers

Just a note about test instructions so you don't feel it to painful to write it every time: usually is about copying another web service test instructions and changing the function name + full param construction. The QA tester is free to test the most essential cases (s)he think of (i.e. no optional params, empty params...).

I've just tested it, it works well (a course with one assignment and a course with two assignments).

Some things that I think need to be changed:

During creation of an assignment I saw many fields which are not returned:

• description
• allow submission from
• prevent late submissions
• send notification to grader
• online text
• file submissions
• maximum number of uploaded files
  ...

I think they should be returned when an admin/teacher call the function. Many client will need to know about them. Don't forget the capability check (the same as the one allowing to update an assignment - note that maybe it's a generic one allowing you to update a module)

About capability, you should not create any new one when creating web service. All capabilities should exist in core and all web service capability checking should match the core ones. For this reason, I don't think you need to change anything in mod/assign/db/access.php and mod/assign/lang/en/assign.php. Actually you don't use these capabilities in the code, so I suppose it's just a missing cleaning.

Once fixed, can you send the issue for peer review to Sam Hemelryk. He's reviewing the Damyon assign module, he will catch all logic issue. I'll be back in a week, talk to you soon.

Hi Jerome,

Are you sure about the commit? I fixed it 4 days ago as shown by the "pull master diff URL" at https://github.com/Lightwork-Marking/moodle-mod_assign/compare/master...MDL-31683

Hi Jerome,

I'll add the fields allowsubmissionsfromdate, preventlatesubmissions, sendnotifications, submissiondrafts. I think the other fields you refer to are from the assign_submission, assign_submission_file and assign_submission_online tables. They are included in the separate web service function mod_assign_get_submissions.

The 2 extra capabilities you refer to are required for another part of our integration work, Marking Management. However, since they are not needed for this web service I'll remove them from this commit as you suggest.

Thanks, Paul

Hi Paul,
a) if you look at the git compare, it looks like you didn't change any lines because the second commit revert the changes. However as you did modified the all lines in both commits, you'll be marked as the last modifier (see the left column in https://github.com/Lightwork-Marking/moodle-mod_assign/blame/23bc526e2fca999333cdee5af2c0ea0708216aac/lib/db/services.php). To resolve this problem, you need with git to soft reset both commits then make one single commit then force push. It's super quick to do and it will fix the "blame" problem.

b) I don't think the other fields are for submissions. If a client let update an assignment, then the client needs to know the assignment settings. It's why I think we need all fields from update.

c) However I've just looked at core_course_get_courses_content. From reading the return value description I can see that the function returns all assignments info that mod_assign_get_assignments (except the grade). I think for you it would be worth to only modified core_get_course_content $options param to return only one kind of 'modname'. Then to add an optional 'grade'. It would not resolve the issue I mentioned in b) but it would do exactly what get_assignments currently does and it's more easily going to be integrated.

c) Ah I forgot that you wanted capabilities as parameters, so core_course_get_course_contents would need to filter that too. Moreover core_course_get_course_contents does not support multiple course. This last issue is really why it's hard to adapt core_course_get_course_contents to your need.
Note that at the end mod_assign_get_assignments code should be pretty similar of core_course_get_course_contents. Ok it's not really holidays if I keep working, talk to you in a week

Hi Jerome,

Yes, you're right, those other fields come from mdl_assign_plugin_config. I hadn't originally planned to include those. However, it should'nt be a problem to add them so I'll update the web service function accordingly.

As for the commit, I don't normally like to modify commit history but will make the change as you suggest in order to resolve the praise/blame issue.

Cheers, Paul

As requested, additional fields have been added and the commits have been modified to remove the commit that changed lines of code unnecessarily.

Hi Paul,

Sorry I haven't been as active in this discussion as I would have liked - those other fields are in mdl_assign_plugin_config as you have said - I would probably prefer that you return all of those settings relating to the assign in a name/value list as you never know what other plugin authors are going to put in there. The "pluginname"_enabled setting is the only one that is guaranteed to exist for all plugins and should be checked to make sure the assignment has the file upload plugin enabled before you post files to it.

• Damyon

name, value pairs included as asked for by Damyon. Assigned to Sam for testing.

sending to integration for Sam review

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

Hi,

I'm taking this issue out of integration because it first requires mod_assign to land.

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

Completed rebase

Hi,

I'm reopening this issue because it requires mod_assign to land first. This has not been integrated yet so we can't integrated this. Please don't send this to integration until MDL-31270 is integrated, we can't put the cart before the horse!

Hi Jerome, Can this now go into integration?

Hi Paul,
as mentioned by email I'm waiting to finish MDL-32581 then I'll re-peer-review as http://docs.moodle.org/dev/Errors_handling_in_web_services recently changed too. If you have some time, update your code following these changes. Mainly look at http://docs.moodle.org/dev/Errors_handling_in_web_services#When_to_send_a_warning_on_the_server_side and the warnings format.
Cheers,
Jerome

Hi Jerome,
Updated warnings and rebased.
Thanks, Paul

Hi Jerome, Can this now go into integration please?
I have also removed some whitespace and removed the Lightwork services addition to lib/db/services.php. The change to add the Lightwork service will now be done under a separate issue in Moodle 2.4 when all web service functions have been added.
Thanks, Paul

Hi Paul, there are some code cleaning to do before I can send this issue to integration (specially the warnings that instead should be exception) Have a look to my comment in MDL-31681.

All your functions will get more love from HQ after we release Moodle 2.3. And hopefully your functions should be some of the first integrated in 2.4 (and 2.3.1 as from 2.3.X we'll integrate web service function in minor version when they are compatible).

Can I ask you one more thing, could you write PHPunit test for your new external functions. I'm writing http://docs.moodle.org/dev/Web_Services_Unit_Test, Moodle PHPunit docs are not finished yet but that should be enough to get you started with the new PHPunit support in Moodle.

Thanks.

Hi Jerome,

I have already run the Moodle code checker on mod_assign_get_assignments and removed whitespace (see my previous comment).

I saw your comment about warnings/exceptions but I don't agree that the warnings in this function should be changed to exceptions. This function accepts an array of courseids. The user's rights to access each course and its assignments are checked every time the function is called. If the code detects that the user no longer has permission to view a course or module then it should create a warning and continue processing the next course or assignment.

It would not be appropriate to throw an exception since this would prevent processing of all the other courses/assignments.

I have already created a web service test for mod_assign_get_grades which I added to webservice/simpletest/testwebservice.php (MDL-31873). Is this how you want the other tests done?

Thanks, Paul

Hi Paul,
about the unit test (as the exception/warnings issue is discussed somewhere else) we switched from simpletest to PHPunit in 2.3. I started to write some unit tests and a Moodledoc. You can get help from:

• http://docs.moodle.org/dev/Web_Services_Unit_Test
• http://tracker.moodle.org/browse/MDL-33529

Note that neither MDL-33529 neither the doc have not been reviewed yet.

Hi Paul,
a PHPUnit test example can be found there: https://github.com/moodle/moodle/blob/master/course/tests/externallib_test.php (you can look at get_categories as example).
Cheers,
Jerome

Hi Jerome,
I have added a PHPUnit test. Note, however that issue MDL-27968 (I've added a comment) means that a coding exception will occur when tests are run until it is fixed.

Also, to allow tests to be run, the assign_add_instance function in mod/assign/lib.php needs to be fixed so that the mod_assign_mod_form parameter either defaults to null or is removed completely (it is not used). See the equivalent functions in the other mods. Its a trivial change. Do you want me to create a separate issue or just include the change with this issue?
Thanks, Paul

Hello guys,
following the Moodle peer-review process, I reassigned the peer-reviewer to the component maintainer. If the component maintainer is unknown/unavailable, assign the peer-reviewer to moodle.com.
Thanks.

Note for the peer-reviewer: I understand the issue history is long. However you just need review the "Pull Master Diff URL". Then you can look at the issue history if anything seems strange. If any question you can contact me in private, I'll be happy to help. There is also a Moodledocs to help peer-reviewing web service contribution: http://docs.moodle.org/dev/How_to_peer_review_a_core_web_service_function. Thanks. Jerome.

Hi Jerome, Damyon,
Can this issue be peer reviewed? It missed 2.3 and so we really want to make sure it's included in 2.4.
Cheers, Paul

Hi Paul,

Sorry this has taken a while for me to review.

The code is good - the only issue I found is that:

• preventlatesubmissions has been removed - this is now a timestamp "cutoffdate". If 0 - late submissions are allowed - otherwise it is a date after which late submissions will not be accepted.

There are a number of other patches currently in integration review that will also be adding settings to the "assign" table - either this feature should wait until they are all integrated and add each of them - or alternatively if the external_single_structure for the assign module could be built from the database table - then it would automatically stay in sync with the module as features are added. Any opinions on this?

• Damyon

One more comment - why is this code using "require_capability" in a try catch instead of "has_capability"?

Hi Damyon,

I don't like the idea of dynamically building the external_single_structure from the database table since I can imagine problems with web clients receiving unexpected/different data due to database changes. However, I also don't like the idea of waiting for additional patches to be integrated since if the patches are delayed then this web service will get missed again.

I am quite happy to create an additional issue to update this web service once your patches have been applied. After all, updating of web services to match new/modified Moodle code is likely to be a common occurence.

What do you think?

I will fix the issue that you found. I use require_capability because it clearly describes the intent of the code and is commonly used throughout Moodle.

Paul

Looks like cutoffdate (MDL-31295) hasn't yet been integrated. I'll check again tomorrow.

Hi Jerome,

Damyon is suggesting that for this and web service MDL-31682 that we could dynamically generate the return structure from the fields in the database. This would allow us to dynamically return data for 3rd party plugins that are added as well as new database fields. I am a little concerned about this approach since clients might receive information they are not expecting and not receive data they are expecting. What is your opinion?

Cheers, Paul

Just to clarify - making this dynamic for this webservice was just a suggestion - I don't mind either way. For MDL-31682 it is a requirement - the core webservice must support 3rd party plugins.

Hi guys,
as Paul said, I also think it's not a good practice to generate param/return descriptions, it would imply the web service API could change, which would break many clients.

That's fine for this issue - I'll put comments on MDL-31682

Damyon have you the permission to submit a tracker issue to integration? If no I'll submit it when Paul says it's ready.

The new database fields including cutoffdate have now been added so this should now be ready for integration.

Damyon, did you see my previous comment on 20th July? I described some minor fixes that are needed for phpunit tests to run in the new assign module. Do you want to make these or should I create a new issue.

submitting to integration.

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

rebase done

Hi,

Sorry to reopen another one, but this is also failing:

There was 1 error:

1) mod_assign_external_testcase::test_get_assignments
Undefined property: stdClass::$assignfeedback_offline_enabled

/Users/danp/git/integration/mod/assign/locallib.php:565
/Users/danp/git/integration/mod/assign/locallib.php:477
/Users/danp/git/integration/mod/assign/lib.php:39
/Users/danp/git/integration/mod/assign/tests/generator/lib.php:131
/Users/danp/git/integration/lib/phpunit/classes/data_generator.php:405
/Users/danp/git/integration/mod/assign/tests/externallib_test.php:66
/Users/danp/git/integration/lib/phpunit/classes/advanced_testcase.php:76

To re-run:
phpunit mod_assign_external_testcase mod/assign/tests/externallib_test.php

We need to have the unit tests 100% passing all the time, so this blocks integration.

That's OK Dan, its been that kind of day! We will investigate and fix as soon as possible.
Cheers, Paul

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

Fixed up failing test, missing new line at end of file and removed merges. Assigning to Damyon for peer review.

Hi Damyon,
I've rebased and so this is now ready for integration.
Thanks, Paul

Thanks. I don't think Daymon needs to re-peerreview as it's coming back from integration. Resending

The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

Apologises for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

Hi Sam, I'm just trying to rebase this with the latest dev changes. However, I've noticed that the cache/classes/loaders.php function implements cache_loader and cache_is_key_aware both of which define functions has(), has_all() and has_any(). This doesn't seem correct to me?

Hi, I've just rebased and pushed with the latest weekly modifications. This issue was missed out from integration last week. Can it be integrated this week?
Cheers, Paul

Hi, I've rebased again to pick up the changes just integrated to fix MDL-36047. Ready for integration.
Cheers, Paul

Hi Paul,

Indeed the inclusion of cache_is_key_aware wasn't meant to be there. Also noting should someone else hit it that if you were running PHP 5.3.2 you would have had a fatal error trying to load any page because of it.
We integrated a fix for it bout 6 hours after MUC itself was integrated. Should be all fixed up now.

Seeing as I am here I will pick this up for integration review

Many thanks
Sam

Hi Paul,

I've been looking over this for the past couple of hours.
I decided given this has been in the works for so long, that you're so close already anyway, and that code freeze is looming I would make the changes I thought were required rather than simply note them.

I've produced a branch that I'd like you to take a look at for me, if you could please review the changes I have made. If you are happy with things then I feel at this point this can land.
If not no probs we can have a look at things this week and get it in for next.

https://github.com/samhemelryk/moodle/tree/wip-MDL-31683-m24
https://github.com/samhemelryk/moodle/commit/8906e1c9de4c38ed00acffed71ce7b4c78ed05a3

Of particular interest is that I changed the capability checks being performed on the course, originally there was viewparticipants, however I felt that was wrong as you were displaying information about the course that was usually associated with the ability to access it. I've changed it to use course:view and if the course is hidden to check the users ability to view hidden courses.

Plenty of other small formatting/code style related changes, nothing serious really.

Again this needs to be reviewed before I can integrate, if you don't have time no probs I'll get one of the guys in the office to check it out and we can consider it again next week.

Many thanks
Sam

Hi Sam, One reason we used capability course:viewparticipants instead of course:view is that we expect a large majority of the users of this web service to be teachers. By default the course:view capability is not set for the teacher role and so in order for teachers to use this web service function the administrator would have to remember to give them the capability.
However, you will have a better understanding of the correct way to use capabilities in Moodle and so if you feel it is appropriate to make this change then that is fine. All your other format changes are fine.
It would be really great if this issue could be integrated this week since its been waiting so long and its blocking MDL-31682 which we also really want to get included before the code freeze.
Cheers, Paul

Hi Sam, I've just found a problem with your changes that I've fixed and pushed. I've also included all your other improvements to the code. The problem happened when not all the courses that the user is enrolled in are specified. A warning message was incorrectly being returned. Let me know what you think.
I hope this can still be included in this week's integration!
Cheers, Paul

Thanks for checking that out and picking up + fixing that bug Paul.

I've looked across the access checks and have in fact removed the cap checks for the course.
Looking through the path I found enrol_get_users_courses with the second argument being set to true is making all of the required access checks for the course anyway.

I'm now happy with these changes and they have been integrated (hoorah).
This will be included in 2.4 (providing testing passes of course) and any further changes can be addressed in new issues.

Cheers
Sam

Thanks guys, passing this now.

Your issue has dug up some gold.
It works great i've been told.
Go forth, be brave, be bold.

yay! "All your thoughts are belong to everyone."

Thanks and ciao!

Hi guys, just a tiny note as I was looking to the warnings in this issue to be consistent in another external function.

This line:

'warningcode' => '1',

should be a string (use the exception code as much as possible)

'warningcode' => 'anexplicitstring',

cheers,
Jerome

One of the unit tests added here caused a regression: MDL-43926.