Allow to search files by comments #9222

nickvergessen · 2018-04-18T09:31:56Z

PS: this is an accident, I was just preparing search for chat messages in talk.

nickvergessen · 2018-04-18T09:32:56Z

lib/private/Comments/Manager.php

+ * @param string $verb Limit the verb of the comment
+ * @return IComment[]
+ */
+ public function search(string $search, string $objectType, string $objectId, string $verb): array {


I guess we should add an limit/offset here, opinions?

Can lead to pretty big requests, yes.
Is this an admin only tool?

No, it's the normal search in the top right, which finds files for you 💃

The problem with pagination is, we paginate the comments. Afterwards we need to look if the user can actually see the file where the comment lives. so while we paginate, the page could still be empty, unless we loop until we found enough comments....

But offset is pretty hard with that, so maybe we don't go for offset at all.

A limit certainly makes sense. Pagination is hairy without a reliable offset. This is not solved in LDAP either, there you also need to start to start from scratch. So this needs some extra handling, and is costly of course… unless here if there is a possibility to know the last item from the previous batch. Then you could it's timestamp and id as starting point. Usually you don't and tracking this over requests is cumbersome. Fake pagination with retrieving more results than shown (also as follow-up request) can be a dirty work around.

The other approach would be having very specialized search implementations that do the permission checks within the same DB query.

nickvergessen · 2018-04-18T09:34:05Z

apps/comments/lib/Search/CommentSearchResult.php

+ $suffix = '…';
+ }
+
+ return $prefix . substr($message, $start, $end - $start) . $suffix;


This is rather arbitrary atm, we could also search for the beginning/end of the next word and if it is within the range cut there. On the other hand I like simplicity

blizzz · 2018-04-18T14:32:10Z

apps/comments/lib/Search/CommentSearchResult.php

+use OCP\Files\NotFoundException;
+use OCP\Search\Result;
+
+class CommentSearchResult extends Result {


Since Comment and Search are already present in the namespace, calling the class Result would suffice. The Comment's Manager is also just called Manager. And the new Provider is also just Provider. It's not that I care too much about the naming, but perhaps we should go to a consistent scheme, which ever it is.

I just used a more specific one because Result would conflict with the extend.

could be imported under an alias or using full name space. anyway, not that important.

blizzz · 2018-04-18T14:39:45Z

apps/comments/lib/Search/CommentSearchResult.php

+ * @param string $path
+ * @throws NotFoundException
+ */
+ public function __construct(string $search,


Passing the IComment would reduce at least 2 parameters and be more flexible to whatever in the future (e.g. when taking the author type into consideration).

blizzz · 2018-04-18T14:41:27Z

apps/comments/lib/Search/Provider.php

+ * @since 7.0.0
+ */
+ public function search($query): array {
+ $cm = \OC::$server->getCommentsManager();


missing DI is due to legacy around search infrastructure I guess?

yeah new $class($options) prevents decent DI…

blizzz · 2018-04-18T14:44:52Z

apps/comments/lib/Search/Provider.php

+
+ $result = [];
+ foreach ($comments as $comment) {
+ if ($comment->getActorType() !== 'users') {


Why the limitation? If it is about the displayName , the Comments Manager can resolve them.

blizzz · 2018-04-18T14:47:17Z

lib/private/Comments/Manager.php

+ * @param string $verb Limit the verb of the comment
+ * @return IComment[]
+ */
+ public function search(string $search, string $objectType, string $objectId, string $verb): array {


A limit certainly makes sense. Pagination is hairy without a reliable offset. This is not solved in LDAP either, there you also need to start to start from scratch. So this needs some extra handling, and is costly of course… unless here if there is a possibility to know the last item from the previous batch. Then you could it's timestamp and id as starting point. Usually you don't and tracking this over requests is cumbersome. Fake pagination with retrieving more results than shown (also as follow-up request) can be a dirty work around.

The other approach would be having very specialized search implementations that do the permission checks within the same DB query.

codecov · 2018-04-19T15:22:44Z

Codecov Report

Merging #9222 into master will increase coverage by 28.21%.
The diff coverage is 1.01%.

@@              Coverage Diff              @@
##             master    #9222       +/-   ##
=============================================
+ Coverage      6.37%   34.58%   +28.21%     
+ Complexity    26228    26128      -100     
=============================================
  Files          1673     1653       -20     
  Lines         96950    96494      -456     
  Branches       1290     1290               
=============================================
+ Hits           6184    33376    +27192     
+ Misses        90766    63118    -27648

Impacted Files	Coverage Δ	Complexity Δ
...pps/comments/composer/composer/autoload_static.php	`0% <ø> (ø)`	`1 <0> (ø)`	⬇️
lib/public/Search/Provider.php	`0% <ø> (ø)`	`7 <0> (ø)`	⬇️
apps/comments/lib/Search/Provider.php	`0% <0%> (ø)`	`10 <10> (?)`
lib/private/Comments/Manager.php	`90.9% <0%> (+90.9%)`	`96 <6> (+6)`	⬆️
...s/comments/composer/composer/autoload_classmap.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
apps/comments/lib/Search/Result.php	`0% <0%> (ø)`	`7 <7> (?)`
apps/comments/lib/AppInfo/Application.php	`100% <100%> (+100%)`	`1 <0> (ø)`	⬇️
lib/private/Memcache/Redis.php	`0% <0%> (-85.72%)`	`24% <0%> (ø)`
lib/private/Files/ObjectStore/Azure.php	`0% <0%> (-84.38%)`	`14% <0%> (ø)`
apps/updatenotification/lib/UpdateChecker.php	`0% <0%> (ø)`	`8% <0%> (-2%)`	⬇️
... and 647 more

blizzz · 2018-04-19T20:43:55Z

lib/public/Comments/ICommentsManager.php

+ * @return IComment[]
+ * @since 14.0.0
+ */
+ public function search(string $search, string $objectType, string $objectId, string $verb): array;


FakeManager in Tests needs to implement this as well

MorrisJobke · 2018-05-17T15:07:38Z

It doesn't find the comments for me - it also never gets into the search provider of comments when adding a breakpoint there :/

MorrisJobke · 2018-06-21T21:36:05Z

🏓

nickvergessen · 2018-06-25T13:49:35Z

Fixed searching, however the results are not looking like the screenshot anymore.
Was there a major redesign/refactoring of the search result design?

MorrisJobke · 2018-06-25T13:54:07Z

Was there a major redesign/refactoring of the search result design?

Yes - #9912

MorrisJobke · 2018-06-25T15:30:05Z

The search itself doesn't work for me. I shared a file with another user and both commented with:

Lorem ipsum dolor sit amet, consectetur, adipisci velit

Searching then for Lorem does not show any search result. It was on a file. Anything that I miss?

nickvergessen · 2018-07-02T14:00:47Z

I just rebased and tested this again: works pretty fine here for both the sharer and the sharee

MorrisJobke · 2018-07-05T10:01:24Z

Doesn't show anything for me on this branch (left is heinz, right is admin, both commented with a string that is then searched for):

What do I do wrong?

nickvergessen · 2018-07-17T12:04:14Z

So the entry point is

server/core/Controller/SearchController.php

Line 49 in 7674e59

$results = $this->searcher->searchPaged($query, $inApps, $page, $size);

Once I step into that, after the init method, I can see this->providers containing the new OCA\Comments\Search\Provider:

server/lib/private/Search.php

Line 49 in 21a720e

$this->initProviders();

And from there on the debugger should be able to step through and show you where you are leaving the trail

MorrisJobke · 2018-07-18T07:31:26Z

Once I step into that, after the init method, I can see this->providers containing the new OCA\Comments\Search\Provider:

For me that was not the case 😢

MorrisJobke · 2018-07-18T07:40:18Z

The constructor of the class OCA\Comments\AppInfo\Application is never called. I needed to actively instantiate it in the app.php of the comments app. Then the provider is properly loaded but the UI is still broken:

I added this to the app.php:

use OCA\Comments\AppInfo\Application;
$app = new Application();

nickvergessen · 2018-07-18T08:13:31Z

Fixed the code so the constructor is always called.

MorrisJobke · 2018-07-18T15:52:21Z

it still looks like that and the path and the avatar is not set properly:

nickvergessen · 2018-07-18T17:21:13Z

Anyone with more frontend knowledge volunteering to do that?

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

danxuliu · 2018-07-25T09:11:43Z

I will take care of this.

danxuliu · 2018-07-25T16:39:55Z

I have rebased on master and made the following changes:

Moved the last commit to the first one
Squashed the fixes into the commits that they fixed
Restored the return type in the documentation of OCP\Search\Provider::search from Result[] to array (of course feel free to add it back, but please modify it everywhere, not just in the base class leaving all the subclasses dangling with the old type ;-) )
Fixed duplicated search results (for some reason, that I have not digged into, the application is created twice; as the provider was registered in the constructor it was registered twice and results were duplicated; now the provider is registered in register())
Fixed search before pagination was introduced (the parameters were passed in wrong order to $cm->search, and users should be passed to resolveDisplayName)
Fixed UI (the plugin now has to be registered with OCA.Search.Core instead of OCA.Search)
Renamed renderFileResult and handleFileClick to renderCommentResult and handleCommentClick (nitpicking, the name should refer to the type of the result)
Added integration tests (no time for unit or acceptance tests, sorry ;-) )

All in all just minor changes; besides the integration tests the bulk of the pull request is still the same (and I have not modified nor tested the pagination). In any case, it has my 👍

Some details:

The link to the file is not included in the search results
The name property in the result includes the full comment, without ellipsis; I have no idea if that is right or not. In any case the UI shows the ellipsized version.

danxuliu · 2018-07-25T16:51:35Z

~~Ok, give me a second, I made a stupid change... :P~~ Done.

What have we learned, kids? Yes, you do not see a parameter without type and think Oh, let's add the missing type and push, no need to test, it is just a minor change and everything will be fine :-P

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

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

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

Signed-off-by: Morris Jobke <hey@morrisjobke.de>

MorrisJobke

Tested and works 👍

MorrisJobke · 2018-07-25T18:29:40Z

I just made the icon brighter (top before, bottom after):

MorrisJobke · 2018-07-25T18:29:55Z

In any case, it has my 👍

-> Merging

nickvergessen added enhancement 3. to review Waiting for reviews feature: comments feature: search labels Apr 18, 2018

nickvergessen added this to the Nextcloud 14 milestone Apr 18, 2018

nickvergessen requested review from rullzer and blizzz April 18, 2018 09:31

nickvergessen commented Apr 18, 2018

View reviewed changes

blizzz reviewed Apr 18, 2018

View reviewed changes

blizzz reviewed Apr 19, 2018

View reviewed changes

nickvergessen force-pushed the feature/noid/search-for-files-by-comments branch 2 times, most recently from 16a35e4 to 686842e Compare April 23, 2018 13:01

MorrisJobke added the stale Ticket or PR with no recent activity label Jun 19, 2018

nextcloud-bot removed the stale Ticket or PR with no recent activity label Jun 21, 2018

nickvergessen force-pushed the feature/noid/search-for-files-by-comments branch from 686842e to a2c03a5 Compare June 25, 2018 13:48

nickvergessen force-pushed the feature/noid/search-for-files-by-comments branch from a2c03a5 to 8fbdab5 Compare July 2, 2018 14:00

nickvergessen force-pushed the feature/noid/search-for-files-by-comments branch from 8fbdab5 to 5ea544c Compare July 17, 2018 14:19

MorrisJobke mentioned this pull request Jul 24, 2018

14.0.0 Beta 1 #10370

Merged

21 tasks

Correctly register everything via the Application class

80207d7

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

MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jul 25, 2018

danxuliu force-pushed the feature/noid/search-for-files-by-comments branch from 29078e1 to 3fa4ad4 Compare July 25, 2018 16:36

nickvergessen and others added 3 commits July 25, 2018 18:53

Allow to search by comments

8c7969e

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

Add integration tests for searching files by comments

1e16f7e

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

Add pagination support

ac2314e

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

danxuliu force-pushed the feature/noid/search-for-files-by-comments branch from 3fa4ad4 to ac2314e Compare July 25, 2018 16:54

Brighter comments icon

2dd55b0

Signed-off-by: Morris Jobke <hey@morrisjobke.de>

MorrisJobke approved these changes Jul 25, 2018

View reviewed changes

MorrisJobke merged commit 61397ee into master Jul 25, 2018

MorrisJobke deleted the feature/noid/search-for-files-by-comments branch July 25, 2018 18:30

MorrisJobke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 25, 2018

Allow to search files by comments #9222

Allow to search files by comments #9222

Conversation

nickvergessen commented Apr 18, 2018

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

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

codecov bot commented Apr 19, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

MorrisJobke commented May 17, 2018

MorrisJobke commented Jun 21, 2018

nickvergessen commented Jun 25, 2018

MorrisJobke commented Jun 25, 2018

MorrisJobke commented Jun 25, 2018

nickvergessen commented Jul 2, 2018

MorrisJobke commented Jul 5, 2018

nickvergessen commented Jul 17, 2018

MorrisJobke commented Jul 18, 2018

MorrisJobke commented Jul 18, 2018 • edited Loading

nickvergessen commented Jul 18, 2018

MorrisJobke commented Jul 18, 2018

nickvergessen commented Jul 18, 2018

danxuliu commented Jul 25, 2018

danxuliu commented Jul 25, 2018 • edited Loading

danxuliu commented Jul 25, 2018 • edited Loading

MorrisJobke left a comment

Choose a reason for hiding this comment

MorrisJobke commented Jul 25, 2018

MorrisJobke commented Jul 25, 2018

codecov bot commented Apr 19, 2018 •

edited

Loading

MorrisJobke commented Jul 18, 2018 •

edited

Loading

danxuliu commented Jul 25, 2018 •

edited

Loading

danxuliu commented Jul 25, 2018 •

edited

Loading