Skip to content

Commit b187d57

Browse files
Chartman123Chartman123
committed
fix: only send one submission notification per user
Signed-off-by: Christian Hartmann <chris-hartmann@gmx.de>
1 parent d37f2bf commit b187d57

4 files changed

Lines changed: 134 additions & 64 deletions

File tree

‎lib/Activity/ActivityManager.php‎

Lines changed: 15 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,6 @@
1212
use OCA\Forms\Service\CirclesService;
1313
use OCP\Activity\IManager;
1414
use OCP\IGroupManager;
15-
use OCP\IUser;
16-
use OCP\Share\IShare;
1715

1816
class ActivityManager {
1917

@@ -128,39 +126,22 @@ public function publishNewSubmission(Form $form, string $submitterID): void {
128126
* Publish a new-Submission Activity for shared forms
129127
*
130128
* @param Form $form The affected Form
131-
* @param string $submitterID ID of the User who submitted the form. Can also be our 'anon-user-'-ID
129+
* @param string $userId ID of the user to notify
130+
* @param string $submitterID ID of the user who submitted the form
132131
*/
133-
public function publishNewSharedSubmission(Form $form, int $shareType, string $shareWith, string $submitterID): void {
134-
$users = [];
135-
switch ($shareType) {
136-
case IShare::TYPE_USER:
137-
$users[] = $shareWith;
138-
break;
139-
case IShare::TYPE_GROUP:
140-
$group = $this->groupManager->get($shareWith);
141-
if ($group !== null) {
142-
$users = array_map(fn (IUser $user) => $user->getUID(), $group->getUsers());
143-
}
144-
break;
145-
case IShare::TYPE_CIRCLE:
146-
$users = $this->circlesService->getCircleUsers($shareWith);
147-
break;
148-
}
149-
150-
foreach ($users as $userId) {
151-
$event = $this->manager->generateEvent();
152-
$event->setApp($this->appName)
153-
->setType(ActivityConstants::TYPE_NEWSHAREDSUBMISSION)
154-
->setAffectedUser($userId)
155-
->setAuthor($submitterID)
156-
->setSubject(ActivityConstants::SUBJECT_NEWSUBMISSION, [
157-
'userId' => $submitterID,
158-
'formTitle' => $form->getTitle(),
159-
'formHash' => $form->getHash()
160-
])
161-
->setObject('form', $form->getId());
132+
public function publishNewSharedSubmission(Form $form, string $userId, string $submitterID): void {
133+
$event = $this->manager->generateEvent();
134+
$event->setApp($this->appName)
135+
->setType(ActivityConstants::TYPE_NEWSHAREDSUBMISSION)
136+
->setAffectedUser($userId)
137+
->setAuthor($submitterID)
138+
->setSubject(ActivityConstants::SUBJECT_NEWSUBMISSION, [
139+
'userId' => $submitterID,
140+
'formTitle' => $form->getTitle(),
141+
'formHash' => $form->getHash()
142+
])
143+
->setObject('form', $form->getId());
162144

163-
$this->manager->publish($event);
164-
}
145+
$this->manager->publish($event);
165146
}
166147
}

‎lib/Service/FormsService.php‎

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -703,10 +703,9 @@ public function notifyNewShares(Form $form, Share $share): void {
703703
* Creates activities for new submissions on a form
704704
*
705705
* @param Form $form Related Form
706-
* @param string $submitter The ID of the user who submitted the form. Can also be our 'anon-user-'-ID
706+
* @param Submission $submission the current submission
707707
*/
708708
public function notifyNewSubmission(Form $form, Submission $submission): void {
709-
$shares = $this->getShares($form->getId());
710709
try {
711710
$this->activityManager->publishNewSubmission($form, $submission->getUserId());
712711
} catch (\Exception $e) {
@@ -718,12 +717,52 @@ public function notifyNewSubmission(Form $form, Submission $submission): void {
718717
);
719718
}
720719

720+
$shares = $this->getShares($form->getId());
721+
$sharedUserIds = [];
721722
foreach ($shares as $share) {
722723
if (!in_array(Constants::PERMISSION_RESULTS, $share['permissions'])) {
723724
continue;
724725
}
725726
try {
726-
$this->activityManager->publishNewSharedSubmission($form, $share['shareType'], $share['shareWith'], $submission->getUserId());
727+
switch ($share['shareType']) {
728+
case IShare::TYPE_USER:
729+
$sharedUserIds[] = $share['shareWith'];
730+
break;
731+
case IShare::TYPE_GROUP:
732+
$group = $this->groupManager->get($share['shareWith']);
733+
if ($group !== null) {
734+
foreach ($group->getUsers() as $user) {
735+
$sharedUserIds[] = $user->getUID();
736+
}
737+
}
738+
break;
739+
case IShare::TYPE_CIRCLE:
740+
$users = $this->circlesService->getCircleUsers($share['shareWith']);
741+
foreach ($users as $userId) {
742+
$sharedUserIds[] = $userId;
743+
}
744+
break;
745+
default:
746+
// ignore other share types
747+
}
748+
} catch (\Exception $e) {
749+
// Handle exceptions silently, as this is not critical.
750+
// We don't want to break the submission process just because of an activity error.
751+
$this->logger->error(
752+
'Error while resolving users',
753+
[$e]
754+
);
755+
}
756+
}
757+
758+
// Deduplicate and exclude form owner to avoid duplicates
759+
$sharedUserIds = array_unique(array_filter($sharedUserIds, function (string $userId) use ($form) {
760+
return $userId !== $form->getOwnerId();
761+
}));
762+
763+
foreach ($sharedUserIds as $userId) {
764+
try {
765+
$this->activityManager->publishNewSharedSubmission($form, $userId, $submission->getUserId());
727766
} catch (\Exception $e) {
728767
// Handle exceptions silently, as this is not critical.
729768
// We don't want to break the submission process just because of an activity error.

‎tests/Unit/Activity/ActivityManagerTest.php‎

Lines changed: 23 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -233,41 +233,38 @@ public function testPublishNewSharedSubmission(int $shareType, string $shareWith
233233
$form->setOwnerId('formOwner');
234234
$submitterId = 'submittingUser';
235235

236-
if ($shareType === IShare::TYPE_CIRCLE) {
237-
$this->circlesService->expects($this->once())
238-
->method('getCircleUsers')
239-
->with('sharedCircle')
240-
->willReturn($sharedUsers);
241-
} elseif ($shareType === IShare::TYPE_GROUP) {
242-
$users = array_map(function ($name) {
243-
$user = $this->createMock(IUser::class);
244-
$user->expects($this->once())->method('getUID')->willReturn($name);
245-
return $user;
246-
}, $sharedUsers);
247-
$group = $this->createMock(IGroup::class);
248-
$group->expects($this->once())->method('getUsers')->willReturn($users);
249-
$this->groupManager->expects($this->once())->method('get')->willReturn($group);
250-
}
251-
252236
$event = $this->createMock(IEvent::class);
253-
$this->manager->expects($this->exactly(count($expected)))
237+
$expectedCount = count($sharedUsers ?? []);
238+
239+
$this->manager->expects($this->exactly($expectedCount))
254240
->method('generateEvent')
255241
->willReturn($event);
256-
$event->expects($this->exactly(count($expected)))->method('setApp')->with('forms')->willReturn($event);
257-
$event->expects($this->exactly(count($expected)))->method('setType')->with('forms_newsharedsubmission')->willReturn($event);
258-
$event->expects($this->exactly(count($expected)))->method('setAffectedUser')->withConsecutive(...[...$expected])->willReturn($event);
259-
$event->expects($this->exactly(count($expected)))->method('setAuthor')->with('submittingUser')->willReturn($event);
260-
$event->expects($this->exactly(count($expected)))->method('setObject')->with('form', 5)->willReturn($event);
261-
$event->expects($this->exactly(count($expected)))->method('setSubject')->with('newsubmission', [
242+
$event->expects($this->exactly($expectedCount))->method('setApp')->with('forms')->willReturn($event);
243+
$event->expects($this->exactly($expectedCount))->method('setType')->with('forms_newsharedsubmission')->willReturn($event);
244+
$event->expects($this->exactly($expectedCount))->method('setAuthor')->with('submittingUser')->willReturn($event);
245+
$event->expects($this->exactly($expectedCount))->method('setObject')->with('form', 5)->willReturn($event);
246+
$event->expects($this->exactly($expectedCount))->method('setSubject')->with('newsubmission', [
262247
'userId' => 'submittingUser',
263248
'formTitle' => 'TestForm-Title',
264249
'formHash' => 'abcdefg12345'
265250
])->willReturn($event);
266-
267-
$this->manager->expects($this->exactly(count($expected)))
251+
$affectedUsers = [];
252+
$event->expects($this->exactly($expectedCount))
253+
->method('setAffectedUser')
254+
->willReturnCallback(function (string $userId) use (&$affectedUsers, &$event) {
255+
$affectedUsers[] = $userId;
256+
return $event;
257+
});
258+
259+
$this->manager->expects($this->exactly($expectedCount))
268260
->method('publish')
269261
->with($event);
270262

271-
$this->activityManager->publishNewSharedSubmission($form, $shareType, $shareWith, $submitterId);
263+
// Call per-user publisher for each expected shared user (new API)
264+
foreach ($sharedUsers ?? [] as $userId) {
265+
$this->activityManager->publishNewSharedSubmission($form, $userId, $submitterId);
266+
}
267+
268+
$this->assertEquals($sharedUsers ?? [], $affectedUsers);
272269
}
273270
}

‎tests/Unit/Service/FormsServiceTest.php‎

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1193,9 +1193,44 @@ public function dataNotifyNewSubmission() {
11931193
'shareWith' => 'user2',
11941194
'shareType' => IShare::TYPE_USER,
11951195
'permissions' => [ Constants::PERMISSION_RESULTS ]
1196-
]],
1196+
]
1197+
],
11971198
1
11981199
],
1200+
'group-share' => [
1201+
[[
1202+
'shareWith' => 'sharedGroup',
1203+
'shareType' => IShare::TYPE_GROUP,
1204+
'permissions' => [ Constants::PERMISSION_RESULTS ]
1205+
]],
1206+
2
1207+
],
1208+
'circle-share' => [
1209+
[[
1210+
'shareWith' => 'sharedCircle',
1211+
'shareType' => IShare::TYPE_CIRCLE,
1212+
'permissions' => [ Constants::PERMISSION_RESULTS ]
1213+
]],
1214+
2
1215+
],
1216+
'overlapping-share' => [
1217+
[[
1218+
'shareWith' => 'user2',
1219+
'shareType' => IShare::TYPE_USER,
1220+
'permissions' => [ Constants::PERMISSION_RESULTS ]
1221+
],
1222+
[
1223+
'shareWith' => 'sharedGroup',
1224+
'shareType' => IShare::TYPE_GROUP,
1225+
'permissions' => [ Constants::PERMISSION_RESULTS ]
1226+
],
1227+
[
1228+
'shareWith' => 'sharedCircle',
1229+
'shareType' => IShare::TYPE_CIRCLE,
1230+
'permissions' => [ Constants::PERMISSION_RESULTS ]
1231+
]],
1232+
2
1233+
]
11991234
];
12001235
}
12011236

@@ -1246,6 +1281,24 @@ public function testNotifyNewSubmission($shares, $shareNotifications) {
12461281
->method('publishNewSubmission')
12471282
->with($form, $submitter);
12481283

1284+
// If shares contain group or circle entries, mock resolution to user ids
1285+
foreach ($shares as $share) {
1286+
if (($share['shareType'] ?? null) === IShare::TYPE_GROUP) {
1287+
$sharedUsers = ['ownerUser', 'user1', 'user2'];
1288+
$users = array_map(function ($name) {
1289+
$user = $this->createMock(IUser::class);
1290+
$user->method('getUID')->willReturn($name);
1291+
return $user;
1292+
}, $sharedUsers);
1293+
$group = $this->createMock(IGroup::class);
1294+
$group->method('getUsers')->willReturn($users);
1295+
$this->groupManager->expects($this->once())->method('get')->with($share['shareWith'])->willReturn($group);
1296+
}
1297+
if (($share['shareType'] ?? null) === IShare::TYPE_CIRCLE) {
1298+
$this->circlesService->expects($this->once())->method('getCircleUsers')->with($share['shareWith'])->willReturn(['ownerUser', 'user1', 'user2']);
1299+
}
1300+
}
1301+
12491302
$this->activityManager->expects($this->exactly($shareNotifications))
12501303
->method('publishNewSharedSubmission');
12511304

0 commit comments

Comments
 (0)