Summary: Ref T2222. Adds a mostly-functional "Pro" comment controller. This does the core stuff, but does not yet do actions (accept, reject, etc.) or inline comments.
Test Plan: Changed the `if (false)` to an `if (true)`, then made some comments, etc. This is normally unreachable.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8304
Summary: Ref T2222. Currently this is a giant header box thing. Move it into the ObjectBox.
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8301
Summary:
Ref T2222. Differential has custom code for managing subscriptions, but no longer requires it.
The one trick is that we don't have a hook for loading related data on the subscriptions workflow right now. Just glue that in for the moment; it's relatively harmless, and once Diffusion converts we'll have more context on how to best surface it properly.
Test Plan: Subscribed and unsubscribed from a revision. Viewed different revisions and saw correct subscription state.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8293
Summary: Ref T2222. Ref T1790. I partially modernized this recently, but bring it to the mail version too.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: zeeg, aran
Maniphest Tasks: T1790, T2222
Differential Revision: https://secure.phabricator.com/D8294
Summary:
Ref T3496. Currently, we call loadAssets() on each revision table, which invokes a new revision query and a pile of subqueries.
Instead, add `needFlags()` and `needDrafts()` to `RevisionQuery`. Some day these could perhaps be more generic.
Test Plan:
- Viewed home, differential, etc., no longer saw 9203809238 queries being run for no reason.
- Drafts and flags still appear properly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3496
Differential Revision: https://secure.phabricator.com/D8277
Summary:
Ref T3886. I spent a few hours trying to make `DifferentialFieldSpecification` extend `PhabricatorCustomField` so I could be more blunt in my approach here and just swap the whole thing over in one go (more or less like I did with Maniphest) but we have a ton of custom fields and things felt really shaky and the change was enormous and hard to keep track of.
Instead, I'm going to do this more gradually and go field-by-field. This implements a CustomField version of the "Title" field.
(There are no links to this in the UI.)
Test Plan:
{F115353}
{F115354}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886
Differential Revision: https://secure.phabricator.com/D8276
Summary: ...do it somewhat generically, so we could fairly easily add this to other applications. Fixes T3496. I got a wee bit lazy and decided not to migrate existing drafts. My excuses aside from laziness are doing it this way will let us see if anyone complains, we can always do a migration later if people do complain, and there's likely to be a lot of garbage data for older / bigger installs, and the migration didn't seem worth itgiven it would also likely be expensive in these cases.
Test Plan: made a draft inline comment on DX and observed DX had a note icon on Differential home page. made a draft comment on DX and observed DX had a note icon on Differential home page. deleted a draft inline comment and noted icon disappeared from Differential homepage. Submitted a draft comment + inline comment and noted icon disappeared.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3496
Differential Revision: https://secure.phabricator.com/D8275
Summary:
Ref T2222. I want to stage a "later" patch to drop this column, but get rid of the last few references to it.
One of these methods has no callers, and the other stuff I've updated to use the modern fields.
Test Plan: Created some inlines, hit "edit", submitted them, `grep.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8240
Summary: Ref T2222. Use new code for rendering. Delete `DifferentialRevisionCommentView`, which has no remaining callsites.
Test Plan: Went through all the different actions and verified the previews rendered correctly. Reloaded page to test draft behavior.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8236
Summary: Ref T2222. Restore this funky is-visible / inline-is-elsewhere logic.
Test Plan: Updated a revision, saw all the inlines render properly when looking at various diffs and versus-diffs. Clicked inline links.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8224
Summary: Ref T2222. These don't work yet. We just have to copy a couple fields, but let's sort that out later since this is purely a new feature.
Test Plan: Looked at a revision, no edit links.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8222
Summary:
Ref T2222. This gets rid of Differential's custom view and uses a standard view instead.
This also mostly fixes the rendering logic for inlines.
This is headed to the `tmp.differential` branch.
Test Plan: {F112696}
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T1790, T2222
Differential Revision: https://secure.phabricator.com/D8215
Summary: Ref T2222. A few rendering interfaces rely on fishing the revision ID out of a DifferentialComment, but it will only have the PHID soon. Pass in the revision and use it to determine the ID instead.
Test Plan: Browsed, previewed, examined comments. Clicked anchors.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8209
Summary:
Fixes T4066.
add `isActionDisabled()` to DifferentialLandingStrategy, which also explains why it is so.
Make an appropriate pop-up in the controller.
Also make the whole UI "workflow", and convert `createMenuItems()` to `createMenuItem()` (Singular).
Test Plan: Click "Land to..." button in all kinds of revisions.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4066
Differential Revision: https://secure.phabricator.com/D8105
Summary: Cleans up some older layouts to new stuffs.
Test Plan: Test with and without a diff ID.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7949
Summary: This removes the bulk of the "Form Errors" text, some variations likely exists. These are a bit redundant and space consuming. I'd also like to back ErrorView more into PHUIObjectBox.
Test Plan: Test out the forms, see errors without the text.
Reviewers: epriestley, btrahan
CC: Korvin, epriestley, aran, hach-que
Differential Revision: https://secure.phabricator.com/D7924
Summary: Two basic changes here, first we fixed up the Diffusion headers to roll out more PHUIObjectBoxes. Second we added some specific styles for when Errors are inside an ObjectBox at the first position.
Test Plan: Tested a number of different layouts for browsing respositories as well as wherever I could find cases with PHUIObjectBox Form Errors (see images attached). Still some minor tightening due after this diff, but didnt want to overload it.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7914
Summary:
Ref T4222. Currently, CelerityResourceResponse holds response resources in flat maps. Instead, specify which map resources appear in.
Also, provide `requireResource()` and `initBehavior()` APIs on the Controller and View base classes. These provide a cleaner abstraction over `require_celerity_resource()` and `Javelin::initBehavior()`, but are otherwise the same. Move a few callsites over.
Test Plan:
- Reloaded pages.
- Browsed around Differential.
Reviewers: btrahan, hach-que
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7876
Summary:
Fixes T4270. When you download raw file content, diffs, and patches we currently give them default (all users) visibility.
Instead, bind them to the repository or revision in question.
(This code could use a bit of cleanup at some point.)
Test Plan: Hit the patch and content download links in Diffusion and the patch download link in Differential, got restricted files with accurate policy bindings.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4270
Differential Revision: https://secure.phabricator.com/D7849
Summary: We currently have a lot of calls to `addCrumb(id(new PhabricatorCrumbView())->...)` which can be expressed much more simply with a convenience method. Nearly all crumbs are only textual.
Test Plan:
- This was mostly automated, then I cleaned up a few unusual sites manually.
- Bunch of grep / randomly clicking around.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: hach-que, aran
Differential Revision: https://secure.phabricator.com/D7787
Summary:
Ref T1715. When the user clicks "Download Raw Diff" in Differential, we try to build a diff of exactly what they're seeing. However:
- This doesn't work if any of the changes have multiple hunks, and fixing it seems hard.
- I suspect this diff is never actually useful anyway? And probably kind of confusing in the best case. You can't really apply it to anyhting, since you'd have to apply another diff first.
Instead, just build the right-side diff, which should align well with user expectation and doesn't suffer from the multi-hunk bug.
Some day, we could maybe add some of the fancy options in T1715.
See: <https://github.com/facebook/phabricator/issues/461>
Test Plan: Downloaded a multi-hunk diff, got the original back and applied it cleanly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1715
Differential Revision: https://secure.phabricator.com/D7694
Summary:
A usable, Land to GitHub flow.
Still to do:
- Refactor all git/hg stratagies to a sane structure.
- Make the dialogs Workflow + explain why it's disabled.
- Show button and request Link Account if GH is enabled, but user is not linked.
- After refreshing token, user ends up in the settings stage.
Hacked something in LandController to be able to show an arbitrary dialog from a strategy.
It's not very nice, but I want to make some more refactoring to the controller/strategy/ies anyway.
Also made PhabricatorRepository::getRemoteURIObject() public, because it was very useful in getting
the domain and path for the repo.
Test Plan:
Went through these flows:
- load revision in hosted, github-backed, non-github backed repos to see button as needed.
- hit land with weak token - sent to refresh it with the extra scope.
- Land to repo I'm not allowed - got proper error message.
- Successfully landed; Failed to apply patch.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D7555
Summary:
Fixes T3034. This is obsoleted by modern policies.
This was written by a Facebook intern and is rarely used -- the Hive install might be the only use in the wild. It has never really worked correctly.
Test Plan: `grep`; browsed Differential.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3034
Differential Revision: https://secure.phabricator.com/D7568
Summary: I've kept this as close as possible to the Git version for ease of review and later refactoring of them both together. At minimum, the functions to get the working dir should probably be cleaned up one day.
Test Plan: Landed a revision.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7534
Summary:
ref T182.
Simple approach of clone, patch, push. While waiting for drydock, implement a hackish mutex
setup for the workspace, which should work ok as long as there's only one committer who is
carefull about theses things.
Less obvious note: This is taking the both author and commiter's 'primary email' for the commit -
which might rub some people wrong.
Test Plan:
With a hosted repo, created some diffs and landed them.
Also clicked button for some error cases, got the right error message.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: hach-que, Korvin, epriestley, aran
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D7486
Summary:
Ref T2222. Shrink the API to make it easier to move this object's storage to ApplicationTransactions.
Fixes T3415. This moves the "Summary" and "Test Plan" into the property list, and thereby fixes all the attribution problems associated with commandeering, creating a revision from another user's diff, etc.
Test Plan: Browsed several revisions.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3415, T2222
Differential Revision: https://secure.phabricator.com/D7375
Summary: Fixes T3898. This feature needs generalization at some point, but just unbreak it for now since a surprising number of users like it.
Test Plan: Pressed "z".
Reviewers: chad, btrahan
Reviewed By: chad
CC: chad, aran, spicyj
Maniphest Tasks: T3898
Differential Revision: https://secure.phabricator.com/D7366
Summary:
Ref T2222. This implements step (1) described there, which is moving over all the inline comments.
The old and new tables are simliar. The only real trick here is that `transactionPHID` and `legacyCommentID` mean roughly the same thing (`null` if the inline is a draft, non-null if it has been submitted) but we don't have real `transactionPHID`s yet. We just make some up -- we'll backfill them later.
Two risks here:
- I need to take a second look at the keys on this table. I think we need to tweak them a bit, and it will be less disruptive to do that before this migration than after.
- This will take a while for Facebook, and other large installs with tens of thousands of revisions. I'll communicate this.
I'm otherwise pretty satisfied with this, seems to work well and is pretty low risk / non-disruptive.
Test Plan:
- Before migrating, then after migrating:
- Made a bunch of inlines (drafts, submitted).
- Edited and deleted inlines.
- Verified inlines showed up in preview.
- Verified that inlines aren't indexed when they're drafts (`bin/search index D935`).
- Verified that inlines ARE indexed when they're not drafts.
- Verified that drafts inlines make revisions appear as "with draft" in the revision list.
- Made left, right, and draft inlines.
- Migrated (`bin/storage upgrade`).
- Verified that my inlines from before the migration still showed up.
- (Repeated all the stuff above.)
- Manually inspected the inline comment table.
Reviewers: btrahan
Reviewed By: btrahan
CC: FacebookPOC, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D7139
Summary: This builds out and implements PHUIPropertyListView (container) and PHUIPropertyListItemView (section) as well as adding tabs.
Test Plan: Tested each page I edited with the exception of Releeph and Phortune, though those changes look ok to me diff wise. Updated examples page with tabs.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7283
Summary:
Ref T603. Allows the Differential view policy to be configured with a default.
I've omitted "edit" because I want to wait and see how comment/comment-action policies work out. I could imagine locking "edit" down to only the owner at some point, and providing a wider "interact" capability, or something like that, which would cover accept/reject/commandeer. Users in this group could still edit indirectly by commandeering first.
Test Plan: Created new revisions from the CLI and conduit.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7269
Summary:
Ref T1279. With the new per-reviewer status, you can always accept or reject a revision.
This is primarily cosmetic/UI changes. In particular, you've always been able to reject a rejected revision, the UI just didn't show you an option.
Test Plan: Accepted accepted revisions; rejected rejected revisions. See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7243
Summary:
Ref T1279. We currently determine reviewers at display time, but this is bad for several reasons:
- It puts queries very close to the display layer.
- We have to query for each revision if we want to figure out authority for several.
- We need to figure it out in several places, so we'll end up with copies of this logic.
- The logic isn't trivial (exceptions for the viewer, exceptions to that rule for install configuration).
- We already do this "figure it out when we need it" stuff in Diffusion for audits and it's really bad: we have half-working copies of the logic spread all over the place.
Instead, put it in the Query. Callers query for it and get the data attached to the reviewer objects.
Test Plan:
- Looked at some revisions, verified the correct lines were highlighted.
- Looked at a revision I created and verified that projects I was a member of were not highlighted.
- With self-accept enabled, these //are// highlighted.
- Looked at a revision I did not create and verified that projects I was a member of were highlighted.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7241
Summary: Ref T603. Clean these up and move them to a single place.
Test Plan:
- Downloaded a raw diff.
- Enabled "attach diffs", created a revision, got an email with a diff.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7179
Summary: Ref T603. Swaps out most `PhabricatorFile` loads for `PhabricatorFileQuery`.
Test Plan:
- Viewed Differential changesets.
- Used `file.info`.
- Used `file.download`.
- Viewed a file.
- Deleted a file.
- Used `/Fnnnn` to access a file.
- Uploaded an image, verified a thumbnail generated.
- Created and edited a macro.
- Added a meme.
- Did old-school attach-a-file-to-a-task.
- Viewed a paste.
- Viewed a mock.
- Embedded a mock.
- Profiled a page.
- Parsed a commit with image files linked to a revision with image files.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7178
Summary:
Three changes here.
- Add `setActionList()`, and use that to set the action list.
- Add `setPropertyList()`, and use that to set the property list.
These will let us add some apropriate CSS so we can fix the border issue, and get rid of a bunch of goofy `.x + .y` selectors.
- Replace `addContent()` with `appendChild()`.
This is just a consistency thing; `AphrontView` already provides `appendChild()`, and `addContent()` did the same thing.
Test Plan:
- Viewed "All Config".
- Viewed a countdown.
- Viewed a revision (add comment, change list, table of contents, comment, local commits, open revisions affecting these files, update history).
- Viewed Diffusion (browse, change, history, repository, lint).
- Viewed Drydock (resource, lease).
- Viewed Files.
- Viewed Herald.
- Viewed Legalpad.
- Viewed macro (edit, edit audio, view).
- Viewed Maniphest.
- Viewed Applications.
- Viewed Paste.
- Viewed People.
- Viewed Phulux.
- Viewed Pholio.
- Viewed Phame (blog, post).
- Viewed Phortune (account, product).
- Viewed Ponder (questions, answers, comments).
- Viewed Releeph.
- Viewed Projects.
- Viewed Slowvote.
NOTE: Images in Files aren't on a black background anymore -- I assume that's on purpose?
NOTE: Some jankiness in Phortune, I'll clean that up when I get back to it. Not related to this diff.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7174
Summary: Missed this case in my sandbox
Test Plan: Reload a test diff
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7168
Summary:
Ref T603. Make Differential behaviors for logged-out and underprivleged users more similar to other apps.
I'm going to drop this "anonymous access" thing at some point, but `reviews.fb.net` actually looks like it's running semi-modern code, so leave it alive until we have a more compelling replacement in the upstream.
Test Plan: As a logged out user, browsed Differential and clicked things and such.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7148
Summary: Ref T603. Makes the majority of reads policy aware (and pretty much all the important ones).
Test Plan:
- Created a comment with `differential.createcomment`.
- Created a new revision with `arc diff` in order to exercise `differential.creatediff`.
- Created an inline comment with `differential.createinline`.
- Added a comment to a revision.
- Edited an inline comment.
- Edited a revision.
- Wrote "Depends on ..." in a summary, saved, verified link was created.
- Browsed a file in Diffusion.
- Got past the code I changed in the Releeph request thing.
- Edited a Releeph request.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7136
Summary: Ref T603. Moves policy information from a custom field to the header for revisions.
Test Plan: Looked at a revision.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7135
Summary: I'd like to reuse this for other content areas, renaming for now. This might be weird to keep setForm, but I can fix that later if we need.
Test Plan: reload a few forms in maniphest, projects, differential
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7120
Summary:
- D6966 accidentally reversed the order of `$diffs`. Reverse it back.
- The new policy header stuff returns `array(icon, text)` but gets `strlen()`'d by a caller. Silence that warning for now.
Test Plan: Created a revision with several diffs. Saw them in the right order; saw no warning on the diff attach screen.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran, mbishopim3
Differential Revision: https://secure.phabricator.com/D7023
Summary: I kind of made a mess of the API doing T2784. I figure just adding this is fine but LMK if you'd prefer something like diffquery got cleaned up more to handle this. Also adds an idx() call as I was getting errors looking at old diffs. Fixes T3823.
Test Plan: used the new api via test console - great success.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3823
Differential Revision: https://secure.phabricator.com/D6966
Summary: The adds the ability to set 'properties' such as state, privacy, due date to the header of objects.
Test Plan: Implemented in Paste, Pholio. Tested various states.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7016
Summary:
Currently, these events don't fire for Conduit updates, which makes them sort of silly.
This will get proper treatment after T2222.
Test Plan: Installed a `throw new Exception(...)` event listener. Performed Conduit and web updates of revisions, saw event listener fire.
Reviewers: btrahan, guywarner
Reviewed By: guywarner
CC: aran
Differential Revision: https://secure.phabricator.com/D7004
Summary: Adds plain support for object lists that just look like lists
Test Plan: review UIexamples and a number of other applications
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6922
Summary: Some more callsites, let me know if you see others, I think think is 98% of them now.
Test Plan: tested each page
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6814
Summary:
This attempts some consistency in form layouts. Notably, they all now contain headers and are 16px off the sides and tops of pages. Also updated dialogs to the same look and feel. I think I got 98% of forms with this pass, but it's likely I missed some buried somewhere.
TODO: will take another pass as consolidating these colors and new gradients in another diff.
Test Plan: Played in my sandbox all week. Please play with it too and let me know how they feel.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6806
Summary:
^\s+(['"])dust\1\s*=>\s*true,?\s*$\n
Test Plan: Looked through the diff.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6769
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:
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: 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: 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 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:
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. 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.
- 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: 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. 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:
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.
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: Used more logical icons for subscribe, auto, and delete instead of the mail icons. Fixes T3329
Test Plan: Tested subscribing and unsubscribing in Maniphest.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3329
Differential Revision: https://secure.phabricator.com/D6151
Summary:
This diff covers a bit of ground.
- PHUIDocumentExample has been added
- PHUIDocument has been extended with new features
- PhabricatorMenuView is now PHUIListView
- PhabricatorMenuItemView is now PHUIItemListView
Overall - I think I've gotten all the edges covered here. There is some derpi-ness that we can talk about, comments in the code. Responsive design is missing from the new features on PHUIDocument, will follow up later.
Test Plan: Tested mobile and desktop menus, old phriction layout, new document views, new lists, and object lists.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6130
Summary: Added constants to PhabricatorEventType. Modified DifferentialRevisionEditor and DifferentialCommentEditor.
Test Plan:
Created a revision. Edited and made a comment on that revision. It's updating as usual. I think nothing broke may be it's working.
Let me know if I have done it correclty.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Maniphest Tasks: T2899
Differential Revision: https://secure.phabricator.com/D5869
Summary:
Fixes T3218.
- Currently, Paste pages don't clear notifications about the paste (notably, token notifications).
- Currently, Paste pages don't show tooltips on tokens.
- `buildApplicationPage()` stopped respecting `pageObjects` (which controls whether "this page has been updated" is shown). Restore that.
- Make `pageObjects` imply "clear notifications on this stuff".
Test Plan: Viewed a tokened Paste. Verified it cleared the notification and hovering over a token showed a tip.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3218
Differential Revision: https://secure.phabricator.com/D5971
Summary:
This creates a common form look and feel across the site. I spent a bit of time working out a number of kinks in our various renderings. Some things:
- Font Styles are correctly applied for form elements now.
- Everything lines up!
- Selects are larger, easier to read, interact.
- Inputs have been squared.
- Consistant CSS applied glow (try it!)
- Improved Mobile Responsiveness
- CSS applied to all form elements, not just Aphront
- Many other minor tweaks.
I tried to hit as many high profile forms as possible in an effort to increase consistency. Stopped for now and will follow up after this lands. I know Evan is not a super fan of the glow, but after working with it for a week, it's way cleaner and responsive than the OS controls. Give it a try.
Test Plan: Tested many applications, forms, mobile and tablet.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5860
Summary:
We now have the app menu top-right, so we don't need the side navbar anymore
Refs T2014
Test Plan: Visited list view in narrow / wide browser. Could access everything either directly (tables :P) or the top right menus
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2014
Differential Revision: https://secure.phabricator.com/D5728
Summary:
Messy POC. I hope I did this one right :P
Refs T2014
Test Plan: Switched browser forcefully into `device-phone`, saw filters in the menu top right. Browsed around Differential in desktop mode, nothing was broken, I think.
Reviewers: epriestley, chad, btrahan
CC: aran, Korvin
Maniphest Tasks: T2014
Differential Revision: https://secure.phabricator.com/D5693
Summary:
Also join concepts of installed and enabled applications.
Also respect uninstalled Maniphest where disabled Maniphest was checked.
Test Plan:
Visited T1, D1.
Uninstalled Maniphest then visited T1, D1.
Disabled Maniphest then visited T1.
Visited /config/edit/maniphest.enabled/.
Reviewers: epriestley, Afaque_Hussain, edward
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5602
Summary:
Fixes a weird string in the flag dialog
Migrated to ObjectItemView (Cards)
Removed filters from side nav (unnecessary)
Go away, go away, panel. Nobody will miss you.
Adding sorting/ordering to Flag (separation like in Maniphest ordering still remaining)
Hacking our way in DifferentialRevisionListController thanks to panels (who added `->setFlush()` btw? It's awesome!)
Test Plan: I would not dare to stand here if it did not work.
Reviewers: epriestley, chad, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5449
Summary: Not for full review. This makes crumbs appear consistently in mobile. It helps give a quick link to the apps home, the page title currently on, and action icons for the object. It will take additional clean-up to make this consistent across apps. Passing for early review from a UEX perspective. I actually really like it and think onces it's everywhere, helps mobile feel complete.
Test Plan: Testing in iOS and Simulator.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2796
Differential Revision: https://secure.phabricator.com/D5446
Summary:
A few things
- pht Maniphest where I could
- implement dust background
- optimize pages for mobile
- adds aphront-two-column-layout
- reworks maniphest page with two column layout
- tweaks task table for mobile, though we should move to object-list-view
Stopping here as I want to get feedback in. Super excited about mobile and the new tasks views. Only sort of excited about the sidebar filters, they need more UI work, but we should talk about that.
Test Plan: Test Maniphest, Differential, and Homepage views. Sort tasks, make reports
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5314
Summary: These actions are dumb, and not smart, and no one likes them.
Test Plan: Looked at a revision and saw fewer actions; higher average action quality.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D5304
Summary: I know that this code would be replaced by something else but until then...
Test Plan: Used it in our renderer.
Reviewers: epriestley, edward
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5165
Summary:
Unmuck almost all of the we-sort-of-have-viewers-some-of-the-time mess.
There are a few notable cases here:
- I used Omnipotent users when indexing objects for search. I think this is correct; we do policy filtering when showing results.
- I cheated in a bad way in the Remarkup object rule, but fixing this requires fixing all the PhabricatorRemarkupEngine callsites (there are 85). I'll do that in the next diff.
- I cheated in a few random places, like when sending mail about package edits. These aren't a big deal.
Test Plan:
- Grepped for all PhabricatorObjectHandleData references.
- Gave them viewers.
Reviewers: vrana
Reviewed By: vrana
CC: aran, edward
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D5151
Summary: Mostly applies a new call spacing rule; also a few things that have slipped through via pull requests and such
Test Plan: `find src/ -type f -name '*.php' | xargs -n16 arc lint --output summary --apply-patches`
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D5002
Summary: Is this correct for Mercurial?
Test Plan: Saw it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4957
Summary: Sgrepped for `"=~/</"` and manually changed every HTML.
Test Plan: This doesn't work yet but it is hopefully one of the last diffs before Phabricator will be undoubtedly HTML safe.
Reviewers: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4927
Summary:
This is pretty brutal and it adds some `phutil_safe_html()`.
But it is a big step in the right direction.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4905
Summary:
Done by searching for `AphrontDialogView` and then `appendChild()`.
Also added some `pht()`.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4882
Summary: Searched for `AphrontFormView` and then for `appendChild()`.
Test Plan: /login/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4855
Summary: Done by searching for `AphrontErrorView` and then `appendChild()`.
Test Plan:
Looked at Commit Detail.
Looked at Revision Detail.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4843
Summary:
See D4812.
- This preference disables the file tree completely.
- It defaults off, so users who want it will have to go turn it on.
- Maybe slightly cleaner would be doing this if the tree was hidden and then ajaxing it in if you press "F", but that's complicated and I don't want to bother.
- Generally, I think this element is useful to something like 5% of users and not useful to 95%.
Test Plan: Enabled and disabled file tree. Looked at commits and revisions; verified they reflected the setting correctly.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4813
Summary: This is the most requested feature in FB by far.
Test Plan:
Toggled, verified that data are saved.
Reloaded, toggled, toggled, toggled, verified that data are saved.
Reloaded.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4812
Summary: Regression to original behavior.
Test Plan: Clicked on it twice, didn't see confirmation dialog.
Reviewers: epriestley, codeblock
Reviewed By: codeblock
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4788
Summary: It's a little bit confusing that you couldn't use %d but kind of expected taken that the number will be formatted so it's not a number anymore.
Test Plan: /paste/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4764
Summary: Convert most phabricator_render_form callsites. In the case of the "headsup view", it converts it by deleting the element entirely (this is the very old Maniphest/Differential header which we no longer use).
Test Plan: Poked around a bit.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4726
Summary: I cheated in a couple of places here, but this is in the process of getting refactored anyway, and there's a pretty clear boundary.
Test Plan: Viewed changesets in Differential, viewed standalone. Viewed context elements.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4723
Summary: Neither author nor reviewer can be a mailing list.
Test Plan: /differential/filter/revisions/, saw "Type a user name".
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4722
Summary:
Created with spatch:
lang=diff
- phutil_render_tag
+ phutil_tag
(X, Y, pht('...'))
The searched for `<` and `&` by sgrep.
Test Plan: Loaded homepage.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4504
Summary:
Created with spatch:
lang=diff
- phutil_render_tag
+ phutil_tag
(X, Y, '...')
Then searched for `&` and `<` in the output and replaced them.
Test Plan: Loaded homepage.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4503
Summary: Revisions you should review usually require faster response than revisions you should update or commit.
Test Plan:
/
/differential/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4606
Summary:
Went through this last night, I had to remove some static vars, but didn't see that as a huge perf issue.
Lint
Test Plan: Tested numerous differential pages, creating a diff, commenting, editing.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4617
Summary:
See title.
Also some minor styling/consistency fixes.
Test Plan:
- Clicked subscribe
- Canceled to make sure it went away
- Clicked it again
- Clicked subscribe
- Saw my name in the cc field.
Reviewers: epriestley, chad, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4571
Summary:
T2345
getConfig throws an Exception when the key does not exist.
Also removes dead code that throws an Exception.
Test Plan:
Reloaded the Phabricator home page. In the process, found
2 Exceptions thrown due to nonexistent keys. After addressing these problems,
the home page loads without Exceptions.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4541
Summary: Fixes T2316
Test Plan:
When the config file allows reopening,
navigate to a closed revision and reopen it in the user interface,
and verify that the revision now "needs review."
Also checks that the reopen option is unavailable when disallowed
by the config file.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2316
Differential Revision: https://secure.phabricator.com/D4526
Summary: Still working through basic re-design. Adds the ability to re-use panel view without the background.
Test Plan: Viewed Diffusion and Differential in Chrome, FF, Safari.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4430
Summary:
- Add a query parameter to select the diff renderer.
- When we populate diffs, use the 1-up renderer if the device isn't a desktop. Don't switch renderers once the page has loaded since it will be a tremendous mess. We can add an "always use 1-up" preference later.
- Tweak JX.Device so we avoid a race condition, where `JX.Device.getDevice()` may not be valid when other behaviors run.
- Implement enough of the 1-up HTML renderer to provide a vague hint that it actually works.
- Fix a couple of bugs with primitive construction.
Test Plan:
{F29168}
{F29169}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4423
Summary:
This is useful for writing feedback - what did I work on with someone?
This creates `AND IN (...)` and not `AND AND AND` if more participants are specified.
User may not expect it but whatever, it works the same for the most common case of 1 extra participant.
It would be nice to have this also for other filters but it would by way harder.
Test Plan: Displayed my revisions with some specific reviewer and some elses revisions with me as reviewer.
Reviewers: epriestley
Reviewed By: epriestley
CC: wez, aran, Korvin
Differential Revision: https://secure.phabricator.com/D4408
Summary:
I want to check the work of several people (bootcampers) at once.
This implements the OR filter required for that.
In the next diff, I plan to implement also the AND filter which can be useful too.
Test Plan:
Searched for several people, clicked all filters.
Searched for one person, verified that pretty URL is still created.
Reviewers: epriestley
Reviewed By: epriestley
CC: wez, aran, Korvin
Differential Revision: https://secure.phabricator.com/D4404
Summary:
Ref T2296. If a file upload fails (e.g., too large), we read the textarea from the "Create New Diff" interface. This means we show the user an error like "empty diff" when we should show them an error like "file upload failed, patch is too large". This is part of the issue in T2296, which features a 2.5MB diff.
Instead, check if a file was specified, so we'll raise a better error.
Test Plan: Tried to upload a large patch, got a "file is too large" error instead of an empty-diff-related error.
Reviewers: btrahan, vrana, codeblock
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2296
Differential Revision: https://secure.phabricator.com/D4369
Summary: We use "D<id>" for revisions.
Test Plan: Looked at revision.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4258
Summary: we don't always have a diff so instead set an explicit title in the controller.
Test Plan: no more fatals. grepped carefully for every call site and tested them all
Summary: basically made the header elements for the individual panes and cleaned up the panes to make it look okay. tried to copy colors from @chad 's mocks.
Test Plan: looks good to me
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T2004
Differential Revision: https://secure.phabricator.com/D4174
Summary:
upgrades are CrumbsView, HeaderView, PropertyListView, and ActionListView. I had to modify CrumbsView stuff a bit to handle the "advanced" diffusion crumbs.
Quirks fixed include making file tree view show up in diffusion, the page not have extra space when the file tree is hidden, links no longer breaking once you visit files (since without the change the files always got "/" appended and thus 404'd), and a differential quirk where it read "next step:" and that colon is a no no,
Test Plan: played around in diffusion and differential
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, chad
Maniphest Tasks: T2048, T2178
Differential Revision: https://secure.phabricator.com/D4169
Summary: actions are still a bit messy - unsatisfactory icons (T2013 will help!)
Test Plan: viewed diffs - they look good
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T2007
Differential Revision: https://secure.phabricator.com/D3904
Summary: Adds very basic crumbs to Differential, to prevent regression when we drop the application menu. I'll do a more proper pass at this but want to unblock landing the commit sequence for all this stuff.
Test Plan: Looked at detail view and list view, saw crumbs, clicked them.
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1960
Differential Revision: https://secure.phabricator.com/D4111
Summary:
- Adds `PhabricatorMenuItemView` which is a non-hacky object representing a single menu item.
- Adds `PhabricatorMenuView`, a collection of items.
- Deletes some busted/old interfaces full of garbage nonsense.
- Merges menu item styles from `aphront-side-nav-view-css` and `phabricator-nav-view-css`. These are old-style and new-style rules which got partially updated recently.
- The new-style menus have a darker background (#ececec) than the old-style menus (#f7f7f7) so some of the highlight/hover colors weren't visible. I shuffled them around but something or other might need further adjustment.
Test Plan: looked at every menu I could
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1960
Differential Revision: https://secure.phabricator.com/D4036
Summary:
AphrontSideNavView is an old class which required you to do a lot of work; it was obsoleted by AphrontSideNavFilterView. Remove all direct callsites so I can clean it up.
This is a precursor to letting me render a filter menu as a dropdown menu for T1960.
Test Plan:
Examined each interface for correct filter construction and selection:
- Browsed Diffusion
- Browsed Differential
- Browsed Files
- Browsed Slowvote
- Browsed Phriction
- Browsed repo edit interface
Grepped for `AphrontSideNavView`. The only remaining instances are in `AphrontSideNavView` itself and `AphrontSideNavFilterView` (which currently uses it).
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1960
Differential Revision: https://secure.phabricator.com/D4034
Summary:
assume at least 360px for a given code pane. that's about when the comment box starts fighting back anyway. we'll use the yet-to-be-built one page render for the narrow viewport cases.
This address the cases as laid out in T2005. It fails the "MMMMM" case pretty horribly. However, if there is a space it works just fine and presumably folks are stretching out their windows on big glorious monitors at 160 characters wide or whatever.
Re-factored things just a tad but figure I'll take a nice big chunk of "renderer" to move forward T2009
Test Plan: looked at all sorts of funky diffs
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T2005
Differential Revision: https://secure.phabricator.com/D4083
Summary: I am an idiot. :( D3962 added the full set of properties and I derp'ed this part of the diff up
Test Plan: ask lesliepc16 to take a look
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: lesliepc16, aran, Korvin
Maniphest Tasks: T2091
Differential Revision: https://secure.phabricator.com/D3984
Summary:
see title. Note that fields that use customs storage don't work because I didn't think it made sense to load a revision object to get that data. Further, we don't have a revision id at some points, so its not clear what does / does not work...?
Also added a link to upsell this diff view as I had trouble finding it.
Test Plan: viewed some diffs
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T2026
Differential Revision: https://secure.phabricator.com/D3962
Summary: 'cuz who cares unless you need review?
Test Plan: noted the UI showed up appropriately to my new business logix
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T2010
Differential Revision: https://secure.phabricator.com/D3958
Summary:
This commit doesn't change license of any file. It just makes the license implicit (inherited from LICENSE file in the root directory).
We are removing the headers for these reasons:
- It wastes space in editors, less code is visible in editor upon opening a file.
- It brings noise to diff of the first change of any file every year.
- It confuses Git file copy detection when creating small files.
- We don't have an explicit license header in other files (JS, CSS, images, documentation).
- Using license header in every file is not obligatory: http://www.apache.org/dev/apply-license.html#new.
This change is approved by Alma Chao (Lead Open Source and IP Counsel at Facebook).
Test Plan: Verified that the license survived only in LICENSE file and that it didn't modify externals.
Reviewers: epriestley, davidrecordon
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2035
Differential Revision: https://secure.phabricator.com/D3886
Summary:
all sorts of stuff
- made comment form width flexible
- made margins element specific rather than part of differential-primary-pane
- made box elements all veritically align left and right until code stuff
- re-factored width calculaton stuff a bunch so only the code section has to suffer from max-width calculations; everything else can flex
- made colspan 3 for rightmost table header element. this is so the "View Options" UI element ends up lining up correctly with the "Show All Lines" element just below
Test Plan: looked at revision view and changeset view and it all looked hot. note I did not test what things looked like with different word wrap values; that should still work given the re-factoring and not re-design here. also toggled haunted panel mode and it looked good.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2006
Differential Revision: https://secure.phabricator.com/D3866
Summary:
See T1963 for discussion of the Facebook-specific hack.
Differential currently uses a one-stage cache (render -> postprocess -> save in cache) rather than the two-stage cache (render -> save in cache -> postprocess) offered by `PhabricatorMarkupInteface`. This breaks Differential comments coming out of cache for the lightbox, and makes various other things suboptimal (status of handles like @mentions and embeds are not displayed accurately).
Instead, use the modern stuff.
Test Plan:
- Created preview comments and inlines in Differential.
- Edited a Differential inline.
- Submitted main and inline Differential comments.
- Viewed and edited Differential summary and test plan.
- Created preview comments and inlines in Diffusion.
- Submitted comments and inlines in Diffusion.
- Verified Differential now loads and saves to the generalized markup cache (Diffusion is close, but main comments still hold a single-stage cache).
- Verified old Differential comments work correctly with the lightbox.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1963
Differential Revision: https://secure.phabricator.com/D3804
Summary:
We need to load all properties with some prefix in one field.
We can't merge them in one property because there will be a race condition for update (we don't have API for load+update+save).
Instead of providing API for this and complicating the code even more, just load everything unconditionally.
It shouldn't waste much bandwith or memory because we use most of the properties anyway.
It also looked overengineered to me.
Test Plan: Displayed revision with fields using diff properties.
Reviewers: epriestley, royw
Reviewed By: royw
CC: aran, royw, Korvin
Differential Revision: https://secure.phabricator.com/D3676
Summary: When we receive an email, figure out if any of the other tos and ccs are users. If they are, pass their phids through the stach as "exclude phids" and exclude them from getting the email.
Test Plan: used the various applications (audit, differential, maniphest) and noted emails were sent as expected.
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: aran, Korvin, vrana
Maniphest Tasks: T1676
Differential Revision: https://secure.phabricator.com/D3645
Summary: We have lots of empty drafts in DB.
Test Plan: Wrote revision comment, deleted it, checked db.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3591
Summary:
It happens to me quite often that I leave the window with revision (by closing it or by visiting a link from it).
When I return then the comment draft is there so I clowncopterize it but forget that I wanted to take some other action than Comment.
Test Plan: Selected "Add Reviewers", added some reviewers, closed the window, opened it - the action and reviewers were still there.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3530
Summary:
This is the last Paste UI element that doesn't work properly on tablets/phones. Make it flexible.
Also add empty states to Paste.
Test Plan: Viewed various errors, and `/uiexample/errors/`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3429
Summary:
I've replaced all `id(new PhabricatorObjectHandleData(...))->loadHandles()` by `$this->loadViewerHandles(...)`.
Lint caught one usage in a static method.
Test Plan: Displayed revision with sporadic author.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3432
Summary: It would be best to add this everywhere at once but I'm too lazy for it.
Test Plan: Displayed package list and detail and revision comment with away users.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3419
Summary:
Some fields require displayed diff (e.g. Lines), some require last manual diff (Lint, Unit).
Lint requires both - it needs to find if the line with the error is displayed or not.
Test Plan: Displayed committed diff with lint errors, clicked on them, got last manual diff at the correct line.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3331
Summary:
Ball is more obvious and visible than I thought.
Delete the status word and display until date in title.
Also display the until date in revision list.
Also display near future dates with DoW instead of year.
Test Plan: Displayed revision and revision list with away reviewers.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3407
Summary: this is useful for certain workflows, typically where the reviewer is a gatekeeper of sorts who does the acutal commit. Special thanks to D2900 which made this relatively brain-dead to code up.
Test Plan: set to "true" in my local development environment and verified test user "xerxes" could close my stuff
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1732
Differential Revision: https://secure.phabricator.com/D3398
Summary: See comment inline. We should fix this properly but it goes faily deep so I just stopped the bleeding for now. It's OK if we end up with a silly-looking file tree view for bizarre edge cases.
Test Plan: Created a problem diff as described, verified it no longer threw.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, Avish
Maniphest Tasks: T1702
Differential Revision: https://secure.phabricator.com/D3373
Summary:
Adds a flexible navigation menu to diffs that shows you your current position in the diff.
Anticipating some "this is the best thing ever" and some "this is the wosrt thing ever" on this, but let's see how much pushback we get? It seems pretty good to me.
Test Plan: Will attach screenshots.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1633, T1591
Differential Revision: https://secure.phabricator.com/D3355
Summary:
Rendering method shouldn't load data.
The view probably shouldn't load data either because it is a job for component (object that both loads data and displays them) but we don't have that concept in Phabricator.
This at least improves the architecture a little bit.
Test Plan: /differential/
Reviewers: epriestley
Reviewed By: epriestley
CC: alanh, aran, Korvin
Differential Revision: https://secure.phabricator.com/D3325
Summary:
This is another experiment for reducing reviewers response time.
I stole the idea (and colors) from [[ http://www.reviewboard.org/media/screenshots/2009/02/02/dashboard.png | ReviewBoard ]].
I actually quite like it (except when everything is red) and I can image that people will review just to have better color balance.
The code is not production ready for these reasons:
- We load holidays again and again for each revision. I couldn't cache it to static variable because it could persist multiple requests, right?
- I don't know how to expand height to the whole cell (I'm really bad in CSS).
- CSS rules are probably in wrong file.
- We probably want to use different colors.
This is how it looks:
{F16406}
Test Plan: Displayed revision list.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3190
Summary:
The logic here was swapped - new file should be on the right side.
Plus we had a fatal for VS = -1 where new file should be on left.
Test Plan:
Downloaded raw diff of:
- base VS change
- change VS change
- change VS change with unmodified file
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3333
Summary: We need to use commit diff in some links and manual diff in some others.
Test Plan: Displayed revision, verified that the action link uses commit diff.
Reviewers: epriestley, nh
Reviewed By: nh
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3300
Summary:
Trigger the crossreference behavior on code blocks. Limited to
Differential, where we know what the project is, but includes regular
comments, inline comments, and previews of both.
(Hopefully event handlers on deleted elements also get deleted, so we
don't leak memory? Also, caching is a problem, and I didn't find a way
to mark existing cache entries as stale, like
`DifferentialChangesetParser::CACHE_VERSION`...)
Test Plan:
Load Differential revision, make lots of comments, click on
things.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1602
Differential Revision: https://secure.phabricator.com/D3283
Summary:
- Add getHelpURI() to PhabricatorApplication for application user guides.
- Add a new "help" icon menu item and skeletal Diviner application.
- Move help tabs to Applications where they exist, document the other ones that don't exist yet.
- Grep for all tab-related stuff and delete it.
Test Plan: Clicked "help" for some apps. Clicked around randomly in a bunch of other apps.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3267
Summary:
In a revision with multiple diffs like
https://secure.phabricator.com/D3168?vs=6094&id=6095
clicking "Show Raw File (Left)" while comparing diffs 1 and 2 brings up
the version from base, instead of from diff 1. This is because the hunks
are stored as diffs between base and diff X, and the raw file is
generated from the hunks. This introduces a hack which is probably not
actually correct but seems to work for the 90% case.
(The "Show Raw File (Right)" button was and remains correct.)
Test Plan: Click raw file buttons while comparing different diffs.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3169
Summary: created a PhabricatorInlineCommentPreviewController so controllers in Diffusion and Differential respectively just have to handle the URI mapping and data loading like good little controllers.
Test Plan:
left inline comments on commits, deleted inline commits, submitted inline comments -- all worked well
did the same on some diffs
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1176
Differential Revision: https://secure.phabricator.com/D3034
Summary: For "accept" and "reject" action, find the time between the action and last time the revision was updated by a new diff. Show it as the responsiveness row.
Test Plan: view it for a couple of engineers.
Reviewers: epriestley, vii
Reviewed By: epriestley
CC: vrana, nh, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2970
Summary:
We have a bit more copy-paste than we need, consolidate a bit.
(Also switch Mercurial to download git diffs, which it handles well; we use them in "arc patch".)
Test Plan:
- Downloaded a raw diff from Differential.
- Downloaded a raw change from Diffusion.
- Downloaded a raw file from Diffusion.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2942
Summary: Did exactly what @epriestley suggested in T1428#2.
Test Plan: Turn it on in your config, post a revision, accept it. Turn it off in your config, post a revision, can't accept it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2900
Summary:
Diff of diffs display changes between new versions of file.
This is bad after rebase because there can be many unrelated changes so it is hard to spot the real change.
This diff unhighlights the lines that were added or removed in rebase.
The changes are still visible (they can be sometimes relevant) but very subtle.
Test Plan:
# Add, change and delete line. Display diff.
# Add and change some lines in parent. Rebase. Display diff. Display diff of diff.
# Change and add some lines. Display diff. Display diff to first diff. Display diff to second diff.
Reviewers: epriestley, jungejason
Reviewed By: jungejason
CC: jungejason, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2761
Summary: this lets people download diffs easily in case arc export, arc patch, etc isn't to their liking or whatever.
Test Plan: "Download Raw Diff" a few times with various things toggled in the "Revision Update History" widget
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1294
Differential Revision: https://secure.phabricator.com/D2855
Summary:
"differential.creatediff" requires a mostly-parsed diff, but there's no reason we can't make DifferentialDiffs out of raw diffs.
This mainly serves to lower the adoption barrier if getting "arc" distributed is too much of a hassle.
Test Plan: Made a diff out of a raw block of diff text.
Reviewers: ffx, vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2751
Test Plan: Created action link using VS diff, selected VS diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2770
Summary:
This is to allow conservative people to bring back the old manners.
NOTE: It doesn't mark all occurrences but I think that's good enough. We will eventually mark them all.
Test Plan: Translated 'closed', displayed closed revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, asukhachev
Maniphest Tasks: T1139
Differential Revision: https://secure.phabricator.com/D2760
Summary: The notification implementation has been extended to Differential. Appropriate changes have been made to the Differential editors and Differential feed story.
Test Plan: Tested out various actions available for Differential and confirmed that the notifications get delivered correctly and feed is generated.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: allenjohnashton, ddfisher, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2696
Summary:
- `kill_init.php` said "Moving 1000 files" - I hope that this is not some limit in `FileFinder`.
- [src/infrastructure/celerity] `git mv utils.php map.php; git mv api/utils.php api.php`
- Comment `phutil_libraries` in `.arcconfig` and run `arc liberate`.
NOTE: `arc diff` timed out so I'm pushing it without review.
Test Plan:
/D1234
Browsed around, especially in `applications/repository/worker/commitchangeparser` and `applications/` in general.
Auditors: epriestley
Maniphest Tasks: T1103
Summary: Also fix the notice text.
Test Plan:
Display diff with inline comments and lint errors.
Click on inline comment and lint link.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2594
Summary:
This adds a link to [Closed] e-mail if it detects some changes.
It compares added and removed lines with 3 lines context.
The subtle form of informing is permissive to false negatives and positives.
I have an e-mail filter for [Closed] e-mails so I wouldn't personally notice this change - we should probably promote this feature a little bit.
Test Plan:
Reparse a diff with a change after last update.
Reparse a diff without a change after last update.
Reviewers: jungejason, epriestley
Reviewed By: jungejason
CC: aran, Koolvin, btrahan
Maniphest Tasks: T201
Differential Revision: https://secure.phabricator.com/D2540
Summary:
I thought about it a little bit and this makes the most sense for me:
# Original author usually writes at least something and commander only updates it.
# There's a creation date of revision (= first diff) by these comments. I don't want to change this date because I use this information. Author should correspond to this date.
# It solves all our repros.
Test Plan: Display commandeered revision.
Reviewers: nh, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2577
Summary:
Instead of assuming the test plan and summary are written by the author
of the differential revision, let's assume they are written by the author
of the latest differential diff.
Test Plan: viewed a drev that had been commandeered but not updated to check authors
Reviewers: epriestley, jungejason, vrana
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1235
Differential Revision: https://secure.phabricator.com/D2550
Summary:
This attaches commit diff to its associated revision as any other diff.
The consequence is that the revision page now shows the actual commit instead of the last diff. It may be disturbing but it is desired.
Another consequence is that lint and unit results are displayed as skipped after the revision was committed. I want to fix it somehow.
My next plan is to automatically diff against the last normal diff and include the link to this diff in commit e-mail if non-empty.
Test Plan:
reparse.php
Diff against last normal diff.
Reviewers: epriestley, jungejason
Reviewed By: jungejason
CC: aran, Koolvin, jungejason
Maniphest Tasks: T201
Differential Revision: https://secure.phabricator.com/D2530
Summary: the existing, more custom text doesn't make sense when viewing someone else's revisions AND in someplaces its the less interesting "no data found".
Test Plan: clicked around the various http://phabricator.dev/differential/filter/* pages with various users.
Reviewers: epriestley, ry
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1222
Differential Revision: https://secure.phabricator.com/D2475
Summary: This allows writing inline comments and reduces different behavior between normal and very large diffs.
Test Plan:
Verify that normal diff works.
Verify that very large diff works.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2361
Summary:
This adds support to differential fields to display warnings before a revision
gets accepted. Since lint and unit are differential fields, the code for their
warnings was moved into their respective field specification classes, so there
is only one code path for warnings (lint, unit, or custom).
Test Plan:
Select 'Accept' on a revision with lint/unit warnings and see messages appear
like they used to. Change it back to 'Comment' and they go away. Repeat with
a revision without lint/unit warnings and see no warnings appear. Checked
darkconsole to see no errors due to this.
Reviewers: jungejason, epriestley, vrana
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2363
Summary: Inspired by D2242.
Test Plan:
Select text in left pane.
Select text in right pane.
Select all.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2249
Summary: "Committed" is SVN-specific language, and confusing in Git and Mercurial. Use neutral language instead.
Test Plan: Inspection.
Reviewers: btrahan, Makinde, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T909
Differential Revision: https://secure.phabricator.com/D2087
Summary:
This is slightly more complicated for this reason:
- We don't set `dateCommitted` for normal commits, only for markcommitted.
-- We need to add this date to old revisions now.
Test Plan:
Reparse a revision - commit date was set.
Conduit `markcommitted` - commit date was set.
Run SQL script.
Display closed revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2282
Summary: NOTE: This is starting to be too hacky.
Test Plan:
View revision with inline diffs, verify that Reply is there.
View standalone - no Reply.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2263
Summary:
- Adds "Commandeer Revision", to allow you to plunder revisions from those lost to sea (e.g., interns who have left or co-workers who are dealing with a family emergency).
- Removes admin-abandon to simplify things, since you can just Commandeer + Abandon now.
- There are other workarounds available but this is the natural/expected workflow (and the one everyone always asks for) and there's no real reason not to allow it.
Test Plan: Swashbuckled.
Reviewers: cpiro, btrahan, vrana, jungejason
Reviewed By: cpiro
CC: aran, zeeg
Differential Revision: https://secure.phabricator.com/D2257
Summary: Partially broken by D2166.
Test Plan:
Hover line number in revision.
Hover line number in standalone view.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2196
Summary: We'll get a typehint warning on the repository if there's no repository. Check outside the method instead.
Test Plan: Loaded page, no warning.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2194
Test Plan:
/rX1
Browse in Diffusion
Open in Editor
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2180
I think the issue is that we don't set the left-side changesetID correctly. This seems to work correctly locally, but I'm not sure I got a good repro. Pushing to verify the production test cases provided in T1076.
Auditors: vrana, btrahan
Summary: NOTE: This is not produced by a script so there might be errors. Please review carefully.
Test Plan: Browse around Differential.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2103
Summary:
- Differential, Maniphest and Diffusion use slightly different styles for the object detail panels.
- Instead, use the same styles and CSS.
- Add object actions to Diffusion, including "Flag".
Test Plan: Looked at revisions, tasks and commit. Flagged and unflagged commits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1041
Differential Revision: https://secure.phabricator.com/D2062
Summary:
I think this feature is probably good, but Differential is also really starting to get a lot of stuff which is not the diff in it. Not sure how best to deal with that.
The mixed table styles are also pretty ugly.
So I guess this is more feedback / proof-of-concept, I think I want to try to improve it somehow before I land it.
Test Plan: Looked at some diffs, some had an awkward, ugly list of diffs affecting the same files.
Reviewers: bill, aran, btrahan
Reviewed By: aran
CC: aran, epriestley
Maniphest Tasks: T829
Differential Revision: https://secure.phabricator.com/D2027
Test Plan: Looked at /differential/ with and without flags.
Reviewers: nh, btrahan
Reviewed By: nh
CC: aran, epriestley
Maniphest Tasks: T1055
Differential Revision: https://secure.phabricator.com/D2044
Summary:
Flags are a personal collection of things you want to take a look at later. You can use several different colors and add notes.
Not really sure if this is actually a good idea or not but it was easy to build.
Planned features:
- Allow Herald rules to add flags.
- In the "edit flag" dialog, have a "[x] Subscribe Me" checkbox that CCs you.
- Support Diffusion.
- Support Phriction.
- Always show flags on an object if you have them (in every view)?
- Edit dialog feels a little heavy?
- More filtering in /flag/ tool.
- Add a top-level links somewhere?
Test Plan: Added, edited and removed flags from things. Viewed flags in flag view.
Reviewers: aran, btrahan
Reviewed By: btrahan
CC: aran, epriestley, Koolvin
Maniphest Tasks: T1041
Differential Revision: https://secure.phabricator.com/D2024
Summary: This header allows recipients to distinguish between CCs generated by Herald and CCs generated by humans.
Test Plan: Created a Herald rule to add a bunch of CC's to every revision. Created a revision. Added some CCs manually. Verified that only manual CCs appeared in the "Explicit" header.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T808
Differential Revision: https://secure.phabricator.com/D2018
Summary:
In the Differential revision list views:
- Allow you to filter by mailables (notably, mailing lists).
- Allow you to filter by user (including disabled users).
Test Plan: Filtered by a mailing list.
Reviewers: btrahan, nh
CC: aran, epriestley
Maniphest Tasks: T1031
Differential Revision: https://secure.phabricator.com/D1994
Summary:
1. Created a filter for user comments that are not submitted yet
2. surface this information to the user by providing a "Draft revisions" link on the differential home page.
Test Plan: tested on one of the diffs
Reviewers: nh, epriestley
Reviewed By: epriestley
CC: aran, epriestley, jungejason
Differential Revision: https://secure.phabricator.com/D1927
Summary:
Depends on D1921. Depends on D1899.
Add the "View Options" dropdown menu to Diffusion, with options like "show standalone", "show raw file", "show all", etc.
Test Plan: Viewed commits in Differential and Diffusion.
Reviewers: davidreuss, nh, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1932
Summary: In very large diffs.
Test Plan:
Display very large diff.
Display normal diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1918
Summary:
- Add inline comments to Audits, like Differential.
- Creates new storage for the comments in the Audits database.
- Creates a new PhabricatorAuditInlineComment class, similar to DifferentialInlineComment.
- Defines an Interface which Differential and Audit comments conform to.
- Makes consumers of DifferentialInlineComments consume objects which implement that interface instead.
- Adds save
NOTE: Some features are still missing! Wanted to cut this off before it got crazy:
- Inline comments aren't shown in the main comment list.
- Inline comments aren't shown in the emails.
- Inline comments aren't previewed.
I'll followup with those but this was getting pretty big.
@vrana, does the SQL change look correct?
Test Plan:
- Created, edited, deleted, replied to, reloaded and saved inline comments in Diffusion, on the left and right side of diffs.
- Created, edited, deleted, replied to, reloaded and saved inline comments in Differentila, on the left and right side of primary and diff-versus-diff diffs.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1898
Summary:
These are all unambiguously unextensible. Issues I hit:
- Maniphest Change/Diff controllers, just consolidated them.
- Some search controllers incorrectly extend from "Search" but should extend from "SearchBase". This has no runtime effects.
- D1836 introduced a closure, which we don't handle correctly (somewhat on purpose; we target PHP 5.2). See T962.
Test Plan: Ran "testEverythingImplemented" unit test to identify classes extending from `final` classes. Resolved issues.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1843
Summary: When a revision is accepted, allow users to manually mark it committed if there's a daemon/tracking problem. This shouldn't happen in most installs but we have less-robust support for Mercurial and some installs may not be fully configured.
Test Plan: Marked a revision committed.
Reviewers: btrahan, Makinde
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T948
Differential Revision: https://secure.phabricator.com/D1808
Summary:
Filter Revisions button currently resets Status and Order fields.
I've rewritten it to GET form because it doesn't perform any action.
It fixed the problem along the way.
Test Plan:
/differential/filter/revisions/
Status: Open.
Filter Revisions.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1771
Summary:
Some text editors support opening multiple files at once.
I've used space as paths separator which may be compatible with some other
editors (I didn't tried any other though).
Note: This approach is incompatible with spaces in paths.
I am fine with changing it to anything else to support such paths or more
editors.
Probably the cleanest solution (yet still incompatible with most editors) would
be to use something like ##editor://open/?file=A&line=1&file=B&line=2## but it
would require also changing the way how it's configured and I think it's not
worth it.
BTW, I've used a hacky bookmarklet for this feature before.
Deleted or added paths may not exist in users filesystem but we don't know which
so the button tries to open everything.
Test Plan:
Click Edit All.
Delete Editor Link in settings, verify that the button is missing.
View diff without revision, verify that the button is missing.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1741
Summary:
The current approach of using a modal overlay dialog to create/edit inline
comments is pretty silly. Use an inline textarea instead.
This element isn't perfect and we have some mild modalness issues, but I think
it's better than the silly thing we've got going on right now. We can keep
poking it as people break it.
Test Plan:
- Created comments; submitted and undid them in empty and nonempty states.
Used undo for nonempty states + cancel.
- Edited comments; saved and canceled them. Used undo for changed state.
- Replied to comments; yada yada as above.
- Deleted comments.
- Did various modal trickery where I clicked "Reply" on something else with a
dialog already up, this very mildly glitches but I think it's not a big issue.
Reviewers: vrana, btrahan, Makinde, nh
Reviewed By: vrana
CC: aran, epriestley
Maniphest Tasks: T431
Differential Revision: https://secure.phabricator.com/D1716
Summary: This control is a very thin shell right now with Maniphest/Differential
code duplication; unify the implemenations better for use in Audit.
Test Plan: Clicked toggle buttons in Differential and Maniphest.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1700
Summary:
Show some statistics, like number of revisions, number of
revisions per week, lines per revision, etc. for phrivolous amusement.
Test Plan:
- Went to /differential/stats/revisions/
Numbers seem right
- Clicked 'Accepted'
Again
- Changed to another user with long history
Load time was not too long though delay noticeable
- Clicked 'Requested changes to'
User was preserved, looks good
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1643
Summary:
Build the revision list table out of custom fields instead of hard-coding it, so
installs can add all sorts of zany things to it.
NOTE: You may need to implement sortFieldsForRevisionList() if you have a custom
DifferentialFieldSelector, or some fields might show up out of order.
This implementation will preserve the expected behavior:
public function sortFieldsForRevisionList(array $fields) {
$default = new DifferentialDefaultFieldSelector();
return $default->sortFieldsForRevisionList($fields);
}
Test Plan:
- Loaded differential revision list, identical to old list.
- Profiled page to verify the cost increase isn't significant (it's quite
small).
Reviewers: jungejason, btrahan
Reviewed By: btrahan
CC: aran, btrahan, davidreuss, epriestley
Maniphest Tasks: T773, T729
Differential Revision: https://secure.phabricator.com/D1388
before displaying it
Summary:
@alok reported a vulnerability where Flash will run carefully-crafted plain text
files.
When the user requests a raw file, cache it into Files if it isn't already
there. Then redirect them to Files. This solves the problem by executing the
SWF/TXT with CDN-domain permissions, not content-domain permissions, provided
the install is correctly configured. (Followup diff coming to make this more
universally true.)
NOTE: We'll still show raw data in Diffusion. The barrier to XSS here is much
higher (you need commit access) but I'll do something similar there. We aren't
vulnerable in Paste, since we already use Files.
Test Plan: Clicked "View Old File", "View New File" in an alt-domain
configuration, got redirected to a cookie-free domain before being delivered the
response.
Reviewers: btrahan, alok
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1607
Summary: Add a "Search for ... in (document group)" thing that picks the current
scope based on the current application.
Test Plan: Conducted searches in several browsers.
Reviewers: btrahan, skrul
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T858
Differential Revision: https://secure.phabricator.com/D1610
Summary: It makes perfect sense to add more reviewers while requesting review.
Test Plan:
Request review. Verify that Add Reviewers field shows and works.
Add some reviewer. Verify that comment preview works.
Submit. Verify that reviewers are saved and displayed.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1473
Summary:
Rough cut that still needs a lot of polish, but replace the directory list with
more of a dashboard type thing:
- Show "Unbreak Now", triage-in-your-projects, and other stuff that you're
supposed to deal with, then feed.
- Move tools a click a way behind nav -- this also lets us put more stuff
there and subtools, etc., later.
- Remove tabs.
- Merge the category/item editing views.
- I also added a light blue wash to the side nav, not sure if I like that or
not.
Test Plan:
- Viewed all elements in empty and nonempty states.
- Viewed applications, edited items/categories.
Reviewers: btrahan, aran
Reviewed By: btrahan
CC: aran, epriestley, davidreuss
Maniphest Tasks: T21, T631
Differential Revision: https://secure.phabricator.com/D1574
Summary:
See D1533#5.
Also deduplicates logic of what is stored to blob in ArcanistDiffWorkflow.
Blame Rev: D1533
Test Plan:
Display raw version of text file.
Display raw version of image.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1583
Summary:
This code was just all kinds of wrong, but got all the common cases anyone cares
about correct.
- In edit-inline-comments.js, if isOnRight() is true, use data.right, not
data.left (derp).
- Set data.left correctly, not to the same value as data.right (derp derp).
- Set "isNewFile" based on $is_new, not $on_right (derp derp derp).
Test Plan:
- Added JS debugging code to print "OLD" vs "NEW" and "LEFT" vs "RIGHT".
Clicked the left and right sides of diff-vs-base and diff-vs-diff diffs,
verified output was accurate in all cases.
- Added comments to the left-display-side of a diff-of-diffs, saved them, they
showed up where I put them.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T543
Differential Revision: https://secure.phabricator.com/D1567
Summary:
When a user selects "show raw file (right)" from the dropdown of a binary file
in differential, they should get more than a blank page.
Test Plan: Loaded a raw binary file from differential
Reviewers: tuomaspelkonen, epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1533
Summary:
Render coverage information in the right gutter, if available.
We could render some kind of summary report deal too but this seems like a good
start.
Test Plan:
- Looked at diffs with coverage.
- Looked at diffs without coverage.
- Used inline comments, diff-of-diff, "show more", "show entire file", "show
generated file", "undo". Nothing seemed disrupted by the addition of a 5th
column.
Reviewers: btrahan, tuomaspelkonen, jungejason
Reviewed By: btrahan
CC: zeeg, aran, epriestley
Maniphest Tasks: T140
Differential Revision: https://secure.phabricator.com/D1527
Summary:
It is possible to open a file in editor by registering a custom URI scheme
(pseudo-protocol). Some editors register it by default.
Having links to open the file in external editor is productivity booster
although it is a little bit harder to set up.
There are several other tools using file_link_format configuration directive
(XDebug, Symfony) to bind to this protocol.
I've added the example with editor: protocol which can be used as a proxy to
actual editor (used by Nette Framework:
http://wiki.nette.org/en/howto-editor-link).
Test Plan:
Configure Editor Link in User Preferences.
Register URI scheme in OS.
Open a file in Diffusion. Click on the Edit button.
Open a revision in Differential. Click on the Edit button.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1422
Summary: There are lots of callsites to $changeset->getFilename() so it seemed
easier to rename getFileName() to getFilename() even if it includes database
change. Plus I think that getFilename() is better.
Test Plan:
Alter database.
Open revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1437
Summary:
We currently allow you to assign code review to disabled users, but
should not.
Test Plan:
- Created revisions with no reviewers and only disabled reviewers, was
appropriately warned.
- Looked at a disabled user handle link, was clearly informed.
- Tried to create a new revision with a disabled reviewer, was rebuffed.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D1429
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
Summary:
Commenting on a diff causes adding the writer to the CCs. It doesn't make much
sense if the writer is author or reviewer who get all the copies anyway.
I've also moved the decision to DifferentialCommentEditor.
Test Plan:
Comment on a diff where I am author
Comment on a diff where I am reviewer
Comment on a diff where I am neither
Explicitely Add CCs where I am author
Reviewers: epriestley
Reviewed By: epriestley
CC: jungejason, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1397
Summary:
engineers requested to supporting filtering by 'committed'
revisions, and I think it makes sense.
Test Plan: verified that all the three options worked
Reviewers: epriestley, btrahan, nh
Reviewed By: nh
CC: nh, wolffiex, aran
Differential Revision: https://secure.phabricator.com/D1383
Phabricator
Summary: ...this breaks without D1328. Used good ole "codemod" to do this
work, with lots of manual edits around 80 chars.
Test Plan: clicked around phabricator tool suite, particular differential, a
bunch
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1351
Summary:
We currently don't link to comments which aren't visible. Link to the
appropriate diff in a new window, indicating where the comment lives.
Test Plan: Clicked visible, not-so-visible comments.
Reviewers: btrahan, jungejason, davidreuss
Reviewed By: btrahan
CC: aran, btrahan, epriestley
Maniphest Tasks: T555, T449
Differential Revision: https://secure.phabricator.com/D1333
Summary:
Provide an easy way to jump to Diffusion from Differential if we have
the data we need to connect them.
Test Plan: Tested menu in linked and unlinked diffs. Used menu item.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Maniphest Tasks: T309
Differential Revision: https://secure.phabricator.com/D1326
Summary:
There are several open Differential tasks that are basically blocked on not
having reasonable places in the UI to put things. Replace the "View Standalone /
Raw" button with a "View Options" dropdown menu so we can shove things like
"Expand All", "Fold / Unfold File", and "View in Diffusion" in there.
This doesn't change any behavior, just puts the existing options in a menu.
Test Plan:
- Toggled menu open by clicking button.
- Clicked menu items.
- Toggled menu closed by clicking button.
- Toggled menu closed by clicking document.
- Toggled menu closed by opening another menu.
- Toggled menu closed by selecting an item.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T497, T309
Differential Revision: https://secure.phabricator.com/D1316
Summary: Preview of Add Reviewers looks silly without actually showing them
Test Plan:
Go to any diff
Leap into action: Add Reviewers
Add some reviewers
Write some comment
Preview including Added reviewers should be displayed
Change action to Comment
Added reviewers should disappear
Repeat with Add CCs
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, vrana
Differential Revision: https://secure.phabricator.com/D1276
Summary: Makes it easier to discover the list of all revisions for a user.
Test Plan:
Opened up /differential/filter/revisions/, and saw that it defaulted
to status of all. Clicked between tabs, and it stayed on all. Selected
open, it only displayed open revisions, including as I switched between
tabs.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1278
Summary:
- Use DifferentialRevisionQuery, not DifferentialRevisionListData, to select
revisions.
- Make UI simpler (I hope?) and more flexible, similar to Maniphest. It now
shows "Active", "Revisions", "Reviews" and "Subscribed" instead of a hodge-podge
of miscellaneous stuff. All now really has all revisions, not just open
revisions.
- Allow views to be filtered and sorted more flexibly.
- Allow anonymous users to use the per-user views, just don't default them
there.
NOTE: This might have performance implications! I need some help evaluating
them.
@nh / @jungejason / @aran, can one of you run some queries agianst FB's corpus?
The "active revisions" view is built much differently now. Before, we issued two
queries:
- SELECT (open revisions you authored that need revision) UNION ALL (open
revisions you are reviewing that need review)
- SELECT (open revisions you authored that need review) UNION ALL (open
revisions you are reviewing that need revision)
These two queries generate the "Action Required" and "Waiting on Others" views,
and are available in P247.
Now, we issue only one query:
- SELECT (open revisions you authored or are reviewing)
Then we divide them into the two tables in PHP. That query is available in P246.
On the secure.phabricator.com data, this new approach seems to be much better
(like, 10x better). But the secure.phabricator.com data isn't very large. Can
someone run it against Facebook's data (using a few heavy-hitting PHIDs, like
ola or something) to make sure it won't cause a regression?
In particular:
- Run the queries and make sure the new version doesn't take too long.
- Run the queries with EXPLAIN and give me the output maybe?
Test Plan:
- Looked at different filters.
- Changed "View User" PHID.
- Changed open/all.
- Changed sort order.
- Ran EXPLAIN / select against secure.phabricator.com corpus.
Reviewers: btrahan, nh, jungejason
Reviewed By: btrahan
CC: cpiro, aran, btrahan, epriestley, jungejason, nh
Maniphest Tasks: T586
Differential Revision: 1186
Summary:
This landed during my review drama embargo and is a generally good idea but had
some implementation issues.
@elynde reports it has been broken for some time, although it still works on
secure.phabricator.com so I'm guessing it's just taking a zillion years to run
at Facebook. It's up to more than a second for me on secure.phabricator.com:
https://secure.phabricator.com/file/view/PHID-FILE-v4ql4c66u3xnkarmrpm4/
The basic problem is that some of the data architecture around this
implementation is hard to scale. I want to pursue a similar feature eventually,
but drive it off notifications that we'll ship through real-time infrastructure
too.
I'm also trying to get rid of DifferentialRevisionListData and this simplifies
that somewhat.
Test Plan:
- Grepped for table name, table constant, query constant, and class name; no
hits.
- Applied SQL patch.
- Verified that Differential no longer shows "Updated".
Reviewers: elynde, btrahan, jungejason
Reviewed By: elynde
CC: aran, elynde
Differential Revision: 1178
Summary: make the change, kill the function. be sure to get a good $user or
$viewer variable
Test Plan:
for each controller or view, look at it in the ui. change timezone, refresh ui
and note change. i did not test the OAuthSettingsPanelController; not sure how
to get to that badboy and i got a bit lazy
Maniphest Tasks: T222
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Maniphest Tasks: T222
Differential Revision: 1166
Summary:
kill the tabs and make it a create button instead. pertinent notes:
* added a "Filter diffs" button to the form. optional, but i thought it
necessary with the new green button
* linked to Arcanist user guide on the create diff page. somewhat unrelated but
i think create diff will get more traffic now so linking to help seemed like a
reasonable add on here.
Test Plan:
viewed differential homepage
* clicked left hand filter elements. noted "Create Diff" button on filters
within user revisions and no button on filters within all revisions.
* entered another user into Select User UI and viewed their diffs via button and
pressing enter
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Differential Revision: 1157
Test Plan:
Login as admin, look at an open revision you don't own, you should be able to
choose '(Admin) Abandon Revision', the option should be on the bottom and should
abandon the revision after sending the comment.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1060
