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
    • Rank:
      47214

      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.

        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: