-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
add activity logging for favorites in dav #48612
base: master
Are you sure you want to change the base?
Conversation
cde368c
to
e747f11
Compare
3a2b521
to
fbd6419
Compare
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.
Would it not be cleaner and make more sense to emit just the event and handle the activity in an event listener?
Then only one place for the activity is required and one would not mix different apps (files app activity in dav app).
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.
I don't get why this is need. Isn't TagService
already doing that?
server/apps/files/lib/Service/TagService.php
Lines 101 to 131 in ff9ea47
protected function addActivity($addToFavorite, $fileId, $path) { | |
$user = $this->userSession->getUser(); | |
if (!$user instanceof IUser) { | |
return; | |
} | |
if ($addToFavorite) { | |
$event = new NodeAddedToFavorite($user, $fileId, $path); | |
} else { | |
$event = new NodeRemovedFromFavorite($user, $fileId, $path); | |
} | |
$this->dispatcher->dispatchTyped($event); | |
$event = $this->activityManager->generateEvent(); | |
try { | |
$event->setApp('files') | |
->setObject('files', $fileId, $path) | |
->setType('favorite') | |
->setAuthor($user->getUID()) | |
->setAffectedUser($user->getUID()) | |
->setTimestamp(time()) | |
->setSubject( | |
$addToFavorite ? FavoriteProvider::SUBJECT_ADDED : FavoriteProvider::SUBJECT_REMOVED, | |
['id' => $fileId, 'path' => $path] | |
); | |
$this->activityManager->publish($event); | |
} catch (\InvalidArgumentException $e) { | |
} catch (\BadMethodCallException $e) { | |
} | |
} | |
} |
EDIT: reading the issue
935e3b1
to
e61f394
Compare
739acab
to
6f98dd1
Compare
6f98dd1
to
205809a
Compare
205809a
to
f6dd0c2
Compare
6eca638
to
e96dc20
Compare
Signed-off-by: grnd-alt <[email protected]>
e96dc20
to
ee093f5
Compare
} else { | ||
$this->getTagger()->unTag($node->getId(), self::TAG_FAVORITE); | ||
$this->eventDispatcher->dispatchTyped(new NodeRemovedFromFavorite($this->userSession->getUser(), $node->getId(), $path)); |
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.
Wouldn't it make sense to dispatch the events in the tagAs and unTag methods in
Line 505 in 77114fb
public function tagAs($objid, $tag) { |
I cannot judge if there are cases where we don't want that, but if not we could save duplicate calls in https://github.com/nextcloud/server/pull/48612/files#diff-d2d90fa28b8b1a36f0664fa4a25a0753a765a824d0bd2524e7457d5be7a2d7a5R62 and potentially forgetting about code paths that should also store an activity entry.
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.
I think we always want activities for tags, no?
So I agree with your comment :)
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.
Looks good, left one comment where I'm not familiar enough with the code paths to judge
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.
Looks good!
Also Julius suggestion makes sense.
Signed-off-by: grnd-alt <[email protected]>
dc5e8fd
to
849a28a
Compare
Summary
Dav api did not log favorite added/removed events.
Checklist