Page MenuHomePhabricator

Update PhrictionSearchEngine, implement Projects
ClosedPublic

Authored by chad on Dec 3 2015, 5:29 PM.
Tags
None
Referenced Files
F12839305: D14652.diff
Thu, Mar 28, 7:20 PM
F12838959: D14652.id.diff
Thu, Mar 28, 6:54 PM
F12838588: D14652.id39766.diff
Thu, Mar 28, 6:22 PM
F12837942: D14652.id38840.diff
Thu, Mar 28, 5:41 PM
F12837624: D14652.id39766.diff
Thu, Mar 28, 5:16 PM
F12837491: D14652.id39759.diff
Thu, Mar 28, 5:06 PM
F12837313: D14652.id38840.diff
Thu, Mar 28, 4:51 PM
F12835344: D14652.id35447.diff
Thu, Mar 28, 3:07 PM

Details

Summary

Implements new search engine, projects into Phriction.

Test Plan

Test new search choices, add project to Phriction page. Verify hierarchy still works, setting policy works, etc.

Diff Detail

Repository
rP Phabricator
Branch
arcpatch-D14652
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 13629
Build 17565: Run Core Tests
Build 17564: arc lint + arc unit

Event Timeline

chad retitled this revision from to Update PhrictionSearchEngine, implement Projects.
chad updated this object.
chad edited the test plan for this revision. (Show Details)
chad added a reviewer: epriestley.

(I haven't forgotten about this, but I don't remember the order stuff off the top of my head and it's a bit tricky, so I'm going to have to dig into that a little to give you an answer.)

Not a priority, feel free to just commandeer down the road.

chad edited edge metadata.

rebase

  • fix order bug, fix edit bug

probably reviewable? been toggling between master and branch, nothing seems broken.

epriestley edited edge metadata.

I think this has a policy problem? See inline.

src/applications/phriction/query/PhrictionDocumentQuery.php
97–124

Although this code should move to willFilterPage(), I think we still need it.

Without this code, I would expect policies to fail to apply correctly to child pages. For example:

  • Set a/ to "Visible To: Admins".
  • Set a/b/ to "Visible To: All Users".
  • View a/b/ as a non-admin.
  • On master: Page should not be visible.
  • With this patch: I expect the page is incorrectly visible, or (hopefully) there's a fatal?
src/applications/phriction/storage/PhrictionDocument.php
100–107

I think we should probably trend away from having attachProjectPHID() methods, since EditEngine and other infrastructure do it for you in modern code and this would have no callers if everything was fully modernized. In legacy code, fine to just EdgeQuery directly when you need this data so everything gets deleted when the code is modernized. Definitely not a big deal, though -- code is fine, just risks not getting cleaned up when we modernize fully.

Actually, it looks like none of this has callers even in this diff, since the EditController changes do just use EdgeQuery directly? Just nuke these changes?

This revision now requires changes to proceed.Sep 8 2016, 2:20 PM
src/applications/phriction/query/PhrictionDocumentQuery.php
97–124

Yeah, I don't know what the fix is here after fiddling with it for a while. It looked like we filter pages already with willFilterPage, How do I integrate this check in with it?

src/applications/phriction/query/PhrictionDocumentQuery.php
171–173

can't say I understand this either.

src/applications/phriction/query/PhrictionDocumentQuery.php
98–123

Just copy this whole block into willFilterPage() at the top.

willFilterPage() does the check, but it depends on page ancestry data which is provided by this block. If we don't run this code, we never set the correct ancestors on the documents, so when we go to run the check it just looks like every document has no ancestors and doesn't need any special rules/policies.

src/applications/phriction/query/PhrictionDocumentQuery.php
98–123

How do I get a db connection running?

src/applications/phriction/query/PhrictionDocumentQuery.php
171–173

The code above uses unset(...) to remove documents from the list, so it's possible we'll reach this point with zero documents left.

If the list is empty, this just returns the empty list early before doing more work.

If we didn't and needContent was set, we'd issue a bad id IN (<empty>) query on line 176.

src/applications/phriction/query/PhrictionDocumentQuery.php
98–123

It's fine to just do this:

$table = new PhrictionDocument();
$conn = $table->establishConnection('r');

That will properly pool/reuse the connection internally.

Oh I guess we need some inline HTML on {key ...} for email. I tested text mode but not HTML email mode.

chad edited edge metadata.
  • fix policy bugs
chad edited the test plan for this revision. (Show Details)
chad edited edge metadata.
epriestley edited edge metadata.

Cool, seems OK to me locally too from banging on it a bit.

This revision is now accepted and ready to land.Sep 8 2016, 11:24 PM
This revision was automatically updated to reflect the committed changes.