Summary: Return `$this` from setter methods for consistency. I started writing a [[https://secure.phabricator.com/differential/diff/32506/ | linter rule]] to detect this, but I don't think it is trivial to do this properly.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13422
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: Modernize Differential edges to subclass `PhabricatorEdgeType`. Largely based on D11045.
Test Plan: From previous experience, these changes are fairly trivial and safe. I poked around a little to make sure things looked reasonably okay.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, Krenair, epriestley
Differential Revision: https://secure.phabricator.com/D11074
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: 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: Changes to using FontAwesome
Test Plan:
Testing UIExamples and each of the pages (except releelph)
{F155942}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9157
Summary:
Ref T3551. Currently, there are many layers of indirection between pull requests and revisions. After D8822, revisions and other types of requested objects are recorded directly on the request. This allows us to simplify data access and querying.
A lot of stuff here is doing `instanceof` checks to keep APIs stable, but most of those can go away in the long run.
Test Plan:
- Browsed requests.
- Verified revision-dependent fields (like "Revision", "Size", "Churn") still render correctly.
- Called `releeph.queryrequests`.
- Called `releephwork.nextrequest`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3551
Differential Revision: https://secure.phabricator.com/D8824
Summary:
Ref T3551. Releeph does a bunch of old-school on-object data loading; start cleaning that up.
This doesn't change anything, just makes the code more modern/consistent.
Test Plan: Edited a request; called `releephwork.nextrequest`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3551
Differential Revision: https://secure.phabricator.com/D8819
Summary: Ref T3718. Move from unbatched / ad-hoc loading to standard stuff for handles.
Test Plan: Looked at some requests and saw no changes.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3718
Differential Revision: https://secure.phabricator.com/D8810
Summary:
Ref T3718. Ref T3644. Ref T3092. Switches from the Releeph UI elements to standard ones. I'll attach some screenshots.
Also fixes CSRF against the request action endpoint.
Test Plan:
- Viewed request details.
- Took actions on a request from detail page.
- Viewed request list.
- Took actions on a request from list page.
- Used keyboard shortcuts to navigate list.
- Used keyboard shortcuts to take actions.
- Simulated errors.
- Viewed on devices.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: grp, FacebookPOC, mattlqx, tala, beng, LegNeato, epriestley
Maniphest Tasks: T3718, T3092, T3644
Differential Revision: https://secure.phabricator.com/D8771
Summary:
Fixes T3738. Facebook uses this to provide a couple of integrations (push karma, is user an intern?), but the mechanism is both very complex and not very general.
Instead, these features are better implemented in Hovercards or via CustomField. We'll help Facebook integrate things when the time comes, but per discussion in T3738 none of this is critical or especially complicated.
Test Plan:
- Grepped for all callsites.
- Viewed a request and verified that author/requestor populated and rendered correctly.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T3738
Differential Revision: https://secure.phabricator.com/D8631
Summary:
Ref T2222.
- Removes `DifferentialTasksAttacher`, which has had no callsites for a very long time.
- Moves `differential.getrevisioncomments` off `DifferentialCommentQuery`.
- Moves Releeph churn field off `DifferentialCommentQuery`.
- Removes dead code in `DifferentialRevisionViewController`.
- Removes `DifferentialException` (no references).
- Removes `DifferentialRevision->loadComments()` (no callsites).
- Removes `DifferentialRevision->loadReviewedBy()` (all callsites updated).
- Removes `DifferentialCommentQuery` (all callsites updated).
Test Plan: Mostly a lot of `grep`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8476
Summary:
Ref T2222. This is the big one.
This migrates each `DifferentialComment` to one or more ApplicationTransactions (action, cc, reviewers, update, comment, inlines), and makes `DifferentialComment` a double-reader for ApplicationTransactions.
The migration is pretty straightforward:
- If a comment took an action not otherwise covered, it gets an "action" transaction. This is something like "epriestley abandoned this revision.".
- If a comment updated the diff, it gets an "updated diff" transaction. Very old transactions of this type may not have a diff ID (probably only at Facebook).
- If a comment added or removed reviewers, it gets a "changed reviewers" transaction.
- If a comment added CCs, it gets a "subscribers" transaction.
- If a comment added comment text, it gets a "comment" transaction.
- For each inline attached to a comment, we generate an "inline" transaction.
Most comments generate a small number of transactions, but a few generate a significant number.
At HEAD, the code is basically already doing this, so comments in the last day or two already obey these rules, roughly, and will all generate only one transaction (except inlines).
Because we've already preallocated PHIDs in the comment text table, we only need to write to the transaction table.
NOTE: This significantly degrades Differential, making inline comments pretty much useless (they each get their own transaction, and don't show line numbers or files). The data is all fine, but the UI is garbage now. This needs to be fixed before we can deploy this to users, but it's easily separable since it's all just display code.
Specifically, they look like this:
{F112270}
Test Plan:
I've migrated locally and put things through their paces, but it's hard to catch sketchy stuff locally because most of my test data is nonsense and bad migrations wouldn't necessarily look out of place.
IMPORTANT: I'm planning to push this to a branch and then shift production over to the branch, and run it for a day or two before bringing it to master.
I generally feel good about this change: it's not that big since we were able to separate a lot of pieces out of it, and it's pretty straightforward. That said, it's still one of the most scary/dangerous changes we've ever made.
Reviewers: btrahan
CC: chad, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8210
Summary: Add the 'Depends On' field to releeph requests. This will help the release engineers to be aware of the dependencies and make sure pick them altogether.
Test Plan: Check sandbox. This field shows up when a revision has some dependencies.
Reviewers: JoelB, lifeihuang, epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7946
Summary: Ref T603. Killing this class is cool because the classes that replace it are policy-aware. Tried to keep my wits about me as I did this and fixed a few random things along the way. (Ones I remember right now are pulling a query outside of a foreach loop in Releeph and fixing the text in UIExample to note that the ace of hearts if "a powerful" card and not the "most powerful" card (Q of spades gets that honor IMO))
Test Plan: tested the first few changes (execute, executeOne X handle, object) then got real mechanical / careful with the other changes.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran, FacebookPOC
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6941
Summary: Ref T3718. Releeph has a custom implementation of this exception; a more general version exists in CustomField. Use the more general one. Nothing catches the specific one.
Test Plan: `grep`
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3718
Differential Revision: https://secure.phabricator.com/D6782
Summary:
Ref T3721. Releeph currently attempts to implement a flexible, field-driven search for branches, but it's building all of its own infrastructure and it ends up heading down some weird paths. In particular, it loads **every** request and then makes calls into fields to filter them. It also tries to be very very general, which isn't really necessary (for example, I think it's reasonable for us to assume that we won't let you disable the "requestor" field).
ApplicationSearch and CustomField provide more scalable approaches to this problem; move search on top of them. The query still ends up doing some filtering in-process, but it's now far more limited in scope and can be denormalized later.
Test Plan: {F54304}
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T3721
Differential Revision: https://secure.phabricator.com/D6758
Summary:
Ref T3718. This moves custom field rendering on the edit screen to PhabricatorCustomField and makes all the APIs conformant.
We still run through edit with both old-school and new-school sets of fields, because the actual editing isn't on the new stuff yet. That will happen in a diff or two.
Test Plan: Edited a request; intentionally introduced errors and verified the form behaved as expected.
Reviewers: btrahan, testuser1122344
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3718
Differential Revision: https://secure.phabricator.com/D6756
Summary: Ref T3718. This is not used and does not seem particularly useful.
Test Plan: Grep.
Reviewers: btrahan
Reviewed By: btrahan
CC: LegNeato, aran
Maniphest Tasks: T3718
Differential Revision: https://secure.phabricator.com/D6755
Summary:
Fixes T3661. Ref T3718. This makes Releeph custom fields extend PhabricatorCustomField so we can start moving over other pieces of infrastructure (rendering, storage, etc) to run through the same pathways. It's roughly the minimum amount of work required to be able to move forward.
NOTE: This removes per-project custom field selectors. Fields are now configured for an entire install. My understanding is that Facebook does not use this feature, and modern field infrastructure has moved away from selectors.
Test Plan: Viewed and edited projects, branches, and requests in Releeph. Grepped for removed config. Grepped for `field_selector`.
Reviewers: btrahan
Reviewed By: btrahan
CC: LegNeato, aran
Maniphest Tasks: T3661, T3718
Differential Revision: https://secure.phabricator.com/D6750
Summary: Fixes a query in √D6260.
Test Plan: View a Releeph RQ and verify that the "churn" field renders and has the right numbers in it.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6702
Summary: Ref T603. This query isn't policy-aware yet, but prepare for it to be one day.
Test Plan: Looked at: home page; differential home; differential detail; diffusion browse. Made differential.query conduit call.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6337
Summary:
Ref T2222.
I'm thinking about how I want to approach the Asana sync, and I want to try to do T2222 first so that we can build it cleanly on top of ApplicationTransactions. I think we can at least walk down this road a little bit and if it turns out to be scary we can take another approach.
I was generally very happy with how the auth migration turned out (seemingly, it was almost completely clean), and want to pursue a similar strategy here. Basically:
- Wrap the new objects in the old objects for reads/writes.
- Migrate all the existing data to the new table.
- Everything hard is done; move things over a piece at a time at a leisurely pace in lots of smallish, relatively-easy-to-understand changes.
This deletes or abstracts all reads of the DifferentialComment table. In particular, these things are **deleted**:
- The script `undo_commits.php`, which I haven't pointed anyone at in a very long time.
- The `differential.getrevisionfeedback` Conduit method, which has been marked deprecated for a year or more.
- The `/stats/` interface in Differential, which should be rebuilt on Fact and has never been exposed in the UI. It does a ton of joins and such which are prohibitively difficult to migrate.
This leaves a small number of reading interfaces, which I replaced with a new `DifferentialCommentQuery`. Some future change will make this actually load transactions and wrap them with DifferentialComment interfaces.
Test Plan: Viewed a revision; made revision comments
Reviewers: btrahan
Reviewed By: btrahan
CC: edward, chad, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6260
Summary: This is just a bit of gardening in order to make the responsive-UI diff easier; I'll be putting `getColorFor($status)` type things in this class, following the pattern in `ManiphestTaskStatus`.
Test Plan: Poke around Releeph.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5893
Summary:
Migrate to `PhabricatorApplicationTransactions` (`ReleephRequestTransactions` applied by `ReleephRequestTransactionalEditor`, instead of `ReleephRequestEvents` created by `ReleephRequestEditor`) and migrate all the old events into transactions. Email is supported in the standard way (no more `ReleephRequestMail`) as well.
This also collapses the Releeph request create and edit controllers into one class, as well as breaking everyone's subject-based mail rules by standardising them (but which should be more easily filtered by looking at headers.)
Test Plan:
* Make requests, then pick them.
* Pick and revert the same request so that discovery happens way after `arc` has told Releeph about what's been happening.
* Try to pick something that fails to pick in a project with pick instructions (and see the instructions are in the email.)
* Load all of FB's Releeph data into my DB and run the `storage upgrade` script.
* Request a commit via the "action" in a Differential revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: epriestley, aran, Korvin, wez
Maniphest Tasks: T3092, T2720
Differential Revision: https://secure.phabricator.com/D5868
Summary: Use a single `PhabricatorMarkupEngine` to render any markup in Releeph's fields, rather than rendering everything from scratch every time.
Test Plan: Check out the "Services" tab in the dark console when rendering a page on a branch page with 500x RQs!
Reviewers: wez, epriestley
Reviewed By: epriestley
CC: epriestley, aran
Maniphest Tasks: T3098
Differential Revision: https://secure.phabricator.com/D5822
Summary: Migration doesn't delete differential.revisionPHID but maybe it should?
Test Plan: Reparsed commit, ran the migration, deleted differential.revisionPHID, looked at task with attached commit with attached revision.
Reviewers: epriestley, edward
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Differential Revision: https://secure.phabricator.com/D5634
Summary: Oops. Functions only return stuff when you call the `return()` function on the `lvalue`.
Test Plan: Use `ReleephDefaultFieldSelector` is the field selector in a test Releeph project.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5362
