Details

    • Type: Sub-task
    • Status: Closed
    • Priority: 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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            ankit_frenz 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_frenz 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_frenz 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_frenz 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
            samhemelryk 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
            samhemelryk 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_frenz Ankit Agarwal added a comment -

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

            Show
            ankit_frenz Ankit Agarwal added a comment - Hi sam, Thanks for the feed back. I have updated the patch based on your feedback. Thanks
            Hide
            samhemelryk 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
            samhemelryk 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_frenz Ankit Agarwal added a comment -

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

            Show
            ankit_frenz Ankit Agarwal added a comment - Thanks sam for the feedback. updated the patch. Submitting for integration
            Hide
            nebgor 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
            nebgor 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_frenz 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_frenz 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
            nebgor 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
            nebgor 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_frenz Ankit Agarwal added a comment -

            Cool,
            Thanks Aparup

            Show
            ankit_frenz Ankit Agarwal added a comment - Cool, Thanks Aparup
            Hide
            nebgor 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
            nebgor 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_frenz 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_frenz 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
            nebgor Aparup Banerjee added a comment -

            created MDL-36738

            Show
            nebgor Aparup Banerjee added a comment - created MDL-36738
            Hide
            poltawski 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
            poltawski 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:
                  Fix Release Date:
                  3/Dec/12