Moodle
  1. Moodle
  2. MDL-36626 Meta: Ical Bug fixes and improvments
  3. MDL-36621

Display the source of event in ical imported events and implement MUC for calendar subscriptions

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.5
    • Component/s: Calendar
    • Labels:
    • Testing Instructions:
      Hide

      TEST 1

      1. login as admin
      2. Purge caches
      3. create a few calendar events
      4. goto calendar>manage subscriptions and add a subscription using url
        https://www.google.com/calendar/ical/en.australian%23holiday%40group.v.calendar.google.com/public/basic.ics
        
      5. Create another subscription from a file, if you dont have a file, just logon to moodle.org and export your calendar as file and use that file.
      6. visit the calendar and make sure you see the imported events from above feed as normal events
      7. Enable the setting calendar_showicalsource from admin settings
      8. click on any date and go to day view
      9. make sure you see ical events with source listed and linked(only for url imported events), other manually created events should appear normally
      10. Change language to rtl. make sure the source info is now right aligned.
      11. Disable the setting calendar_showicalsource from admin settings
      12. Make sure the ical source info is not displayed anymore

      TEST 2

      1. add calendar blocks on any page
      2. see that you can see the calendar event popups without anyissue for both mouseover and onfoucus
      3. beside the event name the subciprion info should not be displayed for ical events.
      4. Enable the setting calendar_showicalsource from admin settings
      5. beside the event name the subciprion info should be displayed for ical events.

      TEST 3

      1. Delete both subscriptions and make sure there is no error.

      TEST 4

      1. A regression testing session is need for the ical feature as almost every function of the ical import is effected by this change.

      TEST 5

      1. Enable performance info and all other debuging info if you dont have it enabled http://docs.moodle.org/23/en/Debugging
      2. Add a ical url subscription
      3. update the subscription and in the performance info make sure you see calendar_subscriptions in the cache section on the landing page after updating the subscription
      4. In the same section make sure you see a lot of hits and very few misses, the number of hits should be somewhere around the number of imported events.
      5. Note the number of DB reads
      6. Goto a moodle instance without this patch and add the same exact ical url subscription and update
      7. note the number of DB reads and make sure it is way more than what is noted in step 5 (around the number of imported events)
      Show
      TEST 1 login as admin Purge caches create a few calendar events goto calendar>manage subscriptions and add a subscription using url https: //www.google.com/calendar/ical/en.australian%23holiday%40group.v.calendar.google.com/ public /basic.ics Create another subscription from a file, if you dont have a file, just logon to moodle.org and export your calendar as file and use that file. visit the calendar and make sure you see the imported events from above feed as normal events Enable the setting calendar_showicalsource from admin settings click on any date and go to day view make sure you see ical events with source listed and linked(only for url imported events), other manually created events should appear normally Change language to rtl. make sure the source info is now right aligned. Disable the setting calendar_showicalsource from admin settings Make sure the ical source info is not displayed anymore TEST 2 add calendar blocks on any page see that you can see the calendar event popups without anyissue for both mouseover and onfoucus beside the event name the subciprion info should not be displayed for ical events. Enable the setting calendar_showicalsource from admin settings beside the event name the subciprion info should be displayed for ical events. TEST 3 Delete both subscriptions and make sure there is no error. TEST 4 A regression testing session is need for the ical feature as almost every function of the ical import is effected by this change. TEST 5 Enable performance info and all other debuging info if you dont have it enabled http://docs.moodle.org/23/en/Debugging Add a ical url subscription update the subscription and in the performance info make sure you see calendar_subscriptions in the cache section on the landing page after updating the subscription In the same section make sure you see a lot of hits and very few misses, the number of hits should be somewhere around the number of imported events. Note the number of DB reads Goto a moodle instance without this patch and add the same exact ical url subscription and update note the number of DB reads and make sure it is way more than what is noted in step 5 (around the number of imported events)
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-36621-master
    • Rank:
      46113

      Description

      A simple text displaying the name of subscription source should be present in the event description.

      I am marking this as mdlqa, it will be nice to get this sorted before 2.4 release, but its optional.

        Issue Links

          Activity

          Hide
          Ankit Agarwal added a comment -

          This cant be properly tested and fixed untill the linked issue is resolved.

          Thanks

          Show
          Ankit Agarwal added a comment - This cant be properly tested and fixed untill the linked issue is resolved. Thanks
          Hide
          Ankit Agarwal added a comment -

          Requesting a review.
          thanks

          Show
          Ankit Agarwal added a comment - Requesting a review. thanks
          Hide
          Damyon Wiese added a comment -

          Hi Ankit,

          This patch looks good - except on the single day page, even though the language string has "{$a->name}({$a->source})" - the brackets get removed when this is displayed and the name and source have no spacing between them. Can you add a space?

          Everything else is great.

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

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - Hi Ankit, This patch looks good - except on the single day page, even though the language string has "{$a->name}({$a->source})" - the brackets get removed when this is displayed and the name and source have no spacing between them. Can you add a space? Everything else is great. [Y] Syntax [Y] Output [Y] Whitespace [Y] Language [-] Databases [Y] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check Thanks, Damyon
          Hide
          Ankit Agarwal added a comment -

          Hi Damyon,
          I think your css is cached, it should show in a new line, can you try purging caches and see if that helps?
          I will update testing instruction to mention purging the cache
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Damyon, I think your css is cached, it should show in a new line, can you try purging caches and see if that helps? I will update testing instruction to mention purging the cache Thanks
          Hide
          Damyon Wiese added a comment -

          Right - yes I cleared the cache and it shows on a new line now.

          All good then - Thanks

          Show
          Damyon Wiese added a comment - Right - yes I cleared the cache and it shows on a new line now. All good then - Thanks
          Hide
          Damyon Wiese added a comment -

          Sending for integration.

          Show
          Damyon Wiese added a comment - Sending for integration.
          Hide
          Ankit Agarwal added a comment -

          @integrator
          Although this is a new feature, but it might be nice to have in 24 as well. It should be a clean cherry-pick incase you decide to backport it.
          Thanks

          Show
          Ankit Agarwal added a comment - @integrator Although this is a new feature, but it might be nice to have in 24 as well. It should be a clean cherry-pick incase you decide to backport it. Thanks
          Hide
          Sam Hemelryk added a comment -

          Hi Ankit,

          I'm sending this back sorry on two points:

          1. This change is epically bad for performance because of the DB interaction with the event constructor.
          This is going to lead to an additional query for every calendar subscription event being shown to the user.
          If a subscription exists for a feed that publishes several events in a month this will add a lot of database interaction to any page displaying a calendar.

          In thinking about what could be done here I think there are a couple of options.
          a) We retrieve any subscription information when we retrieve calendar information so that we have it available already without introducing any more database interaction.
          b) We implement API for calendar subscription to fetch subscriptions by id and then use MUC to cache.

          Personally I like b more, its a little bit more work but it follows onto the next thing I think about this current fix..

          2. I think that this should be an option of the calendar subscription.
          I can imagine that not every site is going to want to show this information as it can be seen to add unnecessary information to the page (the event is valuable, the subscription is not).

          Have a think about those and come back to me if you have any questions or want to discuss it.
          Regarding backporting we (the integrators) discussed the backporting of issues like this the other week. We decided that unless there was a very good reason to backport improvements/new features then we wouldn't. Persuasive arguments would be that it fixes a/several bugs along the way, or that the without the change existing functionality is useless etc.
          As such this issue won't be backported.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Ankit, I'm sending this back sorry on two points: 1. This change is epically bad for performance because of the DB interaction with the event constructor. This is going to lead to an additional query for every calendar subscription event being shown to the user. If a subscription exists for a feed that publishes several events in a month this will add a lot of database interaction to any page displaying a calendar. In thinking about what could be done here I think there are a couple of options. a) We retrieve any subscription information when we retrieve calendar information so that we have it available already without introducing any more database interaction. b) We implement API for calendar subscription to fetch subscriptions by id and then use MUC to cache. Personally I like b more, its a little bit more work but it follows onto the next thing I think about this current fix.. 2. I think that this should be an option of the calendar subscription. I can imagine that not every site is going to want to show this information as it can be seen to add unnecessary information to the page (the event is valuable, the subscription is not). Have a think about those and come back to me if you have any questions or want to discuss it. Regarding backporting we (the integrators) discussed the backporting of issues like this the other week. We decided that unless there was a very good reason to backport improvements/new features then we wouldn't. Persuasive arguments would be that it fixes a/several bugs along the way, or that the without the change existing functionality is useless etc. As such this issue won't be backported. Many thanks Sam
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Ankit Agarwal added a comment -

          Hi Sam,
          I agree with you , it makes sense to cache it and also to create a global config. Since I couldn't find many examples in core code about the caching, I have created a class based on muc docs, can you have a quick look and let me know if am going in the right direction?
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Sam, I agree with you , it makes sense to cache it and also to create a global config. Since I couldn't find many examples in core code about the caching, I have created a class based on muc docs, can you have a quick look and let me know if am going in the right direction? Thanks
          Hide
          Sam Hemelryk added a comment -

          Just noting I had a discussion with Ankit about the cache direction today.

          Show
          Sam Hemelryk added a comment - Just noting I had a discussion with Ankit about the cache direction today.
          Hide
          Ankit Agarwal added a comment -

          I have introduced MUC now. Also introduced a new setting to control display of ical source. Requesting another peer-review.
          Thanks

          Show
          Ankit Agarwal added a comment - I have introduced MUC now. Also introduced a new setting to control display of ical source. Requesting another peer-review. Thanks
          Hide
          Damyon Wiese added a comment -

          Thanks Ankit - I'll relook at this tomorrow.

          Cheers - Damyon

          Show
          Damyon Wiese added a comment - Thanks Ankit - I'll relook at this tomorrow. Cheers - Damyon
          Hide
          Damyon Wiese added a comment -

          Hi Ankit,

          I have a few suggestions on the way the cache is implemented.

          You are caching event_subscriptions records, this means you should invalidate the cache data whenever one of these records is updated or deleted.

          The only place you are invalidating the cache data is in calendar_process_subscription_row (and only on remove).

          Additional functions that need invalidation:
          calendar_update_subscription_events
          calendar_add_subscription
          calendar_process_subscription_row (for update)

          And you have added a function for calendar_get_subscription which makes use of the cache - but you are not using it much. You should replace all calls outside of your new function to

          $DB->get_record('event_subscriptions', array('id'=>$id), '*', MUST_EXIST);
          

          With your new function (it may help to return false from your function instead of throwing a moodle_exception). I count 4 places it could be used in lib/calendar.php

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - Hi Ankit, I have a few suggestions on the way the cache is implemented. You are caching event_subscriptions records, this means you should invalidate the cache data whenever one of these records is updated or deleted. The only place you are invalidating the cache data is in calendar_process_subscription_row (and only on remove). Additional functions that need invalidation: calendar_update_subscription_events calendar_add_subscription calendar_process_subscription_row (for update) And you have added a function for calendar_get_subscription which makes use of the cache - but you are not using it much. You should replace all calls outside of your new function to $DB->get_record('event_subscriptions', array('id'=>$id), '*', MUST_EXIST); With your new function (it may help to return false from your function instead of throwing a moodle_exception). I count 4 places it could be used in lib/calendar.php Thanks, Damyon
          Hide
          Ankit Agarwal added a comment - - edited

          Thanks Damyon,
          For the detailed review.
          I initially thought some of those changes were not within the scope of this issue. But as we discussed now I have introduced caching for subscriptions through out the calendar. This should be a major performance imporvment specially during adding new subscriptions, where it used to do 1 Query per event to fetch subscription to 1 query now in total. So if a subscription has say 200 events, instead of making 200 call to get the same subscription in calendar_add_icalendar_event() it would get it from catch.

          Although some minor api changes were being called for to have caching used properly in most places, so I have done that.
          Also created and linked a issue to have a new api for updating subscriptions.

          Thanks

          Show
          Ankit Agarwal added a comment - - edited Thanks Damyon, For the detailed review. I initially thought some of those changes were not within the scope of this issue. But as we discussed now I have introduced caching for subscriptions through out the calendar. This should be a major performance imporvment specially during adding new subscriptions, where it used to do 1 Query per event to fetch subscription to 1 query now in total. So if a subscription has say 200 events, instead of making 200 call to get the same subscription in calendar_add_icalendar_event() it would get it from catch. Although some minor api changes were being called for to have caching used properly in most places, so I have done that. Also created and linked a issue to have a new api for updating subscriptions. Thanks
          Hide
          Damyon Wiese added a comment -

          Thumbs up from me - thanks Ankit.

          Show
          Damyon Wiese added a comment - Thumbs up from me - thanks Ankit.
          Hide
          Ankit Agarwal added a comment -

          Thanks Damyon,
          Submitting for integration.

          Show
          Ankit Agarwal added a comment - Thanks Damyon, Submitting for integration.
          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
          Sam Hemelryk added a comment -

          Thanks guys, this has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks guys, this has been integrated now.
          Hide
          Rajesh Taneja added a comment -

          Thanks Ankit,

          Works as described.
          DB reads/writes are less with patch.

          Show
          Rajesh Taneja added a comment - Thanks Ankit, Works as described. DB reads/writes are less with patch.
          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:
              1 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: