grafana / oncall

Developer-friendly incident response with brilliant Slack integration
GNU Affero General Public License v3.0
3.49k stars 288 forks source link

iCal feed incorrectly uses Location property #2778

Closed waddles closed 1 year ago

waddles commented 1 year ago

What went wrong?

What happened:

Grafana Oncall uses the Location property of the iCal feed to denote whether a calendar event is from the primary rotation or from an overrides layer. https://github.com/grafana/oncall/blob/5fcff31e4ab24228e655e40074ea425b44dbf2cb/engine/apps/schedules/models/on_call_schedule.py#L176-L181 and https://github.com/grafana/oncall/blob/5fcff31e4ab24228e655e40074ea425b44dbf2cb/engine/apps/schedules/ical_utils.py#L212-L217

This produces an event that looks like this in the iCal feed:

BEGIN:VEVENT
SUMMARY:Rotation L1: user@domain
DTSTART;VALUE=DATE-TIME:20231009T230000Z
DTEND;VALUE=DATE-TIME:20231016T230000Z
DTSTAMP;VALUE=DATE-TIME:20230811T001500Z
UID:OE33ASP3XRKAZ-202310092300-U87QR1IBDZFDA
LAST-MODIFIED;VALUE=DATE-TIME:20230811T001500Z
LOCATION:primary
END:VEVENT

What did you expect to happen:

A more appropriate property for this purpose is Priority which allows arbitrary use of an integer to denote a priority, with lower values representing higher priority. A priority of 0 is the same as not having any priority, so it could easily be switched out without making any change to the meaning of the calendar types. eg.

        if calendar_type == CALENDAR_TYPE_FINAL:
            event_calendar_type = (
                schedule.OVERRIDES
                if event.get(ICAL_PRIORITY, "") == schedule.OVERRIDES
                else schedule.PRIMARY
            )

but would result in a sensible VEVENT:

BEGIN:VEVENT
SUMMARY:Rotation L1: user@domain
DTSTART;VALUE=DATE-TIME:20231009T230000Z
DTEND;VALUE=DATE-TIME:20231016T230000Z
DTSTAMP;VALUE=DATE-TIME:20230811T001500Z
UID:OE33ASP3XRKAZ-202310092300-U87QR1IBDZFDA
LAST-MODIFIED;VALUE=DATE-TIME:20230811T001500Z
PRIORITY:0
END:VEVENT

BEGIN:VEVENT
SUMMARY:Rotation L1: user@domain
DTSTART;VALUE=DATE-TIME:20231016T230000Z
DTEND;VALUE=DATE-TIME:20231023T230000Z
DTSTAMP;VALUE=DATE-TIME:20230811T001500Z
UID:OE33ASP3XRKAZ-202310162300-U87QR1IBDZFDA
LAST-MODIFIED;VALUE=DATE-TIME:20230811T001500Z
PRIORITY:1
END:VEVENT

This would also free up the LOCATION property to be used for more meaningful purposes, for example to indicate a campus, building or kiosk where the user should be located for their shift.

How do we reproduce it?

Subscribe to an iCal feed

Grafana OnCall Version

v1.3.21

Product Area

Schedules

Grafana OnCall Platform?

Kubernetes

User's Browser?

No response

Anything else to add?

No response

matiasb commented 1 year ago

Makes sense :+1:, it should be a relatively simple update. If anyone wants to prepare a PR, that's welcome. Otherwise, will add to my queue.