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

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

          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