feat: add notifications#2681
Conversation
luka-nextcloud
force-pushed
the
add-notifications
branch
2 times, most recently
from
57335c5 to
bd02b34
Compare
Signed-off-by: Luka Trovic <luka@nextcloud.com>
luka-nextcloud
force-pushed
the
add-notifications
branch
from
bd02b34 to
453ec95
Compare
blizzz
left a comment
blizzz
left a comment
There was a problem hiding this comment.
Thanks @luka-nextcloud! I did only little smoke testing, but that was already working. I left some comments around here in the code, the ones marked with 💡 are not blocking, but for consideration.
What I also did not check yet, question back for you, do you think this scales with a bigger number of users, or users in a group?
My impression is that in some places there is code that is quite similar, an almost-duplication. There might be potential to reduce this, but do not consider this critical.
| return [ | ||
| { | ||
| key: 'notify-row', | ||
| label: t('tables', 'Row changes'), |
There was a problem hiding this comment.
💄 less technical could be: Content changes?
| }, | ||
| { | ||
| key: 'notify-column', | ||
| label: t('tables', 'Column changes'), |
There was a problem hiding this comment.
💄 Less technical wording could be Structure changes?
| { | ||
| key: 'notify-row', | ||
| label: t('tables', 'Row changes'), | ||
| description: t('tables', 'Notify me when rows are created, updated or deleted.'), |
There was a problem hiding this comment.
Here maybe row values, or just values, or values in a row.
| } | ||
| } | ||
|
|
||
| private function createEvent($objectType, $object, $subject, $additionalParams = [], $author = null) { |
There was a problem hiding this comment.
💡 This method collected more than one hundred lines now. Maybe it can be split up?
| 'id' => 'unknown', | ||
| 'name' => 'unknown', | ||
| 'type' => 'unknown', | ||
| ]; |
There was a problem hiding this comment.
💡 move 'unknown' into a constant, it is used in more places.
| 'columnId' => $columnId, | ||
| 'exception' => $e->getMessage() | ||
| ]); | ||
| continue; // Skip if column not found |
There was a problem hiding this comment.
should not be necessary as last instruction in the for loop?
|
|
||
| try { | ||
| $column = $this->columnMapper->find($columnId); | ||
| $this->columnCache[$columnId] = $column; |
There was a problem hiding this comment.
the columnMapper already has a cache
| if ($receiverId === '' || $receiverId === $authorId || !isset($recipientSet[$receiverId])) { | ||
| continue; | ||
| } | ||
| if (!$this->configService->isNotifyEnabledForScope($receiverId, 'table', $tableId, 'notify-assigned')) { |
There was a problem hiding this comment.
please use constants when values like notify-assigned are used more than once and especially around multiple places. This will save use from errors through type and simplifies refactoring and understanding of the code.
| return false; | ||
| } | ||
|
|
||
| public function set($key, $value) { |
| }, | ||
|
|
||
| computed: { | ||
| // ...mapState(useTablesStore, ['activeElement', 'isView']), |
|
P.S.: what I forgot to missed to add: 💄 better to not include the row ID in the text of the notification, it does not really help people and looks technical. Just link |
enjeck
left a comment
enjeck
left a comment
There was a problem hiding this comment.
not sure if should be done in another PR, but could we have notifications for asynchronous import? Right now, users never know exactly when import is done and it would be great
| 'id' => $column->getId(), | ||
| 'name' => $column->getTitle(), | ||
| 'type' => $column->getType(), |
There was a problem hiding this comment.
$column could be null, so trying to access these properties errors out. I don't think this is caught by following exception
| if (this.objectType === 'table') { | ||
| activities = activities.filter((activity) => { | ||
| return (activity.object_type === 'tables_row' && activity.object_id === this.objectId) | ||
| return (activity.subject_rich[1].table?.id === this.objectId.toString() && !activity.subject_rich[1].view) |
There was a problem hiding this comment.
Is subject_rich[1] guaranteed to exist. Do we need to guard it with subject_rich?.[1]?.table?.id too?
| * @param array<string, mixed> $additionalParams | ||
| * @param mixed $author | ||
| */ | ||
| private function sendRowNotification(Row2 $row, string $subject, array $additionalParams, $author): void { |
There was a problem hiding this comment.
this function iterates viewMapper->findAll($tableId) twice (regular notifications, then assigned), calling findSharedWithUserIds() per view.
For each shared recipient it calls row2Mapper->isRowInViewPresent(), which executes getWantedRowIds(). So a single row edit on a table with V views and U shares can fire on the order of V×U filter evaluation queries. On busy/large tables this will have slow writes?
Can we fetch views once and reuse?
| private IURLGenerator $urlGenerator, | ||
| protected readonly IURLGenerator $url, |
There was a problem hiding this comment.
why are we injecting IURLGenerator twice? Can we use once?
There was a problem hiding this comment.
there is a lot of logic duplication between ActivityManager (view-context resolution, changed-column filtering, shared-user fan-out) and NotificationHelper. If i understand correctly, in both, we want to figure out which views/users are affected by a specific row/column? Can we reuse?
| */ | ||
| public function filterTypes(array $types): array { | ||
| return $types; | ||
| return array_merge($types, ['tables_sharing', 'tables_row_table', 'tables_row_view', 'tables_column_table', 'tables_column_view']); |
There was a problem hiding this comment.
these constants are reused in various places, would be nice to have shared constants/variables
3 participants
Resolves: #1460
Demo:
demo.webm