Skip to content

Commit 794c3f4

Browse files
Chartman123Chartman123
authored
Merge pull request #3363 from nextcloud/fix/notify-submission
fix: only send a single submission notification per user
2 parents d8deb33 + 5d9eed8 commit 794c3f4

4 files changed

Lines changed: 129 additions & 64 deletions

File tree

‎lib/Activity/ActivityManager.php‎

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

1715
class ActivityManager {
1816

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

162-
$this->manager->publish($event);
163-
}
144+
$this->manager->publish($event);
164145
}
165146
}

‎lib/Service/FormsService.php‎

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -713,10 +713,9 @@ public function notifyNewShares(Form $form, Share $share): void {
713713
* Creates activities for new submissions on a form
714714
*
715715
* @param Form $form Related Form
716-
* @param string $submitter The ID of the user who submitted the form. Can also be our 'anon-user-'-ID
716+
* @param Submission $submission the current submission
717717
*/
718718
public function notifyNewSubmission(Form $form, Submission $submission): void {
719-
$shares = $this->getShares($form->getId());
720719
try {
721720
$this->activityManager->publishNewSubmission($form, $submission->getUserId());
722721
} catch (\Exception $e) {
@@ -728,12 +727,52 @@ public function notifyNewSubmission(Form $form, Submission $submission): void {
728727
);
729728
}
730729

730+
$shares = $this->getShares($form->getId());
731+
$sharedUserIds = [];
731732
foreach ($shares as $share) {
732733
if (!in_array(Constants::PERMISSION_RESULTS, $share['permissions'])) {
733734
continue;
734735
}
735736
try {
736-
$this->activityManager->publishNewSharedSubmission($form, $share['shareType'], $share['shareWith'], $submission->getUserId());
737+
switch ($share['shareType']) {
738+
case IShare::TYPE_USER:
739+
$sharedUserIds[] = $share['shareWith'];
740+
break;
741+
case IShare::TYPE_GROUP:
742+
$group = $this->groupManager->get($share['shareWith']);
743+
if ($group !== null) {
744+
foreach ($group->getUsers() as $user) {
745+
$sharedUserIds[] = $user->getUID();
746+
}
747+
}
748+
break;
749+
case IShare::TYPE_CIRCLE:
750+
$users = $this->circlesService->getCircleUsers($share['shareWith']);
751+
foreach ($users as $userId) {
752+
$sharedUserIds[] = $userId;
753+
}
754+
break;
755+
default:
756+
// ignore other share types
757+
}
758+
} catch (\Exception $e) {
759+
// Handle exceptions silently, as this is not critical.
760+
// We don't want to break the submission process just because of an activity error.
761+
$this->logger->error(
762+
'Error while resolving users',
763+
[$e]
764+
);
765+
}
766+
}
767+
768+
// Deduplicate and exclude form owner to avoid duplicates
769+
$sharedUserIds = array_unique(array_filter($sharedUserIds, function (string $userId) use ($form) {
770+
return $userId !== $form->getOwnerId();
771+
}));
772+
773+
foreach ($sharedUserIds as $userId) {
774+
try {
775+
$this->activityManager->publishNewSharedSubmission($form, $userId, $submission->getUserId());
737776
} catch (\Exception $e) {
738777
// Handle exceptions silently, as this is not critical.
739778
// We don't want to break the submission process just because of an activity error.

‎tests/Unit/Activity/ActivityManagerTest.php‎

Lines changed: 16 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -247,48 +247,38 @@ public function testPublishNewSharedSubmission(int $shareType, string $shareWith
247247
$form->setOwnerId('formOwner');
248248
$submitterId = 'submittingUser';
249249

250-
if ($shareType === IShare::TYPE_CIRCLE) {
251-
$this->circlesService->expects($this->once())
252-
->method('getCircleUsers')
253-
->with('sharedCircle')
254-
->willReturn($sharedUsers);
255-
} elseif ($shareType === IShare::TYPE_GROUP) {
256-
$users = array_map(function ($name) {
257-
$user = $this->createMock(IUser::class);
258-
$user->expects($this->once())->method('getUID')->willReturn($name);
259-
return $user;
260-
}, $sharedUsers);
261-
$group = $this->createMock(IGroup::class);
262-
$group->expects($this->once())->method('getUsers')->willReturn($users);
263-
$this->groupManager->expects($this->once())->method('get')->willReturn($group);
264-
}
265-
266250
$event = $this->createMock(IEvent::class);
267-
$this->manager->expects($this->exactly(count($expected)))
251+
$expectedCount = count($sharedUsers ?? []);
252+
253+
$this->manager->expects($this->exactly($expectedCount))
268254
->method('generateEvent')
269255
->willReturn($event);
270-
$event->expects($this->exactly(count($expected)))->method('setApp')->with('forms')->willReturn($event);
271-
$event->expects($this->exactly(count($expected)))->method('setType')->with('forms_newsharedsubmission')->willReturn($event);
272-
$event->expects($this->exactly(count($expected)))->method('setAuthor')->with('submittingUser')->willReturn($event);
273-
$event->expects($this->exactly(count($expected)))->method('setObject')->with('form', 5)->willReturn($event);
274-
$event->expects($this->exactly(count($expected)))->method('setSubject')->with('newsubmission', [
256+
$event->expects($this->exactly($expectedCount))->method('setApp')->with('forms')->willReturn($event);
257+
$event->expects($this->exactly($expectedCount))->method('setType')->with('forms_newsharedsubmission')->willReturn($event);
258+
$event->expects($this->exactly($expectedCount))->method('setAuthor')->with('submittingUser')->willReturn($event);
259+
$event->expects($this->exactly($expectedCount))->method('setObject')->with('form', 5)->willReturn($event);
260+
$event->expects($this->exactly($expectedCount))->method('setSubject')->with('newsubmission', [
275261
'userId' => 'submittingUser',
276262
'formTitle' => 'TestForm-Title',
277263
'formHash' => 'abcdefg12345'
278264
])->willReturn($event);
279265
$affectedUsers = [];
280-
$event->expects($this->exactly(count($sharedUsers)))
266+
$event->expects($this->exactly($expectedCount))
281267
->method('setAffectedUser')
282268
->willReturnCallback(function (string $userId) use (&$affectedUsers, &$event) {
283269
$affectedUsers[] = $userId;
284270
return $event;
285271
});
286272

287-
$this->manager->expects($this->exactly(count($expected)))
273+
$this->manager->expects($this->exactly($expectedCount))
288274
->method('publish')
289275
->with($event);
290276

291-
$this->activityManager->publishNewSharedSubmission($form, $shareType, $shareWith, $submitterId);
292-
$this->assertEquals($sharedUsers, $affectedUsers);
277+
// Call per-user publisher for each expected shared user (new API)
278+
foreach ($sharedUsers ?? [] as $userId) {
279+
$this->activityManager->publishNewSharedSubmission($form, $userId, $submitterId);
280+
}
281+
282+
$this->assertEquals($sharedUsers ?? [], $affectedUsers);
293283
}
294284
}

‎tests/Unit/Service/FormsServiceTest.php‎

Lines changed: 56 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1165,9 +1165,46 @@ public static function dataNotifyNewSubmission() {
11651165
'shareWith' => 'user2',
11661166
'shareType' => IShare::TYPE_USER,
11671167
'permissions' => [ Constants::PERMISSION_RESULTS ]
1168-
]],
1168+
]
1169+
],
11691170
1
11701171
],
1172+
'group-share' => [
1173+
[[
1174+
'shareWith' => 'sharedGroup',
1175+
'shareType' => IShare::TYPE_GROUP,
1176+
'permissions' => [ Constants::PERMISSION_RESULTS ]
1177+
]],
1178+
2
1179+
],
1180+
'circle-share' => [
1181+
[[
1182+
'shareWith' => 'sharedCircle',
1183+
'shareType' => IShare::TYPE_CIRCLE,
1184+
'permissions' => [ Constants::PERMISSION_RESULTS ]
1185+
]],
1186+
2
1187+
],
1188+
'overlapping-share' => [
1189+
[
1190+
[
1191+
'shareWith' => 'user2',
1192+
'shareType' => IShare::TYPE_USER,
1193+
'permissions' => [ Constants::PERMISSION_RESULTS ]
1194+
],
1195+
[
1196+
'shareWith' => 'sharedGroup',
1197+
'shareType' => IShare::TYPE_GROUP,
1198+
'permissions' => [ Constants::PERMISSION_RESULTS ]
1199+
],
1200+
[
1201+
'shareWith' => 'sharedCircle',
1202+
'shareType' => IShare::TYPE_CIRCLE,
1203+
'permissions' => [ Constants::PERMISSION_RESULTS ]
1204+
]
1205+
],
1206+
2
1207+
]
11711208
];
11721209
}
11731210

@@ -1217,6 +1254,24 @@ public function testNotifyNewSubmission($shares, $shareNotifications) {
12171254
->method('publishNewSubmission')
12181255
->with($form, $submitter);
12191256

1257+
// If shares contain group or circle entries, mock resolution to user ids
1258+
foreach ($shares as $share) {
1259+
if (($share['shareType'] ?? null) === IShare::TYPE_GROUP) {
1260+
$sharedUsers = ['ownerUser', 'user1', 'user2'];
1261+
$users = array_map(function ($name) {
1262+
$user = $this->createMock(IUser::class);
1263+
$user->method('getUID')->willReturn($name);
1264+
return $user;
1265+
}, $sharedUsers);
1266+
$group = $this->createMock(IGroup::class);
1267+
$group->method('getUsers')->willReturn($users);
1268+
$this->groupManager->expects($this->once())->method('get')->with($share['shareWith'])->willReturn($group);
1269+
}
1270+
if (($share['shareType'] ?? null) === IShare::TYPE_CIRCLE) {
1271+
$this->circlesService->expects($this->once())->method('getCircleUsers')->with($share['shareWith'])->willReturn(['ownerUser', 'user1', 'user2']);
1272+
}
1273+
}
1274+
12201275
$this->activityManager->expects($this->exactly($shareNotifications))
12211276
->method('publishNewSharedSubmission');
12221277

0 commit comments

Comments
 (0)