New share provider: Share by mail #657

schiessle · 2016-07-29T13:43:15Z

This introduces a new share provider called "Share by mail" which allows you to enter a email address to the share dialog and send a personalized link.

This is work in progress, but at least the auto-complete function works already.

Fix #1703

mention-bot · 2016-07-29T13:43:17Z

@schiessle, thanks for your PR! By analyzing the annotation information on this pull request, we identified @rullzer, @blizzz and @nickvergessen to be potential reviewers

schiessle · 2016-07-29T13:43:54Z

@jancborchardt I'm sure that's also interesting to you 😉

nickvergessen · 2016-07-29T13:48:00Z

missing from shipped.json

rullzer · 2016-07-29T13:51:18Z

While I think the idea is good. The UX there is terrible. Basically it should fall back to sending the e-mail if no remote is found.

icewind1991 · 2016-07-29T13:53:06Z

apps/sharebymail/lib/FederatedShareProvider.php

+		/*
+		 * Check if file is not already shared with the remote user
+		 */
+		$alreadyShared = $this->getSharedWith($shareWith, self::SHARE_TYPE_REMOTE, $share->getNode(), 1, 0);


SHARE_TYPE_EMAIL?

icewind1991 · 2016-07-29T14:04:17Z

The share provider looks like a lot of duplicate code from the other share providers, probably worth it having a go at factoring out the common logic

jancborchardt · 2016-07-29T14:10:31Z

@schiessle awesome! :) Great that you’re pushing this!

I tend to agree with @rullzer – it’s a bit strange at the moment that you need to decide between remote and email. It should check automatically if a remote by that ID exists – if not share via email.

Also, can we tune that dropdown so that to the left of the remote user it shows their avatar (or colored circle with initial) and next to email it shows an email icon? Or do we need to do a major rewire for that?

schiessle · 2016-07-29T15:22:38Z

The share provider looks like a lot of duplicate code from the other share providers, probably worth it having a go at factoring out the common logic

yes, that's mainly copy&pasted from the federated share provider... This will change a lot, you can ignore it for now

schiessle · 2016-07-29T15:27:31Z

I tend to agree with @rullzer – it’s a bit strange at the moment that you need to decide between remote and email. It should check automatically if a remote by that ID exists – if not share via email.

I'm not sure. This would mean that you would first hand the share to the federated share provider, which would try to create a share. If it fails it would return to the share manager which would then try to do a mail share. But it could also happen that it is a federated share id and the server is just temporarily not available. In this case it would make more sense to re-try it later (with a background job) instead of falling back to email which will fail because for most user email address and federated share ID will not be the same. Such a behavior is also completely in-transparent to the user. I tend to let the user explicitly decide what he want to do. In case we find a exact match of a federated share or a email address we can hide the other option because it is really unlikely that a user has the exact same email address and federated cloud id. But if we enter something for the first time I would just let the user chose.

Also, can we tune that dropdown so that to the left of the remote user it shows their avatar (or colored circle with initial) and next to email it shows an email icon? Or do we need to do a major rewire for that?

We don't have a avatar if your share with a user for the first time... Let's keep this for later. This is out of scope for this PR.

jancborchardt · 2016-07-29T15:39:07Z

I tend to let the user explicitly decide what he want to do.

Nextcloud tends to do stuff automatically. ;) If you share something with me via email – either cause my server is not available or you don’t have my federated cloud ID – it could be added to my Nextcloud as a federated share and communicated back to your server like that. Just automatically, there should be no difference and there doesn’t need to be, because in both cases you have the control over if I »can edit« or not.

MariusBluem · 2016-07-29T15:48:44Z

Uhmm ... Isnt this covered by owncloud/core#25576
It seems to be something different ... but ... a bit ... the same 😁

schiessle · 2016-07-29T15:53:06Z

Nextcloud tends to do stuff automatically. ;) If you share something with me via email – either cause my server is not available or you don’t have my federated cloud ID – it could be added to my Nextcloud as a federated share and communicated back to your server like that.

Unfortunately it is not that easy 😉 If I entered your cloud ID and your server is offline I can't send it to you by mail because in most cases both are not the same. And I will never know that sending the email failed.

Let me try to illustrate the two possible scenarios if we try to make it automatically:

We first try to do a federated share. If it succeed fine. But what do we do if it don't succeed? If we know it is a federated share ID we can retry it and the recipient will receive his share once his Nextcloud is back online -> everything is fine. If we try to be more intelligent as the user we would fall back to a mail share, we would send the mail to the cloud ID. The delivery will fail because in most cases the cloud ID is not a email address at the same time. We will have no chance to detect that sending the mail failed. The user who send the share will see it in his share list and assume that everything is fine because he entered a federated cloud id. -> this is a problem you can't solve.
So we could try to do it the other way around, first try to send a mail. If it is a valid mail address everything fine. If the rare case happens (0.1%) that a user has the same federated cloud ID as email address this would be bad because he would never receive a federated share but always a email. But it is just 0.1% so let's ignore it for now. Let's assume the ID was a cloud ID, we tried to send a mail and will not have any response that we failed to deliver the mail successfully, so we never know if we should retry a federated share -> solution is not feasible at all.

So we have two solutions to automate it and both don't work.

schiessle · 2016-07-29T16:11:31Z

Uhmm ... Isnt this covered by https://github.com/owncloud/core/25576

@MariusBluem link doesn't work

MariusBluem · 2016-07-29T16:13:22Z

Was updated 1 minute later 😒 ... The /pull/ was missing 😉 @schiessle

schiessle · 2016-07-29T16:20:12Z

@Mar1u5 As you said, it is something different. I don't want to have a way to share a public link by mail, this is already today possible. But to create multiple public links, one for each recipient transparent in the share dialog

jancborchardt · 2016-09-26T14:36:26Z

cc @Rocco83 because this is the »generate unique link per email address so revocation on a per-person basis is possible« feature you are interested in. :)

jancborchardt · 2016-10-18T09:49:48Z

@schiessle any progress on this? Do you need any help or reviews? :)

Signed-off-by: Bjoern Schiessle <[email protected]>

…ooks Signed-off-by: Bjoern Schiessle <[email protected]>

Signed-off-by: Bjoern Schiessle <[email protected]>

email address we only return the exact match. It is highly unlikely that the exact same email address and federated cloud id exists Signed-off-by: Bjoern Schiessle <[email protected]>

Signed-off-by: Bjoern Schiessle <[email protected]>

…ail shares Signed-off-by: Bjoern Schiessle <[email protected]>

Signed-off-by: Bjoern Schiessle <[email protected]>

…hare provider is enabled Signed-off-by: Bjoern Schiessle <[email protected]>

Signed-off-by: Bjoern Schiessle <[email protected]>

schiessle · 2016-11-01T19:13:34Z

We need to adjust this provider also for #339 Depending on which one goes in first.

Done. I also added a lot of unit tests (cc @LukasReschke)... Final review round 😉

rullzer · 2016-11-02T09:13:02Z

I'd say lets get this in 👍

LukasReschke · 2016-11-02T09:36:00Z

LGTM

LukasReschke · 2016-11-02T09:36:17Z

@nickvergessen Your call now ;)

nickvergessen · 2016-11-02T10:04:30Z

Yeah lets do that...

* now handled by sharebymail app * see #657 Signed-off-by: Morris Jobke <[email protected]>

schiessle added 2. developing Work in progress feature: sharing labels Jul 29, 2016

schiessle added this to the Nextcloud 11.0 milestone Jul 29, 2016

icewind1991 reviewed Jul 29, 2016
View reviewed changes

MorrisJobke mentioned this pull request Aug 8, 2016

Nextcloud 11 overview #745

Closed

47 tasks

schiessle force-pushed the share-by-mail branch from 199bc25 to 9e607f6 Compare September 7, 2016 09:55

schiessle force-pushed the share-by-mail branch from 9e607f6 to 9701f31 Compare October 5, 2016 14:50

nickvergessen mentioned this pull request Oct 12, 2016

[Feature] Sharing with external email ( not federated ) #1703

Closed

jancborchardt added the design Design, UI, UX, etc. label Oct 18, 2016

blizzz added the enhancement label Oct 18, 2016

rullzer mentioned this pull request Oct 24, 2016

Add ShareesAPI E-mail search #1876

Merged

schiessle force-pushed the share-by-mail branch 2 times, most recently from 367298e to 7384940 Compare October 24, 2016 15:19

schiessle added 17 commits November 1, 2016 19:51

update permissions

96c40d1

Signed-off-by: Bjoern Schiessle <[email protected]>

add method to check if a share provider for a given type is loaded

3181606

Signed-off-by: Bjoern Schiessle <[email protected]>

handle case if no share-by-mail share provider is loaded

77f74b9

Signed-off-by: Bjoern Schiessle <[email protected]>

improve search for federated cloud ids and email adresses

60a3893

Signed-off-by: Bjoern Schiessle <[email protected]>

show correct display name if we have the user in one of our address b…

8c8a019

…ooks Signed-off-by: Bjoern Schiessle <[email protected]>

unified way to display remote shares and mail shares

392c8a1

Signed-off-by: Bjoern Schiessle <[email protected]>

if we have a exact match, either for the federated cloud id or for the

7436e45

email address we only return the exact match. It is highly unlikely that the exact same email address and federated cloud id exists Signed-off-by: Bjoern Schiessle <[email protected]>

send mail for share-by-mail shares

31c8c38

Signed-off-by: Bjoern Schiessle <[email protected]>

don't show 'notify by mail' option or permissions not available for m…

561dd80

…ail shares Signed-off-by: Bjoern Schiessle <[email protected]>

find and show share-by mail links

1e930df

Signed-off-by: Bjoern Schiessle <[email protected]>

unit tests updated and new added

6d7520b

Signed-off-by: Bjoern Schiessle <[email protected]>

enabled by default and add to shipped.json

ce08682

Signed-off-by: Bjoern Schiessle <[email protected]>

always show correct place holder, mention share by mail only if the s…

86b3628

…hare provider is enabled Signed-off-by: Bjoern Schiessle <[email protected]>

add tests for the share-by-mail provider

16ec0f2

Signed-off-by: Bjoern Schiessle <[email protected]>

add new method getSharesInFolder(), see #339

32dbf32

Signed-off-by: Bjoern Schiessle <[email protected]>

remove unused variable

6e52091

Signed-off-by: Bjoern Schiessle <[email protected]>

import exception

f2b2b8d

Signed-off-by: Bjoern Schiessle <[email protected]>

schiessle force-pushed the share-by-mail branch from cbbfe95 to f2b2b8d Compare November 1, 2016 19:12

LukasReschke approved these changes Nov 2, 2016

View reviewed changes

nickvergessen approved these changes Nov 2, 2016

View reviewed changes

nickvergessen merged commit 7da3ba3 into master Nov 2, 2016

nickvergessen deleted the share-by-mail branch November 2, 2016 10:04

nickvergessen mentioned this pull request Nov 3, 2016

Email notification about new share #1277

Closed

MorrisJobke added a commit that referenced this pull request Apr 11, 2017

Removes unused code for link share emails

8fa5141

* now handled by sharebymail app * see #657 Signed-off-by: Morris Jobke <[email protected]>

MorrisJobke mentioned this pull request Apr 11, 2017

Removes unused code for link share emails #4309

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New share provider: Share by mail #657

New share provider: Share by mail #657

schiessle commented Jul 29, 2016 •

edited by nickvergessen

Loading

mention-bot commented Jul 29, 2016

schiessle commented Jul 29, 2016

nickvergessen commented Jul 29, 2016

rullzer commented Jul 29, 2016

icewind1991 Jul 29, 2016

icewind1991 commented Jul 29, 2016

jancborchardt commented Jul 29, 2016

schiessle commented Jul 29, 2016 •

edited

Loading

schiessle commented Jul 29, 2016 •

edited

Loading

jancborchardt commented Jul 29, 2016

MariusBluem commented Jul 29, 2016 •

edited

Loading

schiessle commented Jul 29, 2016 •

edited

Loading

schiessle commented Jul 29, 2016 •

edited by MariusBluem

Loading

MariusBluem commented Jul 29, 2016 •

edited

Loading

schiessle commented Jul 29, 2016 •

edited

Loading

jancborchardt commented Sep 26, 2016

jancborchardt commented Oct 18, 2016

schiessle commented Nov 1, 2016

rullzer commented Nov 2, 2016

LukasReschke commented Nov 2, 2016

LukasReschke commented Nov 2, 2016 •

edited

Loading

nickvergessen commented Nov 2, 2016

New share provider: Share by mail #657

New share provider: Share by mail #657

Conversation

schiessle commented Jul 29, 2016 • edited by nickvergessen Loading

mention-bot commented Jul 29, 2016

schiessle commented Jul 29, 2016

nickvergessen commented Jul 29, 2016

rullzer commented Jul 29, 2016

icewind1991 Jul 29, 2016

Choose a reason for hiding this comment

icewind1991 commented Jul 29, 2016

jancborchardt commented Jul 29, 2016

schiessle commented Jul 29, 2016 • edited Loading

schiessle commented Jul 29, 2016 • edited Loading

jancborchardt commented Jul 29, 2016

MariusBluem commented Jul 29, 2016 • edited Loading

schiessle commented Jul 29, 2016 • edited Loading

schiessle commented Jul 29, 2016 • edited by MariusBluem Loading

MariusBluem commented Jul 29, 2016 • edited Loading

schiessle commented Jul 29, 2016 • edited Loading

jancborchardt commented Sep 26, 2016

jancborchardt commented Oct 18, 2016

schiessle commented Nov 1, 2016

rullzer commented Nov 2, 2016

LukasReschke commented Nov 2, 2016

LukasReschke commented Nov 2, 2016 • edited Loading

nickvergessen commented Nov 2, 2016

schiessle commented Jul 29, 2016 •

edited by nickvergessen

Loading

schiessle commented Jul 29, 2016 •

edited

Loading

schiessle commented Jul 29, 2016 •

edited

Loading

MariusBluem commented Jul 29, 2016 •

edited

Loading

schiessle commented Jul 29, 2016 •

edited

Loading

schiessle commented Jul 29, 2016 •

edited by MariusBluem

Loading

MariusBluem commented Jul 29, 2016 •

edited

Loading

schiessle commented Jul 29, 2016 •

edited

Loading

LukasReschke commented Nov 2, 2016 •

edited

Loading