Details

    • Testing Instructions:
      Hide

      Take the PHP-REST demo client: https://github.com/moodlehq/sample-ws-clients/tree/master/PHP-REST (or another one)

      For each test, you will need to change the code between /// PARAMETERs as specified

      Test 1) - Specify 1 or more assignment ids for which the user has 'mod/assign:grade' capability
      /// PARAMETERS
      $assignmentids[] = 3; // user has 'mod/assign:grade' capability for this assignment
      $assignmentids[] = 4; // user has 'mod/assign:grade' capability for this assignment
      $params = array('assignmentids'=>$assignmentids);
      /// PARAMETERS
      The web service should return the submissions for the specified assignments

      Test 2) - Specify the status of the submissions
      /// PARAMETERS
      $assignmentids[] = 3; // user has 'mod/assign:grade' capability for this assignment
      $assignmentids[] = 4; // user has 'mod/assign:grade' capability for this assignment
      $params = array('assignmentids'=>$assignmentids, 'status'=>'submitted');
      /// PARAMETERS
      The web services shoudld return all submissions for the specified assignments where the status is submitted

      Test 3) - Specify a range of timemodified values
      /// PARAMETERS
      $assignmentids[] = 3; // user has 'mod/assign:grade' capability for this assignment
      $assignmentids[] = 4; // user has 'mod/assign:grade' capability for this assignment
      $params = array('assignmentids'=>$assignmentids, 'since'=>1331862337, 'before'=>1331862900);
      /// PARAMETERS
      The web services shoudld return all submissions for the specified assignments where the timemodified value is between the specified range

      Test 4) - An assignment is specified for which the user does not have the mod/assign:grade capability
      /// PARAMETERS
      $assignmentids[] = 20; //user does not have 'mod/assign:grade' capability for this assignment
      $params = array('assignmentids'=>$assignmentids);
      /// PARAMETERS
      The web service returns a warning message

      Show
      Take the PHP-REST demo client: https://github.com/moodlehq/sample-ws-clients/tree/master/PHP-REST (or another one) For each test, you will need to change the code between /// PARAMETERs as specified Test 1) - Specify 1 or more assignment ids for which the user has 'mod/assign:grade' capability /// PARAMETERS $assignmentids[] = 3; // user has 'mod/assign:grade' capability for this assignment $assignmentids[] = 4; // user has 'mod/assign:grade' capability for this assignment $params = array('assignmentids'=>$assignmentids); /// PARAMETERS The web service should return the submissions for the specified assignments Test 2) - Specify the status of the submissions /// PARAMETERS $assignmentids[] = 3; // user has 'mod/assign:grade' capability for this assignment $assignmentids[] = 4; // user has 'mod/assign:grade' capability for this assignment $params = array('assignmentids'=>$assignmentids, 'status'=>'submitted'); /// PARAMETERS The web services shoudld return all submissions for the specified assignments where the status is submitted Test 3) - Specify a range of timemodified values /// PARAMETERS $assignmentids[] = 3; // user has 'mod/assign:grade' capability for this assignment $assignmentids[] = 4; // user has 'mod/assign:grade' capability for this assignment $params = array('assignmentids'=>$assignmentids, 'since'=>1331862337, 'before'=>1331862900); /// PARAMETERS The web services shoudld return all submissions for the specified assignments where the timemodified value is between the specified range Test 4) - An assignment is specified for which the user does not have the mod/assign:grade capability /// PARAMETERS $assignmentids[] = 20; //user does not have 'mod/assign:grade' capability for this assignment $params = array('assignmentids'=>$assignmentids); /// PARAMETERS The web service returns a warning message
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
    • Rank:
      38260

      Description

      Create web service mod_assignment_get_assignment_submissions. This web service will return data from the assignment_submissions table. It will have 2 parameters:

      • A list of assignment ids (required)
      • A status parameter that allows the status of requested submissions to be specified (optional)
      • A since parameter so that only submissions with a timemodified value >= since are returned (optional)
      • A before parameter so that only submissions with a timemodified value <= before are returned (optional)

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for suggesting these Web service improvements, Paul.

          What sort of submission information would be needed in the return?

          Show
          Michael de Raadt added a comment - Thanks for suggesting these Web service improvements, Paul. What sort of submission information would be needed in the return?
          Hide
          Paul Charsley added a comment -

          Hi Michael,

          The submission information that we propose to return is as follows:

          An array called assignments containing (for each assignment requested):

          The id of the assignment returned
          an array called submissions containing multiple assignment_submission records:

          submission id
          userid
          timecreated
          timemodified
          numfiles
          data2 (in the case of a multiple file upload type this contains a status such as 'submitted')
          submissioncomment
          teacher
          timemarked

          Thanks, Paul

          Show
          Paul Charsley added a comment - Hi Michael, The submission information that we propose to return is as follows: An array called assignments containing (for each assignment requested): The id of the assignment returned an array called submissions containing multiple assignment_submission records: submission id userid timecreated timemodified numfiles data2 (in the case of a multiple file upload type this contains a status such as 'submitted') submissioncomment teacher timemarked Thanks, Paul
          Hide
          Jérôme Mouneyrac added a comment -

          Adding Damyon as he is in charge of assignment for 2.3.

          Show
          Jérôme Mouneyrac added a comment - Adding Damyon as he is in charge of assignment for 2.3 .
          Hide
          Paul Charsley added a comment -

          As discussed with Damyon, this should be renamed to mod_assign_get_submissions since it will be part of the new Moodle 2.3 asssignment module.

          Parameters:
          array of assignment ids
          status (optional) - e.g specifying submitted would only return submitted submissions
          since (optional) - only return submissions created after a certain time
          before (optional) - only return submissions created before a certain time

          Return:
          Array of submissions for each requested assignment id

          Show
          Paul Charsley added a comment - As discussed with Damyon, this should be renamed to mod_assign_get_submissions since it will be part of the new Moodle 2.3 asssignment module. Parameters: array of assignment ids status (optional) - e.g specifying submitted would only return submitted submissions since (optional) - only return submissions created after a certain time before (optional) - only return submissions created before a certain time Return: Array of submissions for each requested assignment id
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Paul,
          for me it's good. We can change the component name later if it's not the correct one, I thought it would have been "mod_assignment". I guess we can put this issue straight in the roadmap. Moving it...

          Damyon if anything is wrong, let us know.

          Thanks Paul.

          Show
          Jérôme Mouneyrac added a comment - Hi Paul, for me it's good. We can change the component name later if it's not the correct one, I thought it would have been "mod_assignment". I guess we can put this issue straight in the roadmap. Moving it... Damyon if anything is wrong, let us know. Thanks Paul.
          Hide
          Jérôme Mouneyrac added a comment -

          I send it to integration as the web service related code was good for me. Integrator/maintainer will review the get assignment submission code logic.

          Show
          Jérôme Mouneyrac added a comment - I send it to integration as the web service related code was good for me. Integrator/maintainer will review the get assignment submission code logic.
          Hide
          Dan Poltawski 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
          Dan Poltawski 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
          Dan Poltawski added a comment -

          Hi,

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

          Show
          Dan Poltawski added a comment - Hi, I'm taking this issue out of integration because it first requires mod_assign to land.
          Hide
          Dan Poltawski 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
          Dan Poltawski 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
          Paul Charsley added a comment -

          rebase complete

          Show
          Paul Charsley added a comment - rebase complete
          Hide
          Dan Poltawski added a comment -

          Hi,

          I'm reopening this issue because:

          • It was spotted that the SQL statements are not using placeholders and instead are embeding variables directly in SQL. Please ensure we use placeholders here.
          • It requires mod_assign to land first. This has not been integrated yet so we can't integrated this. Please don't send this to integration until MDL-31270 is integrated, we can't put the cart before the horse!
          Show
          Dan Poltawski added a comment - Hi, I'm reopening this issue because: It was spotted that the SQL statements are not using placeholders and instead are embeding variables directly in SQL. Please ensure we use placeholders here. It requires mod_assign to land first. This has not been integrated yet so we can't integrated this. Please don't send this to integration until MDL-31270 is integrated, we can't put the cart before the horse!
          Hide
          Paul Charsley added a comment -

          Hi,
          I've rebased and modified to use placeholders

          Show
          Paul Charsley added a comment - Hi, I've rebased and modified to use placeholders
          Hide
          Paul Charsley added a comment -

          Hi Jerome, Can this now go into integration?

          Show
          Paul Charsley added a comment - Hi Jerome, Can this now go into integration?
          Hide
          Jérôme Mouneyrac added a comment -

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

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

          Hi Jerome,
          Updated warnings and rebased.
          Thanks, Paul

          Show
          Paul Charsley added a comment - Hi Jerome, Updated warnings and rebased. Thanks, Paul
          Hide
          Paul Charsley added a comment -

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

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

          Hi Paul,
          can you add the PHPunit test. PHPunit test is now requirement to send a web service function to integration. An example can be found there: https://github.com/moodle/moodle/blob/master/course/tests/externallib_test.php (you can look at get_categories as example).
          Cheers,
          Jerome

          Show
          Jérôme Mouneyrac added a comment - Hi Paul, can you add the PHPunit test. PHPunit test is now requirement to send a web service function to integration. An example can be found there: https://github.com/moodle/moodle/blob/master/course/tests/externallib_test.php (you can look at get_categories as example). Cheers, Jerome
          Hide
          Paul Charsley added a comment -

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

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

          Show
          Paul Charsley added a comment - Hi Jerome, I have added a PHPUnit test. Note, however that issue MDL-27968 (I've added a comment) means that a coding exception will occur when tests are run until it is fixed. Also, to allow tests to be run, the assign_add_instance function in mod/assign/lib.php needs to be fixed so that the mod_assign_mod_form parameter either defaults to null or is removed completely (it is not used). See the equivalent functions in the other mods. Its a trivial change. Do you want me to create a separate issue or just include the change with this issue? Thanks, Paul
          Hide
          Jérôme Mouneyrac added a comment -

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

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

          Show
          Jérôme Mouneyrac added a comment - Hello guys, following the Moodle peer-review process, I reassigned the peer-reviewer to the component maintainer. If the component maintainer is unknown assign the peer-reviewer to moodle.com. Thanks. Note for the peer-reviewer: I understand the issue history is long. However you just need review the "Pull Master Diff URL". Then you can look at the issue history if anything seems strange. If any question you can contact me in private, I'll be happy to help. There is also a Moodledocs to help peer-reviewing web service contribution: http://docs.moodle.org/dev/How_to_peer_review_a_core_web_service_function . Thanks. Jerome.
          Hide
          Paul Charsley added a comment -

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

          Show
          Paul Charsley added a comment - Hi Jerome, Damyon, Can this issue be peer reviewed? It missed 2.3 and so we really want to make sure it's included in 2.4. Cheers, Paul
          Hide
          Damyon Wiese added a comment - - edited

          Hi Paul,

          This will need some changes before it can be sent for integration.

          The biggest issue is the hardcoding of support for the "submission_file" sub plugin and the "submission_onlinetext" sub plugin.

          This is not generic and will not work with third party plugins.

          It would be nice to be able to grab everything with a single table join - but that will not be possible - the plugins have functions to get the files and online text areas that they support and that is the only way to query them (MDL-31276 is in review and contains more functions here for text editors).

          Regards, Damyon

          Show
          Damyon Wiese added a comment - - edited Hi Paul, This will need some changes before it can be sent for integration. The biggest issue is the hardcoding of support for the "submission_file" sub plugin and the "submission_onlinetext" sub plugin. This is not generic and will not work with third party plugins. It would be nice to be able to grab everything with a single table join - but that will not be possible - the plugins have functions to get the files and online text areas that they support and that is the only way to query them ( MDL-31276 is in review and contains more functions here for text editors). Regards, Damyon
          Hide
          Damyon Wiese added a comment -

          The submissions table will also have a groupid added to support team assignments when MDL-31284 is integrated. The group id is 0 for student submissions and > 0 for team submissions (the submission id is 0 in that case).

          If this was built from the database table this would be automatically kept in sync with new features.

          Show
          Damyon Wiese added a comment - The submissions table will also have a groupid added to support team assignments when MDL-31284 is integrated. The group id is 0 for student submissions and > 0 for team submissions (the submission id is 0 in that case). If this was built from the database table this would be automatically kept in sync with new features.
          Hide
          Paul Charsley added a comment -

          Hi Damyon, Jerome,

          As I mentioned in MDL-31683, I'm uncomfortable about dynamically generating the web service based on the assign_plugin_config table. I would rather have a limited number of "hardcoded" sub plugins and not return details of third party plugins.

          Jerome, what is your opinion about this?

          Cheers, Paul

          Show
          Paul Charsley added a comment - Hi Damyon, Jerome, As I mentioned in MDL-31683 , I'm uncomfortable about dynamically generating the web service based on the assign_plugin_config table. I would rather have a limited number of "hardcoded" sub plugins and not return details of third party plugins. Jerome, what is your opinion about this? Cheers, Paul
          Hide
          Damyon Wiese added a comment - - edited

          This webservice must not do a join on any of the tables used by the submission plugins.

          It can be implemented generically without specific knowledge of the plugins and without making the parameters change.

          The plugins can provide a list of the file_areas and editors they support and this can be used to generate a list of all files or text submissions associated with a submission.

          This:

          private static function assign_submissions() {
                  return new external_single_structure(
                      array (
                          'assignmentid' => new external_value(PARAM_INT, 'assignment id'),
                          'submissions' => new external_multiple_structure(new external_single_structure(
                                  array(
                                      'id' => new external_value(PARAM_INT, 'submission id'),
                                      'userid' => new external_value(PARAM_INT, 'student id'),
                                      'timecreated' => new external_value(PARAM_INT, 'submission creation time'),
                                      'timemodified' => new external_value(PARAM_INT, 'submission last modified time'),
                                      'status' => new external_value(PARAM_TEXT, 'submission status'),
                                      'files' => new external_multiple_structure(
                                                             new external_single_structure(
                                                                 array(
                                                                     'id' => new external_value(PARAM_INT, 'submission file id'),
                                                                     'numfiles' => new external_value(PARAM_INT, 'numfiles')
                                                                 ))),
                                     'onlinetexts' => new external_multiple_structure(
                                                            new external_single_structure(
                                                                array(
                                                                    'id' => new external_value(PARAM_INT, 'submission onlinetext id'),
                                                                    'onlinetext' => new external_value(PARAM_TEXT, 'onlinetext')
                                                                )))
                                  )
                          ))
                      )
                  );
              }
          

          Should instead be something like this:

               private static function assign_submissions() {
                  return new external_single_structure(
                      array (
                          'assignmentid' => new external_value(PARAM_INT, 'assignment id'),
                          'submissions' => new external_multiple_structure(new external_single_structure(
                                  array(
                                      'id' => new external_value(PARAM_INT, 'submission id'),
                                      'userid' => new external_value(PARAM_INT, 'student id'),
                                      'timecreated' => new external_value(PARAM_INT, 'submission creation time'),
                                      'timemodified' => new external_value(PARAM_INT, 'submission last modified time'),
                                      'status' => new external_value(PARAM_TEXT, 'submission status'),
                                      'plugins' => new external_multiple_structure(
                                                             new external_single_structure(
                                                                 array(
                                                                     'type' => new external_value(PARAM_TEXT, 'submission plugin type'),
                                                                     'name' => new external_value(PARAM_TEXT, 'submission plugin name'),
                                                                     'fileareas' => new external_multiple_structure(new external_single_structure(
                                                                                        array(
                                                                                              'area' => new external_value(PARAM_TEXT, 'file area'),
                                                                                              'files' => new external_multiple_structure(
                                                                                                             new external_single_structure(
                                                                                                                   array(
                                                                                                                       'filepath' => new external_value(PARAM_TEXT, 'file path'))
                                                                                                                   )
                                                                                                             )
                                                                                                         )
                                                                                        ),
                                                                                    ),
                                                                     'editorfields' => new external_multiple_structure(
                                                                            new external_single_structure(
                                                                                array(
                                                                                     'name' => new external_value(PARAM_TEXT, 'editor field name'),
                                                                                     'description' => new external_value(PARAM_TEXT, 'editor field description'),
                                                                                     'text' => new external_value(PARAM_TEXT, 'editor field value'),
                                                                                     'format' => new external_value(PARAM_INT, 'editor field format')
                                                                                )
                                                                            )
                                                                  )
                                                             )
                                                      )
          
                      )
                  );
              }
          

          (Note - I'm sure those brackets don't match because I wrote that code directly in the comment).

          Show
          Damyon Wiese added a comment - - edited This webservice must not do a join on any of the tables used by the submission plugins. It can be implemented generically without specific knowledge of the plugins and without making the parameters change. The plugins can provide a list of the file_areas and editors they support and this can be used to generate a list of all files or text submissions associated with a submission. This: private static function assign_submissions() { return new external_single_structure( array ( 'assignmentid' => new external_value(PARAM_INT, 'assignment id'), 'submissions' => new external_multiple_structure( new external_single_structure( array( 'id' => new external_value(PARAM_INT, 'submission id'), 'userid' => new external_value(PARAM_INT, 'student id'), 'timecreated' => new external_value(PARAM_INT, 'submission creation time'), 'timemodified' => new external_value(PARAM_INT, 'submission last modified time'), 'status' => new external_value(PARAM_TEXT, 'submission status'), 'files' => new external_multiple_structure( new external_single_structure( array( 'id' => new external_value(PARAM_INT, 'submission file id'), 'numfiles' => new external_value(PARAM_INT, 'numfiles') ))), 'onlinetexts' => new external_multiple_structure( new external_single_structure( array( 'id' => new external_value(PARAM_INT, 'submission onlinetext id'), 'onlinetext' => new external_value(PARAM_TEXT, 'onlinetext') ))) ) )) ) ); } Should instead be something like this: private static function assign_submissions() { return new external_single_structure( array ( 'assignmentid' => new external_value(PARAM_INT, 'assignment id'), 'submissions' => new external_multiple_structure( new external_single_structure( array( 'id' => new external_value(PARAM_INT, 'submission id'), 'userid' => new external_value(PARAM_INT, 'student id'), 'timecreated' => new external_value(PARAM_INT, 'submission creation time'), 'timemodified' => new external_value(PARAM_INT, 'submission last modified time'), 'status' => new external_value(PARAM_TEXT, 'submission status'), 'plugins' => new external_multiple_structure( new external_single_structure( array( 'type' => new external_value(PARAM_TEXT, 'submission plugin type'), 'name' => new external_value(PARAM_TEXT, 'submission plugin name'), 'fileareas' => new external_multiple_structure( new external_single_structure( array( 'area' => new external_value(PARAM_TEXT, 'file area'), 'files' => new external_multiple_structure( new external_single_structure( array( 'filepath' => new external_value(PARAM_TEXT, 'file path')) ) ) ) ), ), 'editorfields' => new external_multiple_structure( new external_single_structure( array( 'name' => new external_value(PARAM_TEXT, 'editor field name'), 'description' => new external_value(PARAM_TEXT, 'editor field description'), 'text' => new external_value(PARAM_TEXT, 'editor field value'), 'format' => new external_value(PARAM_INT, 'editor field format') ) ) ) ) ) ) ); } (Note - I'm sure those brackets don't match because I wrote that code directly in the comment).
          Hide
          Paul Charsley added a comment -

          Hi Damyon, I've made changes as you suggested so this is now ready for peer review.
          Cheers, Paul

          Show
          Paul Charsley added a comment - Hi Damyon, I've made changes as you suggested so this is now ready for peer review. Cheers, Paul
          Hide
          Damyon Wiese added a comment -

          I've checked the patch and the code looks good and the unit tests pass.

          There are git issues here though - this is blocked by MDL-31683 and the code for this patch is merged with the patch for MDL-31683 in a single commit. We'll have to wait until MDL-31683 is integrated, then rebase this patch on top of those changes (and resolve the conflicts).

          • Damyon
          Show
          Damyon Wiese added a comment - I've checked the patch and the code looks good and the unit tests pass. There are git issues here though - this is blocked by MDL-31683 and the code for this patch is merged with the patch for MDL-31683 in a single commit. We'll have to wait until MDL-31683 is integrated, then rebase this patch on top of those changes (and resolve the conflicts). Damyon
          Hide
          Paul Charsley added a comment -

          Hi Damyon, this has now been rebased and is ready for peer review.
          Cheers, Paul

          Show
          Paul Charsley added a comment - Hi Damyon, this has now been rebased and is ready for peer review. Cheers, Paul
          Hide
          Paul Charsley added a comment -

          Hi Damyon, Please can you re-review and send this to integration? The blocking issues that you mentioned in your last comment have been resolved.
          Thanks, Paul

          Show
          Paul Charsley added a comment - Hi Damyon, Please can you re-review and send this to integration? The blocking issues that you mentioned in your last comment have been resolved. Thanks, Paul
          Hide
          Damyon Wiese added a comment -

          Hi Paul,

          There are 4 whitespaces errors in externallib.php (see line 435 for an example - they are all like that) and there is one case where you are using 'assign' in a query (when you should be using a named parameter (line 478)). Can you make a quick fix for these and I'll push it through straight away. Thanks for being persistent, this is almost there.

          Damyon

          Show
          Damyon Wiese added a comment - Hi Paul, There are 4 whitespaces errors in externallib.php (see line 435 for an example - they are all like that) and there is one case where you are using 'assign' in a query (when you should be using a named parameter (line 478)). Can you make a quick fix for these and I'll push it through straight away. Thanks for being persistent, this is almost there. Damyon
          Hide
          Paul Charsley added a comment -

          Hi Damyon, I've made the changes.
          Cheers, Paul

          Show
          Paul Charsley added a comment - Hi Damyon, I've made the changes. Cheers, Paul
          Hide
          Damyon Wiese added a comment -

          Thanks Paul - this looks fine now. Sending for integration.

          Show
          Damyon Wiese added a comment - Thanks Paul - this looks fine now. Sending for integration.
          Hide
          Paul Charsley added a comment -

          Hi Everyone,
          Will this web service function be integrated into Moodle 2.4? I believe that the code freeze date was October 22nd. However, this issue was peer reviewed prior to that. It's also reasonably safe to add this new web service function since it's readonly and is extremely unlikely to adversely affect existing code.
          Cheers, Paul

          Show
          Paul Charsley added a comment - Hi Everyone, Will this web service function be integrated into Moodle 2.4? I believe that the code freeze date was October 22nd. However, this issue was peer reviewed prior to that. It's also reasonably safe to add this new web service function since it's readonly and is extremely unlikely to adversely affect existing code. Cheers, Paul
          Hide
          Dan Poltawski 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
          Dan Poltawski 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
          Paul Charsley added a comment -

          Rebase complete.

          Show
          Paul Charsley added a comment - Rebase complete.
          Hide
          Paul Charsley added a comment -

          Rebased with the latest modifications.

          Show
          Paul Charsley added a comment - Rebased with the latest modifications.
          Hide
          Paul Charsley added a comment -

          Rebased with the latest modifications.

          Show
          Paul Charsley added a comment - Rebased with the latest modifications.
          Hide
          Paul Charsley added a comment -

          Rebased with the latest modifications. Successfully ran PHP unit tests.

          Show
          Paul Charsley added a comment - Rebased with the latest modifications. Successfully ran PHP unit tests.
          Hide
          Paul Charsley added a comment -

          Rebased with latest 2.5dev modifications. Successfully ran PHP unit tests.

          Show
          Paul Charsley added a comment - Rebased with latest 2.5dev modifications. Successfully ran PHP unit tests.
          Hide
          Paul Charsley added a comment -

          Rebased and successfully ran PHP unit tests.

          Show
          Paul Charsley added a comment - Rebased and successfully ran PHP unit tests.
          Hide
          Sam Hemelryk added a comment -

          Hi guys - this has been integrated now.

          Just as a heads up there was only thing that caught my eye here and that was the use of the try catch. I'm really not a fan of try...catch statements being used like that, it covers all exceptions and as such will mask any true errors that may occur at any point making future bugs much harder to track down.
          It's also not great as from what I can see we have API to properly detect any access issues which is the process I would have much rather seeing happening here.
          Either way it works so it went in however please keep this in mind for future work.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys - this has been integrated now. Just as a heads up there was only thing that caught my eye here and that was the use of the try catch. I'm really not a fan of try...catch statements being used like that, it covers all exceptions and as such will mask any true errors that may occur at any point making future bugs much harder to track down. It's also not great as from what I can see we have API to properly detect any access issues which is the process I would have much rather seeing happening here. Either way it works so it went in however please keep this in mind for future work. Many thanks Sam
          Hide
          Adrian Greeve added a comment -

          I keep getting exceptions for assignments that contain online text.
          e.g.

          <?xml version="1.0" encoding="UTF-8" ?>
          <EXCEPTION class="invalid_response_exception">
          <ERRORCODE>invalidresponse</ERRORCODE>
          <MESSAGE>Invalid response value detected</MESSAGE>
          <DEBUGINFO>assignments =&gt; Invalid response value detected: submissions =&gt; Invalid response value detected: plugins =&gt; Invalid response value detected: editorfields =&gt; Invalid response value detected: text =&gt; Invalid response value detected: Invalid external api response: the value is &quot;&lt;p&gt;great scott!&lt;/p&gt;&quot;, the server was expecting &quot;text&quot; type</DEBUGINFO>
          </EXCEPTION>
          

          Assignments without online text will be return properly.

          Show
          Adrian Greeve added a comment - I keep getting exceptions for assignments that contain online text. e.g. <?xml version= "1.0" encoding= "UTF-8" ?> <EXCEPTION class= "invalid_response_exception" > <ERRORCODE>invalidresponse</ERRORCODE> <MESSAGE>Invalid response value detected</MESSAGE> <DEBUGINFO>assignments =&gt; Invalid response value detected: submissions =&gt; Invalid response value detected: plugins =&gt; Invalid response value detected: editorfields =&gt; Invalid response value detected: text =&gt; Invalid response value detected: Invalid external api response: the value is &quot;&lt;p&gt;great scott!&lt;/p&gt;&quot;, the server was expecting &quot;text&quot; type</DEBUGINFO> </EXCEPTION> Assignments without online text will be return properly.
          Hide
          Paul Charsley added a comment -

          Hi Adrian,

          Thanks for finding this problem. I will investigate and resolve.

          Thanks, Paul

          Show
          Paul Charsley added a comment - Hi Adrian, Thanks for finding this problem. I will investigate and resolve. Thanks, Paul
          Hide
          Paul Charsley added a comment -

          I've now resolved the issue which was caused by having the wrong parameter type for online text. I've rebased and retested successfully.

          Show
          Paul Charsley added a comment - I've now resolved the issue which was caused by having the wrong parameter type for online text. I've rebased and retested successfully.
          Hide
          Dan Poltawski added a comment -

          Hi Paul, as we've already integrated the issue, we need a patch on top of the existing commit. Are you able to provide that?

          Show
          Dan Poltawski added a comment - Hi Paul, as we've already integrated the issue, we need a patch on top of the existing commit. Are you able to provide that?
          Hide
          Paul Charsley added a comment -

          Hi Dan,

          Attached is the patch.

          Cheers, Paul

          Show
          Paul Charsley added a comment - Hi Dan, Attached is the patch. Cheers, Paul
          Hide
          Dan Poltawski added a comment -

          Thanks Paul, applied that and sent back to testing

          Show
          Dan Poltawski added a comment - Thanks Paul, applied that and sent back to testing
          Hide
          Adrian Greeve added a comment -

          Test went okay this time
          Test passed.

          Show
          Adrian Greeve added a comment - Test went okay this time Test passed.
          Hide
          Dan Poltawski added a comment -

          Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

          Show
          Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!
          Hide
          Michael de Raadt added a comment -

          Removing the dev_docs_required label as this is now documented in the embedded Web services documentation.

          Show
          Michael de Raadt added a comment - Removing the dev_docs_required label as this is now documented in the embedded Web services documentation.

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: