Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4
    • Component/s: Calendar
    • Labels:
    • Testing Instructions:
      Hide
      1. Goto a course with atleast two gorups
      2. Goto a course calendar ( add upcoming event block in the course)
      3. Click on manage subscription
      4. remove all subscriptions if you have any and add the following http://www.mozilla.org/projects/calendar/caldata/AustraliaHolidays.ics as "site/global event"
      5. Visit the calendar and verify all events are imported upder proper eventtype and course etc
      6. Verify in the database "mdl_event" all event are imported with proper "userid, courseid, gorupid, eventtype"
      7. If you dont remove before adding new subscription old events are over written. So please remove old subscriptions before adding new ones.
      8. Repeat step 4-6 with all event types i.e site, course, user, group
      Show
      Goto a course with atleast two gorups Goto a course calendar ( add upcoming event block in the course) Click on manage subscription remove all subscriptions if you have any and add the following http://www.mozilla.org/projects/calendar/caldata/AustraliaHolidays.ics as "site/global event" Visit the calendar and verify all events are imported upder proper eventtype and course etc Verify in the database "mdl_event" all event are imported with proper "userid, courseid, gorupid, eventtype" If you dont remove before adding new subscription old events are over written. So please remove old subscriptions before adding new ones. Repeat step 4-6 with all event types i.e site, course, user, group
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-36614-master
    • Rank:
      46106

      Description

      Ical imports doesnt populate "eventtype" field in the database. From what I remember this used to happen in 1.9 and had created a completed mess during backup and restore, earlier.
      This should be fixed before we start extensively using this import.

        Issue Links

          Activity

          Hide
          Ankit Agarwal added a comment -

          I discussed this with Sam and we decided to add a column eventtype to the subscriptions and use that to populate the eventtype in the events table when we import events from the subscriptions.

          Show
          Ankit Agarwal added a comment - I discussed this with Sam and we decided to add a column eventtype to the subscriptions and use that to populate the eventtype in the events table when we import events from the subscriptions.
          Hide
          Ankit Agarwal added a comment -

          I am not sure it is a good idea to process events if they are not attached to a subscription id. I cannot think of any situation that this might be happening, also this will mean we wont know the eventtype of the event if it is not attached to a subscription.
          Hence I have forced the existence of reference along with this patch. Please let me know if there is a valid usecase when this might be wrong.
          Requesting peer review.
          Thanks

          Show
          Ankit Agarwal added a comment - I am not sure it is a good idea to process events if they are not attached to a subscription id. I cannot think of any situation that this might be happening, also this will mean we wont know the eventtype of the event if it is not attached to a subscription. Hence I have forced the existence of reference along with this patch. Please let me know if there is a valid usecase when this might be wrong. Requesting peer review. Thanks
          Hide
          Sam Hemelryk added a comment -

          Marking this finished.
          I talked to Ankit about this, just one point to action:
          Rather than introduce a function to get the event type we'll modify the calendar_get_eventtype_choices function to use the values we want.
          That new function was introduced for and only used by the subscriptions so should be no issue.

          We discussed the use of not null during upgrade as well and decided it'd be safe to ignore it given it will only continue to affect developers (and we all hate developers).

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Marking this finished. I talked to Ankit about this, just one point to action: Rather than introduce a function to get the event type we'll modify the calendar_get_eventtype_choices function to use the values we want. That new function was introduced for and only used by the subscriptions so should be no issue. We discussed the use of not null during upgrade as well and decided it'd be safe to ignore it given it will only continue to affect developers (and we all hate developers). Many thanks Sam
          Hide
          Ankit Agarwal added a comment -

          Hi sam,
          Thanks for the feed back.
          I have updated the patch based on your feedback.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi sam, Thanks for the feed back. I have updated the patch based on your feedback. Thanks
          Hide
          Sam Hemelryk added a comment -

          Looks spot on Ankit, only thing I noted was that you left the comment referring to the old new function https://github.com/ankitagarwal/moodle/compare/MDL-36614-master#L0R2685.

          All good to go to integration when you are ready.

          Show
          Sam Hemelryk added a comment - Looks spot on Ankit, only thing I noted was that you left the comment referring to the old new function https://github.com/ankitagarwal/moodle/compare/MDL-36614-master#L0R2685 . All good to go to integration when you are ready.
          Hide
          Ankit Agarwal added a comment -

          Thanks sam for the feedback.
          updated the patch.
          Submitting for integration

          Show
          Ankit Agarwal added a comment - Thanks sam for the feedback. updated the patch. Submitting for integration
          Hide
          Aparup Banerjee added a comment -

          just after a quick look, getting conflict @ calendar_get_eventtype_choices()
          <<<<<<< HEAD
          $choices[SITEID] = get_string('siteevents', 'calendar');
          =======
          $choices['site'] = get_string('globalevents', 'calendar');

          Show
          Aparup Banerjee added a comment - just after a quick look, getting conflict @ calendar_get_eventtype_choices() <<<<<<< HEAD $choices [SITEID] = get_string('siteevents', 'calendar'); ======= $choices ['site'] = get_string('globalevents', 'calendar');
          Hide
          Ankit Agarwal added a comment -

          Hi Aparup,
          conflict is because of MDL-36615 which is in integration but not in stable. Please suggest how you want me to proceed..
          should I cherry pick that commit to this branch and rebase on top of that?

          Thanks

          Show
          Ankit Agarwal added a comment - Hi Aparup, conflict is because of MDL-36615 which is in integration but not in stable. Please suggest how you want me to proceed.. should I cherry pick that commit to this branch and rebase on top of that? Thanks
          Hide
          Aparup Banerjee added a comment -

          Hi Ankit,
          i've resolved these below in integration. We should be all good but please check.
          Conflicts:
          calendar/lib.php (MDL-36615)
          calendar/managesubscriptions_form.php (MDL-36352)

          i think rebasing against integration during beta & mdlqa phase is the best thing to do to minimise on conflicts as well as weeding out more bugs that might not have been tested from your changes and the latests code. We cannot develop against stable for mdlqa issues prior to release, only against integration.git.

          Show
          Aparup Banerjee added a comment - Hi Ankit, i've resolved these below in integration. We should be all good but please check. Conflicts: calendar/lib.php ( MDL-36615 ) calendar/managesubscriptions_form.php ( MDL-36352 ) i think rebasing against integration during beta & mdlqa phase is the best thing to do to minimise on conflicts as well as weeding out more bugs that might not have been tested from your changes and the latests code. We cannot develop against stable for mdlqa issues prior to release, only against integration.git.
          Hide
          Ankit Agarwal added a comment -

          Cool,
          Thanks Aparup

          Show
          Ankit Agarwal added a comment - Cool, Thanks Aparup
          Hide
          Aparup Banerjee added a comment -

          this works for me.

          • noted that adding same events for a group2 will overwrite these existing events for a group1. Discussed with Ankit and this is expected by ical design although i've no idea why this is so. Perhaps the original g1 calendar should be removed? i'm not sure.
          • also noting that from course calendar, adding a sitewide event is a little confusing as there is nothing to say that it is saved and neither is it represented (understandably) in the course calendar management view.

          i'm not sure if theres any MDL for these but i think it would help to display the Type of Event in a column in the manage subscriptions table.

          anyway passing this now.

          Show
          Aparup Banerjee added a comment - this works for me. noted that adding same events for a group2 will overwrite these existing events for a group1. Discussed with Ankit and this is expected by ical design although i've no idea why this is so. Perhaps the original g1 calendar should be removed? i'm not sure. also noting that from course calendar, adding a sitewide event is a little confusing as there is nothing to say that it is saved and neither is it represented (understandably) in the course calendar management view. i'm not sure if theres any MDL for these but i think it would help to display the Type of Event in a column in the manage subscriptions table. anyway passing this now.
          Hide
          Ankit Agarwal added a comment -

          Hi Aparup,
          About the second point , I am not sure, I properly understand what you are referring to. This is the list of all known issues with ical MDL-36626

          Please feel free to create sub-tasks to that as you see necessary.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Aparup, About the second point , I am not sure, I properly understand what you are referring to. This is the list of all known issues with ical MDL-36626 Please feel free to create sub-tasks to that as you see necessary. Thanks
          Hide
          Aparup Banerjee added a comment -

          created MDL-36738

          Show
          Aparup Banerjee added a comment - created MDL-36738
          Hide
          Dan Poltawski added a comment -

          Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week!

          ciao
          Dan

          Show
          Dan Poltawski added a comment - Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week! ciao Dan

            People

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

              Dates

              • Created:
                Updated:
                Resolved: