Summary:
Ref T10538. Ref T10537. This creates PHIDs which represent GitHub users, and uses them as the actors for synchronized comments.
I've just made them Doorkeeper objects. There are three major kinds of objects they //could// possibly be:
- Nuance requestor objects.
- External account objects.
- Doorkeeper objects.
I don't think we actually need distinct nuance requestor objects. These don't really do anything right now, and were originally created before Doorkeeper. I think Doorkeeper is a superset of nuance requestor functionality, and better developed and more flexible.
Likewise, doorkeeper objects are much more flexible than external account objects, and it's nice to imagine that we can import from Twootfeed or whatever without needing to build full OAuth for it. I also like less stuff touching auth code, when possible.
Making these separate from external accounts does make it a bit harder to reconcile external users with internal users, but I think that's OK, and that it's generally desirable to show the real source of a piece of content. That is, if I wrote a comment on GitHub but also have a Phabricator account, I think it's good to show "epriestley (GitHub)" (the GitHub user) as the author, not "epriestley" (the Phabricator user). I think this is generally less confusing overall, and we can add more linkage later to make it clearer.
Test Plan:
{F1194104}
{F1194105}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537, T10538
Differential Revision: https://secure.phabricator.com/D15541
Summary: Fixes T10648. This was goofed and always did a meaningless no-op slice -- I mucked it up while doing the disabled project stuff elsewhere.
Test Plan:
- Tagged something with 5 projects.
- Saw the list sliced to 4 (the limit) with "...".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10648
Differential Revision: https://secure.phabricator.com/D15508
Summary:
adds the `{{PHID....}}` rule. Should mostly be useful in UI code that refers to Objects.
It doesn't add any mention links/transactions.
Test Plan: Comment with this, see email (plain + html) and comment box.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15488
Summary: Ref T10457. This is mostly just for consitency, but I imagine it will make managing large/complex build processes easier, and if we support Herald rules it would eventually let you write "Build plan's tags include [whatever]" to apply behavior to a group of plans.
Test Plan: {F1133107}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10457
Differential Revision: https://secure.phabricator.com/D15360
Summary: Fixes T10413. I accidentally hid these //everywhere//, but only intended to hide them on workboards.
Test Plan:
- Viewed a workboard, saw un-archived projects only.
- Viewed a task detail page, saw archived and un-archived projects.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10413
Differential Revision: https://secure.phabricator.com/D15335
Summary: Ref T10349.
Test Plan:
- Added archived and unarchived project tags to a task.
- Saw unarchived tags, only, on cards.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10349
Differential Revision: https://secure.phabricator.com/D15297
Summary: No UI changes, just some search and replace for UI consistency.
Test Plan: Test person and object hovercards still work. UIExamples too.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15172
Summary: Fixes T10247. The flavor icons are unhelpful/confusing in these contexts; show a boringer icon instead.
Test Plan: Used tokenizer to select user with custom profile icon. Reloaded page. Saw boringer icon in both cases.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10247
Differential Revision: https://secure.phabricator.com/D15154
Summary: Mostly for consistency, we're not using other forms of icons and this makes all classes that use an icon call it in the same way.
Test Plan: tested uiexamples, lots of other random pages.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15125
Summary: Ref T10054. This primarily improves aesthetics and consistency for member/wathcher lists in projects.
Test Plan:
{F1068873}
{F1068874}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15103
Summary:
Ref T8980. This isn't 100% coverage but should be pretty much all of the common ones.
These feel a touch iffy to me at first glance so I didn't go crazy trying to hunt all of them down. I have some other plans for them so maybe they'll feel better by the end of it.
Test Plan: Hovered over author, reviewers, blocked tasks, projects, etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8980
Differential Revision: https://secure.phabricator.com/D14877
Summary: Ref T9132. This allows you to prefill EditEngine forms with stuff like `?subscribers=epriestley`, and we'll figure out what you mean.
Test Plan:
- Did `/?subscribers=...` with various values (good, bad, mis-capitalized).
- Did `/?projects=...` with various values (good, bad, mis-capitalized).
- Reviewed documentation.
- Reviewed {nav Config > HTTP Parameter Types}.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14404
Summary:
Ref T182. This allows you to assign blueprints that a repository can use to perform working copy operations. Eventually, this will support "merge this" in Differential, etc.
This is just UI for now, with no material effects.
Most of this diff is just taking logic that was in the existing "Blueprints" CustomField and putting it in more general places so Diffusion (which does not use CustomFields) can also access it.
Test Plan:
- Configured repository automation for a repository.
- Removed repository automation for a repository.
Reviewers: chad
Reviewed By: chad
Subscribers: avivey
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D14259
Summary: Ref T9494. Depends on D14216. Remove 10 copies of this code.
Test Plan: Ran `arc unit --everything`, browsed Config > Modules, clicked around Herald / etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9494
Differential Revision: https://secure.phabricator.com/D14217
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
