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

Send an email once a file/folder is shared with a user #5897

Merged
merged 2 commits into from
Aug 1, 2017

Conversation

MorrisJobke
Copy link
Member

  • only if user has set an email address
  • only for user shares (no group shares for now)

The sharing email is basically the same as for the "share by email". 😉

@MorrisJobke
Copy link
Member Author

Tested with an instance with index.php and without. Worked both fine:

bildschirmfoto 2017-07-26 um 23 45 15

@mario
Copy link
Contributor

mario commented Jul 27, 2017

Considering there is only one available action, putting "Click the button below to open it" (also what is "it"?) is redundant. Please remove that part.

@mario
Copy link
Contributor

mario commented Jul 27, 2017

Actually, like mentioned in the chat, the entire body text is largely useless and redundant.

@MorrisJobke
Copy link
Member Author

Then the email looks super empty. I guess we need some adjusted text in there (also it avoid to be classified as spam, because they check for a given word count sometimes).

bildschirmfoto 2017-07-27 um 12 20 50

@mario
Copy link
Contributor

mario commented Jul 27, 2017 via email

@MorrisJobke
Copy link
Member Author

The white rectangle is also ugly. Maybe just button below the text?

This is super hard to accomplish due to the weird stuff, that HTML emails need to be compatible with the new stuff like gmail and the old stuff like Outlook (old regarding rendering engine).

Copy link
Member

@LukasReschke LukasReschke left a comment

Choose a reason for hiding this comment

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

😀

@schiessle
Copy link
Member

In the default activity settings email notifications are enabled for "A file or folder has been shared". Does this mean that users will get now two mails by default. One immediately after the share was created and one later when the activity mails are send out?

@MorrisJobke
Copy link
Member Author

Does this mean that users will get now two mails by default. One immediately after the share was created and one later when the activity mails are send out?

Seems so. We should disable the activity email then.

@MorrisJobke
Copy link
Member Author

This would then look like this:

bildschirmfoto 2017-07-28 um 08 32 45

I still don't like it. Can we get this in like it is now, because those are already the emails that are send out for email shares and then do the rewording in an additional step? Thanks

* only if user has set an email address
* only for user shares (no group shares for now)

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke MorrisJobke force-pushed the add-share-mail-for-user-share branch from d8c5426 to c43abe4 Compare July 28, 2017 06:36
@codecov
Copy link

codecov bot commented Jul 28, 2017

Codecov Report

Merging #5897 into master will decrease coverage by 0.01%.
The diff coverage is 29.16%.

@@             Coverage Diff              @@
##             master    #5897      +/-   ##
============================================
- Coverage      53.2%   53.18%   -0.02%     
- Complexity    22759    22766       +7     
============================================
  Files          1404     1404              
  Lines         87529    87576      +47     
  Branches       1327     1327              
============================================
+ Hits          46570    46578       +8     
- Misses        40959    40998      +39
Impacted Files Coverage Δ Complexity Δ
lib/private/Server.php 93.75% <100%> (+0.02%) 121 <0> (ø) ⬇️
lib/private/Share20/Manager.php 89.07% <22.72%> (-5.14%) 229 <3> (+7)
core/js/js.js 61.27% <0%> (-0.56%) 0% <0%> (ø)

@mario
Copy link
Contributor

mario commented Jul 28, 2017

@MorrisJobke sure. Would you be so kind to open an issue with all emails that we send like this (and assign them to me) so I can re-word them together with you? :)

@schiessle
Copy link
Member

schiessle commented Jul 28, 2017

Seems so. We should disable the activity email then.Seems so. We should disable the activity email then.

I fear that this will always lead to confusions because with this PR we only send notifications for individual shares, so if someone want to have notification about group shares they have to enable the activity settings which will then lead to duplicates for individual shares. Maybe we should reduce the activity mails than to non-individual shares only... But this will also be confusing "why can I disable mails for group shares but not for individual shares"?

In general I really like the idea about direct mail notifications for shares as introduced here but I fear that we create two parallel worlds of notifications which will become quite confusing.

As a minimum I would suggest to change at least the activity defaults within this PR to disable activity mails for shares.

@LukasReschke
Copy link
Member

I still don't like it. Can we get this in like it is now, because those are already the emails that are send out for email shares and then do the rewording in an additional step? Thanks

👍 🙈

@MorrisJobke
Copy link
Member Author

I fear that this will always lead to confusions because with this PR we only send notifications for individual shares, so if someone want to have notification about group shares they have to enable the activity settings which will then lead to duplicates for individual shares.

I would simply add group emails as well, but those need more code and those I would also not backport. 😉

In general I really like the idea about direct mail notifications for shares as introduced here but I fear that we create two parallel worlds of notifications which will become quite confusing.

Of course we should streamline everything.

As a minimum I would suggest to change at least the activity defaults within this PR to disable activity mails for shares.

I thought the activity defaults are set in the activity app. Let me create a PR there.

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke
Copy link
Member Author

I thought the activity defaults are set in the activity app. Let me create a PR there.

Was done in files_sharing. So I added a commit here.

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Fine lets get this in. Then I'll spend some time getting more event ready so we can decouple this.

@rullzer rullzer merged commit c845280 into master Aug 1, 2017
@rullzer rullzer deleted the add-share-mail-for-user-share branch August 1, 2017 11:27
@LukasReschke
Copy link
Member

Pro-tip: If CI says stuff doesn't work then don't merge it. This here is broken.

@MorrisJobke MorrisJobke mentioned this pull request Aug 1, 2017
@jancborchardt
Copy link
Member

@MorrisJobke @mario if there is that wording discussion issue please CC me. I also agree the notification mails should be cleaner, but of course also not too empty. :) We could think about using filetype icons, or even generated previews.

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

Successfully merging this pull request may close these issues.

6 participants