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

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

Merged
merged 24 commits into from Oct 4, 2017

Conversation

@blizzz
Copy link
Member

@blizzz blizzz commented Aug 31, 2017

@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
Copy link

@mention-bot mention-bot commented Aug 31, 2017

@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.

@blizzz blizzz requested review from nickvergessen, daita and schiessle Aug 31, 2017
@blizzz blizzz added this to the Nextcloud 13 milestone Aug 31, 2017
@daita
Copy link
Member

@daita daita commented Sep 2, 2017

could be fun to merge with #5280

@blizzz
Copy link
Member Author

@blizzz blizzz commented Sep 3, 2017

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

@blizzz blizzz force-pushed the split-sharees-api-logic branch from 5c7de25 to c97ca38 Sep 5, 2017
@codecov
Copy link

@codecov codecov bot commented Sep 5, 2017

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
use OCP\Collaboration\Collaborators\ISearchPlugin;
use OCP\Collaboration\Collaborators\ISearchResult;

class CirclePlugin implements ISearchPlugin {

This comment has been minimized.

@nickvergessen

nickvergessen Sep 6, 2017
Member

should be provided by the circles app?

This comment has been minimized.

@blizzz

blizzz Sep 6, 2017
Author Member

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.

This comment has been minimized.

@blizzz

blizzz Sep 6, 2017
Author Member

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).

$result['wide'] = [];
}

if (!$searchResult->hasExactIdMatch('emails') && filter_var($search, FILTER_VALIDATE_EMAIL)) {

This comment has been minimized.

@nickvergessen

nickvergessen Sep 6, 2017
Member

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

This comment has been minimized.

@nickvergessen

nickvergessen Sep 6, 2017
Member

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

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

This comment has been minimized.

@nickvergessen

nickvergessen Sep 6, 2017
Member

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

This comment has been minimized.

@blizzz

blizzz Sep 6, 2017
Author Member

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.

This comment has been minimized.

@blizzz

blizzz Sep 6, 2017
Author Member

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
Copy link
Member

@nickvergessen nickvergessen commented Sep 6, 2017

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
Copy link
Member Author

@blizzz blizzz commented Sep 6, 2017

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.

blizzz added a commit to nextcloud/circles that referenced this pull request Sep 6, 2017
required for nextcloud/server#6328

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz mentioned this pull request Sep 6, 2017
0 of 1 task complete
@blizzz
Copy link
Member Author

@blizzz blizzz commented Sep 6, 2017

@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'.

throw new \InvalidArgumentException('Type must not be empty');
}

if(trim($type) === 'exact') {

This comment has been minimized.

@nickvergessen

nickvergessen Sep 7, 2017
Member

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

This comment has been minimized.

@blizzz

blizzz Sep 7, 2017
Author Member

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

@blizzz
Copy link
Member Author

@blizzz blizzz commented Sep 13, 2017

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

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

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

return false;
return true;

This comment has been minimized.

@blizzz

blizzz Sep 14, 2017
Author Member

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

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

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

return false;
return true;

This comment has been minimized.

@blizzz

blizzz Sep 14, 2017
Author Member

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

@blizzz blizzz force-pushed the split-sharees-api-logic branch from 4ff9aec to 82c8193 Sep 14, 2017
@blizzz blizzz added 3. to review and removed 2. developing labels Sep 14, 2017
@blizzz
Copy link
Member Author

@blizzz blizzz commented Sep 14, 2017

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 😸

@blizzz blizzz requested review from rullzer and LukasReschke Sep 14, 2017
@daita
daita approved these changes Sep 15, 2017
blizzz added 15 commits Sep 7, 2017
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
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>
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>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz force-pushed the split-sharees-api-logic branch from 82c8193 to 937a80c Sep 26, 2017
@blizzz
Copy link
Member Author

@blizzz blizzz commented Sep 26, 2017

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
Copy link

@codecov-io codecov-io commented Sep 27, 2017

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 blizzz mentioned this pull request Oct 2, 2017
2 of 4 tasks complete
@blizzz
Copy link
Member Author

@blizzz blizzz commented Oct 4, 2017

🏓

@nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Oct 4, 2017

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

@nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Oct 4, 2017

Okay lets do it in a follow up

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

This comment has been minimized.

@nickvergessen

nickvergessen Oct 4, 2017
Member

For me this is an empty string (when removing the email after it was set before), and therefor results in an entry admin ()
bildschirmfoto vom 2017-10-04 15-24-54

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

This comment has been minimized.

@blizzz

blizzz Oct 4, 2017
Author Member

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 blizzz merged commit 2d62f97 into master Oct 4, 2017
4 checks passed
4 checks passed
Scrutinizer 3 new issues, 48 updated code elements
Details
codecov/patch 69.54% of diff hit (target 53.06%)
Details
codecov/project 53.09% (+0.03%) compared to 271959b
Details
continuous-integration/drone/pr the build was successful
Details
@blizzz blizzz deleted the split-sharees-api-logic branch Oct 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.