Provide links to calendar in event creation/update activities #19039
Conversation
tcitworld
force-pushed
the
dav-activity-provide-links-to-calendar
branch
from
71abc1c
to
4b908a4
Compare
There was a problem hiding this comment.
I'm blocking, to put this on hold for now. I'm worried I have to change the url format in order to fix nextcloud/calendar#1894. Will look into that issue today.
Never mind, it's yet again related to unencoded group names
|
@georgehrke yay or nay? |
|
@tcitworld can you remove the version check as suggested by @nickvergessen. Then this is good to go |
tcitworld
force-pushed
the
dav-activity-provide-links-to-calendar
branch
from
4b908a4
to
9ee897d
Compare
|
Removed the version check, properly injected dependencies, moved |
|
Activity PR is merged |
tcitworld
force-pushed
the
dav-activity-provide-links-to-calendar
branch
from
9ee897d
to
ef1b510
Compare
| @@ -435,29 +435,41 @@ public function onTouchCalendarObject($action, array $calendarData, array $share | |||
| $users = $this->getUsersForShares($shares); | |||
| $users[] = $owner; | |||
|
|
|||
| foreach ($users as $user) { | |||
| // Users for share can return the owner itself if the calendar is published | |||
| foreach (array_unique($users) as $user) { | |||
There was a problem hiding this comment.
getUsersForShares potentially returning the owner itself because of public links for calendar should be fixed somewhere else, this is a temporary fix. We need a proper nc: property to indicate that. cc @georgehrke
| $link = [ | ||
| 'view' => 'dayGridMonth', | ||
| 'timeRange' => $timeRange->format('Y-m-d'), | ||
| 'mode' => 'sidebar', |
There was a problem hiding this comment.
This could get user's skipPopover setting to know if we use the popover or the sidebar, do you want that?
There was a problem hiding this comment.
We actually have a special route for this, but it's also hardcoded to sidebar at the moment.
https://github.com/nextcloud/calendar/blob/master/src/router.js#L102
I would just keep it as is and improve later on :)
There was a problem hiding this comment.
If you use that route, we could actually get rid of firstoccurrence
There was a problem hiding this comment.
I'm using /dayGridMonth/now/edit/sidebar/$objectId/next then.
|
Slighty off-topic, it's kinda sad we can't properly detect event being renamed, it makes you think you updated two different events when you just renamed one twice. |
| * @param array $eventData | ||
| * @return array | ||
| */ | ||
| protected function generateObjectParameter(array $eventData) { |
There was a problem hiding this comment.
This was in the base because it's also used in Todo.php and it makes it more obvious that it is shared.
But I guess it's fine eitherway.
There was a problem hiding this comment.
Yeah, my reasoning was that Calendar.php doesn't need it.
| 'objectId' => $objectId, | ||
| 'recurrenceId' => $linkData['firstoccurence'] | ||
| ]; | ||
| $params['link'] = $this->url->linkToRouteAbsolute('calendar.view.indexview.timerange.edit', $link); |
There was a problem hiding this comment.
this will 💥 when calendar is disabled,
so maybe we return the params without the link (since it's optional anyway) in the exception catching?
There was a problem hiding this comment.
So just wrap in AppManager::isEnabledForUser?
There was a problem hiding this comment.
Not sure I follow here, we set the link inside the AppManager::isEnabledForUser check already, and params without the link will be return at the end properly.
There was a problem hiding this comment.
Ah, didnt see it as it was all at the end of this long if
Signed-off-by: Thomas Citharel <tcit@tcit.fr>
tcitworld
force-pushed
the
dav-activity-provide-links-to-calendar
branch
from
ef1b510
to
deb2ea9
Compare
Since Calendar 2.0 we have dedicated routes so we can link to events directly from the Activity app.
See also nextcloud/activity#420 for proper formatting in the Activity app.
(The last item has also a link to the event)