Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Calendar, Web Services
    • Labels:
    • Testing Instructions:
      Hide
      1. Run unit tests and make sure all is good
      2. Make sure the new calendar units tests are executed with the phpunit run. Check phpunit.xml and make sure there is an entry for calendar.
      3. Setup a ws client of your choice. (/moodle/admin/settings.php?section=webservicesoverview might help)
      4. create 6 calendar events (1 site, 1 course with 2 childern(using repeat in new event form), 1 group event, 1 user event)
      5. Goto event table in database using database browser of your choice and note the ids of each of them.
      6. Lets say the ids are 1, 2-3-4,5,6 respectively
      7. Assign a user the permission to make ws calls
      8. Enrol this user in the course and group for events above as student.
      9. Modfiy student role and remove the following permissions:-
        1. moodle/calendar:manageownentries from user context
        2. moodle/calendar:managegroupentries from course context
        3. moodle/calendar:manageentries from course context
        4. moodle/calendar:manageentries from site context (You will need to enable the student role for site context from admin>users>permissions>define role)
      10. try making the ws call to delete all events, param would be something like (assuming you are using xml rpc client, if you are using something else refer the api documentation for correct params format)
                $events = array(
                    array('eventid' => $siteevent->id, 'repeat' => 0),
                    array('eventid' => $courseevent->id, 'repeat' => 0),
                    array('eventid' => $userevent->id, 'repeat' => 0),
                    array('eventid' => $groupevent->id, 'repeat' => 0)
                );
        

        Since the user doesn't have the permissions the events should not be deleted.

      11. Give back the user cap 9.1 and make a ws call with the following param, and make sure it is deleted
                $events = array(
                    array('eventid' => $userevent->id, 'repeat' => 0),
                );
        
      12. Give back user all caps as step 9 and making delete call with following params:-
                $events = array(
                    array('eventid' => $siteevent->id, 'repeat' => 0),
                    array('eventid' => $courseevent->id, 'repeat' => 1),
                    array('eventid' => $groupevent->id, 'repeat' => 0)
                );
        

        Make sure all events are deleted.

      13. Treat yourself for getting through this crappy test
      Show
      Run unit tests and make sure all is good Make sure the new calendar units tests are executed with the phpunit run. Check phpunit.xml and make sure there is an entry for calendar. Setup a ws client of your choice. (/moodle/admin/settings.php?section=webservicesoverview might help) create 6 calendar events (1 site, 1 course with 2 childern(using repeat in new event form), 1 group event, 1 user event) Goto event table in database using database browser of your choice and note the ids of each of them. Lets say the ids are 1, 2-3-4,5,6 respectively Assign a user the permission to make ws calls Enrol this user in the course and group for events above as student. Modfiy student role and remove the following permissions:- moodle/calendar:manageownentries from user context moodle/calendar:managegroupentries from course context moodle/calendar:manageentries from course context moodle/calendar:manageentries from site context (You will need to enable the student role for site context from admin>users>permissions>define role) try making the ws call to delete all events, param would be something like (assuming you are using xml rpc client, if you are using something else refer the api documentation for correct params format) $events = array( array('eventid' => $siteevent->id, 'repeat' => 0), array('eventid' => $courseevent->id, 'repeat' => 0), array('eventid' => $userevent->id, 'repeat' => 0), array('eventid' => $groupevent->id, 'repeat' => 0) ); Since the user doesn't have the permissions the events should not be deleted. Give back the user cap 9.1 and make a ws call with the following param, and make sure it is deleted $events = array( array('eventid' => $userevent->id, 'repeat' => 0), ); Give back user all caps as step 9 and making delete call with following params:- $events = array( array('eventid' => $siteevent->id, 'repeat' => 0), array('eventid' => $courseevent->id, 'repeat' => 1), array('eventid' => $groupevent->id, 'repeat' => 0) ); Make sure all events are deleted. Treat yourself for getting through this crappy test
    • Pull Master Branch:
      MDL-37077-master
    • Rank:
      46631

      Description

      WS Api to delete calendar events

        Issue Links

          Activity

          Hide
          Ankit Agarwal added a comment -

          Things that need to be considered here are:-

          1. Bulk delete for multiple events
          2. Delete event of a repeated series
          3. Ical subsciptions delete? (I am not including this atm, as this neeeds separate apis imo)
          Show
          Ankit Agarwal added a comment - Things that need to be considered here are:- Bulk delete for multiple events Delete event of a repeated series Ical subsciptions delete? (I am not including this atm, as this neeeds separate apis imo)
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Hi Ankit, minor notes about the web service (I didn't check the code logic itself, I let that to the peer-reviewer):

          • bump main version.php (if it was a plugin bump the plugin version.php). Moodle install new web service during update.
          • we don't need any $transaction->rollback($e); in external functions. It is the server that handle it (rest use abort_all_db_transactions)
          • I would throw the exact same exception: throw $ex;
          • You don't need to return null after throw exception as the code is not executed
          • But at the end I would remove the entire try catch. I don't think it's useful, except if you want to catch specific exception and change them to something more clear for a client.
          • I'll add @return null in the phpdoc

          except this minor things, all good, thanks

          Show
          Jérôme Mouneyrac added a comment - - edited Hi Ankit, minor notes about the web service (I didn't check the code logic itself, I let that to the peer-reviewer): bump main version.php (if it was a plugin bump the plugin version.php). Moodle install new web service during update. we don't need any $transaction->rollback($e); in external functions. It is the server that handle it (rest use abort_all_db_transactions) I would throw the exact same exception: throw $ex; You don't need to return null after throw exception as the code is not executed But at the end I would remove the entire try catch. I don't think it's useful, except if you want to catch specific exception and change them to something more clear for a client. I'll add @return null in the phpdoc except this minor things, all good, thanks
          Hide
          Jérôme Mouneyrac added a comment -

          PS: for performance purpose you may want to modify the capability checks function to support bulk operations - so you can get the call out of the foreach. Personally I'm ok like it is at the moment, your choice

          Show
          Jérôme Mouneyrac added a comment - PS: for performance purpose you may want to modify the capability checks function to support bulk operations - so you can get the call out of the foreach. Personally I'm ok like it is at the moment, your choice
          Hide
          Ankit Agarwal added a comment -

          Hi Jerome,
          Thanks for your feedback.

          PS: for performance purpose you may want to modify the capability checks function to support bulk operations - so you can get the call out of the foreach. Personally I'm ok like it is at the moment, your choice

          I will leave that out for the moment as that is a fair bit of improvement and may require changes in a lot of places in calendar.

          I will update the patch based on all other suggestions you made.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Jerome, Thanks for your feedback. PS: for performance purpose you may want to modify the capability checks function to support bulk operations - so you can get the call out of the foreach. Personally I'm ok like it is at the moment, your choice I will leave that out for the moment as that is a fair bit of improvement and may require changes in a lot of places in calendar. I will update the patch based on all other suggestions you made. Thanks
          Hide
          Dan Poltawski added a comment -

          Urm, why are we returning null in this function? Surely it'd be better to return nothing or true?

          Show
          Dan Poltawski added a comment - Urm, why are we returning null in this function? Surely it'd be better to return nothing or true?
          Hide
          Ankit Agarwal added a comment -

          To keep it consistent with other Ws functions designs (delete_categories_returns(), delete_courses_returns()).

          Show
          Ankit Agarwal added a comment - To keep it consistent with other Ws functions designs (delete_categories_returns(), delete_courses_returns()).
          Hide
          Dan Poltawski added a comment -

          Hmm, thanks for explaining (seems weird to me)

          Show
          Dan Poltawski added a comment - Hmm, thanks for explaining (seems weird to me)
          Hide
          Dan Poltawski added a comment -

          Hi Ankit,

          This looks OK, but your unit tests need more work:

          • PHPUnit_Framework_Error_Warning - err what? Why are we testing for an PHPUNIT exception in a phpunit test? I don't even understand what you are trying to achieve there. But it isn't correct.
          • You should encapsulte your tests with an exception into a seperate function (test). Funamentally the test can't continue after an exception, so your code doing asserts after an exception is completely broken. That code will never get excuted. PHPunit isn't doing any magic, it works exactly like an exception works normally, the execution flow will leave the function as soon as an exception occurs.
          • I'm not quite sure why it was necesary to create a phpunit_calendar class rather than just a method of the existing test class (if its supposed to be more generic then it shoudn't be in the externallib file).
          Show
          Dan Poltawski added a comment - Hi Ankit, This looks OK, but your unit tests need more work: PHPUnit_Framework_Error_Warning - err what? Why are we testing for an PHPUNIT exception in a phpunit test? I don't even understand what you are trying to achieve there. But it isn't correct. You should encapsulte your tests with an exception into a seperate function (test). Funamentally the test can't continue after an exception, so your code doing asserts after an exception is completely broken. That code will never get excuted. PHPunit isn't doing any magic, it works exactly like an exception works normally, the execution flow will leave the function as soon as an exception occurs. I'm not quite sure why it was necesary to create a phpunit_calendar class rather than just a method of the existing test class (if its supposed to be more generic then it shoudn't be in the externallib file).
          Hide
          Ankit Agarwal added a comment -

          Hi Dan,
          Thanks for the detailed review.

          1. I was trying to test negative test case with invalid parameters, anyways you are right it is better to remove that portion.
          2. Will remove the assets after the exception
          3. I reason I created it as separate class was to make it little more generic, so that later it can be used in calendar unit tests as well, whenever that happens. The intial plan was to make data generators MDL-37084, but after discussion there I moved it in the externallib. Please suggest if you want it a method of the class itself.

          Thanks

          Show
          Ankit Agarwal added a comment - Hi Dan, Thanks for the detailed review. I was trying to test negative test case with invalid parameters, anyways you are right it is better to remove that portion. Will remove the assets after the exception I reason I created it as separate class was to make it little more generic, so that later it can be used in calendar unit tests as well, whenever that happens. The intial plan was to make data generators MDL-37084 , but after discussion there I moved it in the externallib. Please suggest if you want it a method of the class itself. Thanks
          Hide
          Dan Poltawski added a comment -

          I was trying to test negative test case with invalid parameters, anyways you are right it is better to remove that portion.

          It is fine [and encouraged] to do those tests for invalid data. But you shouldn't be testing for a phpunit exception, that is the internals of the phpunit. What actually comes from there?

          I reason I created it as separate class was to make it little more generic, so that later it can be used in calendar unit tests as well, whenever that happens. The intial plan was to make data generators MDL-37084, but after discussion there I moved it in the externallib. Please suggest if you want it a method of the class itself.

          I'd prefer for it to be a method of the class if its not being used outside of that externalib class.

          Show
          Dan Poltawski added a comment - I was trying to test negative test case with invalid parameters, anyways you are right it is better to remove that portion. It is fine [and encouraged] to do those tests for invalid data. But you shouldn't be testing for a phpunit exception, that is the internals of the phpunit. What actually comes from there? I reason I created it as separate class was to make it little more generic, so that later it can be used in calendar unit tests as well, whenever that happens. The intial plan was to make data generators MDL-37084 , but after discussion there I moved it in the externallib. Please suggest if you want it a method of the class itself. I'd prefer for it to be a method of the class if its not being used outside of that externalib class.
          Hide
          Dan Poltawski added a comment -

          (as phpunit_calendar namespace could easily be used again without someone noticing its used there)

          Show
          Dan Poltawski added a comment - (as phpunit_calendar namespace could easily be used again without someone noticing its used there)
          Hide
          Ankit Agarwal added a comment -

          It is fine [and encouraged] to do those tests for invalid data. But you shouldn't be testing for a phpunit exception, that is the internals of the phpunit. What actually comes from there?

          The $name param is mandatory, calendar_event() class was generating some error, which was converted to phpunit_framework exception somewhere down the line. Anyways it was a minor thing, am okay to remove those lines from the test.

          Show
          Ankit Agarwal added a comment - It is fine [and encouraged] to do those tests for invalid data. But you shouldn't be testing for a phpunit exception, that is the internals of the phpunit. What actually comes from there? The $name param is mandatory, calendar_event() class was generating some error, which was converted to phpunit_framework exception somewhere down the line. Anyways it was a minor thing, am okay to remove those lines from the test.
          Hide
          Dan Poltawski added a comment -

          Better that we find the right solution than ignore it.

          Can you be more specific about the error, maybe you need to use assertDebuggingCalled()?

          Show
          Dan Poltawski added a comment - Better that we find the right solution than ignore it. Can you be more specific about the error, maybe you need to use assertDebuggingCalled()?
          Hide
          Ankit Agarwal added a comment -

          Had a discussion with Dan, and we decided it was a minor thing and didn't need testing.

          Made other changes, pushing back for review.
          Thanks

          Show
          Ankit Agarwal added a comment - Had a discussion with Dan, and we decided it was a minor thing and didn't need testing. Made other changes, pushing back for review. Thanks
          Hide
          Dan Poltawski added a comment -

          To clarify: we decided it was php internals you were testing, not the code.

          Show
          Dan Poltawski added a comment - To clarify: we decided it was php internals you were testing, not the code.
          Hide
          Dan Poltawski added a comment -

          Integrated to master.

          Show
          Dan Poltawski added a comment - Integrated to master.
          Hide
          David Monllaó added a comment -

          It passes. Works as expected. I had to assign the user as student at system level

          Show
          David Monllaó added a comment - It passes. Works as expected. I had to assign the user as student at system level
          Hide
          Dan Poltawski added a comment -

          Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

          Show
          Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: