Page MenuHomePhabricator

Add filter by object ability to flag query
ClosedPublic

Authored by btrahan on Oct 24 2013, 7:29 PM.
Tags
None
Referenced Files
F14058594: D7392.diff
Sun, Nov 17, 1:14 PM
F14057313: D7392.id16653.diff
Sun, Nov 17, 2:13 AM
F14041342: D7392.id16678.diff
Mon, Nov 11, 5:41 PM
F14018358: D7392.diff
Tue, Nov 5, 9:40 AM
F14015938: D7392.id16649.diff
Mon, Nov 4, 3:11 AM
F14015914: D7392.id16649.diff
Mon, Nov 4, 2:42 AM
F14015441: D7392.id16653.diff
Sun, Nov 3, 6:53 PM
F14015328: D7392.id16653.diff
Sun, Nov 3, 5:35 PM

Details

Reviewers
epriestley
Maniphest Tasks
Restricted Maniphest Task
Commits
Restricted Diffusion Commit
rPda845460586c: Add filter by object ability to flag query
Summary

See title. Fixes T1809.

Test Plan

verified each type that has flaggable interface still can be flagged

verified that new custom query filter works

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

This looks great to me, see inline for discussion.

src/applications/flag/interface/PhabricatorFlaggableInterface.php
5

I think we can safely put getPHID() here now. Flags requires it and LiskDAO should always provide it.

src/applications/flag/query/PhabricatorFlagSearchEngine.php
104

This is kind of funky. The idea is that we want to render a dropdown with human-readable names, like "Task" instead of "ManiphestTask".

PHIDType already has these strings, so we can figure this out like:

$all_types = PhabricatorPHIDType::getAllTypes();
foreach ($objects as $object) {
  $phid = $object->generatePHID();
  $phid_type = phid_get_type($phid);
  $type_object = idx($all_types, $phid_type);
  if ($type_object) {
    $human_readable_name = $type_object->getTypeName();
  }
}

This will give us nice names -- "Task", "Revision", etc.

The generatePHID() part is a little funky. Possibly we should do this instead, in LiskDAO or wherever the base for generatePHID() lives:

public function getPHIDType() {
  return null;
}

public function generatePHID() {
  // return null if no phid type, or:
  return PhabricatorPHID::generatePHID($this->getPHIDType());
}

...or whatever (I don't have a copy open right now and might have goofed some method names). Basically, have subclasses like ManiphestTask just return their type instead of actually doing the generation. Only ApplicationTransaction and ApplicationTransactionComment have nontrivial logic in generatePHID().

But we could also skip this, there's no real harm in generating the PHIDs (they aren't written to the DB or anything).

btrahan updated this revision to Unknown Object (????).Oct 25 2013, 12:38 AM

using the generate_phid technique - I think we can think about something a bit cleaner if / when this comes up again.

{F74660}

is not the prettiest thing I have ever seen.

Maybe asort() the list (and then shove "All" on afterward, I guess)?

src/applications/flag/query/PhabricatorFlagQuery.php
15 ↗(On Diff #16653)

Maybe just use null as the default.

98–106 ↗(On Diff #16653)

Oh -- you should be able to do this in the DB. The table has a type column, which I believe is correctly populated.

btrahan updated this revision to Unknown Object (????).Oct 25 2013, 7:48 PM

updated...

  • use withTypes - no need to add the "withObjectType"
  • doing so moves the filtering to the DB and makes default null
  • make the list sorted and slap the default on the front at the end