Add repositories to fulltext search index.
Needs RevisionPublic

Authored by 20after4 on Thu, Feb 2, 6:34 PM.

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Summary

This implements a simplistic PhabricatorRepositoryFulltextEngine
Currently only the repository name, description, timestamps and
status are indexed.

Note: I had to change the search index workflow to disambiguate
PhabricatorRepository from PhabricatorRepositoryCommit

Test Plan

ran ./bin/search index --type PhabricatorRepository --force
then searched for some repositories. Saw reasonable results matching on
either title or description.

Diff Detail

Repository
rP Phabricator
Branch
master
Lint
Lint OK
Unit
Unit Tests OK
Build Status
Buildable 15507
Build 20444: Run Core Tests
Build 20443: arc lint + arc unit
20after4 created this revision.Thu, Feb 2, 6:34 PM
epriestley requested changes to this revision.Fri, Feb 3, 1:55 PM

Looks good. Couple of minor nits inline.

Your test plan should also include something like this:

  • Edited a repository to have "platypus" in the description, searched for "platypus", found the repository.

Even when bin/search index works, the actual "edit stuff with the web UI" workflow may still not be fully hooked up. Usually, this is because the Editor does not override supportsSearch(), so edits don't rebuild the index. In this case it looks like you're in the clear, so I'd expect this to work correctly with no further changes, but it's worth double checking.

src/applications/repository/search/PhabricatorRepositoryFulltextEngine.php
9

This kind of documentation is unconventional in this codebase, and the docs say $repository while the actual variable is $repo. I think this is perfectly clear from context anyway, just nuke it?

14

Newline is probably a better delimiter than colon, since users might reasonably search for "x: y" and get a false/confusing positive here. (In theory, they can search for a newline too, I guess, but not reasonably using the normal UI).

This revision now requires changes to proceed.Fri, Feb 3, 1:55 PM
20after4 marked 2 inline comments as done.Sat, Feb 18, 8:40 PM

Looks good. Couple of minor nits inline.

Your test plan should also include something like this:

  • Edited a repository to have "platypus" in the description, searched for "platypus", found the repository.

Even when bin/search index works, the actual "edit stuff with the web UI" workflow may still not be fully hooked up. Usually, this is because the Editor does not override supportsSearch(), so edits don't rebuild the index. In this case it looks like you're in the clear, so I'd expect this to work correctly with no further changes, but it's worth double checking.

Ok I'll test re-indexing, I think that works fine but you're right that I should have tested more thoroughly and mentioned it in my test plan. ;)

Also, thanks for pointing out "supportsSearch" I wasn't aware of the need to override that method.

src/applications/repository/search/PhabricatorRepositoryFulltextEngine.php
9

I was trying to get code intelligence to act intelligently. It didn't work and I forgot to delete the comment.

Code intelligence: still not very intelligent in 2017.

14

Good point.