Skip to content

notify impersonated user with a notification and an email#84

Open
cagdasbas wants to merge 7 commits intonextcloud:masterfrom
cagdasbas:master
Open

notify impersonated user with a notification and an email#84
cagdasbas wants to merge 7 commits intonextcloud:masterfrom
cagdasbas:master

Conversation

@cagdasbas
Copy link
Copy Markdown

Attempt to solve #80
Notifier and mailer are mostly from quota_warning. A dirty hack but I'm willing to clean up with your feedback.

@nickvergessen
Copy link
Copy Markdown
Member

I think a notification is enough for now.
We want to work a bit on the notifications soon, to allow receiving them as emails. So that would be doubled.

Otherwise I'm fine with the approach

Comment thread l10n/en_GB.json Outdated
Comment thread lib/Notification/Notifier.php Outdated
Comment thread lib/Notification/Notifier.php Outdated
Comment thread img/app-alert.svg Outdated
@cagdasbas
Copy link
Copy Markdown
Author

cagdasbas commented Dec 4, 2019

I think a notification is enough for now.

My thought was the same at first. But as soon as the admin user impersonates another, he/she can delete the notification. The user will only see it if already logged in at the moment. That's why I decided to add email notification.

How about deleting the email and adding an event to the activities of the user?

@nickvergessen
Copy link
Copy Markdown
Member

But as soon as the admin user impersonates another, he/she can delete the notification.

Let me fix that instead.

@cagdasbas
Copy link
Copy Markdown
Author

But as soon as the admin user impersonates another, he/she can delete the notification.

Let me fix that instead.

Sure. I'm removing the email then?

@cagdasbas cagdasbas marked this pull request as ready for review December 10, 2019 21:12
@nickvergessen
Copy link
Copy Markdown
Member

Sure. I'm removing the email then?

Yeah, sorry for the huge delay

@nickvergessen
Copy link
Copy Markdown
Member

Patch for notifications is at nextcloud/notifications#550

Comment thread lib/Controller/SettingsController.php Outdated
Comment thread lib/Controller/SettingsController.php Outdated
Çağdaş Baş and others added 6 commits January 22, 2020 21:50
Signed-off-by: Çağdaş Baş <cagdas@ouva.co>
Signed-off-by: Çağdaş Baş <cagdas@ouva.co>
Signed-off-by: Çağdaş Baş <cagdas@ouva.co>
Signed-off-by: Çağdaş Baş <cagdas@ouva.co>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Çağdaş Baş <cagdas@ouva.co>
Co-Authored-By: Sascha Wiswedel <wiswedel@users.noreply.github.com>
Signed-off-by: Çağdaş Baş <cagdas@ouva.co>
Copy link
Copy Markdown
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • would be good to have the test related changes in a different PR
  • translations do not go in via github, but transifex

Comment thread lib/Controller/SettingsController.php Outdated
Signed-off-by: Çağdaş Baş <cagdasbs@ouva.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants