feat: activity support #2002

luka-nextcloud · 2025-08-15T08:32:50Z

Resolves: #1897

Koc · 2025-08-15T21:04:01Z

can you please add import support 🙏? It can helps me a lot for #1801

enjeck

Not sure if this is intentional or not, but if you change a column name, the activity log that references the column still uses the old name which can be confusing

luka-nextcloud · 2025-08-21T08:30:17Z

Not sure if this is intentional or not, but if you change a column name, the activity log that references the column still uses the old name which can be confusing

@enjeck Since we use the activity log to track the object's history, I think we should respect the column name at the time the action was logged

enjeck · 2025-08-26T06:05:14Z

@enjeck Since we use the activity log to track the object's history, I think we should respect the column name at the time the action was logged

I agree!

enjeck · 2025-09-01T06:58:04Z

lib/Activity/ActivityManager.php

+		switch ($subjectIdentifier) {
+			case self::SUBJECT_TABLE_CREATE:
+				$subject = $ownActivity ? $l->t('You have created a new table {table}'): $l->t('{user} has created a new table {table}');
+				break;


Is this the right way to deal with translations with params? I figured it would be something using square brackets e.g [table], as described in https://docs.nextcloud.com/server/latest/developer_manual/basics/translations.html#php-backend.

enjeck

@luka-nextcloud Are you done here? Because from reading #1897, there are some requirements listed there and I wonder if we will be covering them all here

luka-nextcloud · 2025-09-03T15:36:24Z

@luka-nextcloud Are you done here? Because from reading #1897, there are some requirements listed there and I wonder if we will be covering them all here

@enjeck In my opinion, we are done for now. The remaining requirement is clean-up schedule (number 5 and 6) which I think should be discussed and done from the activity app.

Signed-off-by: Luka Trovic <[email protected]>

blizzz

Great effort @luka-nextcloud 🙇 Everything looks good and works (two glitches, nothing big). Happy about this!

There are quick formal matters: please add declare(strict_types=1); to all new classes and add type hints and return types to the methods that do not have them (typically missing in some of the new classes).

And smaller functional cases:

table deletion activity did not show up for me (owner of the table was someone else), but only for this someone else.
Renaming a table, and Description update, have an issue with activity formatting or so: Rich subject or a parameter for "{user} has renamed the table {before} to {table}" is malformed

Backend-wise all is good. The frontend-usage is nice as well, I didn't check the code there though. If the items mentioned above can be addressed, green lights from my side.

blizzz · 2025-09-12T17:54:41Z

lib/Activity/ActivityManager.php

+		private IManager $manager,
+		private IFactory $l10nFactory,
+		private TableMapper $tableMapper,
+		private Row2Mapper $rowMapper,


blizzz · 2025-09-12T17:55:39Z

lib/Activity/ActivityManager.php

+	public const SUBJECT_PARAMS_MAX_LENGTH = 4000;
+	public const SHORTENED_MAX_LENGTH = 2000;
+	public const ROW_NAME_MAX_LENGTH = 20;


seem to be unusued?

blizzz · 2025-09-12T18:03:43Z

lib/Activity/Filter.php

+	private $l10n;
+	private $urlGenerator;


provide type or use property promotion

blizzz · 2025-09-12T18:05:23Z

lib/Activity/SettingChanges.php

+	/** @var IL10N */
+	protected $l;
+
+	/**
+	 * @param IL10N $l
+	 */
+	public function __construct(IL10N $l) {
+		$this->l = $l;
+	}


Suggested change

/** @var IL10N */

protected $l;

/**

* @param IL10N $l

*/

public function __construct(IL10N $l) {

$this->l = $l;

}

public function __construct(protected IL10N $l) {

}

blizzz · 2025-09-12T18:07:37Z

lib/Activity/TablesProvider.php

+	) {
+	}
+
+	public function parse($language, IEvent $event, ?IEvent $previousEvent = null): IEvent {


Suggested change

public function parse($language, IEvent $event, ?IEvent $previousEvent = null): IEvent {

public function parse(string $language, IEvent $event, ?IEvent $previousEvent = null): IEvent {

blizzz · 2025-09-12T18:10:11Z

lib/Helper/CircleHelper.php

+				return [];
+			}
+			$members = $circle->getMembers();
+			return array_map(fn ($member) => $member->getUserId(), $members);


Suggested change

return array_map(fn ($member) => $member->getUserId(), $members);

return array_map(static fn ($member) => $member->getUserId(), $members);

blizzz · 2025-09-12T18:14:43Z

lib/Service/ShareService.php

+			/** @var Share $share */
+			foreach ($shares as $share) {
+				if ($share->getReceiverType() === ShareReceiverType::USER) {
+					$sharedWithUserIds[] = $share->getReceiver();


consider using the user id as key. Instead of array_unique() later on, only array_keys() can be returned. Then array_flip() may help with array_merge.

There might be a lot of users in the result set, this would take some short cuts in cpu and memory in such cases.

blizzz · 2025-09-12T18:23:42Z

vite.config.ts

-					'micromark-factory-title',
-					'micromark-util-normalize-identifier',
-					'micromark-factory-whitespace',
-                ],


Not sure what this removal is about, but if it is some cleanup of unnecessary things (smells like it!), all cool. I want to double check that this is not something unintended.

blizzz · 2025-09-12T19:01:23Z

lib/Activity/ActivityManager.php

+						$columns .= ', ';
+					}
+				}
+				$subject = $ownActivity ? $l->t('You have updated cell(s) ' . $columns . ' on row {row} in table {table}') : $l->t('{user} has updated cell(s) ' . $columns . ' on row {row} in table {table}');


Suggested change

$subject = $ownActivity ? $l->t('You have updated cell(s) ' . $columns . ' on row {row} in table {table}') : $l->t('{user} has updated cell(s) ' . $columns . ' on row {row} in table {table}');

$subject = $ownActivity ? $l->t('You have updated one or more cells ' . $columns . ' on row {row} in table {table}') : $l->t('{user} has updated one or more cells ' . $columns . ' on row {row} in table {table}');

The bracket is not that natural to read. But if we can figure out whether is one or more we can use the plural functions.

luka-nextcloud force-pushed the activity-support branch from 01bac38 to efc2122 Compare August 15, 2025 18:27

luka-nextcloud marked this pull request as ready for review August 15, 2025 18:27

luka-nextcloud requested review from enjeck and blizzz as code owners August 15, 2025 18:27

enjeck reviewed Aug 20, 2025

View reviewed changes

luka-nextcloud force-pushed the activity-support branch from efc2122 to 85c62c1 Compare August 20, 2025 19:51

luka-nextcloud force-pushed the activity-support branch 2 times, most recently from 99bc822 to 48e8f85 Compare August 27, 2025 14:54

enjeck reviewed Sep 1, 2025

View reviewed changes

feat: activity support

b31e436

Signed-off-by: Luka Trovic <[email protected]>

luka-nextcloud force-pushed the activity-support branch from 48e8f85 to b31e436 Compare September 4, 2025 09:37

blizzz reviewed Sep 12, 2025

View reviewed changes

	public function parse($language, IEvent $event, ?IEvent $previousEvent = null): IEvent {
	public function parse(string $language, IEvent $event, ?IEvent $previousEvent = null): IEvent {

	return array_map(fn ($member) => $member->getUserId(), $members);
	return array_map(static fn ($member) => $member->getUserId(), $members);

	$subject = $ownActivity ? $l->t('You have updated cell(s) ' . $columns . ' on row {row} in table {table}') : $l->t('{user} has updated cell(s) ' . $columns . ' on row {row} in table {table}');
	$subject = $ownActivity ? $l->t('You have updated one or more cells ' . $columns . ' on row {row} in table {table}') : $l->t('{user} has updated one or more cells ' . $columns . ' on row {row} in table {table}');

feat: activity support #2002

Are you sure you want to change the base?

feat: activity support #2002

Uh oh!

Conversation

luka-nextcloud commented Aug 15, 2025

Uh oh!

Koc commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

enjeck left a comment

Choose a reason for hiding this comment

Uh oh!

luka-nextcloud commented Aug 21, 2025

Uh oh!

enjeck commented Aug 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enjeck left a comment

Choose a reason for hiding this comment

Uh oh!

luka-nextcloud commented Sep 3, 2025

Uh oh!

blizzz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Koc commented Aug 15, 2025 •

edited

Loading