Details

    • Testing Instructions:
      Hide
      1. Create a courses.
      2. Create a forum in this course.
      3. Add a discussion with a few replies.
      4. Add another forum that is hidden with an impossible conditional field (eg. firstname is empty).
      5. Enrol a user into the course.
      6. Create a web service token for the enrolled student to use for the following steps when making calls with the web service.
      7. Create a web service client (similar to the one attached) that includes the first forum id in the params.
      8. Execute the client and ensure the discussions for that forum are returned.
      9. Change the params to include the id of the hidden forum and ensure exception thrown.
      10. Run the PHP Unit test phpunit mod/forum/tests/externallib_test.php.
      11. Run the PHP Unit test phpunit lib/testing/tests/generator_test.php.
      Show
      Create a courses. Create a forum in this course. Add a discussion with a few replies. Add another forum that is hidden with an impossible conditional field (eg. firstname is empty). Enrol a user into the course. Create a web service token for the enrolled student to use for the following steps when making calls with the web service. Create a web service client (similar to the one attached) that includes the first forum id in the params. Execute the client and ensure the discussions for that forum are returned. Change the params to include the id of the hidden forum and ensure exception thrown. Run the PHP Unit test phpunit mod/forum/tests/externallib_test.php. Run the PHP Unit test phpunit lib/testing/tests/generator_test.php.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-30102_master

      Gliffy Diagrams

      1. ws_client.php
        1 kB
        Mark Nelson
      2. ws_curl.php
        18 kB
        Mark Nelson

        Issue Links

          Activity

          Hide
          Juan Leyva added a comment -

          Hi,

          I think that in this function we could have the same privacy problems that in MDL-37247, at forum level you need to check for groups/groupings/conditional activities also (not only permissions at context level)

          Regards

          Show
          Juan Leyva added a comment - Hi, I think that in this function we could have the same privacy problems that in MDL-37247 , at forum level you need to check for groups/groupings/conditional activities also (not only permissions at context level) Regards
          Hide
          Mark Nelson added a comment -

          Hi Juan, good point. Not sure how I forgot it again! Will adjust commit now.

          Show
          Mark Nelson added a comment - Hi Juan, good point. Not sure how I forgot it again! Will adjust commit now.
          Hide
          Mark Nelson added a comment -

          Ok, have updated. Thanks

          Show
          Mark Nelson added a comment - Ok, have updated. Thanks
          Hide
          Frédéric Massart added a comment -

          Hi Mark, great patch. Overall I only have minor comments, see the general ones for the ones that I'm concerned about.

          1. Our coding guide lines don't seem to precise if we need punctuations at the end of Doc Blocks but I'd say so, if I'm right you're missing a few in the data generator.
          2. Generator:622+696 I personally like the @return void in the doc blocks so that as a developer you don't have to scroll down to see if the information is missing of if there is no return. Also, the documentation defines a $userid which you're not using.
          3. GenratorTest:216+~249 Could you confirm the reason why the call getDataGenerator is using $this instead of self::getDataGenerator()?
          4. GenratorTest:287 The variable $forum1->id is not defined.
          5. WS:163 A capital to 'Forum ID'?
          6. WS:199 Would get_field_sql be appropriate here, to lighten the query a bit? Also, should you handle the possible 'false' value returned by get_*sql()? _php -r "var_dump(array_keys((array) false));"
          7. WS:206 I think you should validate the parameters before the if. Especially if the if if based on a passed parameter.
          8. WS:236 You should not access $instances directly but use get_instances_of instead. (See Doc Block of $instances)
          9. WS:251 I'd require the capability as soon as possible to prevent unnecessary code execution.
          10. WS:263 $cantrack and $forumtracked should be initialised above, or they could inherit values from a previous loop.
          11. WS:271 To me the code below would be more readable if $d was $discussion, but that's just me.
          12. WS:279+320 If this is relevant I think that using user_get_user_details() instead of a raw query is more appropriate. What do you think about it?
          13. WS:304 Shouldn't the default value be an int?
          14. WS:350,351 Shouldn't that be PARAM_INT?
          15. Forum/Lib What do you think about throwing a message to the developer when $unused is not null?
          16. Forum/Test:200 stdClass should have parenthesis.
          17. Forum/Test:253 Typo in the comment: don't.
          18. Forum/Test:386 Could you confirm why you're cleaning the return value after assertEquals?
          19. Forum/Test:404 Could you confirm that the rest of the test will be executed even after the exception is caught?

          General

          1. If one of the forums in the list requires more permission, or is hidden, then the whole query would fail throwing an exception. I see two issues here, the first one is that I should still receive information for the other ones, or the very least, debug which forum caused the query to fail.
          2. I am not sure about querying all the discussions for all the courses the user is enrolled in. I think that this could lead to a potentially massive request, and heavy processing. Perhaps we could set a default limit of discussions newer than x, or ask for a course ID when the forum is not specified, or both. Of course the developer could still send a massively big query to Moodle, but they would have to ask for it, instead of just using our default parameters.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Hi Mark, great patch. Overall I only have minor comments, see the general ones for the ones that I'm concerned about. Our coding guide lines don't seem to precise if we need punctuations at the end of Doc Blocks but I'd say so, if I'm right you're missing a few in the data generator. Generator:622+696 I personally like the @return void in the doc blocks so that as a developer you don't have to scroll down to see if the information is missing of if there is no return. Also, the documentation defines a $userid which you're not using. GenratorTest:216+~249 Could you confirm the reason why the call getDataGenerator is using $this instead of self::getDataGenerator()? GenratorTest:287 The variable $forum1->id is not defined. WS:163 A capital to 'Forum ID'? WS:199 Would get_field_sql be appropriate here, to lighten the query a bit? Also, should you handle the possible 'false' value returned by get_* sql()? _php -r "var_dump(array_keys((array) false));" WS:206 I think you should validate the parameters before the if . Especially if the if if based on a passed parameter. WS:236 You should not access $instances directly but use get_instances_of instead. (See Doc Block of $instances) WS:251 I'd require the capability as soon as possible to prevent unnecessary code execution. WS:263 $cantrack and $forumtracked should be initialised above, or they could inherit values from a previous loop. WS:271 To me the code below would be more readable if $d was $discussion, but that's just me. WS:279+320 If this is relevant I think that using user_get_user_details() instead of a raw query is more appropriate. What do you think about it? WS:304 Shouldn't the default value be an int? WS:350,351 Shouldn't that be PARAM_INT? Forum/Lib What do you think about throwing a message to the developer when $unused is not null? Forum/Test:200 stdClass should have parenthesis. Forum/Test:253 Typo in the comment: don't . Forum/Test:386 Could you confirm why you're cleaning the return value after assertEquals? Forum/Test:404 Could you confirm that the rest of the test will be executed even after the exception is caught? General If one of the forums in the list requires more permission, or is hidden, then the whole query would fail throwing an exception. I see two issues here, the first one is that I should still receive information for the other ones, or the very least, debug which forum caused the query to fail. I am not sure about querying all the discussions for all the courses the user is enrolled in. I think that this could lead to a potentially massive request, and heavy processing. Perhaps we could set a default limit of discussions newer than x, or ask for a course ID when the forum is not specified, or both. Of course the developer could still send a massively big query to Moodle, but they would have to ask for it, instead of just using our default parameters. Cheers, Fred
          Hide
          Dan Poltawski added a comment -

          If one of the forums in the list requires more permission, or is hidden, then the whole query would fail throwing an exception. I see two issues here, the first one is that I should still receive information for the other ones, or the very least, debug which forum caused the query to fail.

          1. What is the use case for this when someone gives intentionally incorrect parameters? Why should we handle this gracefully? Surely they should be checking their parameters before using it.
          2. If you are throwing an exception, it makes total sense that the execution is halted, no you shouldn't still recieve the information (Of course, I don't write web services, so there might be a use case to be more tolerant or a principle behind how we design web services).

          I am not sure about querying all the discussions for all the courses the user is enrolled in. I think that this could lead to a potentially massive request, and heavy processing. Perhaps we could set a default limit of discussions newer than x, or ask for a course ID when the forum is not specified, or both. Of course the developer could still send a massively big query to Moodle, but they would have to ask for it, instead of just using our default parameters.

          I agree. As Mark commented to me in a private chat, perhaps we should enforce the user of parameters, or at least a course as you suggest.

          Show
          Dan Poltawski added a comment - If one of the forums in the list requires more permission, or is hidden, then the whole query would fail throwing an exception. I see two issues here, the first one is that I should still receive information for the other ones, or the very least, debug which forum caused the query to fail. What is the use case for this when someone gives intentionally incorrect parameters? Why should we handle this gracefully? Surely they should be checking their parameters before using it. If you are throwing an exception, it makes total sense that the execution is halted, no you shouldn't still recieve the information (Of course, I don't write web services, so there might be a use case to be more tolerant or a principle behind how we design web services). I am not sure about querying all the discussions for all the courses the user is enrolled in. I think that this could lead to a potentially massive request, and heavy processing. Perhaps we could set a default limit of discussions newer than x, or ask for a course ID when the forum is not specified, or both. Of course the developer could still send a massively big query to Moodle, but they would have to ask for it, instead of just using our default parameters. I agree. As Mark commented to me in a private chat, perhaps we should enforce the user of parameters, or at least a course as you suggest.
          Hide
          Mark Nelson added a comment -

          Hi Fred, thanks for that. I have addressed the majority of the points you have stated.

          Regarding the general ones.

          1. In reality the majority of cases this function will be used is by a user who has already performed a call to get_forums, which will return a list of forums the user can see (ie, they are not hidden to them). The only way an exception will be thrown is if they hard code a hidden forum into their code or use old data from get_forums. Also, if no forums are specified then only the discussions belonging to the forums the user can view are returned, no exceptions are thrown. So I think this is fine as it is.
          2. Good point. Maybe we could require that the user specifies a forum id? Or should we add additional parameters?
          Show
          Mark Nelson added a comment - Hi Fred, thanks for that. I have addressed the majority of the points you have stated. Regarding the general ones. In reality the majority of cases this function will be used is by a user who has already performed a call to get_forums, which will return a list of forums the user can see (ie, they are not hidden to them). The only way an exception will be thrown is if they hard code a hidden forum into their code or use old data from get_forums. Also, if no forums are specified then only the discussions belonging to the forums the user can view are returned, no exceptions are thrown. So I think this is fine as it is. Good point. Maybe we could require that the user specifies a forum id? Or should we add additional parameters?
          Hide
          Damyon Wiese added a comment -

          Thanks Mark this has been added to master only.

          The webservice looks good and had a nice set of tests.

          Show
          Damyon Wiese added a comment - Thanks Mark this has been added to master only. The webservice looks good and had a nice set of tests.
          Hide
          Rossiani Wijaya added a comment -

          This is working great.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This is working great. Test passed.
          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
          Damyon Wiese added a comment -

          Congratulations! This issue has been resolved. Thanks for helping to make Moodle better for everyone!

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Congratulations! This issue has been resolved. Thanks for helping to make Moodle better for everyone! Regards, Damyon
          Hide
          Petr Skoda added a comment -

          Hi! forum code can not be in core, the create_forum_post() and create_forum_discussion() generator MUST be in forum generator.

          Show
          Petr Skoda added a comment - Hi! forum code can not be in core, the create_forum_post() and create_forum_discussion() generator MUST be in forum generator.
          Hide
          Damyon Wiese added a comment -

          Those 2 functions are in the generator - looking at it now - the coding exceptions refer to phpunit_util::create_forum_post() - but that looks like a mistake.

          Is that what you meant?

          Show
          Damyon Wiese added a comment - Those 2 functions are in the generator - looking at it now - the coding exceptions refer to phpunit_util::create_forum_post() - but that looks like a mistake. Is that what you meant?
          Hide
          Petr Skoda added a comment -
          Show
          Petr Skoda added a comment - See MDL-38581
          Hide
          Damyon Wiese added a comment -

          Ah looked too quickly - yes that definitely shouldn't be there.

          Show
          Damyon Wiese added a comment - Ah looked too quickly - yes that definitely shouldn't be there.
          Hide
          trushal added a comment - - edited

          One question with this webservice for iOS

          How do i get full discussion for forum with default webservice of moodles

          currently i am not able to see what written by others in forum discussion.

          Also can we get private files list through webservice in iOS

          Thanks

          Show
          trushal added a comment - - edited One question with this webservice for iOS How do i get full discussion for forum with default webservice of moodles currently i am not able to see what written by others in forum discussion. Also can we get private files list through webservice in iOS Thanks
          Hide
          Frédéric Massart added a comment -

          Hi Trushal,

          Can I suggest you to post in the forums on Moodle.org? This is a better place to get an answer than the tracker, especially as this issue is "Closed".

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Hi Trushal, Can I suggest you to post in the forums on Moodle.org? This is a better place to get an answer than the tracker, especially as this issue is "Closed". Cheers, Fred

            People

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

              Dates

              • Created:
                Updated:
                Resolved: