Add all properties while creating a subscription #25231

tcitworld · 2016-06-22T14:38:43Z

Should fix issue #24469

This is my first PR, please don't eat me. :-)

mention-bot · 2016-06-22T14:38:44Z

By analyzing the blame information on this pull request, we identified @DeepDiver1975, @nickvergessen and @LukasReschke to be potential reviewers

ghost · 2016-06-22T14:43:27Z

@tcitworld

Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

tcitworld · 2016-06-22T14:50:31Z

And for now, MIT licensed.

DeepDiver1975 · 2016-06-22T16:03:58Z

This is my first PR, please don't eat me. :-)

No worries - THX

May I ask you to add unit tests? Ping me or @georgehrke on irc in case you need a helping hand.

And for now, MIT licensed.

THX

ghost · 2016-06-23T08:48:49Z

@tcitworld

Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

ghost · 2016-06-23T08:48:49Z

@tcitworld

Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

tcitworld · 2016-06-24T11:51:53Z

Tests added.

DeepDiver1975 · 2016-06-27T19:45:41Z

Tests added.

THX @tcitworld - let's see what Jenkins says about it .... #25280

ghost · 2016-06-28T08:02:41Z

@tcitworld

Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

DeepDiver1975 · 2016-06-28T08:14:10Z

@tcitworld please consider signing the CLA - this will grant you full access to the git repo and you can work on branches within the repo. CI will work then much easier - for all of us 😉

tcitworld · 2016-06-28T09:09:07Z

Done.

DeepDiver1975 · 2016-06-28T14:28:07Z

Jenkins is green on #25280

DeepDiver1975 · 2016-06-28T14:28:10Z

👍

DeepDiver1975 · 2016-06-28T14:28:19Z

@georgehrke mind testing? THX

georgehrke · 2016-06-29T12:01:05Z

apps/dav/lib/CalDAV/CalDavBackend.php

@@ -1083,18 +1083,25 @@ function createSubscription($principalUri, $uri, array $properties) {
 			if (isset($properties[$xmlName])) {

 				$values[$dbName] = $properties[$xmlName];
-				$fieldNames[] = $dbName;
+			} else {
+				if ($dbName == 'calendarorder') {


Can you elaborate why this is necessary? it's not in https://github.com/tcitworld/core/blob/9fc27d1d649fa25ba7b4d88a021a7a1b71532c93/apps/dav/lib/CalDAV/CalDavBackend.php#LL1045-1049

sorry, i mean https://github.com/tcitworld/core/blob/9fc27d1d649fa25ba7b4d88a021a7a1b71532c93/apps/dav/lib/CalDAV/CalDavBackend.php#LL376-380

I enforce default values to null (and 0 when database needs it). This part can be removed though.

georgehrke · 2016-06-29T12:03:49Z

code looks good otherwise to me. Will test asap

georgehrke · 2016-06-29T13:00:42Z

<A:mkcol xmlns:A="DAV:">
    <A:set>
        <A:prop>
            <B:subscribed-strip-attachments xmlns:B="http://calendarserver.org/ns/" />
            <B:subscribed-strip-todos xmlns:B="http://calendarserver.org/ns/" />
            <A:resourcetype>
                <A:collection />
                <B:subscribed xmlns:B="http://calendarserver.org/ns/" />
            </A:resourcetype>
            <E:calendar-color xmlns:E="http://apple.com/ns/ical/">#1C4587FF</E:calendar-color>
            <A:displayname>Jewish holidays</A:displayname>
            <C:calendar-description xmlns:C="urn:ietf:params:xml:ns:caldav">Foo</C:calendar-description>
            <E:calendar-order xmlns:E="http://apple.com/ns/ical/">19</E:calendar-order>
            <B:source xmlns:B="http://calendarserver.org/ns/">
                <A:href>webcal://www.webcal.fi/cal.php?id=49&amp;format=ics&amp;wrn=1&amp;wp=1&amp;wf=26&amp;color=%231C4587&amp;cntr=us&amp;lang=en&amp;rid=wc</A:href>
            </B:source>
            <E:refreshrate xmlns:E="http://apple.com/ns/ical/">P1W</E:refreshrate>
            <B:subscribed-strip-alarms xmlns:B="http://calendarserver.org/ns/" />
        </A:prop>
    </A:set>
</A:mkcol>

the database fields striptodos, stripalarms and stripattachments remain null.

georgehrke · 2016-06-30T11:50:42Z

@tcitworld I used https://chrome.google.com/webstore/detail/advanced-rest-client/hgmloofddffdnphfgcellkdfbfbjeloo?hl=en-US to send a MKCOL request using the mentioned payload and checked the db afterwords. Nothing fancy with Calendar.app.

Fixes owncloud#24469

tcitworld · 2016-06-30T13:56:46Z

The $properties array shows up like this for your request :

Array
(
    [{http://calendarserver.org/ns/}subscribed-strip-attachments] => 
    [{http://calendarserver.org/ns/}subscribed-strip-todos] => 
    [{http://apple.com/ns/ical/}calendar-color] => #1C4587FF
    [{DAV:}displayname] => Jewish holidays
    [{urn:ietf:params:xml:ns:caldav}calendar-description] => Foo
    [{http://apple.com/ns/ical/}calendar-order] => 19
    [{http://calendarserver.org/ns/}source] => Sabre\DAV\Xml\Property\Href Object
        (
            [hrefs:protected] => Array
                (
                    [0] => webcal://www.webcal.fi/cal.php?id=49&format=ics&wrn=1&wp=1&wf=26&color=%231C4587&cntr=us&lang=en&rid=wc
                )

            [autoPrefix:protected] => 
        )

    [{http://apple.com/ns/ical/}refreshrate] => P1W
    [{http://calendarserver.org/ns/}subscribed-strip-alarms] => 
)

You'll notice that the subscribed-strip-attachments and subscribed-strip-todos keys exist even if they're not send through the request. It could be a SabreDAV issue, right ?

DeepDiver1975 · 2016-06-30T14:05:39Z

It could be a SabreDAV issue, right ?

I don't think so. These elements are part of the xml which is sent with the mkcol.
Sabre deserialized the xml to the properties array you are receiving

georgehrke · 2016-06-30T14:06:55Z

The problem here is isset. You need to check with array_key_exists for those strip properties

tcitworld · 2016-06-30T14:09:11Z

The problem here is isset. You need to check with array_key_exists for those strip properties

~~That's what I was doing and I figured the keys always exist (see above). Sorry if I'm missing something.~~

Sorry for wasting your time. 😳

tcitworld · 2016-06-30T15:17:29Z

Fixed.

DeepDiver1975 · 2016-06-30T16:10:17Z

Since you are a contributor and have push access: please creat a branch in here and recreate the pr.thx

lock · 2019-08-05T08:01:56Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

DeepDiver1975 added this to the 9.1-current milestone Jun 22, 2016

DeepDiver1975 added the 2 - Developing label Jun 22, 2016

DeepDiver1975 added 3 - To Review and removed 2 - Developing labels Jun 28, 2016

georgehrke reviewed Jun 29, 2016
View reviewed changes

tcitworld added 2 commits June 30, 2016 14:04

Add all properties while creating a subscription

2b0adc6

Fixes owncloud#24469

added more tests for subscription

f66164c

Fix DB issue

ab562ad

tcitworld force-pushed the add-all-properties-create-subscription branch from 9fc27d1 to ab562ad Compare June 30, 2016 14:56

DeepDiver1975 closed this Jun 30, 2016

tcitworld mentioned this pull request Jun 30, 2016

Add all properties when creating a subscription #25318

Merged

tcitworld deleted the add-all-properties-create-subscription branch June 30, 2016 16:15

lock bot locked as resolved and limited conversation to collaborators Aug 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add all properties while creating a subscription #25231

Add all properties while creating a subscription #25231

tcitworld commented Jun 22, 2016

mention-bot commented Jun 22, 2016

ghost commented Jun 22, 2016

tcitworld commented Jun 22, 2016

DeepDiver1975 commented Jun 22, 2016

ghost commented Jun 23, 2016

ghost commented Jun 23, 2016

tcitworld commented Jun 24, 2016

DeepDiver1975 commented Jun 27, 2016

ghost commented Jun 28, 2016

DeepDiver1975 commented Jun 28, 2016

tcitworld commented Jun 28, 2016

DeepDiver1975 commented Jun 28, 2016

DeepDiver1975 commented Jun 28, 2016

DeepDiver1975 commented Jun 28, 2016

georgehrke Jun 29, 2016

georgehrke Jun 29, 2016

tcitworld Jun 29, 2016

georgehrke commented Jun 29, 2016

georgehrke commented Jun 29, 2016 •

edited by tcitworld

georgehrke commented Jun 30, 2016

tcitworld commented Jun 30, 2016

DeepDiver1975 commented Jun 30, 2016

georgehrke commented Jun 30, 2016

tcitworld commented Jun 30, 2016 •

edited

tcitworld commented Jun 30, 2016

DeepDiver1975 commented Jun 30, 2016

lock bot commented Aug 5, 2019

Navigation Menu

Add all properties while creating a subscription #25231

Add all properties while creating a subscription #25231

Conversation

tcitworld commented Jun 22, 2016

mention-bot commented Jun 22, 2016

ghost commented Jun 22, 2016

tcitworld commented Jun 22, 2016

DeepDiver1975 commented Jun 22, 2016

ghost commented Jun 23, 2016

ghost commented Jun 23, 2016

tcitworld commented Jun 24, 2016

DeepDiver1975 commented Jun 27, 2016

ghost commented Jun 28, 2016

DeepDiver1975 commented Jun 28, 2016

tcitworld commented Jun 28, 2016

DeepDiver1975 commented Jun 28, 2016

DeepDiver1975 commented Jun 28, 2016

DeepDiver1975 commented Jun 28, 2016

georgehrke Jun 29, 2016

Choose a reason for hiding this comment

georgehrke Jun 29, 2016

Choose a reason for hiding this comment

tcitworld Jun 29, 2016

Choose a reason for hiding this comment

georgehrke commented Jun 29, 2016

georgehrke commented Jun 29, 2016 • edited by tcitworld

georgehrke commented Jun 30, 2016

tcitworld commented Jun 30, 2016

DeepDiver1975 commented Jun 30, 2016

georgehrke commented Jun 30, 2016

tcitworld commented Jun 30, 2016 • edited

tcitworld commented Jun 30, 2016

DeepDiver1975 commented Jun 30, 2016

lock bot commented Aug 5, 2019

georgehrke commented Jun 29, 2016 •

edited by tcitworld

tcitworld commented Jun 30, 2016 •

edited