Details

    • Type: Sub-task
    • Status: Closed
    • Priority: 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

      Description

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

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              jerome 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
              jerome 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
              markn 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
              markn 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
              jerome 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
              jerome 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
              markn 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
              markn 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
              jerome Jérôme Mouneyrac added a comment -

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

              Show
              jerome Jérôme Mouneyrac added a comment - looks good to me, you can send it to integration. Cheers.
              Hide
              stronk7 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
              stronk7 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 CiBoT added a comment -

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

              Show
              cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
              Hide
              markn 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
              markn 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
              stronk7 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
              stronk7 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
              jerome Jérôme Mouneyrac added a comment -

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

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

              Shifting to the next sprint.

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

              Integrated, thanks!

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

              Tested each section and passed.

              Show
              abgreeve Adrian Greeve added a comment - Tested each section and passed.
              Hide
              stronk7 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
              stronk7 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:
                    Fix Release Date:
                    14/May/13