feat: add notifications#2681
Conversation
57335c5 to
bd02b34
Compare
bd02b34 to
453ec95
Compare
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.
|
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
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
453ec95 to
606faaa
Compare
I think the asynchronous import notification should be another PR since this PR is already big enough. |
Signed-off-by: Luka Trovic <luka@nextcloud.com>
606faaa to
91c815b
Compare
blizzz
left a comment
There was a problem hiding this comment.
When both content notifications and assignment notifications are enabled, it leads to two notifications. Imo this is not necessary and the assigned one is already sufficient. Can we only emit this one, if otherwise two would be send?
And one thing design-wise. The settings are not so easy to find, and table managers do not know that they are not global settings:
| /** | ||
| * @return string|bool | ||
| */ | ||
| public function get(string $key) { |
There was a problem hiding this comment.
| /** | |
| * @return string|bool | |
| */ | |
| public function get(string $key) { | |
| public function get(string $key): string|bool { |
| /** | ||
| * Set a configuration value for the current user | ||
| * | ||
| * @param string $key | ||
| * @param mixed $value | ||
| * @return void | ||
| * @throws BadRequestError | ||
| * @throws PermissionError | ||
| */ | ||
| public function set(string $key, mixed $value) { |
There was a problem hiding this comment.
| /** | |
| * Set a configuration value for the current user | |
| * | |
| * @param string $key | |
| * @param mixed $value | |
| * @return void | |
| * @throws BadRequestError | |
| * @throws PermissionError | |
| */ | |
| public function set(string $key, mixed $value) { | |
| /** | |
| * Set a configuration value for the current user | |
| * | |
| * @throws BadRequestError | |
| * @throws PermissionError | |
| */ | |
| public function set(string $key, mixed $value): void { |
| ? $l->t('{user} has updated {row} in view {view}') | ||
| : $l->t('{user} has updated {row} in table {table}'); |
There was a problem hiding this comment.
| ? $l->t('{user} has updated {row} in view {view}') | |
| : $l->t('{user} has updated {row} in table {table}'); | |
| ? $l->t('{user} has updated this {row} in view {view}') | |
| : $l->t('{user} has updated this {row} in table {table}'); |
| ? $l->t('{user} has deleted the {row} in view {view}') | ||
| : $l->t('{user} has deleted the {row} in table {table}'); |
There was a problem hiding this comment.
| ? $l->t('{user} has deleted the {row} in view {view}') | |
| : $l->t('{user} has deleted the {row} in table {table}'); | |
| ? $l->t('{user} has deleted this {row} in view {view}') | |
| : $l->t('{user} has deleted this {row} in table {table}'); |
Resolves: #1460
Demo:
demo.webm