Moodle
  1. Moodle
  2. MDL-37562

Api for updating calendar subscriptions

    Details

    • Testing Instructions:
      Hide
      1. run phpunit --verbose calendar/tests/calendarical_tests.php and make sure it passes
      2. A Regression testing with calendar subscriptions is required i.e (add, edit, update, delete etc)
        Test
      3. Goto site>calendar>manage subscription
      4. Add an ical subscription using url
        code}
        https://www.google.com/calendar/ical/en.australian%23holiday%40group.v.calendar.google.com/public/basic.ics

         

      5. Not the details in database (event_subscription).
      6. Goto the same page again and from the top list of subscription, update the time interval for the one that you just added
      7. Check in database pollinterval is updated accordingly
      Show
      run phpunit --verbose calendar/tests/calendarical_tests.php and make sure it passes A Regression testing with calendar subscriptions is required i.e (add, edit, update, delete etc) Test Goto site>calendar>manage subscription Add an ical subscription using url code} https://www.google.com/calendar/ical/en.australian%23holiday%40group.v.calendar.google.com/public/basic.ics   Not the details in database (event_subscription). Goto the same page again and from the top list of subscription, update the time interval for the one that you just added Check in database pollinterval is updated accordingly
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-37562-master

      Description

      while working on MDL-36621, I noticed we need an api to update ical subscriptions instead of doing update_record all over the place. Also would be easier to invalidate cache with an api.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Sam Hemelryk added a comment -

            Hi Ankit,

            Thanks for the report.
            I've changed this to an improvement issue as it sounds like what you are talking about is introducing two new things:

            1. An API.
            2. Caching

            Both very much worth doing, but both things that likely can't be backported.

            Many thanks
            Sam

            Show
            Sam Hemelryk added a comment - Hi Ankit, Thanks for the report. I've changed this to an improvement issue as it sounds like what you are talking about is introducing two new things: An API. Caching Both very much worth doing, but both things that likely can't be backported. Many thanks Sam
            Hide
            Rossiani Wijaya added a comment -

            Hi Ankit,

            Some feedback for the patch.

            calendar/lib.php
             
            line 2804: remove $cache declaration
            line 3063-3068: use || (or logic) since it is using the same string otherwise make the string different.
            line 3070: capitalize for 'Update'
            

            The rest of the patch looks great.

            [y] Syntax
            [-] Output
            [y] Whitespace
            [-] Language
            [-] Databases
            [-] Testing
            [-] Security
            [-] Documentation
            [y] Git
            [y] Sanity check

            Show
            Rossiani Wijaya added a comment - Hi Ankit, Some feedback for the patch. calendar/lib.php   line 2804: remove $cache declaration line 3063-3068: use || (or logic) since it is using the same string otherwise make the string different. line 3070: capitalize for 'Update' The rest of the patch looks great. [y] Syntax [-] Output [y] Whitespace [-] Language [-] Databases [-] Testing [-] Security [-] Documentation [y] Git [y] Sanity check
            Hide
            Ankit Agarwal added a comment -

            Thanks Rosie for the review,
            Patch has been updated.
            Submitting for integration.
            Thanks

            Show
            Ankit Agarwal added a comment - Thanks Rosie for the review, Patch has been updated. Submitting for integration. Thanks
            Hide
            Dan Poltawski added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            Damyon Wiese added a comment -

            Hi Ankit

            This change looks good - can you just add a line to calendar/upgrade.txt describing the new function?

            Thanks!

            Show
            Damyon Wiese added a comment - Hi Ankit This change looks good - can you just add a line to calendar/upgrade.txt describing the new function? Thanks!
            Hide
            Ankit Agarwal added a comment -

            Thanks Damyon,
            Changes made.

            Show
            Ankit Agarwal added a comment - Thanks Damyon, Changes made.
            Hide
            Damyon Wiese added a comment -

            Thanks Ankit - One more thing I spotted is that the test file is incorrectly named - it should be "calendarical_test.php" or this won't be run by a ./vendor/bin/phpunit with no arguments.

            The other file "externallib_tests.php" is wrong too (I'll add an issue about that and fix the check on the ci server to detect these).

            Show
            Damyon Wiese added a comment - Thanks Ankit - One more thing I spotted is that the test file is incorrectly named - it should be "calendarical_test.php" or this won't be run by a ./vendor/bin/phpunit with no arguments. The other file "externallib_tests.php" is wrong too (I'll add an issue about that and fix the check on the ci server to detect these).
            Hide
            Ankit Agarwal added a comment -

            Updated.
            Thanks

            Show
            Ankit Agarwal added a comment - Updated. Thanks
            Hide
            Damyon Wiese added a comment -

            Thanks Ankit,

            This has been integrated to master only.

            Show
            Damyon Wiese added a comment - Thanks Ankit, This has been integrated to master only.
            Hide
            Andrew Davis added a comment -

            Works as described. Passing.

            Show
            Andrew Davis added a comment - Works as described. Passing.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Your awesome contributions are now part of Moodle, your fav LMS out there.

            Closing this as fixed.

            Many thanks for all the hard work, ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Your awesome contributions are now part of Moodle, your fav LMS out there. Closing this as fixed. Many thanks for all the hard work, ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: