From 5d9eed827a54b6826fb827b9944887e07c0297ba Mon Sep 17 00:00:00 2001 From: Christian Hartmann Date: Tue, 26 May 2026 17:35:16 +0200 Subject: [PATCH] fix: only send one submission notification per user Signed-off-by: Christian Hartmann --- lib/Activity/ActivityManager.php | 49 ++++++------------ lib/Service/FormsService.php | 45 ++++++++++++++-- tests/Unit/Activity/ActivityManagerTest.php | 42 ++++++--------- tests/Unit/Service/FormsServiceTest.php | 57 ++++++++++++++++++++- 4 files changed, 129 insertions(+), 64 deletions(-) diff --git a/lib/Activity/ActivityManager.php b/lib/Activity/ActivityManager.php index ab864ef4f..5e842d854 100644 --- a/lib/Activity/ActivityManager.php +++ b/lib/Activity/ActivityManager.php @@ -11,8 +11,6 @@ use OCA\Forms\Service\CirclesService; use OCP\Activity\IManager; use OCP\IGroupManager; -use OCP\IUser; -use OCP\Share\IShare; class ActivityManager { @@ -127,39 +125,22 @@ public function publishNewSubmission(Form $form, string $submitterID): void { * Publish a new-Submission Activity for shared forms * * @param Form $form The affected Form - * @param string $submitterID ID of the User who submitted the form. Can also be our 'anon-user-'-ID + * @param string $userId ID of the user to notify + * @param string $submitterID ID of the user who submitted the form */ - public function publishNewSharedSubmission(Form $form, int $shareType, string $shareWith, string $submitterID): void { - $users = []; - switch ($shareType) { - case IShare::TYPE_USER: - $users[] = $shareWith; - break; - case IShare::TYPE_GROUP: - $group = $this->groupManager->get($shareWith); - if ($group !== null) { - $users = array_map(fn (IUser $user) => $user->getUID(), $group->getUsers()); - } - break; - case IShare::TYPE_CIRCLE: - $users = $this->circlesService->getCircleUsers($shareWith); - break; - } - - foreach ($users as $userId) { - $event = $this->manager->generateEvent(); - $event->setApp($this->appName) - ->setType(ActivityConstants::TYPE_NEWSHAREDSUBMISSION) - ->setAffectedUser($userId) - ->setAuthor($submitterID) - ->setSubject(ActivityConstants::SUBJECT_NEWSUBMISSION, [ - 'userId' => $submitterID, - 'formTitle' => $form->getTitle(), - 'formHash' => $form->getHash() - ]) - ->setObject('form', $form->getId()); + public function publishNewSharedSubmission(Form $form, string $userId, string $submitterID): void { + $event = $this->manager->generateEvent(); + $event->setApp($this->appName) + ->setType(ActivityConstants::TYPE_NEWSHAREDSUBMISSION) + ->setAffectedUser($userId) + ->setAuthor($submitterID) + ->setSubject(ActivityConstants::SUBJECT_NEWSUBMISSION, [ + 'userId' => $submitterID, + 'formTitle' => $form->getTitle(), + 'formHash' => $form->getHash() + ]) + ->setObject('form', $form->getId()); - $this->manager->publish($event); - } + $this->manager->publish($event); } } diff --git a/lib/Service/FormsService.php b/lib/Service/FormsService.php index 1335659b6..239ee94d9 100644 --- a/lib/Service/FormsService.php +++ b/lib/Service/FormsService.php @@ -713,10 +713,9 @@ public function notifyNewShares(Form $form, Share $share): void { * Creates activities for new submissions on a form * * @param Form $form Related Form - * @param string $submitter The ID of the user who submitted the form. Can also be our 'anon-user-'-ID + * @param Submission $submission the current submission */ public function notifyNewSubmission(Form $form, Submission $submission): void { - $shares = $this->getShares($form->getId()); try { $this->activityManager->publishNewSubmission($form, $submission->getUserId()); } catch (\Exception $e) { @@ -728,12 +727,52 @@ public function notifyNewSubmission(Form $form, Submission $submission): void { ); } + $shares = $this->getShares($form->getId()); + $sharedUserIds = []; foreach ($shares as $share) { if (!in_array(Constants::PERMISSION_RESULTS, $share['permissions'])) { continue; } try { - $this->activityManager->publishNewSharedSubmission($form, $share['shareType'], $share['shareWith'], $submission->getUserId()); + switch ($share['shareType']) { + case IShare::TYPE_USER: + $sharedUserIds[] = $share['shareWith']; + break; + case IShare::TYPE_GROUP: + $group = $this->groupManager->get($share['shareWith']); + if ($group !== null) { + foreach ($group->getUsers() as $user) { + $sharedUserIds[] = $user->getUID(); + } + } + break; + case IShare::TYPE_CIRCLE: + $users = $this->circlesService->getCircleUsers($share['shareWith']); + foreach ($users as $userId) { + $sharedUserIds[] = $userId; + } + break; + default: + // ignore other share types + } + } catch (\Exception $e) { + // Handle exceptions silently, as this is not critical. + // We don't want to break the submission process just because of an activity error. + $this->logger->error( + 'Error while resolving users', + [$e] + ); + } + } + + // Deduplicate and exclude form owner to avoid duplicates + $sharedUserIds = array_unique(array_filter($sharedUserIds, function (string $userId) use ($form) { + return $userId !== $form->getOwnerId(); + })); + + foreach ($sharedUserIds as $userId) { + try { + $this->activityManager->publishNewSharedSubmission($form, $userId, $submission->getUserId()); } catch (\Exception $e) { // Handle exceptions silently, as this is not critical. // We don't want to break the submission process just because of an activity error. diff --git a/tests/Unit/Activity/ActivityManagerTest.php b/tests/Unit/Activity/ActivityManagerTest.php index 259bd55ee..548a882ca 100644 --- a/tests/Unit/Activity/ActivityManagerTest.php +++ b/tests/Unit/Activity/ActivityManagerTest.php @@ -247,48 +247,38 @@ public function testPublishNewSharedSubmission(int $shareType, string $shareWith $form->setOwnerId('formOwner'); $submitterId = 'submittingUser'; - if ($shareType === IShare::TYPE_CIRCLE) { - $this->circlesService->expects($this->once()) - ->method('getCircleUsers') - ->with('sharedCircle') - ->willReturn($sharedUsers); - } elseif ($shareType === IShare::TYPE_GROUP) { - $users = array_map(function ($name) { - $user = $this->createMock(IUser::class); - $user->expects($this->once())->method('getUID')->willReturn($name); - return $user; - }, $sharedUsers); - $group = $this->createMock(IGroup::class); - $group->expects($this->once())->method('getUsers')->willReturn($users); - $this->groupManager->expects($this->once())->method('get')->willReturn($group); - } - $event = $this->createMock(IEvent::class); - $this->manager->expects($this->exactly(count($expected))) + $expectedCount = count($sharedUsers ?? []); + + $this->manager->expects($this->exactly($expectedCount)) ->method('generateEvent') ->willReturn($event); - $event->expects($this->exactly(count($expected)))->method('setApp')->with('forms')->willReturn($event); - $event->expects($this->exactly(count($expected)))->method('setType')->with('forms_newsharedsubmission')->willReturn($event); - $event->expects($this->exactly(count($expected)))->method('setAuthor')->with('submittingUser')->willReturn($event); - $event->expects($this->exactly(count($expected)))->method('setObject')->with('form', 5)->willReturn($event); - $event->expects($this->exactly(count($expected)))->method('setSubject')->with('newsubmission', [ + $event->expects($this->exactly($expectedCount))->method('setApp')->with('forms')->willReturn($event); + $event->expects($this->exactly($expectedCount))->method('setType')->with('forms_newsharedsubmission')->willReturn($event); + $event->expects($this->exactly($expectedCount))->method('setAuthor')->with('submittingUser')->willReturn($event); + $event->expects($this->exactly($expectedCount))->method('setObject')->with('form', 5)->willReturn($event); + $event->expects($this->exactly($expectedCount))->method('setSubject')->with('newsubmission', [ 'userId' => 'submittingUser', 'formTitle' => 'TestForm-Title', 'formHash' => 'abcdefg12345' ])->willReturn($event); $affectedUsers = []; - $event->expects($this->exactly(count($sharedUsers))) + $event->expects($this->exactly($expectedCount)) ->method('setAffectedUser') ->willReturnCallback(function (string $userId) use (&$affectedUsers, &$event) { $affectedUsers[] = $userId; return $event; }); - $this->manager->expects($this->exactly(count($expected))) + $this->manager->expects($this->exactly($expectedCount)) ->method('publish') ->with($event); - $this->activityManager->publishNewSharedSubmission($form, $shareType, $shareWith, $submitterId); - $this->assertEquals($sharedUsers, $affectedUsers); + // Call per-user publisher for each expected shared user (new API) + foreach ($sharedUsers ?? [] as $userId) { + $this->activityManager->publishNewSharedSubmission($form, $userId, $submitterId); + } + + $this->assertEquals($sharedUsers ?? [], $affectedUsers); } } diff --git a/tests/Unit/Service/FormsServiceTest.php b/tests/Unit/Service/FormsServiceTest.php index 10d7e7024..33d6762d2 100644 --- a/tests/Unit/Service/FormsServiceTest.php +++ b/tests/Unit/Service/FormsServiceTest.php @@ -1165,9 +1165,46 @@ public static function dataNotifyNewSubmission() { 'shareWith' => 'user2', 'shareType' => IShare::TYPE_USER, 'permissions' => [ Constants::PERMISSION_RESULTS ] - ]], + ] + ], 1 ], + 'group-share' => [ + [[ + 'shareWith' => 'sharedGroup', + 'shareType' => IShare::TYPE_GROUP, + 'permissions' => [ Constants::PERMISSION_RESULTS ] + ]], + 2 + ], + 'circle-share' => [ + [[ + 'shareWith' => 'sharedCircle', + 'shareType' => IShare::TYPE_CIRCLE, + 'permissions' => [ Constants::PERMISSION_RESULTS ] + ]], + 2 + ], + 'overlapping-share' => [ + [ + [ + 'shareWith' => 'user2', + 'shareType' => IShare::TYPE_USER, + 'permissions' => [ Constants::PERMISSION_RESULTS ] + ], + [ + 'shareWith' => 'sharedGroup', + 'shareType' => IShare::TYPE_GROUP, + 'permissions' => [ Constants::PERMISSION_RESULTS ] + ], + [ + 'shareWith' => 'sharedCircle', + 'shareType' => IShare::TYPE_CIRCLE, + 'permissions' => [ Constants::PERMISSION_RESULTS ] + ] + ], + 2 + ] ]; } @@ -1217,6 +1254,24 @@ public function testNotifyNewSubmission($shares, $shareNotifications) { ->method('publishNewSubmission') ->with($form, $submitter); + // If shares contain group or circle entries, mock resolution to user ids + foreach ($shares as $share) { + if (($share['shareType'] ?? null) === IShare::TYPE_GROUP) { + $sharedUsers = ['ownerUser', 'user1', 'user2']; + $users = array_map(function ($name) { + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn($name); + return $user; + }, $sharedUsers); + $group = $this->createMock(IGroup::class); + $group->method('getUsers')->willReturn($users); + $this->groupManager->expects($this->once())->method('get')->with($share['shareWith'])->willReturn($group); + } + if (($share['shareType'] ?? null) === IShare::TYPE_CIRCLE) { + $this->circlesService->expects($this->once())->method('getCircleUsers')->with($share['shareWith'])->willReturn(['ownerUser', 'user1', 'user2']); + } + } + $this->activityManager->expects($this->exactly($shareNotifications)) ->method('publishNewSharedSubmission');