mod_assign_get_grades

From reading http://docs.moodle.org/dev/Gradebook_API, I see that you can get grade from any activity. Should not it be a generic function to retrieve grade like core_grade_get_grades(). It seems to already exists: MDL-30085. Can this function match your need?

Hi Jerome,

No, the data we require comes from the new (in Moodle 2.3) assign_grades table. I believe that this table displays grades before they are added to the gradebook. Damyon can confirm.

Thanks, Paul

No problem, moving it in the roadmap, you can start

My web service review (not related to the get assignement grade logic):

get_grades_parameters

• 'since' description could be a bit more explicit, mentioning it's a timestamp (I'm supposing it's a timestamp ). Normally the descripition should not be much different from the get_grades phpdoc. Note that the description is displayed in the API documentation (Admin Settings > Plugins > Web services > API documentation), so it's essential to fully detail how to use the function in the description info.
• would it be possible that someone wants to retrieve different assignment with different 'since'? If yes, then 'since' should be inside the 'assignmentids' array.

get_grades

• global $CFG; not used, it can be removed
• the capability check seems weird (it sounds to me like a WRITE cap): 'mod/assign:grade'. Please let the integrator knows where you found to logic to get grade in order to accelerate her/his review.
• I would avoid the multiple next to each other $sql = $sql . xxxx ; I would do one $sql = only.

db/services.php

• the service should be disabled by default

webservice/simpletest/testwebservice.php:

• missing unit tests

Once fixed you can submit it, thanks.

I wrote http://docs.moodle.org/dev/How_to_contribute_a_web_service_function_to_core from my last web service reviews. Hopefully it will get lot better but it should be a good start. Let me know what you think.

Cheers,
Jerome

Changes have been checked in for the issues described above. I would generally expect this web service to be used to retrieve all assignments for a given 'since' value and so have left the parameter as it is. The capability is a new capability added in the new mod/assign/db/access.php.

I've read the HOWTO page. It looks good. One think that might be added although maybe it is more of a general git comment and that is that it is important to be aware of the core.autocrlf setting. Having this set to true can cause the issue where lines of code are accidentally changed as described in the HOWTO.

Hi Paul,
I didn't know about this core.autocrlf settings. You can add it to the howto.
You can also submit the issue to integration.

Thank you.

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 complete

Hi,

I'm reopening this issue because:

• It was spotted that the SQL statements are not using placeholders and instead are embeding variables directly in SQL. Please ensure we use placeholders here.

• It requires mod_assign to land first. This has not been integrated yet so we can't integrate this. Please don't send this to integration until MDL-31270 is integrated, we can't put the cart before the horse!

Hi,
I've rebased and modified the sql to use placeholders.

Hi Paul/Yang, if I don't review these "assign" issues as soon as the new "assign" module get integrated, please ping me

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,
can you add the PHPunit test. PHPunit test is now requirement to send a web service function to integration. An 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

I reassign the issue to you Paul. You don't need to assign the issue to the peer-reviewer. The assignee is only the person resolving the issue. Thank you.

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

This generally looks good - I found some minor issues - once these are addressed this will be fine for integration.

  

74 +               $sql = "SELECT cm.id, cm.instance FROM {course_modules} cm JOIN {modules} md ON md.id = cm.module ".
75 +               "WHERE md.name = 'assign' AND cm.instance ".$sqlassignmentids;

This should be using a parameter for the module name and should be using single quoted strings for all SQL.

Hi Damyon, I've fixed the issue you found. This should now be ready for integration. Note my comment on 14th August. PHP unit tests for the assign module will not work until this is fixed.
Cheers, Paul

Hi Paul, One more thing:

$requestedassignmentids = $params['assignmentids'];

This should be using
$DB->get_in_or_equal instead of just concatinating the ids to the SQL (this will properly handle a list with a single entry).

Regards, Damyon

Hi Damyon,
Are you sure, I think I'm already doing that (see line 73)?
Paul

Sorry - my bad - I missed that when looking this morning.

This looks ready to go to me.

Thanks Paul

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

Note to the integrator:
In my comment on 14th August I mentioned 2 minor changes that are required in the assign module in order for phpunit tests to run correctly. These are:

1) Modify the declaration of assign_add_instance in mod/assign/lib.php (similar to other mods such as forum). As shown below:

function assign_add_instance(stdClass $data, mod_assign_mod_form $form = null)

2) Modify the constructor in mod/assign/locallib.php for the problem described in MDL-27968. A quick workaround in order to run the tests is to comment out the call to get the renderer. See below:

//$this->output = $PAGE->get_renderer('mod_assign');

Hi,

I'm sorry but I'm sending this out from current integration, no way we can add it with those changes in assign pending. I'd recommend Jerome and Paul to share this and MDL-27968 with Damyon in order to get the blocking situation cleaned.

But we cannot add stuff causing phpunit breakage at all. 100% forbidden. Sorry.

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

I have created MDL-35388 to fix the problems preventing php unit tests being run for the assignment module web services. I will submit it for peer review as soon as I am able (tomorrow hopefully).

I have now submitted MDL-35388 to Damyon for peer review and submission to integration

Resubmitted for peer review and integration now that MDL-35388 has been submitted for 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,

We seemingly there are have merge commits on this branch, we really need a clean branch on top of moodle.git for pulling these changes into core. There is also trailing whitespace in mod/assign/db/services.php.

If other things are OK i'll fix this up myself in order to try and prevent slowing down things too much.

Hi,

I'm afraid this is failing:
There was 1 error:

1) mod_assign_external_testcase::test_get_grades
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:121
/Users/danp/git/integration/lib/phpunit/classes/data_generator.php:405
/Users/danp/git/integration/mod/assign/tests/externallib_test.php:58
/Users/danp/git/integration/lib/phpunit/classes/advanced_testcase.php:76

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

Can you squash that merge commit Paul ?

Yes, just as soon as I work out how to squash a merge!
Cheers, Paul

http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html

You will then have to use --force when you re-push the branch.

Done. Note that you can't use interactive rebase to squash merge commits. You have to use reset.
Cheers, Paul

Thanks Paul, sending for 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

Thanks Paul, i've integrated this now.

I'm unable to locate the mod_assign_get_grades function for the WS service.

I spoke to Jerome and he said the version for mod/assign needs to be updated.

Will ping integrator regarding this.

I've done a minor version bump for this now.

Tested on integration repo for master.

It works as expected.

Test passed.

Congratulations, you've done it!

Nf n erjneq sbe fhpprfshy vagrtengvba vagb guvf jrrxf eryrnfr, V pna abj qvfpybfr gb lbh gur rkvfgnapr bs shapgvba fge_ebg13(), gb tb va lbhe gbbyxvg nybat jvgu uggc://cuc.arg/znahny/ra/shapgvba.tmtrgff.cuc

Cyrnfr qb abg nyybj guvf vasbezngvba gb cnff shegure.