Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: Calendar, Web Services
    • Labels:
    • Testing Instructions:
      Hide
      1. This is moderately difficult test, requires fair bit of time testing various logic flow paths.
      2. Run unit tests and make sure all is good
      3. 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.
      4. Setup a ws client of your choice. (/moodle/admin/settings.php?section=webservicesoverview might help)
      5. User from now on refer's to an non-admin/managerial single user.
      6. create 6 calendar events (1 site, 1 course with 2 childern(using repeat in new event form), 1 group event, 1 user event)
      7. Goto event table in database using database browser of your choice and note the ids of each of them.
      8. Lets say the ids are 1, 2-3-4,5,6 respectively
      9. Assign a user the permission to make ws calls
      10. Make a call with params like this (Assuming your are using xmlrpc client) :-
                $events = array ('eventids' => array($siteevent->id), 'courseids' => array($course->id), 'groupids' => array($group->id));
                $options = array ('siteevents' => true, 'userevents' => true);
        
      11. Make sure only site event and user event is returned as, the user doesnot have permissions on other events.
      12. Make sure you get warnings for rest
      13. Enrol the user in the course and group
      14. Make call again with the same params and make sure you get 4 events (user, site, course, and group- one each)
      15. Change options to where time() represents the time the events where created and weeksecs is the number of secs in a week. Alternatively browse the database and find out the timestamp for the second course event that was created and use any timestamp greater than that.
                $options = array ('siteevents' => true, 'userevents' => true, 'timeend' => time() + 7*WEEKSECS);
        
      16. Make sure 5 events are returned (site, group, user -one each and 2 course events)
      17. This is how you can pass params in xml-rpc client
        $post = xmlrpc_encode_request($functionname, array( $events, $options));
        
      Show
      This is moderately difficult test, requires fair bit of time testing various logic flow paths. 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) User from now on refer's to an non-admin/managerial single user. 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 Make a call with params like this (Assuming your are using xmlrpc client) :- $events = array ('eventids' => array($siteevent->id), 'courseids' => array($course->id), 'groupids' => array($group->id)); $options = array ('siteevents' => true , 'userevents' => true ); Make sure only site event and user event is returned as, the user doesnot have permissions on other events. Make sure you get warnings for rest Enrol the user in the course and group Make call again with the same params and make sure you get 4 events (user, site, course, and group- one each) Change options to where time() represents the time the events where created and weeksecs is the number of secs in a week. Alternatively browse the database and find out the timestamp for the second course event that was created and use any timestamp greater than that. $options = array ('siteevents' => true , 'userevents' => true , 'timeend' => time() + 7*WEEKSECS); Make sure 5 events are returned (site, group, user -one each and 2 course events) This is how you can pass params in xml-rpc client $post = xmlrpc_encode_request($functionname, array( $events, $options));
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-37100-master
    • Rank:
      46662

      Description

      Implement core_calendar_delete_calendar_events()

        Issue Links

          Activity

          Hide
          Ankit Agarwal added a comment -

          Requesting a review.
          I have not bumped the version, since its going to create a lot of conflicts during rebasing. I will make sure the bump is added before it is integrated.
          Thanks

          Show
          Ankit Agarwal added a comment - Requesting a review. I have not bumped the version, since its going to create a lot of conflicts during rebasing. I will make sure the bump is added before it is integrated. Thanks
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Ankit.

          • I would definitively return a warning when you clean the courseid and groupid. And later if you fail to include the event, the reason is that the client needs to clean/sync the courseid/groupid. In your situation it means that their DB is out of sync. For event failing, it could be because the event doesn't exist anymore. So in this case too the clients need to sync their DB.
          • some wrong indentation, additional blank lines
          • for return values, I don't see the point to return a default value. Use VALUE_OPTIONAL, it will just no be returned.
          Show
          Jérôme Mouneyrac added a comment - Thanks Ankit. I would definitively return a warning when you clean the courseid and groupid. And later if you fail to include the event, the reason is that the client needs to clean/sync the courseid/groupid. In your situation it means that their DB is out of sync. For event failing, it could be because the event doesn't exist anymore. So in this case too the clients need to sync their DB. some wrong indentation, additional blank lines for return values, I don't see the point to return a default value. Use VALUE_OPTIONAL, it will just no be returned.
          Hide
          Ankit Agarwal added a comment - - edited

          Hi, Jerome,
          I have fixed all the issues you mentioned. Also added warnings. rebased on top of current integration to reduce conflicts. Submitting for integration.
          Thanks

          Show
          Ankit Agarwal added a comment - - edited Hi, Jerome, I have fixed all the issues you mentioned. Also added warnings. rebased on top of current integration to reduce conflicts. Submitting for integration. Thanks
          Hide
          Aparup Banerjee added a comment -

          Hi Ankit,
          nice work on this , i've only picked out one issue which could be a quick fix.

          • lib/db/services.php patch line 720: 'capabilities'=> 'moodle/calendar:manageentries', 'moodle/calendar:manageownentries', 'moodle/calendar:managegroupentries' , is that really necessary to just simply get (read) the calendar events? we might need some simpler reading ones (or not) but i do think we don't need manage capabilities to simple get events here.
          Show
          Aparup Banerjee added a comment - Hi Ankit, nice work on this , i've only picked out one issue which could be a quick fix. lib/db/services.php patch line 720: 'capabilities'=> 'moodle/calendar:manageentries', 'moodle/calendar:manageownentries', 'moodle/calendar:managegroupentries' , is that really necessary to just simply get (read) the calendar events? we might need some simpler reading ones (or not) but i do think we don't need manage capabilities to simple get events here.
          Hide
          Ankit Agarwal added a comment - - edited

          Hi Aparup,
          I had a talk with Jerome about this. And the summary was these capabilities concept is not used anywhere except, giving some msgs in administration. The basic meaning of this capability thing is "User 'may' require these capabilities".
          Reading a calendar event is bit complicated as we discussed, we cannot have a separate capability for it, as it must be synchronous with course enrolments, and various other stuff. For example it doesn’t make sense for a user not to have read calendar event capability, for user context. In any case, that is something that will require massive re factoring of calendar code and isn't in the scope of this issue.
          So it makes sense for me to keep them as it is.
          Thanks

          Show
          Ankit Agarwal added a comment - - edited Hi Aparup, I had a talk with Jerome about this. And the summary was these capabilities concept is not used anywhere except, giving some msgs in administration. The basic meaning of this capability thing is "User 'may' require these capabilities". Reading a calendar event is bit complicated as we discussed, we cannot have a separate capability for it, as it must be synchronous with course enrolments, and various other stuff. For example it doesn’t make sense for a user not to have read calendar event capability, for user context. In any case, that is something that will require massive re factoring of calendar code and isn't in the scope of this issue. So it makes sense for me to keep them as it is. Thanks
          Hide
          Damyon Wiese added a comment -

          The code only uses these capabilities to show what capabilities a user is missing - but the check for missing capabilities does not look at the context at all (so if you are assigned a role with the required capability anywhere that is enough).

          So the meaning of the capabilities in db/services.php seems to be - just list all the capabilities that get checked at any context in this function.

          Given that - almost all of the existing webservices get this wrong. IMO we shouldn't hold up this issue for this problem - but we should add a new issue to clear this up in the docs and fix the existing functions.

          Note: the documentation for this says: "Note: in services.php you'll find out that you can set a 'requiredcapabilities' settings for each web service function declaration. This is just a text help for the administrator. Moodle servers do not do any automaticcally check these capibilities. This is due to the fact that capability checks require a specific context, so you need to implement these checks in your external function"

          Show
          Damyon Wiese added a comment - The code only uses these capabilities to show what capabilities a user is missing - but the check for missing capabilities does not look at the context at all (so if you are assigned a role with the required capability anywhere that is enough). So the meaning of the capabilities in db/services.php seems to be - just list all the capabilities that get checked at any context in this function. Given that - almost all of the existing webservices get this wrong. IMO we shouldn't hold up this issue for this problem - but we should add a new issue to clear this up in the docs and fix the existing functions. Note: the documentation for this says: "Note: in services.php you'll find out that you can set a 'requiredcapabilities' settings for each web service function declaration. This is just a text help for the administrator. Moodle servers do not do any automaticcally check these capibilities. This is due to the fact that capability checks require a specific context, so you need to implement these checks in your external function"
          Hide
          Aparup Banerjee added a comment -

          ah yes! Thanks Damyon, now that you mention it, i recall web services and its general specification of capabilities involved but not necessarily used.

          i'm currently testing this locally before integrating.

          Show
          Aparup Banerjee added a comment - ah yes! Thanks Damyon, now that you mention it, i recall web services and its general specification of capabilities involved but not necessarily used. i'm currently testing this locally before integrating.
          Hide
          Aparup Banerjee added a comment -

          Thanks Ankit! this has been integrated into master now

          Show
          Aparup Banerjee added a comment - Thanks Ankit! this has been integrated into master now
          Hide
          Aparup Banerjee added a comment -

          passed! works great for me.

          Show
          Aparup Banerjee added a comment - passed! works great for me.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks!

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks! Closing as fixed, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: