1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-24 14:30:56 +01:00
Commit graph

2056 commits

Author SHA1 Message Date
epriestley
19572f53fd Don't consider accepting on behalf of valid-but-accepted reviewers to be a validation error
Summary:
Fixes T12757. Here's a simple repro for this:

  - Add a package you own as a reviewer to a revision you're reviewing.
  - Open two windows, select "Accept", don't submit the form.
  - Submit the form in window A.
  - Submit the fomr in window B.

Previously, window B would show an error, because we considered accepting on behalf of the package invalid, as the package had already accepted.

Instead, let repeat-accepts through without complaint.

Some product stuff:

  - We could roadblock users with a more narrow validation error message here instead, like "Package X has already been accepted.", but I think this would be more annoying than helpful.
  - If your accept has no effect (i.e., everything you're accepting for has already accepted) we currently just let it through. I think this is fine -- and a bit tricky to tailor -- but the ideal/consistent beavior is to do a "no effect" warning like "All the reviewers you're accepting for have already accepted.". This is sufficiently finnicky/rare (and probably not terribly useful/desiable in this specific case)that I'm just punting.

Test Plan: Did the flow above, got an "Accept" instead of a validation error.

Reviewers: chad, lvital

Reviewed By: chad, lvital

Subscribers: lvital

Maniphest Tasks: T12757

Differential Revision: https://secure.phabricator.com/D18019
2017-05-25 14:30:19 -07:00
Chad Little
00400ae6f9 Search and Replace calls to setShade
Summary: grep for setShade and update to setColor. Add deprecated warning.

Test Plan: Diffusion, Workboards, Maniphest, Project tags, tokenizer, uiexamples

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, O14 ATC Monitoring

Differential Revision: https://secure.phabricator.com/D17995
2017-05-22 18:59:53 +00:00
epriestley
af07600aaa Make Differential objective markers show a brighter "editing" state
Summary:
Ref T12733.

  - While editing a comment, show a pink star ({icon star, color=pink}) with a tooltip.
  - Slight UI tweaks, including draft comments getting an indigo pencil ({icon pencil, color=indigo}).

Test Plan: {F4968470}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12733

Differential Revision: https://secure.phabricator.com/D17977
2017-05-20 07:57:38 -07:00
epriestley
bdecff7d67 Show "objectives" UI only if prototypes are enabled
Summary: See D17955.

Test Plan: Loaded a revision, no longer saw annotations with prototypes off. Still saw annotations with prototypes on.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17983
2017-05-20 07:55:48 -07:00
epriestley
fdf00f6df4 Clean up some minor UI behaviors in Differential
Summary:
Minor UI tweaks:

  - Use the dynamic icon for each file (e.g., image, text), not a hard-coded icon.
  - Render the path (less important) in grey and the filename (more important) in black.

Test Plan: {F4966176}

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17957
2017-05-19 12:01:58 -07:00
epriestley
6c46f27d98 Add quest objectives to the minimap
Summary:
Add important objectives (like waygates and quest markers) to the minimap.

This also probably fixes @cspeckmim's bug with the {key @} keyboard shortcut.

Test Plan:
(This is probably easier to undestand if you `arc patch` + click around.)

{F4966037}

Reviewers: chad, amckinley

Reviewed By: chad

Subscribers: cspeckmim

Differential Revision: https://secure.phabricator.com/D17955
2017-05-19 12:01:01 -07:00
epriestley
fb9f3cc0b4 Restore the "buoyant" header in Differential
Summary:
Fixes T1591. This was removed long ago because it was a mess to implement and caused a bunch of weird issues, and also my tolerance for dealing with weird JS issues was much, much lower.

I have now survived the fires of JX.Scrollbar and would love to address 200 small nitpicks about obscure browser behaviors on Linux, so open the floodgates again.

A secondary goal here is to create room to add a global view state menu on the right, with 300 options like "hide all inlines", "hide done inlines", "hide collapsed inlines", "hide ghosts", "show ghosts", "enable filetree", "disable filetree", etc, etc. Not sure how much of this I'll actually do. I have one more experiment I want to try first.

Test Plan: {F4963294}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T1591

Differential Revision: https://secure.phabricator.com/D17945
2017-05-18 10:24:26 -07:00
epriestley
f78ce156f1 Restore "h" to hide or show files, and modernize file visibility toggling
Summary:
Ref T12616. This puts "h" back to collapse or expand the current file.

This removes some very complicated/messy code around following links in the table of contents and getting files auto-expanded. I suspect no one will miss this, but we can restore it if ayone notices.

Test Plan: Pressed "h" to collapse/expand a file. Also used the menu items.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17940
2017-05-18 10:21:37 -07:00
epriestley
9eb285f4aa Fix "left"/"right" changeset ID selection for synthetic deletions
Summary:
Fixes T8323. See that task for a description.

We were using `nonempty()`, but that rule doesn't cover synthetic deletions (file present in an earlier diff, but no longer present in the later diff).

Test Plan: Followed the steps in T8323, got a clean comment.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8323

Differential Revision: https://secure.phabricator.com/D17929
2017-05-17 08:54:13 -07:00
epriestley
51df02821b Move the "select a line range" inline code to DiffInline
Summary: Ref T12616. This makes line range selection use the new code, and removes the remainder of the old "hover a line number" / "select a line range" code.

Test Plan: Hovered line numbers; selected line ranges.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17927
2017-05-17 08:41:26 -07:00
epriestley
422eb9db83 Correct the generation of "<th />" IDs on left-hand-side of image changesets
Summary:
Fixes T7682. The left-hand-side "<th />" row did not generate with the correct ID.

(I couldn't reproduce the exact issue described in T7682, but hovering comments on either side now works properly for me.)

Test Plan: {F4962479}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7682

Differential Revision: https://secure.phabricator.com/D17926
2017-05-17 06:26:45 -07:00
epriestley
e4e91ebf6f In Differential, allow "r" to create comments and "R" to quote
Summary:
Ref T11401. Fixes T5232. Ref T12616.

Partly, this moves more code over to the new stuff.

This also allows "r" to work if you have code selected (not just comments). If you "reply" to code, you start a new comment.

You can "R" a comment to quote it. This just starts a new comment normally if you "R" a block of code. This is sort of a power-user version of "quote" since it seems like it probably doesn't really make sense to put it in the UI ever (maybe).

With the new click-to-select, you can click + "R" to reply-with-quote.

Test Plan: Used "r" and "R" to reply to comments and code.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616, T11401, T5232

Differential Revision: https://secure.phabricator.com/D17920
2017-05-16 17:37:54 -07:00
epriestley
8052ab84bf Remove "^" (Prev) and "V" (Next) actions on Differential inline comments
Summary:
Ref T12616. Fixes T12715. I suspect these are very rarely used. (I think you tried to get rid of them before but I pushed back since we couldn't really offer great alternatives at the time?)

Now that the code is in a better place:

  - Click an inline's header (just the colored part) to select it with the keyboard selection cursor.
  - Click again to deselect it.
  - You can use "n" and "p" to jump to comments, so "click + n" is the same as the old "V" action.
  - This also makes it easier to swap between keyboard and mouse workflows, since you can jump into things with the keyboard at any inline.

Also, make "Reply" render more consistently.

Test Plan:
  - Did all that stuff, things seemed to work OK.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12715, T12616

Differential Revision: https://secure.phabricator.com/D17908
2017-05-16 09:44:00 -07:00
epriestley
1b5a276a02 Add Differential keyboard shortcuts for "mark done" and "hide/show"
Summary:
Fixes T8130. Allows selected comments to be shown/hidden (with "q") or marked done/not-done (with "w").

(These key selections are because "qwer" are right next to each other on QWERTY keyboards, and now mean "hide, done, edit, reply".)

Also, allow "N" and "P" to do next/previous inline, including hidden inlines. This makes "q" to hide/show a little more powerful and a little easier to undo.

Test Plan: Used "q", "w", "N" and "P" to navigate and interact with comments.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8130

Differential Revision: https://secure.phabricator.com/D17906
2017-05-16 08:23:22 -07:00
epriestley
2fb1edfeb1 Restore the Differential "edit" and "reply" keyboard shortcuts
Summary: Ref T12616. This makes "edit" and "reply" work again.

Test Plan:
Used "e" and "r" to edit and reply.

Also used them in bogus ways and got useful UI feedback.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17895
2017-05-16 06:25:35 -07:00
epriestley
588a66c04d Move most Differetial keyboard shortcuts into DiffChangesetList
Summary: Ref T12616. This moves most keyboard shortcuts into DiffChangesetList. It breaks some shortcuts that I plan to restore later, noted in T12616 (toggle file, edit inline, reply to inline), since I think ripping them out now and rebuilding them in a little bit will make things much simpler.

Test Plan:
  - Used j, k, n, p, J, K shortcuts to navigate a revision.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17859
2017-05-16 06:24:42 -07:00
epriestley
4fd4ec3d27 Hide inlines one-by-one, instead of in a big group
Summary:
Ref T12616. Fixes T12153. Currently, when you hide inlines, they hide completely and turn into a little bubble on the previous line.

Instead, collapse them to a single line one-by-one. Narrowly, this fixes T12153.

In the future, I plan to make these changes so this feature makes more sense:

  - Introduce global "hide everything" states (T8909) so you can completely hide stuff if you want, and this represents more of a halfway state between "nuke it" and "view it".
  - Make the actual rendering better, so it says "epriestley: blah blah..." instead of just "..." -- and looks less dumb.

The real goal here is to introduce `DiffInline` and continue moving stuff from the tangled jungle of a million top-level behaviors to sensible smooth statefulness.

Test Plan:
  - Hid and revealed inlines in unified and two-up modes.
  - These look pretty junk for now:

{F4948659}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616, T12153

Differential Revision: https://secure.phabricator.com/D17861
2017-05-16 06:19:56 -07:00
epriestley
fe44e987fb Translate "Loading..." text in inline comments
Summary: Ref T12616. This cements the relationship between ChangesetList (parent container) and Changeset (child) and passes translations down so Changeset can use them to translate the text "Loading..."

Test Plan: Viewed loading changes.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17846
2017-05-16 06:19:01 -07:00
epriestley
64a54aac9d Merge "differential-dropdown-menus" behavior into DiffChangesetList
Summary: Ref T12616. This ends up being a little messy ("one giant function") and maybe I'll clean it up a bit later, but continue consolidating the wild jungle of behaviors into a smaller set of responsible objects.

Test Plan: Clicked all the menu options, saw them work properly. Grepped for removed methods.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17845
2017-05-16 06:18:26 -07:00
epriestley
11c5638832 Fix some error log issues with uninitialized commit/revision lists
Summary:
Fixes T12679. Reproduction steps appear to be:

  - As a logged-out user, view revision list or commit list.
  - Enable bucketing by action required.
  - Before patch: `foreach (null as ...)` causes error spew.
  - After patch: `foreach (array() as ...)` works great.

Test Plan:
  - Reproduced issue by following steps above in Differential (revisions) and Diffusion (audits/commits).
  - After patches, no more errors in the log.

Reviewers: chad, amckinley

Reviewed By: chad

Maniphest Tasks: T12679

Differential Revision: https://secure.phabricator.com/D17872
2017-05-14 13:28:02 -07:00
epriestley
b7c4f60e23 Only recognize "Fixes ..." in main revision content like the Summary or Test Plan
Summary:
Fixes T12642. Currently, writing "Fixes T..." in a comment gets picked up as a formal "fixes".

This is a bit confusing, and can also give you a "no effect" error if you "fixes ..." a task which is already "fixes"'d.

We could make the duplicate action a non-error, but just prevent the text from having an effect instead, which seems cleaner.

Test Plan:
  - Wrote "Fixes ..." in a summary, saw a "fixes" relationship established.
  - Wrote "Fixes ..." in a comment, got a "mention" instead.
  - `var_dump()`'d some stuff as a sanity check, looked reasonable.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12642

Differential Revision: https://secure.phabricator.com/D17805
2017-04-30 13:12:28 -07:00
epriestley
7a992b5488 When a package or project has been accepted or rejected, show who did it ("Accepted (by dog)")
Summary: Makes it more clear whose authority actions have been taken under.

Test Plan: {F4916376}

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17741
2017-04-20 13:07:08 -07:00
epriestley
af1d494d66 Fix an issue where rejecting reviewers weren't powerful enough
Summary:
Previously, "reject" and "reject older" were separate statuses. Now, they're both shades of "reject".

Set the "older reject" flag properly when we find a non-current reject.

Test Plan:
  - User A accepts a revision.
  - User B rejects it.
  - Author updates it.
  - Before patch: incorrectly transitions to "accepted" ("older" reject is ignored).
  - After patch: correctly transitions to "needs review".

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17653
2017-04-11 09:54:34 -07:00
epriestley
26d6096e0a When reviewing, always show "Accept" checkboxes for packages/projects, even if there's only one checkbox
Summary: Fixes T12533.

Test Plan: {F4853371}

Reviewers: chad, lvital

Reviewed By: lvital

Maniphest Tasks: T12533

Differential Revision: https://secure.phabricator.com/D17652
2017-04-10 17:28:02 -07:00
epriestley
3a3626834e Replace Remarkup calls to PhabricatorHash::digest() with SHA256
Summary:
Ref T12509. Many of the calls to HMAC+SHA1 are just to compute cachekeys for remarkup objects.

Make these use HMAC+SHA256 instead. There is no downside to swapping these since they just cause a cache miss in the worst case.

I also plan to get rid of `PhabricatorMarkupInterface` eventually, but this doesn't go that far.

Test Plan: Browsed some different types of documents (tasks, legalpad documents, phame blogs / posts, pholio mocks, etc).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12509

Differential Revision: https://secure.phabricator.com/D17631
2017-04-06 15:43:18 -07:00
epriestley
f1eeaaf59f Fix scope of "Accept" when you don't check all the "Force Accept" boxes
Summary:
Ref T12272. I wrote this correctly, then broke it by adding the simplification which treats "accept the defaults" as "accept everything".

This simplification lets us render "epriestley accepted this revision." instead of "epriestley accepted this revision onbehalf of: long, list, of, every, default, reviewer, they, have, authority, over." so it's a good thing, but make it only affect the reviewers it's supposed to affect.

Test Plan:
  - Did an accept with a force-accept available but unchecked.
  - Before patch: incorrectly accepted all possible reviewers.
  - After patch: accepted only checked reviewers.
  - Also checked the force-accept box, accepted, got a proper force-accept.

Reviewers: chad, lvital

Reviewed By: lvital

Maniphest Tasks: T12272

Differential Revision: https://secure.phabricator.com/D17634
2017-04-06 15:03:32 -07:00
epriestley
cefbdbcffe Provide a "Reviewers" attachment to "differential.revision.search"
Summary: Allow API callers to retrieve reviewer information via a new "reviewers" attachment.

Test Plan: {F4675784}

Reviewers: chad, lvital

Reviewed By: lvital

Subscribers: lvital

Differential Revision: https://secure.phabricator.com/D17633
2017-04-06 14:46:39 -07:00
epriestley
f70ff34d97 Fix a copy/paste typo with sticky accept
The root issue here is actually just that I cherry-picked stable locally
but did not push it. However, this is a minor issue I also caught while
double-checking things.

Auditors: chad
2017-04-04 18:33:59 -07:00
epriestley
873b39be82 Remove PhabricatorFile::buildFromFileDataOrHash()
Summary:
Ref T12464. This is a very old method which can return an existing file instead of creating a new one, if there's some existing file with the same content.

In the best case this is a bad idea. This being somewhat reasonable predates policies, temporary files, etc. Modern methods like `newFromFileData()` do this right: they share underlying data in storage, but not the actual `File` records.

Specifically, this is the case where we get into trouble:

  - I upload a private file with content "X".
  - You somehow generate a file with the same content by, say, viewing a raw diff in Differential.
  - If the diff had the same content, you get my file, but you don't have permission to see it or whatever so everything breaks and is terrible.

Just get rid of this.

Test Plan:
  - Generated an SSH key.
  - Viewed a raw diff in Differential.
  - (Did not test Phragment.)

Reviewers: chad

Reviewed By: chad

Subscribers: hach-que

Maniphest Tasks: T12464

Differential Revision: https://secure.phabricator.com/D17617
2017-04-04 16:18:00 -07:00
epriestley
45b386596e Make the Files "TTL" API more structured
Summary:
Ref T11357. When creating a file, callers can currently specify a `ttl`. However, it isn't unambiguous what you're supposed to pass, and some callers get it wrong.

For example, to mean "this file expires in 60 minutes", you might pass either of these:

  - `time() + phutil_units('60 minutes in seconds')`
  - `phutil_units('60 minutes in seconds')`

The former means "60 minutes from now". The latter means "1 AM, January 1, 1970". In practice, because the GC normally runs only once every four hours (at least, until recently), and all the bad TTLs are cases where files are normally accessed immediately, these 1970 TTLs didn't cause any real problems.

Split `ttl` into `ttl.relative` and `ttl.absolute`, and make sure the values are sane. Then correct all callers, and simplify out the `time()` calls where possible to make switching to `PhabricatorTime` easier.

Test Plan:
- Generated an SSH keypair.
- Viewed a changeset.
- Viewed a raw diff.
- Viewed a commit's file data.
- Viewed a temporary file's details, saw expiration date and relative time.
- Ran unit tests.
- (Didn't really test Phragment.)

Reviewers: chad

Reviewed By: chad

Subscribers: hach-que

Maniphest Tasks: T11357

Differential Revision: https://secure.phabricator.com/D17616
2017-04-04 16:16:28 -07:00
epriestley
9ebb5f8cda Don't downgrade accepts on update (fix "sticky accept")
Summary:
Fixes T12496. Sticky accept was accidentally impacted by the "void" changes in D17566.

Instead, don't always downgrade all accepts/rejects: on update, we only want to downgrade accepts.

Test Plan:
  - With sticky accept off, updated an accepted revision: new state is "needs review".
  - With sticky accept on, updated an accepted revision: new state is "accepted" (sticky accept working correctly).
  - Did "reject" + "request review" to make sure that still works, worked fine.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12496

Differential Revision: https://secure.phabricator.com/D17605
2017-04-03 09:55:22 -07:00
epriestley
163e1ec442 Expose the commit/task/revision relationship edges to "edge.search"
Summary: Fixes T12480.

Test Plan: {F4465908}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12480

Differential Revision: https://secure.phabricator.com/D17604
2017-04-02 19:49:55 -07:00
epriestley
2fbc9a52da Allow users to "Force accept" package reviews if they own a more general package
Summary:
Ref T12272. If you own a package which owns "/", this allows you to force-accept package reviews for packages which own sub-paths, like "/src/adventure/".

The default UI looks something like this:

```
[X] Accept as epriestley
[X] Accept as Root Package
[ ] Force accept as Adventure Package
```

By default, force-accepts are not selected.

(I may do some UI cleanup and/or annotate "because you own X" in the future and/or mark these accepts specially in some way, particularly if this proves confusing along whatever dimension.)

Test Plan: {F4314747}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12272

Differential Revision: https://secure.phabricator.com/D17569
2017-03-28 11:51:40 -07:00
epriestley
ddc02ce420 When voiding "Accept" reviews, also void "Reject" reviews
Summary: Ref T10967. This change is similar to D17566, but for rejects.

Test Plan:
  - Create a revision as A, with reviewer B.
  - Reject as B.
  - Request review as A.
  - Before patch: stuck in "rejected".
  - After patch: transitions back to "needs review".

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17568
2017-03-28 11:51:06 -07:00
epriestley
415ad78484 Remove old code for "Request Review" action from Differential
Summary: Ref T10967. This moves all remaining "request review" pathways (just `differential.createcomment`) to the new code, and removes the old action.

Test Plan: Requested review on a revision, `grep`'d for the action constant.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17567
2017-03-28 11:50:40 -07:00
epriestley
aea46e55da Fix an issue where "Request Review" of a fully-accepted revision would transition to "Accepted"
Summary:
Ref T10967. This is explained in more detail in T10967#217125

When an author does "Request Review" on an accepted revision, void (in the sense of "cancel out", like a bank check) any "accepted" reviewers on the current diff.

Test Plan:
  - Create a revision with author A and reviewer B.
  - Accept as B.
  - "Request Review" as A.
  - (With sticky accepts enabled.)
  - Before patch: revision swithced back to "accepted".
  - After patch: the earlier review is "voided" by te "Request Review", and the revision switches to "Review Requested".

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17566
2017-03-28 11:50:15 -07:00
epriestley
24b6c7d718 Allow users to resign if they have authority over any reviewer
Summary:
Ref T11050. The old rule was "you can only resign if you're a reviewer".

With the new behavior of "resign", the rule should be "you can resign if you're a reviewer, or you have authority over any reviewer". Make it so.

Also fixes T12446. I don't know how to reproduce that but I'm pretty sure this'll fix it?

Test Plan:
  - Could not resign from a revision with no authority/reviewer.
  - Resigned from a revision with myself as a reviewer.
  - Resigned from a revision with a package I owned as a reviewer.
  - Could not resign from a revision I had already resigned from.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12446, T11050

Differential Revision: https://secure.phabricator.com/D17558
2017-03-24 13:14:47 -07:00
Chad Little
ffab52f17e Restrict Differential buckets to just ApplicationSearch views
Summary: Ref T9363, If we're in a dashboard panel, only show buckets with data, or a fallback if nothing exists.

Test Plan: Test 'active revisions' panel in a dashboard and in Differential.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T9363

Differential Revision: https://secure.phabricator.com/D17544
2017-03-23 12:09:44 -07:00
epriestley
aa91dc992e Record which user accepted on behalf of packages/owners reviewers
Summary:
Ref T12271. Don't do anything with this yet, but store who accepted/rejected/whatever on behalf of reviewers.

In the future, we could use this to render stuff like "Blessed Committers (accepted by epriestley)" or whatever. I don't know that this is necessarily super useful, but it's easy to track, seems likely to be useful, and would be a gigantic pain to backfill later if we decide we want it.

Test Plan: Accepted/rejected a revision, saw reviewers update appropriately.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12271

Differential Revision: https://secure.phabricator.com/D17537
2017-03-22 14:26:37 -07:00
epriestley
fab37aa4e3 When accepting revisions, allow users to accept on behalf of a subset of reviewers
Summary:
Ref T12271. Currenty, when you "Accept" a revision, you always accept it for all reviewers you have authority over.

There are some situations where communication can be more clear if users can accept as only themselves, or for only some packages, etc. T12271 discusses some of these use cases in more depth.

Instead of making "Accept" a blanket action, default it to doing what it does now but let the user uncheck reviewers.

In cases where project/package reviewers aren't in use, this doesn't change anything.

For now, "reject" still acts the old way (reject everything). We could make that use checkboxes too, but I'm not sure there's as much of a use case for it, and I generally want users who are blocking stuff to have more direct accountability in a product sense.

Test Plan:
  - Accepted normally.
  - Accepted a subset.
  - Tried to accept none.
  - Tried to accept bogus reviewers.
  - Accepted with myself not a reviewer
  - Accepted with only one reviewer (just got normal "this will be accepted" text).

{F4251255}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12271

Differential Revision: https://secure.phabricator.com/D17533
2017-03-22 14:25:04 -07:00
epriestley
e1ee8ba428 Fix a bad getStatus() call which is fataling during Herald rule evaluation
Summary: Hit this while `arc diff`'ing something which is triggering 2+ rules which add reviewers, I think.

Test Plan: Dug this out of a production stack trace; will push and `arc diff` again.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17534
2017-03-22 10:03:38 -07:00
epriestley
3e7b63aa73 Add a <reviewer, revision> key to the reviewers table
Summary:
Ref T10967. I'm not 100% sure we need this, but the old edge table had it and I recall an issue long ago where not having this key left us with a bad query plan.

Our data doesn't really provide a way to test this key (we have many revisions and few reviewers, so the query planner always uses revision keys), and building a convincing test case would take a while (lipsum needs some improvements to add reviewers). But in the worst case this key is mostly useless and wastes a few MB of disk space, which isn't a big deal.

So I can't conclusively prove that this key does anything to the dashboard query, but the migration removed it and I'm more comfortable keeping it so I'm not worried about breaking stuff.

At the very least, MySQL does select this key in the query plan when I do a "Reviewers:" query explicitly so it isn't //useless//.

Test Plan: Ran `bin/storage upgrade`, ran dashboard query, the query plan didn't get any worse.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17532
2017-03-22 09:51:06 -07:00
epriestley
8913552970 Store "resigned" as an explicit reviewer state
Summary:
Fixes T11050. Today, when a user resigns, we just delete the record of them ever being a reviewer.

However, this means you have no way to say "I don't care about this and don't want to see it on my dashboard" if you are a member of any project or package reviewers.

Instead, store "resigned" as a distinct state from "not a reviewer", and treat it a little differently in the UI:

  - On the bucketing screen, discard revisions any responsible user has resigned from.
  - On the main `/Dxxx` page, show these users as resigned explicitly (we could just hide them, too, but I think this is good to start with).
  - In the query, don't treat a "resigned" state as a real "reviewer" (this change happened earlier, in D17517).
  - When resigning, write a "resigned" state instead of deleting the row.
  - When editing a list of reviewers, I'm still treating this reviewer as a reviewer and not special casing it. I think that's sufficiently clear but we could tailor this behavior later.

Test Plan:
  - Resigned from a revision.
  - Saw "Resigned" in reviewers list.
  - Saw revision disappear from my dashboard.
  - Edited revision, saw user still appear as an editable reviewer. Saved revision, saw no weird side effects.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11050

Differential Revision: https://secure.phabricator.com/D17531
2017-03-22 09:50:50 -07:00
epriestley
0ceab7d36f Rename "getReviewerStatus()" to "getReviewers()"
Summary:
Ref T10967. Improves some method names:

  - `Revision->getReviewerStatus()` -> `Revision->getReviewers()`
  - `Revision->attachReviewerStatus()` -> `Revision->attachReviewers()`
  - `Reviewer->getStatus()` -> `Reviewer->getReviewerStatus()` (this is mostly to make this more greppable)

Test Plan:
  - bunch o' `grep`
  - Browsed around.
  - If I missed anything, it should fatal in an obvious way. We have a lot of other `getStatus()` calls and it's hard to be sure I got them all.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17522
2017-03-20 17:11:40 -07:00
epriestley
a15df4f8d5 Rename "needReviewerStatus()" into "needReviewers()"
Summary: Ref T10967. The old name was because we had a `getReviewers()` tied to `needRelationships()`, rename this method to use a simpler and more clear name.

Test Plan: `grep`, browsed around.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17519
2017-03-20 16:46:16 -07:00
epriestley
d179d0150c Remove obsolete "relationships" code from Differential
Summary:
Ref T10967. There have been two different ways to load reviewers for a while: `needReviewerStatus()` and `needRelationships()`.

The `needRelationships()` stuff was a false start along time ago that didn't really go anywhere. I believe the idea was that we might want to load several different types of edges (subscribers, reviewers, etc) on lots of different types of objects. However, all that stuff pretty much ended up modularizing so that main `Query` classes did not need to know about it, so `needRelationships()` never got generalized or went anywhere.

A handful of things still use it, but get rid of them: they should either `needReviewerStatus()` to get reviewer info, or the ~3 callsites that care about subscribers can just load them directly.

Test Plan:
  - Grepped for removed methods (`needRelationships()`, `getReviewers()`, `getCCPHIDs()`, etc).
  - Browsed Diffusion, Differential.
  - Called `differential.query`.

It's possible I missed some stuff, but it should mostly show up as super obvious fatals ("call needReviewerStatus() before getReviewerStatus()!").

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17518
2017-03-20 16:45:48 -07:00
epriestley
dccd799b1b Move many "reviewers" readers to new storage
Summary:
Ref T10967.

When we query for revisions with particular reviewers, use the new table to drive the query.

When we load revisions for use in the application, also use the new table to drive the query.

This doesn't convert everything: there's some old `loadRelationships()` stuff still using the old table. But this moves the major stuff over.

(This also changes the icon for "commented" from a question mark to a speech bubble.)

Test Plan:
  - Viewed revision lists and detail views on old and new code, saw identical outcomes.
  - Updated revisions, accepted/rejected/commented on revisions.
  - Hit the "Accepted Older" and "Commented Older" states by taking an action and then updating.
  - Grepped for removed methods (like `getEdgeData()` and `getDiffID()`).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17517
2017-03-20 16:45:28 -07:00
epriestley
794b456530 Store "last comment" and "last action" diffs on reviewers
Summary:
Ref T10967. We have a "commented" state to help reviewers get a better sense of who is part of a discussion, and a "last action" state to help distinguish between "accept" and "accepted an older version", for the purposes of sticky accepts and as a UI hint.

Currently, these are first-class states, partly beacuse we were somewhat limited in what we could do with edges. However, a more flexible way to represent them is as flags separate from the primary state flag.

In the new storage, write them as separate state information: `lastActionDiffPHID` stores the Diff PHID of the last review action (accept, reject, etc). `lastCommentDiffPHID` stores the Diff PHID of the last comment (top-level or inline).

Test Plan: Applied storage changes, commented and acted on a revision. Saw appropriate state reflected in the database.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17514
2017-03-20 16:44:05 -07:00
epriestley
77b3efafbd Use ModularTransactions for accept/reject/resign in "differential.createcomment"
Summary:
Ref T10967. `differential.createcomment` is a frozen API method which has been obsoleted by `differential.revision.edit`.

It is the only remaining way to apply an "accept", "reject", or "resign" action using the old "ACTION" code.

Instead of using the old code, sneakly apply a new type of transaction in these cases instead.

Then, remove all the remaining old code for this stuff on the write pathways.

Test Plan:
  - Used "differential.createcomment" to accept, reject, and resign from a revision.
  - Grepped for all removed ACTION_X constants, found them only in rendering code.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17513
2017-03-20 16:43:43 -07:00
epriestley
a9cbbf3e5e Apply Owners reviewers using ModularTransactions
Summary: Ref T10967. See that task for some discussion. This lets us do double writes on this pathway.

Test Plan: Set an Owners package to auto-review. Created revisions which triggered it: one with no reviewers (autoreview added); one with the package as a blocking reviewer explicitly (no automatic stuff happened, as expected).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17512
2017-03-20 16:43:17 -07:00
epriestley
216052baf9 Apply reviewer changes from Herald via ModularTransactions
Summary:
Ref T10967. This converts the reviewer update action in Herald from an older edge write to a newer ModularTransactions write.

The major value from this is that we get a double-write to the new reviewers table.

Test Plan:
  - Wrote a Herald rule to add a reviewer and a blocking reviewer.
  - Saw them added properly to a revision with: no reviewers; both as blocking; A as blocking, B as nonblocking; A as nonblocking, B as blocking.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17511
2017-03-20 16:42:54 -07:00
epriestley
688c120f9f Provide PhabricatorEnv::isSelfURI to test if a URI points at the current install
Summary:
Ref T5378. This repackages an existing check to see if a URI is a URI for the current install into a more reasonable form.

In an upcoming change, I'll use this new check to test whether `http://example.whatever.com/T123` is a link to a task on the current install or not.

Test Plan: This stuff has good test coverage already; added some more.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T5378

Differential Revision: https://secure.phabricator.com/D17502
2017-03-17 16:44:53 -07:00
epriestley
7626ec0ce1 Correct an issue where "View Raw File" in Differential generated a file with overbroad permissions
Summary:
Via HackerOne. When you view a raw file in Differential, we currently generate a permanent file with default permissions. This may be incorrect: default permissions may be broader than the diff's permissions.

The other three methods of downloading/viewing raw files ("Download" in Diffusion and Differential, "View Raw" in Diffusion and Differential) already apply policies correctly and generate temporary files. However, this workflow was missed when other workflows were updated.

Beyond updating the workflow, delete any files we've generated in the past. This wipes the slate clean on any security issues and frees up a little disk space.

Test Plan:
  - Ran migration script, saw existing files get purged.
  - Did "View Raw File", got a new file.
  - Verified that the file was temporary and properly attached to the diff, with "NO ONE" permissions.
  - Double-checked that Diffusion already runs policy logic correctly and applies appropriate policies.
  - Double-checked that "Download Raw Diff" in Differential already runs policy logic correctly.
  - Double-chekced that "Download Raw Diff" in Diffusion already runs policy logic correctly.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17504
2017-03-16 09:51:48 -07:00
epriestley
251ee9b660 Add dedicated "reviewers" storage to Differential and do double writes
Summary:
Ref T10967. This is an incremental step toward removing "reviewers" back to a dedicated storage table so we can handle changes like T11050.

This adds the storage table, and starts doing double writes to it (so new or updated reviewers write to both the old edge table and the new "reviewers" table).

Then we can do a migration, swap readers over one at a time, and eventually remove the old write and old storage and then implement new features.

This change has no user-facing impact, it just causes us to write new data to two places instead of one.

This is not completely exhaustive: the Herald "Add Reviewers" action is still doing a manual EDGE transaction. I'll clean that up next and do another pass to look for anything else I missed.

This is also a bit copy/pastey for now but the logic around "RESIGN" is a little different in the two cases until T11050. I'll unify it in future changes.

Test Plan:
  - Did a no-op edit.
  - Did a no-op comment.
  - Added reviewers.
  - Removed reviewers.
  - Accepted and rejected revisions.

After all of these edits, did a `SELECT * FROM differential_reviewer` manually and saw consistent-looking rows in the database.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17495
2017-03-14 11:51:51 -07:00
epriestley
0a0ac1302f Prevent users from taking "edit"-like actions via comment forms if they don't have edit permission
Summary:
Ref T12335. Fixes T11207. Edit-like interactions which are not performed via "Edit <object>" are a bit of a grey area, policy-wise.

For example, you can correctly do these things to an object you can't edit:

  - Comment on it.
  - Award tokens.
  - Subscribe or unsubscribe.
  - Subscribe other users by mentioning them.
  - Perform review.
  - Perform audit.
  - (Maybe some other stuff.)

These behaviors are all desirable and correct. But, particularly now that we offer stacked actions, you can do a bunch of other stuff which you shouldn't really be able to, like changing the status and priority of tasks you can't edit, as long as you submit the change via the comment form.

(Before the advent of stacked actions there were fewer things you could do via the comment form, and more of them were very "grey area", especially since "Change Subscribers" was just "Add Subscribers", which you can do via mentions.)

This isn't too much of a problem in practice because we won't //show// you those actions if the edit form you'd end up on doesn't have those fields. So on intalls like ours where we've created simple + advanced flows, users who shouldn't be changing task priorities generally don't see an option to do so, even though they technically could if they mucked with the HTML.

Change this behavior to be more strict: unless an action explicitly says that it doesn't need edit permission (comment, review, audit) don't show it to users who don't have edit permission and don't let them take the action.

Test Plan:
  - As a user who could not edit a task, tried to change status via comment form; received policy exception.
  - As a user who could not edit a task, viewed a comment form: no actions available (just "comment").
  - As a user who could not edit a revision, viewed a revision form: only "review" actions available (accept, resign, etc).
  - Viewed a commit form but these are kind of moot because there's no separate edit permission.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12335, T11207

Differential Revision: https://secure.phabricator.com/D17452
2017-03-02 16:56:57 -08:00
epriestley
6c21646b5f Put revisions waiting on other reviewers in their own bucket
Summary: Fixes T12323. See that task for discussion.

Test Plan: {F3424441}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12323

Differential Revision: https://secure.phabricator.com/D17425
2017-02-27 10:47:15 -08:00
epriestley
c5fa7421c2 Allow commits to be queried by repository using the tagged(...) typehaead function
Summary:
Fixes T12322. Allows you to search for commits using the `tagged(...)` repository function, so you can find "any commmit in any repository tagged with android" or similar.

I moved the function from Differential (which was the application using it) to Diffusion (which is more accurately the application which provides it).

I fixed a bug where searching for `tagged(xyz)` would have no effect (constraint was ignored) if there were no repositories tagged with "xyz". The fix isn't perfectly clean, but should work properly for the moment.

Test Plan:
  - Searched with `tagged(...)` in Diffusion and Differential.
  - Searched by repository.
  - Searched with `tagged(...)` for a project with no tagged repositories.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12322

Differential Revision: https://secure.phabricator.com/D17426
2017-02-27 10:46:55 -08:00
epriestley
99bcf5f112 Make bin/lipsum generate hanldle generator keys and arguments more clearly
Summary:
Ref T12319. Currently, `bin/lipsum` uses substring matches against human-readable text to chose which objects to generate.

Instead:

  - Use separate selector keys which are guaranteed to be unique.
  - When a match is exact, select only that generator.
  - When a match is ambiguous, fail and warn the user.

Test Plan: Generated several types of objects, tried to generate ambiguous objects like "e".

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12319

Differential Revision: https://secure.phabricator.com/D17420
2017-02-27 09:09:28 -08:00
epriestley
1b2c047ce0 Correct spelling of "phabrictor" in Lipsum and elsewhere
Summary: Ref T12319. The product name is misspelled in some methods, and a few places in the documentation.

Test Plan: `grep`

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12319

Differential Revision: https://secure.phabricator.com/D17419
2017-02-27 09:09:13 -08:00
epriestley
84aff44bcd Add a "Red/Green Colorblind" accessibility mode, make all web UIs and email respect it
Summary:
Fixes T12172. Fixes T12060. This allows runtime code building CSS for mail to read CSS variables, then makes all the code do that.

It reverts the non-colorblind red/green to the colors in use before T12060, which seem better for non-colorblind users since no one really complained?

Test Plan:
  - Viewed code diffs in Web UI.
  - Viewed prose diffs in Web UI.
  - Viewed code diffs in email.
  - Viewed prose diffs in email.

All modes respected the accessibility color scheme.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12172, T12060

Differential Revision: https://secure.phabricator.com/D17269
2017-02-23 10:57:39 -08:00
Jakub Vrana
a778151f28 Fix errors found by PHPStan
Test Plan: Ran `phpstan analyze -a autoload.php phabricator/src`.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, hach-que

Differential Revision: https://secure.phabricator.com/D17371
2017-02-17 10:10:15 +00:00
epriestley
fd0591e168 Restore "Auditor" as an alias for the commit message field "Auditors"
Summary:
Fixes T12197. I //think// this field was never recognized by Differential (it doesn't appear in D17070, but maybe that isn't the right change).

It was recognized by the ad-hoc regular expression which I replaced with a formal parser in D17262.

Allow the former parser to accept "Auditor" as an alias for "Auditors".

Test Plan: Committed a change with `Auditor: dog`, saw the audit trigger correctly in the web UI.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12197

Differential Revision: https://secure.phabricator.com/D17306
2017-02-03 09:14:32 -08:00
epriestley
9b92e56dfc Don't link "Dxxx" on Differential revision pages
Summary: Ref T12027. See T12043 for discussion.

Test Plan: Double-clicked "Dxxx" to select it.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12027

Differential Revision: https://secure.phabricator.com/D17283
2017-01-31 18:55:22 -08:00
epriestley
bd9e54b621 Navigage Buildkite builds with more nuance
Summary:
Ref T12173.

  - If we want to fetch a tag, Buildkite needs it as a "branch" (this means more like "ref to fetch").
  - The API gets upset if we pass "refs/tags/...", so just pass the tag name without the prefix, which works.
  - Do a better job with commits and pass a real branch to fetch.

Test Plan:
  - Built a commit with Buildkite.
  - Build a revision with Buildkite.

Reviewers: chad

Reviewed By: chad

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T12173

Differential Revision: https://secure.phabricator.com/D17282
2017-01-31 17:26:45 -08:00
epriestley
bc41c3f5a5 Use DifferentialCommitMessageParser and Modular Transactions to implement "Auditors: ..."
Summary:
Ref T10978. Updates how we implement "Auditors: ..." in commit messages:

  - Use the same parsing code as everything else.
    - (Also: parse package names.)
  - Use the new transaction code.

Also, fix some UI strings.

Test Plan: Used `bin/repository reparse --herald <commit>` to re-run this code on commits with various messages (valid Auditors, invalid Auditors, no Auditors). Saw appropriate auditors added in the UI.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10978

Differential Revision: https://secure.phabricator.com/D17262
2017-01-30 15:23:05 -08:00
Sébastien Santoro
e16080ce7e Fix typo in DifferentialRevisionCommandeerTransaction
Test Plan: Check at /applications/mailcommands/PhabricatorDifferentialApplication/revision/

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D17265
2017-01-30 12:23:07 -08:00
epriestley
df939f1337 Fix two issues with embedding other fields inside "Summary" or "Test Plan" in Differential with the web UI
Summary:
Ref T11114. Converting to EditEngine caused us to stop running this validation, since these fields no longer subclass this parent. Restore the validation.

Also, make sure we check the //first// line of the value, too. After the change to make "Tests: xyz" a valid title, you could write silly summaries / test plans and escape the check if the first line was bogus.

Test Plan: {F2493228}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17248
2017-01-25 13:07:30 -08:00
Chad Little
20d1bb8fdf Remove counts from home navigation
Summary: Ref T12136. This just yanks the band-aid off. Fundamentally these were useful well before Dashboards and advanced bucketing, but not so much any more. They also have some performance hit.

Test Plan: Add some tasks and diffs onto a new instance, see there is no count on the home menu bar.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12136

Differential Revision: https://secure.phabricator.com/D17238
2017-01-21 13:55:40 -08:00
epriestley
269dd81f91 Allow users to re-accept or re-reject a revision if they have authority over package/project reviewers not yet in the target state
Summary:
To set this up:

  - alice accepts a revision.
  - Something adds a package or project she has authority over as a reviewer.
  - Because alice has already accepted, she can not re-accept, but she should be able to (in order to accept on behalf of the new project or package).

Test Plan:
  - Created a revision.
  - Accepted as user "dog".
  - Added "dog project".
  - Re-accepted.
  - Could not three-accept.
  - Removed "dog project.
  - Rejected.
  - Added "dog project".
  - Re-rejected.
  - Could not three-reject.

Reviewers: chad, eadler

Reviewed By: chad, eadler

Differential Revision: https://secure.phabricator.com/D17226
2017-01-18 13:16:01 -08:00
epriestley
903e37a21b Show yellow "draft" bubble in Audit
Summary: Fixes T6660. Uses the new stuff in Audit to build an EditEngine-aware icon.

Test Plan: {F2364304}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T6660

Differential Revision: https://secure.phabricator.com/D17208
2017-01-16 10:28:59 -08:00
epriestley
62cf4e6b95 Remove some remnants of the old ways commit mesage fields worked from Differential
Summary:
Ref T11114. Ref T12085. I missed a few pieces of cleanup when moving all this stuff over.

In particular, load all fields which use Custom Field storage before doing commit-message-related stuff, instead of just the ones that claim they appear on commit messages.

Test Plan: Edited revisions and made API calls without apparent issues. See followup on T12085, shortly.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12085, T11114

Differential Revision: https://secure.phabricator.com/D17207
2017-01-13 15:29:07 -08:00
epriestley
7276af6a81 Make yellow "draft" bubbles more generic
Summary:
Fixes T12095. Ref T6660. The old code for this was specific to Differential, using the `DifferentialDraft` table.

Instead, make the `EditEngine` / `VersionedDraft` code create and remove a `<objectPHID, authorPHID>` edge when a particular author creates drafts.

Some applications have drafts beyond `VersionedDrafts`, notably inline comments. Before writing "yes, draft" or "no, no draft", ask the object if it has any custom draft stuff we need to know about.

This should fix all the yellow bubble bugs I created in T11114 and allow us to bring the feature to Audit fairly easily.

Test Plan: Created and deleted comments and inlines, reloading the list view after each change. Couldn't find a way to break the list view anymore.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12095, T6660

Differential Revision: https://secure.phabricator.com/D17205
2017-01-13 09:02:19 -08:00
epriestley
e684794bf3 Get "Create Revision" out of Quick Create menu for now
Summary:
Ref T12098.

We have two methods (`supportsEditEngineConfiguration()` and `isEngineConfigurable()`) which sort of do the same thing and probably should be merged.

For now, just swap which one we override to get "Create Revision" out of the Quick Create menu.

Test Plan: No more "Create Revision" in Quick Create menu.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12098

Differential Revision: https://secure.phabricator.com/D17204
2017-01-13 09:00:44 -08:00
epriestley
45c740ac98 Render revision and audit state icons in Maniphest
Summary:
Fixes T7076. This could probably use some tweaking but should get the basics in place.

This shows overall object state (e.g., "Needs Review"), not individual viewer state (e.g., "you need to review this"). After the bucketing changes it seems like we're mostly in a reasonable place on showing global state instead of viewer state. This makes the overall change much easier than it might otherwise have been.

Test Plan: {F2351867}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7076

Differential Revision: https://secure.phabricator.com/D17193
2017-01-12 13:23:13 -08:00
epriestley
7d3d022407 Restore "[Action]" mail subject lines to Differential/Diffusion
Summary: Ref T11114. Ref T10978. These hadn't made it over to EditEngine yet.

Test Plan:
  - Took various actions on revisions and commits.
  - Used `bin/mail show-outbound --id ...` to examine the "Vary Subject", saw it properly generate "[Accepted]", "[Resigned]", etc.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114, T10978

Differential Revision: https://secure.phabricator.com/D17191
2017-01-12 11:44:24 -08:00
epriestley
b5722a9963 Use EditEngine stacked comments in Diffusion
Summary: Ref T10978. Ref T8739. Fixes T10446. Converts Diffusion to modern comment/preview code, like Differential.

Test Plan: {F2342933}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10978, T10446, T8739

Differential Revision: https://secure.phabricator.com/D17183
2017-01-11 14:46:48 -08:00
epriestley
52d563f8b8 Make differential.querydiffs more liberal about arguments
Summary:
Fixes T12092. D17164 made `DiffQuery` more strict about arguments using modern conventions, but `differential.querydiffs` uses bizarre ancient conventions.

Give it more modern conventions instead.

Test Plan: Made a `querydiffs` call with only revision IDs.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12092

Differential Revision: https://secure.phabricator.com/D17172
2017-01-10 13:47:38 -08:00
epriestley
00e2755eab Provide tailored strings for revision creation
Summary: See D17169. Ref T11114.

Test Plan: {F2333825}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17170
2017-01-10 12:45:36 -08:00
epriestley
fda83094ac Restore missing behavior for Differential keyboard navigation
Summary: Fixes T12086. This got dropped by accident while cleaning up haunting.

Test Plan: Loaed a revision, hit "?", hit n/j/p/etc

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12086

Differential Revision: https://secure.phabricator.com/D17166
2017-01-09 12:57:49 -08:00
epriestley
2dfe79cfc7 When updating revisions in response to commits, reuse previously generated diffs
Summary:
Fixes T10968. In rare situations, we can generate a diff, then hit an error which causes this update to fail.

When it does, we tend to get stuck in a loop creating diffs, which can fill the database up with garbage. We saw this once in the Phacility cluster, and one instance hit it, too.

Instead: when we create a diff, keep track of which commit we generated it from. The next time through, reuse it if we already built it.

Test Plan:
  - Used `bin/differential attach-commit <commit> <revision>` to hit this code.
  - Simulated a filesystem write failure, saw the diff get reused.
  - Also did a normal update, which worked properly.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10968

Differential Revision: https://secure.phabricator.com/D17164
2017-01-09 12:13:44 -08:00
epriestley
425deeb523 Fix an issue which could prevent blocking reviewers from being removed from revisions
Summary: Ref T11114. After evaluating typeahead tokens, we could process blocking reviewer removals incorrectly: we may get structures back.

Test Plan: Removed blocking reviewers from the web UI.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17163
2017-01-09 08:41:46 -08:00
epriestley
aa6e788f36 Mark "v3" API methods as stable; mark obsoleted methods as "Frozen"
Summary:
Ref T12074. The "v3" API methods (`*.search`, `*.edit`) are currently marked as "unstable", but they're pretty stable and essentially all new code should be using them.

Although these methods are seeing some changes, almost all changes are additive (support for new constraints or attachemnts) and do not break backward compatibility. We have no major, compatibility-breaking changes planned.

I don't want to mark the older methods "deprecated" yet since `arc` still uses a lot of them and there are some capabilities not yet available on the v3 methods, but introduce a new "frozen" status with pointers to the new methods.

Overall, this should gently push users toward the newer methods.

Test Plan: {F2325323}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12074

Differential Revision: https://secure.phabricator.com/D17158
2017-01-09 07:16:27 -08:00
epriestley
1f2306999b Fix a case where "Accept + Comment" would ignore the "Accept"
Summary:
Ref T11114. When you comment, we try to upgrade your review status to "commented".

This can conflict with upgrading it to "accepted" or "rejected", or removing it entirely.

For now, just avoid making this update. After T10967, I expect "you commented" to be orthogonal to accepted/rejected so it should stop conflicting on its own.

Test Plan:
  - As an "added" reviewer, accepted a revision with a comment in the same transaction.
  - Before patch: accept didn't stick.
  - After patch: accept sticks.

This may be somewhat magical/order-dependent but I was able to reproduce it locally.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17146
2017-01-05 11:30:20 -08:00
Chad Little
96fbf37dcc Bring up contrast on light green / red diffs
Summary: Minor color saturation here, ideal for low quality monitors.

Test Plan:
Review new colors in various scenarios.

{F2305178}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17141
2017-01-04 15:18:24 -08:00
epriestley
2855470b31 Show an info view warning for ongoing or failed builds in Differential
Summary:
Fixes T10136. This reinforces ongoing or failed builds in the comment action area.

We already emit a similar message for unit test failures from `arc unit`. This should probably obsolete that, eventually.

Test Plan:
{F2304809}

{F2304810}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10136

Differential Revision: https://secure.phabricator.com/D17140
2017-01-04 15:12:45 -08:00
epriestley
ef05bf335d Allow Harbormaster builds to publish to a different object
Summary:
Fixes T9276. Fixes T8650. The story so far:

  - We once published build updates to Revisions.
  - An unrelated fix (D10911) sent them to the Diffs instead of Revisions, which isn't useful, since you can't see a diff's timeline anywhere.
  - This also caused a race condition, where the RevisionEditor and DiffEditor would update the diff simultaneously (T8650).
  - The diff update was just disabled to avoid the race (part of D13441).
  - Instead, allow the updates to go somewhere else. In this case, we send commit updates to the commit but send diff updates to the revision so you can see 'em.
  - Since everything will be using the revision editor now, we should either get proper lock behavior for free or it should be easy to add if something whack is still happening.
  - Overall, this should pretty much put us back in working order like we were before D10911.

This behavior is undoubtedly refinable, but this should let us move forward.

Test Plan:
Saw a build failure in timeline:

{F2304575}

Reviewers: chad

Reviewed By: chad

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T9276, T8650

Differential Revision: https://secure.phabricator.com/D17139
2017-01-04 13:46:39 -08:00
epriestley
50de3071ac Define Differential email action in terms of EditEngine
Summary: Ref T11114. Move email/command actions, like "!reject", to modular transactions + editengine.

Test Plan: Used `bin/mail receive-test` to pipe "!stuff" to an object, saw appropraite effects in web UI.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17133
2017-01-02 13:25:45 -08:00
epriestley
35750b9c61 Make some Differential comment actions (like "Accept" and "Reject") conflict with one another
Summary:
Ref T11114. When a user selects "Accept", and then selects "Reject", remove the "Accept". It does not make sense to both accept and reject a revision.

For now, every one of the "actions" conflicts: accept, reject, resign, claim, close, commandeer, etc, etc. I couldn't come up with any combinations that it seems like users are reasonably likely to want to try, and we haven't received combo-action requests in the past that I can recall.

Test Plan:
  - Selected "Accept", then selected "Reject". One replaced the other.
  - Selected "Accept", then selected "Change Subscribers". Both co-existed happily.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17132
2017-01-02 13:25:12 -08:00
epriestley
65c1c758ed Use extended policies in Differential diffs
Summary:
Fixes T9648. Diffs currently use `return $this->getRevision()->getViewPolicy();` to inherit their revision's view policy.

After the introduction of object policies, this is wrong for policies like "Subscribers", because it means "Subscribers to this object, the diff". Since Diffs have no subscribers, this always fails.

Instead, use extended policies so that the object policy evaluates in the context of the correct object (the revision).

Test Plan:
  - Create a revision.
  - Subscribe `alice` to it.
  - Set view policy to "Subscribers".
  - View revision as `alice`.
  - Before patch: nonsense fatal about missing diff because of policy error.
  - After patch: `alice` can see the revision.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9648

Differential Revision: https://secure.phabricator.com/D17123
2017-01-01 09:56:30 -08:00
epriestley
81e2a1cf6b Always parse the first line of a commit message as a title
Summary: Fixes T10312. If your first line is "Reviewers: xyz", it's a title, not a "Reviewers" field.

Test Plan: Added unit test.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10312

Differential Revision: https://secure.phabricator.com/D17122
2017-01-01 09:56:15 -08:00
epriestley
ab17a7d4bf Be more lenient when accepting "Differential Revision" in the presence of custom ad-hoc commit message fields
Summary:
Fixes T8360. We will now parse revisions out of "Differential Revision: X" followed by other ad-hoc fields which we do not recognize. Previously, these fields would be treated as part of the value.

(In the general case, other fields may line wrap so we can't assume that fields are only one line long. However, we can make that assumption safely for this field.)

Also maybe fix whatever was going on in T9965 although that didn't really have a reproduction case.

Test Plan: Added unit tests.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8360

Differential Revision: https://secure.phabricator.com/D17121
2017-01-01 09:56:02 -08:00
epriestley
7bf49d254e Use a more conventional spelling of "CLOSED"
Summary: Ref T11114. Wow!

Test Plan: Spelling!

Reviewers: chad, eadler

Reviewed By: chad, eadler

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17125
2017-01-01 09:27:50 -08:00
epriestley
69194fdaf5 Make marking comments as "Done" work cleanly on EditEngine
Summary: Ref T11114. Fixes T10323.

Test Plan:
  - Marked comments as done only: no warning about not leaving a comment.
  - Did nothing: warning about posting an empty comment.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114, T10323

Differential Revision: https://secure.phabricator.com/D17120
2016-12-31 10:12:01 -08:00
epriestley
a4ba7daf90 Add transitional support for mail tags to Differential on EditEngine
Summary: Ref T11114. Although I plan to rewrite this system eventually (T10448) it's easy enough to punt for now.

Test Plan: punt

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17119
2016-12-31 10:11:45 -08:00
epriestley
b373dcef74 Restore some minor state behaviors to Differential on EditEngine
Summary:
Ref T11114. This restores:

  - Commandeering should exeucte Herald.
  - Commandeering should swap reviewers.
  - "Request Review" on an "Accepted" revision should downgrade reviewers so they have to accept again.

Test Plan:
  - Commandeered, saw Herald run and reviewers swap.
  - Requested review of an accepted revision, saw it drop down to "Needs Review" with "Accepted Prior" on the reviewer.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17118
2016-12-31 10:11:28 -08:00
epriestley
9b4090af55 Restore quote and warning behaviors to Differential EditEngine comment area
Summary: Ref T11114. This restores warnings (e.g., failing unit tests) and fixes "Quote" behavior for comments.

Test Plan:
  - Quoted a comment.
  - Viewed a warning.

{F2283275}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17117
2016-12-31 10:11:03 -08:00
epriestley
18249b097f Make inline comment preview and submission mostly work on EditEngine
Summary: Ref T11114. This comments nearly working on EditEngine. Only significant issue I caught is that the "View" link doesn't render properly because it depends on JS which is tricky to hook up. I'll clean that up in a future diff.

Test Plan: {F2279201}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17116
2016-12-31 10:10:29 -08:00
epriestley
f7b5955d33 Order actions sensibly within Differential revision comment action groups
Summary:
Ref T11114. See D17114 for some discussion.

For review actions: accept, reject, resign.

For revision actions, order is basically least-severe to most-severe action pairs: plan changes, request review, close, reopen, abandon, reclaim, commandeer.

Test Plan: Viewed revisions as an author and a reviewer, saw sensible action order within action groups.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17115
2016-12-31 10:10:05 -08:00
epriestley
48fcfeadaf Allow comment actions to be grouped; group Differential "Review" and "Revision" actions
Summary:
Ref T11114. Differential has more actions than it once did, and may have further actions in the future.

Make this dropdown a little easier to parse by grouping similar types of actions, like "Accept" and "Reject".

(The action order still needs to be tweaked a bit.)

Test Plan: {F2274526}

Reviewers: chad

Reviewed By: chad

Subscribers: eadler

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17114
2016-12-31 10:09:41 -08:00
epriestley
5a6643f36f Restore "Accept", "Reject" and "Resign" actions to Differential on EditEngine
Summary:
Ref T11114. Some rough edges, but this largely makes Accept, Reject and Resign work in the new EditEngine comment area.

Ref T11050. This lays a little bit of groundwork for having "resign" mean "I don't want to review this, even if projects or packages I'm a member of need to", not just "remove me personally as a user reviewer".

Test Plan: Accepted, rejected and resigned from revisions without any major state issues.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114, T11050

Differential Revision: https://secure.phabricator.com/D17113
2016-12-31 10:09:27 -08:00
epriestley
8b74cd481a Restore "Commandeer" action to Differential on EditEngine
Summary:
Ref T11114. This has two pieces of side-effect logic which I've noted locally:

  - Commandeer needs to apply Herald rules.
  - Commandeer needs to move the old author to become a reviewer and remove
    the actor as a reviewer.

Test Plan: Commandeered some revisions.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17111
2016-12-31 10:09:00 -08:00
epriestley
deb19b2d57 Restore "Plan Changes" and "Request Review" actions to Differential on EditEngine
Summary:
Ref T11114. This restores these actions.

One behavior is incomplete: "Request Review" on an accepted revision does not downgrade reviewers properly. I've noted this locally.

Test Plan: Planned changes and requested review of a revision.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17109
2016-12-31 10:08:05 -08:00
epriestley
a90ab7f403 Restore "Close" and "Reopen" actions to Differential on EditEngine
Summary:
Ref T11114. This restores these actions as selectable in the comment area.

This does not implement one special rule ("Closing a revision in response to a commit is OK from any status.") but I have a note about that separately.

Test Plan: Closed and reopened revisions.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17108
2016-12-31 10:07:27 -08:00
epriestley
3c5a17ba8a Restore "Reclaim" and "Abandon" actions to Differential on EditEngine
Summary: Ref T11114. This begins restoring comment actions to Differential, but on top of EditEngine.

Test Plan: {F2263148}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17107
2016-12-31 10:06:46 -08:00
epriestley
c05306d746 Move Differential to EditEngine comments
Summary:
Ref T11114. This is a transitional change that breaks a bunch of stuff. I'll hold it until I've restored features.

This stuff works:

  - Commenting.
  - Subscribers/tags/reviewers.
  - Pinning.
  - Drafts.

This stuff does not work yet:

  - Preview of inline comments.
  - Probably submitting inlines, whatsoever.
  - Comment-area warnings like "There are failing tests."
  - All meaningful actions (accept, reject, etc).

Test Plan: Commented on a revision. Essentially nothing else works yet.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17106
2016-12-31 10:06:15 -08:00
epriestley
4d8ac00602 Add missing "array" typehint to DifferentialCommitMessageParser
Summary: See D17058.

Test Plan: Ran `arc diff`, which parsed fields as a side effect.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17112
2016-12-29 09:22:13 -08:00
epriestley
28d74ae572 Rename Differenital "EditPro" controller back to "Edit"
Summary: Ref T11114. We seem to be in reasonable shape here and I don't think anything needs to revert, so rename this back to boring old "edit".

Test Plan: Created, updated, edited a revision via web UI.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17091
2016-12-16 13:17:12 -08:00
epriestley
895cdaca5d Simplify "Blame Revision" field in Differential
Summary: Ref T11114. This is still mostly in use, but toss a few commit message parsing things.

Test Plan: Viewed/edited/upated blame rev from CLI/web UI.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17089
2016-12-16 12:03:46 -08:00
epriestley
60f41b87e9 Simplify "Tasks" field in Differential
Summary:
Ref T11114. Keep UI, throw everything else away.

Includes an imperfect-but-not-too-awful fix to keep the field actually working.

Test Plan: Edited tasks from CLI.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17088
2016-12-16 10:26:34 -08:00
epriestley
f1f24e0360 Simplify "Repository" field in Differential
Summary: Ref T11114. Keep mail and UI, toss the rest.

Test Plan: Edited/viewed repositories.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17087
2016-12-16 10:25:38 -08:00
epriestley
18debbfdb4 Simplify Differential "Reviewers" field
Summary: Ref T11114. Keep rendering and mail, toss the rest.

Test Plan: Edited and viewed reviewers.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17086
2016-12-16 10:25:22 -08:00
epriestley
2ebbac86de Simplify Differential "Summary" field
Summary: Ref T11114. Keep UI stuff and mail stuff, toss editing.

Test Plan: Viewed and edited revision summaries.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17085
2016-12-16 10:24:39 -08:00
epriestley
c458f09dcc Simplify "Test Plan" custom field
Summary: Ref T11114. This leaves mail integration and UI integration, but strips all the editing (now handled by EditEngine) and commit message stuff (now handled by CommitMessageField).

Test Plan: Viewed and edited test plans and test plan transactions.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17084
2016-12-16 10:24:18 -08:00
epriestley
9e4c16c4c3 Remove Differential "Title" custom field
Summary: Ref T11114. Obsoleted by Modular Transactions + EditEngine + CommitMessageField + we just "hard code" the title of revisions into the page because we're craaazy.

Test Plan:
  - Made an edit on `stable`.
  - Viewed the edit on this change, it still had the proper UI strings.
  - Edited/created/updated revisions.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17083
2016-12-16 10:23:26 -08:00
epriestley
f552a20c61 Remove Differential "View Policy" field
Summary: Ref T11114. Obsoleted by EditEngine.

Test Plan: Edited the view policy of a revision.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17082
2016-12-16 10:23:05 -08:00
epriestley
84572a3b93 Remove Differential subscribers field
Summary: Ref T11114. This is obsoleted by `DifferentialSubscribersCommitMessageField` and EditEngine.

Test Plan: Edited a revision's subscribers.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17081
2016-12-16 10:22:48 -08:00
epriestley
3893b5f1a5 Remove "Revision ID" custom field
Summary: Ref T11114. Obsoleted by `DifferentialRevisionIDCommitMessageField`.

Test Plan:
- Grepped for removed class.
- Created a new revision, verified that the amended message included a proper revision ID.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17080
2016-12-16 10:22:28 -08:00
epriestley
77601bf58c Remove "Reviewed By" Differential field
Summary: Ref T11114. This is replaced by `DifferentialReviewedByCommitMessageField.php`.

Test Plan:
  - Used `differential.getcommitmessage` to query an accepted revision, saw "Reviewed By".
  - Grepped for removed class name.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17079
2016-12-16 10:21:40 -08:00
epriestley
5e606504b7 Remove "DifferentialProjectsField" custom field
Summary: Ref T11114. This is entirely obsoleted by EditEngine.

Test Plan: Edited projects on a revision.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17078
2016-12-16 10:21:26 -08:00
epriestley
8bba1eba85 Remove "DifferentialParentRevisionsField" custom field
Summary: Ref T11114. This was obsoleted by UI changes and hacked around for performance in T11404. It no longer does anything.

Test Plan: Grepped for removed class name.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17077
2016-12-16 10:21:09 -08:00
epriestley
c57c39f5d2 Remove "Next Step" Differential custom field
Summary: Ref T12027. This is purely a UI hint for new users that I'd like to integrate into "Land Revision" in the future instead.

Test Plan: Grepped for removed class, browsed Differential.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12027

Differential Revision: https://secure.phabricator.com/D17076
2016-12-16 10:20:35 -08:00
epriestley
5ea071f658 Remove "DifferentialGitSVNIDField" custom field in Differential
Summary: Ref T11114. This is obsolted by the narrower `DifferentialGitSVNIDCommitMessageField`.

Test Plan: Browsed around.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17075
2016-12-16 10:11:52 -08:00
epriestley
4df072cca6 Remove "DifferentialEditPolicyField" custom field
Summary: Ref T11114. This is now entirely handled by EditEngine and standard policy code.

Test Plan: Edited the edit policy of a revision.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17074
2016-12-16 10:11:18 -08:00
epriestley
bc6522dbca Remove "DifferentialConflictsField" custom field
Summary: Ref T11114. This is a pure paring field and now entirely handled by `DifferentialConflictsCommitMessageField`.

Test Plan: Grepped for removed class name.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17073
2016-12-16 10:10:45 -08:00
epriestley
93c0ffd02c Remove "Child Revisions" custom field in Differential
Summary: Ref T11114. This was obsoleted by the "Stack" graph and does nothing.

Test Plan: Viewed revisions, still saw dependency graphs.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17072
2016-12-16 10:10:13 -08:00
epriestley
74a0caf9ce Remove "Author" CustomField in Differential
Summary: Ref T11114. This hasn't done anything since we moved author information to the subheader.

Test Plan: Browsed Differential, still saw author information.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17071
2016-12-16 10:09:48 -08:00
epriestley
914d9fa8b9 Simplify Auditors custom field in Differential
Summary: Ref T11114. This field just stores the value of "Auditors" so you can trigger auditors explicitly later on if you want.

Test Plan: Created and edited revisions with "Auditors".

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17070
2016-12-16 10:09:30 -08:00
epriestley
d12856b5d4 Remove "Apply Patch" UI field from Differential
Summary: Ref T12026. This simplifies the UI and makes T11114 easier. I plan to integrate this into "Download Raw Diff" in the future.

Test Plan:
  - Browsed revisions.
  - Grepped for removed class name.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12026

Differential Revision: https://secure.phabricator.com/D17069
2016-12-16 10:09:15 -08:00
epriestley
a74d602b3c Make stored custom fields work with v3 EditEngine API
Summary: Ref T11114. This makes the unusual stored custom fields ("Blame Rev", "Revert Plan", etc) work somewhat correctly (?) with EditEngine.

Test Plan:
  - Created, updated and edited revisions with unusual stored custom fields like "Blame Rev".
  - Observed that these fields now populate in "differential.revision.edit" when available.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17068
2016-12-16 10:09:03 -08:00
epriestley
64509dcca7 Drive CLI-based revision edits through "differential.revision.edit" API + EditEngine
Summary:
Ref T11114. This creates `differential.revision.edit` (a modern, v3 API method) and redefines the existing methods in terms of it.

Both `differential.createrevision` and `differential.updaterevision` are now internally implemented by building a `differential.revision.edit` API call and then executing it.

I //think// this covers everything except custom fields, which need some tweaking to work with EditEngine. I'll clean that up in the next change.

Test Plan:
  - Created, updated, and edited revisions via `arc`.
  - Called APIs manually via test console.
  - Stored custom fields ("Blame Rev", "Revert Plan") aren't exposed yet.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17067
2016-12-16 10:08:49 -08:00
epriestley
24926f9453 Move Differential commit message rendering to dedicated classes
Summary:
Ref T11114. This probably still has some bugs, but survives basic sanity checks.

Continue pulling commit message logic out of CustomField so we can reduce the amount of responsibility/bloat in the classtree and send more code through EditEngine.

Test Plan:
  - Called `differential.getcommitmessage` via API console for various revisions/parameters (edit and create mode, with and without fields, with and without revisions).
  - Used `--create`, `--edit` and `--update` modes of `arc diff` from the CLI.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17066
2016-12-16 10:08:34 -08:00
epriestley
de4d7e1b10 Support arbitrarily long filenames in Differential
Summary:
Fixes T11660. Currently, if you try to diff a path with more than 255 total characters, we fail to create the diff because we have a `text255` column.

There are actually two issues here:

  - File names may be arbitrarily long (T11660).
  - File names may not be UTF8 (T6633, etc). This is much more complicated and has other issues -- largely that we can't JSON-encode non-UTF8 filenames. I'm punting on that for now and will deal with it later. This doesn't specifically address non-UTF8 paths, although it is a change that's (probably?) required to eventually support them.

This will cause some potentially slow migrations, but better to do them now, if possible, so we have fewer complicated/slow upgrades overall.

Test Plan:
Created a change touching file: //very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_directory_name/very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_long_filename.txt//

{F2137737}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11660

Differential Revision: https://secure.phabricator.com/D17062
2016-12-15 11:35:15 -08:00
epriestley
89d88dafcc Fix a Differential exception in invalid/missing fields
Summary: Ref T11114. Missed this while converting.

Test Plan: Tried to create a revision with no test plan. Before: fatal; after: helpful message.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17061
2016-12-15 11:34:54 -08:00
epriestley
8476ad1a28 Separate all commit message field parsing out of Differential custom fields
Summary:
Ref T11114. See that task for some discussion.

Overall, Differential custom fields ended up with too many responsibilities. Later work in EditEngine provides a more promising model for achieving modularity with smaller, more consistent components.

In particular, we have some custom fields like `DifferentialGitSVNIDField` and `DifferentialConflictsField` which serve //only// to support the field parser.

This starts pulling commit message responsibilities out of the core list of custom fields and into simpler dedicated parsers.

Test Plan: Created and edited revisions from the CLI. Added a bit of test coverage.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17058
2016-12-14 18:44:14 -08:00
epriestley
552c546689 Separate commit message parsing and validation from Conduit
Summary:
Ref T11114. I want to move this step away from custom fields. To start with, isolate all the parsing in one class with a clearer API boundary.

Next, I'll make this class use new field objects to perform parsing, without CustomField interactions.

Test Plan: Created and edited revisions from the CLI, using valid and invalid commit messages.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17055
2016-12-14 14:14:47 -08:00
epriestley
378387a078 Fix an issue with mentioning revisions on the new EditEngine code
Summary:
Ref T12020. Ref T11114. If we continue here on a mention, we try to generate `$old`, which requires reviewers to be attached. They won't be for simple codepaths like mentions.

Instead, just bail early: we don't need to do anything anyway since we can't possibly find any more errors with zero transactions.

Test Plan: Mentioned a revision on a task.

Reviewers: chad, avivey

Reviewed By: avivey

Maniphest Tasks: T11114, T12020

Differential Revision: https://secure.phabricator.com/D17059
2016-12-14 14:11:10 -08:00
epriestley
102ea3cfa4 Replace Differential Edit controller with EditEngine-driven EditPro controller
Summary:
Ref T11114. This replaces the old edit controller with a new one based entirely on EditEngine.

This removes the CustomFieldEditEngineExtension hack for Differential, since remaining field types are fairly straightforward and work with existing EditEngine support, as far as I can tell.

Test Plan:
  - Created a revision via web diffs.
  - Updated a revision via web diffs.
  - Edited a revision via web.
  - Edited nonstandard custom fields ("Blame Revision", "JIRA Issues").
  - Created a revision via CLI.
  - Updated a revision via CLI.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17054
2016-12-14 07:27:39 -08:00
epriestley
32ce21a181 Allow the new Differential EditEngine form to create/update diffs for revisions
Summary: Ref T11114. Much of this is around making the "comment-while-updating" flow work correctly.

Test Plan:
  - Created new diffs by copy/pasting, then:
    - used one to create a new revision;
    - used one to update an existing revision, with a comment.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17053
2016-12-14 07:27:25 -08:00
epriestley
7f99f2cde8 Add EditEngine + Modular Transactions for reviewers
Summary: Ref T11114. This one is a bit more complex, but I think I covered everything.

Test Plan:
  - Added reviewers.
  - Removed reviewers.
  - Made reviewers blocking.
  - Made reviewers nonblocking.
  - Tried to make the author a reviewer.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17050
2016-12-13 18:20:58 -08:00
epriestley
6c9af81f7a Support "Test Plan" with modular transactions and EditEngine
Summary: Ref T11114. The only real trick here is that we respect configuration in `differential.fields`.

Test Plan: Turned plan on and off, tried to remove the plan, edited the plan.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17048
2016-12-13 18:20:16 -08:00
epriestley
5349d6bd5c Add Summary and Repository EditEngine fields + Modular Transactions to Differential
Summary: Ref T11114. These are unambiguous and always-enabled.

Test Plan: {F2117777}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17047
2016-12-13 18:18:32 -08:00
epriestley
0906bf547b Begin adding "pro" modular transaction fields to Differential
Summary:
Ref T11114. Currently, all of Differential is extremely custom CustomFields. I want to back away from that somewhat and leverage more EditEngine / ModularTransactions infrastructure.

This allows EditEngine, ModularTransactions, and CustomFields to coexist in an uneasy peace. The "EditPro" controller applies a //different edit// than the CustomFields do, but everything works out in the end. I think.

Hopefully the horrible mess I am creating here will be short-lived.

Test Plan:
  - Edited a revision with the normal editor.
  - Edited a revision with the pro editor.
  - Created a revision with `arc diff`.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17044
2016-12-13 14:50:31 -08:00
epriestley
eda64b8549 Add a very basic EditPro controller for Differential
Summary: Ref T11114. This doesn't really support anything yet, but technically works if you manually go to `/editpro/`.

Test Plan: {F2117302}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17043
2016-12-13 14:36:06 -08:00
epriestley
77fa1ea738 Rename "DifferentialReviewer" to "DifferentialReviewerProxy"
Summary: Ref T10967. This makes room for a `DifferentialReviewer` object which can be a real storage table.

Test Plan: Grepped for `DifferentialReviewer`, browsed Differential.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17041
2016-12-13 14:35:35 -08:00
epriestley
1e9a462baa Remove most of the legacy hunk code
Summary: Ref T8475. This gets rid of most of the old "legacy hunk" code. I'll nuke the rest (and drop the old table) once we're more sure that we're in the clear.

Test Plan: Browsed Differential.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8475

Differential Revision: https://secure.phabricator.com/D17040
2016-12-13 14:34:36 -08:00
epriestley
fc6bfbdb10 Truncate the one-line diff update summary when updating a revision to 250 bytes
Summary:
Fixes T7899. If you create or update a revision and type an enormously long first line, we currently fatal trying to insert it into the database.

This text is only used to show a single-line summary of the diff in the "History" tab, which should probably be updated anyway. For now, stop fataling.

Test Plan:
Uploaded a diff with the description "MMMM..." (thousands of them).

Before patch: fatal on description being too long.
After patch: beautiful "MMMM" summary.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7899

Differential Revision: https://secure.phabricator.com/D17038
2016-12-13 14:28:24 -08:00
Chad Little
dece7af50b Prettier file embeds
Summary: Spruce up the file embeds a little more, hover state, icons, file size.

Test Plan:
Add a psd and pdf, see new icons. Check differential, still see icons there too. Test mobile, desktop.

{F2042539}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16950
2016-11-27 14:57:06 -08:00
epriestley
e713cc08e6 Make Calendar query for indirect invites/RSVPs by default, like Differential
Summary:
Ref T11816. Since the dashboard got updated, Differential now interprets "Responsible Users: epriestley" to mean "epriestley, or any project or package epriestley is part of". You can query for just "epriestley" with "exact(epriestley)".

Give Calendar invites the same behavior: "epriestley" means "any event epriestley is invited to, or a project they are a member of is invited to". Individual invites can be queried with "exact(epriestley)".

This is a little bit copy-pastey but I want to wait for a third use case to clean it up since I think I'm going to have to do a bunch of generalization around "how does an individual PHID get turned into a bunch of PHIDs".

Test Plan: Queried for "Invited: dog", "invited: viewer", "invited; exact(dog)", etc.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11816

Differential Revision: https://secure.phabricator.com/D16870
2016-11-15 12:47:01 -08:00
epriestley
745429aac1 Fix some slop with Differential field lists
Summary: I moved and then un-moved this incorrectly in D16846.

Test Plan: Looked at the old code, which worked better.

Reviewers: jacksongabbard, chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D16849
2016-11-12 16:27:03 -08:00
epriestley
6dfc7e48ae Don't let users write summaries or test plans which will become ambiguous in commit messages
Summary:
Ref T11085. To recreate the issue:

  - From the web UI, click "Edit Revision".
  - Write something like this as your "Summary" (i.e., put another field marker, like "Test Plan:", into the summary):

> This is a test of the
> Test Plan: field to see
> if it works.

  - Save changes.

Later, when the summary is amended into a commit message, the parser will see two "Test Plan:" fields and fail to parse the message.

Instead, prevent users from making this edit.

Test Plan: {F1917640}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11085

Differential Revision: https://secure.phabricator.com/D16846
2016-11-12 08:03:46 -08:00