Details

      Description

      follow http://docs.moodle.org/dev/How_to_contribute_a_web_service_function_to_core

      get/ delete / update

      • Get:
        • Params: IDs of Notes
        • Return: Contents of Notes {RelID, Subject, Summary, Content)
        • Process: Get the notes from the DB and return them
      • Delete:
        • Params: IDs of Notes
        • Return: Array of Note Id's and errors
        • Process: Delete the notes from the DB
      • Update:
        • Params: IDs of Notes, Contents of Notes {Subject, Summary, Content)
        • Return: Array of Note Id's and errors
        • Process: Update the Contents of Notes in the DB based on the IDs

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Jérôme Mouneyrac added a comment -

            Thanks Jason. Few little things, it's almost there:

            • bump the version.php (the function is saved at update)
            • define the web service in db/services.php
            • do not echo anything. It will break the web service (SOAP is guarantee to break on printing stuff in a function). Remove echo "\n". $notedetails->courseid;
            • $errormessage has no value.
            • You commented //return $resultnotes;, I think you wanted to return it and also write the matching return description. Have a look to get_grades_returns() it should be similar.
            Show
            Jérôme Mouneyrac added a comment - Thanks Jason. Few little things, it's almost there: bump the version.php (the function is saved at update) define the web service in db/services.php do not echo anything. It will break the web service (SOAP is guarantee to break on printing stuff in a function). Remove echo "\n". $notedetails->courseid; $errormessage has no value. You commented //return $resultnotes;, I think you wanted to return it and also write the matching return description. Have a look to get_grades_returns() it should be similar.
            Hide
            Jason Fowler added a comment -

            Pushing for peer review

            Show
            Jason Fowler added a comment - Pushing for peer review
            Hide
            Jérôme Mouneyrac added a comment -

            Hi Jason. Here my review:

            • define the web service in db/services.php
            • use external_format_value() and external_format_text() for the new functions. You'll find some help in http://docs.moodle.org/dev/How_to_contribute_a_web_service_function_to_core#Create_a_tracker_issue. You should also find some examples of their usage in other functions too.
            • always use the values validated/cleaned by validate_parameters(). Instead of using $notes, use $params['notes'].
            • try to match the xxx() function parameters with the xxx_parameters() description. Here if you want delete_notes($notes = array()) in the description add ,VALUE_DEFAULT,array() for the parameter 'notes'.
            • $success = false; => never used.
            • your returned warnings are not correctly formatted. Look how other functions format their returned value. To quickly explain, xxx_return() is going to be used by the webservice servers to validate everything that the xxx() function returns. So the xxx() return value has to match the xxx_return() description.
            • 'noteid' => new external_multiple_structure(self::assign_grades(), 'list of assignment grade information'). The description is wrong.

            Let's fix that, and send it back to peer-review. I'll finish later. Thank you.

            Show
            Jérôme Mouneyrac added a comment - Hi Jason. Here my review: define the web service in db/services.php use external_format_value() and external_format_text() for the new functions. You'll find some help in http://docs.moodle.org/dev/How_to_contribute_a_web_service_function_to_core#Create_a_tracker_issue . You should also find some examples of their usage in other functions too. always use the values validated/cleaned by validate_parameters(). Instead of using $notes, use $params ['notes'] . try to match the xxx() function parameters with the xxx_parameters() description. Here if you want delete_notes($notes = array()) in the description add ,VALUE_DEFAULT,array() for the parameter 'notes'. $success = false; => never used. your returned warnings are not correctly formatted. Look how other functions format their returned value. To quickly explain, xxx_return() is going to be used by the webservice servers to validate everything that the xxx() function returns. So the xxx() return value has to match the xxx_return() description. 'noteid' => new external_multiple_structure(self::assign_grades(), 'list of assignment grade information'). The description is wrong. Let's fix that, and send it back to peer-review. I'll finish later. Thank you.
            Hide
            Jason Fowler added a comment -

            I had the services defined there, must have wiped it out with an update

            Show
            Jason Fowler added a comment - I had the services defined there, must have wiped it out with an update
            Hide
            Jason Fowler added a comment -
            • try to match the xxx() function parameters with the xxx_parameters() description. Here if you want delete_notes($notes = array()) in the description add ,VALUE_DEFAULT,array() for the parameter 'notes'.
              • I don't understand what you mean here, I took the params from the function that was already in the file, and emulated that in the functions I added. Can you be more specific?
            • your returned warnings are not correctly formatted. Look how other functions format their returned value. To quickly explain, xxx_return() is going to be used by the webservice servers to validate everything that the xxx() function returns. So the xxx() return value has to match the xxx_return() description.
              • I don't understand what you mean here, I took the warnings from the function that was already in the file, and emulated that in the functions I added. Can you be more specific?
            Show
            Jason Fowler added a comment - try to match the xxx() function parameters with the xxx_parameters() description. Here if you want delete_notes($notes = array()) in the description add ,VALUE_DEFAULT,array() for the parameter 'notes'. I don't understand what you mean here, I took the params from the function that was already in the file, and emulated that in the functions I added. Can you be more specific? your returned warnings are not correctly formatted. Look how other functions format their returned value. To quickly explain, xxx_return() is going to be used by the webservice servers to validate everything that the xxx() function returns. So the xxx() return value has to match the xxx_return() description. I don't understand what you mean here, I took the warnings from the function that was already in the file, and emulated that in the functions I added. Can you be more specific?
            Hide
            Jason Fowler added a comment -

            All ready for the review Jerome

            Show
            Jason Fowler added a comment - All ready for the review Jerome
            Hide
            Jérôme Mouneyrac added a comment - - edited

            Thanks Jason. Here's my review:

            lib/db/services.php
            -------------------

            • typo: methodname' => 'delte_notes', //Note: the web service function will fail to install on Moodle upgrade with this typo. Don't forget to test that your function gets installed properly, and if you can test it with a web service client it's even better.

            delete_notes_parameters
            -----------------------

            • phpdoc: the matching code is in notes/delete.php
            • typo: 'id of the user the note'
            • coding style issue: "Array of Notes",VALUE_DEFAULT,array()
            • read the external_warnings class to know what you should assign to $resultnote['errormessage']. It is not a string. It's an array expecting: item, itemid, warningcode, message => http://docs.moodle.org/dev/Errors_handling_in_web_services#Format_2.
              The best would be to try to use the change made in MDL-37143, so you can explain your warnings to the client dev. If so, then mark your issue as blocked by MDL-37143. Your issue can still be sent to integration. Whatever you decide, don't forget to modify the PHPunit test to test the warnings.
            • you don't need courseid as parameters. You already retrieve it with note_load(), just move it at the beginning of your foreach. It also avoid someone sending a fake courseid and bypass your capability check.
            • add the PHPUnit clean_returnvalue line mentioned in http://docs.moodle.org/dev/Web_Services_Unit_Test:

              // We need to execute the return values cleaning process to simulate the web service server
              $returnvalue = external_api::clean_returnvalue(COMPONENT_external::FUNCTION_NAME_returns(), $returnvalue);
              

              Note that I've just added this test recently to the docs. So normal that you skipped it last time.

            • global $DB is not used.

            get_notes_parameters
            --------------------

            • 'id of the user the note'

            get_notes
            ---------

            • same comment about addnotes.php that it not the place to look, it's notes/index.php. You don't have to mention it if you don't want thought.
            • global $DB is not used.
            • $success and $errormessage not used. Note that here you may want to use external_warnings(). However my next point will suggest that you may want to remove it if you decide that courseid is not a needed parameter.
            • In my opinion for what your function does, you don't need courseid as parameter. Once again people could fake the courseid. However note, that you could use this courseid param to do more stuff like returning all notes of a course that a user can see. I think it's probably a much more appropriate features. To know note id, client dev need to call some sort of get notes on courseid.
            • About: $resultnote["content"] = external_format_text($resultnote["content"]);
              external_format_text returns an array containing two elements. Look at the function declaration. Also here you are putting this returned array into 'content' key but nowhere in the return value description (get_notes_returns) you describe it. This will cause the key to be ignored by Moodle web service servers. Search other functions that use the external_format_text() function and look at their return value description.

            At the end, it works like that:

            //OUTPUT in xxx()
            list($text, $textformat) = external_format_text($text, $textformatfromDB, $contextid, $component, $filearea, $itemid);
            

            //OUTPUT in xxx_returns()                
            'text' => new external_value(PARAM_RAW, 'formatted text'),
            'textformat' => new external_format_value('text'),
            Reference: http://docs.moodle.org/dev/How_to_contribute_a_web_service_function_to_core#Create_a_tracker_issue
            

            update_notes_parameters
            -----------------------

            • wrong usage of external_format_value.

              //INPUT in xxx_parameters()
              'inputtextaera' => new external_value(PARAM_RAW, 'some html param'),
              'inputtextareaformat' => new external_format_value('inputtextarea', VALUE_DEFAULT), // VALUE_DEFAULT has for default format FORMAT_HTML. If not mention VALUE_REQUIRED is used.
              

              Reference: http://docs.moodle.org/dev/How_to_contribute_a_web_service_function_to_core#Create_a_tracker_issue

            • I checked in Moodle interface but I don't think you can change the user of a note. However looking at notes/edit.php, I see that it expect userid. So you can hack the edit form to move the note to a different user. I'll write a minor security issue for that. Don't use the userid params expect if you can find a use case where it is used. Remove the code related.
            • if you have the noteid why would you need the clientnoteid? noteid is already unique, the client can retrieve its own internal id itself.
            • same coding style issue with space "Array of Notes",VALUE_DEFAULT,array()

            update_notes
            ------------

            • phpdoc: the code logic is in notes/edit.php
            • phpdoc: An array of ids for the notes to delete => update
            • phpdoc: the function doesn't return null
            • $params['format'] = external_validate_format($params['format']);
              Don't do that now, but in the foreach (which you do anyway). Just for the info, $params['format'], the key format doesn't exist, it would be in $params['notes'][X][format].
            • same issue with falsifiable courseid. I don't think you need the parameters as you need anyway to retrieve the one from the note. Plus I don't think you can move note in Moodle between courses so you don't want courseid as parameter. Remove the code related
              *

               // Need to support 'html' and 'text' format values for backward compatibility.
                switch (strtolower($note['format'])) {
               	        case 'html':
               	            $textformat = FORMAT_HTML;
               	              break;
               	         case 'text':
               	             $textformat = FORMAT_PLAIN;
               	          default:
               	              $textformat = external_validate_format($note['format']);
               	              break;
               	      }
                $dbnote->content = $note['text'];
                $dbnote->format = $textformat;
              

              should be replaced by

              $dbnote->content = $note['text'];
              $dbnote->format = $external_validate_format($note['format']); //note that it is not optimal to validate the form at this moment but it saves to do a for each only for that.
              

            • wrong add_to_log, check the one in notes/edit.php
            • wrong warnings usage

            Don't be scare by the length of the review, often you have the same issue in each functions.

            Show
            Jérôme Mouneyrac added a comment - - edited Thanks Jason. Here's my review: lib/db/services.php ------------------- typo: methodname' => 'delte_notes', //Note: the web service function will fail to install on Moodle upgrade with this typo. Don't forget to test that your function gets installed properly, and if you can test it with a web service client it's even better. delete_notes_parameters ----------------------- phpdoc: the matching code is in notes/delete.php typo: 'id of the user the note' coding style issue: "Array of Notes",VALUE_DEFAULT,array() read the external_warnings class to know what you should assign to $resultnote ['errormessage'] . It is not a string. It's an array expecting: item, itemid, warningcode, message => http://docs.moodle.org/dev/Errors_handling_in_web_services#Format_2 . The best would be to try to use the change made in MDL-37143 , so you can explain your warnings to the client dev. If so, then mark your issue as blocked by MDL-37143 . Your issue can still be sent to integration. Whatever you decide, don't forget to modify the PHPunit test to test the warnings. you don't need courseid as parameters. You already retrieve it with note_load(), just move it at the beginning of your foreach. It also avoid someone sending a fake courseid and bypass your capability check. add the PHPUnit clean_returnvalue line mentioned in http://docs.moodle.org/dev/Web_Services_Unit_Test: // We need to execute the return values cleaning process to simulate the web service server $returnvalue = external_api::clean_returnvalue(COMPONENT_external::FUNCTION_NAME_returns(), $returnvalue); Note that I've just added this test recently to the docs. So normal that you skipped it last time. global $DB is not used. get_notes_parameters -------------------- 'id of the user the note' get_notes --------- same comment about addnotes.php that it not the place to look, it's notes/index.php. You don't have to mention it if you don't want thought. global $DB is not used. $success and $errormessage not used. Note that here you may want to use external_warnings(). However my next point will suggest that you may want to remove it if you decide that courseid is not a needed parameter. In my opinion for what your function does, you don't need courseid as parameter. Once again people could fake the courseid. However note, that you could use this courseid param to do more stuff like returning all notes of a course that a user can see. I think it's probably a much more appropriate features. To know note id, client dev need to call some sort of get notes on courseid. About: $resultnote ["content"] = external_format_text($resultnote ["content"] ); external_format_text returns an array containing two elements. Look at the function declaration. Also here you are putting this returned array into 'content' key but nowhere in the return value description (get_notes_returns) you describe it. This will cause the key to be ignored by Moodle web service servers. Search other functions that use the external_format_text() function and look at their return value description. At the end, it works like that: //OUTPUT in xxx() list($text, $textformat) = external_format_text($text, $textformatfromDB, $contextid, $component, $filearea, $itemid); //OUTPUT in xxx_returns() 'text' => new external_value(PARAM_RAW, 'formatted text'), 'textformat' => new external_format_value('text'), Reference: http://docs.moodle.org/dev/How_to_contribute_a_web_service_function_to_core#Create_a_tracker_issue update_notes_parameters ----------------------- wrong usage of external_format_value. //INPUT in xxx_parameters() 'inputtextaera' => new external_value(PARAM_RAW, 'some html param'), 'inputtextareaformat' => new external_format_value('inputtextarea', VALUE_DEFAULT), // VALUE_DEFAULT has for default format FORMAT_HTML. If not mention VALUE_REQUIRED is used. Reference: http://docs.moodle.org/dev/How_to_contribute_a_web_service_function_to_core#Create_a_tracker_issue I checked in Moodle interface but I don't think you can change the user of a note. However looking at notes/edit.php, I see that it expect userid. So you can hack the edit form to move the note to a different user. I'll write a minor security issue for that. Don't use the userid params expect if you can find a use case where it is used. Remove the code related. if you have the noteid why would you need the clientnoteid? noteid is already unique, the client can retrieve its own internal id itself. same coding style issue with space "Array of Notes",VALUE_DEFAULT,array() update_notes ------------ phpdoc: the code logic is in notes/edit.php phpdoc: An array of ids for the notes to delete => update phpdoc: the function doesn't return null $params ['format'] = external_validate_format($params ['format'] ); Don't do that now, but in the foreach (which you do anyway). Just for the info, $params ['format'] , the key format doesn't exist, it would be in $params ['notes'] [X] [format] . same issue with falsifiable courseid. I don't think you need the parameters as you need anyway to retrieve the one from the note. Plus I don't think you can move note in Moodle between courses so you don't want courseid as parameter. Remove the code related * // Need to support 'html' and 'text' format values for backward compatibility. switch (strtolower($note['format'])) { case 'html': $textformat = FORMAT_HTML; break; case 'text': $textformat = FORMAT_PLAIN; default: $textformat = external_validate_format($note['format']); break; } $dbnote->content = $note['text']; $dbnote->format = $textformat; should be replaced by $dbnote->content = $note['text']; $dbnote->format = $external_validate_format($note['format']); //note that it is not optimal to validate the form at this moment but it saves to do a for each only for that. wrong add_to_log, check the one in notes/edit.php wrong warnings usage Don't be scare by the length of the review, often you have the same issue in each functions.
            Hide
            Jason Fowler added a comment -

            In all places, I need the courseid for the context to validate the capability.

            Show
            Jason Fowler added a comment - In all places, I need the courseid for the context to validate the capability.
            Hide
            Jérôme Mouneyrac added a comment -

            I know you need the courseid to check the context/capability. However you already get it from the call load_note. I don't see any issue to get the note info first and validating the context/checking the capability after. If you look at load_note it's only a DB request. Nothing is written at this stage, it's ok to retrieve the note info first.

            Note that if you were keeping the courseid parameter, you would have to check that the courseid sent is the same as the courseid retrieved by load_note. But I think the solution I mention above is better. You don't need the courseid parameter for what get_notes/delete_notes are doing at the moment.

            Show
            Jérôme Mouneyrac added a comment - I know you need the courseid to check the context/capability. However you already get it from the call load_note. I don't see any issue to get the note info first and validating the context/checking the capability after. If you look at load_note it's only a DB request. Nothing is written at this stage, it's ok to retrieve the note info first. Note that if you were keeping the courseid parameter, you would have to check that the courseid sent is the same as the courseid retrieved by load_note. But I think the solution I mention above is better. You don't need the courseid parameter for what get_notes/delete_notes are doing at the moment.
            Hide
            Jason Fowler added a comment -

            okay, fair enough, will get rid of it then

            Show
            Jason Fowler added a comment - okay, fair enough, will get rid of it then
            Show
            Jérôme Mouneyrac added a comment - I updated https://tracker.moodle.org/browse/MDL-30072?focusedCommentId=195817&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-195817
            Hide
            Jérôme Mouneyrac added a comment -

            Just for the info, I created MDL-37411 about the userid issue in web interface.

            Show
            Jérôme Mouneyrac added a comment - Just for the info, I created MDL-37411 about the userid issue in web interface.
            Hide
            Jason Fowler added a comment -

            In your peer review: if you have the noteid why would you need the clientnoteid? noteid is already unique, the client can retrieve its own internal id itself.

            I have no idea, I was merely following what you created under create_note ... I figured you had a reason for doing this, and I didn't want to deviate from that.

            Show
            Jason Fowler added a comment - In your peer review: if you have the noteid why would you need the clientnoteid? noteid is already unique, the client can retrieve its own internal id itself. I have no idea, I was merely following what you created under create_note ... I figured you had a reason for doing this, and I didn't want to deviate from that.
            Hide
            Jason Fowler added a comment -

            Haven't done anything with the PHPUnit test yet, but would like feed back on the changes I have made so far, will continue with the other stuff on the review in the mean time.

            Show
            Jason Fowler added a comment - Haven't done anything with the PHPUnit test yet, but would like feed back on the changes I have made so far, will continue with the other stuff on the review in the mean time.
            Hide
            Jérôme Mouneyrac added a comment - - edited

            Hi Jason, it starts to be hard for me to remember quickly what you changed last time. Can you, just for me do multiple commits. When you send the issue to integration you can "soft reset" all in one commit, but at this stage it's easier for me to quickly check your last commit changes otherwise I need to re-read your all code each time.

            clientnoteid: yes it can be confusing. clientnoteid is useful when creating a note (when a client wants to create multiple notes in one ws call, the client needs to know which one has been created, and which one failed. It's why Moodle return the matching clientnoteid). For any other calls (get/delete/update), from the client perspective noteid should be enough to deal with Moodle web service (at this moment of the call, the client will know wich of its own clientnoteid match the noteid). Moodle never needs to know about the clientnoteid.

            I'll have another look monday.

            Show
            Jérôme Mouneyrac added a comment - - edited Hi Jason, it starts to be hard for me to remember quickly what you changed last time. Can you, just for me do multiple commits. When you send the issue to integration you can "soft reset" all in one commit, but at this stage it's easier for me to quickly check your last commit changes otherwise I need to re-read your all code each time. clientnoteid: yes it can be confusing. clientnoteid is useful when creating a note (when a client wants to create multiple notes in one ws call, the client needs to know which one has been created, and which one failed. It's why Moodle return the matching clientnoteid). For any other calls (get/delete/update), from the client perspective noteid should be enough to deal with Moodle web service (at this moment of the call, the client will know wich of its own clientnoteid match the noteid). Moodle never needs to know about the clientnoteid. I'll have another look monday.
            Hide
            Jason Fowler added a comment -

            Changes made, and pushed for peer review

            Show
            Jason Fowler added a comment - Changes made, and pushed for peer review
            Hide
            Jason Fowler added a comment -

            I will squash my changes after the peer review

            Show
            Jason Fowler added a comment - I will squash my changes after the peer review
            Hide
            Jason Fowler added a comment -

            Changes made, getting the unit tests to pass, and getting whitespace under control.

            Pushing for review.

            Show
            Jason Fowler added a comment - Changes made, getting the unit tests to pass, and getting whitespace under control. Pushing for review.
            Hide
            Jason Fowler added a comment -

            Fred has said that I should re-write the add_to_log function calls in to the core functions for saving and deleting notes. Will now be doing that.

            Show
            Jason Fowler added a comment - Fred has said that I should re-write the add_to_log function calls in to the core functions for saving and deleting notes. Will now be doing that.
            Hide
            Jason Fowler added a comment -

            add_to_log has been moved, and the test without the capability have been fixed.

            Show
            Jason Fowler added a comment - add_to_log has been moved, and the test without the capability have been fixed.
            Hide
            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
            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
            Frédéric Massart added a comment -

            Hi Jason,

            Please find my comments as follows:

            General

            • You need to bump the version as you're defining a new WS
            • I think that'd be great to change the API of note_delete to accept a $note as parameter, so you don't need to fecth the information again. We want the webservices to be as light as possible, loading the node is an overload that we don't need to suffer from. But, I'd keep the posibility of passing an int for backward compatilibility, and throw a debugging message to developers to tell them that they should update their code. This would require a note in upgrade.txt as well. I know it's not a blocker, but I don't think raising another issue for this minor improvement is worth it.
            • Please standardise the use of your quotes in your array keys, sometimes they are single ones, sometimes double. I don't mind which one, even if the single ones seem more used, but in one diff you can expect to have all the same.

            notes/externallib.php

            1. 202: Guideline issue
            2. 215: Whitespace issue
            3. 225-227: Documentation issue
            4. 243: Guideline issue
            5. 245: Could you explain why the context needs to be validated? The above method will return an exception if the context is not found.
            6. 251
              • You pass too many paramaters to the constructor, but anyway you can't use the external_warning class here, you have to set an array of information. Grep for the use of external_warnings. In any case a warning code is required for app developers to handle the warning easily.
              • Could you add some Unit Test to check the external_warning when note_delete() fails?
            7. 266: To me this function should not return anything, except an array of external_warnings if we encountered errors. Returnings the IDs passed previously is unnecessary and has no value for the client.
            8. 275: Whitespace issue
            9. 304-306: Documentation issue
            10. 320
              • Could you comment that you're actually converting an object to array? Do you need to do so?
              • What happens if the note does not exist?
              • Please add Unit Test for non-existing note.
            11. 326: I think it'd be easier to read if you were constructing the array or information about the node yourself, instead of casting the object to array.
            12. 366: Shouldn't publishstate be an optional_value as it's not likely that it would change on every edit.
            13. 379-381: Documentation issue
            14. 405: Guideline issue
            15. 406: Guideline issue (new class must end with parenthesis)
            16. 422: Shouldn't you throw an exception (or external warning) when the publishstate is not an expected value?
            17. 430: See above (#251)
            18. 445: See above (#266)
            • The parameters passed to your WS methods do not need to be nested such as $notes['noteids'] = array(1, 2, 3). It is simpler if they are $notes = array(1, 2, 3). You just nedd to remove the single_structure. If you do so, $notes could perhaps be renamed $noteids.
            • General: Your Doc Blocks could be more descriptive, "Returns description of method parameters" could state function name.

            notes/lib.php

            1. You could probably put everything on one line, or two but not three.
            2. And convert to moodle_url()
            3. Do you think you should write something in upgrade.txt as you define a new log type (note:add)?

            notes/tests/externallib_test.php

            1. Delete, Get: Add tests passing multiple IDs at once.
            2. Delete, Get: Add tests passing unexisting IDs.
            3. 159: You should remove that capability as soon as the notes are created.
            4. Update: You could test each possible value of publishstate.
            • In your tests, you should create a user and use that one instead of $USER as it is, it's less random.

            Cheers,
            Fred

            Show
            Frédéric Massart added a comment - Hi Jason, Please find my comments as follows: General You need to bump the version as you're defining a new WS I think that'd be great to change the API of note_delete to accept a $note as parameter, so you don't need to fecth the information again. We want the webservices to be as light as possible, loading the node is an overload that we don't need to suffer from. But, I'd keep the posibility of passing an int for backward compatilibility, and throw a debugging message to developers to tell them that they should update their code. This would require a note in upgrade.txt as well. I know it's not a blocker, but I don't think raising another issue for this minor improvement is worth it. Please standardise the use of your quotes in your array keys, sometimes they are single ones, sometimes double. I don't mind which one, even if the single ones seem more used, but in one diff you can expect to have all the same. notes/externallib.php 202: Guideline issue 215: Whitespace issue 225-227: Documentation issue 243: Guideline issue 245: Could you explain why the context needs to be validated? The above method will return an exception if the context is not found. 251 You pass too many paramaters to the constructor, but anyway you can't use the external_warning class here, you have to set an array of information. Grep for the use of external_warnings. In any case a warning code is required for app developers to handle the warning easily. Could you add some Unit Test to check the external_warning when note_delete() fails? 266: To me this function should not return anything, except an array of external_warnings if we encountered errors. Returnings the IDs passed previously is unnecessary and has no value for the client. 275: Whitespace issue 304-306: Documentation issue 320 Could you comment that you're actually converting an object to array? Do you need to do so? What happens if the note does not exist? Please add Unit Test for non-existing note. 326: I think it'd be easier to read if you were constructing the array or information about the node yourself, instead of casting the object to array. 366: Shouldn't publishstate be an optional_value as it's not likely that it would change on every edit. 379-381: Documentation issue 405: Guideline issue 406: Guideline issue (new class must end with parenthesis) 422: Shouldn't you throw an exception (or external warning) when the publishstate is not an expected value? 430: See above (#251) 445: See above (#266) The parameters passed to your WS methods do not need to be nested such as $notes ['noteids'] = array(1, 2, 3). It is simpler if they are $notes = array(1, 2, 3). You just nedd to remove the single_structure. If you do so, $notes could perhaps be renamed $noteids. General: Your Doc Blocks could be more descriptive, "Returns description of method parameters" could state function name. notes/lib.php You could probably put everything on one line, or two but not three. And convert to moodle_url() Do you think you should write something in upgrade.txt as you define a new log type (note:add)? notes/tests/externallib_test.php Delete, Get: Add tests passing multiple IDs at once. Delete, Get: Add tests passing unexisting IDs. 159: You should remove that capability as soon as the notes are created. Update: You could test each possible value of publishstate. In your tests, you should create a user and use that one instead of $USER as it is, it's less random. Cheers, Fred
            Hide
            Jason Fowler added a comment -

            notes/externallib.php
            5 -> it doesn't validfate the context, it validates the user's ability to operate in the context. it was used in the create_notes WS, and when I looked at it, it made sense to keep it for the others.

            8 -> I decided to do it this way as it allows the app developer a way to easily identify the notes that have successfully been acted upon. It may not be a requirement for this, but it is certainly useful in my opinion.

            notes/lib.php

            3 -> it wasn't added as part of the webservices - it was being used in other areas of the code (user/addnote.php)

            Show
            Jason Fowler added a comment - notes/externallib.php 5 -> it doesn't validfate the context, it validates the user's ability to operate in the context. it was used in the create_notes WS, and when I looked at it, it made sense to keep it for the others. 8 -> I decided to do it this way as it allows the app developer a way to easily identify the notes that have successfully been acted upon. It may not be a requirement for this, but it is certainly useful in my opinion. notes/lib.php 3 -> it wasn't added as part of the webservices - it was being used in other areas of the code (user/addnote.php)
            Hide
            Jason Fowler added a comment -

            All of fred's suggestions and concerns have been made/addressed. Pushing for integration.

            Show
            Jason Fowler added a comment - All of fred's suggestions and concerns have been made/addressed. Pushing for integration.
            Hide
            Jason Fowler added a comment -

            Just noticed some white space issues so I've updated the patch

            Show
            Jason Fowler added a comment - Just noticed some white space issues so I've updated the patch
            Hide
            Damyon Wiese added a comment -

            Hi Jason,

            It's a bit hard to match up your comments with Freds review - the numbers don't seem to make sense.

            I agree with Fred that your return signature should just be an array of external warnings, the note id is part of the warning and successful operations should just not be returned (no warnings = 100% success). The structure you have created is too nested and too much work for a client to parse to see if the operation was successful. This is the only major issue - so if you can fix this up I will be happy to integrate it.

            I also found incorrect whitespace in this patch (commas with no spaces, and a few places where you have cut and pasted comments from another function - search for ?elete).

            Thanks, Damyon

            Show
            Damyon Wiese added a comment - Hi Jason, It's a bit hard to match up your comments with Freds review - the numbers don't seem to make sense. I agree with Fred that your return signature should just be an array of external warnings, the note id is part of the warning and successful operations should just not be returned (no warnings = 100% success). The structure you have created is too nested and too much work for a client to parse to see if the operation was successful. This is the only major issue - so if you can fix this up I will be happy to integrate it. I also found incorrect whitespace in this patch (commas with no spaces, and a few places where you have cut and pasted comments from another function - search for ?elete). Thanks, Damyon
            Hide
            Jason Fowler added a comment -

            Thanks Damyon, I've cleaned the white space and comments, and have restructured the return values for the update and delete so they no longer return the note id of successful actions.

            Show
            Jason Fowler added a comment - Thanks Damyon, I've cleaned the white space and comments, and have restructured the return values for the update and delete so they no longer return the note id of successful actions.
            Hide
            Damyon Wiese added a comment -

            Thanks Jason, but the update is not complete:

            code like this:

            +                $resultnote['noteid'] = $noteid;
            +                $resultnote['warnings'] = array(array('item'=>'note', 'itemid'=>$noteid, 'warningcode'=>'badid', 'message'=>'Note does not exist'));
            ...
            +       return $resultnotes;
            

            should be more like this:

            +                $warnings[] = array('item'=>'note', 'itemid'=>$noteid, 'warningcode'=>'badid', 'message'=>'Note does not exist');
            ...
            +       return $warnings;
            

            (Let me know if this does not make sense).

            Show
            Damyon Wiese added a comment - Thanks Jason, but the update is not complete: code like this: + $resultnote['noteid'] = $noteid; + $resultnote['warnings'] = array(array('item'=>'note', 'itemid'=>$noteid, 'warningcode'=>'badid', 'message'=>'Note does not exist')); ... + return $resultnotes; should be more like this: + $warnings[] = array('item'=>'note', 'itemid'=>$noteid, 'warningcode'=>'badid', 'message'=>'Note does not exist'); ... + return $warnings; (Let me know if this does not make sense).
            Hide
            Jason Fowler added a comment -

            All updated now Damyon.

            Show
            Jason Fowler added a comment - All updated now Damyon.
            Hide
            Damyon Wiese added a comment -

            Sorry Jason, this is still not right.

            } catch (dml_missing_record_exception $e) {
            +                // Catch the exception to prevent it from halting the bulk process.
            +                // Prepare a warning error message.
            +                $resultnote['noteid'] = $note['id'];
            +                $resultnote['errormessage'] = array('item'=>'note', 'itemid'=>$note["id"], 'warningcode'=>'badid', 'message'=>'Note does not exist');
            +            }
            

            Should be $warnings[] = blah

            Show
            Damyon Wiese added a comment - Sorry Jason, this is still not right. } catch (dml_missing_record_exception $e) { + // Catch the exception to prevent it from halting the bulk process. + // Prepare a warning error message. + $resultnote['noteid'] = $note['id']; + $resultnote['errormessage'] = array('item'=>'note', 'itemid'=>$note["id"], 'warningcode'=>'badid', 'message'=>'Note does not exist'); + } Should be $warnings[] = blah
            Hide
            Jason Fowler added a comment -

            Okay, hopefully having removed that try catch, and doing it the right way now, everything should be fine.

            Show
            Jason Fowler added a comment - Okay, hopefully having removed that try catch, and doing it the right way now, everything should be fine.
            Hide
            Damyon Wiese added a comment -

            Thanks Jason,

            The fixes to update and delete look correct. However, still I think that the nesting in get_notes is too much. It also should return a list of notes and a list of warnings instead of nesting the warnings inside each note (there are some implications for optional parameters there - ie you have not marked warnings as optional in the response but do not always include it - this kind of thing breaks strict protocols like soap.

            Taking this out of the integration queue for this week - feel free to resubmit once you've had a look at the warnings in get_notes.

            Show
            Damyon Wiese added a comment - Thanks Jason, The fixes to update and delete look correct. However, still I think that the nesting in get_notes is too much. It also should return a list of notes and a list of warnings instead of nesting the warnings inside each note (there are some implications for optional parameters there - ie you have not marked warnings as optional in the response but do not always include it - this kind of thing breaks strict protocols like soap. Taking this out of the integration queue for this week - feel free to resubmit once you've had a look at the warnings in get_notes.
            Hide
            CiBoT added a comment -

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

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

            Thanks!

            Show
            Damyon Wiese 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. Thanks!
            Hide
            Jason Fowler added a comment -

            Rebased

            Show
            Jason Fowler added a comment - Rebased
            Hide
            Damyon Wiese added a comment -

            Thanks Jason,

            This webservice has been integrated to master now. Thanks for all the updates to this issue - it finally made it.

            Show
            Damyon Wiese added a comment - Thanks Jason, This webservice has been integrated to master now. Thanks for all the updates to this issue - it finally made it.
            Hide
            Damyon Wiese added a comment -

            Also - don't forget to add your new webservices here:

            http://docs.moodle.org/dev/Web_services_Roadmap

            Thanks, Damyon

            Show
            Damyon Wiese added a comment - Also - don't forget to add your new webservices here: http://docs.moodle.org/dev/Web_services_Roadmap Thanks, Damyon
            Hide
            David Monllaó added a comment -

            Passing as unit tests are running on the CI server

            Show
            David Monllaó added a comment - Passing as unit tests are running on the CI server
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Because

            A
            MARVELOUS
            A       U
            Z  YOU  P
            I  ARE  E
            N  PPL  R
            G       B
              TNKS! 
            

            Closing, ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Because A MARVELOUS A U Z YOU P I ARE E N PPL R G B TNKS! Closing, ciao
            Hide
            Francois J added a comment -

            Hi,

            I have issues with the delete_notes WS functions...
            After a review of the notes/externallib.php code review, a fix could be, on line 229:

            $params = self::validate_parameters(self::delete_notes_parameters(), array('notes' => $notes));
            

            Instead of:

            $params = self::validate_parameters(self::delete_notes_parameters(), $notes);
            

            The errors I have with the WS function are:
            Calling parameters do not match signature
            (If I incorporate the 'notes' index that is missing) Or
            Missing required key in single structure: notes
            (If I do not incorporate it...)

            As I have tested all the combinations possible for the WS function parameter, I hope it is not a misunderstanding on my side...

            Show
            Francois J added a comment - Hi, I have issues with the delete_notes WS functions... After a review of the notes/externallib.php code review, a fix could be, on line 229 : $params = self::validate_parameters(self::delete_notes_parameters(), array( 'notes' => $notes)); Instead of: $params = self::validate_parameters(self::delete_notes_parameters(), $notes); The errors I have with the WS function are: Calling parameters do not match signature (If I incorporate the 'notes' index that is missing) Or Missing required key in single structure: notes (If I do not incorporate it...) As I have tested all the combinations possible for the WS function parameter, I hope it is not a misunderstanding on my side...

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: