Skip to content

feat: Add sharing activity for teams #1754

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.

Sign up for GitHub

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

Merged
artonge merged 1 commit into from
Jan 23, 2025
+99 −48
Merged

feat: Add sharing activity for teams #1754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 59 additions & 7 deletions lib/FilesHooks.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
use OCA\Activity\BackgroundJob\RemoteActivity;
use OCA\Activity\Extension\Files;
use OCA\Activity\Extension\Files_Sharing;
use OCA\Circles\CirclesManager;
use OCA\Circles\Model\Member;
use OCP\Activity\IManager;
use OCP\Constants;
use OCP\Files\Config\IUserMountCache;
Expand Down Expand Up @@ -60,7 +62,8 @@ public function __construct(
protected IUserMountCache $userMountCache,
protected IConfig $config,
protected NotificationGenerator $notificationGenerator,
protected ITagManager $tagManager
protected ITagManager $tagManager,
protected ?CirclesManager $teamManager,
) {
}

Expand Down Expand Up @@ -664,6 +667,16 @@ public function share($share) {
(int)$share->getId()
);
break;
case IShare::TYPE_CIRCLE:
$this->shareWithTeam(
$share->getSharedWith(),
$share->getNodeId(),
$share->getNodeType(),
$share->getTarget(),
(int)$share->getId(),
$share->getSharedBy(),
);
break;
case IShare::TYPE_LINK:
$this->shareByLink(
$share->getNodeId(),
Expand Down Expand Up @@ -726,7 +739,8 @@ protected function shareWithGroup($shareWith, $fileSource, $itemType, $fileTarge
$offset = 0;
$users = $group->searchUsers('', self::USER_BATCH_SIZE, $offset);
while (!empty($users)) {
$this->addNotificationsForGroupUsers($users, 'shared_with_by', $fileSource, $itemType, $fileTarget, $shareId);
$userIds = array_map(fn (IUser $user) => $user->getUID(), $users);
$this->addNotificationsForUsers($userIds, 'shared_with_by', $fileSource, $itemType, $fileTarget, $shareId);
$offset += self::USER_BATCH_SIZE;
$users = $group->searchUsers('', self::USER_BATCH_SIZE, $offset);
}
Expand Down Expand Up @@ -758,6 +772,42 @@ protected function shareByLink($fileSource, $itemType, $linkOwner) {
);
}

/**
* Sharing a file or folder with a team
*
* @param string $shareWith
* @param int $fileSource File ID that is being shared
* @param string $itemType File type that is being shared (file or folder)
* @param string $fileTarget File path
* @param int $shareId The Share ID of this share
*/
protected function shareWithTeam(string $shareWith, int $fileSource, string $itemType, string $fileTarget, int $shareId, string $sharer): void {
if ($this->teamManager === null) {
return;
}

try {
$this->teamManager->startSuperSession();
$team = $this->teamManager->getCircle($shareWith);
$members = $team->getInheritedMembers();
$members = array_filter($members, fn ($member) => $member->getUserType() === Member::TYPE_USER);
$userIds = array_map(fn ($member) => $member->getUserId(), $members);
} catch (\Throwable $e) {
$this->logger->debug('Fetching team members for share activity failed', ['exception' => $e]);
// error in teams app - setting users list to empty
$userIds = [];
}

// Activity for user performing the share
$this->shareNotificationForSharer('shared_team_self', $shareWith, $fileSource, $itemType);
// Activity for original owner of the file (re-sharing)
if ($this->currentUser->getUID() !== null) {
$this->shareNotificationForOriginalOwners($this->currentUser->getUID(), 're-shared_team_by', $shareWith, $fileSource, $itemType);
}
// Activity for all affected users
$this->addNotificationsForUsers($userIds, 'shared_with_by', $fileSource, $itemType, $fileTarget, $shareId);
}

/**
* Manage unsharing events
*
Expand Down Expand Up @@ -878,9 +928,11 @@ protected function unshareFromGroup(IShare $share) {

$offset = 0;
$users = $group->searchUsers('', self::USER_BATCH_SIZE, $offset);
$users = array_map(fn (IUser $user) => $user->getUID(), $users);
$shouldFlush = $this->startActivityTransaction();
while (!empty($users)) {
$this->addNotificationsForGroupUsers($users, $actionUser, $share->getNodeId(), $share->getNodeType(), $share->getTarget(), (int)$share->getId());
$userIds = array_map(fn (IUser $user) => $user->getUID(), $users);
Comment on lines +931 to +934
Copy link
Contributor

@come-nc come-nc Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$users contains string[] after line 931, then you try to convert to string again.
Results in:

Argument #1 ($user) must be of type OCP\IUser, string given in file 'server/server/apps/activity/lib/FilesHooks.php' line 935

Copy link
Contributor Author

@susnux susnux Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh seems like a rebase issue or similar, line 931 should be removed...

Copy link
Contributor Author

@susnux susnux Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(or remove 934 to keep the activity transaction as short as possible)

$this->addNotificationsForUsers($userIds, $actionUser, $share->getNodeId(), $share->getNodeType(), $share->getTarget(), (int)$share->getId());
$offset += self::USER_BATCH_SIZE;
$users = $group->searchUsers('', self::USER_BATCH_SIZE, $offset);
}
Expand Down Expand Up @@ -940,18 +992,18 @@ protected function unshareLink(IShare $share) {
}

/**
* @param IUser[] $usersInGroup
* @param string[] $usersIds
* @param string $actionUser
* @param int $fileSource File ID that is being shared
* @param string $itemType File type that is being shared (file or folder)
* @param string $fileTarget File path
* @param int $shareId The Share ID of this share
*/
protected function addNotificationsForGroupUsers(array $usersInGroup, $actionUser, $fileSource, $itemType, $fileTarget, $shareId) {
protected function addNotificationsForUsers(array $usersIds, $actionUser, $fileSource, $itemType, $fileTarget, $shareId) {
$affectedUsers = [];

foreach ($usersInGroup as $user) {
$affectedUsers[$user->getUID()] = $fileTarget;
foreach ($usersIds as $user) {
$affectedUsers[$user] = $fileTarget;
}

// Remove the triggering user, we already managed his notifications
Expand Down
58 changes: 27 additions & 31 deletions tests/FilesHooksTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
use OCP\Files\Mount\IMountPoint;
use OCP\Files\NotFoundException;
use OCP\IConfig;
use OCP\IDBConnection;
use OCP\IGroup;
use OCP\IGroupManager;
use OCP\IURLGenerator;
Expand All @@ -57,33 +58,25 @@
* @package OCA\Activity
*/
class FilesHooksTest extends TestCase {
/** @var FilesHooks */
protected $filesHooks;
/** @var IManager|MockObject */
protected $activityManager;
/** @var Data|MockObject */
protected $data;
/** @var UserSettings|MockObject */
protected $settings;
/** @var IGroupManager|MockObject */
protected $groupManager;
/** @var View|MockObject */
protected $view;
/** @var IRootFolder|MockObject */
protected $rootFolder;
/** @var IShareHelper|MockObject */
protected $shareHelper;
/** @var IURLGenerator|MockObject */
protected $urlGenerator;
/** @var IUserMountCache|MockObject */
protected $userMountCache;
/** @var IConfig|MockObject */
protected $config;
/** @var NotificationGenerator|MockObject */
protected $notificationGenerator;
/** @var TagManager|MockObject */
protected $tagManager;
protected FilesHooks $filesHooks;
protected IManager&MockObject $activityManager;
protected Data&MockObject $data;
protected UserSettings&MockObject $settings;
protected IGroupManager&MockObject $groupManager;
protected View&MockObject $view;
protected IRootFolder&MockObject $rootFolder;
protected IShareHelper&MockObject $shareHelper;
protected IURLGenerator&MockObject $urlGenerator;
protected IUserMountCache&MockObject $userMountCache;
protected IConfig&MockObject $config;
protected NotificationGenerator&MockObject $notificationGenerator;
protected TagManager&MockObject $tagManager;
protected $tags;
/**
* @todo With PHP 8.2 we can put this directly on the declaration instead of a comment but 8.1 does not allow the parenthesis
* @var (OCA\Circles\CirclesManager&MockObject)|null
*/
protected $teamManager;

protected function setUp(): void {
parent::setUp();
Expand All @@ -103,6 +96,7 @@ protected function setUp(): void {
$this->tags = $this->createMock(Tags::class);
$this->tagManager->method('getUsersFavoritingObject')
->willReturn([]);
$this->teamManager = null;

$this->tagManager->method('load')
->willReturn($this->tags);
Expand All @@ -113,7 +107,7 @@ protected function setUp(): void {
/**
* @param array $mockedMethods
* @param string $user
* @return FilesHooks|MockObject
* @return FilesHooks&MockObject
*/
protected function getFilesHooks(array $mockedMethods = [], string $user = 'user'): FilesHooks {
$currentUser = $this->createMock(CurrentUser::class);
Expand All @@ -136,14 +130,15 @@ protected function getFilesHooks(array $mockedMethods = [], string $user = 'user
$this->view,
$this->rootFolder,
$this->shareHelper,
\OC::$server->getDatabaseConnection(),
\OCP\Server::get(IDBConnection::class),
$this->urlGenerator,
$logger,
$currentUser,
$this->userMountCache,
$this->config,
$this->notificationGenerator,
$this->tagManager
$this->tagManager,
$this->teamManager,
])
->onlyMethods($mockedMethods)
->getMock();
Expand All @@ -157,14 +152,15 @@ protected function getFilesHooks(array $mockedMethods = [], string $user = 'user
$this->view,
$this->rootFolder,
$this->shareHelper,
\OC::$server->getDatabaseConnection(),
\OCP\Server::get(IDBConnection::class),
$this->urlGenerator,
$logger,
$currentUser,
$this->userMountCache,
$this->config,
$this->notificationGenerator,
$this->tagManager
$this->tagManager,
$this->teamManager,
);
}

Expand Down
23 changes: 13 additions & 10 deletions tests/psalm-baseline.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
<?xml version="1.0" encoding="UTF-8"?>
<files psalm-version="5.19.0@06b71be009a6bd6d81b9811855d6629b9fe90e1b">
<files psalm-version="5.25.0@01a8eb06b9e9cc6cfb6a320bf9fb14331919d505">
<file src="lib/BackgroundJob/RemoteActivity.php">
<UndefinedClass>
<code>ClientException</code>
<code><![CDATA[ClientException]]></code>
</UndefinedClass>
</file>
<file src="lib/CurrentUser.php">
Expand All @@ -21,27 +21,30 @@
</InvalidArrayOffset>
</file>
<file src="lib/FilesHooks.php">
<InvalidArgument>
<code><![CDATA[$share->getId()]]></code>
</InvalidArgument>
<UndefinedClass>
<code><![CDATA[$this->teamManager]]></code>
<code><![CDATA[$this->teamManager]]></code>
<code><![CDATA[Member]]></code>
<code><![CDATA[protected]]></code>
</UndefinedClass>
<UndefinedDocblockClass>
<code>$shareOwner</code>
<code>$storage</code>
<code><![CDATA[$shareOwner]]></code>
<code><![CDATA[$storage]]></code>
</UndefinedDocblockClass>
</file>
<file src="lib/Migration/Version2008Date20181011095117.php">
<UndefinedClass>
<code>SchemaException</code>
<code><![CDATA[SchemaException]]></code>
</UndefinedClass>
</file>
<file src="lib/Migration/Version2011Date20201006132544.php">
<UndefinedClass>
<code>Type</code>
<code><![CDATA[Type]]></code>
</UndefinedClass>
</file>
<file src="lib/Migration/Version2011Date20201006132545.php">
<UndefinedClass>
<code>Type</code>
<code><![CDATA[Type]]></code>
</UndefinedClass>
</file>
</files>
Loading