Details

    • Testing Instructions:
      Hide

      run phpunit core_grades_external_testcase lib/tests/grades_externallib_test.php

      Testing:

      Enable Web Services in Advanced features
      Enable Web services for mobile devices in Plugins / WebServices / Services

      Use this client: https://gist.github.com/jleyva/9687810
      The curl.php file is here: https://github.com/moodlehq/sample-ws-clients/blob/master/PHP-REST/curl.php

      You need a token related to a service, for get a token you need to create a new service, add a shortname to that service, and add the core_grade_get_grades function to that service

      Create Token:

      1. Click on Site administration ► Plugins ► Web services ► Manage tokens
      2. Click add, select user and service (You should get two tokens, one for student and one for teacher account)

      1 Create a new course, enrol the student and user used before
      2 As teacher, create a new assign (require a only text submission) as a the same student you used for getting the token, submit a text.
      3 Grade the submission as the teacher
      4 Edit the client.php for adding your custom tokens and Moodle URL, also the parameters required (courseid, userids with the student user id, and cmid for the assign module)
      5 Open the script in a browser
      6 The script gets the current grade, then update the grade (using the update_grade) function to a random (0, 100) them, retrieve again the grade to check if it were successfully updated
      7 You can change the userid to a different id that the current user to test if you got permission errors, you should change alto the activityid to an existing activity, you should get errors also

      Show
      run phpunit core_grades_external_testcase lib/tests/grades_externallib_test.php Testing: Enable Web Services in Advanced features Enable Web services for mobile devices in Plugins / WebServices / Services Use this client: https://gist.github.com/jleyva/9687810 The curl.php file is here: https://github.com/moodlehq/sample-ws-clients/blob/master/PHP-REST/curl.php You need a token related to a service, for get a token you need to create a new service, add a shortname to that service, and add the core_grade_get_grades function to that service Create Token: Click on Site administration ► Plugins ► Web services ► Manage tokens Click add, select user and service (You should get two tokens, one for student and one for teacher account) 1 Create a new course, enrol the student and user used before 2 As teacher, create a new assign (require a only text submission) as a the same student you used for getting the token, submit a text. 3 Grade the submission as the teacher 4 Edit the client.php for adding your custom tokens and Moodle URL, also the parameters required (courseid, userids with the student user id, and cmid for the assign module) 5 Open the script in a browser 6 The script gets the current grade, then update the grade (using the update_grade) function to a random (0, 100) them, retrieve again the grade to check if it were successfully updated 7 You can change the userid to a different id that the current user to test if you got permission errors, you should change alto the activityid to an existing activity, you should get errors also
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_27_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-30085_grade_ws

      Gliffy Diagrams

        Issue Links

          Activity

          Hide
          jleyva Juan Leyva added a comment -

          It would be interesting if this function can work like:

          /grade/querylib.php

          grade_get_course_grades

          Returning the full grade items objets for the given course and user/s

          Also, since these are grades for the gradebook that can overwrite the activites grades, returning an object with the grades and information of the original activities would be interesting

          Show
          jleyva Juan Leyva added a comment - It would be interesting if this function can work like: /grade/querylib.php grade_get_course_grades Returning the full grade items objets for the given course and user/s Also, since these are grades for the gradebook that can overwrite the activites grades, returning an object with the grades and information of the original activities would be interesting
          Hide
          marina Marina Glancy added a comment -

          I suggest that this function also returns all gradingform instances for advanced grading, if applicable. In this case we won't need function like MDL-31890

          Show
          marina Marina Glancy added a comment - I suggest that this function also returns all gradingform instances for advanced grading, if applicable. In this case we won't need function like MDL-31890
          Hide
          mhughes2k Michael Hughes added a comment -

          Marina,

          I'd disagree (and this is just my opinion), from a developer perspective I think its' better separation if each web service method did exactly 1 thing, and certainly I've got more cases for just needing the grades than the grade and grading form. I think it would be better architecturally to layer another method on top of a basic get_grades and and get_grading_form_instances that combines them, and have the two component web services available as well.

          Show
          mhughes2k Michael Hughes added a comment - Marina, I'd disagree (and this is just my opinion), from a developer perspective I think its' better separation if each web service method did exactly 1 thing, and certainly I've got more cases for just needing the grades than the grade and grading form. I think it would be better architecturally to layer another method on top of a basic get_grades and and get_grading_form_instances that combines them, and have the two component web services available as well.
          Hide
          mhughes2k Michael Hughes added a comment -

          Also as any work started on this? As we've got local systems developers who are wanting access to grade information, and if it's not started I'd be happy to start looking at implementing this, otherwise a timescale would be good

          M

          Show
          mhughes2k Michael Hughes added a comment - Also as any work started on this? As we've got local systems developers who are wanting access to grade information, and if it's not started I'd be happy to start looking at implementing this, otherwise a timescale would be good M
          Hide
          bass_rock Daniel Brooks added a comment -

          I too am curious if this has been started otherwise I will look into doing this as well.

          Show
          bass_rock Daniel Brooks added a comment - I too am curious if this has been started otherwise I will look into doing this as well.
          Hide
          mhughes2k Michael Hughes added a comment -

          Has there been any movement on this yet?

          Show
          mhughes2k Michael Hughes added a comment - Has there been any movement on this yet?
          Hide
          andyjdavis Andrew Davis added a comment -

          I've started on this. Here is what I've done so far. https://github.com/andyjdavis/moodle/compare/master...MDL-30085_grade_ws

          Currently all there is is a web service that wraps the lib/gradelib.php method get_grades(). I'm interested to get feedback before I write a bunch more code to make sure I'm doing this correctly.

          Show
          andyjdavis Andrew Davis added a comment - I've started on this. Here is what I've done so far. https://github.com/andyjdavis/moodle/compare/master...MDL-30085_grade_ws Currently all there is is a web service that wraps the lib/gradelib.php method get_grades(). I'm interested to get feedback before I write a bunch more code to make sure I'm doing this correctly.
          Hide
          jerome Jérôme Mouneyrac added a comment -

          Thanks Andrew, I quickly reviewed your draft, I didn't notice anything major:

          • capabilities are only use as text information. Just mention important capabilities that the user require.
          • userids and PARAM_SEQUENCE: even though it is not wrong at all (I find it easy to use), it is not consistent for the rest of the web services. For consistency use external_nultiple_structure(new external_value(PARAM_INT,...
          • we usually validate_context($context) in web service functions
          Show
          jerome Jérôme Mouneyrac added a comment - Thanks Andrew, I quickly reviewed your draft, I didn't notice anything major: capabilities are only use as text information. Just mention important capabilities that the user require. userids and PARAM_SEQUENCE: even though it is not wrong at all (I find it easy to use), it is not consistent for the rest of the web services. For consistency use external_nultiple_structure(new external_value(PARAM_INT,... we usually validate_context($context) in web service functions
          Hide
          andyjdavis Andrew Davis added a comment -

          Putting this up for Jerome/peer review.

          Show
          andyjdavis Andrew Davis added a comment - Putting this up for Jerome/peer review.
          Hide
          jerome Jérôme Mouneyrac added a comment - - edited

          Thanks Andrew. My review of get_grades:

          get_grades_parameters()
          -----------------------

          • I don't think you need to have grades single_structure. All the 'grades' attributs should be the top parameters.
          • iteminstance: it's the first time I see this param name in external function description. It looks to me that we don't have any function returning this info. In that case either (a) we need to return the cm->instance in core_course_get_course_contents() , either (b) we need a new external function to return the module instance, either (c) your function should deal with course module id and retrieve the module instance itself. It needs to be thought carefully. In theory that it is good to reduce the information returned by the Moodle (so the client only deal with one 'id' by forum/database...) which would be the module id. But in reality, I found out it is easier for client devs to know everything (module id + instance id). I would pick (a). What do you think?
          • The above point make me think we need some Moodledocs to list all common used parameters in external API so we can be consistent in the param names. I'll create this doc.
          • userids description is wrong: 'A comma separated list of user IDs or empty to just retrieve grade item information'. If you make the attributs top parameters, then VALUE_OPTIONAL become VALUE_DEFAULT, array()

          get_grades()
          ------------

          • typo: array of grade innformation
          • $USER not used
          • $userids = null; $userids can be null then you do some cout($userids) and other $userids[0]. However as I mentioned, if it's a top parameters, it should be defaulted to array(), so it will never be null. However you still going to do $userids[0] and this will need to be checked.
          • errorcoursecontextnotvalid exception: just a note, here it's pretty obvious but when it's not don't heistate to put some debug info in english for the client developer to debug. For example with nopermissiontoviewgrades exception it could be good to mention to the client dev the three cases, so it doesn't need to look at the code to know why it failed - he got some clue when his customer complains. It's a pretty minor comment and I would not fail integration for that, but always try to put yourself as a client dev using your function.
          • you return straight away from grade_get_grades() core lib function. I'm pretty sure it will not work when you have textaera field like the 'feedback'/'feedbackformat'. You need to do something like:

            // adapt the code with grades/feedback
            list($categoryinfo['description'], $categoryinfo['descriptionformat']) = 
            external_format_text($category->description, $category->descriptionformat,
            $context->id, 'coursecat', 'description', null);
            

          get_grades_returns()
          --------------------

          • I notice you you don't use any VALUE_OPTIONAL meaning that grade_get_grades() must return all these fields otherwise it will fail during validation. For example if grade_get_grades() don't return outcomes, the return value validation will fail. Have a look to MDL-37354 to know how to test the return values.
          • missing characters: "Is the student\s"
          Show
          jerome Jérôme Mouneyrac added a comment - - edited Thanks Andrew. My review of get_grades: get_grades_parameters() ----------------------- I don't think you need to have grades single_structure. All the 'grades' attributs should be the top parameters. iteminstance: it's the first time I see this param name in external function description. It looks to me that we don't have any function returning this info. In that case either (a) we need to return the cm->instance in core_course_get_course_contents() , either (b) we need a new external function to return the module instance, either (c) your function should deal with course module id and retrieve the module instance itself. It needs to be thought carefully. In theory that it is good to reduce the information returned by the Moodle (so the client only deal with one 'id' by forum/database...) which would be the module id. But in reality, I found out it is easier for client devs to know everything (module id + instance id). I would pick (a). What do you think? The above point make me think we need some Moodledocs to list all common used parameters in external API so we can be consistent in the param names. I'll create this doc. userids description is wrong: 'A comma separated list of user IDs or empty to just retrieve grade item information'. If you make the attributs top parameters, then VALUE_OPTIONAL become VALUE_DEFAULT, array() get_grades() ------------ typo: array of grade innformation $USER not used $userids = null; $userids can be null then you do some cout($userids) and other $userids [0] . However as I mentioned, if it's a top parameters, it should be defaulted to array(), so it will never be null. However you still going to do $userids [0] and this will need to be checked. errorcoursecontextnotvalid exception: just a note, here it's pretty obvious but when it's not don't heistate to put some debug info in english for the client developer to debug. For example with nopermissiontoviewgrades exception it could be good to mention to the client dev the three cases, so it doesn't need to look at the code to know why it failed - he got some clue when his customer complains. It's a pretty minor comment and I would not fail integration for that, but always try to put yourself as a client dev using your function. you return straight away from grade_get_grades() core lib function. I'm pretty sure it will not work when you have textaera field like the 'feedback'/'feedbackformat'. You need to do something like: // adapt the code with grades/feedback list($categoryinfo['description'], $categoryinfo['descriptionformat']) = external_format_text($category->description, $category->descriptionformat, $context->id, 'coursecat', 'description', null); get_grades_returns() -------------------- I notice you you don't use any VALUE_OPTIONAL meaning that grade_get_grades() must return all these fields otherwise it will fail during validation. For example if grade_get_grades() don't return outcomes, the return value validation will fail. Have a look to MDL-37354 to know how to test the return values. missing characters: "Is the student\s"
          Hide
          jerome Jérôme Mouneyrac added a comment -

          My review for update_grade:

          update_grade_parameters()
          -----------------------

          • there is not much point to have one parameter that is a external_single_structure, they all could be top parameters, except if you transform the function in "super" bulk.
          • As I mention it, could would it be good to make this function a "super" bulk function? It's just a question I asked myself, you know better than me the "gradebook" use cases. I understand it's already a bulk on the component level, so it may be enough for most uses.
          • 'studentid'/'grade' description are identical: 'Student grade'
          • studentid should be an INT
          • few missing description for example plusfactor...

          update_grade()
          --------------
          Note that I didn't check the matching code logic in Moodle.

          • $USER not used
          • Some not standard coding style:
          • remove commented code: //$transaction = $DB->start_delegated_transaction();
          • many equals in the same line: $hidinggrades = $editinggradeitem = $editinggrades = false;
          • $grades = $itemdetails = null;
          • space uasage in "if ( isset(...))"
          • calling a variable $e
          • all exceptions have the same code error: nopermissiontoviewgrades. It means that the client will not be able to implement code logic to let their customer to know exactly why. Moreover as their is no debug info, without looking at the code, they will not know themself what are the reason. It may be ok as it may not, think how you would manage these exceptions if you were using your function as a client dev.

          update_grade_returns()
          ----------------------

          • I would indicate what's the meaning of the return value. GRADE_UPDATE_OK mean nothing for someone not having access to the source code.

            new external_value(PARAM_INT, GRADE_UPDATE_OK . ' => OK, ' . GRADE_UPDATE_FAILED . ' => FAILED.')

          These reviews looks long but these are not too big issues.

          Thanks Andrew.

          Show
          jerome Jérôme Mouneyrac added a comment - My review for update_grade: update_grade_parameters() ----------------------- there is not much point to have one parameter that is a external_single_structure, they all could be top parameters, except if you transform the function in "super" bulk. As I mention it, could would it be good to make this function a "super" bulk function? It's just a question I asked myself, you know better than me the "gradebook" use cases. I understand it's already a bulk on the component level, so it may be enough for most uses. 'studentid'/'grade' description are identical: 'Student grade' studentid should be an INT few missing description for example plusfactor... update_grade() -------------- Note that I didn't check the matching code logic in Moodle. $USER not used Some not standard coding style: remove commented code: //$transaction = $DB->start_delegated_transaction(); many equals in the same line: $hidinggrades = $editinggradeitem = $editinggrades = false; $grades = $itemdetails = null; space uasage in "if ( isset(...))" calling a variable $e all exceptions have the same code error: nopermissiontoviewgrades. It means that the client will not be able to implement code logic to let their customer to know exactly why. Moreover as their is no debug info, without looking at the code, they will not know themself what are the reason. It may be ok as it may not, think how you would manage these exceptions if you were using your function as a client dev. update_grade_returns() ---------------------- I would indicate what's the meaning of the return value. GRADE_UPDATE_OK mean nothing for someone not having access to the source code. new external_value(PARAM_INT, GRADE_UPDATE_OK . ' => OK, ' . GRADE_UPDATE_FAILED . ' => FAILED.') These reviews looks long but these are not too big issues. Thanks Andrew.
          Hide
          jerome Jérôme Mouneyrac added a comment -

          Ah I forgot, you need to bump /version.php, so the integrator knows he needs to bump it.

          Show
          jerome Jérôme Mouneyrac added a comment - Ah I forgot, you need to bump /version.php, so the integrator knows he needs to bump it.
          Hide
          jleyva Juan Leyva added a comment -

          Just a few comments,

          I'm using this code for creating a demo plugin for the new Moodle Mobile app, see http://docs.moodle.org/dev/Moodle_Mobile_Developing_a_plugin_tutorial#Plugin_spec

          I've noticed some issues:

          • Invalid return parameters type, some PARAM_INT should be PARAM_FLOAT
          • The data structure returned by get_grades contains both objects and arrays, you have to convert all the objects to key-value arrays
          • One of the parameters required by get_grades is the instanceid, I think that it should be changed to course module id (cmid) currently in Moodle there are no WebServices for getting course modules containing the instance id, the only Web Service that returns valid information regarding modules is course_get_contents that returns the mod type and the cmid.
          • I don't like the current input parameters declaration for get_grades, it forces this type of parameters in REST:
            grades[courseid], grades[userids][0]
            Why don't just avoid the initial grades - single structure declaration? I think that it doesn't make sense, the most natural for me is having just 4 parameters and not 1 parameter that is a structure.

          I made quick fixes (not optimal) for some of those issues.
          You can see my version here:
          https://github.com/cvaconsulting/moodle-local_custommm

          Show
          jleyva Juan Leyva added a comment - Just a few comments, I'm using this code for creating a demo plugin for the new Moodle Mobile app, see http://docs.moodle.org/dev/Moodle_Mobile_Developing_a_plugin_tutorial#Plugin_spec I've noticed some issues: Invalid return parameters type, some PARAM_INT should be PARAM_FLOAT The data structure returned by get_grades contains both objects and arrays, you have to convert all the objects to key-value arrays One of the parameters required by get_grades is the instanceid, I think that it should be changed to course module id (cmid) currently in Moodle there are no WebServices for getting course modules containing the instance id, the only Web Service that returns valid information regarding modules is course_get_contents that returns the mod type and the cmid. I don't like the current input parameters declaration for get_grades, it forces this type of parameters in REST: grades [courseid] , grades [userids] [0] Why don't just avoid the initial grades - single structure declaration? I think that it doesn't make sense, the most natural for me is having just 4 parameters and not 1 parameter that is a structure. I made quick fixes (not optimal) for some of those issues. You can see my version here: https://github.com/cvaconsulting/moodle-local_custommm
          Hide
          andyjdavis Andrew Davis added a comment -

          Hi. I've dealt with all the items Jerome raised against "get_grades". Next up is Jerome's comments on update_grade and Juan's items.

          Show
          andyjdavis Andrew Davis added a comment - Hi. I've dealt with all the items Jerome raised against "get_grades". Next up is Jerome's comments on update_grade and Juan's items.
          Hide
          renep Rene P added a comment -

          I wanted to create new Web Service function: core_grade_get_grade_items and ticket was closed as this ticket was under development. Now the issue why I can't wait on this function to get ready is even being finished, it doesn't fulfil my needs as core_grade_xxx_grades() has itemtype, iteminstance and itemmodule as required parameters. But all I need is get all grade items by courseid.

          So I was asked to copy my text from MDL-37667 to here.

          Parameters: itemtype, itemmodule and iteminstance are required parameters for grade_get_grades. If they are null, then SQL WHERE clause will be: "itemtype IS NULL AND itemmodule IS NULL AND iteminstance IS NULL AND courseid = 4" therefore SQL returns empty. Basically what I want is get all grade_items (grading schemes) by courseid only with same returns of: id, courseid, grademax, gradepass, aggregationcoef. core_grade_get_grades() is very specific and foreign application needs to know all parameters in order to work.

          There is also another way to fix the issue here: make everything besides $courseid as optional parameter. This means gradelib would need to be modified and I don't know how much would you like to do that. Here is an example of what I mean:

          $params = array();
          if(!empty($itemtype)) { 
              $params['itemtype] = $itemtype;
          }
           
          if ($grade_items = grade_item::fetch_all($params)) {

          Show
          renep Rene P added a comment - I wanted to create new Web Service function: core_grade_get_grade_items and ticket was closed as this ticket was under development. Now the issue why I can't wait on this function to get ready is even being finished, it doesn't fulfil my needs as core_grade_xxx_grades() has itemtype, iteminstance and itemmodule as required parameters. But all I need is get all grade items by courseid. So I was asked to copy my text from MDL-37667 to here. Parameters: itemtype , itemmodule and iteminstance are required parameters for grade_get_grades . If they are null, then SQL WHERE clause will be: "itemtype IS NULL AND itemmodule IS NULL AND iteminstance IS NULL AND courseid = 4" therefore SQL returns empty. Basically what I want is get all grade_items (grading schemes) by courseid only with same returns of: id, courseid, grademax, gradepass, aggregationcoef. core_grade_get_grades() is very specific and foreign application needs to know all parameters in order to work. There is also another way to fix the issue here: make everything besides $courseid as optional parameter. This means gradelib would need to be modified and I don't know how much would you like to do that. Here is an example of what I mean: $params = array(); if(!empty($itemtype)) { $params['itemtype] = $itemtype; }   if ($grade_items = grade_item::fetch_all($params)) {
          Hide
          jerome Jérôme Mouneyrac added a comment -

          Note: Don't forget to add your function to the WS API list: http://docs.moodle.org/dev/Web_services_Roadmap#Core_web_service_functions (this is an automatic message)

          Show
          jerome Jérôme Mouneyrac added a comment - Note: Don't forget to add your function to the WS API list: http://docs.moodle.org/dev/Web_services_Roadmap#Core_web_service_functions (this is an automatic message)
          Hide
          andyjdavis Andrew Davis added a comment -

          I'm through all the items from Jerome. Juan and Rene, I'll be moving onto your items shortly.

          I haven't yet bumped version.php. I'll do that right at the end to avoid git conflicts.

          Show
          andyjdavis Andrew Davis added a comment - I'm through all the items from Jerome. Juan and Rene, I'll be moving onto your items shortly. I haven't yet bumped version.php. I'll do that right at the end to avoid git conflicts.
          Hide
          andyjdavis Andrew Davis added a comment -

          This is still progressing. I've updated the branch. Right now I have two things to do.

          1) I'm having troubles getting outcomes to work. I suspect this is more a problem with my unit test code than with the code being tested. I'm not sure I'm inserting the outcome grade item correctly.

          2) As Rene described it would be great to be able to bulk fetch grade items rather than forcing client code to fetch them one by one. This should be do-able.

          I have unit tests for both outcomes and bulk grade item fetching commented out. Once they pass I think we're done.

          Show
          andyjdavis Andrew Davis added a comment - This is still progressing. I've updated the branch. Right now I have two things to do. 1) I'm having troubles getting outcomes to work. I suspect this is more a problem with my unit test code than with the code being tested. I'm not sure I'm inserting the outcome grade item correctly. 2) As Rene described it would be great to be able to bulk fetch grade items rather than forcing client code to fetch them one by one. This should be do-able. I have unit tests for both outcomes and bulk grade item fetching commented out. Once they pass I think we're done.
          Hide
          andyjdavis Andrew Davis added a comment -

          Putting this up for peer review. I've added support for bulk fetching grade items. Note that this change affects both the grade webservice and the core grade API so I suspect that there will be a few conversations about this change before it gets integrated.

          Show
          andyjdavis Andrew Davis added a comment - Putting this up for peer review. I've added support for bulk fetching grade items. Note that this change affects both the grade webservice and the core grade API so I suspect that there will be a few conversations about this change before it gets integrated.
          Hide
          andyjdavis Andrew Davis added a comment -

          Of course this should not be deployed to anyone's production environment however if anyone has a test environment and plans on using the grade web service I am really interested to know if what we have will meet your needs.

          Show
          andyjdavis Andrew Davis added a comment - Of course this should not be deployed to anyone's production environment however if anyone has a test environment and plans on using the grade web service I am really interested to know if what we have will meet your needs.
          Hide
          jerome Jérôme Mouneyrac added a comment -

          I quickly read your last two commits I didn't notice anything wrong on the web service basis. You can send to integration. Thanks Andrew.

          Show
          jerome Jérôme Mouneyrac added a comment - I quickly read your last two commits I didn't notice anything wrong on the web service basis. You can send to integration. Thanks Andrew.
          Hide
          damyon Damyon Wiese added a comment -

          Hi Andrew,

          There are quite a few comments here but the bulk of the code is good this so these changes should be fairly easy/quick. I'll put letters so you can keep track of the issues I raise:

          A) This webservice is using the module context for external_format_text - I think this is incorrect - it should be the course context as the gradebook belongs to the course not the module.

          The gradebook itself is not passing context to format_string and friends when it should be - but that is an existing bug.

          B) You are not handling hidden grades or the capability to view hidden grades.

          C) Juan has said that you should be using cmid - not instance and you have not responded to this - the current code still uses instance.

          D) There are also whitespace errors and long lines all through this patch - I would fix them for you - but as there are other issues it would be nice if you looked at this too.

          E) I do agree that this is not the correct function to return entire grading forms - that makes the response to complicated (if this, expect this, etc).

          F) I have concerns about the performance of this method - just noting this here but it will probably be expensive to load all the grades for a large course.

          G) You cannot continue testing after catching an exception within php unit - you need to break your test down into smaller tests.

          H) The change to gradelib looks ok - but needs documentation in the phpdoc at least.

          I) Your return from get_grades uses userids as the keys of the array in an external_multiple_structure - it shouldn't - that wont necessarily be supported by all webservice clients. You should change the return structure to include the userid in the grade structure. (Your outcomes array has the same issue).

          Thanks, Damyon

          Show
          damyon Damyon Wiese added a comment - Hi Andrew, There are quite a few comments here but the bulk of the code is good this so these changes should be fairly easy/quick. I'll put letters so you can keep track of the issues I raise: A) This webservice is using the module context for external_format_text - I think this is incorrect - it should be the course context as the gradebook belongs to the course not the module. The gradebook itself is not passing context to format_string and friends when it should be - but that is an existing bug. B) You are not handling hidden grades or the capability to view hidden grades. C) Juan has said that you should be using cmid - not instance and you have not responded to this - the current code still uses instance. D) There are also whitespace errors and long lines all through this patch - I would fix them for you - but as there are other issues it would be nice if you looked at this too. E) I do agree that this is not the correct function to return entire grading forms - that makes the response to complicated (if this, expect this, etc). F) I have concerns about the performance of this method - just noting this here but it will probably be expensive to load all the grades for a large course. G) You cannot continue testing after catching an exception within php unit - you need to break your test down into smaller tests. H) The change to gradelib looks ok - but needs documentation in the phpdoc at least. I) Your return from get_grades uses userids as the keys of the array in an external_multiple_structure - it shouldn't - that wont necessarily be supported by all webservice clients. You should change the return structure to include the userid in the grade structure. (Your outcomes array has the same issue). Thanks, Damyon
          Hide
          andyjdavis Andrew Davis added a comment -

          A) I think using the module context is correct although I'm not 100% and using the course context would certainly simplify thing. Looking at the phpdocs for external_format_text() it sounds like we should be using the module context. I think...

          @param int $contextid This parameter and the next two identify the file area to use.
          @param string $component

          B) Wow, well spotted. Fixing now.

          C) The code is actually using cmid. Its just that the variable names and descriptions are a bit misleading. After looking at get_course_contents_returns() I've switched from calling it instance id to activity id as this is more like the names get_course_contents_returns() uses.

          Show
          andyjdavis Andrew Davis added a comment - A) I think using the module context is correct although I'm not 100% and using the course context would certainly simplify thing. Looking at the phpdocs for external_format_text() it sounds like we should be using the module context. I think... @param int $contextid This parameter and the next two identify the file area to use. @param string $component B) Wow, well spotted. Fixing now. C) The code is actually using cmid. Its just that the variable names and descriptions are a bit misleading. After looking at get_course_contents_returns() I've switched from calling it instance id to activity id as this is more like the names get_course_contents_returns() uses.
          Hide
          andyjdavis Andrew Davis added a comment -

          F) Yes it could be quite expensive loading all the grades however it will be less expensive than, for someone who genuinely needs all the grades in a course, calling get course contents then individually loading each grade item one after the other. It could however be easy for people to shoot themselves in the foot if the start requesting a bunch of data they don't actually need.

          G) I believe that if you call setExpectedException() and allow the exception to be caught by phpunit then you are correct that you cannot run any further tests within that test method. However, if you catch the exceptions yourself you can continue.

          H) Will do. I've uncovered some quirks to do with bulk fetching but I'll get them ironed out and documented and we'll see how it looks.

          Show
          andyjdavis Andrew Davis added a comment - F) Yes it could be quite expensive loading all the grades however it will be less expensive than, for someone who genuinely needs all the grades in a course, calling get course contents then individually loading each grade item one after the other. It could however be easy for people to shoot themselves in the foot if the start requesting a bunch of data they don't actually need. G) I believe that if you call setExpectedException() and allow the exception to be caught by phpunit then you are correct that you cannot run any further tests within that test method. However, if you catch the exceptions yourself you can continue. H) Will do. I've uncovered some quirks to do with bulk fetching but I'll get them ironed out and documented and we'll see how it looks.
          Hide
          damyon Damyon Wiese added a comment -

          No problem Andrew,

          I'll pull this out of integration for this week - feel free to resubmit when your happy.

          on G) I think I have seen problems with this and transactions so your code may be Ok - if your happy it works I'll recheck at integration.

          Show
          damyon Damyon Wiese added a comment - No problem Andrew, I'll pull this out of integration for this week - feel free to resubmit when your happy. on G) I think I have seen problems with this and transactions so your code may be Ok - if your happy it works I'll recheck at integration.
          Hide
          cibot CiBoT added a comment -

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

          Show
          cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          renep Rene P added a comment - - edited

          Thank you for making modifications as I previously requested, but I would like to comment on couple of things:
          1) This can't work with version < 2.4, because function get_fast_modinfo() needed course as parameter not course or course->id
          2) If somebody still tries to implement this on 2.2, then besides first problem, you need to add external_format_text (function) and external_settings (class) into docroot/lib/externallib.php
          3) I've noticed that grade_items are inserted into array with itemnumber as index and you can't get course totals with this function. Why is that?

          Otherwise it seems that I have gotten this to work on Moodle 2.2 and besides not getting all grade_items (due to itemnumber) and course totals, it works perfectly.

          Just out of curiosity: why externallib.php goes to docroot/lib/grade not docroot/grade/ and also, do you have any thoughts by what time is this function code finalized?

          Show
          renep Rene P added a comment - - edited Thank you for making modifications as I previously requested, but I would like to comment on couple of things: 1) This can't work with version < 2.4, because function get_fast_modinfo() needed course as parameter not course or course->id 2) If somebody still tries to implement this on 2.2, then besides first problem, you need to add external_format_text (function) and external_settings (class) into docroot/lib/externallib.php 3) I've noticed that grade_items are inserted into array with itemnumber as index and you can't get course totals with this function. Why is that? Otherwise it seems that I have gotten this to work on Moodle 2.2 and besides not getting all grade_items (due to itemnumber) and course totals, it works perfectly. Just out of curiosity: why externallib.php goes to docroot/lib/grade not docroot/grade/ and also, do you have any thoughts by what time is this function code finalized?
          Hide
          andyjdavis Andrew Davis added a comment - - edited

          Ok, I think I've dealt with all of this. I'll go through a few items specifically.

          I) Your return from get_grades uses userids as the keys of the array in an external_multiple_structure - it shouldn't - that wont necessarily be supported by all webservice clients. You should change the return structure to include the userid in the grade structure. (Your outcomes array has the same issue).

          I find this limitation a bit odd but it is done all the same.

          I've noticed that grade_items are inserted into array with itemnumber as index and you can't get course totals with this function. Why is that?

          This was an oversight on my part. Among the activites you will now find a grade item with key 'course' that is the course total.

          Show
          andyjdavis Andrew Davis added a comment - - edited Ok, I think I've dealt with all of this. I'll go through a few items specifically. I) Your return from get_grades uses userids as the keys of the array in an external_multiple_structure - it shouldn't - that wont necessarily be supported by all webservice clients. You should change the return structure to include the userid in the grade structure. (Your outcomes array has the same issue). I find this limitation a bit odd but it is done all the same. I've noticed that grade_items are inserted into array with itemnumber as index and you can't get course totals with this function. Why is that? This was an oversight on my part. Among the activites you will now find a grade item with key 'course' that is the course total.
          Hide
          andyjdavis Andrew Davis added a comment -

          ping.

          Show
          andyjdavis Andrew Davis added a comment - ping.
          Hide
          andyjdavis Andrew Davis added a comment -

          Rebased and resolved conflicts.

          Show
          andyjdavis Andrew Davis added a comment - Rebased and resolved conflicts.
          Hide
          jerome Jérôme Mouneyrac added a comment - - edited

          Thanks Andrew,

          • 'activityid' => new external_format_value(PARAM_INT, 'The activity ID', VALUE_OPTIONAL),
            

            it just needs to be external_value.

          • if I remember well VALUE_OPTIONAL can not be used as parameter top level (for example in get_grades_parameters(): 'component' and 'activityid'). If you look in webservices/lib.php:get_virtual_method_code() it should throw an exception for Zend servers (SOAP/XMLRPC). Instead of VALUE_OPTIONAL, use VALUE_DEFAULT. The default value should be the ones matching the main function declaration.
          • (optional) it is better practice to use the validated parameters that you put in $params as in

             $params = self::validate_parameters(self::get_grades_parameters(),
            +            array('courseid' => $courseid, 'component' => $component, 'activityid' => $activityid, 'userids' => $userids));
            

            You want to use $params['courseid'] which has been validated/cleaned instead the original $courseid. It does not make a big difference, as validate_parameters() would throw an exception on error, but it's a good pratice in Moodle web service to use the result of validate_parameters() - it's also good if one day something change in validate_parameters and doesn't throw exception.

          • (optional) a trivial word about exception and web services and ws clients.

            throw new moodle_exception('errorcoursecontextnotvalid' , 'webservice', '', $exceptionparam);
            

            When a core server returns an exception, it returns the error code. Here the error code is 'errorcoursecontextnotvalid'. Then the client can generate specific against the error code. The translated message and the debuginfo are also returned by the ws server. It is common practice to write some debug info in english (not translated) as it's sometimes more helpful than the translated exception message for the client developer.

          From the web service review structure that's all. You can send it to integration when the mandatory points are fixed everywhere..

          Show
          jerome Jérôme Mouneyrac added a comment - - edited Thanks Andrew, 'activityid' => new external_format_value(PARAM_INT, 'The activity ID', VALUE_OPTIONAL), it just needs to be external_value. if I remember well VALUE_OPTIONAL can not be used as parameter top level (for example in get_grades_parameters(): 'component' and 'activityid'). If you look in webservices/lib.php:get_virtual_method_code() it should throw an exception for Zend servers (SOAP/XMLRPC). Instead of VALUE_OPTIONAL, use VALUE_DEFAULT. The default value should be the ones matching the main function declaration. (optional) it is better practice to use the validated parameters that you put in $params as in $params = self::validate_parameters(self::get_grades_parameters(), + array('courseid' => $courseid, 'component' => $component, 'activityid' => $activityid, 'userids' => $userids)); You want to use $params ['courseid'] which has been validated/cleaned instead the original $courseid. It does not make a big difference, as validate_parameters() would throw an exception on error, but it's a good pratice in Moodle web service to use the result of validate_parameters() - it's also good if one day something change in validate_parameters and doesn't throw exception. (optional) a trivial word about exception and web services and ws clients. throw new moodle_exception('errorcoursecontextnotvalid' , 'webservice', '', $exceptionparam); When a core server returns an exception, it returns the error code. Here the error code is 'errorcoursecontextnotvalid'. Then the client can generate specific against the error code. The translated message and the debuginfo are also returned by the ws server. It is common practice to write some debug info in english (not translated) as it's sometimes more helpful than the translated exception message for the client developer. in your PHPunit tests don't forget to clean_returnvalue() like indicated into: http://docs.moodle.org/dev/Web_Services_Unit_Test From the web service review structure that's all. You can send it to integration when the mandatory points are fixed everywhere..
          Hide
          andyjdavis Andrew Davis added a comment -

          I believe I've fixed all of this except for one point. Calling clean_returnvalue() appears to "lose" array keys. For example if you request the grades for several activities using their $cm IDs. They are the keys in the returned array.

          Before
          array(1) {
            [1] =>
            array(13) {
              .
              .
              .
            }
          }
           
          After
          array(1) {
            [0] =>
            array(9) {
              .
              .
              .
            }
          }

          I'd prefer to retain the keys. If you are loading the grades for 300 students having to loop to find a specific student would not be very nice. Is this a genuine problem or actually just a bug in the behaviour of clean_returnvalue()?

          Show
          andyjdavis Andrew Davis added a comment - I believe I've fixed all of this except for one point. Calling clean_returnvalue() appears to "lose" array keys. For example if you request the grades for several activities using their $cm IDs. They are the keys in the returned array. Before array(1) { [1] => array(13) { . . . } }   After array(1) { [0] => array(9) { . . . } } I'd prefer to retain the keys. If you are loading the grades for 300 students having to loop to find a specific student would not be very nice. Is this a genuine problem or actually just a bug in the behaviour of clean_returnvalue()?
          Hide
          jerome Jérôme Mouneyrac added a comment - - edited

          Hi Andrew, no worries, it's not a bug. You may have an incomplete return values description function (i.e xxx_returns). The clean_returnvalue() function removes all not described information. It's to avoid that web service functions leak information. Leak could happen, for example, if an external function returns a $user set directly from a $DB call - it won't be good to send the password by error. As a developer, to not forget any return value attributs, I recommend to explicitly build your return value into the external function (it's not mandatory thought).

          Show
          jerome Jérôme Mouneyrac added a comment - - edited Hi Andrew, no worries, it's not a bug. You may have an incomplete return values description function (i.e xxx_returns). The clean_returnvalue() function removes all not described information. It's to avoid that web service functions leak information. Leak could happen, for example, if an external function returns a $user set directly from a $DB call - it won't be good to send the password by error. As a developer, to not forget any return value attributs, I recommend to explicitly build your return value into the external function (it's not mandatory thought).
          Hide
          andyjdavis Andrew Davis added a comment -

          Sorry, I think my code sample is unclear. The problem is the index switching from [1] => to [0] =>. The removal of the fields within the nested array is fine.

          Show
          andyjdavis Andrew Davis added a comment - Sorry, I think my code sample is unclear. The problem is the index switching from [1] => to [0] =>. The removal of the fields within the nested array is fine.
          Hide
          damyon Damyon Wiese added a comment -

          I don't think all protocols/clients supported named arrays. You should add the index to each object instead.

          Show
          damyon Damyon Wiese added a comment - I don't think all protocols/clients supported named arrays. You should add the index to each object instead.
          Hide
          jerome Jérôme Mouneyrac added a comment -

          Sorry Andrew for reading too quickly At "lose array keys" I deduced you had the problem I commented about. As Daymon says, the ws servers ignore the keys for multiple_structure (a list entity), and they check the key for single_structure (an object entity).

          Show
          jerome Jérôme Mouneyrac added a comment - Sorry Andrew for reading too quickly At "lose array keys" I deduced you had the problem I commented about. As Daymon says, the ws servers ignore the keys for multiple_structure (a list entity), and they check the key for single_structure (an object entity).
          Hide
          andyjdavis Andrew Davis added a comment -

          I believe this is now all fixed. Submitting for integration.

          Show
          andyjdavis Andrew Davis added a comment - I believe this is now all fixed. Submitting for integration.
          Hide
          poltawski Dan Poltawski added a comment -

          Taking this out of integration for this week, as we are still are in the on sync period. Thanks

          Show
          poltawski Dan Poltawski added a comment - Taking this out of integration for this week, as we are still are in the on sync period. Thanks
          Hide
          poltawski Dan Poltawski added a comment -

          Hi Andrew,

          I haven't looked at this issue in any detail at all, but I just pulled the code and ran phpunit:

          phpunit
          Moodle 2.6dev (Build: 20130606), pgsql, f4d0ced8c805af3a4ae5747fe66e5a905e9014ce
          PHP Fatal error:  Cannot redeclare class core_grade_external_testcase in /Users/danp/git/integration/grade/tests/externallib_test.php on line 184
          PHP Stack trace:
          PHP   1. {main}() /Users/danp/git/integration/vendor/phpunit/phpunit/composer/bin/phpunit:0
          PHP   2. PHPUnit_TextUI_Command::main() /Users/danp/git/integration/vendor/phpunit/phpunit/composer/bin/phpunit:63
          PHP   3. PHPUnit_TextUI_Command->run() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/TextUI/Command.php:129
          PHP   4. PHPUnit_TextUI_Command->handleArguments() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/TextUI/Command.php:138
          PHP   5. PHPUnit_Util_Configuration->getTestSuiteConfiguration() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/TextUI/Command.php:657
          PHP   6. PHPUnit_Util_Configuration->getTestSuite() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/Util/Configuration.php:797
          PHP   7. PHPUnit_Framework_TestSuite->addTestFiles() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/Util/Configuration.php:873
          PHP   8. PHPUnit_Framework_TestSuite->addTestFile() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php:416
          PHP   9. PHPUnit_Util_Fileloader::checkAndLoad() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php:355
          PHP  10. PHPUnit_Util_Fileloader::load() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/Util/Fileloader.php:76
           
          Fatal error: Cannot redeclare class core_grade_external_testcase in /Users/danp/git/integration/grade/tests/externallib_test.php on line 184
           
          Call Stack:
              0.0018     228768   1. {main}() /Users/danp/git/integration/vendor/phpunit/phpunit/composer/bin/phpunit:0
              0.0234     538744   2. PHPUnit_TextUI_Command::main() /Users/danp/git/integration/vendor/phpunit/phpunit/composer/bin/phpunit:63
              0.0234     538976   3. PHPUnit_TextUI_Command->run() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/TextUI/Command.php:129
              0.0234     539408   4. PHPUnit_TextUI_Command->handleArguments() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/TextUI/Command.php:138
              2.7512   48829880   5. PHPUnit_Util_Configuration->getTestSuiteConfiguration() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/TextUI/Command.php:657
              3.9411   89971688   6. PHPUnit_Util_Configuration->getTestSuite() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/Util/Configuration.php:797
              4.0121   91850496   7. PHPUnit_Framework_TestSuite->addTestFiles() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/Util/Configuration.php:873
              4.0224   92333888   8. PHPUnit_Framework_TestSuite->addTestFile() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php:416
              4.0228   92337688   9. PHPUnit_Util_Fileloader::checkAndLoad() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php:355
              4.0228   92337816  10. PHPUnit_Util_Fileloader::load() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/Util/Fileloader.php:76
          
          

          Show
          poltawski Dan Poltawski added a comment - Hi Andrew, I haven't looked at this issue in any detail at all, but I just pulled the code and ran phpunit: phpunit Moodle 2.6dev (Build: 20130606), pgsql, f4d0ced8c805af3a4ae5747fe66e5a905e9014ce PHP Fatal error: Cannot redeclare class core_grade_external_testcase in /Users/danp/git/integration/grade/tests/externallib_test.php on line 184 PHP Stack trace: PHP 1. {main}() /Users/danp/git/integration/vendor/phpunit/phpunit/composer/bin/phpunit:0 PHP 2. PHPUnit_TextUI_Command::main() /Users/danp/git/integration/vendor/phpunit/phpunit/composer/bin/phpunit:63 PHP 3. PHPUnit_TextUI_Command->run() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/TextUI/Command.php:129 PHP 4. PHPUnit_TextUI_Command->handleArguments() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/TextUI/Command.php:138 PHP 5. PHPUnit_Util_Configuration->getTestSuiteConfiguration() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/TextUI/Command.php:657 PHP 6. PHPUnit_Util_Configuration->getTestSuite() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/Util/Configuration.php:797 PHP 7. PHPUnit_Framework_TestSuite->addTestFiles() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/Util/Configuration.php:873 PHP 8. PHPUnit_Framework_TestSuite->addTestFile() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php:416 PHP 9. PHPUnit_Util_Fileloader::checkAndLoad() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php:355 PHP 10. PHPUnit_Util_Fileloader::load() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/Util/Fileloader.php:76   Fatal error: Cannot redeclare class core_grade_external_testcase in /Users/danp/git/integration/grade/tests/externallib_test.php on line 184   Call Stack: 0.0018 228768 1. {main}() /Users/danp/git/integration/vendor/phpunit/phpunit/composer/bin/phpunit:0 0.0234 538744 2. PHPUnit_TextUI_Command::main() /Users/danp/git/integration/vendor/phpunit/phpunit/composer/bin/phpunit:63 0.0234 538976 3. PHPUnit_TextUI_Command->run() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/TextUI/Command.php:129 0.0234 539408 4. PHPUnit_TextUI_Command->handleArguments() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/TextUI/Command.php:138 2.7512 48829880 5. PHPUnit_Util_Configuration->getTestSuiteConfiguration() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/TextUI/Command.php:657 3.9411 89971688 6. PHPUnit_Util_Configuration->getTestSuite() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/Util/Configuration.php:797 4.0121 91850496 7. PHPUnit_Framework_TestSuite->addTestFiles() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/Util/Configuration.php:873 4.0224 92333888 8. PHPUnit_Framework_TestSuite->addTestFile() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php:416 4.0228 92337688 9. PHPUnit_Util_Fileloader::checkAndLoad() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php:355 4.0228 92337816 10. PHPUnit_Util_Fileloader::load() /Users/danp/git/integration/vendor/phpunit/phpunit/PHPUnit/Util/Fileloader.php:76
          Hide
          cibot CiBoT added a comment -

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

          Show
          cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          andyjdavis Andrew Davis added a comment - - edited

          I've added a second commit that resolves that error. The problem is that we've code an external API that is dealing with grading but which has been claiming to be grades (grading and grades at two separate but similarly named parts of Moodle).

          2.5 introduced a webservice that is, as far as I can tell, for core_grading. However, it says that it's core_grade. That mis-naming caused the class redeclaration error.

          I've fixed it up but afaik the erroneously named web service has been released as part of 2.5. I'm not sure if this needs some sort of special process. Is it just a matter of adding a note to the release notes?

          See the 2nd commit in isolation to see what Im talking about. https://github.com/andyjdavis/moodle/commit/c20def57388a67eb5597baccea1f16707f391b97

          Show
          andyjdavis Andrew Davis added a comment - - edited I've added a second commit that resolves that error. The problem is that we've code an external API that is dealing with grading but which has been claiming to be grades (grading and grades at two separate but similarly named parts of Moodle). 2.5 introduced a webservice that is, as far as I can tell, for core_grading. However, it says that it's core_grade. That mis-naming caused the class redeclaration error. I've fixed it up but afaik the erroneously named web service has been released as part of 2.5. I'm not sure if this needs some sort of special process. Is it just a matter of adding a note to the release notes? See the 2nd commit in isolation to see what Im talking about. https://github.com/andyjdavis/moodle/commit/c20def57388a67eb5597baccea1f16707f391b97
          Hide
          damyon Damyon Wiese added a comment -

          Yuk - (the existing wrong named webservice). I think all we can do here is rename the webservice and document it in the upgrade.txt (So can you please add this to the patch?).

          Also all of the "@since Moodle 2.5" should be 2.6.

          The rest of the patch looks good (just running unit tests now).

          Show
          damyon Damyon Wiese added a comment - Yuk - (the existing wrong named webservice). I think all we can do here is rename the webservice and document it in the upgrade.txt (So can you please add this to the patch?). Also all of the "@since Moodle 2.5" should be 2.6. The rest of the patch looks good (just running unit tests now).
          Hide
          damyon Damyon Wiese added a comment -

          Tests are passing - once the upgrade.txt and comments are fixed this patch looks ready to me.

          Thanks Andrew

          Show
          damyon Damyon Wiese added a comment - Tests are passing - once the upgrade.txt and comments are fixed this patch looks ready to me. Thanks Andrew
          Hide
          pcharsle Paul Charsley added a comment -

          The incorrectly named function core_grade_get_definitions that should be core_grading_get_definitions is my fault. Sorry, that shouldn't have slipped through. I will ensure that MDL-31890 is also renamed and will update the moodle docs.

          Paul

          Show
          pcharsle Paul Charsley added a comment - The incorrectly named function core_grade_get_definitions that should be core_grading_get_definitions is my fault. Sorry, that shouldn't have slipped through. I will ensure that MDL-31890 is also renamed and will update the moodle docs. Paul
          Hide
          marina Marina Glancy added a comment -

          I'm reopening this issue since it needs one more commit upon Damyon's review and Andrew said it's ok to delay till the next integration cycle. Thanks

          Show
          marina Marina Glancy added a comment - I'm reopening this issue since it needs one more commit upon Damyon's review and Andrew said it's ok to delay till the next integration cycle. Thanks
          Hide
          cibot CiBoT added a comment -

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

          Show
          cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          andyjdavis Andrew Davis added a comment -

          I've added another commit that fixes up the phpdocs version numbers and adds an upgrade.txt. The three commits can either be left as they are or squished into one.

          Show
          andyjdavis Andrew Davis added a comment - I've added another commit that fixes up the phpdocs version numbers and adds an upgrade.txt. The three commits can either be left as they are or squished into one.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Unless I'm wrong, the classpath for the new 2 functions is incorrect (should be lib/...):

          https://github.com/andyjdavis/moodle/compare/master...MDL-30085_grade_ws#L4R102

          Also, please, when creating new WS functions... always add some complete execution (aka, instructions for configuring the server and use some of the available clients/provide one to verify functions are working). That would help discovering problems like this IMO. It's nice to have unit tests but they don't test the whole WS stack.

          That or let's aim to create some client able to be executed in the CI server continuously. That would be really, really ideal.

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Unless I'm wrong, the classpath for the new 2 functions is incorrect (should be lib/...): https://github.com/andyjdavis/moodle/compare/master...MDL-30085_grade_ws#L4R102 Also, please, when creating new WS functions... always add some complete execution (aka, instructions for configuring the server and use some of the available clients/provide one to verify functions are working). That would help discovering problems like this IMO. It's nice to have unit tests but they don't test the whole WS stack. That or let's aim to create some client able to be executed in the CI server continuously. That would be really, really ideal. Ciao
          Hide
          cibot CiBoT added a comment -

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

          Show
          cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          andyjdavis Andrew Davis added a comment -

          Ok, I've fixed the classpath plus some more bits and pieces. I've also added some more to the testing instructions. Something on the CI server testing the webservices would certainly be nice.

          Show
          andyjdavis Andrew Davis added a comment - Ok, I've fixed the classpath plus some more bits and pieces. I've also added some more to the testing instructions. Something on the CI server testing the webservices would certainly be nice.
          Hide
          samhemelryk Sam Hemelryk added a comment -

          This looks good thanks Andrew, there is one thing I want to check with Jerome when he gets online: you've changed core_grade_external => core_grading_external but not provided an deprecated method as has been done with other web servcies that have been renamed. I just want to check with Jerome and find out what the policy is there.

          Show
          samhemelryk Sam Hemelryk added a comment - This looks good thanks Andrew, there is one thing I want to check with Jerome when he gets online: you've changed core_grade_external => core_grading_external but not provided an deprecated method as has been done with other web servcies that have been renamed. I just want to check with Jerome and find out what the policy is there.
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Hi Andrew,

          I've just spoken to Jerome and have confirmed that we will need to create a "deprecated" service with the old name that maps to the new name.
          Could you please do that and ping me when done, I'll keep this in integration review until then if possible.

          Cheers
          Sam

          Show
          samhemelryk Sam Hemelryk added a comment - Hi Andrew, I've just spoken to Jerome and have confirmed that we will need to create a "deprecated" service with the old name that maps to the new name. Could you please do that and ping me when done, I'll keep this in integration review until then if possible. Cheers Sam
          Hide
          samhemelryk Sam Hemelryk added a comment -

          (have a look at what we did when we renamed the classes from moodle_grading => core_grading)

          Show
          samhemelryk Sam Hemelryk added a comment - (have a look at what we did when we renamed the classes from moodle_grading => core_grading)
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Reopening this sorry Andrew as we're getting ready for testing now.

          Show
          samhemelryk Sam Hemelryk added a comment - Reopening this sorry Andrew as we're getting ready for testing now.
          Hide
          cibot CiBoT added a comment -

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

          Show
          cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          jleyva Juan Leyva added a comment - - edited

          Hi,

          MDL-31890 included the deprecated service with the old name mapping to the new one (see commit https://github.com/moodle/moodle/commit/9028d9b513fa2e6f673ab875bb53a8056778d21b#diff-abfc6b5afd59c0585981a5daa32764c0)

          Andrew Davis do you mind if I continue the work on this issue (preserving all your commits)? It will be very interesting for the Mobile app having this webservice in 2.7

          As far as I see, the only missing work is just rebase to current master

          Show
          jleyva Juan Leyva added a comment - - edited Hi, MDL-31890 included the deprecated service with the old name mapping to the new one (see commit https://github.com/moodle/moodle/commit/9028d9b513fa2e6f673ab875bb53a8056778d21b#diff-abfc6b5afd59c0585981a5daa32764c0 ) Andrew Davis do you mind if I continue the work on this issue (preserving all your commits)? It will be very interesting for the Mobile app having this webservice in 2.7 As far as I see, the only missing work is just rebase to current master
          Hide
          dougiamas Martin Dougiamas added a comment -

          +1 please Juan thanks!

          Show
          dougiamas Martin Dougiamas added a comment - +1 please Juan thanks!
          Hide
          cibot CiBoT added a comment -

          Results for MDL-30085

          • Remote repository: git://github.com/jleyva/moodle.git
          Show
          cibot CiBoT added a comment - Results for MDL-30085 Remote repository: git://github.com/jleyva/moodle.git Remote branch MDL-30085 _grade_ws to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1639 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1639/artifact/work/smurf.html
          Hide
          cibot CiBoT added a comment -

          Results for MDL-30085

          • Remote repository: git://github.com/jleyva/moodle.git
          Show
          cibot CiBoT added a comment - Results for MDL-30085 Remote repository: git://github.com/jleyva/moodle.git Remote branch MDL-30085 _grade_ws to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1640 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1640/artifact/work/smurf.html
          Hide
          jleyva Juan Leyva added a comment -

          Note for integrators, the Coding style problems are because the new code uses old variables with invalid name format.

          I've fixed the rest of coding style and comment issues (see my two additional commits)

          Show
          jleyva Juan Leyva added a comment - Note for integrators, the Coding style problems are because the new code uses old variables with invalid name format. I've fixed the rest of coding style and comment issues (see my two additional commits)
          Hide
          cibot CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          damyon Damyon Wiese added a comment -

          Thanks for picking this up again.

          Everything looks mostly fine but unit tests are completely crashing at about 39%:

          PHP Fatal error:  Cannot redeclare class core_grade_external in /home/damyonw/Documents/Moodle/integration/master/moodle/grade/externallib.php 
          

          Also - (minor) the phpdocs need updating to say since 2.7 instead of 2.6 and there are a couple of phpdocs that look like they are not finished ("[update_grades description]").

          The main thing to fix here is the unit tests issue (I haven't looked closely at it sorry).

          Thanks!

          Show
          damyon Damyon Wiese added a comment - Thanks for picking this up again. Everything looks mostly fine but unit tests are completely crashing at about 39%: PHP Fatal error: Cannot redeclare class core_grade_external in /home/damyonw/Documents/Moodle/integration/master/moodle/grade/externallib.php Also - (minor) the phpdocs need updating to say since 2.7 instead of 2.6 and there are a couple of phpdocs that look like they are not finished (" [update_grades description] "). The main thing to fix here is the unit tests issue (I haven't looked closely at it sorry). Thanks!
          Hide
          cibot CiBoT added a comment -

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

          Show
          cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          jleyva Juan Leyva added a comment -

          Hi,
          sorry for not running the tests for the entire site... I was running only for the component

          The problem is that grade/externallib.php declares the core_grade_external for backwards compatibility (it was incorrectly named and was renamed to core_grading_external)

          Any idea?

          • rename core_grade_external to core_grades_external (core_grades I think is the correct component name)
          • remove this deprecated class?

          Suggestions are welcome

          /**
           * core grading functions. Renamed to core_grading_external
           *
           * @since Moodle 2.5
           * @deprecated since 2.6 See MDL-30085. Please do not use this class any more.
           * @see core_grading_external
           */
          class core_grade_external extends external_api {
           

          Show
          jleyva Juan Leyva added a comment - Hi, sorry for not running the tests for the entire site... I was running only for the component The problem is that grade/externallib.php declares the core_grade_external for backwards compatibility (it was incorrectly named and was renamed to core_grading_external) Any idea? rename core_grade_external to core_grades_external (core_grades I think is the correct component name) remove this deprecated class? Suggestions are welcome /** * core grading functions. Renamed to core_grading_external * * @since Moodle 2.5 * @deprecated since 2.6 See MDL-30085. Please do not use this class any more. * @see core_grading_external */ class core_grade_external extends external_api {  
          Hide
          jleyva Juan Leyva added a comment -

          After talking with Daymon in the developers chat we agreed to rename the function from core_grade_external to core_grades_external

          Show
          jleyva Juan Leyva added a comment - After talking with Daymon in the developers chat we agreed to rename the function from core_grade_external to core_grades_external
          Hide
          jleyva Juan Leyva added a comment -

          Renamed core_grade_* to core_grades_*
          Fixed invalid @since tags
          Fixed invalid/missing function documentation

          I've executed all the Moodle site tests, everything is OK

          Show
          jleyva Juan Leyva added a comment - Renamed core_grade_* to core_grades_* Fixed invalid @since tags Fixed invalid/missing function documentation I've executed all the Moodle site tests, everything is OK
          Hide
          cibot CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          poltawski Dan Poltawski added a comment -

          Hi Juan,

          lib/grade/externallib.php:

          1. lib/grade/externallib.php is missing a MOODLE_INTERNAL check
          2. This should have MUST_EXIST: $course = $DB->get_record('course', array('id' => $params['courseid']));
          Show
          poltawski Dan Poltawski added a comment - Hi Juan, lib/grade/externallib.php: lib/grade/externallib.php is missing a MOODLE_INTERNAL check This should have MUST_EXIST: $course = $DB->get_record('course', array('id' => $params ['courseid'] ));
          Hide
          poltawski Dan Poltawski added a comment -

          Sorry Juan - i'm reopening this as I don't think there is enough time for us to get it in this week.

          Show
          poltawski Dan Poltawski added a comment - Sorry Juan - i'm reopening this as I don't think there is enough time for us to get it in this week.
          Hide
          jleyva Juan Leyva added a comment -

          Thanks Dan,

          fixed and rebased with current mater.

          Regards

          Show
          jleyva Juan Leyva added a comment - Thanks Dan, fixed and rebased with current mater. Regards
          Hide
          cibot CiBoT added a comment -

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

          Show
          cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          cibot CiBoT added a comment -

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

          Show
          cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          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
          cibot CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          poltawski Dan Poltawski added a comment -

          Thanks Juan! We got there eventually - integrated to master

          Show
          poltawski Dan Poltawski added a comment - Thanks Juan! We got there eventually - integrated to master
          Hide
          damyon Damyon Wiese added a comment -

          Wow - what a long running issue.

          Thanks for getting it over the line!

          Show
          damyon Damyon Wiese added a comment - Wow - what a long running issue. Thanks for getting it over the line!
          Hide
          skodak Petr Skoda added a comment -

          A little problem:

          Fields list in snapshot record does not match fields list in 'external_services'. Record is missing fields: component, timemodified, shortname
          line 670 of /lib/classes/event/base.php: call to debugging()
          line 87 of /admin/webservice/service.php: call to core\event\base->add_record_snapshot()

          Show
          skodak Petr Skoda added a comment - A little problem: Fields list in snapshot record does not match fields list in 'external_services'. Record is missing fields: component, timemodified, shortname line 670 of /lib/classes/event/base.php: call to debugging() line 87 of /admin/webservice/service.php: call to core\event\base->add_record_snapshot()
          Hide
          skodak Petr Skoda added a comment -

          Sorry, I have to fail this, the "core_grades_" prefix is for stuff located in dirroot/grade/ not dirroot/lib/grade - this goes directly agains all new classloading and frankenstyle rules. There is enough confusion already, please fix the naming.

          My +1 to revert this and define the proper class and method names for all grading related stuff first.

          Show
          skodak Petr Skoda added a comment - Sorry, I have to fail this, the "core_grades_" prefix is for stuff located in dirroot/grade/ not dirroot/lib/grade - this goes directly agains all new classloading and frankenstyle rules. There is enough confusion already, please fix the naming. My +1 to revert this and define the proper class and method names for all grading related stuff first.
          Hide
          skodak Petr Skoda added a comment - - edited

          The naming rules for externallib.php are simple:
          1/ the externallib files are defined in plugin or core subsystem directories - see core_component
          2/ the class name is "core_nameofsubsystem_external" or "plugintype_pluginname_external"
          3/ the web service function names are created as "core_nameofsubsystem_method_name" or "plugintype_pluginname_method_name"

          Show
          skodak Petr Skoda added a comment - - edited The naming rules for externallib.php are simple: 1/ the externallib files are defined in plugin or core subsystem directories - see core_component 2/ the class name is "core_nameofsubsystem_external" or "plugintype_pluginname_external" 3/ the web service function names are created as "core_nameofsubsystem_method_name" or "plugintype_pluginname_method_name"
          Hide
          dougiamas Martin Dougiamas added a comment -

          So what should it be instead of core_grades_ ?

          Your comments are not exactly clearing the confusion around naming ... can you be specific for this case?

          Show
          dougiamas Martin Dougiamas added a comment - So what should it be instead of core_grades_ ? Your comments are not exactly clearing the confusion around naming ... can you be specific for this case?
          Hide
          skodak Petr Skoda added a comment -

          In case of grades the "core_grades_" points to dirroot/grades/ and we already have an externallib.php there witch invalid class name core_grading_external and core_grade_external. The dirroot/lib/grade/ is not a subsystem location defined in core_component. But we have core_grading subsystem which is in /grade/grading/ which does not have externallib.php now.

          This seems quite messy to me and I believe this patch makes it even more confusing...

          Show
          skodak Petr Skoda added a comment - In case of grades the "core_grades_" points to dirroot/grades/ and we already have an externallib.php there witch invalid class name core_grading_external and core_grade_external. The dirroot/lib/grade/ is not a subsystem location defined in core_component. But we have core_grading subsystem which is in /grade/grading/ which does not have externallib.php now. This seems quite messy to me and I believe this patch makes it even more confusing...
          Hide
          dougiamas Martin Dougiamas added a comment -

          Agreed that all that could use a cleanup, but that would have a lot of consequences and possible regressions, not only on our code but for others. I suggest leaving that until we tackle the whole gradebook (possibly later this year).

          The main thing now is to have this external API function named with something that works (and to have it in 2.7).

          Show
          dougiamas Martin Dougiamas added a comment - Agreed that all that could use a cleanup, but that would have a lot of consequences and possible regressions, not only on our code but for others. I suggest leaving that until we tackle the whole gradebook (possibly later this year). The main thing now is to have this external API function named with something that works (and to have it in 2.7).
          Hide
          skodak Petr Skoda added a comment -

          what regressions? the rules for externallib.php are simple and the existing files may be fixed while keeping backwards compatibility. This patch needs to be reverted and reworked to match the expected class names/locations.

          Show
          skodak Petr Skoda added a comment - what regressions? the rules for externallib.php are simple and the existing files may be fixed while keeping backwards compatibility. This patch needs to be reverted and reworked to match the expected class names/locations.
          Hide
          skodak Petr Skoda added a comment -

          Also it is possible to use automatic classloading now because the existing rules for externallib.php are fully compatible with it.

          Show
          skodak Petr Skoda added a comment - Also it is possible to use automatic classloading now because the existing rules for externallib.php are fully compatible with it.
          Hide
          poltawski Dan Poltawski added a comment -

          We'll look at this in MDL-44603.

          Show
          poltawski Dan Poltawski added a comment - We'll look at this in MDL-44603 .
          Hide
          skodak Petr Skoda added a comment -

          This patch is clearly wrong, why not revert it?

          Show
          skodak Petr Skoda added a comment - This patch is clearly wrong, why not revert it?
          Hide
          jleyva Juan Leyva added a comment - - edited

          Petr, moving the new functions and tests to core/grade/ will solve the problem?

          And.. I suppose I have to create a new test files in core/grade/test since the current externallib_test.php is used by grading...

          Show
          jleyva Juan Leyva added a comment - - edited Petr, moving the new functions and tests to core/grade/ will solve the problem? And.. I suppose I have to create a new test files in core/grade/test since the current externallib_test.php is used by grading...
          Hide
          skodak Petr Skoda added a comment -

          there is no core/grade/ directory, please have a look at the core subsystems defined in core_component class and use them for all web services

          Show
          skodak Petr Skoda added a comment - there is no core/grade/ directory, please have a look at the core subsystems defined in core_component class and use them for all web services
          Hide
          jleyva Juan Leyva added a comment -

          Sorry, I meant $CFG->dirroot.'/grade', directory

          I will move there the functions and I will create a new testcase file

          I will create a new bug also for moving the grading webservices to the correct place

          Show
          jleyva Juan Leyva added a comment - Sorry, I meant $CFG->dirroot.'/grade', directory I will move there the functions and I will create a new testcase file I will create a new bug also for moving the grading webservices to the correct place
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment - - edited

          Hi Juan (et all),

          we have been discussing a bit about this here and have come with this plan:

          0) revert this issue (current 5 commits)

          1) perform the move (at least the one involving core_grades/core_grading stuff as a reference) to level0 lib/classes/[grades|grading]_external.php (note it also could be grade/classes/external.php and grading/classes/external.php, but we have decided to follow the event's way for subsystems) and document it. Petr will be doing that in MDL-44603, keeping BC in mind.

          2) Your new grades_external stuff (this issue) lands to classland.

          So I'm marking this as blocked by MDL-44603, reverting and reopening.

          Feel free to ask anything here or in the linked issue. Thanks!

          Edited: I've seen you already have added a 6th commit, fixing the naming. That would make this integrable, but still we are going with the plan above. Sorry for using your case as guinea pig, but it will be worth the effort.

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - - edited Hi Juan (et all), we have been discussing a bit about this here and have come with this plan: 0) revert this issue (current 5 commits) 1) perform the move (at least the one involving core_grades/core_grading stuff as a reference) to level0 lib/classes/ [grades|grading] _external.php (note it also could be grade/classes/external.php and grading/classes/external.php, but we have decided to follow the event's way for subsystems) and document it. Petr will be doing that in MDL-44603 , keeping BC in mind. 2) Your new grades_external stuff (this issue) lands to classland. So I'm marking this as blocked by MDL-44603 , reverting and reopening. Feel free to ask anything here or in the linked issue. Thanks! Edited: I've seen you already have added a 6th commit, fixing the naming. That would make this integrable, but still we are going with the plan above. Sorry for using your case as guinea pig, but it will be worth the effort.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          integration.git history has been rewritten and this was never integrated.

          Reopening it for a patch following the route started with MDL-44603.

          Ciao

          Side note: I finally had to leave a commit from Dan doing a version bump. Not sure why but neither revert nor rewrite machinery liked to delete that one. Not critical in any case.

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - integration.git history has been rewritten and this was never integrated. Reopening it for a patch following the route started with MDL-44603 . Ciao Side note: I finally had to leave a commit from Dan doing a version bump . Not sure why but neither revert nor rewrite machinery liked to delete that one. Not critical in any case.
          Hide
          cibot CiBoT added a comment -

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

          Show
          cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          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
          cibot CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Let's make this to break today, yay! Integrated (master only), thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Let's make this to break today, yay! Integrated (master only), thanks!
          Hide
          phalacee Jason Fowler added a comment -

          I have no idea what the client would need to be to test the update_grades(), so I think I will wait on the new testing instructions. I can't get the client to run at the moment anyway, so once the instructions are updated, I will try again.

          Show
          phalacee Jason Fowler added a comment - I have no idea what the client would need to be to test the update_grades(), so I think I will wait on the new testing instructions. I can't get the client to run at the moment anyway, so once the instructions are updated, I will try again.
          Hide
          jleyva Juan Leyva added a comment -

          Hi,

          I'm working in adding the update_grades to the test client, I will add a new comment when it's done

          Regards

          Show
          jleyva Juan Leyva added a comment - Hi, I'm working in adding the update_grades to the test client, I will add a new comment when it's done Regards
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Thanks for adding more webservices Juan,

          I am passing this on behalf of Jason, as he has left for the day and couldn't complete this.

          I did following to test this:

          1. Enabled webservice and enabled REST protocol.
          2. Created a course with 1 assignment and enrolled 1 student and 1 teacher
          3. As teacher graded student.
          4. Created external services and added two functions core_grades_get_grades and core_grades_update_grades
          5. Added token for teacher and student for this service from "Manage tokens"
          6. Updated client_grade.php with token, courseid, assignment id and userid
          7. ran php client_grade.php (from command line)
          8. Got json encoded string with grades.
          9. Tried getting grade for different user and got permission error.

          Passing...

          Show
          rajeshtaneja Rajesh Taneja added a comment - Thanks for adding more webservices Juan, I am passing this on behalf of Jason, as he has left for the day and couldn't complete this. I did following to test this: Enabled webservice and enabled REST protocol. Created a course with 1 assignment and enrolled 1 student and 1 teacher As teacher graded student. Created external services and added two functions core_grades_get_grades and core_grades_update_grades Added token for teacher and student for this service from "Manage tokens" Updated client_grade.php with token, courseid, assignment id and userid ran php client_grade.php (from command line) Got json encoded string with grades. Tried getting grade for different user and got permission error. Passing...
          Hide
          jleyva Juan Leyva added a comment -

          Hi Rajesh,

          I've updated the client so it tests all the functions get_grades and updated_grades, the script does the following:

          • First get the current grade
          • Then update the grade to random(0,100)
          • Then get the grade again to check if it was successfully updated

          You may change the user and activity ids to check if it fails correctly

          Show
          jleyva Juan Leyva added a comment - Hi Rajesh, I've updated the client so it tests all the functions get_grades and updated_grades, the script does the following: First get the current grade Then update the grade to random(0,100) Then get the grade again to check if it was successfully updated You may change the user and activity ids to check if it fails correctly
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Thanks for updating this Juan,

          I tested this again and got passing result:

          Current grade is: 80
           
          Updating current grade to 49 (random(0, 100))
           
          Grade updated
           
          Retrieving grade again to check if matches the new grade (49)
           
          Grade retrieved is: 49
           
          OK it worked!! Thanks for testing
          

          Show
          rajeshtaneja Rajesh Taneja added a comment - Thanks for updating this Juan, I tested this again and got passing result: Current grade is: 80   Updating current grade to 49 (random(0, 100))   Grade updated   Retrieving grade again to check if matches the new grade (49)   Grade retrieved is: 49   OK it worked!! Thanks for testing
          Hide
          marina Marina Glancy added a comment -

          Hi, I just had a quick review of the code and did not see any events triggered when grade is updated. Petr Skoda was working on MDL-40910 (integrated this cycle as well), should not the same event \core\grade\user_graded be triggered in update_grade webservice? If yes, we need to create an issue for it. Thanks.

          Show
          marina Marina Glancy added a comment - Hi, I just had a quick review of the code and did not see any events triggered when grade is updated. Petr Skoda was working on MDL-40910 (integrated this cycle as well), should not the same event \core\grade\user_graded be triggered in update_grade webservice? If yes, we need to create an issue for it. Thanks.
          Hide
          marina Marina Glancy added a comment -

          Thanks for your hard work. Your code is now part of Moodle.

          Show
          marina Marina Glancy added a comment - Thanks for your hard work. Your code is now part of Moodle.

            People

            • Votes:
              7 Vote for this issue
              Watchers:
              18 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                12/May/14