feat: use icalendar instead manual template#9187
Conversation
|
this function replaces usage of |
There was a problem hiding this comment.
See inline - I think you can lean more heavily on icalendar to handle the formatting.
Also, for the new code it'd be good to run ruff (or at least change ' to ", which I see has been mixed a bit in the new code)
Edit to add: I think working in UTC is a good choice.We could add the meeting time zone to the description as an informational detail. (Just the name, not the full definition.) I don't know whether anyone cares though.
|
|
||
| # LOCATION: only if location should be shown | ||
| if item.timeslot.show_location and item.timeslot.get_location(): | ||
| event.add('location', vText(item.timeslot.get_location())) |
There was a problem hiding this comment.
Here and elsewhere: the event.add() method converts strings to vText internally, so I think you can just pass a string instead of doing it yourself. I don't think it's harmful, though.
(According to the icalendar docs, you do need to manually wrap things in vText for parameter values, but that's when you're doing things like organizer.params["cn"] = vText(...).)
| event.add('url', item.session.agenda.get_versionless_href()) | ||
|
|
||
| # DESCRIPTION: build comprehensive description | ||
| description_parts = [ics_esc(item.timeslot.name)] |
There was a problem hiding this comment.
I don't think you need the ics_esc - icalendar does this itself. E.g., the newlines/semicolons/commas/backslashes in this test are all escaped:
>>> e.add("description", "This is a description\nwith multiple lines\nand other weirdnesses; this is a \\ (backslash), or something isn't it nice!?")results in
DESCRIPTION:This is a description\nwith multiple lines\nand other weirdnes
ses\; this is a \\ (backslash)\, or something isn't it nice!?
| pass | ||
|
|
||
| # Join all description parts with 2 newlines (not escaped) | ||
| description = "\n\n".join(description_parts) |
There was a problem hiding this comment.
Is the "not escaped" something you're doing for a particular purpose or is it to replicate the previous behavior? Similar to the ical_esc comment above, I think icalendar is doing escaping for you and you just want to build up the literal string that you want and let it do the rest.
| "updated": updated | ||
| }, content_type="text/calendar") | ||
| ical_content = generate_agenda_ical(schedule, assignments) | ||
| ical_content = parse_ical_line_endings(ical_content) |
There was a problem hiding this comment.
(broken record) I think that icalendar handles the newlines correctly for you so we shouldn't need to call the fixup method on its output
|
quick profiling comparison of rendering:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9187 +/- ##
==========================================
+ Coverage 88.74% 88.76% +0.01%
==========================================
Files 321 320 -1
Lines 41853 41754 -99
==========================================
- Hits 37144 37063 -81
+ Misses 4709 4691 -18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jennifer-richards
left a comment
jennifer-richards
left a comment
There was a problem hiding this comment.
I think the test probably needs another tweak. I also pointed at what I think is a long-standing issue with the URL property in the icalendar files we generate. We could change that here, but might be better to deal with it separately.
| break | ||
|
|
||
| self.assertContains(r, f"LOCATION:{slot.location.name}") | ||
| self.assertContains(r, f"URL:{session.agenda().get_href()}") |
There was a problem hiding this comment.
This isn't a new issue, as you're preserving existing behavior, but I think we may be violating RFC5545 with the URL parameter. It says
Description: This property may be used in a calendar component to
convey a location where a more dynamic rendition of the calendar
information associated with the calendar component can be found.
This memo does not attempt to standardize the form of the URI, nor
the format of the resource pointed to by the property value. If
the URL property and Content-Location MIME header are both
specified, they MUST point to the same resource.
I'm pretty sure the intent is that URL point a place to get the calendar event itself, not just "some handy URL for it". I.e., it should probably contain ical_url.
The RFC doesn't define a format for the URL or the resource it points to, so it's hard to say we're technically wrong, but I don't think this property is affecting calendars the way it is intended
There was a problem hiding this comment.
@jennifer-richards so you think every event's "URL" should point to its own session's ics URL, like
URL:https://datatracker.ietf.org/meeting/123/session/34370.ics
There was a problem hiding this comment.
I think that's more clearly in line with the intended purpose. See the example in 3.8.4.6
There was a problem hiding this comment.
(echoing comments from slack): I finally actually reviewed how a couple calendar tools use the URL field and I think we should point to the "session materials" link. That's the most "dynamic rendition of the calendar information" we have.
| cal = Calendar.from_ical(r.content) | ||
|
|
||
| for component in cal.walk(): | ||
| if component.name == "VEVENT": |
There was a problem hiding this comment.
Is anything ensuring that a VEVENT is encountered? The test should verify that the expected number of VEVENTs is present.
There was a problem hiding this comment.
added test to count events and test specifically the values of first event
3 participants
fixes #5393 for route
/meeting/{meeting-number}/session/{session-id}.icsor any route that usesviews.agenda_icalcreates .ics files using icalendar package instead of hand-made template
Uses simplyfied format using UTC, like
instead of