Moodle

Ical import should consider all line endings while importing

Details

  • Type: Sub-task Sub-task
  • Status: Closed 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 sitepages>calander>manage subscription
    2. Remove all subscriptions you have there
    3. add this http://ical.mac.com/ical/US32Holidays.ics as subscription
    4. Make sure all events are created properly
    5. Remove the subscription
    6. Make sure all events corresponding to the url are deleted.
    7. repeat step 3 -6 for the two attached file.
    Show
    Goto sitepages>calander>manage subscription Remove all subscriptions you have there add this http://ical.mac.com/ical/US32Holidays.ics as subscription Make sure all events are created properly Remove the subscription Make sure all events corresponding to the url are deleted. repeat step 3 -6 for the two attached file.
  • Affected Branches:
    MOODLE_24_STABLE
  • Fixed Branches:
    MOODLE_24_STABLE
  • Pull from Repository:
  • Pull Master Branch:
    MDL-36592-master
  • Pull Master Diff URL:

Description

copying my comment from linked issue:-
While working on this I noticed some files dont work, while others did with the correct format. after spending quite some time debugging the problem I found was the "newline" character used in those files i.e "\n" instead of "\r\n". Which was breaking everything as $ical::unserialize couldnt find the event details as it does an explode on "\r\n" only. I will create a blocker for that .
try using this file http://ical.mac.com/ical/US32Holidays.ics
Thanks

  1. File
    AustraliaHolidays.ics
    14/Nov/12 3:47 PM
    5 kB
    Michael de Raadt
  2. File
    US32Holidays.ics
    14/Nov/12 3:47 PM
    11 kB
    Michael de Raadt

Issue Links

blocks

Functional Test - A test of a single Moodle feature MDLQA-5207 CLONE - A user can import a calendar via URL

  • Passed - The test passed
discovered while testing

New Feature - A new Moodle feature which has yet to be developed. MDL-16660 Calendar: IMPORT or Subscribe to iCal feed into Moodle calendar

  • Blocker - Blocks development and/or testing, prevents Moodle from running (applicable to bugs only)
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.
will help resolve

Sub-task - Issues are sometimes broken into multiple sub-tasks. MDL-36525 Calendar import should follow redirects

  • Blocker - Blocks development and/or testing, prevents Moodle from running (applicable to bugs only)
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Rossiani Wijaya added a comment -

Hi Ankit,

The patch looks great. I also tested on 2.4.

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

Show
Rossiani Wijaya added a comment - Hi Ankit, The patch looks great. I also tested on 2.4. [y] Syntax [y] Output [y] Whitespace [-] Language [-] Databases [y] Testing [-] Security [-] Documentation [y] Git [y] Sanity check
Hide
Permalink
Ankit Agarwal added a comment -

Thanks Rosie for the review.
Sending for integration.
Thanks

Show
Ankit Agarwal added a comment - Thanks Rosie for the review. Sending for integration. Thanks
Hide
Permalink
Sam Hemelryk added a comment -

Hi Ankit,

The bennu lib is an external library, we need to avoid changes to it, or at least minimise changes to ensure should (however unlikely) an updated version get released we can upgrade to it with minimal conflicts.

1/ The white space fixes need to be reverted sorry.
2/ Lets consider the approach taken of modifying the bennu lib.
One of two things needs to happen, I don't mind which way we go, I just thought I'd point out the options available.
a/ If we decide to take the current approach of modifying the bennu lib we need to ensure we note any changes we make in its moodle readme file so that we can reapply them if we upgrade.
Keeping them simple is also a goal so perhaps fixing the line endings as one operation and then splitting in another operation would be favourable as we would have a line that we would need to copy when upgrading bennu rather than a line we would need to modify.
b/ Alternatively we can correct any line ending issues before we call the unserialise function.
This approach would see us fix the line endings before we call serialise. While not as convenient this does mean we don't need to modify the bennu lib and is perhaps more in line with how bennu has been written.

Worth noting is that both the iCal RFC used when this library was written (2445) and the updated iCal RFC (5545) state line endings should in fact be CRLF. What its doing at the moment is correct according to the spec, and if you validate that feed you'll get warnings.

Many thanks
Sam

Show
Sam Hemelryk added a comment - Hi Ankit, The bennu lib is an external library, we need to avoid changes to it, or at least minimise changes to ensure should (however unlikely) an updated version get released we can upgrade to it with minimal conflicts. 1/ The white space fixes need to be reverted sorry. 2/ Lets consider the approach taken of modifying the bennu lib. One of two things needs to happen, I don't mind which way we go, I just thought I'd point out the options available. a/ If we decide to take the current approach of modifying the bennu lib we need to ensure we note any changes we make in its moodle readme file so that we can reapply them if we upgrade. Keeping them simple is also a goal so perhaps fixing the line endings as one operation and then splitting in another operation would be favourable as we would have a line that we would need to copy when upgrading bennu rather than a line we would need to modify. b/ Alternatively we can correct any line ending issues before we call the unserialise function. This approach would see us fix the line endings before we call serialise. While not as convenient this does mean we don't need to modify the bennu lib and is perhaps more in line with how bennu has been written. Worth noting is that both the iCal RFC used when this library was written (2445) and the updated iCal RFC (5545) state line endings should in fact be CRLF. What its doing at the moment is correct according to the spec, and if you validate that feed you'll get warnings. Many thanks Sam
Hide
Permalink
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
Permalink
Ankit Agarwal added a comment - - edited

Hi Sam,

  1. I have removed the code style improvements (my editor was adamant about it )
  2. As we discussed this, this cant be done with explode so I have used the preg_split as before. and updated the readme.
  3. Just for note this is not an upstream issue as the standard is "\r\n" (http://www.ietf.org/rfc/rfc2445.txt) but many ical generators donot follow that, so we cannot be that strict.
    Pushing back for integration
    Thanks
Show
Ankit Agarwal added a comment - - edited Hi Sam, I have removed the code style improvements (my editor was adamant about it ) As we discussed this, this cant be done with explode so I have used the preg_split as before. and updated the readme. Just for note this is not an upstream issue as the standard is "\r\n" ( http://www.ietf.org/rfc/rfc2445.txt ) but many ical generators donot follow that, so we cannot be that strict. Pushing back for integration Thanks
Hide
Permalink
Sam Hemelryk added a comment -

Hi Ankit,

This has been integrated now.

Just to clarify for anyone else we discussed this modification of Bennu and while we acknowledge the fix is against the nature of Bennu and would not necessarily be accepted upstream it works and "unblocks" the QA issue.

Cheers
Sam

Show
Sam Hemelryk added a comment - Hi Ankit, This has been integrated now. Just to clarify for anyone else we discussed this modification of Bennu and while we acknowledge the fix is against the nature of Bennu and would not necessarily be accepted upstream it works and "unblocks" the QA issue. Cheers Sam
Hide
Permalink
Sam Hemelryk added a comment -

Testing will be performed as part of the QA testing on the linked issue.

Show
Sam Hemelryk added a comment - Testing will be performed as part of the QA testing on the linked issue.
Hide
Permalink
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

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: