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 ical #316

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Fix ical #316

wants to merge 5 commits into from

Conversation

filug
Copy link

@filug filug commented Apr 18, 2021

This MR is changing the way how 'full day' leaves are represented in ICS calendars.

Instead of "hardcoded" 0:00→23:59 duration, now DTSTART & DTEND fields can use Date (instead of DateTime) notation which gives ical clients possibility to show events/leaves as 'full day' event.

Here you can see the difference

Old visualization

image

New visualization

image

This original visualization was (at least for me) extremely annoying. When I wanted to mix two types of calendars: one with my appointments and second with all leaves in my company (so 'global' or 'my subordinates' calendar) generated view was like this

image

In case when more than one coworker was listed situation was even worst.

About VTIMEZONE

ICS content returned by jorani to the clients were tested by my on https://icalendar.org/validator.html page and this validator complains about missing VTIMEZONE entries. Therefore, this ICAL element were added as well.
This part of code was borrowed from https://gist.github.com/thomascube/47ff7d530244c669825736b10877a200 and only slightly aligned with the rest of jorani software.

In case of full day leave, previous implementation was using 0:00...23:59
notation, which by most (if not all) calendar applications were not treated
as full day event. Instead  each leave was shown as long event, one or more
days long.

Such visualization cause that calendar view was absolutely unreadable. In
case when 'global' were subscribed, and then for example merged with others
'Personal' calendars, one person leave event generates calendar mess.

Now full day leaves are reported as full day events, therefore they can/are
represented by the applications in special way which are free of previous
effect.
Seems there is no need to create another one VCalendar instance and then
use it to create VTIMEZONE object.

VCalendar object already exist and it can be used here without any
problems.
In this same way like in previous commit VEVENT creation can use existing
already VCalendar instance instead of creating own temporary object which
is useless once VEVENT is created.

Here existing VCalendar object is now provided to createVEvent function
and it can be used as an factory for VEVENT.
Changed from add_vtimezone to addVTimezone.
By mistake, for every event from 'collaborators' view separate VCalendar
object was created which finally returns one(last) calendar object with
only one event inside.
@bbalet
Copy link
Owner

bbalet commented Apr 18, 2021

Hi,

Thanks for this PR. Did you test it with Google Calendar and Outlook? In a previous PR the solution was only working with MacOS' client See #218

@filug
Copy link
Author

filug commented Apr 18, 2021

Hi,

Thanks for this PR. Did you test it with Google Calendar and Outlook?

My Jorani instance is working in intranet only, therefore Google Calendar was tested like this:

  1. Download ICS file ('global' calendar) and save it in ics file,
  2. import ics file to Google Calendar

seems to work, see below.

Jorani configuration

image

Google calendar

image

and last event details

image

My Jorani instance is working in intranet, therefore I have no possibility to subscribe this calendar to Google Calendar. That's the reason why ICS file were exported/imported manually.

Outlook was not tested (yet), as I'm Linux guy, I have limited access to Windows workstations and Outlook as well. Will try to test it asap (up to 3 days I think).

Moreover: Calendar was tested with Thunderbird (where it seems to work, see my previous screenshots).

In a previous PR the solution was only working with MacOS' client See #218

Here you have much more changes, so I agree I should to be tested carefully.
BTW: I have no access to MacOS at all, so I'm not capable to test my changes on this platform.

@filug
Copy link
Author

filug commented Apr 20, 2021

@bbalet here you can see Outlook test result

Configuration at Jorani side

image

and outlook calendar

image (3)

It seems to work as well.

@bbalet
Copy link
Owner

bbalet commented Apr 20, 2021

Looks good to me.
Thank you for the comprehensive tests.
I'll check a bit and merge.

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

Successfully merging this pull request may close these issues.

2 participants