Skip to content

Better Calendar Widget Data #36620

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.

Sign up for GitHub

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

Closed
miaulalala wants to merge 3 commits into from
Closed

Better Calendar Widget Data #36620

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
47371f5
Better Calendar Widget Data
miaulalala Feb 8, 2023
f386706
fixup! Better Calendar Widget Data
miaulalala Feb 22, 2023
9537acd
fixup! Better Calendar Widget Data
miaulalala Feb 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 39 additions & 4 deletions apps/dav/lib/CalDAV/CalDavBackend.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,7 @@ public function getCalendarsForUser($principalUri) {
'{' . Plugin::NS_CALDAV . '}supported-calendar-component-set' => new SupportedCalendarComponentSet($components),
'{' . Plugin::NS_CALDAV . '}schedule-calendar-transp' => new ScheduleCalendarTransp($row['transparent']?'transparent':'opaque'),
'{' . \OCA\DAV\DAV\Sharing\Plugin::NS_OWNCLOUD . '}owner-principal' => $this->convertPrincipal($principalUri, !$this->legacyEndpoint),

Copy link
Member

@tcitworld tcitworld Feb 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick (non-blocking): Extra line that came out of nowhere

];

$calendar = $this->rowToCalendar($row, $calendar);
Expand Down Expand Up @@ -1847,7 +1848,24 @@ public function search(array $calendarInfo, $pattern, array $searchProperties,
->from('calendarobjects', 'c')
->where($outerQuery->expr()->isNull('deleted_at'));

if (isset($options['timerange'])) {
if (isset($options['timerange']) && in_array('VTODO', $options['types'])) {
// allow VTODOS in the same query if they are requested in the options.
// they do not have a first / last occurence set and would otherwise not be returned
if (isset($options['timerange']['start']) && $options['timerange']['start'] instanceof DateTimeInterface) {
$or1 = $outerQuery->expr()->orX();
$or1->add($outerQuery->expr()->gt('lastoccurence',
$outerQuery->createNamedParameter($options['timerange']['start']->getTimeStamp())));
$or1->add($outerQuery->expr()->isNull('lastoccurence'));
$outerQuery->andWhere($or1);
}
if (isset($options['timerange']['end']) && $options['timerange']['end'] instanceof DateTimeInterface) {
$or2 = $outerQuery->expr()->orX();
$or2->add($outerQuery->expr()->gt('firstoccurence',
$outerQuery->createNamedParameter($options['timerange']['start']->getTimeStamp())));
$or2->add($outerQuery->expr()->isNull('firstoccurence'));
$outerQuery->andWhere($or2);
}
} else {
Comment on lines +1851 to +1868
Copy link
Contributor Author

@miaulalala miaulalala Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following problem presents itself when adding VTODO support for combined searches where [options => [ types => [VEVENT, VTODO]], [timerange => [start => 123456]]]

A VTODO will never have a firstoccurence/lastoccurence set (db entry null), but a VEVENT timerange filter will always look for it, thus ignoring null values.

My changes would allow the query to do an AND WHERE(firtsoccurence = 12346 OR firstoccurence IS NULL) when the VTODO type is passed.

(The filters need adjusting too further below)

if (isset($options['timerange']['start']) && $options['timerange']['start'] instanceof DateTimeInterface) {
$outerQuery->andWhere($outerQuery->expr()->gt('lastoccurence',
$outerQuery->createNamedParameter($options['timerange']['start']->getTimeStamp())));
Expand All @@ -1863,16 +1881,33 @@ public function search(array $calendarInfo, $pattern, array $searchProperties,
}

if (!empty($options['types'])) {
$or = $outerQuery->expr()->orX();
$or3 = $outerQuery->expr()->orX();
foreach ($options['types'] as $type) {
$or->add($outerQuery->expr()->eq('componenttype',
$or3->add($outerQuery->expr()->eq('componenttype',
$outerQuery->createNamedParameter($type)));
}
$outerQuery->andWhere($or);
$outerQuery->andWhere($or3);
}

$outerQuery->andWhere($outerQuery->expr()->in('c.id', $outerQuery->createFunction($innerQuery->getSQL())));

$allowedSorts = ['id','calendarid', 'lastmodified', 'firstoccurence', 'lastoccurence'];
if(!empty($options['sort_asc'])) {
foreach($options['sort_asc'] as $sort) {
if(!in_array($sort, $allowedSorts)) {
continue;
}
$outerQuery->orderBy($sort, 'ASC');
Copy link
Member

@tcitworld tcitworld Feb 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought (non-blocking): Should we validate or restrict the value of $sort before using it (against valid column names)? In order not to explode at runtime.

(same applies just below)

}
}
if(!empty($options['sort_desc'])) {
foreach($options['sort_desc'] as $sort) {
if(!in_array($sort, $allowedSorts)) {
continue;
}
$outerQuery->orderBy($sort, 'DESC');
}
}
if ($offset) {
$outerQuery->setFirstResult($offset);
}
Expand Down
14 changes: 13 additions & 1 deletion apps/dav/lib/CalDAV/CalendarImpl.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,16 +32,18 @@
use OCA\DAV\CalDAV\InvitationResponse\InvitationResponseServer;
use OCP\Calendar\Exceptions\CalendarException;
use OCP\Calendar\ICreateFromString;
use OCP\Calendar\IGetTimezone;
use OCP\Calendar\IHandleImipMessage;
use OCP\Constants;
use Sabre\DAV\Exception\Conflict;
use Sabre\VObject\Component\VCalendar;
use Sabre\VObject\Component\VEvent;
use Sabre\VObject\Component\VTimeZone;
use Sabre\VObject\ITip\Message;
use Sabre\VObject\Reader;
use function Sabre\Uri\split as uriSplit;

class CalendarImpl implements ICreateFromString, IHandleImipMessage {
class CalendarImpl implements ICreateFromString, IHandleImipMessage, IGetTimezone {
private CalDavBackend $backend;
private Calendar $calendar;
/** @var array<string, mixed> */
Expand Down Expand Up @@ -235,4 +237,14 @@ public function handleIMipMessage(string $name, string $calendarData): void {
public function getInvitationResponseServer(): InvitationResponseServer {
return new InvitationResponseServer(false);
}

public function getCalendarTimezoneString(): ?string {
Copy link
Member

@tcitworld tcitworld Feb 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: There is a very similar block of code in #36192, it would be nice to reuse it somehow. :)

$vTimezoneString = $this->calendarInfo['{urn:ietf:params:xml:ns:caldav}calendar-timezone'];
if(empty($vTimezoneString)) {
return null;
}
$cal = Reader::read($vTimezoneString);
Copy link
Member

@tcitworld tcitworld Feb 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: I would catch exceptions here and return null properly. I'm not sure server validates this property, so a client could put garbage inside it.

$components = $cal->VTIMEZONE;
return $components->TZID->getValue();

Check failure

Code scanning / Psalm

UndefinedPropertyFetch

Instance property Sabre\VObject\Property::$TZID is not defined

Check notice

Code scanning / Psalm

PossiblyNullPropertyFetch

Cannot get property on possibly null variable $components of type Sabre\VObject\Property|null

Check notice

Code scanning / Psalm

PossiblyNullReference

Cannot call method getValue on possibly null value
}
}
41 changes: 41 additions & 0 deletions lib/public/Calendar/IGetTimezone.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
<?php
/*
* *
* * dav App
* *
* * @copyright 2023 Anna Larch <[email protected]>
* *
* * @author Anna Larch <[email protected]>
* *
* * This library is free software; you can redistribute it and/or
* * modify it under the terms of the GNU AFFERO GENERAL PUBLIC LICENSE
* * License as published by the Free Software Foundation; either
* * version 3 of the License, or any later version.
* *
* * This library is distributed in the hope that it will be useful,
* * but WITHOUT ANY WARRANTY; without even the implied warranty of
* * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* * GNU AFFERO GENERAL PUBLIC LICENSE for more details.
* *
* * You should have received a copy of the GNU Affero General Public
* * License along with this library. If not, see <http://www.gnu.org/licenses/>.
* *
*
*/
namespace OCP\Calendar;

use OCP\Calendar\Exceptions\CalendarException;
use OCP\Calendar\ICalendar;
use Sabre\VObject\Component\VTimeZone;

interface IGetTimezone extends ICalendar {
Copy link
Member

@ChristophWurst ChristophWurst Feb 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the purpose of this new interface and public API? it's not programmed against anywhere, is it?

Copy link
Contributor Author

@miaulalala miaulalala Feb 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CalendarImpl implements it. But Thomas suggested using the new Timezone stuff in #36192 so I will look into that one

Copy link
Member

@ChristophWurst ChristophWurst Feb 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It implements it but there is no code that uses it. Hence there is no need for a public API here.

Copy link
Contributor Author

@miaulalala miaulalala Feb 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/nextcloud/calendar/pull/4969/files

/**
* Get the calendar timezone as a string
* i.e. Europe/Vienna
* as set in the VTIMEZONE->TZID for a calendar
*
* @since 26.0.0
*/
public function getCalendarTimezoneString(): ?string;
Copy link
Member

@tcitworld tcitworld Feb 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: I would name it getCalendarTimezoneID

Copy link
Contributor Author

@miaulalala miaulalala Feb 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point

}