-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Better Calendar Widget Data #36620
Better Calendar Widget Data #36620
Conversation
Signed-off-by: Anna Larch <anna@nextcloud.com>
} | ||
$cal = Reader::read($vTimezoneString); | ||
$components = $cal->VTIMEZONE; | ||
return $components->TZID->getValue(); |
Check failure
Code scanning / Psalm
UndefinedPropertyFetch
} | ||
$cal = Reader::read($vTimezoneString); | ||
$components = $cal->VTIMEZONE; | ||
return $components->TZID->getValue(); |
Check notice
Code scanning / Psalm
PossiblyNullPropertyFetch
} | ||
$cal = Reader::read($vTimezoneString); | ||
$components = $cal->VTIMEZONE; | ||
return $components->TZID->getValue(); |
Check notice
Code scanning / Psalm
PossiblyNullReference
* | ||
* @since 26.0.0 | ||
*/ | ||
public function getCalendarTimezoneString(): ?string; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point
if(empty($vTimezoneString)) { | ||
return null; | ||
} | ||
$cal = Reader::read($vTimezoneString); |
There was a problem hiding this comment.
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.
@@ -1873,6 +1874,16 @@ public function search(array $calendarInfo, $pattern, array $searchProperties, | |||
|
|||
$outerQuery->andWhere($outerQuery->expr()->in('c.id', $outerQuery->createFunction($innerQuery->getSQL()))); | |||
|
|||
if(!empty($options['sort_asc'])) { | |||
foreach($options['sort_asc'] as $sort) { | |||
$outerQuery->orderBy($sort, 'ASC'); |
There was a problem hiding this comment.
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)
@@ -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), | |||
|
There was a problem hiding this comment.
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
@@ -235,4 +237,14 @@ public function handleIMipMessage(string $name, string $calendarData): void { | |||
public function getInvitationResponseServer(): InvitationResponseServer { | |||
return new InvitationResponseServer(false); | |||
} | |||
|
|||
public function getCalendarTimezoneString(): ?string { |
There was a problem hiding this comment.
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. :)
use OCP\Calendar\ICalendar; | ||
use Sabre\VObject\Component\VTimeZone; | ||
|
||
interface IGetTimezone extends ICalendar { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 { |
There was a problem hiding this comment.
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)
Superceded by #39937 |
Signed-off-by: Anna Larch anna@nextcloud.com
Summary
TODO
Checklist