Calendar API for apps by georgehrke · Pull Request #6840 · nextcloud/server

georgehrke · 2017-10-15T21:26:21Z

implements / fixes #5282

georgehrke · 2017-10-15T21:27:01Z

@tcitworld @nickvergessen @MorrisJobke Any comment on the API design? :)

if u give me a 👍 I will go ahead and connect it to the dav app.

georgehrke · 2017-10-15T21:30:03Z

The first version of this API will only allow read-only access, which should already cover >95% of the use cases. If demanded, we can still add write support in a later revision.

codecov · 2017-10-16T08:09:15Z

Codecov Report

Merging #6840 into master will increase coverage by 0.04%.
The diff coverage is 77.9%.

@@             Coverage Diff              @@
##             master    #6840      +/-   ##
============================================
+ Coverage     50.74%   50.79%   +0.04%     
- Complexity    24423    24474      +51     
============================================
  Files          1580     1583       +3     
  Lines         93366    93537     +171     
  Branches       1359     1359              
============================================
+ Hits          47380    47513     +133     
- Misses        45986    46024      +38

Impacted Files	Coverage Δ	Complexity Δ
apps/dav/composer/composer/autoload_static.php	`0% <ø> (ø)`	`1 <0> (ø)`	⬇️
apps/dav/appinfo/app.php	`28.57% <0%> (-11.43%)`	`0 <0> (ø)`
apps/dav/composer/composer/autoload_classmap.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
apps/dav/lib/AppInfo/Application.php	`20.22% <25%> (-0.71%)`	`14 <2> (+1)`
lib/private/Server.php	`83.13% <50%> (-0.16%)`	`126 <1> (+1)`
apps/dav/lib/CalDAV/CalDavBackend.php	`84.99% <77.01%> (-0.73%)`	`242 <23> (+23)`
apps/dav/lib/CalDAV/CalendarManager.php	`78.57% <78.57%> (ø)`	`4 <4> (?)`
lib/private/Calendar/Manager.php	`88.88% <88.88%> (ø)`	`12 <12> (?)`
apps/dav/lib/CalDAV/CalendarImpl.php	`96.66% <96.66%> (ø)`	`10 <10> (?)`
... and 2 more

tcitworld · 2017-10-16T08:11:15Z

lib/public/Calendar/ICalendar.php

+	 * @return string defining the technical unique key
+	 * @since 13.0.0
+	 */
+	public function getKey();


Maybe change this name to something more obvious?

I chose this name for consistency reasons with the contacts api: https://github.com/nextcloud/server/blob/master/lib/public/IAddressBook.php#L46

What other name would u suggest?

Well, I guess I would have gone with something telling it's the ID of the calendar but let's be consistent with the contact api.

nickvergessen · 2017-10-16T10:21:04Z

This is just a public interface for existing stuff? Or will it implement new features?

georgehrke · 2017-10-16T10:21:53Z

This is just a public interface for existing stuff? Or will it implement new features?

I will implement new stuff. The calendar API does not exist yet.
We need something similar to the contacts api.

Edit: I just want to discuss the API design before going ahead and implementing everything :)

LukasReschke · 2017-10-19T09:01:37Z

lib/public/Calendar/ICalendar.php

+
+	/**
+	 * In comparison to getKey() this function returns a human readable (maybe translated) name
+	 * @return mixed


LukasReschke · 2017-10-19T09:01:46Z

lib/public/Calendar/ICalendar.php

+	 * @return array an array of events/journals/todos which are arrays of key-value-pairs
+	 * @since 13.0.0
+	 */
+	public function search($pattern, $searchProperties, $options, $limit, $offset);


Typehint all the arrays

LukasReschke · 2017-10-19T09:02:37Z

lib/public/Calendar/ICalendar.php

+	 * @param array $options - optional parameters:
+	 * 	['timerange' => ['start' => new DateTime(...), 'end' => new DateTime(...)]]
+	 * @param integer|null $limit - limit number of search results
+	 * @param integer $offset - offset for paging of search results


Can that also be null? Because the param above is. Also, you may want to add a default = null in the function definition then.

Or maybe just 0?

Having $offset = null would be semantically equivalent to $offset = 0, which is already covered by integer.

Do we usually allow both null and integer in other APIs?

Ok, according to https://github.com/nextcloud/server/search?utf8=✓&q=integer%7Cnull+%24limit&type= we usually do. So I'll allow both int and null for consistency

LukasReschke · 2017-10-19T09:02:58Z

lib/public/Calendar/ICalendar.php

+	public function search($pattern, $searchProperties, $options, $limit, $offset);
+
+	/**
+	 * @return mixed


Mixed is so horrible. What can be in here? ;-) - Explaiiin ;-)

LukasReschke · 2017-10-19T09:03:44Z

lib/public/Calendar/IManager.php

+namespace OCP\Calendar;
+
+/**
+ * This class provides access to the calendar app.


Is it really the calendar app or just the nextcloud caldav backend?

CalDAV backend indeed, will change

georgehrke · 2017-10-20T09:16:33Z

@LukasReschke If you are happy, I will go ahead with implementing the Calendar Manager and connecting it to the dav app.

ChristophWurst · 2017-10-23T15:13:09Z

lib/public/Calendar/ICalendar.php

+	 * @return array an array of events/journals/todos which are arrays of key-value-pairs
+	 * @since 13.0.0
+	 */
+	public function search($pattern, $searchProperties=[], $options=[], $limit, $offset);


From http://php.net/manual/en/functions.arguments.php#functions.arguments.default

Note that when using default arguments, any defaults should be on the right side of any non-default arguments; otherwise, things will not work as expected.

Also I'd suggest to type-hint the two array parameters with array.

tcitworld

Is there a way to add shares information easily here ?

tcitworld · 2017-10-29T20:27:41Z

lib/public/Calendar/IManager.php

+	 * @return array
+	 * @since 13.0.0
+	 */
+	public function getCalendars();


Array of ICalendar I guess (please precise :) )

tcitworld · 2017-10-29T20:31:13Z

lib/public/Calendar/ICalendar.php

+	 * @since 13.0.0
+	 */
+	public function getDisplayName();
+


Maybe add getDisplayColor() or getColor() too since that will probably be used a lot.

georgehrke · 2017-11-07T09:52:31Z

Is there a way to add shares information easily here ?

What kind of information do you want to request here?
Are u referring to nextcloud/calendar#643 (comment) ?

tcitworld · 2017-11-07T09:54:38Z

I was thinking more of the people with whom the calendar is shared, but public shares are good too.

Signed-off-by: Georg Ehrke <developer@georgehrke.com>

Requested changes applied

Signed-off-by: Georg Ehrke <developer@georgehrke.com>

georgehrke · 2017-11-09T16:10:48Z

please review @tcitworld @MorrisJobke @LukasReschke :)

georgehrke added the 2. developing Work in progress label Oct 15, 2017

georgehrke force-pushed the feature/5282/calendar_api branch from 267390e to 5b04846 Compare October 16, 2017 08:08

tcitworld reviewed Oct 16, 2017

View reviewed changes

LukasReschke previously requested changes Oct 19, 2017

View reviewed changes

georgehrke force-pushed the feature/5282/calendar_api branch 2 times, most recently from 89be19e to d403d30 Compare October 20, 2017 09:15

ChristophWurst previously requested changes Oct 23, 2017

View reviewed changes

georgehrke force-pushed the feature/5282/calendar_api branch from d403d30 to f83215b Compare October 23, 2017 16:01

tcitworld previously approved these changes Oct 29, 2017

View reviewed changes

georgehrke force-pushed the feature/5282/calendar_api branch from f83215b to 9814821 Compare October 30, 2017 17:21

georgehrke force-pushed the feature/5282/calendar_api branch 2 times, most recently from fbef205 to 26a474d Compare November 7, 2017 01:13

georgehrke self-assigned this Nov 7, 2017

georgehrke force-pushed the feature/5282/calendar_api branch 2 times, most recently from a4e1206 to 360e9d9 Compare November 9, 2017 11:44

Public interfaces for calendar API

5d206ee

Signed-off-by: Georg Ehrke <developer@georgehrke.com>

georgehrke force-pushed the feature/5282/calendar_api branch from 360e9d9 to 0b0b85b Compare November 9, 2017 12:02

georgehrke force-pushed the feature/5282/calendar_api branch from ff9ad26 to 1684ee5 Compare November 9, 2017 14:11

georgehrke changed the title ~~Public interfaces for calendar API~~ Calendar API for apps Nov 9, 2017

georgehrke added 5 commits November 9, 2017 15:14

implement CalendarManager

556b2a2

Signed-off-by: Georg Ehrke <developer@georgehrke.com>

integrate Dav app with Calendar API

fa416e0

Signed-off-by: Georg Ehrke <developer@georgehrke.com>

update autoloader to included added classes and interface

f06fc67

Signed-off-by: Georg Ehrke <developer@georgehrke.com>

update dav autoloaders after adding calendar api

dd13aec

Signed-off-by: Georg Ehrke <developer@georgehrke.com>

add option to filter CalendarAPI by componenttype

7784672

Signed-off-by: Georg Ehrke <developer@georgehrke.com>

georgehrke force-pushed the feature/5282/calendar_api branch from 1684ee5 to 7784672 Compare November 9, 2017 14:15

georgehrke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 9, 2017

georgehrke added this to the Nextcloud 13 milestone Nov 11, 2017

LukasReschke approved these changes Nov 16, 2017

View reviewed changes

MorrisJobke approved these changes Nov 16, 2017

View reviewed changes

MorrisJobke merged commit f32fbbc into master Nov 16, 2017

MorrisJobke deleted the feature/5282/calendar_api branch November 16, 2017 20:10

MorrisJobke mentioned this pull request Dec 12, 2017

Fix unit tests for CalendarManager #7458

Merged

Uh oh!

Conversation

georgehrke commented Oct 15, 2017

Uh oh!

georgehrke commented Oct 15, 2017

Uh oh!

georgehrke commented Oct 15, 2017

Uh oh!

codecov bot commented Oct 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickvergessen commented Oct 16, 2017

Uh oh!

georgehrke commented Oct 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georgehrke commented Oct 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcitworld left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georgehrke commented Nov 7, 2017

Uh oh!

tcitworld commented Nov 7, 2017

Uh oh!

georgehrke commented Nov 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Oct 16, 2017 •

edited

Loading

georgehrke commented Oct 16, 2017 •

edited

Loading