Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewire button to notify users for local shares to use notifications API #32494

Closed
PVince81 opened this issue Aug 28, 2018 · 7 comments · Fixed by #33180
Closed

Rewire button to notify users for local shares to use notifications API #32494

PVince81 opened this issue Aug 28, 2018 · 7 comments · Fixed by #33180
Assignees
Milestone

Comments

@PVince81
Copy link
Contributor

Since recent OC versions (10.0.8 or 10.0.9), an email is already sent to recipients through the OC notifications framework. This obsoletes the need for this button.

Would be good if we could remove it at least in OC 11 as it would save a lot of refactoring work that is needed to remove the old Sharing 1.0 code. See #26608

@pmaier1

@PVince81
Copy link
Contributor Author

I remember a discussion with @pmaier1 where it was said that we should not remove the button.

However would could maybe rewire it to send a notification through the notification framework instead of a direct email.

@VicDeo
Copy link
Member

VicDeo commented Oct 11, 2018

@PVince81 What's proposed

  • add a new endpoint into files_sharing to send the notification via OCP\Notification\IManager OCA\Files_Sharing\Controllers\ShareController new controller is needed
  • rewire the button to this endpoint and reword the text into 'Notify user'
    is it ok?

@PVince81 PVince81 modified the milestones: backlog, development Oct 12, 2018
@PVince81 PVince81 changed the title Remove button to notify users for local shares Rewire button to notify users for local shares to use notifications API Oct 12, 2018
@PVince81
Copy link
Contributor Author

actually this is the current controller https://github.com/owncloud/core/blob/v10.0.10/apps/files_sharing/lib/API/Share20OCS.php#L51 🙈

one day we should convert this to OCSController...

I suggest adding a method there, it needs to be a public API (OCS) so we can reuse it from Phoenix later on

@PVince81
Copy link
Contributor Author

at first I thought just rewire the "MailNotifications" but you're right that we need to think further and moving to a controller method is the right way to go

@VicDeo
Copy link
Member

VicDeo commented Oct 15, 2018

@PVince81 one day is now :)
#33180

@VicDeo VicDeo mentioned this issue Oct 15, 2018
14 tasks
@VicDeo
Copy link
Member

VicDeo commented Oct 15, 2018

@PVince81 if we are removing mail notification for sharing in favor of activity emails \OC\Share\MailNotifications should die along with mail templates core/templates/mail.php and core/templates/altmail.php
\OC\Share\MailNotifications is used in core/ajax/share.php only

@PVince81
Copy link
Contributor Author

@VicDeo the mail notification for link shares must stay. It's only the ones for local shares that would be moved to notifications, since local users can receive those.

@PVince81 PVince81 added the v11 label Oct 18, 2018
@PVince81 PVince81 modified the milestones: development, QA Jan 11, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants