Splits off the logic from sharees endpoint thus making it available from within Nc/via PHP. #6328

blizzz · 2017-08-31T20:51:20Z

@schiessle as discussed yesterday. Took a bit more effort, because i decided to split off the different getters to Plugins. However, I did not create a register function or anything. Can be added later if it becomes necessary. Right now, I doubt this and want to keep effort small. I did not touch tests, yet, however Smoke tests with the share dialog worked for me. Are you OK with that approach?

necessary for #2443

P.S.: Context: @schiessle prefered to have the logic outside of the Sharing scope, due to his concern that a universal API is established again. Therefore we thought the Collaborator terminology could fit.

mention-bot · 2017-08-31T20:51:21Z

@blizzz, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nickvergessen, @rullzer and @schiessle to be potential reviewers.

ArtificialOwl · 2017-09-02T15:59:57Z

could be fun to merge with #5280

blizzz · 2017-09-03T21:38:38Z

on first glance, i did not see conflict potential. different controller and endpoint.

codecov · 2017-09-05T19:21:46Z

Codecov Report

Merging #6328 into master will increase coverage by 2.51%.
The diff coverage is 70.43%.

@@             Coverage Diff              @@
##             master    #6328      +/-   ##
============================================
+ Coverage     53.06%   55.57%   +2.51%     
+ Complexity    22570    21264    -1306     
============================================
  Files          1417     1261     -156     
  Lines         87810    73270   -14540     
  Branches       1340        0    -1340     
============================================
- Hits          46597    40723    -5874     
+ Misses        41213    32547    -8666

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Server.php	`83.47% <20%> (-0.78%)`	`124 <1> (+1)`
lib/private/App/InfoParser.php	`89.18% <25%> (-2.4%)`	`63 <0> (+3)`
lib/private/legacy/app.php	`53.83% <33.33%> (-0.12%)`	`220 <0> (+2)`
...private/Collaboration/Collaborators/MailPlugin.php	`58.75% <58.75%> (ø)`	`19 <19> (?)`
...ivate/Collaboration/Collaborators/SearchResult.php	`62.96% <62.96%> (ø)`	`12 <12> (?)`
...es_sharing/lib/Controller/ShareesAPIController.php	`70% <63.63%> (+5.52%)`	`24 <3> (-90)`	⬇️
...ivate/Collaboration/Collaborators/RemotePlugin.php	`73.33% <73.33%> (ø)`	`17 <17> (?)`
lib/private/Collaboration/Collaborators/Search.php	`76.92% <76.92%> (ø)`	`12 <12> (?)`
...ivate/Collaboration/Collaborators/LookupPlugin.php	`79.31% <79.31%> (ø)`	`5 <5> (?)`
...rivate/Collaboration/Collaborators/GroupPlugin.php	`80.76% <80.76%> (ø)`	`16 <16> (?)`
... and 385 more

nickvergessen · 2017-09-06T07:55:56Z

lib/private/Collaboration/Collaborators/CirclePlugin.php

+use OCP\Collaboration\Collaborators\ISearchPlugin;
+use OCP\Collaboration\Collaborators\ISearchResult;
+
+class CirclePlugin implements ISearchPlugin {


should be provided by the circles app?

Yes, then I need to add a register method and such… or another entry for the appinfo xml file… not much of a big deal, of course, I was hoping to get around this. After all, it's "just" axe sharpening for the autocomplete.

The annoying part here is the shareType. The client needs to request the shareTypes as they are define in \OC\Share\Constants (made available via \OCP\Share). This makes it hard to register new share types for apps, and even harder for clients for figuring out new features. OTOH all the types may have special aspects themselves, so special treatment might be necessary nevertheless.

That said, what makes most sense to me is to allow registering plugins for existing share types. And opening this for 1:x relationships (one type, many plugins).

nickvergessen · 2017-09-06T07:58:39Z

lib/private/Collaboration/Collaborators/MailPlugin.php

+			$result['wide'] = [];
+		}
+
+		if (!$searchResult->hasExactIdMatch('emails') && filter_var($search, FILTER_VALIDATE_EMAIL)) {


This should also not be done, when remote sharing has an exact match and vice-versa.
See old line 567 of the ShareesController code.

What would be the best to achive this? Or where is this done now

Ah it's in the Search class now, all good

nickvergessen · 2017-09-06T08:01:55Z

lib/private/Collaboration/Collaborators/SearchResult.php

+			'groups' => [],
+			'remotes' => [],
+			'emails' => [],
+			'circles' => [],


I somewhat dislike this. Would be nicer if this is kept modular and provided by each app

In general, yes, I am not sure which side effects this might have for consumers of the endpoint.

OTOH it's better to risk a bug there now, than carrying cruft forever with us. I have a look at it again.

ah, i could simply merge it into the existing results array in the controller to preserver the state there, and have it leaner in the new impl.

nickvergessen · 2017-09-06T08:03:04Z

Nice cleanup, I always disliked the naming anyway.
I would be fine to use a new route and just have two routes linking to the the same class/method.

blizzz · 2017-09-06T12:03:12Z

I would be fine to use a new route and just have two routes linking to the the same class/method.

From within cire, pointing to the files_sharing controller? It might be disabled… We could move this, too, or think about a new Controller in the server… but this I see in a follow-up PR, rather.

required for nextcloud/server#6328 Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

blizzz · 2017-09-06T21:14:15Z

@nickvergessen so I moved the Circles plugin to its own app (nextcloud/circles#126) → registering Plugins works.

Also, the result types are not fixed but filled by the plugins. I added a public class for this to have type name validated.

The LookupPlugin might go to the ~~Federation~~ files_sharing app (since it acts on its config), however this is a special case, because this is not activate by a provided share type, but a flag 😒 but this can be dealt with, with some extra effort.

Also, the MailPlugin can go to 'sharebymail' app, unless something speaks against it ( @schiessle ?), and the RemotePlugin to 'federation'.

nickvergessen · 2017-09-07T11:13:12Z

lib/public/Collaboration/Collaborators/SearchResultType.php

+			throw new \InvalidArgumentException('Type must not be empty');
+		}
+
+		if(trim($type) === 'exact') {


trim is not necessary, you do it in the beginning already

was a leftover when i was pushing code around… thanks for pointing that out 👈 👍

blizzz · 2017-09-13T14:24:11Z

Now 🤹‍ with the ShareeAPIController unit tests was exciting 💤 ;)

blizzz · 2017-09-14T20:55:10Z

lib/private/Collaboration/Collaborators/MailPlugin.php

@@ -159,6 +159,6 @@ public function search($search, $limit, $offset, ISearchResult $searchResult) {

 		$searchResult->addResultSet($emailType, $result['wide'], $result['exact']);

-		return false;
+		return true;


@schiessle this (= always more results available) feels wrong, but was required due to the unit tests. Or did I oversee anything?

blizzz · 2017-09-14T20:55:42Z

lib/private/Collaboration/Collaborators/RemotePlugin.php

@@ -116,7 +116,7 @@ public function search($search, $limit, $offset, ISearchResult $searchResult) {

 		$searchResult->addResultSet($resultType, $result['wide'], $result['exact']);

-		return false;
+		return true;


@schiessle this (= always more results available) feels wrong, but was required due to the unit tests. Or did I oversee anything?

blizzz · 2017-09-14T21:57:00Z

I kept LookupPlugin inside lib/…, because it is involved differently for whatever reason (invocation by explicit flag, not ShareType). Since there is no dedicated ShareType, I refrain to change this.

This said, I'd be happy about final reviews 😸

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

adds two small fixes → they actually work \o/ Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

also moves registering default plugins to Server for proper unit testing Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

blizzz · 2017-09-26T23:28:57Z

due to requests from @nickvergessen and @BernhardPosselt i simplified the required XML. OK now? Needs green lights on nextcloud/appstore#521, too.

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

codecov-io · 2017-09-27T11:23:34Z

Codecov Report

Merging #6328 into master will increase coverage by 0.03%.
The diff coverage is 69.54%.

@@             Coverage Diff              @@
##             master    #6328      +/-   ##
============================================
+ Coverage     53.06%   53.09%   +0.03%     
- Complexity    22570    22594      +24     
============================================
  Files          1417     1425       +8     
  Lines         87810    87889      +79     
  Branches       1340     1340              
============================================
+ Hits          46597    46668      +71     
- Misses        41213    41221       +8

Impacted Files	Coverage Δ	Complexity Δ
lib/private/legacy/app.php	`53.32% <12.5%> (-0.63%)`	`222 <0> (+4)`
lib/private/Server.php	`83.47% <20%> (-0.78%)`	`124 <1> (+1)`
lib/private/App/InfoParser.php	`89.18% <25%> (-2.4%)`	`63 <0> (+3)`
...private/Collaboration/Collaborators/MailPlugin.php	`58.75% <58.75%> (ø)`	`19 <19> (?)`
...ivate/Collaboration/Collaborators/SearchResult.php	`62.96% <62.96%> (ø)`	`12 <12> (?)`
...es_sharing/lib/Controller/ShareesAPIController.php	`70% <63.63%> (+5.52%)`	`24 <3> (-90)`	⬇️
...ivate/Collaboration/Collaborators/RemotePlugin.php	`73.33% <73.33%> (ø)`	`17 <17> (?)`
lib/private/Collaboration/Collaborators/Search.php	`76.92% <76.92%> (ø)`	`12 <12> (?)`
...ivate/Collaboration/Collaborators/LookupPlugin.php	`79.31% <79.31%> (ø)`	`5 <5> (?)`
...rivate/Collaboration/Collaborators/GroupPlugin.php	`80.76% <80.76%> (ø)`	`16 <16> (?)`
... and 16 more

blizzz · 2017-10-04T12:22:58Z

🏓

nickvergessen · 2017-10-04T12:49:51Z

Looks good and still works, but: current user is returned (was not the case before).
Maybe we can fix this before merging?

nickvergessen · 2017-10-04T13:05:14Z

Okay lets do it in a follow up

nickvergessen · 2017-10-04T13:27:16Z

lib/private/Collaboration/Collaborators/MailPlugin.php

+					$emailAddresses = [$emailAddresses];
+				}
+				foreach ($emailAddresses as $emailAddress) {
+					$exactEmailMatch = strtolower($emailAddress) === $lowerSearch;


For me this is an empty string (when removing the email after it was set before), and therefor results in an entry admin ()

Of course this makes no sense, but we should have a look

I took it over as it is from the old method and have not spotted a post processing that would remove it. Thus, it should be reproducable with the old code I think… anyhow, yes, something we should fix

blizzz requested review from nickvergessen, ArtificialOwl and schiessle August 31, 2017 20:51

blizzz added 2. developing Work in progress enhancement labels Aug 31, 2017

blizzz added this to the Nextcloud 13 milestone Aug 31, 2017

blizzz force-pushed the split-sharees-api-logic branch from 5c7de25 to c97ca38 Compare September 5, 2017 19:21

nickvergessen reviewed Sep 6, 2017

View reviewed changes

blizzz added a commit to nextcloud/circles that referenced this pull request Sep 6, 2017

add the collaborator search plugin

8a00033

required for nextcloud/server#6328 Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

blizzz mentioned this pull request Sep 6, 2017

add the collaborator search plugin nextcloud/circles#126

Merged

1 task

nickvergessen reviewed Sep 7, 2017

View reviewed changes

blizzz commented Sep 14, 2017

View reviewed changes

blizzz force-pushed the split-sharees-api-logic branch from 4ff9aec to 82c8193 Compare September 14, 2017 21:45

blizzz added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 14, 2017

blizzz requested review from rullzer and LukasReschke September 14, 2017 21:57

ArtificialOwl approved these changes Sep 15, 2017

View reviewed changes

blizzz added 16 commits September 26, 2017 23:10

updated autoloader

7122739

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

add missing PHP doc

2eff174

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

minor fixes

f446ec8

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

split off getUsers unit tests

dd9e191

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

split off getGroups unit tests

b35760d

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

split off getRemote unit tests

c92f7ee

adds two small fixes → they actually work \o/ Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

split off getEmail tests

3d1d03a

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

adjust ShareeAPIController search test

38ff7ea

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

adjust ShareeAPIController invalid-input search unit test

4a315ed

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

split off former searchSharee unit test

a28b1d9

also moves registering default plugins to Server for proper unit testing Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

adapt unit test for ensuring exception

3888b3a

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

move splitUserRemote unit tests

a7a651f

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

remove unused method

f2c8e1e

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

shareeAPIController unit test clean up and last fixes

c43685e

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

add tests for Lookup Plugin

3db3e65

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

reduce nesting of the required app XML

937a80c

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

blizzz force-pushed the split-sharees-api-logic branch from 82c8193 to 937a80c Compare September 26, 2017 23:22

blizzz mentioned this pull request Sep 26, 2017

adapt info.xml nextcloud/circles#135

Merged

adapt attribute name (changed letter case style)

a579f8a

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

blizzz mentioned this pull request Oct 2, 2017

Missing pieces: user mentions in Comments #2443

Closed

4 tasks

nickvergessen reviewed Oct 4, 2017

View reviewed changes

nickvergessen approved these changes Oct 4, 2017

View reviewed changes

blizzz merged commit 2d62f97 into master Oct 4, 2017

blizzz deleted the split-sharees-api-logic branch October 4, 2017 13:43

blizzz mentioned this pull request Oct 5, 2017

Show sharee in dropdown to send notification mail again #6276

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Splits off the logic from sharees endpoint thus making it available from within Nc/via PHP. #6328

Splits off the logic from sharees endpoint thus making it available from within Nc/via PHP. #6328

blizzz commented Aug 31, 2017 •

edited

Loading

mention-bot commented Aug 31, 2017

ArtificialOwl commented Sep 2, 2017

blizzz commented Sep 3, 2017

codecov bot commented Sep 5, 2017 •

edited

Loading

nickvergessen Sep 6, 2017

blizzz Sep 6, 2017

blizzz Sep 6, 2017

nickvergessen Sep 6, 2017

nickvergessen Sep 6, 2017

nickvergessen Sep 6, 2017

blizzz Sep 6, 2017

blizzz Sep 6, 2017

nickvergessen commented Sep 6, 2017

blizzz commented Sep 6, 2017

blizzz commented Sep 6, 2017

nickvergessen Sep 7, 2017

blizzz Sep 7, 2017

blizzz commented Sep 13, 2017

blizzz Sep 14, 2017 •

edited

Loading

blizzz Sep 14, 2017

blizzz commented Sep 14, 2017

blizzz commented Sep 26, 2017

codecov-io commented Sep 27, 2017 •

edited

Loading

blizzz commented Oct 4, 2017

nickvergessen commented Oct 4, 2017

nickvergessen commented Oct 4, 2017

nickvergessen Oct 4, 2017

blizzz Oct 4, 2017

Splits off the logic from sharees endpoint thus making it available from within Nc/via PHP. #6328

Splits off the logic from sharees endpoint thus making it available from within Nc/via PHP. #6328

Conversation

blizzz commented Aug 31, 2017 • edited Loading

mention-bot commented Aug 31, 2017

ArtificialOwl commented Sep 2, 2017

blizzz commented Sep 3, 2017

codecov bot commented Sep 5, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickvergessen commented Sep 6, 2017

blizzz commented Sep 6, 2017

blizzz commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blizzz commented Sep 13, 2017

blizzz Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blizzz commented Sep 14, 2017

blizzz commented Sep 26, 2017

codecov-io commented Sep 27, 2017 • edited Loading

Codecov Report

blizzz commented Oct 4, 2017

nickvergessen commented Oct 4, 2017

nickvergessen commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blizzz commented Aug 31, 2017 •

edited

Loading

codecov bot commented Sep 5, 2017 •

edited

Loading

blizzz Sep 14, 2017 •

edited

Loading

codecov-io commented Sep 27, 2017 •

edited

Loading