🔗☑️ Accept all incoming shares #16828

nickvergessen · 2019-08-21T14:23:51Z

Respect the accepted flag from the share table (not used at all at the moment)
Generate a notification for a user when an individual share is done
Generate a notification for all members when a group share is done
Generate notifications for all group shares if a user is added to a group
Remove the notifications when the share is deleted
Remove the notifications when a member is removed from a group
Update the accepted flag to 1 (accepted) for all existing shares on update
Adjust the email that is sent for user shares or drop it

nickvergessen · 2019-08-22T12:55:02Z

There is currently an inconsistence with remote shares to normal user shares and group shares.

User shares trigger an individual email, federated shares had a notification to accept and group shares had nothing.

Now since you need to accept all of them, they will all trigger a notification. Should be enough? Or will there be complains again from people how dont have the notifications app installed, or no client set up and therefor dont notice notifications.
But in that case I would say all shares should also trigger an email, not just some.

And then I think this is just working around the mail notification topic again, so in the end notifications should have an option to come via mail

wiswedel · 2019-08-22T13:08:38Z

Now since you need to accept all of them, they will all trigger a notification. Should be enough? Or will there be complains again from people how dont have the notifications app installed, or no client set up and therefor dont notice notifications.

We require notifications to be installed and activated. Makes no sense otherwise.

But in that case I would say all shares should also trigger an email, not just some.

And then I think this is just working around the mail notification topic again, so in the end notifications should have an option to come via mail

Yeah, fire out emails as much as possible. Not all people have NC or clients open all day (like us 😉 )

lib/public/Share/IShareProvider.php

nickvergessen · 2019-09-06T11:30:14Z

User and group shares are now ready for testing and the tech is ready for review. I think we should look at the other share providers in a follow up.

Remarks

When the notification is accepted on the files app, you need to reload the page to see it (same for federated shares, but this used to work, so also something for a follow up). At least a note should be displayed to the users.

blizzz · 2019-09-06T13:14:25Z

apps/files_sharing/lib/Controller/ShareAPIController.php

+		}
+
+		if (!$this->canAccessShare($share)) {
+			throw new OCSNotFoundException($this->l->t('Wrong share ID, share doesn\'t exist'));


adjust error message

I copied this behaviour from updateShare and I think it's okay to not leak the existence if the user can not interact with the share

apps/files_sharing/lib/Controller/ShareAPIController.php

apps/files_sharing/lib/Notification/Listener.php

apps/files_sharing/lib/Notification/Notifier.php

lib/public/Share/IManager.php

blizzz · 2019-09-06T13:38:59Z

lib/public/Share/IShareProvider.php

+	 * @return IShare The share object
+	 * @since 17.0.0
+	 */
+//	public function acceptShare(IShare $share, string $recipient): IShare;


temporary until the other implementeations are adjusted then the interface will require it #TechnicalReviewOnly

wiswedel · 2019-10-02T07:27:03Z

Please don't let this get forgotten. Thx.

wiswedel · 2019-10-11T11:21:54Z

@nickvergessen What's the status on this?

rullzer · 2019-10-30T08:35:42Z

apps/files_sharing/lib/AppInfo/Application.php

+		$manager->registerNotifierService(Notifier::class);
+
+		$dispatcher = $this->getContainer()->getServer()->getEventDispatcher();
+		$dispatcher->addListener('OCP\Share::postShare', function(GenericEvent $event) {


Ah those crappy old events...

rullzer

It doesn't seem to work for me. I share a folder to a user. The get the notifiation. But the folder already shows up directly...

apps/files_sharing/lib/Migration/SetAcceptedStatus.php

nickvergessen · 2019-10-31T11:04:08Z

It doesn't seem to work for me. I share a folder to a user. The get the notifiation. But the folder already shows up directly...

My last commit was missing here. Pushed it now while I rebased on top of 620 missing commits from master 😅

tobiasKaminsky · 2019-11-01T14:37:45Z

Hm. For me it also does not work, but with

share works
receiving user sees a notification
but clicking on accept results in 404 (url: http://localhost/nc/ocs/v2.phpex.php/ocsapp/apps/files_sharing/api/v1/shares/pending/588)

Signed-off-by: Joas Schilling <coding@schilljs.com>

nickvergessen · 2019-11-12T16:42:24Z

Rebased due to the heavy conflicts with roelands work of the share expiration notification.

After that I retested it again and it worked fine here. But @tobiasKaminsky your URL looks like the configuration of the URLs is not dont correctly, because I don't juggle with URLs at all:

server/apps/files_sharing/lib/Notification/Notifier.php

Line 245 in 0fe4b71

    
           ->setLink($this->url->linkToOCSRouteAbsolute('files_sharing.ShareAPI.acceptShare', ['id' => $share->getId()]), 'POST')

tobiasKaminsky · 2019-11-13T07:17:40Z

I tested it and now it works 🎉

Two things I noticed, which should then be handled in a new PR, if approved:

be in files root view
receive a share
click "accept" on notification
nothing happens
where is my file? ;-) I guess this might be irritating users.
--> can we check if we are in the folder where the shared item should pop up and do a reload?
share a file with user A
user A rejects it
do not get a notification / info about it
it is just like you never shared to the user
--> maybe add a notification like "user A rejected your share"

rullzer · 2019-11-13T13:01:12Z

Yay works now indeed.
I say lets get it in and create separate issues to fix.

apps/files_sharing/lib/External/Manager.php

apps/files_sharing/lib/Notification/Listener.php

lib/private/Share20/DefaultShareProvider.php

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

wiswedel · 2019-11-19T18:27:45Z

@rullzer do you want me to report QA findings in separate issues just like @tobiasKaminsky earlier?

rullzer · 2019-11-19T19:25:31Z

Yes track sepratly please

nickvergessen added enhancement 2. developing Work in progress feature: sharing feature: activity and notification labels Aug 21, 2019

nickvergessen added this to the Nextcloud 18 milestone Aug 21, 2019

nickvergessen force-pushed the feature/noid/accept-incoming-shares branch from 4e8bf0b to 860617b Compare September 4, 2019 14:51

nickvergessen commented Sep 4, 2019

View reviewed changes

lib/public/Share/IShareProvider.php Outdated Show resolved Hide resolved

nickvergessen commented Sep 4, 2019

View reviewed changes

lib/public/Share/IShareProvider.php Outdated Show resolved Hide resolved

nickvergessen force-pushed the feature/noid/accept-incoming-shares branch 2 times, most recently from 78462ce to c77bf7c Compare September 6, 2019 11:27

nickvergessen changed the title ~~🚧 Accept all incoming shares 🚧~~ 🔗☑️ Accept all incoming shares Sep 6, 2019

nickvergessen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 6, 2019

nickvergessen requested review from ArtificialOwl, blizzz, juliusknorr and rullzer September 6, 2019 11:30

blizzz reviewed Sep 6, 2019

View reviewed changes

jancborchardt mentioned this pull request Sep 12, 2019

Ask before automatically syncing a folder that was just shared. nextcloud/desktop#1199

Closed

rullzer reviewed Oct 30, 2019

View reviewed changes

rullzer requested changes Oct 30, 2019

View reviewed changes

apps/files_sharing/lib/Migration/SetAcceptedStatus.php Outdated Show resolved Hide resolved

nickvergessen force-pushed the feature/noid/accept-incoming-shares branch 2 times, most recently from 9fdda51 to 3dfd13f Compare October 31, 2019 11:04

nickvergessen added 6 commits November 12, 2019 17:37

Accept existing shares on update

43517cf

Signed-off-by: Joas Schilling <coding@schilljs.com>

Use constants for the magic strings

19e1892

Signed-off-by: Joas Schilling <coding@schilljs.com>

The share manager knows which provider can accept shares

e79ab7d

Signed-off-by: Joas Schilling <coding@schilljs.com>

Correctly use the displayname of the sharer in the notification

0fe4b71

Signed-off-by: Joas Schilling <coding@schilljs.com>

Fix "Invalid parameter number: mixed named and positional parameters"

6d9afca

Signed-off-by: Joas Schilling <coding@schilljs.com>

Make sure the super share also has a valid type

d3b6289

Signed-off-by: Joas Schilling <coding@schilljs.com>

nickvergessen force-pushed the feature/noid/accept-incoming-shares branch from 3dfd13f to d3b6289 Compare November 12, 2019 16:40

nickvergessen requested review from blizzz and rullzer November 12, 2019 16:42

rullzer approved these changes Nov 13, 2019

View reviewed changes

ChristophWurst reviewed Nov 14, 2019

View reviewed changes

apps/files_sharing/lib/External/Manager.php Outdated Show resolved Hide resolved

apps/files_sharing/lib/Notification/Listener.php Show resolved Hide resolved

lib/private/Share20/DefaultShareProvider.php Show resolved Hide resolved

This was referenced Nov 18, 2019

Click on accepting share, but nothing happens #17981

Closed

Reject incoming share #17982

Open

Fixes

e163213

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

ChristophWurst approved these changes Nov 18, 2019

View reviewed changes

rullzer merged commit ccc0a5e into master Nov 18, 2019

rullzer deleted the feature/noid/accept-incoming-shares branch November 18, 2019 19:11

wiswedel mentioned this pull request Nov 19, 2019

Group share accepting/rejecting keeps re-triggering notifications for others #18020

Closed

This was referenced Nov 20, 2019

Accept incoming shares in integration tests #18031

Merged

Accept incoming shares in acceptance tests #18039

Merged

icewind1991 mentioned this pull request Dec 5, 2019

No way to accept shares if notifications app is not enabled #18255

Closed

This was referenced Dec 12, 2019

Incoming share email broken #18380

Closed

Guests should auto-accept incoming shares nextcloud/guests#222

Closed

mgallien mentioned this pull request May 6, 2021

Newly shared folder gets automatically synced nextcloud/desktop#1169

Closed

susnux mentioned this pull request Jun 25, 2024

fix: Add acceptShare as an interface #46099

Merged

4 tasks

Uh oh!

🔗☑️ Accept all incoming shares #16828

🔗☑️ Accept all incoming shares #16828

Uh oh!

Conversation

nickvergessen commented Aug 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nickvergessen commented Aug 22, 2019

Uh oh!

wiswedel commented Aug 22, 2019

Uh oh!

Uh oh!

Uh oh!

nickvergessen commented Sep 6, 2019

Remarks

Uh oh!

blizzz Sep 6, 2019

Choose a reason for hiding this comment

Uh oh!

nickvergessen Sep 9, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

blizzz Sep 6, 2019

Choose a reason for hiding this comment

Uh oh!

nickvergessen Sep 6, 2019

Choose a reason for hiding this comment

Uh oh!

wiswedel commented Oct 2, 2019

Uh oh!

wiswedel commented Oct 11, 2019

Uh oh!

rullzer Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

rullzer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nickvergessen commented Oct 31, 2019

Uh oh!

tobiasKaminsky commented Nov 1, 2019

Uh oh!

nickvergessen commented Nov 12, 2019

Uh oh!

tobiasKaminsky commented Nov 13, 2019

Uh oh!

rullzer commented Nov 13, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wiswedel commented Nov 19, 2019

Uh oh!

rullzer commented Nov 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

nickvergessen commented Aug 21, 2019 •

edited

Loading