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

      Description

      Implement core_calendar_delete_calendar_events()

        Gliffy Diagrams

          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: