Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2, 2.5
    • Fix Version/s: 2.5
    • Component/s: Forum, Web Services
    • Labels:
      None
    • Testing Instructions:
      Hide

      Test pre-requisites

      • Create a course.
      • Create a forum and assignment in this course.
      • Enrol a user into the course.
      • Create a web service token for the user and the admin.

      Test 1

      1. Create a web service client (similar to the one attached) using the students token, the delete_modules function and passing the course module ids in the params.
      2. Execute the client and ensure you get a require_capability warning.

      Test 2

      1. Edit the token in the web service client and set it to the admin's token.
      2. Ensure you can delete the modules.

      Test 3

      1. Edit the params in the web service to invalid course module ids.
      2. Ensure exception is thrown.

      Test 4

      1. Execute phpunit course/tests/externallib_test.php and ensure there are no failures/errors.
      Show
      Test pre-requisites Create a course. Create a forum and assignment in this course. Enrol a user into the course. Create a web service token for the user and the admin. Test 1 Create a web service client (similar to the one attached) using the students token, the delete_modules function and passing the course module ids in the params. Execute the client and ensure you get a require_capability warning. Test 2 Edit the token in the web service client and set it to the admin's token. Ensure you can delete the modules. Test 3 Edit the params in the web service to invalid course module ids. Ensure exception is thrown. Test 4 Execute phpunit course/tests/externallib_test.php and ensure there are no failures/errors.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-30098_master
    • Rank:
      24729

      Description

      Create a delete_modules() WS function that can be used by all modules.

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

        Issue Links

          Activity

          Hide
          Jérôme Mouneyrac added a comment - - edited

          I started to implement a lib function to create any module: https://github.com/mouneyrac/moodle/tree/MDL-30099
          It is basically a replicate of the front end code logic.

          Show
          Jérôme Mouneyrac added a comment - - edited I started to implement a lib function to create any module: https://github.com/mouneyrac/moodle/tree/MDL-30099 It is basically a replicate of the front end code logic.
          Hide
          Mark Nelson added a comment - - edited

          Note to integrator - this diff may contain two commits, one for a separate tracker issue and this one. The reason for this is because one depends on the other, so please, when integrating ensure the blocked issue has been integrated first and then ignore that commit in this diff.

          Show
          Mark Nelson added a comment - - edited Note to integrator - this diff may contain two commits, one for a separate tracker issue and this one. The reason for this is because one depends on the other, so please, when integrating ensure the blocked issue has been integrated first and then ignore that commit in this diff.
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Mark. My review:

          delete_forum

          • I can not see anything specific to forum in the code. It is a generic delete_modules() function - which is better than writing a function for each specific modules. Verify that course_delete_module() is enough to delete any kind of modules properly. Then rename your function, and explain what's function is about in the phpdoc/service.php/description functions. To know if you wrote all the docs properly, check what your obtain in the API documentation: Plugins > Web services > API documentation. A client developer should be able to understand what the function does and how to use it from reading this page only.
          • when possible, bulk functions are named with a 's' at the end: YYY_XXXs
          • Remove the '' in the _parameters() function, it's a typo: ... PARAM_INT, 'course module ID', '', VALUE_REQUIRED, ... (PS: you have the issue in get_forum too)
          • This is a picky note : $arrcourseschecked could be $checkedcourses.(PS: you have the issue in get_forum too)
          • return an external_warnings when the course module doesn't exist. It is important for the client to know that it needs to clean its local out-of-sync database.

          get_forum

          • I just guessed what the function does from reading the parameters. I would change the function name for get_course_forums() or get_forums_by_courses() or get_my_forums(). Your service.php information need to be fixed too: 'Returns a feedback instance from the database. Don't forget to verify that the generated documentation is explicit and useful for the client developer.
          • Optional: I think it would not be worth to have some prebuild array containing the description of the common part of modules. So we don't have to write them for each get_MODULEs() function. It would also be good if we could have some generic code to call in each get_MODULEs. Then writing a new get_MODULEs would be about taking care of the specific attribut of each modules.
          Show
          Jérôme Mouneyrac added a comment - Thanks Mark. My review: delete_forum I can not see anything specific to forum in the code. It is a generic delete_modules() function - which is better than writing a function for each specific modules. Verify that course_delete_module() is enough to delete any kind of modules properly. Then rename your function, and explain what's function is about in the phpdoc/service.php/description functions. To know if you wrote all the docs properly, check what your obtain in the API documentation: Plugins > Web services > API documentation. A client developer should be able to understand what the function does and how to use it from reading this page only. when possible, bulk functions are named with a 's' at the end: YYY_XXXs Remove the '' in the _parameters() function, it's a typo: ... PARAM_INT, 'course module ID', '', VALUE_REQUIRED, ... (PS: you have the issue in get_forum too) This is a picky note : $arrcourseschecked could be $checkedcourses.(PS: you have the issue in get_forum too) return an external_warnings when the course module doesn't exist. It is important for the client to know that it needs to clean its local out-of-sync database. get_forum I just guessed what the function does from reading the parameters. I would change the function name for get_course_forums() or get_forums_by_courses() or get_my_forums(). Your service.php information need to be fixed too: 'Returns a feedback instance from the database. Don't forget to verify that the generated documentation is explicit and useful for the client developer. Optional: I think it would not be worth to have some prebuild array containing the description of the common part of modules. So we don't have to write them for each get_MODULEs() function. It would also be good if we could have some generic code to call in each get_MODULEs. Then writing a new get_MODULEs would be about taking care of the specific attribut of each modules.
          Hide
          Mark Nelson added a comment -

          Hi Jerome, I created a separate tracker issue MDL-37247 for retrieving a forum instance. This issue will now solely focus on creating a generic web service function that utilises the newly created core delete function to delete any modules' instances.

          Show
          Mark Nelson added a comment - Hi Jerome, I created a separate tracker issue MDL-37247 for retrieving a forum instance. This issue will now solely focus on creating a generic web service function that utilises the newly created core delete function to delete any modules' instances.
          Hide
          Jérôme Mouneyrac added a comment -

          looks good to me, you can send it to integration. Cheers.

          Show
          Jérôme Mouneyrac added a comment - looks good to me, you can send it to integration. Cheers.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Sorry, have to reopen this, no matter code looks perfect. It's conflicting badly with recent added stuff. Plz, solve that and resend it back to integration.

          TIA and ciao

          Edited: I forgot it, surely it would be worth linking this to the META about deprecation for 2.5 and also adding the api change to the corresponding upgrade.txt file.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Sorry, have to reopen this, no matter code looks perfect. It's conflicting badly with recent added stuff. Plz, solve that and resend it back to integration. TIA and ciao Edited: I forgot it, surely it would be worth linking this to the META about deprecation for 2.5 and also adding the api change to the corresponding upgrade.txt file.
          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
          Mark Nelson added a comment - - edited

          Hi Eloy, this should not be integrated before MDL-37082 (this diff does include that commit however). Once MDL-37082 has been integrated and accepted I will fix any issues here. I do not believe the dev_docs_required label should be placed here, but the other issue. Thanks.

          Show
          Mark Nelson added a comment - - edited Hi Eloy, this should not be integrated before MDL-37082 (this diff does include that commit however). Once MDL-37082 has been integrated and accepted I will fix any issues here. I do not believe the dev_docs_required label should be placed here, but the other issue. Thanks.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          (from jabber) Eloy: aha, perfect. I really didn't see MDL-37082 yesterday, lol and assumed you were changing the api and implementing the WS together. It seemed a bit "too much" at first sight, but decided to allow it as far as the change & unification was pretty good. But agree, MDL-37082 is the one with the api change and dev docs required. Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - (from jabber) Eloy: aha, perfect. I really didn't see MDL-37082 yesterday, lol and assumed you were changing the api and implementing the WS together. It seemed a bit "too much" at first sight, but decided to allow it as far as the change & unification was pretty good. But agree, MDL-37082 is the one with the api change and dev docs required. Thanks!
          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
          Michael de Raadt added a comment -

          Shifting to the next sprint.

          Show
          Michael de Raadt added a comment - Shifting to the next sprint.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          Adrian Greeve added a comment -

          Tested each section and passed.

          Show
          Adrian Greeve added a comment - Tested each section and passed.
          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

            People

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

              Dates

              • Created:
                Updated:
                Resolved: