From a4e15e2db1da787e489bd3019b67b76861dda438 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 12 Aug 2020 01:43:53 +0200 Subject: [PATCH] merge file activity settings into a single 'favorite changed' item Signed-off-by: Robin Appelman --- lib/BackgroundJob/RemoteActivity.php | 2 +- lib/Consumer.php | 11 +- lib/Controller/RemoteActivityController.php | 2 +- lib/Extension/Files.php | 3 +- lib/FilesHooks.php | 218 ++++++++++---------- templates/settings/form.php | 11 - tests/ConsumerTest.php | 2 +- tests/FilesHooksTest.php | 154 ++++++++------ 8 files changed, 206 insertions(+), 197 deletions(-) diff --git a/lib/BackgroundJob/RemoteActivity.php b/lib/BackgroundJob/RemoteActivity.php index c1e5a5b9e..6d9852bca 100644 --- a/lib/BackgroundJob/RemoteActivity.php +++ b/lib/BackgroundJob/RemoteActivity.php @@ -117,7 +117,7 @@ protected function translateType($internalType, $secondPath) { case Files::TYPE_SHARE_CREATED: case Files::TYPE_SHARE_RESTORED: return 'Create'; - case Files::TYPE_SHARE_CHANGED: + case Files::TYPE_FILE_CHANGED: if ($secondPath !== '') { return 'Move'; } diff --git a/lib/Consumer.php b/lib/Consumer.php index b047beb2d..a60528540 100644 --- a/lib/Consumer.php +++ b/lib/Consumer.php @@ -67,16 +67,13 @@ public function receive(IEvent $event) { $emailSetting = $this->userSettings->getUserSetting($event->getAffectedUser(), 'email', $event->getType()); $emailSetting = ($emailSetting) ? $this->userSettings->getUserSetting($event->getAffectedUser(), 'setting', 'batchtime') : false; - // User is not the author or wants to see their own actions - $createStream = !$selfAction || $this->userSettings->getUserSetting($event->getAffectedUser(), 'setting', 'self'); - // Add activity to stream - if ($createStream) { + if (!$selfAction) { $activityId = $this->data->send($event); + } - if (!$selfAction && $notificationSetting && $activityId) { - $this->notificationGenerator->sendNotificationForEvent($event, $activityId); - } + if (!$selfAction && $notificationSetting && $activityId) { + $this->notificationGenerator->sendNotificationForEvent($event, $activityId); } // User is not the author or wants to see their own actions diff --git a/lib/Controller/RemoteActivityController.php b/lib/Controller/RemoteActivityController.php index 890aa20ea..e26aebc17 100644 --- a/lib/Controller/RemoteActivityController.php +++ b/lib/Controller/RemoteActivityController.php @@ -228,7 +228,7 @@ protected function translateType($type) { return Files::TYPE_SHARE_CREATED; case 'Move': case 'Update': - return Files::TYPE_SHARE_CHANGED; + return Files::TYPE_FILE_CHANGED; case 'Delete': return Files::TYPE_SHARE_DELETED; } diff --git a/lib/Extension/Files.php b/lib/Extension/Files.php index 28798a410..61705c78f 100644 --- a/lib/Extension/Files.php +++ b/lib/Extension/Files.php @@ -24,7 +24,8 @@ class Files { const TYPE_SHARE_CREATED = 'file_created'; - const TYPE_SHARE_CHANGED = 'file_changed'; + const TYPE_FILE_CHANGED = 'file_changed'; + const TYPE_FAVORITE_CHANGED = 'file_favorite_changed'; const TYPE_SHARE_DELETED = 'file_deleted'; const TYPE_SHARE_RESTORED = 'file_restored'; } diff --git a/lib/FilesHooks.php b/lib/FilesHooks.php index a1b061acd..ab4c8a86e 100755 --- a/lib/FilesHooks.php +++ b/lib/FilesHooks.php @@ -26,6 +26,7 @@ use OC\Files\Filesystem; use OC\Files\View; +use OC\Tags; use OCA\Activity\BackgroundJob\RemoteActivity; use OCA\Activity\Extension\Files; use OCA\Activity\Extension\Files_Sharing; @@ -42,6 +43,7 @@ use OCP\IGroup; use OCP\IGroupManager; use OCP\ILogger; +use OCP\ITagManager; use OCP\IURLGenerator; use OCP\IUser; use OCP\Share; @@ -103,21 +105,25 @@ class FilesHooks { protected $oldParentId; /** @var NotificationGenerator */ protected $notificationGenerator; - - public function __construct(IManager $manager, - Data $activityData, - UserSettings $userSettings, - IGroupManager $groupManager, - View $view, - IRootFolder $rootFolder, - IShareHelper $shareHelper, - IDBConnection $connection, - IURLGenerator $urlGenerator, - ILogger $logger, - CurrentUser $currentUser, - IUserMountCache $userMountCache, - IConfig $config, - NotificationGenerator $notificationGenerator + /** @var Tags */ + protected $tagManager; + + public function __construct( + IManager $manager, + Data $activityData, + UserSettings $userSettings, + IGroupManager $groupManager, + View $view, + IRootFolder $rootFolder, + IShareHelper $shareHelper, + IDBConnection $connection, + IURLGenerator $urlGenerator, + ILogger $logger, + CurrentUser $currentUser, + IUserMountCache $userMountCache, + IConfig $config, + NotificationGenerator $notificationGenerator, + ITagManager $tagManager ) { $this->manager = $manager; $this->activityData = $activityData; @@ -133,10 +139,12 @@ public function __construct(IManager $manager, $this->userMountCache = $userMountCache; $this->config = $config; $this->notificationGenerator = $notificationGenerator; + $this->tagManager = $tagManager->load('files'); } /** * Store the create hook events + * * @param string $path Path of the file that has been created */ public function fileCreate($path) { @@ -145,45 +153,57 @@ public function fileCreate($path) { } if ($this->currentUser->getUserIdentifier() !== '') { - $this->addNotificationsForFileAction($path, Files::TYPE_SHARE_CREATED, 'created_self', 'created_by'); + $this->addNotificationsForFileAction($path, 'created_self', 'created_by'); } else { - $this->addNotificationsForFileAction($path, Files::TYPE_SHARE_CREATED, '', 'created_public'); + $this->addNotificationsForFileAction($path, '', 'created_public'); } } /** * Store the update hook events + * * @param string $path Path of the file that has been modified */ public function fileUpdate($path) { - $this->addNotificationsForFileAction($path, Files::TYPE_SHARE_CHANGED, 'changed_self', 'changed_by'); + $this->addNotificationsForFileAction($path, 'changed_self', 'changed_by'); } /** * Store the delete hook events + * * @param string $path Path of the file that has been deleted */ public function fileDelete($path) { - $this->addNotificationsForFileAction($path, Files::TYPE_SHARE_DELETED, 'deleted_self', 'deleted_by'); + $this->addNotificationsForFileAction($path, 'deleted_self', 'deleted_by'); } /** * Store the restore hook events + * * @param string $path Path of the file that has been restored */ public function fileRestore($path) { - $this->addNotificationsForFileAction($path, Files::TYPE_SHARE_RESTORED, 'restored_self', 'restored_by'); + $this->addNotificationsForFileAction($path, 'restored_self', 'restored_by'); + } + + private function getFileChangeActivitySettings(int $fileId, array $users): array { + $favoriteUsers = $this->tagManager->getUsersFavoritingObject($fileId); + $users = array_intersect($users, $favoriteUsers); + $filteredEmailUsers = $this->userSettings->filterUsersBySetting($users, 'email', Files::TYPE_FAVORITE_CHANGED); + $filteredNotificationUsers = $this->userSettings->filterUsersBySetting($users, 'notification', Files::TYPE_FAVORITE_CHANGED); + + return [$filteredEmailUsers, $filteredNotificationUsers]; } /** * Creates the entries for file actions on $file_path * - * @param string $filePath The file that is being changed - * @param int $activityType The activity type - * @param string $subject The subject for the actor - * @param string $subjectBy The subject for other users (with "by $actor") + * @param string $filePath The file that is being changed + * @param int $activityType The activity type + * @param string $subject The subject for the actor + * @param string $subjectBy The subject for other users (with "by $actor") */ - protected function addNotificationsForFileAction($filePath, $activityType, $subject, $subjectBy) { + protected function addNotificationsForFileAction($filePath, $subject, $subjectBy) { // Do not add activities for .part-files if (substr($filePath, -5) === '.part') { return; @@ -197,7 +217,7 @@ protected function addNotificationsForFileAction($filePath, $activityType, $subj $accessList = $this->getUserPathsFromPath($filePath, $uidOwner); - $this->generateRemoteActivity($accessList['remotes'], $activityType, time(), $this->currentUser->getCloudId(), $accessList['ownerPath']); + $this->generateRemoteActivity($accessList['remotes'], Files::TYPE_FILE_CHANGED, time(), $this->currentUser->getCloudId(), $accessList['ownerPath']); if ($this->config->getSystemValueBool('activity_use_cached_mountpoints', false)) { $mountsForFile = $this->userMountCache->getMountsForFileId($fileId); @@ -212,14 +232,10 @@ protected function addNotificationsForFileAction($filePath, $activityType, $subj $affectedUsers = $accessList['users']; } - $filteredStreamUsers = $this->userSettings->filterUsersBySetting(array_keys($affectedUsers), 'stream', $activityType); - $filteredEmailUsers = $this->userSettings->filterUsersBySetting(array_keys($affectedUsers), 'email', $activityType); + [$filteredEmailUsers, $filteredNotificationUsers] = $this->getFileChangeActivitySettings($fileId, array_keys($affectedUsers)); foreach ($affectedUsers as $user => $path) { - $user = (string) $user; - if (empty($filteredStreamUsers[$user]) && empty($filteredEmailUsers[$user])) { - continue; - } + $user = (string)$user; if ($user === $this->currentUser->getUID()) { $userSubject = $subject; @@ -232,9 +248,9 @@ protected function addNotificationsForFileAction($filePath, $activityType, $subj $this->addNotificationsForUser( $user, $userSubject, $userParams, $fileId, $path, true, - !empty($filteredStreamUsers[$user]), $filteredEmailUsers[$user] ?? false, - $activityType + $filteredNotificationUsers[$user] ?? false, + Files::TYPE_FILE_CHANGED ); } } @@ -383,21 +399,17 @@ protected function fileRenaming($oldPath, $newPath) { $renameRemotes = []; foreach ($accessList['remotes'] as $remote => $info) { $renameRemotes[$remote] = [ - 'token' => $info['token'], - 'node_path' => substr($newPath, strlen($info['node_path'])), + 'token' => $info['token'], + 'node_path' => substr($newPath, strlen($info['node_path'])), 'second_path' => substr($oldPath, strlen($info['node_path'])), ]; } - $this->generateRemoteActivity($renameRemotes, Files::TYPE_SHARE_CHANGED, time(), $this->currentUser->getCloudId()); + $this->generateRemoteActivity($renameRemotes, Files::TYPE_FILE_CHANGED, time(), $this->currentUser->getCloudId()); $affectedUsers = $accessList['users']; - $filteredStreamUsers = $this->userSettings->filterUsersBySetting(array_keys($affectedUsers), 'stream', Files::TYPE_SHARE_CHANGED); - $filteredEmailUsers = $this->userSettings->filterUsersBySetting(array_keys($affectedUsers), 'email', Files::TYPE_SHARE_CHANGED); + [$filteredEmailUsers, $filteredNotificationUsers] = $this->getFileChangeActivitySettings($fileId, array_keys($affectedUsers)); foreach ($affectedUsers as $user => $path) { - if (empty($filteredStreamUsers[$user]) && empty($filteredEmailUsers[$user])) { - continue; - } if ($user === $this->currentUser->getUID()) { $userSubject = 'renamed_self'; @@ -417,9 +429,9 @@ protected function fileRenaming($oldPath, $newPath) { $this->addNotificationsForUser( $user, $userSubject, $userParams, $fileId, $path . '/' . $fileName, true, - !empty($filteredStreamUsers[$user]), $filteredEmailUsers[$user] ?? false, - Files::TYPE_SHARE_CHANGED + $filteredNotificationUsers[$user] ?? false, + Files::TYPE_FILE_CHANGED ); } } @@ -480,9 +492,9 @@ protected function fileMoving($oldPath, $newPath) { } } - $this->generateRemoteActivity($deleteRemotes, Files::TYPE_SHARE_DELETED, time(), $this->currentUser->getCloudId()); - $this->generateRemoteActivity($addRemotes, Files::TYPE_SHARE_CREATED, time(), $this->currentUser->getCloudId()); - $this->generateRemoteActivity($moveRemotes, Files::TYPE_SHARE_CHANGED, time(), $this->currentUser->getCloudId()); + $this->generateRemoteActivity($deleteRemotes, Files::TYPE_FILE_CHANGED, time(), $this->currentUser->getCloudId()); + $this->generateRemoteActivity($addRemotes, Files::TYPE_FILE_CHANGED, time(), $this->currentUser->getCloudId()); + $this->generateRemoteActivity($moveRemotes, Files::TYPE_FILE_CHANGED, time(), $this->currentUser->getCloudId()); } /** @@ -496,14 +508,9 @@ protected function generateDeleteActivities($users, $pathMap, $fileId, $oldFileN return; } - $filteredStreamUsers = $this->userSettings->filterUsersBySetting($users, 'stream', Files::TYPE_SHARE_DELETED); - $filteredEmailUsers = $this->userSettings->filterUsersBySetting($users, 'email', Files::TYPE_SHARE_DELETED); + [$filteredEmailUsers, $filteredNotificationUsers] = $this->getFileChangeActivitySettings($fileId, $users); foreach ($users as $user) { - if (empty($filteredStreamUsers[$user]) && empty($filteredEmailUsers[$user])) { - continue; - } - $path = $pathMap[$user]; if ($user === $this->currentUser->getUID()) { @@ -517,9 +524,9 @@ protected function generateDeleteActivities($users, $pathMap, $fileId, $oldFileN $this->addNotificationsForUser( $user, $userSubject, $userParams, $fileId, $path . '/' . $oldFileName, true, - !empty($filteredStreamUsers[$user]), $filteredEmailUsers[$user] ?? false, - Files::TYPE_SHARE_DELETED + $filteredNotificationUsers[$user] ?? false, + Files::TYPE_FILE_CHANGED ); } } @@ -535,13 +542,9 @@ protected function generateAddActivities($users, $pathMap, $fileId, $fileName) { return; } - $filteredStreamUsers = $this->userSettings->filterUsersBySetting($users, 'stream', Files::TYPE_SHARE_CREATED); - $filteredEmailUsers = $this->userSettings->filterUsersBySetting($users, 'email', Files::TYPE_SHARE_CREATED); + [$filteredEmailUsers, $filteredNotificationUsers] = $this->getFileChangeActivitySettings($fileId, $users); foreach ($users as $user) { - if (empty($filteredStreamUsers[$user]) && empty($filteredEmailUsers[$user])) { - continue; - } $path = $pathMap[$user]; @@ -556,9 +559,9 @@ protected function generateAddActivities($users, $pathMap, $fileId, $fileName) { $this->addNotificationsForUser( $user, $userSubject, $userParams, $fileId, $path . '/' . $fileName, true, - !empty($filteredStreamUsers[$user]), $filteredEmailUsers[$user] ?? false, - Files::TYPE_SHARE_CREATED + $filteredNotificationUsers[$user] ?? false, + Files::TYPE_FILE_CHANGED ); } } @@ -577,14 +580,9 @@ protected function generateMoveActivities($users, $beforePathMap, $afterPathMap, return; } - $filteredStreamUsers = $this->userSettings->filterUsersBySetting($users, 'stream', Files::TYPE_SHARE_CHANGED); - $filteredEmailUsers = $this->userSettings->filterUsersBySetting($users, 'email', Files::TYPE_SHARE_CHANGED); + [$filteredEmailUsers, $filteredNotificationUsers] = $this->getFileChangeActivitySettings($fileId, $users); foreach ($users as $user) { - if (empty($filteredStreamUsers[$user]) && empty($filteredEmailUsers[$user])) { - continue; - } - if ($oldFileName === $fileName) { $userParams = [[$newParentId => $afterPathMap[$user] . '/']]; } else { @@ -602,9 +600,9 @@ protected function generateMoveActivities($users, $beforePathMap, $afterPathMap, $this->addNotificationsForUser( $user, $userSubject, $userParams, $fileId, $afterPathMap[$user] . '/' . $fileName, true, - !empty($filteredStreamUsers[$user]), $filteredEmailUsers[$user] ?? false, - Files::TYPE_SHARE_CHANGED + $filteredNotificationUsers[$user] ?? false, + Files::TYPE_FILE_CHANGED ); } } @@ -673,7 +671,7 @@ protected function getSourcePathAndOwner($path) { // Probably a remote user, let's try to at least generate activities // for the current user if ($currentUser === null) { - [,$owner,] = explode('/', $view->getAbsolutePath($path), 3); + [, $owner,] = explode('/', $view->getAbsolutePath($path), 3); } else { $owner = $currentUser; } @@ -683,25 +681,26 @@ protected function getSourcePathAndOwner($path) { $info = Filesystem::getFileInfo($path); if ($info !== false) { $ownerView = new View('/' . $owner . '/files'); - $fileId = (int) $info['fileid']; + $fileId = (int)$info['fileid']; $path = $ownerView->getPath($fileId); } - return array($path, $owner, $fileId); + return [$path, $owner, $fileId]; } /** * Manage sharing events + * * @param array $params The hook params */ public function share($params) { if ($params['itemType'] === 'file' || $params['itemType'] === 'folder') { - if ((int) $params['shareType'] === Share::SHARE_TYPE_USER) { - $this->shareWithUser($params['shareWith'], (int) $params['fileSource'], $params['itemType'], $params['fileTarget']); - } else if ((int) $params['shareType'] === Share::SHARE_TYPE_GROUP) { - $this->shareWithGroup($params['shareWith'], (int) $params['fileSource'], $params['itemType'], $params['fileTarget'], (int) $params['id']); - } else if ((int) $params['shareType'] === Share::SHARE_TYPE_LINK) { - $this->shareByLink((int) $params['fileSource'], $params['itemType'], $params['uidOwner']); + if ((int)$params['shareType'] === Share::SHARE_TYPE_USER) { + $this->shareWithUser($params['shareWith'], (int)$params['fileSource'], $params['itemType'], $params['fileTarget']); + } else if ((int)$params['shareType'] === Share::SHARE_TYPE_GROUP) { + $this->shareWithGroup($params['shareWith'], (int)$params['fileSource'], $params['itemType'], $params['fileTarget'], (int)$params['id']); + } else if ((int)$params['shareType'] === Share::SHARE_TYPE_LINK) { + $this->shareByLink((int)$params['fileSource'], $params['itemType'], $params['uidOwner']); } } } @@ -724,9 +723,9 @@ protected function shareWithUser($shareWith, $fileSource, $itemType, $fileTarget // New shared user $this->addNotificationsForUser( $shareWith, 'shared_with_by', [[$fileSource => $fileTarget], $this->currentUser->getUserIdentifier()], - (int) $fileSource, $fileTarget, $itemType === 'file', - $this->userSettings->getUserSetting($shareWith, 'stream', Files_Sharing::TYPE_SHARED), - $this->userSettings->getUserSetting($shareWith, 'email', Files_Sharing::TYPE_SHARED) ? $this->userSettings->getUserSetting($shareWith, 'setting', 'batchtime') : false + (int)$fileSource, $fileTarget, $itemType === 'file', + $this->userSettings->getUserSetting($shareWith, 'email', Files_Sharing::TYPE_SHARED) ? $this->userSettings->getUserSetting($shareWith, 'setting', 'batchtime') : false, + $this->userSettings->getUserSetting($shareWith, 'notification', Files_Sharing::TYPE_SHARED) ); } @@ -781,14 +780,15 @@ protected function shareByLink($fileSource, $itemType, $linkOwner) { $this->addNotificationsForUser( $linkOwner, 'shared_link_self', [[$fileSource => $path]], - (int) $fileSource, $path, $itemType === 'file', - $this->userSettings->getUserSetting($linkOwner, 'stream', Files_Sharing::TYPE_SHARED), - $this->userSettings->getUserSetting($linkOwner, 'email', Files_Sharing::TYPE_SHARED) ? $this->userSettings->getUserSetting($linkOwner, 'setting', 'batchtime') : false + (int)$fileSource, $path, $itemType === 'file', + $this->userSettings->getUserSetting($linkOwner, 'email', Files_Sharing::TYPE_SHARED) ? $this->userSettings->getUserSetting($linkOwner, 'setting', 'batchtime') : false, + $this->userSettings->getUserSetting($linkOwner, 'notification', Files_Sharing::TYPE_SHARED) ); } /** * Manage unsharing events + * * @param IShare $share * @throws \OCP\Files\NotFoundException */ @@ -806,6 +806,7 @@ public function unShare(IShare $share) { /** * Manage unsharing a share from self only events + * * @param IShare $share * @throws \OCP\Files\NotFoundException */ @@ -853,8 +854,8 @@ protected function unshareFromUser(IShare $share) { $this->addNotificationsForUser( $share->getSharedWith(), $actionUser, [[$share->getNodeId() => $share->getTarget()], $this->currentUser->getUserIdentifier()], $share->getNodeId(), $share->getTarget(), $share->getNodeType() === 'file', - $this->userSettings->getUserSetting($share->getSharedWith(), 'stream', Files_Sharing::TYPE_SHARED), - $this->userSettings->getUserSetting($share->getSharedWith(), 'email', Files_Sharing::TYPE_SHARED) ? $this->userSettings->getUserSetting($share->getSharedWith(), 'setting', 'batchtime') : false + $this->userSettings->getUserSetting($share->getSharedWith(), 'email', Files_Sharing::TYPE_SHARED) ? $this->userSettings->getUserSetting($share->getSharedWith(), 'setting', 'batchtime') : false, + $this->userSettings->getUserSetting($share->getSharedWith(), 'notification', Files_Sharing::TYPE_SHARED) ); } @@ -949,8 +950,8 @@ protected function unshareLink(IShare $share) { $this->addNotificationsForUser( $owner, $actionSharer, [[$share->getNodeId() => $share->getTarget()]], $share->getNodeId(), $share->getTarget(), $share->getNodeType() === 'file', - $this->userSettings->getUserSetting($owner, 'stream', Files_Sharing::TYPE_SHARED), - $this->userSettings->getUserSetting($owner, 'email', Files_Sharing::TYPE_SHARED) ? $this->userSettings->getUserSetting($owner, 'setting', 'batchtime') : false + $this->userSettings->getUserSetting($owner, 'email', Files_Sharing::TYPE_SHARED) ? $this->userSettings->getUserSetting($owner, 'setting', 'batchtime') : false, + $this->userSettings->getUserSetting($owner, 'notification', Files_Sharing::TYPE_SHARED) ); if ($share->getSharedBy() !== $share->getShareOwner()) { @@ -958,8 +959,8 @@ protected function unshareLink(IShare $share) { $this->addNotificationsForUser( $owner, $actionOwner, [[$share->getNodeId() => $share->getTarget()], $share->getSharedBy()], $share->getNodeId(), $share->getTarget(), $share->getNodeType() === 'file', - $this->userSettings->getUserSetting($owner, 'stream', Files_Sharing::TYPE_SHARED), - $this->userSettings->getUserSetting($owner, 'email', Files_Sharing::TYPE_SHARED) ? $this->userSettings->getUserSetting($owner, 'setting', 'batchtime') : false + $this->userSettings->getUserSetting($owner, 'email', Files_Sharing::TYPE_SHARED) ? $this->userSettings->getUserSetting($owner, 'setting', 'batchtime') : false, + $this->userSettings->getUserSetting($owner, 'notification', Files_Sharing::TYPE_SHARED) ); } } @@ -987,20 +988,17 @@ protected function addNotificationsForGroupUsers(array $usersInGroup, $actionUse } $userIds = array_keys($affectedUsers); - $filteredStreamUsersInGroup = $this->userSettings->filterUsersBySetting($userIds, 'stream', Files_Sharing::TYPE_SHARED); $filteredEmailUsersInGroup = $this->userSettings->filterUsersBySetting($userIds, 'email', Files_Sharing::TYPE_SHARED); + $filteredNotificationUsers = $this->userSettings->filterUsersBySetting($userIds, 'notification', Files_Sharing::TYPE_SHARED); $affectedUsers = $this->fixPathsForShareExceptions($affectedUsers, $shareId); foreach ($affectedUsers as $user => $path) { - if (empty($filteredStreamUsersInGroup[$user]) && empty($filteredEmailUsersInGroup[$user])) { - continue; - } $this->addNotificationsForUser( $user, $actionUser, [[$fileSource => $path], $this->currentUser->getUserIdentifier()], $fileSource, $path, ($itemType === 'file'), - !empty($filteredStreamUsersInGroup[$user]), - $filteredEmailUsersInGroup[$user] ?? false + $filteredEmailUsersInGroup[$user] ?? false, + $filteredNotificationUsers[$user] ?? false ); } } @@ -1018,7 +1016,7 @@ protected function fixPathsForShareExceptions(array $affectedUsers, $shareId) { $queryBuilder->select(['share_with', 'file_target']) ->from('share') ->where($queryBuilder->expr()->eq('parent', $queryBuilder->createParameter('parent'))) - ->setParameter('parent', (int) $shareId); + ->setParameter('parent', (int)$shareId); $query = $queryBuilder->execute(); while ($row = $query->fetch()) { @@ -1053,8 +1051,8 @@ protected function shareNotificationForSharer($subject, $shareWith, $fileSource, $this->addNotificationsForUser( $sharer, $subject, [[$fileSource => $path], $shareWith], $fileSource, $path, ($itemType === 'file'), - $this->userSettings->getUserSetting($sharer, 'stream', Files_Sharing::TYPE_SHARED), - $this->userSettings->getUserSetting($sharer, 'email', Files_Sharing::TYPE_SHARED) ? $this->userSettings->getUserSetting($sharer, 'setting', 'batchtime') : false + $this->userSettings->getUserSetting($sharer, 'email', Files_Sharing::TYPE_SHARED) ? $this->userSettings->getUserSetting($sharer, 'setting', 'batchtime') : false, + $this->userSettings->getUserSetting($sharer, 'notification', Files_Sharing::TYPE_SHARED) ); } @@ -1079,8 +1077,8 @@ protected function reshareNotificationForSharer($owner, $subject, $shareWith, $f $this->addNotificationsForUser( $owner, $subject, [[$fileSource => $path], $this->currentUser->getUserIdentifier(), $shareWith], $fileSource, $path, ($itemType === 'file'), - $this->userSettings->getUserSetting($owner, 'stream', Files_Sharing::TYPE_SHARED), - $this->userSettings->getUserSetting($owner, 'email', Files_Sharing::TYPE_SHARED) ? $this->userSettings->getUserSetting($owner, 'setting', 'batchtime') : false + $this->userSettings->getUserSetting($owner, 'email', Files_Sharing::TYPE_SHARED) ? $this->userSettings->getUserSetting($owner, 'setting', 'batchtime') : false, + $this->userSettings->getUserSetting($owner, 'notification', Files_Sharing::TYPE_SHARED) ); } @@ -1143,21 +1141,17 @@ protected function shareNotificationForOriginalOwners($currentOwner, $subject, $ * @param int $fileId * @param string $path * @param bool $isFile If the item is a file, we link to the parent directory - * @param bool $streamSetting * @param int $emailSetting + * @param bool $notificationSetting * @param string $type */ - protected function addNotificationsForUser($user, $subject, $subjectParams, $fileId, $path, $isFile, $streamSetting, $emailSetting, $type = Files_Sharing::TYPE_SHARED) { - if (!$streamSetting && !$emailSetting) { - return; - } - + protected function addNotificationsForUser($user, $subject, $subjectParams, $fileId, $path, $isFile, $emailSetting, $notificationSetting, $type = Files_Sharing::TYPE_SHARED) { $user = (string)$user; $selfAction = $user === $this->currentUser->getUID(); $app = $type === Files_Sharing::TYPE_SHARED ? 'files_sharing' : 'files'; - $link = $this->urlGenerator->linkToRouteAbsolute('files.view.index', array( + $link = $this->urlGenerator->linkToRouteAbsolute('files.view.index', [ 'dir' => ($isFile) ? dirname($path) : $path, - )); + ]); $objectType = ($fileId) ? 'files' : ''; @@ -1180,10 +1174,10 @@ protected function addNotificationsForUser($user, $subject, $subjectParams, $fil } // Add activity to stream - if ($streamSetting && (!$selfAction || $this->userSettings->getUserSetting($this->currentUser->getUID(), 'setting', 'self'))) { + if (!$selfAction || $this->userSettings->getUserSetting($this->currentUser->getUID(), 'setting', 'self')) { $activityId = $this->activityData->send($event); - if ($activityId) { + if ($activityId && !$selfAction && $notificationSetting) { $this->notificationGenerator->sendNotificationForEvent($event, $activityId); } } diff --git a/templates/settings/form.php b/templates/settings/form.php index 359d132f0..b59d85771 100644 --- a/templates/settings/form.php +++ b/templates/settings/form.php @@ -62,18 +62,7 @@ -
- checked="checked" /> - -
- - checked="checked" /> - -
-
t('You need to set up your email address before you can receive notification emails.')); ?> diff --git a/tests/ConsumerTest.php b/tests/ConsumerTest.php index bc4eaf9f8..ffde9d6fb 100644 --- a/tests/ConsumerTest.php +++ b/tests/ConsumerTest.php @@ -158,7 +158,7 @@ public function testReceiveStream(string $type, string $author, string $affected ->setLink('link'); $this->deleteTestActivities(); - if ($author === $affectedUser && $this->userSettings->getUserSetting($affectedUser, 'setting', 'self') === false) { + if ($author === $affectedUser) { $this->data->expects($this->never()) ->method('send'); } else { diff --git a/tests/FilesHooksTest.php b/tests/FilesHooksTest.php index c61de5682..73ac77322 100755 --- a/tests/FilesHooksTest.php +++ b/tests/FilesHooksTest.php @@ -24,6 +24,7 @@ namespace OCA\Activity; use OC\Files\Config\CachedMountFileInfo; +use OC\Tags; use OCA\Activity\Extension\Files; use OCA\Activity\Extension\Files_Sharing; use OCA\Activity\Tests\TestCase; @@ -32,6 +33,7 @@ use OCP\Files\NotFoundException; use OCP\IConfig; use OCP\ILogger; +use OCP\ITagManager; use OCP\IUser; use OCP\Share\IShare; use OCP\Share\IShareHelper; @@ -77,6 +79,9 @@ class FilesHooksTest extends TestCase { protected $config; /** @var NotificationGenerator|MockObject */ protected $notificationGenerator; + /** @var ITagManager|MockObject */ + protected $tagManager; + protected $tags; protected function setUp(): void { parent::setUp(); @@ -92,6 +97,13 @@ protected function setUp(): void { $this->userMountCache = $this->createMock(IUserMountCache::class); $this->config = $this->createMock(IConfig::class); $this->notificationGenerator = $this->createMock(NotificationGenerator::class); + $this->tagManager = $this->createMock(ITagManager::class); + $this->tags = $this->createMock(Tags::class); + $this->tags->method('getUsersFavoritingObject') + ->willReturn([]); + + $this->tagManager->method('load') + ->willReturn($this->tags); $this->filesHooks = $this->getFilesHooks(); } @@ -128,7 +140,8 @@ protected function getFilesHooks(array $mockedMethods = [], string $user = 'user $currentUser, $this->userMountCache, $this->config, - $this->notificationGenerator + $this->notificationGenerator, + $this->tagManager ]) ->onlyMethods($mockedMethods) ->getMock(); @@ -148,7 +161,8 @@ protected function getFilesHooks(array $mockedMethods = [], string $user = 'user $currentUser, $this->userMountCache, $this->config, - $this->notificationGenerator + $this->notificationGenerator, + $this->tagManager ); } @@ -181,7 +195,7 @@ public function testFileCreate(string $currentUser, string $selfSubject, string $filesHooks->expects($this->once()) ->method('addNotificationsForFileAction') - ->with('path', Files::TYPE_SHARE_CREATED, $selfSubject, $othersSubject); + ->with('path', $selfSubject, $othersSubject); $filesHooks->fileCreate('path'); } @@ -209,7 +223,7 @@ public function testFileUpdate(): void { $filesHooks->expects($this->once()) ->method('addNotificationsForFileAction') - ->with('path', Files::TYPE_SHARE_CHANGED, 'changed_self', 'changed_by'); + ->with('path', 'changed_self', 'changed_by'); $filesHooks->fileUpdate('path'); } @@ -221,7 +235,7 @@ public function testFileDelete(): void { $filesHooks->expects($this->once()) ->method('addNotificationsForFileAction') - ->with('path', Files::TYPE_SHARE_DELETED, 'deleted_self', 'deleted_by'); + ->with('path', 'deleted_self', 'deleted_by'); $filesHooks->fileDelete('path'); } @@ -233,7 +247,7 @@ public function testFileRestore(): void { $filesHooks->expects($this->once()) ->method('addNotificationsForFileAction') - ->with('path', Files::TYPE_SHARE_RESTORED, 'restored_self', 'restored_by'); + ->with('path', 'restored_self', 'restored_by'); $filesHooks->fileRestore('path'); } @@ -253,64 +267,96 @@ public function dataAddNotificationsForFileAction(): array { return [ [ [ - [['user', 'user1', 'user2'], 'stream', Files::TYPE_SHARE_RESTORED, ['user' => true]], - [['user', 'user1', 'user2'], 'email', Files::TYPE_SHARE_RESTORED, ['user' => 42]], + 'email' => ['user' => 42], + 'notification' => ['user' => true], ], 'mountcache_used' => false, + 'isFavorite' => true, [ 'user' => [ 'subject' => 'restored_self', 'subject_params' => [[1337 => '/user/files/path']], 'path' => '/user/files/path', - 'stream' => true, + 'notification' => true, 'email' => 42, ], + 'user1' => [ + 'subject' => 'restored_by', + 'subject_params' => [[1337 => '/user1/files/path'], 'user'], + 'path' => '/user1/files/path', + 'notification' => false, + 'email' => 0, + ], ], ], [ [ - [['user', 'user1', 'user2'], 'stream', Files::TYPE_SHARE_RESTORED, ['user1' => true]], - [['user', 'user1', 'user2'], 'email', Files::TYPE_SHARE_RESTORED, []], + 'email' => [], + 'notification' => ['user1' => false], ], 'mountcache_used' => false, + 'isFavorite' => true, [ + 'user' => [ + 'subject' => 'restored_self', + 'subject_params' => [[1337 => '/user/files/path']], + 'path' => '/user/files/path', + 'notification' => false, + 'email' => 0, + ], 'user1' => [ 'subject' => 'restored_by', 'subject_params' => [[1337 => '/user1/files/path'], 'user'], 'path' => '/user1/files/path', - 'stream' => true, + 'notification' => false, 'email' => 0, ], ], ], [ [ - [['user', 'user1', 'user2'], 'stream', Files::TYPE_SHARE_RESTORED, ['user' => true]], - [['user', 'user1', 'user2'], 'email', Files::TYPE_SHARE_RESTORED, ['user' => 42]], + 'email' => ['user' => 42], + 'notification' => ['user' => false], ], 'mountcache_used' => true, + 'isFavorite' => true, [ 'user' => [ 'subject' => 'restored_self', 'subject_params' => [[1337 => '/path']], 'path' => '/path', - 'stream' => true, + 'notification' => false, 'email' => 42, ], + 'user1' => [ + 'subject' => 'restored_by', + 'subject_params' => [[1337 => '/path'], 'user'], + 'path' => '/path', + 'notification' => false, + 'email' => 0, + ], ], ], [ [ - [['user', 'user1', 'user2'], 'stream', Files::TYPE_SHARE_RESTORED, ['user1' => true]], - [['user', 'user1', 'user2'], 'email', Files::TYPE_SHARE_RESTORED, []], + 'email' => [], + 'notification' => ['user1' => true], ], 'mountcache_used' => true, + 'isFavorite' => true, [ + 'user' => [ + 'subject' => 'restored_self', + 'subject_params' => [[1337 => '/path']], + 'path' => '/path', + 'notification' => false, + 'email' => 0, + ], 'user1' => [ 'subject' => 'restored_by', 'subject_params' => [[1337 => '/path'], 'user'], 'path' => '/path', - 'stream' => true, + 'notification' => true, 'email' => 0, ], ], @@ -323,15 +369,21 @@ public function dataAddNotificationsForFileAction(): array { * * @param array $filterUsers * @param bool $mountCacheUsed + * @param bool $isFavorite * @param array $addNotifications */ - public function testAddNotificationsForFileAction(array $filterUsers, bool $mountCacheUsed, array $addNotifications): void { + public function testAddNotificationsForFileAction(array $filterUsers, bool $mountCacheUsed, bool $isFavorite, array $addNotifications): void { $filesHooks = $this->getFilesHooks([ 'getSourcePathAndOwner', 'getUserPathsFromPath', 'addNotificationsForUser', ]); + if ($isFavorite) { + $this->tags->method('getUsersFavoritingObject') + ->willReturn(['user', 'user1', 'user2']); + } + $filesHooks->expects($this->once()) ->method('getSourcePathAndOwner') ->with('path') @@ -393,7 +445,9 @@ public function testAddNotificationsForFileAction(array $filterUsers, bool $moun $this->settings->expects($this->exactly(2)) ->method('filterUsersBySetting') - ->willReturnMap($filterUsers); + ->willReturnCallback(function($users, $method, $type) use ($filterUsers) { + return $filterUsers[$method]; + }); $i = 2; foreach ($addNotifications as $user => $arguments) { @@ -406,14 +460,14 @@ public function testAddNotificationsForFileAction(array $filterUsers, bool $moun 1337, $arguments['path'], true, - $arguments['stream'], $arguments['email'], - Files::TYPE_SHARE_RESTORED + $arguments['notification'], + Files::TYPE_FILE_CHANGED ); $i++; } - self::invokePrivate($filesHooks, 'addNotificationsForFileAction', ['path', Files::TYPE_SHARE_RESTORED, 'restored_self', 'restored_by']); + self::invokePrivate($filesHooks, 'addNotificationsForFileAction', ['path', 'restored_self', 'restored_by']); } public function testHookShareWithUser(): void { @@ -495,7 +549,7 @@ public function testShareWithUser(string $itemType, string $fileTarget, bool $is ->method('getUserSetting') ->willReturnMap( [ - ['recipient', 'stream', Files_Sharing::TYPE_SHARED, true], + ['recipient', 'notification', Files_Sharing::TYPE_SHARED, true], ['recipient', 'email', Files_Sharing::TYPE_SHARED, true], ['recipient', 'setting', 'batchtime', 42], ] @@ -513,8 +567,8 @@ public function testShareWithUser(string $itemType, string $fileTarget, bool $is 1337, $fileTarget, $isFile, - true, - 42 + 42, + true ); self::invokePrivate($filesHooks, 'shareWithUser', [ @@ -561,7 +615,7 @@ public function dataShareWithGroup(): array { 2, 1, ['user1'], [ - [['user1'], 'stream', Files_Sharing::TYPE_SHARED, ['user1' => true]], + [['user1'], 'notification', Files_Sharing::TYPE_SHARED, ['user1' => true]], [['user1'], 'email', Files_Sharing::TYPE_SHARED, []], ], [ @@ -569,7 +623,7 @@ public function dataShareWithGroup(): array { 'subject' => 'shared_with_by', 'subject_params' => [[42 => '/file'], 'user'], 'path' => '/file', - 'stream' => true, + 'notification' => true, 'email' => 0, ], ], @@ -582,7 +636,7 @@ public function dataShareWithGroup(): array { 2, 1, ['user1'], [ - [['user1'], 'stream', Files_Sharing::TYPE_SHARED, ['user1' => false]], + [['user1'], 'notification', Files_Sharing::TYPE_SHARED, ['user1' => false]], [['user1'], 'email', Files_Sharing::TYPE_SHARED, ['user1' => false]], ], [], @@ -658,8 +712,8 @@ public function testShareWithGroup(array $usersInGroup, int $settingCalls, int $ 42, $arguments['path'], true, - $arguments['stream'], $arguments['email'], + $arguments['notification'], Files_Sharing::TYPE_SHARED ); $i++; @@ -670,32 +724,6 @@ public function testShareWithGroup(array $usersInGroup, int $settingCalls, int $ ]); } - public function dataAddNotificationsForUserWithoutSettings(): array { - return [ - ['user', 'subject', ['parameter'], 42, 'path', true, false, false, Files::TYPE_SHARE_CREATED] - ]; - } - - /** - * @dataProvider dataAddNotificationsForUserWithoutSettings - * - * @param string $user - * @param string $subject - * @param array $parameter - * @param int $fileId - * @param string $path - * @param bool $isFile - * @param bool $stream - * @param bool $email - * @param string $type - */ - public function testAddNotificationsForUserWithoutSettings(string $user, string $subject, array $parameter, int $fileId, string $path, bool $isFile, bool $stream, bool $email, string $type): void { - $this->activityManager->expects($this->never()) - ->method('generateEvent'); - - self::invokePrivate($this->filesHooks, 'addNotificationsForUser', [$user, $subject, $parameter, $fileId, $path, $isFile, $stream, $email, $type]); - } - public function dataReshareNotificationForSharer(): array { return [ [null], @@ -728,7 +756,7 @@ public function testReshareNotificationForSharer(?string $path): void { $this->settings->expects(($path !== null) ? $this->exactly(3) : $this->never()) ->method('getUserSetting') ->willReturnMap([ - ['owner', 'stream', Files_Sharing::TYPE_SHARED, true], + ['owner', 'notification', Files_Sharing::TYPE_SHARED, true], ['owner', 'email', Files_Sharing::TYPE_SHARED, true], ['owner', 'setting', 'batchtime', 21], ]); @@ -779,7 +807,7 @@ public function testShare(?string $path): void { $this->settings->expects(($path !== null) ? $this->exactly(3) : $this->never()) ->method('getUserSetting') ->willReturnMap([ - ['user', 'stream', Files_Sharing::TYPE_SHARED, true], + ['user', 'notification', Files_Sharing::TYPE_SHARED, true], ['user', 'email', Files_Sharing::TYPE_SHARED, true], ['user', 'setting', 'batchtime', 21], ]); @@ -915,7 +943,7 @@ public function testShareNotificationForSharer(?string $path): void { $this->settings->expects(($path !== null) ? $this->exactly(3) : $this->never()) ->method('getUserSetting') ->willReturnMap([ - ['user', 'stream', Files_Sharing::TYPE_SHARED, true], + ['user', 'notification', Files_Sharing::TYPE_SHARED, true], ['user', 'email', Files_Sharing::TYPE_SHARED, true], ['user', 'setting', 'batchtime', 21], ]); @@ -962,7 +990,7 @@ public function dataAddNotificationsForUser(): array { * @param string $path * @param string $urlPath * @param bool $isFile - * @param bool $stream + * @param bool $notification * @param bool $email * @param string $type * @param bool $selfSetting @@ -971,7 +999,7 @@ public function dataAddNotificationsForUser(): array { * @param bool $sentStream * @param bool $sentEmail */ - public function testAddNotificationsForUser(string $user, string $subject, array $parameter, int $fileId, string $path, string $urlPath, bool $isFile, bool $stream, bool $email, string $type, bool $selfSetting, bool $selfEmailSetting, string $app, bool $sentStream, bool $sentEmail): void { + public function testAddNotificationsForUser(string $user, string $subject, array $parameter, int $fileId, string $path, string $urlPath, bool $isFile, bool $notification, bool $email, string $type, bool $selfSetting, bool $selfEmailSetting, string $app, bool $sentStream, bool $sentEmail): void { $this->settings->expects($this->any()) ->method('getUserSetting') ->willReturnMap([ @@ -1029,13 +1057,13 @@ public function testAddNotificationsForUser(string $user, string $subject, array ->method('generateEvent') ->willReturn($event); - $this->data->expects($sentStream ? $this->once() : $this->never()) + $this->data->expects(($user !== 'user' || $selfSetting) ? $this->once() : $this->never()) ->method('send') ->with($event); $this->data->expects($sentEmail ? $this->once() : $this->never()) ->method('storeMail') ->with($event, $this->anything()); - self::invokePrivate($this->filesHooks, 'addNotificationsForUser', [$user, $subject, $parameter, $fileId, $path, $isFile, $stream, $email, $type]); + self::invokePrivate($this->filesHooks, 'addNotificationsForUser', [$user, $subject, $parameter, $fileId, $path, $isFile, $email, $notification, $type]); } }