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

Shared File Notifications #4499

Closed
MTRichards opened this issue Aug 18, 2013 · 19 comments
Closed

Shared File Notifications #4499

MTRichards opened this issue Aug 18, 2013 · 19 comments

Comments

@MTRichards
Copy link
Contributor

As a user, I want the option to send an email when I share a folder or file with another user of ownCloud

In the share dialog, have a checkbox that says “notify user by email” on a per user/group (line in the share dialog) and an email is sent to the user or group that the file or folder has been shared with them.
(This shared notification will also be entered in the activity feed)

User receives an email, which includes:

  • The name of the user that shared the file
  • The name of the file or folder
  • The CRUDS privileges they have
  • The expiration
  • The path to the file in owncloud (web/desktop/mobile), and a link to the file or folder shared for the browser
    Shared file notification emails are enabled via a setting in the admin panel, “enable email notifications”

The notification requires email to be set up to send from ownCloud

When a group is selected, all members receive an email if the “send email notification” box is checked

If some of the members of the group do not have an email configured, an error is shown after the share email is sent “some groups members have no email and will not receive notification”

Email text has to be editable, stored with other email footers in lib/defaults.php

#3791 For more on themable strings

@schiessle
Copy link
Contributor

I'm started to work on it. The only problem I see at the moment: We don't know when the user checks the "notify user by email" checkbox. We can't decide if we already have the final CRUDS and expiration date, so it is not really possible to include that information in a meaningful way. Therefore I would suggest to keep this information out for now.

@MTGap
Copy link
Contributor

MTGap commented Aug 21, 2013

Why are you working on this? The UI is being scrapped, please wait until I'm done.

@karlitschek
Copy link
Contributor

This is one of the new features that we need for ownCloud 6. It is really important that we get the sharing refactoring done very very soon so that we can iterate on new features.

@schiessle
Copy link
Contributor

As said by Frank, this is a new feature we need for ownCloud6. We will not get it done if we don't work in parallel. I based my work on your branch so I hope I can stay in sync as good as possible. UI changes shouldn't be a big problem if all the back-end code is in place. To make sure that we don't diverge we could also work in the same branch if you prefer it.

@MTGap
Copy link
Contributor

MTGap commented Aug 21, 2013

Where is your branch for me to see?

@ghost ghost assigned schiessle Aug 22, 2013
@schiessle
Copy link
Contributor

I just uploaded the branch https://github.com/owncloud/core/tree/sharing_mail_notification it is based on the "files_sharing-ported" branch.

Sending out mails works just fine. I just need to store the status of the "inform users by mail" checkbox in the database.

@MTRichards , this is the text I send to the user at the moment. Is this OK for you:
"[UsersDisplayName] shared a [file|folder] called [file/folder name] with you. You can find the [file|folder] here: [link either directly in the folder or to the folder where the file is located]"

@MTGap For every user I need the individual file/folder name for the email, what's the best way to retrieve it? Does any method already exists for it, or should I just create my own db query? Thanks!

@MTGap
Copy link
Contributor

MTGap commented Aug 22, 2013

There shouldn't be a checkbox for sending a notification email. It doesn't affect the owner of the file so there should be no need to make that decision. That said, it should be the user receiving the notifications that should be able to configure if they receive notifications or not.

And I don't understand why you separated it from the actual sharing action. You should be passing around the Share object to the defaults class rather than individual parameters.

@schiessle
Copy link
Contributor

And I don't understand why you separated it from the actual sharing action. You should be passing around the Share object to the defaults class rather than individual parameters.

Because the requirement says: "In the share dialog, have a checkbox that says “notify user by email” on a per user/group (line in the share dialog) and an email is sent to the user or group that the file or folder has been shared with them." So it is a separate ajax call independent from the actual share action and it is the user who shares something who decides on a per share basis who should be notified.

@MTGap Would be great if you could answer my question so that I can implement it the "right way". I know the item which was shared and to whom it was shared. Does the API provides a call go get the actual file name for the given user or do I need to query the database independent from the API?

@MTGap
Copy link
Contributor

MTGap commented Aug 22, 2013

Because the requirement says: "In the share dialog, have a checkbox that says “notify user by email” on a per user/group (line in the share dialog) and an email is sent to the user or group that the file or folder has been shared with them." So it is a separate ajax call independent from the actual share action and it is the user who shares something who decides on a per share basis who should be notified.

And I'm saying that I don't like this requirement, it's a sloppy UX.

@MTGap
Copy link
Contributor

MTGap commented Aug 22, 2013

$shareManager->getShares($itemType, array('shareWith' => $user, 'isShareWithUser' => true, 'itemSource' => $itemSource)

@schiessle
Copy link
Contributor

@MTGap Thanks for providing me this information. I just updated the sharing_mail_notification branch. Everything seems to work as expected. Would be great if you could have a look, especially at the last commit (24b387b) where I read/write the information to the database. Hope I did everything the right way...

@MTGap
Copy link
Contributor

MTGap commented Aug 28, 2013

I am still strongly against this... and will not accept these changes because of the UX.

For the sake of learning:

  • getMetadata is only for file metadata and nothing else
  • Use booleans instead of ints and cast it with addType
  • You shouldn't assume that getShares will return a share, and with that filter you will also receive group shares - you need more information to filter the share you want

The last bullet is simplified greatly with the proper controller that I have for the new UI.

@schiessle
Copy link
Contributor

If someone wants to have a look at the current implementation and maybe want to try it out: This is the up-to-date branch where we develop this feature https://github.com/owncloud/core/tree/sharing_mail_notification_master

Basically everything works and is implemented. I will issue a pull request soon.

@MTRichards
Copy link
Contributor Author

There are two levels of notification that are needed. One is the user activity stream, which is a separate topic - where all of this will also be added and eventually available on all devices for the user to flip through. However, for some users this is not enough of a notification, we get to requirement two.

Two is this, a specific request to send an email to a user/group when a file or folder is shared. We can't make this a default behavior, because that is super irritating for a user to get a million emails, and there are cases where the user MUST get an email for retention reasons, so we can't make it something the user can set in personal and then not get an email. So for this use case, it leaves one option - make it a file owner selectable option when shared. The option can be turned on or off by the admin for the entire system, and if turned on the checkbox appears when a file or folder is shared.

Does this make sense now?

@schiessle
Copy link
Contributor

The option can be turned on or off by the admin for the entire system, and if turned on the checkbox appears
when a file or folder is shared.

Does this make sense now?

Just for the record, it makes sense to me and this is exactly the way I implemented it. I will soon open a pull request for it.

Currently the mail contains:

  • who shared the file/folder to you
  • filename/foldername
  • direct link to the file/folder
  • the expiration date, if it is already set

What it doesn't contain:

  • the CRUDS privileges, because we don't know if the user adjusted this privileges before activating the "send notification"-checkbox or if he will adjust them later. So I thought that no information are better than (potentially) false information

@MTRichards Are you agree with this decision?

@MTRichards
Copy link
Contributor Author

Yes, I concur that CRUDS is a bit challenging since they can be changed at any time, so it is OK to not have those in there.

@MTRichards
Copy link
Contributor Author

On the text:

@MTRichards , this is the text I send to the user at the moment. Is this OK for you:
"[UsersDisplayName] shared a [file|folder] called [file/folder name] with you. You can find the [file|folder] here: [link either directly in the folder or to the folder where the file is located]"

Looks good!

@schiessle
Copy link
Contributor

merged in master(OC6)

@simonbuehler
Copy link

don't know if this is the correct place for it: the link in the activities page is missing the path:

You shared test.txt
instead of
You shared Notes/test.txt

i do get "Notes/test.txt changed" in the activities correct

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants