Summary: Use `PhutilClassMapQuery` where appropriate.
Test Plan: Browsed around the UI to verify things seemed somewhat working.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13429
Summary:
Fixes T8727. When I added `setParentQuery()`, I increased the cache hit rate but also accidentally propagated policy exception settings.
Instead, make the policy exception beahvior explicit: no exceptions should be raised when querying handles.
Test Plan: Will test production.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8727
Differential Revision: https://secure.phabricator.com/D13489
Summary:
Ref T8631. The query plan for feed stories is really bad right now, because we miss caches we should be hitting:
- The workspace cache is stored at each query, so adjacent queries can't benefit from the cache (only subqueries). Feed has primarily sibling queries.
- There is no technical reason to do this. Store the workspace cache on the root query, so sibling queries can hit it.
- In `ObjectQuery`, we check the workspace once, then load all the PHIDs. When the PHIDs are a mixture of transactions and objects, we always miss the workspace and load the objects twice.
- Instead, check the workspace after loading each type of object.
- `HandleQuery` does not set itself as the parent query for `ObjectQuery`, so handles never hit the workspace cache.
- Pass it, so they can hit the workspace cache.
- Feed's weird `PhabricatorFeedStory::loadAllFromRows()` method does not specify a parent query on its object/handle queries.
- Just declare the object query to be the "root" query until this eventually gets cleaned up.
Test Plan: Saw queries for each object drop from 4-6x to 1x in `/feed/`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8631
Differential Revision: https://secure.phabricator.com/D13479
Summary: These class properties don't seem to be used.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13295
Summary: All classes should extend from some other class. See D13275 for some explanation.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13283
Summary: Ref T8449. Get rid of the `S123` stuff in headers, it feels like clutter.
Test Plan: Looked at objects in Spaces.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8449
Differential Revision: https://secure.phabricator.com/D13243
Summary: Ref T8387. This is now completely obsoleted by mailing list users.
Test Plan: Grepped for `mailinglist` and related symbols.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: eadler, epriestley
Maniphest Tasks: T8387
Differential Revision: https://secure.phabricator.com/D13129
Summary: This class is no longer used.
Test Plan: `grep`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13117
Summary:
Fixes T7923.
Prevent the user from finding tasks that they can't edit in merge workflows. Also ensure that we query properly on final merge action just in case.
Test Plan: Tried to find a task I couldn't edit in various searches under the "merge" dialogue and couldn't find the task. Removed this big of code and tried to merge in a task and after hitting "merge" observed the page reloaded with no task merged in.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7923
Differential Revision: https://secure.phabricator.com/D12872
Summary:
Ref T7707. Handles currently have a "status" field and a "disabled" field.
The "status" field has these possible values: "open", "closed", "1", "2". durp durp durp
Instead, do:
- status = <open, closed>
- availability = <full, partial, none, disabled>
I think these make more sense? And are a bit more general? And use the same kind of constants for all values!
Test Plan: Looked at all affected handles in all states (probably).
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7707
Differential Revision: https://secure.phabricator.com/D12832
Summary: Use `__CLASS__` instead of hard-coding class names. Depends on D12605.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12806
Conpherence thread handles need to load other handles in order to load. Currently, HandlePool can loop when reentered. Instead, clear the on-deck list before querying so that reentering it will query for only new handles, not reissue queries for in-flight handles.
Auditors: btrahan
Fixes T8140. If a HandleList contained a null (because some caller sloppily added `null` as a handle), iteration (e.g., via `iterator_to_array()`) would abort prematurely.
In T8140, some of the project transactions add a `null` for an old file PHID when there's no old profile image.
Auditors: btrahan
Fixes T8125. In Feed, we query for a bunch of objects and also a bunch of transactions.
The transactions require the objects. Normally, whichever executes last will fill out of the Workspace cheaply, so this query strategy is fine overall.
The new "in-flight" code would mark everything in flight before the transactions loaded, though, so they'd fail to load even though the query plan is not cyclic.
Instead, be more surgical and mark things in flight only immediately before we put them in flight.
Test Plan: Feed now shows more stories again; files with cycles still load in finite time.
Auditors: btrahan
Summary:
Fixes T6726. Currently, a file may be attached to itself (or to other files, ultimately forming a loop). In this case, we currently run around the loop forever trying to load all the files.
Instead, decline to load objects if we're inside a query which is already loading them. This produces the right policy result //and// completes in finite time.
Test Plan:
- Looped two files by writing `{F123}` and `{F124}` on the other files, respectively.
- Loaded `F123`.
- Saw long hang; used `debug.time-limit` to see huge stack trace instead.
- Wrote patch.
- `F123` now loads correctly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6726
Differential Revision: https://secure.phabricator.com/D12756
Summary:
- Now that we have "browse", this is a much more reasonable control for random sets of things.
- The new explicit search scope selector reduces the need to fiddle with this field manually, too.
Test Plan:
{F379292}
{F379293}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12510
Summary:
Ref T4100. This just makes the "specify stuff in query parameters" workflow a little better:
- You can now do `?projects=differential,diffusion`.
- You can now do `?projects=projects(alincoln)`.
Test Plan: Did that stuff ^^^^
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4100
Differential Revision: https://secure.phabricator.com/D12468
Summary:
Ref T4100. Overall:
- Use token background color to communicate token type (blue = object, yellow = function, grey = disabled/closed, red = invalid).
- Use token icon color to make color choices consistent (specifically, use project icon colors in project tokens).
- For functions, use token icon to communicate function result type (e.g., viewer() has a user icon; members(...) has a group icon), since we don't need the icon to indicate "this is a function" anymore.
Test Plan:
{F374615}
{F374616}
{F374617}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4100
Differential Revision: https://secure.phabricator.com/D12446
Summary:
Ref T7803. Ref T5873. I want to drive Conduit through more shared infrastructure, but can't currently add parameters automatically.
Put a `getX()` around the `defineX()` methods so the parent can provide default behaviors.
Also like 60% of methods don't define any special error types; don't require them to implement this method. I want to move away from this in general.
Test Plan:
- Ran `arc unit --everything`.
- Called `conduit.query`.
- Browsed Conduit UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: hach-que, epriestley
Maniphest Tasks: T5873, T7803
Differential Revision: https://secure.phabricator.com/D12380
Summary:
Ref T7689. This gives HandleLists `renderList()` and `renderHandle()` methods, which return views that can perform just-in-time data fetching and generally look and feel like other rendering code, instead of being odd pseudo-functional methods on `Controller`.
Also converts callsites on the Maniphest detail page to use these methods.
Next changes will wipe out more of the callsites.
Test Plan:
- Viewed Maniphest detail page with many relevant handles.
- Created a new subtask.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7689
Differential Revision: https://secure.phabricator.com/D12205
Summary:
Ref T7689, which discusses some of the motivation here. Briefly, these methods are awkward:
- Controller->loadHandles()
- Controller->loadViewerHandles()
- Controller->renderHandlesForPHIDs()
This moves us toward better semantics, less awkwardness, and a more reasonable attack on T7688 which won't double-fetch a bunch of data.
Test Plan:
- Added unit tests.
- Converted one controller to the new stuff.
- Viewed countdown lists, saw handles render.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7689
Differential Revision: https://secure.phabricator.com/D12202
Summary: Ref T5952, T3404. This lays the basic plumbing for how this will work, all the way to deploying on Maniphest. Aside from what is mentioned on T5952, I think page(s) on editing application emails could use a little more helpful text about what's going on, similar to how the config page that's getting deprecated works.
Test Plan: ran migration and noted my create email address migrated successfully. used bin/mail to make a task. added another email and used bin/mail to make a task. deleted an email. edited an email. invoked various error states and they all looked good.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T3404, T5952
Differential Revision: https://secure.phabricator.com/D11418
Summary: Ref T6741. As an optimization, we omit policy checks on ObjectQuery, but need to retain them if the query requests non-view permissions. This primarily affected Almanac properties.
Test Plan: Almanac properties now do full policy checks on ObjectQuery loads.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6741
Differential Revision: https://secure.phabricator.com/D11005
Summary:
Ref T4712. Thus far, it seems that most "non-standard" things can be done pretty easily in the controller. Aside from deploying, this diff had to fix a few bugs / missing implementations of stuff.
(Notably, PhabricatorAuthProviderConfig, HeraldRule, PhabricatorSlowvotePoll, and AlmanacNetwork needed to implement PhabricatorApplicationTransactionInterface, PhabricatorAuthAuthProviderPHIDType had to be added, and a rendering bug in transactions of type PhabricatorOAuth2AuthProvider had to be fixed.)
Test Plan: Almanac - looked at binding, device, network, and service view controllers and verified timeline displayed properly. Herald - looked at a rule and verified timeline. Slowvote - looked at a vote and verified timeline. Auth - looked at an auth provider (Facebook) and verified proper display of transactions within timeline.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10921
Summary:
Ref T2787. These were still stuck in the stone ages.
(The handles are pretty skeletal but most aren't used anywehre.)
Test Plan: Funded an initiative without anything breaking. Grepped for removed constants.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10647
Summary: Ref T4896. This was used by the old audit comment storage, which is now defunct.
Test Plan: Grepped for callsites in the codebase.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10152
Summary: Ref T5655. Rename `PhabricatorPHIDType` subclasses for clarity (see discussion in D9839). I'm not too keen on some of the resulting class names, so feel free to suggest alternatives.
Test Plan: Ran unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, hach-que
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9986
Summary: Ref T5655. Some discussion in D9839. Generally speaking, `Phabricator{$name}Application` is clearer than `PhabricatorApplication{$name}`.
Test Plan:
# Pinned and uninstalled some applications.
# Applied patch and performed migrations.
# Verified that the pinned applications were still pinned and that the uninstalled applications were still uninstalled.
# Performed a sanity check on the database contents.
Reviewers: btrahan, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: hach-que, epriestley, Korvin
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9982
Summary: Instead of implementing the `getTypeConstant` method in all subclasses of `PhabricatorPHIDType`, provide a `final` implementation in the base class which uses reflection. See D9837 for a similar implementation.
Test Plan: Ran `arc unit`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9985
Summary: I'm pretty sure that `@group` annotations are useless now... see D9855. Also fixed various other minor issues.
Test Plan: Eye-ball it.
Reviewers: #blessed_reviewers, epriestley, chad
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9859
Summary: Toss the hard-codes and use slim tags.
Test Plan: Scoped out task list.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9748
Summary:
Standardizes tag rendering in Maniphest and Maniphest/Diffusion list views.
(This might need some size/spacing tweaks, I tried to make it look reasonable.)
Test Plan: See screenshots.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9741
Summary: This further helps differentiate types/roles for projects.
Test Plan: {F169758}
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9710
Summary: Ref T2628. There are a few UIs that need updates, but generally I want to show project icons everywhere that we show project names, to more strongly reinforce the ideas of projects being groups/tags/policies/etc.
Test Plan: See screenshot.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T2628
Differential Revision: https://secure.phabricator.com/D9709
Summary: Ran `arc lint --apply-patches --everything` over rP, mainly to change double quotes to single quotes where appropriate. These changes also validate that the `ArcanistXHPASTLinter::LINT_DOUBLE_QUOTE` rule is working as expected.
Test Plan: Eyeballed it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9431
Summary: Fixes T5090. Introduced getIcon into Handle stack which allows you to specify a per handle icon. getIcon falls back ot getTypeIcon.
Test Plan: changed the icon on a project a bunch. verified transactions showed up. verified icon showed up in typeahead. verified icon showed up in tokens that were pre-generated (not typed in). units test passed.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5090
Differential Revision: https://secure.phabricator.com/D9264
Summary: The removes the sprite sheet 'icons' and replaces it with FontAwesome fonts.
Test Plan:
- Grep for SPRITE_ICONS and replace
- Grep for sprite-icons and replace
- Grep for PhabricatorActionList and choose all new icons
- Grep for Crumbs and fix icons
- Test/Replace PHUIList Icon support
- Test/Replace ObjectList Icon support (foot, epoch, etc)
- Browse as many pages as I could get to
- Remove sprite-icons and move remarkup to own sheet
- Review this diff in Differential
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9052
Summary:
Fixes T4917. Currently, if a user doesn't have access to, e.g., Phriction, they still get a checkbox in the search results to search for Wiki Documents. Those results will be filtered anyway, so this is confusing at best.
Instead, bind PHID types to applications. This is a relatively tailored fix; some areas for potential future work:
- Go through every PHID type and bind them all to applications. Vaguely nice to have, but doesn't get us anything for now.
- If no searchable application is installed, we don't show you an error state. This isn't currently possible ("People" is always installed) but in the interest of generality we could throw an exception or something at least.
- The elasticserach thing could probably constrain types to visible types, but we don't have a viewer there easily right now.
Test Plan: Uninstalled Phriction, saw the checkbox vanish.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4917
Differential Revision: https://secure.phabricator.com/D8904
Summary:
There are quite a few tests in Arcanist, libphutil and Phabricator that do something similar to `$this->assertEqual(false, ...)` or `$this->assertEqual(true, ...)`.
This is unnecessarily verbose and it would be cleaner if we had `assertFalse` and `assertTrue` methods.
Test Plan: I contemplated adding a unit test for the `getCallerInfo` method but wasn't sure if it was required / where it should live.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8460
