Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #355 url escaping #356

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

NicoHood
Copy link
Contributor

@NicoHood NicoHood commented Jun 15, 2022

@niccokunzmann

See #355

Note: I have not tested this inside the library itself, only via monkey patch. But this should result in the same code. Maybe someone can write a test for that?

@mister-roboto
Copy link

@NicoHood thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

@NicoHood
Copy link
Contributor Author

@jenkins-plone-org please run jobs

@niccokunzmann
Copy link
Member

@NicoHood, we are in the process of setting up maintenance for this repository again, see #360. It will take some time but I hope, we will get further towards integrating your changes. Thanks a lot for the effort and patience already!
If you like to run the latest tests for this PR, you can rebase/merge master.

@niccokunzmann
Copy link
Member

niccokunzmann commented Aug 21, 2022

@NicoHood, I am starting to get ready for this one - I am a maintainer here now just because I wanted to help you a bit but it is nice to work on this one here, too! So, the tests are running for the branches and if you were to rebase/merge, you can see that.

Also, I am hesitant to merge what is not tests as it may break in the future. There is something that we can add as tests - you have a lot of data - would you like to add that?

@NicoHood
Copy link
Contributor Author

I just generated a quick test ics file

BEGIN:VCALENDAR
VERSION:2.0
PRODID:-//ical.marudot.com//iCal Event Maker
CALSCALE:GREGORIAN
BEGIN:VTIMEZONE
TZID:Europe/Berlin
LAST-MODIFIED:20201011T015911Z
TZURL:http://tzurl.org/zoneinfo-outlook/Europe/Berlin
X-LIC-LOCATION:Europe/Berlin
BEGIN:DAYLIGHT
TZNAME:CEST
TZOFFSETFROM:+0100
TZOFFSETTO:+0200
DTSTART:19700329T020000
RRULE:FREQ=YEARLY;BYMONTH=3;BYDAY=-1SU
END:DAYLIGHT
BEGIN:STANDARD
TZNAME:CET
TZOFFSETFROM:+0200
TZOFFSETTO:+0100
DTSTART:19701025T030000
RRULE:FREQ=YEARLY;BYMONTH=10;BYDAY=-1SU
END:STANDARD
END:VTIMEZONE
BEGIN:VEVENT
DTSTAMP:20220822T164528Z
UID:[email protected]
DTSTART;TZID=Europe/Berlin:20220822T120000
DTEND;TZID=Europe/Berlin:20220822T120000
SUMMARY:test
DESCRIPTION:https://www.facebook.com/events/756119502186737/?acontext=%7B%22source%22%3A5%2C%22action_history%22%3A[%7B%22surface%22%3A%22page%22%2C%22mechanism%22%3A%22main_list%22%2C%22extra_data%22%3A%22%5C%22[]%5C%22%22%7D]%2C%22has_source%22%3Atrue%7D
END:VEVENT
END:VCALENDAR

I guess you just need to validate if the description is parsed as:

https://www.facebook.com/events/756119502186737/?acontext=%7B%22source%22%3A5%2C%22action_history%22%3A[%7B%22surface%22%3A%22page%22%2C%22mechanism%22%3A%22main_list%22%2C%22extra_data%22%3A%22%5C%22[]%5C%22%22%7D]%2C%22has_source%22%3Atrue%7D

I hope this helps. I am sorry that I aint got time to write tests myself. Thanks a lot for taking over that important part :)

niccokunzmann added a commit to niccokunzmann/icalendar that referenced this pull request Aug 23, 2022
@niccokunzmann
Copy link
Member

@NicoHood Created NicoHood#1 to your PR to add a test

@NicoHood
Copy link
Contributor Author

I rebased my branch and cherry-picked your test commit. Is it fine now?

@niccokunzmann
Copy link
Member

The tests are not running, so I will not merge it for now.

@jacadzaca jacadzaca linked an issue Sep 5, 2022 that may be closed by this pull request
niccokunzmann added a commit that referenced this pull request Oct 3, 2022
@niccokunzmann
Copy link
Member

I guess you just need to validate if the description is parsed as: [...] - @NicoHood (comment)

I created PR #426 and the tests are running - I would like to have tests failing so we can make sure that code changes are needed - do you have more examples of failed unescaping? The one in the comment is not failing.

@NicoHood
Copy link
Contributor Author

NicoHood commented Oct 3, 2022

It is not failing? That sounds weird.

@NicoHood
Copy link
Contributor Author

NicoHood commented Oct 3, 2022

So I dont know what your tests are doing, but I created my own "test":

test.ical

BEGIN:VEVENT
DTSTAMP:20220822T164528Z
UID:[email protected]
DTSTART;TZID=Europe/Berlin:20220822T120000
DTEND;TZID=Europe/Berlin:20220822T120000
SUMMARY:test
DESCRIPTION:https://www.facebook.com/events/756119502186737/?acontext=%7B%22source%22%3A5%2C%22action_history%22%3A[%7B%22surface%22%3A%22page%22%2C%22mechanism%22%3A%22main_list%22%2C%22extra_data%22%3A%22%5C%22[]%5C%22%22%7D]%2C%22has_source%22%3Atrue%7D
END:VEVENT

icaltest.py:

#!/usr/bin/env python3

import icalendar


with open('test.ical') as f:
    ical_text = f.read()
    print(ical_text)

    print('---')
    ical = icalendar.Calendar.from_ical(ical_text)
    print(ical['DESCRIPTION'])

    expected_result = 'https://www.facebook.com/events/756119502186737/?acontext=%7B%22source%22%3A5%2C%22action_history%22%3A[%7B%22surface%22%3A%22page%22%2C%22mechanism%22%3A%22main_list%22%2C%22extra_data%22%3A%22%5C%22[]%5C%22%22%7D]%2C%22has_source%22%3Atrue%7D'
    print(ical['DESCRIPTION'] == expected_result)

Output

$ ./icaltest.py 
BEGIN:VEVENT
DTSTAMP:20220822T164528Z
UID:[email protected]
DTSTART;TZID=Europe/Berlin:20220822T120000
DTEND;TZID=Europe/Berlin:20220822T120000
SUMMARY:test
DESCRIPTION:https://www.facebook.com/events/756119502186737/?acontext=%7B%22source%22%3A5%2C%22action_history%22%3A[%7B%22surface%22%3A%22page%22%2C%22mechanism%22%3A%22main_list%22%2C%22extra_data%22%3A%22%5C%22[]%5C%22%22%7D]%2C%22has_source%22%3Atrue%7D
END:VEVENT
---
https://www.facebook.com/events/756119502186737/?acontext=%7B%22source%22:5,%22action_history%22:[%7B%22surface%22:%22page%22,%22mechanism%22:%22main_list%22,%22extra_data%22:%22\%22[]\%22%22%7D],%22has_source%22:true%7D
False

I am using python-icalendar 4.1.0-2 from ArchLinux. For me it is not working, which is expected.

@niccokunzmann
Copy link
Member

Can you test it with the master branch? I will add another test.

pip install https://github.com/collective/icalendar

niccokunzmann added a commit that referenced this pull request Oct 3, 2022
@niccokunzmann
Copy link
Member

I added 334b019 and the tests seem to run: #426 - it seems that the latest version would not contain the error. I think it is time for another release soon...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unwanted unescaping of http url strings
3 participants