Only share to users and groups when necessary

[lukasdotcom]

Different method than #52 that gets all users that have access to a file first and then updates the shares based on that. Thanks @SystemKeeper

[SystemKeeper]

This does not make sense to me. If one group member fails, all are failed?
But I also checked, the return value of shareWithApprovers is never used, can we get rid of it entirely?

[lukasdotcom]

I just removed createdShares as it is not needed anyway

[julien-nc]

If some people already have access, we don't create a group share but user shares. But then, if a user is added to the "approver" group, this user won't have access to the file that they are allowed to approve.

Not sure how to deal with that limitation but it's worth mentioning it in the README. Wdyt?

[lukasdotcom]

If some people already have access, we don't create a group share but user shares. But then, if a user is added to the "approver" group, this user won't have access to the file that they are allowed to approve.

Not sure how to deal with that limitation but it's worth mentioning it in the README. Wdyt?

It seems like a lot of users wanted the behavior with sharing with individuals instead when it makes sense, but mentioning it in the README is a good idea.

[SystemKeeper]

I agree, mentioning it in the README totally makes sense.

I can't really judge how the app is used, I totally can see use cases that would require one or the other. Besides listening on group changes and checking access, the only other idea would be to still create a group share and for those who already have access, to leave the share (if I understand it correctly for that USERGROUP share permission is set to 0). That would prevent double shares and new users would still gain access (which then could be result in a double share for that new user).

Is it better? No idea.

[julien-nc]

Lgtm 👍

[SystemKeeper]

Did the following test:

• Have a group with 100 users
• Group has access to the files via groupfolders

Before this PR on the first (createShares = true) request
2872 queries done

After this PR on the first (createShares = true) request
160 queries done

The second request (createShares = false) is still quite heavy, I think this is coming from

approval/lib/Service/ApprovalService.php

Lines 765 to 820 in fb7da39

	public function sendRequestNotification(int $fileId, array $rule, string $requestUserId, bool $checkAccess): void {
	// find users involved in rules matching tags
	$rulesUserIds = [];
	$thisRuleUserIds = $this->getRuleAuthorizedUserIds($rule, 'approvers');
	foreach ($thisRuleUserIds as $userId) {
	if (!in_array($userId, $rulesUserIds, true)) {
	$rulesUserIds[] = $userId;
	}
	}
	// create activity (which deals with access checks)
	$this->activityManager->triggerEvent(
	ActivityManager::APPROVAL_OBJECT_NODE, $fileId,
	ActivityManager::SUBJECT_MANUALLY_REQUESTED,
	['users' => $thisRuleUserIds, 'who' => $requestUserId]
	);
	$paramsByUser = [];
	$root = $this->root;
	if ($checkAccess) {
	// only notify users having access to the file
	foreach ($rulesUserIds as $userId) {
	$userFolder = $root->getUserFolder($userId);
	$node = $userFolder->getFirstNodeById($fileId);
	if ($node !== null) {
	$path = $userFolder->getRelativePath($node->getPath());
	$type = $node->getType() === FileInfo::TYPE_FILE
	? 'file'
	: 'folder';
	$paramsByUser[$userId] = [
	'type' => $type,
	'fileId' => $fileId,
	'fileName' => $node->getName(),
	'relativePath' => $path,
	];
	}
	}
	} else {
	// we don't check if users have access to the file because they might not have yet (share is not effective yet)
	// => notify every approver
	foreach ($rulesUserIds as $userId) {
	$node = $root->getFirstNodeById($fileId);
	if ($node !== null) {
	// we don't know the path in user storage
	$path = '';
	$type = $node->getType() === FileInfo::TYPE_FILE
	? 'file'
	: 'folder';
	$paramsByUser[$userId] = [
	'type' => $type,
	'fileId' => $fileId,
	'fileName' => $node->getName(),
	'relativePath' => $path,
	];
	}
	}
	}

Maybe we could do a similar thing there.

Otherwise from what I can judge, looks good to me.

[lukasdotcom]

Did the following test:

* Have a group with 100 users

* Group has access to the files via groupfolders

Before this PR on the first (createShares = true) request 2872 queries done

After this PR on the first (createShares = true) request 160 queries done

The second request (createShares = false) is still quite heavy, I think this is coming from

approval/lib/Service/ApprovalService.php

Lines 765 to 820 in fb7da39

	public function sendRequestNotification(int $fileId, array $rule, string $requestUserId, bool $checkAccess): void {
	// find users involved in rules matching tags
	$rulesUserIds = [];
	$thisRuleUserIds = $this->getRuleAuthorizedUserIds($rule, 'approvers');
	foreach ($thisRuleUserIds as $userId) {
	if (!in_array($userId, $rulesUserIds, true)) {
	$rulesUserIds[] = $userId;
	}
	}
	// create activity (which deals with access checks)
	$this->activityManager->triggerEvent(
	ActivityManager::APPROVAL_OBJECT_NODE, $fileId,
	ActivityManager::SUBJECT_MANUALLY_REQUESTED,
	['users' => $thisRuleUserIds, 'who' => $requestUserId]
	);
	$paramsByUser = [];
	$root = $this->root;
	if ($checkAccess) {
	// only notify users having access to the file
	foreach ($rulesUserIds as $userId) {
	$userFolder = $root->getUserFolder($userId);
	$node = $userFolder->getFirstNodeById($fileId);
	if ($node !== null) {
	$path = $userFolder->getRelativePath($node->getPath());
	$type = $node->getType() === FileInfo::TYPE_FILE
	? 'file'
	: 'folder';
	$paramsByUser[$userId] = [
	'type' => $type,
	'fileId' => $fileId,
	'fileName' => $node->getName(),
	'relativePath' => $path,
	];
	}
	}
	} else {
	// we don't check if users have access to the file because they might not have yet (share is not effective yet)
	// => notify every approver
	foreach ($rulesUserIds as $userId) {
	$node = $root->getFirstNodeById($fileId);
	if ($node !== null) {
	// we don't know the path in user storage
	$path = '';
	$type = $node->getType() === FileInfo::TYPE_FILE
	? 'file'
	: 'folder';
	$paramsByUser[$userId] = [
	'type' => $type,
	'fileId' => $fileId,
	'fileName' => $node->getName(),
	'relativePath' => $path,
	];
	}
	}
	}

Maybe we could do a similar thing there.

Otherwise from what I can judge, looks good to me.

Did the following test:

* Have a group with 100 users

* Group has access to the files via groupfolders

Before this PR on the first (createShares = true) request 2872 queries done

After this PR on the first (createShares = true) request 160 queries done

The second request (createShares = false) is still quite heavy, I think this is coming from

approval/lib/Service/ApprovalService.php

Lines 765 to 820 in fb7da39

	public function sendRequestNotification(int $fileId, array $rule, string $requestUserId, bool $checkAccess): void {
	// find users involved in rules matching tags
	$rulesUserIds = [];
	$thisRuleUserIds = $this->getRuleAuthorizedUserIds($rule, 'approvers');
	foreach ($thisRuleUserIds as $userId) {
	if (!in_array($userId, $rulesUserIds, true)) {
	$rulesUserIds[] = $userId;
	}
	}
	// create activity (which deals with access checks)
	$this->activityManager->triggerEvent(
	ActivityManager::APPROVAL_OBJECT_NODE, $fileId,
	ActivityManager::SUBJECT_MANUALLY_REQUESTED,
	['users' => $thisRuleUserIds, 'who' => $requestUserId]
	);
	$paramsByUser = [];
	$root = $this->root;
	if ($checkAccess) {
	// only notify users having access to the file
	foreach ($rulesUserIds as $userId) {
	$userFolder = $root->getUserFolder($userId);
	$node = $userFolder->getFirstNodeById($fileId);
	if ($node !== null) {
	$path = $userFolder->getRelativePath($node->getPath());
	$type = $node->getType() === FileInfo::TYPE_FILE
	? 'file'
	: 'folder';
	$paramsByUser[$userId] = [
	'type' => $type,
	'fileId' => $fileId,
	'fileName' => $node->getName(),
	'relativePath' => $path,
	];
	}
	}
	} else {
	// we don't check if users have access to the file because they might not have yet (share is not effective yet)
	// => notify every approver
	foreach ($rulesUserIds as $userId) {
	$node = $root->getFirstNodeById($fileId);
	if ($node !== null) {
	// we don't know the path in user storage
	$path = '';
	$type = $node->getType() === FileInfo::TYPE_FILE
	? 'file'
	: 'folder';
	$paramsByUser[$userId] = [
	'type' => $type,
	'fileId' => $fileId,
	'fileName' => $node->getName(),
	'relativePath' => $path,
	];
	}
	}
	}

Maybe we could do a similar thing there.

Otherwise from what I can judge, looks good to me.

I'll look at that in a new pr.

[lukasdotcom]

/backport to stable32