add activity logging for favorites in dav #48612

grnd-alt · 2024-10-08T10:49:35Z

Resolves: Missing activity for favorites via WebDAV #26393

Summary

Dav api did not log favorite added/removed events.

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Screenshots before/after for front-end changes
Documentation (manuals or wiki) has been updated or is not required
Backports requested where applicable (ex: critical bugfixes)

susnux

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).

artonge

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

apps/dav/lib/Server.php

apps/dav/lib/Connector/Sabre/TagsPlugin.php

lib/public/ITags.php

juliusknorr · 2024-11-18T09:32:01Z

apps/dav/lib/Connector/Sabre/TagsPlugin.php

+				$this->eventDispatcher->dispatchTyped(new NodeAddedToFavorite($this->userSession->getUser(), $node->getId(), $path));
 			} else {
 				$this->getTagger()->unTag($node->getId(), self::TAG_FAVORITE);
+				$this->eventDispatcher->dispatchTyped(new NodeRemovedFromFavorite($this->userSession->getUser(), $node->getId(), $path));


Wouldn't it make sense to dispatch the events in the tagAs and unTag methods in

server/lib/private/Tags.php

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.

I think we always want activities for tags, no?
So I agree with your comment :)

juliusknorr

Looks good, left one comment where I'm not familiar enough with the code paths to judge

susnux

Looks good!
Also Julius suggestion makes sense.

juliusknorr · 2024-11-21T07:47:46Z

Failure looks related


There was 1 failure:

1) OCA\Files\Tests\Service\TagServiceTest::testFavoriteActivity
Expectation failed for method name is "dispatchTyped" when invoked 2 time(s).
Method was expected to be called 2 times, actually called 0 times.

Signed-off-by: grnd-alt <salimbelakkaf@outlook.de>

Signed-off-by: grnd-alt <github@belakkaf.net>

grnd-alt · 2024-12-12T14:14:25Z

@susnux @juliusknorr I've added a commit after your reviews, idk if I should merge without confirmation from one of you

The `$path` argument was added in #48612, but was never actually used by the callers. The path was therefore missing in the favorite/unfavorite events, which lead to a broken activity information. I also added a fallback to handle `addToFavorites` and `removeFromFavorites`, which are part of a public API, and are calling `tagAs` and `untag` without `$path`. Fix https://github.com/nextcloud/server/issues/55118 Signed-off-by: Louis Chemineau <louis@chmn.me>

The `$path` argument was added in #48612, but was never actually used by the callers. The path was therefore missing in the favorite/unfavorite events, which lead to a broken activity information. I also added a fallback to handle `addToFavorites` and `removeFromFavorites`, which are part of a public API, and are calling `tagAs` and `untag` without `$path`. Fix nextcloud/activity#2134 Signed-off-by: Louis Chemineau <louis@chmn.me>

The `$path` argument was added in #48612, but was never actually used by the callers. The path was therefore missing in the favorite/unfavorite events, which lead to a broken activity information. I also added a fallback to handle `addToFavorites` and `removeFromFavorites`, which are part of a public API, and are calling `tagAs` and `untag` without `$path`. Fix nextcloud/activity#2134 Signed-off-by: Louis Chemineau <louis@chmn.me> [skip ci]

The `$path` argument was added in #48612, but was never actually used by the callers. The path was therefore missing in the favorite/unfavorite events, which lead to a broken activity information. I also added a fallback to handle `addToFavorites` and `removeFromFavorites`, which are part of a public API, and are calling `tagAs` and `untag` without `$path`. Fix nextcloud/activity#2134 Signed-off-by: Louis Chemineau <louis@chmn.me>

grnd-alt requested review from joshtrichards, juliusknorr, susnux and szaimen October 8, 2024 10:50

grnd-alt force-pushed the fix/activity-log-for-favorites-in-dav branch from cde368c to e747f11 Compare October 8, 2024 10:56

szaimen requested review from artonge and removed request for szaimen October 8, 2024 11:32

szaimen added enhancement 3. to review Waiting for reviews labels Oct 8, 2024

szaimen added this to the Nextcloud 31 milestone Oct 8, 2024

grnd-alt force-pushed the fix/activity-log-for-favorites-in-dav branch 3 times, most recently from 3a2b521 to fbd6419 Compare October 8, 2024 12:55

susnux reviewed Oct 8, 2024

View reviewed changes

artonge reviewed Oct 8, 2024

View reviewed changes

apps/dav/lib/Server.php Outdated Show resolved Hide resolved

artonge reviewed Oct 8, 2024

View reviewed changes

apps/dav/lib/Connector/Sabre/TagsPlugin.php Show resolved Hide resolved

grnd-alt force-pushed the fix/activity-log-for-favorites-in-dav branch 4 times, most recently from 935e3b1 to e61f394 Compare October 14, 2024 08:29

grnd-alt self-assigned this Oct 15, 2024

grnd-alt force-pushed the fix/activity-log-for-favorites-in-dav branch 7 times, most recently from 739acab to 6f98dd1 Compare October 22, 2024 14:18

artonge reviewed Oct 22, 2024

View reviewed changes

lib/public/ITags.php Outdated Show resolved Hide resolved

grnd-alt force-pushed the fix/activity-log-for-favorites-in-dav branch from 6f98dd1 to 205809a Compare October 22, 2024 15:53

grnd-alt requested review from artonge and susnux October 29, 2024 09:12

grnd-alt force-pushed the fix/activity-log-for-favorites-in-dav branch from e96dc20 to ee093f5 Compare November 7, 2024 10:01

juliusknorr reviewed Nov 18, 2024

View reviewed changes

juliusknorr approved these changes Nov 18, 2024

View reviewed changes

susnux approved these changes Nov 18, 2024

View reviewed changes

grnd-alt force-pushed the fix/activity-log-for-favorites-in-dav branch from dc5e8fd to 849a28a Compare November 19, 2024 11:05

grnd-alt added 2 commits December 3, 2024 20:56

fix(dav): add activity logging for favorites in dav

2d02d83

Signed-off-by: grnd-alt <salimbelakkaf@outlook.de>

refactor(tags): move favorite event dispatching to tags.php

8d953ae

Signed-off-by: grnd-alt <salimbelakkaf@outlook.de>

grnd-alt force-pushed the fix/activity-log-for-favorites-in-dav branch from 849a28a to 8d953ae Compare December 3, 2024 19:56

fix(tests): remove redundant unit test for activity

eb61a3a

Signed-off-by: grnd-alt <github@belakkaf.net>

grnd-alt requested review from juliusknorr and susnux December 5, 2024 12:50

juliusknorr merged commit cba556d into master Dec 12, 2024
188 checks passed

juliusknorr deleted the fix/activity-log-for-favorites-in-dav branch December 12, 2024 14:18

skjnldsv mentioned this pull request Jan 7, 2025

31.0.0 beta 1 #50066

Merged

artonge mentioned this pull request Sep 15, 2025

fix: Dispatch favorite event with an actual path #55122

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

add activity logging for favorites in dav #48612

add activity logging for favorites in dav #48612

grnd-alt commented Oct 8, 2024 •

edited

Loading

Uh oh!

susnux left a comment

Uh oh!

artonge left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

juliusknorr Nov 18, 2024

Uh oh!

susnux Nov 18, 2024

Uh oh!

juliusknorr left a comment

Uh oh!

susnux left a comment

Uh oh!

juliusknorr commented Nov 21, 2024

Uh oh!

grnd-alt commented Dec 12, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

	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) {
	}
	}
	}

Uh oh!

add activity logging for favorites in dav #48612

add activity logging for favorites in dav #48612

Conversation

grnd-alt commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

susnux left a comment

Choose a reason for hiding this comment

Uh oh!

artonge left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

juliusknorr Nov 18, 2024

Choose a reason for hiding this comment

Uh oh!

susnux Nov 18, 2024

Choose a reason for hiding this comment

Uh oh!

juliusknorr left a comment

Choose a reason for hiding this comment

Uh oh!

susnux left a comment

Choose a reason for hiding this comment

Uh oh!

juliusknorr commented Nov 21, 2024

Uh oh!

grnd-alt commented Dec 12, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

grnd-alt commented Oct 8, 2024 •

edited

Loading

artonge left a comment •

edited

Loading