Expose Horde Api calendars (timeobjects) to DAV in readonly mode #226

ralflang · 2017-08-13T16:58:18Z

Expose Horde Api calendars (timeobjects) to DAV in readonly mode
Omit tasks as they already have their own backend.

External calendars are attributed to the viewing user's principal.

External calendar IDs are transformed as they may contain slash / which is interpreted as directory separator and breaks stuff.

Tested this patch against a production horde 5.2 but there is no real difference with current master.

…t tasks

yunosh · 2017-08-15T10:08:08Z

kronolith/lib/Application.php

+                'principaluri' => 'principals/' . $user,
+                '{http://sabredav.org/ns}owner-principal' =>
+                    'principals/-system-', // TODO: Not sure who should really own these, system or the calling user
+                '{DAV:}displayname' => sprintf("%s: %s", ucfirst($calendar->api()),  $calendar->name()),


This should be $registry->get('name', $calendar->api()).

yunosh · 2017-08-15T10:09:13Z

kronolith/lib/Application.php

+                '{http://sabredav.org/ns}owner-principal' =>
+                    'principals/-system-', // TODO: Not sure who should really own these, system or the calling user
+                '{DAV:}displayname' => sprintf("%s: %s", ucfirst($calendar->api()),  $calendar->name()),
+                '{' . CalDAV\Plugin::NS_CALDAV . '}calendar-description' =>


I would actually leave this off until there is a better value to get than the duplication of the calendar name.

yunosh · 2017-08-15T10:11:37Z

kronolith/lib/Application.php

+                    $calendar->caldavUrl(),
+                'principaluri' => 'principals/' . $user,
+                '{http://sabredav.org/ns}owner-principal' =>
+                    'principals/-system-', // TODO: Not sure who should really own these, system or the calling user


I tend to own this to the user. In fact we have both, global and per-user resourced provided through the timeobjects API. But it makes more sense IMO to have different per-user calendars with the same data, than single global calendars with individual data.

yunosh · 2017-08-15T10:13:31Z

kronolith/lib/Application.php

@@ -945,6 +945,35 @@ public function davGetCollections($user)
                '{http://sabredav.org/ns}read-only' => !$share->hasPermission($hordeUser, Horde_Perms::EDIT),
            );
        }
+        // External Calendars from other Horde Apps
+                foreach ($calendar_manager->get(Kronolith::ALL_EXTERNAL_CALENDARS) as $calendarId => $calendar) {


yunosh · 2017-08-15T10:14:04Z

kronolith/lib/Application.php

+                'principaluri' => 'principals/' . $user,
+                '{http://sabredav.org/ns}owner-principal' =>
+                    'principals/-system-', // TODO: Not sure who should really own these, system or the calling user
+                '{DAV:}displayname' => sprintf("%s: %s", ucfirst($calendar->api()),  $calendar->name()),


Single quotes please; spacing.

yunosh · 2017-08-15T10:14:24Z

kronolith/lib/Application.php

+        $exploded = explode(':', $internal, 2);
+        $driverType = null;
+        if ($exploded[0] == "external") {
+            $driverType = "Horde";


Single quotes please.

yunosh · 2017-08-15T10:17:44Z

kronolith/lib/Application.php

+        $driverType = null;
+        if ($exploded[0] == "external") {
+            $driverType = "Horde";
+            $internalName = str_replace(':', '/', $exploded[1]);


Why using a different separator in Kronolith_Calendar_External in the first place?

When using the original separator "/", caldav misbehaves as it uses the calendar id string in several occassions for creating URLs. Calendars won't be accessible without creating additional caldav directory hierarchies.

yunosh · 2017-08-15T10:18:33Z

kronolith/lib/Application.php

+        if ($share) {
+            $ical->setAttribute('X-WR-CALNAME', $share->get('name'));
+        } else {
+            // TODO: Get Calendar object and run ->name()


This should be implemented.

yunosh · 2017-08-15T10:19:02Z

kronolith/lib/Calendar/External.php

@@ -96,6 +96,24 @@ public function api()
    {
        return $this->_api;
    }
+   /**


Vertical spacing

yunosh · 2017-08-15T10:20:33Z

kronolith/lib/Calendar/External.php

+     */
+    public function internalId()
+    {
+        return sprintf("external:%s:%s", $this->_api, str_replace('/', ':', $this->_id));


See above about the different separator.

…" field

yunosh · 2017-08-15T20:41:02Z

kronolith/lib/Application.php

                '{DAV:}displayname' => sprintf("%s: %s", ucfirst($calendar->api()),  $calendar->name()),
                '{' . CalDAV\Plugin::NS_CALDAV . '}calendar-description' =>
-                    $calendar->name(), // TODO: Implement an interface for description and fallback to name if none given
+                    '', // TODO: Implement an interface for description and fallback to empty if none given


You can leave that property out completely.

yunosh · 2017-08-15T20:43:20Z

kronolith/lib/Application.php

            $internalName = str_replace(':', '/', $exploded[1]);
        } elseif (!Kronolith::hasPermission($internal, Horde_Perms::SHOW)) {
-            throw new Kronolith_Exception(_("Calendar does not exist or no permission to edit"));
+            throw new Kronolith_Exception(_('Calendar does not exist or no permission to edit'));


Gettext strings OTOH must have double quotes :)

Done. I was not aware.

CODING_STANDARDS shows the double quote example in section 23 internatl. , but does not instruct to use double quotes. In 27 quotes it says Should be used in the gettext shortcut _("") format.

yunosh · 2017-08-16T09:42:26Z

The patch is fine now, many thanks!
It doesn't work as expected yet, though. One of my address books returning anniversaries results in a 500 error from Sabre, stating "The objectData argument must contain an 'uri' property". Other paths to events (like anniversaries from address books) return a 404 right away.

ralflang · 2017-08-16T09:44:01Z

On 16.08.2017 11:42, Jan Schneider wrote: It doesn't work as expected yet, though. One of my address books returning anniversaries results in a 500 error from Sabre, stating "The objectData argument must contain an 'uri' property". Other paths return a 404 right away.

I will check on this - I tested with whups and one other not yet released application as data provider. I will add some aniversaries and try to reproduce.

…

-- Ralf Lang Linux Consultant / Developer Tel.: +49-170-6381563 Mail: [email protected] B1 Systems GmbH Osterfeldstraße 7 / 85088 Vohburg / http://www.b1-systems.de GF: Ralph Dehner / Unternehmenssitz: Vohburg / AG: Ingolstadt,HRB 3537

ralflang · 2017-08-16T11:36:35Z

On 16.08.2017 11:43, Ralf Lang wrote: On 16.08.2017 11:42, Jan Schneider wrote: > It doesn't work as expected yet, though. One of my address books > returning anniversaries results in a 500 error from Sabre, stating > "The objectData argument must contain an 'uri' property". Other paths > return a 404 right away. I will check on this - I tested with whups and one other not yet released application as data provider. I will add some aniversaries and try to reproduce.

Now the previously working stuff is broken too. Seems like one of the refactoring steps changed something. This call winds down to calling calendarobject->get() with missing properties. I am checking why... #3 Sabre\DAV\Server->getPropertiesForPath(calendars/lang/calendar~external:tickets:created, Array ([0] => {DAV:}displayname,[1] => {DAV:}resourcetype,[2] => {DAV:}getcontenttype,[3] => {DAV:}getcontentlength,[4] => {DAV:}getlastmodified), 1) called at [/usr/share/php5/PEAR/Sabre/DAV/Browser/Plugin.php:252]

…

-- Ralf Lang Linux Consultant / Developer Tel.: +49-170-6381563 Mail: [email protected] B1 Systems GmbH Osterfeldstraße 7 / 85088 Vohburg / http://www.b1-systems.de GF: Ralph Dehner / Unternehmenssitz: Vohburg / AG: Ingolstadt,HRB 3537

…n exception. Fixes whups items in horde 5.2 TODO: prevent eternal recursion for birthdays in turba, probably due to recurrences TODO: Test again against master

… crafting a uid

ralflang · 2017-08-17T16:38:41Z

I have implemented providing a crafted UID. This works both against our production horde (whups, turba) and against a master 6.0.0 checkout from yesterday. If it still crashes for you, can you provide an example event? Regards Ralf

yunosh · 2017-08-25T10:12:26Z

I still see a few issues:

The external calendars now show up both unter the -system- and the personal namespace.
The Application prefix of the calendar names always is "Horde", not "Address Book" etc.
The calculation of birthdays/anniversaries is off (a 200o years).
The event IDs (not UIDs) seem to be missing some prefix, e.g.: _contactsFaG7igYk1U2zDEzVgN_SUUO.ics. The leading underscore looks like there is missing something? Or is it simply superfluous?

We also need to think about the UIDs once again. First of all, the timeobject APIs should probably export any existing UIDs. On the other hand, this would lead to the same UIDs for two different kind of objects (contact + event). This is probably a bad idea.

ralflang · 2017-08-27T18:55:54Z

The external calendars now show up both unter the -system- and the personal namespace.
The Application prefix of the calendar names always is "Horde", not "Address Book" etc.

Both fixed.

The calculation of birthdays/anniversaries is off (a 200o years).
Cannot reproduce against master nor 5.3 - can you provide an example addressbook entry?
The event IDs (not UIDs) seem to be missing some prefix, e.g.: _contactsFaG7igYk1U2zDEzVgN_SUUO.ics. The leading underscore looks like there is missing something? Or is it simply superfluous?

_contacts (_api) is a prefix added to the item id to prevent any clashes with existing IDs.
See

horde/kronolith/lib/Event/Horde.php

Line 105 in 8aba6f7

$this->id = '_' . $this->_api . $event['id'];

It gets removed for toTimeObject.

--
We also need to think about the UIDs once again. First of all, the timeobject APIs should probably export any existing UIDs. On the other hand, this would lead to the same UIDs for two different kind of objects (contact + event). This is probably a bad idea.

I think we should not mix this patch with concerns of the exporting applications. These should be independent follow-up patches. Currently, if the application exports no UID to timeobjects (which seems standard case), we craft a uid. The fallback for application-provided UIDs is already there.

It is either the exporting application's responsibility to make sure the timeobjects, cost objects etc do not have the same uid as the original application object (like a contact or ticket) or it needs to be handled by the infrastructure. Either way it should not be the receiver's (kronolith's) task. For example, the uid could be (originalitemuid)@TimeObjectCategory.Api

yunosh · 2017-08-28T08:19:06Z

It is either the exporting application's responsibility to make sure the timeobjects, cost objects etc do not have the same uid as the original application object (like a contact or ticket) or it needs to be handled by the infrastructure. Either way it should not be the receiver's (kronolith's) task. For example, the uid could be (originalitemuid)@TimeObjectCategory.Api

I don't agree. If Kronolith is converting this object from one format to a different (incompatible) format, it basically creates a new object, and thus shouldn't reuse the original UiD. Unless we agree it's perfectly fine to have a contact (or any other object FWIW) and an event with the same UID.

ralflang · 2017-08-28T08:28:10Z

Am 28.08.2017 um 10:19 schrieb Jan Schneider:

It is either the exporting application's responsibility to make sure the timeobjects, cost objects etc do not have the same uid as the original application object (like a contact or ticket) or it needs to be handled by the infrastructure. Either way it should not be the receiver's (kronolith's) task. For example, the uid could be ***@***.*** I don't agree. If Kronolith is converting this object from one format to a different (incompatible) format, it basically creates a new object, and thus shouldn't reuse the original UiD. Unless we agree it's perfectly fine to have a contact (or any other object FWIW) and an event with the same UID.

I agree with the first part - We currently do not reuse uids but craft them inside kronolith. The second part does not feel right to me, but I can do as you think. You're the expert in caldav and kronolith. Currently I see no app exporting timeobjects including UIDs. If an app exports timeobjects (or other api hashes like cost objects, clients) WITH UIDs, it should know if the uid must be same or different from the original item. Should I simply ignore the exported uid (if any) in favor of the kronolith-provided one or amend it to make it different?

…

-- Ralf Lang Linux Consultant / Developer

Expose Horde Api calendars (timeobjects) to DAV in readonly mode, omi…

99531b1

…t tasks

ralflang requested a review from mrubinsk August 13, 2017 16:58

yunosh requested changes Aug 15, 2017

View reviewed changes

root added 5 commits August 15, 2017 13:15

Indention

a643ad3

Attribute external calendars to the browsing user, blank "description…

bbe9219

…" field

Use registry name of providing app, not Api name

31250a0

Vertical Spacing

c190c21

implement X-WR-CALNAME for external calendars

3ce136c

yunosh reviewed Aug 15, 2017

View reviewed changes

leave out empty calendar description property, gettext use double quotes

7f6e232

root and others added 3 commits August 16, 2017 16:31

Only try to load a share if we expect one - otherwise need to catch a…

30f71be

…n exception. Fixes whups items in horde 5.2 TODO: prevent eternal recursion for birthdays in turba, probably due to recurrences TODO: Test again against master

Fix recurrences for Horde Backend dav objects (birthdays in turba) by…

11ad83c

… crafting a uid

Merge branch 'master' into kronolith-externals

926c749

ralflang added 2 commits August 27, 2017 10:17

Fix display application name instead of "Horde"

38f6689

Do not show External calendars for pseudo user -system-

2d5ab72

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose Horde Api calendars (timeobjects) to DAV in readonly mode #226

Expose Horde Api calendars (timeobjects) to DAV in readonly mode #226

ralflang commented Aug 13, 2017 •

edited

Loading

yunosh Aug 15, 2017

yunosh Aug 15, 2017

yunosh Aug 15, 2017

yunosh Aug 15, 2017

yunosh Aug 15, 2017

yunosh Aug 15, 2017

yunosh Aug 15, 2017

ralflang Aug 15, 2017

yunosh Aug 15, 2017

yunosh Aug 15, 2017

yunosh Aug 15, 2017

yunosh Aug 15, 2017

ralflang Aug 16, 2017

yunosh Aug 15, 2017

ralflang Aug 16, 2017

yunosh commented Aug 16, 2017 •

edited

Loading

ralflang commented Aug 16, 2017 via email

ralflang commented Aug 16, 2017 via email

ralflang commented Aug 17, 2017 via email

yunosh commented Aug 25, 2017 •

edited by ralflang

Loading

ralflang commented Aug 27, 2017

yunosh commented Aug 28, 2017

ralflang commented Aug 28, 2017 via email •

edited

Loading

Expose Horde Api calendars (timeobjects) to DAV in readonly mode #226

Are you sure you want to change the base?

Expose Horde Api calendars (timeobjects) to DAV in readonly mode #226

Conversation

ralflang commented Aug 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yunosh commented Aug 16, 2017 • edited Loading

ralflang commented Aug 16, 2017 via email

ralflang commented Aug 16, 2017 via email

ralflang commented Aug 17, 2017 via email

yunosh commented Aug 25, 2017 • edited by ralflang Loading

ralflang commented Aug 27, 2017

-- We also need to think about the UIDs once again. First of all, the timeobject APIs should probably export any existing UIDs. On the other hand, this would lead to the same UIDs for two different kind of objects (contact + event). This is probably a bad idea.

yunosh commented Aug 28, 2017

ralflang commented Aug 28, 2017 via email • edited Loading

ralflang commented Aug 13, 2017 •

edited

Loading

yunosh commented Aug 16, 2017 •

edited

Loading

yunosh commented Aug 25, 2017 •

edited by ralflang

Loading

--
We also need to think about the UIDs once again. First of all, the timeobject APIs should probably export any existing UIDs. On the other hand, this would lead to the same UIDs for two different kind of objects (contact + event). This is probably a bad idea.

ralflang commented Aug 28, 2017 via email •

edited

Loading