Summary:
Ref T3671. A lot of applications have pretty ad-hoc preview code. Clean it up a bit and add Summary preview to Differential.
After ApplicationTransactions we might want to try to serialize the whole form and show a preview of all the transactions, but this seems not very useful in most cases (I'd guess that Remarkup previews are 99% of the value) and tricky to get right (e.g., adding images which don't exist yet to Pholio mocks).
I think I can add this in a few other places, too.
Test Plan:
Edited Maniphest Tasks and Differential Revisions, mashed some buttons. Verified previews rendered correctly. Grepped for removed CSS classes (no hits).
{F52907}
Reviewers: btrahan, Firehed
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T3671
Differential Revision: https://secure.phabricator.com/D6673
Summary: Ref T2852. Current code works fine, but although we want to drop creation stories, we really only want to drop the story text, not the other effects of the creation story. Also generalize this mechanism so we don't have Asana-specific code in the publishers.
Test Plan: Used `bin/feed republish` to publish creation and non-creation stories. Verified creation story published no text.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6639
Summary: Ref T2852. Currently there's effectively a double notification: one for creating the task, and one for the "alincoln created this revision" story. Drop the "create" story.
Test Plan: Used `bin/feed republish` to republish "create" and non-"create" stories. Verified "create" was dropped as unsupported, while non-"create" went through.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6589
Summary: Ref T2715. Had to start loading status information in the query class. Debated trying to clean up some of the attach / load stuff but decided to just add status under the new paradigm for now.
Test Plan: phid.query also made a status and checked that out. also played in conpherence.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2715
Differential Revision: https://secure.phabricator.com/D6585
Summary: Ref T2852. If the prefix is removed by configuration, we'll incorrectly leave a leading space. Trim any leading whitespace off.
Test Plan: Ran `bin/feed republish` to sync an object to Asana
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6575
Summary: Ref T2852. Pulls the Differential-specific aspects of the Asana sync out of the worker. Next diff will add a publisher for Audit/Diffusion.
Test Plan: Published events, including state changes. Saw them reflected correctly in Asana.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6569
Summary: I copied this over wrong. Ref T2715
Test Plan: Looked at Maniphest with linked commits/revisions, e.g.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2715
Differential Revision: https://secure.phabricator.com/D6531
Summary:
Fixes T3030. T1977 attempted to fix this but either didn't work (I think this is the case) or was broken later. We don't send `DifferentialCommentMail` on a create or update; we send `DifferentialReviewRequestMail`.
Also update the details to be more clear.
Test Plan:
Verified review request mail is marked undeliverable:
```
$ ./bin/mail show-outbound --id 6644
...
PARAMETERS
...
mailtags: ["differential-review-request"]
...
subject: D922: asdf
subject-prefix: [Differential]
vary-subject-prefix: [Request, 100 lines]
...
RECIPIENTS
! duck (duck)
- This mail has tags which control which users receive it, and this recipient has not elected to receive mail with any of the tags on this message (Settings > Email Preferences).
BODY
epriestley requested code review of "asdf".
...
```
Verified update mail is marked undeliverable:
```
$ ./bin/mail show-outbound --id 6646
...
Message: Message has no valid recipients: all To/Cc are disabled, invalid, or configured not to receive this mail.
PARAMETERS
...
mailtags: ["differential-updated"]
...
subject: D922: asdf
subject-prefix: [Differential]
vary-subject-prefix: [Updated, 100 lines]
...
RECIPIENTS
! duck (duck)
- This mail has tags which control which users receive it, and this recipient has not elected to receive mail with any of the tags on this message (Settings > Email Preferences).
BODY
epriestley updated the revision "asdf".
...
```
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3030
Differential Revision: https://secure.phabricator.com/D6518
Summary: Fixes T3564. This was a change out of FB recently, see D6340. Add a missing label.
Test Plan: "Request Review" now has a label.
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3564
Differential Revision: https://secure.phabricator.com/D6484
Summary:
Ref T2852. We need to distinguish between an API call which worked but got back nothing (404) and an API call which failed.
In particular, Asana hit a sync issue which was likely the result of treating a 500 (or some other error) as a 404.
Also clean up a couple small things.
Test Plan: Ran syncs against deleted tasks and saw successful syncs of non-tasks, and simulated random failures and saw them get handled correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6470
Summary:
Change all instances of `id(new DifferentialRevision())->load($id)` for `DifferentialRevisionQuery` where reviewers are loaded.
Also make sure that the new reviewer status is being loaded so that all calls to `getReviewers` can be removed in the near future.
Test Plan: Use all three controllers with several revisions and check they still work in sane way
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D6466
Summary:
Ref T1279.
Switched all differential conduit methods to use `DifferentialRevisionQuery` where `loadRelations` was being used.
Test Plan: Called all the methods changed and verified they still worked as advertised.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T603, T1279
Differential Revision: https://secure.phabricator.com/D6452
Summary: Add `getReviewerStatus` to get an array of `DifferentialReviewer` objects. The method `needReviewerStatus` in `DifferentialRevisionQuery` loads the edges into the revisions loaded.
Test Plan: Added `->needReviewerStatus(true)` to `DifferentialRevisionSearchEngine` and checked through logging that the data was being loaded correctly.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6450
Summary: rPad17c99c1b0222292a47ca79561a356cb8b5a5d5 stopped the fatal and this provides the forward fix. I think this is what a forward fix is anyway.
Test Plan: viewed a revision (D63 is my boy) and no fatals
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6448
Summary:
Fixes T2691. Now, all PhabricatorActionListViews in the codebase setObjectHref to $request->getRequestURI. This value is passed over to PhabricatorActionItems right before they are rendered. If a PhabricatorActionItem is a workflow and there is no user OR the user is logged out, we used this objectURI to construct a log in URI.
Potentially added some undesirable behavior to aggressively setUser (and later setObjectURI) from within the List on Actions... This should be okay-ish unless there was a vision of actions having different user objects associated with them. I think this is a safe assumption.
Test Plan: played around with a mock all logged out (Ref T2652) and it worked!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2691
Differential Revision: https://secure.phabricator.com/D6416
Summary: One place used status, other used state. Killed state in favor of status.
Test Plan: None at all
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6422
Summary:
Keep track of the state of a reviewer in an edge between reviewer and revision.
The edge stores the state of the review, added or rejected. And if the revision was
accepted by that reviewer the id of the diff accepted.
Test Plan:
Create diffs and clowncopterize reviewer list changes. This includes:
* Adding new reviewers
* Resigning
* Commandering a revision
Reviewers: epriestley
CC: aran, Korvin
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D6372
Conflicts:
src/applications/differential/editor/DifferentialCommentEditor.php
Summary:
1. Show add reviewer typehead when user selects resign as a reviewer.
2. Change the label for add reviewers typehead when user selects resign as a reviewer.
Test Plan:
1. Add yourself as a reviewer in a diff.
2. Select "Resign as Reviewer" in comment editor.
Add reviewer typehead should display, with label "Suggest Another Reviewer".
Add reviewer typehead is also displayed after user refreshed the page with "Resign as Reviewer"
selected.
Reviewers: wez, epriestley
Reviewed By: epriestley
CC: aran, epriestley, akramer, person
Differential Revision: https://secure.phabricator.com/D6340
Summary:
This leaves the space between the properties and the blurb looking a bit empty, but there will be more stuff there soon (status, VCS names, email, phone/fax numbers, etc., and custom user fields).
I removed "view lint messages" since I'm pretty sure no one has ever clicked it. I think providing better search (e.g, T2625) to that UI in Diffusion is a preferable approach.
Test Plan: {F49423}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6403
Summary: Ref T2852. When a Differential revision is linked to an Asana task, show the related task in Differential.
Test Plan: {F49234}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6387
Summary: Ref T603. Show object visibility in the UI. This isn't editable or mutable yet, but will be after T2222.
Test Plan: {F48689} {F48690}
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6361
Summary: Minor tweaks for consistency, and raise a friendlier error if the user doesn't upload anything.
Test Plan:
{F48686}
{F48685}
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6360
Summary: Fixes spacing in Differential revision detail and Diffusion browse views.
Test Plan:
{F48677}
{F48678}
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6359
Summary: Ref T3485. Moves flag icon inline in the header.
Test Plan: {F48654}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3485
Differential Revision: https://secure.phabricator.com/D6355
Summary:
Ref T603. Ref T2625. Fixes T3241. Depends on D5451. Depends on D6346.
@wez, this changes the Differential revision list UI substantially and may generate a lot of bikeshedding / who-moved-my-cheese churn. See T3417 for context, for example. The motivations for this change are:
- The list now works on devices, like phones and tablets. This is a requirement to make the rest of Differential work on devices.
- Although ApplicationSearch intentionally presents a simpler interface initially and some options which were one click away before aren't now, it is much more powerful than the search it replaces and allows users to build, save, share, fork, edit, and customize a much wider range of queries. Users who used the old filters frequently can use Advanced Search -> Save Custom Query to create new versions of them, and of any other query. "Edit Queries.." allows users to remove and reorder queries, including builtin queries. Basically, there are like three things which have gone from "1-click" to "a few clicks", and ten trillion things which have gone from "hard/impossible" to "relatively easy".
The local screenshots look a bit iffy, but I think a lot of this is the fakenesss of my test data. If they still feel iffy in production we can tweak them until they feel good, like we did for Maniphest.
Test Plan:
{F48477}
{F48478}
Reviewers: btrahan, chad, wez
Reviewed By: btrahan
CC: aran, s
Maniphest Tasks: T603, T2625, T3241
Differential Revision: https://secure.phabricator.com/D6347
Summary:
Tried out `PhabricatorObjectItemView` for Differential. It looks smexy and smooth.
Refs T2014
- Title and Date as Maniphest
- Author in the handle icon
- Bar color reflects revision status (Needs Review, Accepted, Abandoned etc.) @chad looking for non-blue is faster than keeping watch for everything that's not "Closed" in old table form
- Some status information are in footer icons; currently only stale/old status display as well as saved drafts, maybe more in future; these come into my mind:
- No reviewer warning
- Push Blocking Priority (T2730)
- Trivial, fast review guaranteed
- Sketch / Just looking for advice/help
- Arcanist Project (T2614)
- Denote "Public Send-in" (T1476)
{F37662}
{F37663}
{F37664}
{F37665}
Some flaws:
- Date and reviewers on every entry the same?
- No respect for Differential fields (for some reason, every entry appeared the same, so broke it to parts)
- Plenty of (potential) increase in height - advise reducing paging length from 100 to 50 - or just ignore me
Suggestions for the future:
- Expand the meta information regarding revisions; e.g. the various status displays above
- Uh... T2543, T1279, T793, T731 and what else I want for Differential, because they are awesome!
- T793 should be in particular easy appearance-wise, just copy-paste from Maniphest
Test Plan: By looking at it, of course. Verified there are no errors or crashed
Reviewers: epriestley, chad, btrahan, liguobig
Reviewed By: chad
CC: aran, Korvin, edward, nh
Maniphest Tasks: T2014
Differential Revision: https://secure.phabricator.com/D5451
Conflicts:
src/__celerity_resource_map__.php
Summary:
Ref T603. Ref T2625. Use cursors to page Differential queries, not offsets.
The trick here is that some queries are ordered. In these cases, we either need to pass some kind of tuple or do a cursor lookup. For example, if you are viewing revisions ordered by `dateModified`, we can either have the next page be something like:
?afterDateModified=2398329373&afterID=292&order=modified
...or some magical token:
?afterToken=2398329373:292&order=modified
I think we did this in Conpherence, but one factor there was that paging orders update with some frequency. In most cases, I think it's reasonable to pass just the ID and do a lookup to get the actual clause value (e.g., go look up object ID 292 and see what its dateModified is) and I think this is much simpler in general.
Test Plan: Set page size in Differential to 3, and paged through result lists ordered by date created and date modified.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603, T2625
Differential Revision: https://secure.phabricator.com/D6345
Summary:
Ref T603. Ref T2625. Makes `DifferentialRevisionQuery` do policy checks.
Note that it still uses inefficient offset-based paging, but it's rare to page through revisions. I'll switch to cursor paging in a future diff.
Test Plan: Viewed a bunch of Differential interfaces, home page, etc. This shouldn't actually materially impact anything.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603, T2625
Differential Revision: https://secure.phabricator.com/D6344
Summary:
Currently, when querying for responsible users (revisions where a given user is either the author or a reviewer), we do this:
- If the query passes a bunch of hard-coded special cases, use a special hard-coded UNION.
- Otherwise, use a very complicated JOIN/WHERE clause.
This is bad for several reasons:
- Tons and tons of hard-coding and special casing.
- The JOIN/WHERE clause performs very poorly for large datasets.
- (As a material consequence, the homepage issues a responsible query which barely misses the hard-coded special cases and goes down the slow path.)
Instead, //always// use the UNION strategy to execute a "responsible" query. Specifically, if we have responsible PHIDs, temporarily add them to the author list and build a normal query, then repeat for reviewers, then UNION any clauses we built.
Fixes T3377. Ref T603. Ref T2625. Depends on D6342.
There's various folklore about UNION ALL / UNION DISTINCT performance. UNION DISTINCT is simpler here and the number of rows is small, although we could use UNION ALL in the form:
SELECT * FROM ((SELECT ...) UNION ALL (SELECT ...) ORDER) GROUP LIMIT
...if we find that there's some performance benefit at some point.
Test Plan: Used DarkConsole to examine queries. Viewed home page and Differential dashboard/authors/subscribers.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603, T2625, T3377
Differential Revision: https://secure.phabricator.com/D6343
Summary:
Ref T603. This introduces a policy-aware DifferentialDiffQuery and converts most callsites.
I've left unusual callsites (mostly: hard to get the viewer, unusual query, queries related to active diffs) alone for now, so this isn't exhaustive but hits 60-80% of sites.
Test Plan: Created diff; created revision; viewed diffs and revisions; made additional conduit calls.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6338
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 T603. This is a very old, very bad version of `DifferentialRevisionQuery`. I want to modernize only the latter. Express the remaining callsite of the former in terms of `DifferentialRevisionQuery`.
Test Plan: Executed all four modes of `differential.find`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6335
Summary:
Ref T603.
- Primarily, this gets rid of a `DifferentialRevisionListData` callsite.
- Also modernize and clean up some UI stuff.
Test Plan:
{F48260}
{F48261}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6334
Summary: See discussion in IRC. Not 100% sure what's going on here because of email ghost theives, but conceivably a commit with no changes will end up with `null` changesets instead of `array()` changesets, which throws. Such diffs are certianly possible (`git commit --allow-empty`) even if they aren't the issue in this specific case. See T3416. Initialize changesets to `array()` to avoid throwing.
Test Plan:
Viewed some commits?
iiam
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6339
Summary: Ref T2222. Currently, we load inline comments by `commentID` here, but we always pass every commentID associated with the revision. Instead, just load non-draft comments by revision ID. This simplifies querying a little bit and is likely faster anyway (draft comments are currently loaded separately).
Test Plan: Looked at some revisions and verified inlines showed up correctly and in the right places.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6270
Summary:
Ref T2222. My path forward here wasn't very good -- I was thinking I could set `transactionPHID` for the inline comments as I migrated, but it must be unique and an individual DifferentialComment may have more than one inline comment. Dropping the unique requirement just creates more issues for us, not fewer.
So the migration in D6266 isn't actually useful. Undo it -- this can't be a straight revert because some installs may already have upgraded.
Test Plan: Ran new migrations, verified the world ended up back in the same place as before (made comments, viewed reivsions).
Reviewers: btrahan
Reviewed By: btrahan
CC: wez, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6269
Summary: Ref T2222. I didn't translate this query properly; reproduce the original.
Test Plan: When viewing a revision with non-draft inline comments by a user other than the viewer, the inline comments now appear on the changesets themselves.
Reviewers: kawakami, btrahan, garoevans
Reviewed By: garoevans
CC: aran, mbishopim3
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6281
Summary: I assume this is a bug!
Test Plan: Look at it
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6279
Summary:
Ref T2222. This adds PHIDs to all Differential comments so I can migrate the inlinecommment table to transaction_comment in the next diff.
@wez, this will issue a few million queries for Facebook (roughly, one for each Differential comment ever made). It's safe to skip the `.php` half of the patch, bring Phabricator up normally, and then apply this patch with Phabricator running if that eases the migration, although the next few diffs will probably be downtime-required migrations so maybe it's easier to just schedule some downtime.
Test Plan: Ran migration locally. Verified existing comments and new comments received PHIDs.
Reviewers: btrahan
Reviewed By: btrahan
CC: wez, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6266
Summary:
Ref T2222. See D6260.
Push all this junk behind a Query so I can move the storage out from underneath it.
Test Plan: Viewed home page, list view, revision. Made draft, looked at preview, submitted draft, viewed inline, replied to inline.
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6262
Summary:
Ref T2222. Ref T1460. Depends on D6260.
This creates the new tables, but doesn't start using them. I added three new fields for {T1460}, to represent fixed/done/replied states.
Test Plan: Ran storage upgrade.
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T1460, T2222
Differential Revision: https://secure.phabricator.com/D6261
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
